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

Ignore template literal types which contain intersections in removeStringLiteralsMatchedByTemplateLiterals #53406

Closed
wants to merge 2 commits into from

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Mar 21, 2023

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:

image

After:

image

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.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 21, 2023
@jakebailey
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 21, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at fbc0ccc. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 21, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/150405/artifacts?artifactName=tgz&fileId=18A3F24D60E2EA4BD9511817F01BF78FD2838F7D01AC669A8E718BCBA0B9F1AF02&fileName=/typescript-5.1.0-insiders.20230321.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.1.0-pr-53406-2".;

@jakebailey
Copy link
Member Author

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 21, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 21, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 21, 2023

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!

@typescript-bot
Copy link
Collaborator

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.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/53406/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/53406/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot typescript-bot added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Mar 21, 2023
Copy link
Member

@weswigham weswigham left a 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.

@jakebailey
Copy link
Member Author

jakebailey commented Mar 21, 2023

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.)

@weswigham
Copy link
Member

Yes, I believe so. Though the example I gave was broken (albeit differently) in 5.0.2, too.

@jakebailey jakebailey marked this pull request as draft March 21, 2023 19:14
@jakebailey
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 21, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at fa22d3a. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 21, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/150548/artifacts?artifactName=tgz&fileId=67D5594E7973670EA5E02B25D4E8AF14651FAAD590C1B4800CD726DC16E0575D02&fileName=/typescript-5.1.0-insiders.20230321.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.1.0-pr-53406-15".;

@jakebailey jakebailey closed this Aug 22, 2023
@jakebailey jakebailey deleted the fix-52345-3 branch October 12, 2023 02:52
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.

3 participants