Skip to content

Add missing type argument constraints check #51766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Conversation

apendua
Copy link
Contributor

@apendua apendua commented Dec 5, 2022

Fixes #29530

Normally the argument constraints check would be executed via checkTypeReferenceNode() but since an expression of the form import(...).Type<A, B, C> yields SyntaxKind.ImportType rather than SyntaxKind.TypeReference, this is not happening. I was trying to find a place in the code where this validation could be added, which would be as close to checkImportType() as possible and it would not require any major refactoring. This is how I landed at resolveImportSymbolType(), but I am not sure if it's the best possible choice.

My guts feeling is that I should be able to perform a similar check using the type evaluated with getTypeReferenceType(), and so the constraints validation could potentially be encapsulated within checkImportType(), which I think would be more appropriate. I'd appreciate any piece of advise on this matter.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Dec 5, 2022
@@ -17592,6 +17592,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return getTypeOfSymbol(symbol); // intentionally doesn't use resolved symbol so type is cached as expected on the alias
}
else {
tryGetDeclaredTypeOfSymbol(resolvedSymbol); // call this first to ensure typeParameters is populated (if applicable)
Copy link
Member

@sandersn sandersn Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason you can't inspect the type returned from tryGetDeclaredTypeOfSymbol for type parameters? Or are TypeParameters only available only on Symbol(Links) instead of type references?

tryGetDeclaredTypeOfSymbol(resolvedSymbol).typeParameters would work, I think, if you check that the type is an InterfaceType (getObjectFlags(t) & (ObjectFlags.Class | ObjectFlags.Interface)) and that typeParameters is not undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandersn

is there a reason you can't inspect the type returned from tryGetDeclaredTypeOfSymbol for type parameters? Or are TypeParameters only available only on Symbol(Links) instead of type references?

So I tried the approach that you suggested and I observed that the type returned by tryGetDeclaredTypeOfSymbol() is not InterfaceType but TypeReference, where target is InterfaceType. When I try target.typeParameters, this unfortunately doesn't work because the elements of target.typeParameters do not have constraint property for some reason.

Next, I examined other uses of checkTypeArgumentConstraint() and it seems like typeParameters are obtained via getTypeParametersForTypeReference(). When you check the implementation of that function, it seems to be conceptually equivalent to what I was doing (it uses getSymbolLinks() at the end of the day):

    function getTypeParametersForTypeReference(node: TypeReferenceNode | ExpressionWithTypeArguments) {
        const type = getTypeFromTypeReference(node);
        if (!isErrorType(type)) {
            const symbol = getNodeLinks(node).resolvedSymbol;
            if (symbol) {
                return symbol.flags & SymbolFlags.TypeAlias && getSymbolLinks(symbol).typeParameters ||
                    (getObjectFlags(type) & ObjectFlags.Reference ? (type as TypeReference).target.localTypeParameters : undefined);
            }
        }
        return undefined;
    }

So I ended up extracting a getTypeParametersForTypeAndSymbol() sub-routine in order to leverage the same logic for handling edge cases. Does that make more sense to you now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, extracting the existing code seems like a good solution.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can get typeParameters more directly.

@sandersn sandersn self-assigned this Dec 14, 2022
@apendua apendua requested review from sandersn and removed request for weswigham and ahejlsberg December 14, 2022 10:42
@@ -17592,6 +17592,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return getTypeOfSymbol(symbol); // intentionally doesn't use resolved symbol so type is cached as expected on the alias
}
else {
tryGetDeclaredTypeOfSymbol(resolvedSymbol); // call this first to ensure typeParameters is populated (if applicable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, extracting the existing code seems like a good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Missing error when passing unconstrained generic to imported type
3 participants