-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Ignore template literal types which contain intersections in removeStringLiteralsMatchedByTemplateLiterals #53406
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
Conversation
…ringLiteralsMatchedByTemplateLiterals
@typescript-bot pack this |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at fbc0ccc. You can monitor the build here. |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@typescript-bot test this |
Heya @jakebailey, I'm starting to run the diff-based user code test suite on this PR at fbc0ccc. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
Heya @jakebailey, I'm starting to run the diff-based top-repos suite on this PR at fbc0ccc. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
Heya @jakebailey, I'm starting to run the parallelized Definitely Typed test suite on this PR at fbc0ccc. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
Heya @jakebailey, I'm starting to run the extended test suite on this PR at fbc0ccc. Hold tight - I'll update this comment with the log link once the build has been queued. |
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
Hey @jakebailey, the results of running the DT tests are ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eeeeeyes and no. I think I just found a bug in main
thinking about this (though there was also a different bug in 5.0.2). Consider:
type MaybeCat = `c${string}` & `${string}t`;
type MaybeScats = `s${MaybeCat}s`;
declare var x: MaybeScats;
x = "scats"; // should be OK
x = "scas"; // should error
On 5.0.2, the first assignment works, but MaybeScats
simplifies to sc${string}s
- the t
requirement is silently dropped, which isn't right - the second assignment doesn't error when it should. Right now in the nightly, we've changed that - it more correctly stays as s${MaybeCat}s
now (since the intersection is left well enough alone), but we're not chopping the string up correctly to handle comparison to the nested intersection of template literals, and both assignments are errors - as-is this would exacerbate that.
While in the above example, you could see us inlining the intersection to produce sc${string}ts
, a more complex intersection like a${string}b${string}c & ${string}-${string}-${string}
doesn't have an obvious non-intersected form that I can see, so I don't think we can use just intersections as an indicator that we can skip comparing against it. Intersections containing object/anonymous types (or generics constrained only to such), however, would seem to be good.
Hm, so there's a bug to report on main, then, as a result of my other PR? (At least thankfully the other PR, #53413, saves the bulk of the time on this issue, just not all.) |
Yes, I believe so. Though the example I gave was broken (albeit differently) in 5.0.2, too. |
@typescript-bot pack this |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at fa22d3a. You can monitor the build here. |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
For #52345
There's no reason to waste time checking template literal types which contain intersections in
removeStringLiteralsMatchedByTemplateLiterals
; the only thing that these can match are other template literals with intersections too.e.g.,
"foo"
will not match`fo${"o" & { _brand: any}}`
.Before:
After:
Maybe there's still something to do to speed up
removeSubtypes
for this case; will have to look harder. But, I want feedback on this PR first.