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 isTypeAssignableTo public on TypeChecker #56448

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Nov 17, 2023

For #9879

See:

This function has been exported on the checker since TypeScript 3.7. It's meaning has not changed in that time, and many people have begun depending on it.

ts-eslint doesn't use it as it's internal (as they're trying to do "the right thing"), but using it would fix many many lint bugs, and wouldn't require waiting for 5.4 (as stated above, it's been around since 3.7 and has seemingly worked the same).

We've previously been unhappy with exporting relation APIs because (1) we're free to add and remove relations at any time, (2) we're free to change relations, and (3) it's sometimes hard to document what they are.

But, assignability is assignability; it's unlikely that this relation is going to go away, and if we change its meaning, it's probably the case that dependents want that behavior change too to match the language. That and its pervasiveness has made it such that we probably can't remove it anyhow (as they say, "load-bearing").

So, this PR just exports it. This does not fully cover #9879, of course, but it seems worse to not do anything when we could just export this one well-defined function so many are already using unsafely.

cc @JoshuaKGoldberg

@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

@@ -5158,7 +5158,7 @@ export interface TypeChecker {
/** @internal */ getPromiseLikeType(): Type;
/** @internal */ getAsyncIterableType(): Type | undefined;

/** @internal */ isTypeAssignableTo(source: Type, target: Type): boolean;
isTypeAssignableTo(source: Type, target: Type): boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs a doc; please leave suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking on what I'd want as a user, it's mostly just knowing what kind of caching I'd have to worry about (since that was brought up by TS folks previously) and examples of what direction the assignability runs in. Vaguely:

/**
 * Performs an assignability check, including using persistent caches.
 * @returns Whether `source` is assignable to `type`.
 * @example
 * ```ts
 * declare const literal: ts.Type; // Type of "abc"
 * declare const primitive: ts.Type; // Type of string
 * isTypeAssignableTo(literal, literal); // true
 * isTypeAssignableTo(literal, primitive); // true
 * isTypeAssignableTo(primitive, literal); // false
 * isTypeAssignableTo(primitive, primitive); // true
 * ```

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added something similar to the above.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

YES! Thank you very much for getting this started!

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.

One thing that might be important is the ability to resolve a type in the context of a file. For example most of our "promise checking" rules currently do really funky checks on the type to see if it has specific signatures.

The ability to do like "grab the global promise type and check if thing is assignable to that type" would vastly simplify our logic.

Similar for like Function (which I believe we shim right now via "has at least one call signature").

Error is another case we've used a few times where we do some shonky thing like "keep checking the inheritance chain until we find the last parent, ensure that parent is named error and it is in the TS lib file"

@jakebailey
Copy link
Member Author

My impression from the recent thread was that this function was all that was needed to make everyone happy; is that incorrect such that we need to discuss this more?

@bradzacher
Copy link
Contributor

This will definitely do for a huge chunk of new cases!
There's also our existing rules which... Approximate the API in some cases by manually pulling apart types and applying some heuristic.

If there's ways to fetch "common" types or construct new types based on common types then we could vastly simplify some existing rules.

@jakebailey
Copy link
Member Author

#52473 added quite a few of these, but I am curious what else you're looking for.

@bradzacher
Copy link
Contributor

The only thing missing would be the ability to get a type from the globals, I think. Or less generally - some of the more "spec standard" types like Promise.
Really getPromiseType would be the biggest one for us and would help us out given how many promise rules we have.

As an example the no-floating-promise rule and no-misused-promises rule rules have their own logic (some of which is based off ts-api-utils.isThenable) to vaguely determine if a type is a promise. They are mostly correct - but it's a few hundred lines of code that could generally be reduced to checker.isTypeAssignableTo(type, anyPromise).

@jakebailey
Copy link
Member Author

jakebailey commented Nov 18, 2023

This isn't a public method (yet), but theoretically you could use getAwaitedType; if it returns something different than the input, then the input has a promise.

@fatcerberus
Copy link

But, assignability is assignability; it's unlikely that this relation is going to go away

Given that assignability is the basic primitive the type system is built on (conditional types check assignability, generic constraints are based on assignability, A & B means the set of values assignable to both, heck even parameter contravariance is just assignability with the polarity reversed!), I'd be very surprised if it went away without, you know, rearchitecting the whole language from scratch. Which I guess is what is meant by "load-bearing"

@jakebailey
Copy link
Member Author

Which I guess is what is meant by "load-bearing"

In this instance, I meant that we couldn't remove the function without breaking the ecosystem, even though it's not public.

@nickserv
Copy link
Contributor

nickserv commented Nov 18, 2023

@jakebailey The GitHub search in your original comment has non-JavaScript files, try this:

https://github.com/search?q=%2F%5C.isTypeAssignableTo%2F+%28language%3AJavaScript+OR+language%3ATypeScript%29&type=code

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.

think-amazing

@jakebailey
Copy link
Member Author

jakebailey commented Nov 18, 2023

This isn't a public method (yet), but theoretically you could use getAwaitedType; if it returns something different than the input, then the input has a promise.

I have not tried to stick this into your codebase, but testing on ts-ast-viewer.com shows that it seems to work:

> function containsPromise(type) { return type !== checker.getAwaitedType(type) }
< undefined
> [checker.typeToString(type), containsPromise(type)]
< (2) ['number', false]
> [checker.typeToString(type), containsPromise(type)]
< (2) ['number | undefined', false]
> [checker.typeToString(type), containsPromise(type)]
< (2) ['Promise<number>', true]
> [checker.typeToString(type), containsPromise(type)]
< (2) ['Promise<number> | Promise<string> | Promise<boolea…| string[]> | Promise<bigint | Promise<number[]>>', true]

A single equality check is almost surely better than 100 lines of code!

I actually would have thought people would have already used getAwaitedType externally, but apparently not. I only see it used in our codefixes. I know a few people have asked for that one already on the Discord.

EDIT: this is not totally accurate; for non-promise Thenables, this method returns undefined, so that's another special case.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

One thing: it still might need some discussion of caching.

@jakebailey
Copy link
Member Author

One thing: it still might need some discussion of caching.

My general feeling is that because we don't provide any mechanism to create new types (only those derived from actual code), that the cache sizes would be bounded.

@sandersn
Copy link
Member

Were there specific concerns we brought up in discord? I don't remember.

@jakebailey
Copy link
Member Author

I don't recall any, besides what I mentioned in the thread here (then went back on once I realized that you couldn't just make random types and overflow the cache).

@sandersn
Copy link
Member

:shipit:

@jakebailey
Copy link
Member Author

Alrighty, if there are no more objections, I'm going to merge this one in.

@jakebailey jakebailey merged commit 14da488 into microsoft:main Dec 8, 2023
19 checks passed
@jakebailey jakebailey deleted the export-isTypeAssignableTo branch December 8, 2023 21:16
@JoshuaKGoldberg
Copy link
Contributor

Rainn Wilson as Dwight Schrute from The Office tearfully saying 'thank you', happily

@@ -5158,7 +5158,20 @@ export interface TypeChecker {
/** @internal */ getPromiseLikeType(): Type;
/** @internal */ getAsyncIterableType(): Type | undefined;

/** @internal */ isTypeAssignableTo(source: Type, target: Type): boolean;
/**
* Returns true if the "source" type is assignable to the "target" type.

Choose a reason for hiding this comment

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

Suggested change
* Returns true if the "source" type is assignable to the "target" type.
* Returns true if the `source` type is assignable to the `target` type.

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
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants