Allow nongeneric string mapping types to exist #47050
Conversation
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 361eaff. You can monitor the build here. |
|
Hey @weswigham, 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
| @@ -19497,6 +19510,13 @@ namespace ts { | |||
| return Ternary.True; | |||
| } | |||
| } | |||
| else if (target.flags & TypeFlags.StringMapping) { | |||
src/compiler/checker.ts
Outdated
| @@ -22060,7 +22087,8 @@ namespace ts { | |||
| const value = (source as StringLiteralType).value; | |||
| return !!(target.flags & TypeFlags.Number && value !== "" && isFinite(+value) || | |||
| target.flags & TypeFlags.BigInt && value !== "" && isValidBigIntString(value) || | |||
| target.flags & (TypeFlags.BooleanLiteral | TypeFlags.Nullable) && value === (target as IntrinsicType).intrinsicName); | |||
| target.flags & (TypeFlags.BooleanLiteral | TypeFlags.Nullable) && value === (target as IntrinsicType).intrinsicName || | |||
| target.flags & TypeFlags.StringMapping && isUnchangedByStringMapping(target.symbol, getStringLiteralType(value))); | |||
There was a problem hiding this comment.
Couldn't target be a string mapping over pretty much anything? That doesn't seem right.
|
Out of curiosity, how would something like |
|
Shouldn't make much difference; Capitalizing the combined hole or just the first hole of two is equivalent, behavior wise, and one of the two is canonical via construction, if that merging behavior exists, I think. Capitalize (and uncapitalize) are already smart enough to only apply to the first character/hole of the template. |
| >TU3 : Symbol(TU3, Decl(intrinsicTypes.ts, 1, 36)) | ||
| >Uppercase : Symbol(Uppercase, Decl(lib.es5.d.ts, --, --)) | ||
|
|
||
| type TU4 = Uppercase<any>; // any | ||
| type TU4 = Uppercase<any>; // Uppercase<`${any}`> |
There was a problem hiding this comment.
Is there a reason we can't just produce Uppercase<any>? The choice to qualify it as `${any}` seems to be made here: https://github.com/microsoft/TypeScript/pull/47050/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R15322
There was a problem hiding this comment.
Iirc, mostly just because, as written, nongeneric string mapping types can only exist over string, template literals, or other mappings (any open-ended stringy type) - it's a bit easier to work with and check for when the type set they're constructed with is smaller (by explicitly coercing anything else into a template type).
|
Since |
|
Ping @ahejlsberg again~ |
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at ca66747. You can monitor the build here. |
|
Heya @weswigham, I've started to run the abridged perf test suite on this PR at ca66747. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at ca66747. You can monitor the build here. |
|
Heya @weswigham, I've started to run the diff-based user code test suite on this PR at ca66747. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the perf test suite on this PR at ca66747. You can monitor the build here. Update: The results are in! |
|
@weswigham |
|
Heya @weswigham, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@weswigham Here they are:Comparison Report - main..47050
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here they are:Comparison Report - main..47050
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // you'd think these were equivalent - the outer `Uppercase` conceptually | ||
| // makes the inner `Lowercase` effectively a noop - but that's not so; | ||
| // the german sharp s makes that not completely true (lowercases to ss, | ||
| // which then uppercases to SS), so arbitrary nestings of mappings make differing sets! |
There was a problem hiding this comment.
Ugh.
But is it true that Uppercase<Lowercase<string>> is just Uppercase<string>?
Does last one wins apply, or is there something 'bout Turkish ı?
There was a problem hiding this comment.
But is it true that
Uppercase<Lowercase<string>>is justUppercase<string>?
Nope.
Does last one wins apply,
Nope, differing mappings stack forever. Only, eg, Uppercase<Uppercase<...>> will collapse the multiple consecutive Uppercases.
is there something 'bout Turkish ı?
Yep.
"ı".toLowerCase().toUpperCase() !== "ı"
There was a problem hiding this comment.
I realized it while authoring, but I wanted it on the record (and I just wanted to comment with a rhyme).
src/compiler/checker.ts
Outdated
| const mappingStack = []; | ||
| while (target.flags & TypeFlags.StringMapping) { | ||
| mappingStack.unshift(target.symbol); | ||
| target = (target as StringMappingType).type; | ||
| } | ||
| const mappedSource = reduceLeft(mappingStack, (memo, value) => getStringMappingType(value, memo), source); |
There was a problem hiding this comment.
You could just use push and reduceRight, no? I guess we might not define our own reduceRight.
Also, would be nice to have a comment. Consider:
| const mappingStack = []; | |
| while (target.flags & TypeFlags.StringMapping) { | |
| mappingStack.unshift(target.symbol); | |
| target = (target as StringMappingType).type; | |
| } | |
| const mappedSource = reduceLeft(mappingStack, (memo, value) => getStringMappingType(value, memo), source); | |
| // We need to see whether applying the same mappings of the target | |
| // onto the source would produce an identical type *and* that | |
| // it's compatible with the inner-most non-string-mapped type. | |
| // | |
| // The intuition here is that if same mappings don't affect the source at all, | |
| // and the source is compatible with the unmapped target, then they must | |
| // still reside in the same domain. | |
| const mappingStack = []; | |
| while (target.flags & TypeFlags.StringMapping) { | |
| mappingStack.push(target.symbol); | |
| target = (target as StringMappingType).type; | |
| } | |
| const mappedSource = mappingStack.reduceRight((memo, value) => getStringMappingType(value, memo), source); |
There was a problem hiding this comment.
I agree, a comment like that would make it clearer what's going on.
There was a problem hiding this comment.
You could just use push and reduceRight, no? I guess we might not define our own reduceRight.
Yep, that's the reason. We only have a reduceLeft and not a reduceRight utility function, and I usually prefer to use those if they're available (otherwise we should remove them right?). Given this array will likely never be more than a handful of elements long, my preference would be to keep the core helper in use, rather than use the (es6) array method. Less work for that one guy patching the TS compiler to run on a blackberry phone browser.
src/compiler/checker.ts
Outdated
| target = (target as StringMappingType).type; | ||
| } | ||
| const mappedSource = reduceLeft(mappingStack, (memo, value) => getStringMappingType(value, memo), source); | ||
| return mappedSource === source && isMemberOfStringMapping(source, target); |
There was a problem hiding this comment.
It's too bad that it's not clearer that target is modified, otherwise it looks at first glance like it'll result in runaway recursion.
src/compiler/checker.ts
Outdated
| type.flags & TypeFlags.StringLiteral ? getStringLiteralType(applyStringMapping(symbol, (type as StringLiteralType).value)) : | ||
| type.flags & TypeFlags.TemplateLiteral ? getTemplateLiteralType(...applyTemplateStringMapping(symbol, (type as TemplateLiteralType).texts, (type as TemplateLiteralType).types)) : |
There was a problem hiding this comment.
The fact that you have to spread the arguments in, ugh.
|
This awesome feature was released with TS4.8 but wasn't mentioned in the release notes. I guess it flew in under the radar. I wonder if it could be added retroactively? |
In the same way that
`${number}`represents the set of all number strings, this letsUppercase<string>represent the set of all strings unaffected by uppercasing (implying they're already uppercase), and the same forLowercase,Capitalize, andUncapitalize. As part of this, all of these string mapping types also are actually applied to template literal types, rather than... not. SoLowercase<`A${string}`>is now`a${Lowercase<string>}`, rather than the very clearly wrong`A${string}`.CapitalizeandUncapitalizeonly affect the first segment of a pattern template literal, reflecting their implementation, soCapitalize<`a${string}`>is just`A${string}`, whileCapitalize<`${string}-${string}`>is`${Capitalize<string>}-${string}`.Technically, this is a breaking change, since now
Uppercase<string>isn't juststring.