Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make getTypeOfSymbol, isArrayType, isTupleType public on TypeChecker #52467

Merged
merged 3 commits into from
Feb 2, 2023

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jan 27, 2023

Fixes #37711

These are APIs which are used by typescript-eslint and appear to be generally useful: https://github.com/typescript-eslint/typescript-eslint/blob/09c04fb0d5a5dee0dc266abb2e21aa9a1c489712/packages/type-utils/typings/typescript.d.ts

By exposing getTypeOfSymbol, we unlock a whole slew of uses. For example, the internal function getTypeOfPropertyOfType can be written externally using already public functions:

function getTypeOfPropertyOfType(checker: TypeChecker, type: Type, name: string) {
    const prop = getPropertyOfType(type, escapeLeadingUnderscores(name));
    return prop ? getTypeOfSymbol(prop) : undefined;
}

Draft for now because isArrayType and isTupleType need comments describing their caveats, and/or we should export versions which handle subtypes of these. (Maybe we should export isArrayLikeType and invent a isTupleLikeType instead?)

/cc @JoshuaKGoldberg @bradzacher

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 27, 2023
@JoshuaKGoldberg
Copy link
Contributor

Yes this is great, thank you for looking at exposing these! We feel just as bad about using these @internal APIs as we should (a lot).

...can you do isTypeAssignableTo next? 😜

/** @internal */ isArrayType(type: Type): boolean;
/** @internal */ isTupleType(type: Type): boolean;
isArrayType(type: Type): boolean;
isTupleType(type: Type): boolean;
/** @internal */ isArrayLikeType(type: Type): boolean;
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Maybe we should export isArrayLikeType and invent a isTupleLikeType instead?)

I'm not sure we have a need need for an isTupleLikeType on our end. We have very little logic that deals with tuples [Sourcegraph query for isTupleType in typescript-eslint]. Do you see a reason why we'd want that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main "problem" with these functions (that we have to document in this PR) is that they don't return true for things that extend from Array/tuple, e.g. TS's own NodeArray probably won't return true even though it is an Array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that doesn't happen for tuples, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much code actively extends tuple though? Is it a pattern people even really use?
Seems like it would be a suuuper niche usecase to do type T = [1,2,3] & SomeType (I've never seen it!).

Tuples themselves are already pretty niche, so it's going to be so rare that people do the above, probably.

As josh said - we barely touch on tuples in our rules, and even in the broader OSS ecosystem there's only 70 usages in sourcegraph..

Probably not worth it TBH, considering you'd need to build it!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean specifically like:

type SomeTuple = [..., ..., ...]

interface SomeInterface extends SomeTuple {
    someExtraInfo: ...;
}

Which is analogous to our NodeArray type for a fixed size array.

But yes, it's not a blocker for this PR, I just was hoping to have it because it rounds out isArrayType+isArrayLikeType with isTupleType+isTupleLikeType.

@jakebailey
Copy link
Member Author

...can you do isTypeAssignableTo next? 😜

That is of course #9879 which I personally think we should address (but is it of course larger than these pretty-safe ones).

@jakebailey jakebailey marked this pull request as ready for review February 1, 2023 20:18
@jakebailey
Copy link
Member Author

I have added descriptions for these extra functions (besides getTypeOfSymbol which I don't think needs one); this should be ready for review.

I have opted to not also export isTupleLikeType; this function appears to misbehave when given a type which extends an empty tuple but fixing that breaks (changes?) things in mysterious ways which I don't want to handle in this PR.

Copy link
Contributor

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leoclap

I love this and I greatly appreciate you've clarified the caveats for the methods. In the past the explanations have often been just "they're not safe for all cases" - so the explicit documentation goes a long way to making the checker API more accessible to those that don't know the checker internals.

also doing some searching: fixes #37711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose isArrayType, isTupleType, isArrayLikeType in TypeChecker
5 participants