Preserve type parameter constraint in emitted mapped types while preserving their distributivity #54759
+482
−16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…erving their distributivity
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. |
typescript-bot
added
For Milestone Bug
This comment was marked as duplicate.
This comment was marked as duplicate.
|
@typescript-bot test this |
|
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at bfdd0d9. 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 bfdd0d9. You can monitor the build here. Update: The results are in! |
|
Heya @gabritto, I've started to run the perf test suite on this PR at bfdd0d9. 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 bfdd0d9. You can monitor the build here. Update: The results are in! |
|
This looks ok to me, but I don't fully understand the code, so I'll defer to @weswigham. |
|
@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 - main..54759
System
Hosts
Scenarios
TSServerComparison Report - main..54759
System
Hosts
Scenarios
StartupComparison Report - main..54759
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @gabritto, the results of running the DT tests are ready. |
weswigham
requested changes
Jul 24, 2023
There was a problem hiding this comment.
I think this may need to be conflict-resolved with #55054 now that that's in (maybe there's another test-case for modifier-preserving non-homomorphic types here, too), but this largely looks good. I'd like to see the name (or preferably the whole type parameter) reused for the key type parameter, though.
src/compiler/checker.ts
Outdated
| const newParam = createTypeParameter(createSymbol(SymbolFlags.TypeParameter, "T" as __String)); | ||
| const name = typeParameterToName(newParam, context); | ||
| const newConstraintParam = createTypeParameter(createSymbol(SymbolFlags.TypeParameter, "T" as __String)); | ||
| const newTypeParam = createTypeParameter(createSymbol(SymbolFlags.TypeParameter, "K" as __String)); |
There was a problem hiding this comment.
Ideally, this should reuse the symbol name of the existing key type, rather than always using K, just in case.
There was a problem hiding this comment.
Also, it would definitely be better just to reuse the existing mapped type's key type parameter wholesale; I can't really think of why we'd need a unique copy here, and it's usually better to keep type identities minimal where possible.
There was a problem hiding this comment.
I've applied the first suggestion (reusing the symbol). I'm not exactly sure what's the expected result that I'm supposed to get here but I think I might be somewhat close using this extra diff:
git diff
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index dfbcd01413..33bbc22259 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -6642,7 +6642,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
let appropriateConstraintTypeNode: TypeNode;
let newTypeVariable: TypeReferenceNode | undefined;
let templateType = getTemplateTypeFromMappedType(type);
- let typeParameter = getTypeParameterFromMappedType(type);
+ const typeParameter = getTypeParameterFromMappedType(type);
// If the mapped type isn't `keyof` constraint-declared, _but_ still has modifiers preserved, and its naive instantiation won't preserve modifiers because its constraint isn't `keyof` constrained, we have work to do
const needsModifierPreservingWrapper = !isMappedTypeWithKeyofConstraintDeclaration(type)
&& !(getModifiersTypeFromMappedType(type).flags & TypeFlags.Unknown)
@@ -6653,14 +6653,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// We do this to ensure we retain the toplevel keyof-ness of the type which may be lost due to keyof distribution during `getConstraintTypeFromMappedType`
if (isHomomorphicMappedTypeWithNonHomomorphicInstantiation(type) && context.flags & NodeBuilderFlags.GenerateNamesForShadowedTypeParams) {
const newConstraintParam = createTypeParameter(createSymbol(SymbolFlags.TypeParameter, "T" as __String));
- const newTypeParam = createTypeParameter(typeParameter.symbol);
const name = typeParameterToName(newConstraintParam, context);
const target = type.target as MappedType;
- typeParameter = newTypeParam;
newTypeVariable = factory.createTypeReferenceNode(name);
templateType = instantiateType(
getTemplateTypeFromMappedType(target),
- makeArrayTypeMapper([getTypeParameterFromMappedType(target), getModifiersTypeFromMappedType(target)], [newTypeParam, newConstraintParam])
+ makeArrayTypeMapper([getTypeParameterFromMappedType(target), getModifiersTypeFromMappedType(target)], [typeParameter, newConstraintParam])
);
}
appropriateConstraintTypeNode = factory.createTypeOperatorNode(SyntaxKind.KeyOfKeyword, newTypeVariable || typeToTypeNodeHelper(getModifiersTypeFromMappedType(type), context));
but the typeParameterToName in typeParameterToDeclarationWithConstraint still uses NodeBuilderFlags.GenerateNamesForShadowedTypeParams. Even if I save those flags on the side and generate this without this generation for shadowed type params then I get the same problem with templateTypeNode.
Am I even on the right track here? Or did I misunderstand your suggestion? It might be worthwhile to take a look at the diff here to see how it behaves with the latest patch that I pushed out.
There was a problem hiding this comment.
Or did I misunderstand your suggestion?
No, that diff is it, it's just unfortunate that we're manufacturing new type identities basically to work around a shortcoming in scope tracking in the type parameter name serializer. I think the issue as-is that the type parameter isn't "visible" to the serializer because the context.enclosingDeclaration is too broad - maybe if, in addition to the patch you linked, you also temporarily override context.enclosingDeclaration with a fake scope, as in signatureToSignatureDeclarationHelper it'd work. In fact, mapped types are basically signature scopes (with only a type parameter) - that's almost certainly what's missing, and what manufacturing a new type is just a kludge covering.
There was a problem hiding this comment.
To get it right:
- I should stop reassigning
typeParameterhere (like in the shortgit diffposted above in the hidden details) - and I should reuse the fake scope strategy used by
signatureToSignatureDeclarationHelper
The second thing... doesn't look completely straightforward so tbh this might take me a longer while especially since I have some more pressing reviews to address right now.
src/compiler/checker.ts
Outdated
| const newConstraintParam = createTypeParameter(createSymbol(SymbolFlags.TypeParameter, "T" as __String)); | ||
| const newTypeParam = createTypeParameter(createSymbol(SymbolFlags.TypeParameter, "K" as __String)); | ||
| const name = typeParameterToName(newConstraintParam, context); | ||
| const target = type.target as MappedType; |
There was a problem hiding this comment.
...This is probably more elegant than using a prependTypeMapping call on the existing mapper.
…mit-type-param-constraint
|
Any motion on this? |
…mit-type-param-constraint
Andarist
force-pushed
the
fix/mapped-type-dts-emit-type-param-constraint
branch
from
0c140e9 to
b618196
Compare
…mit-type-param-constraint
weswigham
reviewed
Jan 9, 2024
…mit-type-param-constraint
|
is this still planned to be shipped with 5.7? |
No. I believe this PR is stalled for the moment. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #54560
fixes #55049
The idea behind this PR is that:
mappedType.targetis the homomorphic one so it feels like it can be used to create a proper instantiationKandTand instantiate the target's template type with them properly