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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
const type = 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.

const typeParameters = type && getTypeParametersForTypeAndSymbol(type, resolvedSymbol);
if (node.typeArguments && typeParameters) {
addLazyDiagnostic(() => {
checkTypeArgumentConstraints(node, typeParameters);
});
}
return getTypeReferenceType(node, resolvedSymbol); // getTypeReferenceType doesn't handle aliases - it must get the resolved symbol
}
}
Expand Down Expand Up @@ -37238,12 +37245,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return getEffectiveTypeArguments(node, typeParameters)[index];
}

function getEffectiveTypeArguments(node: TypeReferenceNode | ExpressionWithTypeArguments, typeParameters: readonly TypeParameter[]): Type[] {
function getEffectiveTypeArguments(node: TypeReferenceNode | ExpressionWithTypeArguments | NodeWithTypeArguments, typeParameters: readonly TypeParameter[]): Type[] {
return fillMissingTypeArguments(map(node.typeArguments!, getTypeFromTypeNode), typeParameters,
getMinTypeArgumentCount(typeParameters), isInJSFile(node));
}

function checkTypeArgumentConstraints(node: TypeReferenceNode | ExpressionWithTypeArguments, typeParameters: readonly TypeParameter[]): boolean {
function checkTypeArgumentConstraints(node: TypeReferenceNode | ExpressionWithTypeArguments | NodeWithTypeArguments, typeParameters: readonly TypeParameter[]): boolean {
let typeArguments: Type[] | undefined;
let mapper: TypeMapper | undefined;
let result = true;
Expand All @@ -37264,13 +37271,20 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return result;
}

function getTypeParametersForTypeAndSymbol(type: Type, symbol: Symbol) {
if (!isErrorType(type)) {
return symbol.flags & SymbolFlags.TypeAlias && getSymbolLinks(symbol).typeParameters ||
(getObjectFlags(type) & ObjectFlags.Reference ? (type as TypeReference).target.localTypeParameters : undefined);
}
return undefined;
}

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 getTypeParametersForTypeAndSymbol(type, symbol);
}
}
return undefined;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
tests/cases/compiler/file2.ts(1,37): error TS2344: Type 'T' does not satisfy the constraint 'string'.


==== tests/cases/compiler/file1.ts (0 errors) ====
export type Foo<T extends string> = { foo: T }

==== tests/cases/compiler/file2.ts (1 errors) ====
type Bar<T> = import('./file1').Foo<T>;
~
!!! error TS2344: Type 'T' does not satisfy the constraint 'string'.
!!! related TS2208 tests/cases/compiler/file2.ts:1:10: This type parameter might need an `extends string` constraint.

14 changes: 14 additions & 0 deletions tests/baselines/reference/unmetTypeConstraintInImportCall.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
=== tests/cases/compiler/file1.ts ===
export type Foo<T extends string> = { foo: T }
>Foo : Symbol(Foo, Decl(file1.ts, 0, 0))
>T : Symbol(T, Decl(file1.ts, 0, 16))
>foo : Symbol(foo, Decl(file1.ts, 0, 37))
>T : Symbol(T, Decl(file1.ts, 0, 16))

=== tests/cases/compiler/file2.ts ===
type Bar<T> = import('./file1').Foo<T>;
>Bar : Symbol(Bar, Decl(file2.ts, 0, 0))
>T : Symbol(T, Decl(file2.ts, 0, 9))
>Foo : Symbol(Foo, Decl(file1.ts, 0, 0))
>T : Symbol(T, Decl(file2.ts, 0, 9))

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
=== tests/cases/compiler/file1.ts ===
export type Foo<T extends string> = { foo: T }
>Foo : Foo<T>
>foo : T

=== tests/cases/compiler/file2.ts ===
type Bar<T> = import('./file1').Foo<T>;
>Bar : Bar<T>

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
tests/cases/compiler/file2.js(3,36): error TS2344: Type 'T' does not satisfy the constraint 'string'.


==== tests/cases/compiler/file1.js (0 errors) ====
/**
* @template {string} T
* @typedef {{ foo: T }} Foo
*/

export default {};

==== tests/cases/compiler/file2.js (1 errors) ====
/**
* @template T
* @typedef {import('./file1').Foo<T>} Bar
~
!!! error TS2344: Type 'T' does not satisfy the constraint 'string'.
!!! related TS2208 tests/cases/compiler/file2.js:2:14: This type parameter might need an `extends string` constraint.
*/

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
=== tests/cases/compiler/file1.js ===

/**
* @template {string} T
* @typedef {{ foo: T }} Foo
*/

export default {};

=== tests/cases/compiler/file2.js ===

/**
* @template T
* @typedef {import('./file1').Foo<T>} Bar
*/

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
=== tests/cases/compiler/file1.js ===
/**
* @template {string} T
* @typedef {{ foo: T }} Foo
*/

export default {};
>{} : {}

=== tests/cases/compiler/file2.js ===

/**
* @template T
* @typedef {import('./file1').Foo<T>} Bar
*/

7 changes: 7 additions & 0 deletions tests/cases/compiler/unmetTypeConstraintInImportCall.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// @noEmit: true
// @filename: file1.ts
export type Foo<T extends string> = { foo: T }

// @noEmit: true
// @filename: file2.ts
type Bar<T> = import('./file1').Foo<T>;
19 changes: 19 additions & 0 deletions tests/cases/compiler/unmetTypeConstraintInJSDocImportCall.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// @allowJs: true
// @checkJs: true
// @noEmit: true
// @filename: file1.js
/**
* @template {string} T
* @typedef {{ foo: T }} Foo
*/

export default {};

// @allowJs: true
// @checkJs: true
// @noEmit: true
// @filename: file2.js
/**
* @template T
* @typedef {import('./file1').Foo<T>} Bar
*/