Uh oh!
There was an error while loading. Please reload this page.
Add optionality to mapped type indexed access substitutions - #57549
Conversation
Anders Hejlsberg (ahejlsberg)
commented
Feb 26, 2024
TypeScript Bot (@typescript-bot) test top200 |
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the faster perf test suite on this PR at ab3184b. You can monitor the build here. Update: The results are in! |
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based top-repos suite on this PR at ab3184b. You can monitor the build here. Update: The results are in! |
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the parallelized Definitely Typed test suite on this PR at ab3184b. You can monitor the build here. Update: The results are in! |
Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based user code test suite on this PR at ab3184b. You can monitor the build here. Update: The results are in! |
TypeScript Bot (typescript-bot)
commented
Feb 26, 2024
Anders Hejlsberg (@ahejlsberg) 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
|
TypeScript Bot (typescript-bot)
commented
Feb 26, 2024
Anders Hejlsberg (@ahejlsberg) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Feb 26, 2024
Hey Anders Hejlsberg (@ahejlsberg), the results of running the DT tests are ready. |
| 0; | ||
| } | ||
| function getCombinedMappedTypeOptionality(type: MappedType): number { |
There was a problem hiding this comment.
If I'm not mistaken this currently falls short here:
typeObj={a: 1,b: 2};typeIdentity<T>={[KinkeyofT]: T[K]};constmapped: {[KinkeyofIdentity<Partial<Obj>>]: Obj[K]}={}constresolveMapped=<Kextendskeyoftypeofmapped>(key: K)=>mapped[key].toString();// should error but it doesntSome extra handling of homomorphic mapped types might have to be added here.
There was a problem hiding this comment.
Yeah, looks like we need to recurse in getCombinedMappedTypeOptionality.
TypeScript Bot (typescript-bot)
commented
Feb 26, 2024
Anders Hejlsberg (@ahejlsberg) Here are the results of running the top-repos suite comparing Everything looks good! |
| const templateMapper = combineTypeMappers(objectType.mapper, mapper); | ||
| return instantiateType(getTemplateTypeFromMappedType(objectType.target as MappedType || objectType), templateMapper); | ||
| const instantiatedTemplateType = instantiateType(getTemplateTypeFromMappedType(objectType.target as MappedType || objectType), templateMapper); | ||
| return addOptionality(instantiatedTemplateType, /*isProperty*/ true, getCombinedMappedTypeOptionality(objectType) > 0); |
There was a problem hiding this comment.
Taking known properties into consideration would fix this inconsistency that this PR creates:
typeObj={a: string;b: number;};typeObj2={b: number;c: boolean;};declareconstmapped: {[Kinkeyof(Partial<Obj>&Required<Obj2>)]: number;};// displays the same way as `resolved` below!mapped;// ^? const mapped: { a?: number | undefined; b: number; c: number; }constaccessMapped=<KextendskeyofObj2>(key: K)=>mapped[key].toString();declareconstresolved: {a?: number|undefined;b: number;c: number};constaccessResolved=<KextendskeyofObj2>(key: K)=>resolved[key].toString();There was a problem hiding this comment.
Right, when mapping over intersections constructed from multiple mapped types the PR makes the most conservative assumption about optionality. I think that's an acceptable compromise and I'm concerned with the potential cost of examining every member possibly selected by the key.
| return modifiers & MappedTypeModifiers.ExcludeOptional ? -1 : modifiers & MappedTypeModifiers.IncludeOptional ? 1 : 0; | ||
| } | ||
| function getModifiersTypeOptionality(type: Type): number { |
There was a problem hiding this comment.
I don't understand what this is computing. What does a result of 2 mean?
There was a problem hiding this comment.
The result is always either -1, 0, or 1. -1 means optionality is stripped (i.e. -?), 0 means optionality is unchanged, 1 means optionality is added (i.e. +?). When a homomorphic mapped type doesn't modify optionality, we (recursively) consult the optionality of the type you're mapping over to see if it strips or adds optionality.
There was a problem hiding this comment.
Should this be an enum? Then it'd be easier to tell what the numbers mean.
There was a problem hiding this comment.
It could be an enum, but number feels more appropriate since we use the greater and less than operators to relate the combined values.
There was a problem hiding this comment.
Would a comment about what each number represents work?
Ryan Cavanaugh (RyanCavanaugh)
left a comment
There was a problem hiding this comment.
Behavior looks good
Fixes#57487.