Fixed an issue with not being able to use mapped type over union constraint as rest param #49947
Conversation
…traint as rest param
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Andarist
force-pushed
the
fix/rest-param-using-mapped-type-over-union-constraint
branch
from
778e73c to
6554965
Compare
src/compiler/checker.ts
Outdated
| @@ -19860,7 +19860,7 @@ namespace ts { | |||
| return varianceResult; | |||
| } | |||
| } | |||
| else if (isReadonlyArrayType(target) ? isArrayOrTupleType(source) : isArrayType(target) && isTupleType(source) && !source.target.readonly) { | |||
| else if (isReadonlyArrayType(target) ? everyType(source, isArrayOrTupleType) : isArrayType(target) && isTupleType(source) && !source.target.readonly) { | |||
There was a problem hiding this comment.
There is a good chance that this should do the same thing in the whenFalse branch of this ternary expression:
| else if (isReadonlyArrayType(target) ? everyType(source, isArrayOrTupleType) : isArrayType(target) && isTupleType(source) && !source.target.readonly) { | |
| else if (isReadonlyArrayType(target) ? everyType(source, isArrayOrTupleType) : isArrayType(target) && everyType(source, isTupleType) && !source.target.readonly) { |
but to make such a change I will have to figure out a test case validating it. I don't think it's going to be super hard but my available time is limited rn.
There was a problem hiding this comment.
The false branch is just for read-only types, so yeah, it should probably be the same, otherwise it'll work for non-readonly tuples, but not readonly ones.
There was a problem hiding this comment.
I've pushed out a fix and tests for this.
| type Mapped<T> = { [P in keyof T]: T[P] extends string ? [number] : [string] } | ||
|
|
||
| declare function test<T extends [number] | [string]>( |
There was a problem hiding this comment.
This repro is a lot easier to understand for me because it has fewer moving parts and fewer repeated identifier, so it's easier to tell what dependencies exist between elements:
declare function test<T extends [number] | []>(
fn: (...args: { [_ in keyof T]: [boolean] | [string] }) => void
): numberThere was a problem hiding this comment.
I agree with the fact that the proposed version is more succinct but at the same time, I don't feel like the added test case is overly verbose. For me personally, this is a little bit harder to read because it's less "usual". The inlined mapped type also doesn't make it completely obvious to me if this mapped type is still homomorphic or not (although that's just my lack of expertise).
Overall, I don't mind making a change but perhaps it would be good to have some guidelines in the CONTRIBUTING.md for how one should write tests etc. For an external contributor like me, it's not that obvious what kind of tests are preferred by the team.
Please let me know if I should adjust the test, add a new one or leave it as is.
…mapped-type-over-union-constraint
|
Just needs a resync with |
…mapped-type-over-union-constraint # Conflicts: # src/compiler/checker.ts
|
synced this with main, could you rerun the tests? |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 29d4bb9. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 29d4bb9. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 29d4bb9. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 29d4bb9. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 29d4bb9. You can monitor the build here. Update: The results are in! |
|
I should have waited for the test run, I guess; PR seems to be erroring. |
|
Eh, it seems that this change conflicts with the merged #52651 . I will have to investigate this and report back sometime later. |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here they are:Comparison Report - main..49947
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
…aints but avoid resolving the apparent type of mapped types for contextual types
| @@ -13861,7 +13861,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const typeVariable = getHomomorphicTypeVariable(type); | |||
| if (typeVariable && !type.declaration.nameType) { | |||
| const constraint = getConstraintOfTypeParameter(typeVariable); | |||
| if (constraint && isArrayType(constraint)) { | |||
| if (constraint && everyType(constraint, isArrayType)) { | |||
There was a problem hiding this comment.
In a way, this reverts the recent change from #52651 . That issue is now fixed by avoiding getting the apparent type of mapped types within getApparentTypeOfContextualType - you can see the change in the commit here
I think this is a better fix for that issue anyway, I just didn't figure it out earlier. Getting the "resolved" apparent type for mapped types with tuple constraint (and array) constraints is important for relationship checking. The mapped type that resolves~ to a tuple is usually "normalized" here, that allows tuple/array-oriented branches to relate things.
src/compiler/checker.ts
Outdated
| // That would evaluate mapped types with array or tuple type constraints too eagerly | ||
| // and thus it would prevent `getTypeOfPropertyOfContextualType` from obtaining per-position contextual type for elements of array literal expressions. | ||
| // Apparent type of other mapped types is already the mapped type itself so we can just avoid calling `getApparentType` here for all mapped types. | ||
| t => getObjectFlags(t) & ObjectFlags.Mapped ? t : getApparentType(t), |
There was a problem hiding this comment.
This might also need to handle intersections better, their mapped types parts should be left alone similarly. I don't have a test case to validate this at hand though so I refrained from making further changes to this. However, I think that I know how to write a sensible test case for this with the changes from this PR. So I would like to wait for one of them to land so I could tweak this part of the code and add the appropriate test case to the other PR.
There was a problem hiding this comment.
And I pushed out this extra fix here: 56f12bc
There was a problem hiding this comment.
Would you care to open a new PR for the further improvement? Since it comes with a test case proving it's utility, it looks pretty good - will just need to make sure the extended suites stay clean when that handling is in place.
There was a problem hiding this comment.
Oh, I thought that this would be linked better - this commit is already part of the #52062 :) it only made sense for me to add this change there as that PR was already concerned with intersected mapped types and thus I already had an idea what a test to write there for this patch
|
@jakebailey @weswigham I managed to resolve the conflict with #52651 by rolling back its changes and applying a different fix for the issue that it was fixing. Now every test passes again - the code has changed though so this might require an extra round of review + the extended test suite run :) |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 940e1ad. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 940e1ad. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 940e1ad. 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 940e1ad. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 940e1ad. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here they are:
CompilerComparison Report - main..49947
System
Hosts
Scenarios
TSServerComparison Report - main..49947
System
Hosts
Scenarios
StartupComparison Report - main..49947
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
…mapped-type-over-union-constraint
Andarist
deleted the
fix/rest-param-using-mapped-type-over-union-constraint
branch
Fixes a repro case mentioned in this comment #29919 (comment) but I don't think that it fixes the issue as a whole.