Fixed apparent type of homomorphic mapped type with non-homomorphic instantiation #56727
Conversation
typescript-bot
added
the
For Uncommitted Bug
| const modifiersConstraint = getConstraintOfType(getModifiersTypeFromMappedType(type)); | ||
| if (modifiersConstraint) { | ||
| homomorphicMappedTypeApparentTypeStack.push(type); | ||
| constraint = getApparentType(modifiersConstraint); |
There was a problem hiding this comment.
I'm not super sure if I should be calling getApparentType here but then... we get to this place from getApparentType so it feels like it's OK to do right now.
| if (typeVariable && !mappedType.declaration.nameType) { | ||
| let constraint: Type | undefined; | ||
| if (!type.target) { | ||
| constraint = getConstraintOfTypeParameter(typeVariable); |
There was a problem hiding this comment.
The other branch is now using getApparentType. If that's the correct thing to do then I feel like this probably should call that too. This would fix issues like this one:
declare function func2<A extends readonly any[], B extends A>(
arr: A,
fields: [
// A rest element type must be an array type.(2574)
...{
[K in keyof B]: { v: B[K] };
},
],
): [A, B];
declare const tuple: [number | string, number | boolean, number | bigint];
const result = func2(tuple, [{ v: "foo" }, { v: true }, { v: 6 }]);
There was a problem hiding this comment.
You probably could... or you could move the apparent type check forward and replace everyType(constraint, isArrayOrTupleType) with something like everyType(constraint, isApparentArrayOrTupleType), provided it's not critical the instantiation here be with the apparent types (and I don't think the instantiation using an apparent constraint is the important part, since mapped types have lazy apparent type members). It's possible changing the should-this-be-a-mapped-array check we use everywhere to something that looks into the apparent types of the members of the constraint could be prudent.
typescript-bot
added
For Backlog Bug
There was a problem hiding this comment.
This looks OK to me - calling getApparentType on part of a type while resolving an apparent type is fairly normal behavior (mapped types can just be circular, which you guard against), and using that information to see if the apparent mapped type should be array or tuple like seems reasonable to me. Still, I'd like to let @ahejlsberg get a chance to review this if he'd like, too, so once the extended tests come back (presumably clean), I'll hold off on merging this for a little bit.
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at d762b80. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the regular perf test suite on this PR at d762b80. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at d762b80. You can monitor the build here. Update: The results are in! |
|
@weswigham 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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
|
This PR was missing a "user test this" and broke VS Code's compiling, though I think in a good way: node_modules/@types/sinon/index.d.ts:47:20 - error TS2370: A rest parameter must be of an array type.
47 calledWith(...args: Partial<MatchArguments<TArgs>>): boolean;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
node_modules/@types/sinon/index.d.ts:1120:13 - error TS2370: A rest parameter must be of an array type.
1120 ...args: Partial<MatchArguments<TArgs>>
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
node_modules/@types/sinon/index.d.ts:1127:69 - error TS2370: A rest parameter must be of an array type.
1127 alwaysCalledWith<TArgs extends any[]>(spy: SinonSpy<TArgs>, ...args: Partial<MatchArguments<TArgs>>): void;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
node_modules/@types/sinon/index.d.ts:1133:68 - error TS2370: A rest parameter must be of an array type.
1133 neverCalledWith<TArgs extends any[]>(spy: SinonSpy<TArgs>, ...args: Partial<MatchArguments<TArgs>>): void;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Found 4 errors in the same file, starting at: node_modules/@types/sinon/index.d.ts:47(I believe they are just using an old version of the sinon types.) |
|
It is slightly surprising to me since I wouldn't exactly expect some types to stop being spreadable with this change. Unless this is hitting some circularity case here or something. I'll reduce this to a minimal repro case later so we can assess with more confidence what happens here. |
fixes #56726