Revert fix for intersections in template literals, fix differently #52836
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@typescript-bot test this |
typescript-bot
added
the
For Uncommitted Bug
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at bb599af. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at bb599af. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at bb599af. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at bb599af. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at bb599af. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at bb599af. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at bb599af. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at bb599af. You can monitor the build here. Update: The results are in! |
This comment was marked as outdated.
This comment was marked as outdated.
typescript-bot
added
For Milestone Bug
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52836
System
Hosts
Scenarios
TSServerComparison Report - main..52836
System
Hosts
Scenarios
StartupComparison Report - main..52836
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
This comment was marked as outdated.
This comment was marked as outdated.
weswigham
requested changes
Mar 14, 2023
|
@weswigham Do you have any commentary on this in general? I'm doubting my choice here; it does allow you to write template types that contain intersections, which, you can only use by creating more template literals (as in my Path example). However, I'm somewhat wondering if I should instead be treating these like unions, i.e. expand them out at the top where if you have say |
weswigham
reviewed
Mar 16, 2023
There was a problem hiding this comment.
Do you have any commentary on this in general? I'm doubting my choice here; it does allow you to write template types that contain intersections, which, you can only use by creating more template literals (as in my Path example).
Hm, I think only being able to interact with branded holes with other branded types is kinda the point of branding your literal types. Simultaneously, today you can use a template literal to "unbrand" a branded type (I don't know how prevalent that is, and, obviously if you look at the prompting issue, it isn't desirable to some). I'm not crushed by replacing the "unbranding" behavior with respecting brands, though, since you can also "unbrand" a branded literal type with something like
type Unbrand<T, TBrand> = T extends infer Q & TBrand ? Q : never;
type Q = Unbrand<"a" & {ok: any}, {ok: any}>;which is, IMO, better, since you have to specify what brands you're removing from the type.
However, I'm somewhat wondering if I should instead be treating these like unions, i.e. expand them out at the top where if you have say
a${A & B}b, that it turns intoa${A}b&a${B}b.
No, I don't think that's right - intersected literals always become never, so the only meaningful intersections are branded literals. So if you split the brand and the literal apart, you'd have an object alone inside a template hole, like making a/${"b" & {path:any}}/c into a/b/c & a/${{path:any}}/c, and that later type has basically no meaning (in fact, by current rules, it just makes the type into string, which'll then evaporate entirely) - it's only useful when you consider it with what it's intersected with anyway.
|
Thanks for the feedback! I've made the changes. |
weswigham
approved these changes
Mar 19, 2023
typescript-bot
added
For Uncommitted Bug
|
This breaks fragmented-store on DT. I'll get more details and file a bug. |
|
Hm, I could have sworn I ran dt. |
|
(But, that is why I decided to merge yesterday afternoon; so I could get this feedback sooner rather than later.) |
This was referenced Mar 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For #52345
This reverts #48044, which with nearly a year more experience no longer looks correct to my eyes (I regret merging it), and instead implements a different fix for the issue. This one instead avoids expanding out intersections, leaving them in place like the other non-string-y types.