Reduce template literal types with a single placeholder and no extra texts #55371
Conversation
typescript-bot
added
the
For Uncommitted Bug
…le placeholder and no extra texts
Andarist
force-pushed
the
fix/template-literal-type-with-no-texts-relations
branch
from
b6b579b to
12dbb98
Compare
…type-with-no-texts-relations
typescript-bot
added
For Backlog Bug
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at d6fe15f. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at d6fe15f. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at d6fe15f. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at d6fe15f. You can monitor the build here. Update: The results are in! |
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@gabritto Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
|
@typescript-bot pack this |
|
Hey @gabritto, 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 |
|
Hey @gabritto, the results of running the DT tests are ready. |
There was a problem hiding this comment.
While this might be a true observation - ${T} is identical to T when T is only string-like - this isn't how we should implement it, imo. Rather than messing with type relations, this should be true on construction, eg, inside getTemplateLiteralType we should check for and produce this simplification, just so the simplifiable type identity never even exists inside the checker. A "late" simplification like this we should generally only do if it's too costly/circularity inducing to check upfront, but I don't think that'll be the case here (and, broadly speaking, could be done in getSimplifiedType rather than as a new relationship rule).
As an aside that won't really matter once that's done, since I do think we can do that, there's also an inference issue here, which is probably the better, broader fix for the original problem (which can probably be rewritten to have non-string constraints). When we see ${T} and infer to ${U}, the texts all match - ideally I'd think we want to infer T to U as a better match, no template wrapper required.
…type-with-no-texts-relations
Andarist
force-pushed
the
fix/template-literal-type-with-no-texts-relations
branch
from
a3d3a15 to
aac8961
Compare
Andarist
changed the title
|
@typescript-bot run DT |
|
Heya @gabritto, I've started to run the regular perf test suite on this PR at aac8961. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at aac8961. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at aac8961. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at aac8961. You can monitor the build here. Update: The results are in! |
|
Hey @gabritto, 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 |
src/compiler/checker.ts
Outdated
| texts.length === 2 && texts[0] === "" && texts[1] === "" | ||
| // literals (including string enums) are stringified below | ||
| && !(types[0].flags & TypeFlags.Literal) | ||
| // infer T extends StringLike can't be unwrapped eagerly |
There was a problem hiding this comment.
Do you have an example of when this matters? I.e. an example of why we shouldn't unwrapp infer T?
There was a problem hiding this comment.
it's mainly because of how string enums might be "stringified" by template literal types
const enum StringLiteralEnum { Zero = "0", True = "true", False = "false", Undefined = "undefined", Null = "null" }
type TStringLiteralEnum0 = "0" extends `${infer T extends StringLiteralEnum}` ? T : never;
// ^? type TStringLiteralEnum0 = StringLiteralEnum.Zero
type TStringLiteralEnum0Unwrapped = "0" extends infer T extends StringLiteralEnum ? T : never;
// ^? type TStringLiteralEnum0Unwrapped = never
|
@gabritto Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@Andarist do you intend to fix the inference problem as well? (see #56659 (comment)). |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @gabritto, the results of running the DT tests are ready. |
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
I can investigate this
Users might also construct such types and it makes sense to allow those: function f3<T3 extends "a" | "b">(x: T3) {
const test1: `${T3}` = x
const test2: T3 = "" as `${T3}`;
}To allow this we need to either do this simplification or mess with the relationship checking (what I had here before). |
Right, I remember now. I think you mentioned that before but I had forgotten, sorry. |
|
@Andarist can you update this with latest main so I can merge? |
…type-with-no-texts-relations
|
@gabritto with pleasure! done :) |
|
This PR is effectively a breaking change as demonstrated in #58687. I'm not sure I like it. |
|
Note that this PR started as a rule in type relationships (but perhaps it wasnt working for some enum-related cases correctly either), it was requested to make it unwrap those eagerly instead |
|
First off, the real fix for #56659 appears to be #57808 which was merged a few days after this PR. When I back out this PR, the repro in #56659 still checks with no errors. That leaves #55364, which with this PR backed out exhibits the following behavior: interface TypeMap {
a: 'A'
b: 'B'
}
declare const f: <T extends 'a' | 'b'>(x: `${T}`) => TypeMap[T];
type F1 = <T extends 'a' | 'b'>(x: `${T}`) => TypeMap[T];
const f1: F1 = f; // Ok
type F2 = <T extends 'a' | 'b'>(x: `${T}`) => TypeMap[`${T}`];
const f2: F2 = f; // Error, T is not assignable to `${T}`In my opinion that's working as intended. It is true that for a subset of types All up, my recommendation is that we revert this PR. |
|
I'll revert this. |
|
@gabritto i have proposed fix in the works, i should be able to post it soon |
fixes #55364
fixes #56659