From 17844b403fc802784c8867e2148d819d6550ba51 Mon Sep 17 00:00:00 2001 From: Ben Lickly Date: Tue, 18 Feb 2025 13:48:31 -0800 Subject: [PATCH 1/4] Add failing unit test --- ...7-generics-doesnt-drop-trailing-unknown.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports57-generics-doesnt-drop-trailing-unknown.ts diff --git a/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports57-generics-doesnt-drop-trailing-unknown.ts b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports57-generics-doesnt-drop-trailing-unknown.ts new file mode 100644 index 0000000000000..5b3e75aa13df3 --- /dev/null +++ b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports57-generics-doesnt-drop-trailing-unknown.ts @@ -0,0 +1,19 @@ +/// + +// @isolatedDeclarations: true +// @declaration: true +// @lib: es2015 +//// +////let x: unknown; +////export const s = new Set([x]); +//// + +verify.codeFix({ + description: "Add annotation of type 'Set'", + index: 0, + newFileContent: +` +let x: unknown; +export const s: Set = new Set([x]); +`, +}); From 5a76015e37902ce865fa13da365a209940219fff Mon Sep 17 00:00:00 2001 From: Ben Lickly Date: Tue, 18 Feb 2025 15:28:53 -0800 Subject: [PATCH 2/4] Add a few more test cases --- ...8-genercs-doesnt-drop-trailing-unknown-2.ts | 17 +++++++++++++++++ ...OnExports59-drops-unneeded-after-unknown.ts | 18 ++++++++++++++++++ ...ts60-drops-unneeded-non-trailing-unknown.ts | 18 ++++++++++++++++++ 3 files changed, 53 insertions(+) create mode 100644 tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports58-genercs-doesnt-drop-trailing-unknown-2.ts create mode 100644 tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts create mode 100644 tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports60-drops-unneeded-non-trailing-unknown.ts diff --git a/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports58-genercs-doesnt-drop-trailing-unknown-2.ts b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports58-genercs-doesnt-drop-trailing-unknown-2.ts new file mode 100644 index 0000000000000..df71c96aad80f --- /dev/null +++ b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports58-genercs-doesnt-drop-trailing-unknown-2.ts @@ -0,0 +1,17 @@ +/// + +// @isolatedDeclarations: true +// @declaration: true +// @lib: es2015 +//// +////export const s = new Set(); +//// + +verify.codeFix({ + description: "Add annotation of type 'Set'", + index: 0, + newFileContent: +` +export const s: Set = new Set(); +`, +}); diff --git a/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts new file mode 100644 index 0000000000000..4d60ae3144827 --- /dev/null +++ b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts @@ -0,0 +1,18 @@ +/// + +// @isolatedDeclarations: true +// @declaration: true +//// +////export interface Foo {} +////export function g(x: Foo) { return x; } +//// + +verify.codeFix({ + description: "Add return type 'Foo'", + index: 0, + newFileContent: +` +export interface Foo {} +export function g(x: Foo): Foo { return x; } +`, +}); \ No newline at end of file diff --git a/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports60-drops-unneeded-non-trailing-unknown.ts b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports60-drops-unneeded-non-trailing-unknown.ts new file mode 100644 index 0000000000000..a44acbea25d4b --- /dev/null +++ b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports60-drops-unneeded-non-trailing-unknown.ts @@ -0,0 +1,18 @@ +/// + +// @isolatedDeclarations: true +// @declaration: true +//// +////export interface Foo {} +////export function f(x: Foo) { return x; } +//// + +verify.codeFix({ + description: "Add return type 'Foo'", + index: 0, + newFileContent: +` +export interface Foo {} +export function f(x: Foo): Foo { return x; } +`, +}); \ No newline at end of file From 27d4fe8f7f90383c2c4e118ce2466cfa69ba9e9e Mon Sep 17 00:00:00 2001 From: Ben Lickly Date: Tue, 18 Feb 2025 15:28:53 -0800 Subject: [PATCH 3/4] Minimial change to fix bug --- src/services/codefixes/helpers.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/services/codefixes/helpers.ts b/src/services/codefixes/helpers.ts index 107f954def412..ad037622a97f6 100644 --- a/src/services/codefixes/helpers.ts +++ b/src/services/codefixes/helpers.ts @@ -621,6 +621,13 @@ function endOfRequiredTypeParameters(checker: TypeChecker, type: GenericType): n const fullTypeArguments = type.typeArguments; const target = type.target; for (let cutoff = 0; cutoff < fullTypeArguments.length; cutoff++) { + const firstTypeToDrop = fullTypeArguments[cutoff]; + // Our trick for figuring out if generics have unnecessary defaults by filling in + // missing type arguments incorrectly guesses that trailing unknowns are unnneceesary. + // Simply bail on trailing unknowns to avoid this case. + if ((firstTypeToDrop.getFlags() | TypeFlags.Unknown) === TypeFlags.Unknown) { + continue; + } const typeArguments = fullTypeArguments.slice(0, cutoff); const filledIn = checker.fillMissingTypeArguments(typeArguments, target.typeParameters, cutoff, /*isJavaScriptImplicitAny*/ false); if (filledIn.every((fill, i) => fill === fullTypeArguments[i])) { From 2600cfe527d83391796c775c930be79597cde5c9 Mon Sep 17 00:00:00 2001 From: Ben Lickly Date: Wed, 19 Feb 2025 16:50:26 -0800 Subject: [PATCH 4/4] Simplify implementation to actually look which parameters have constraints Also, update the test that this improves. --- src/services/codefixes/helpers.ts | 6 +----- ...ypeAnnotationOnExports59-drops-unneeded-after-unknown.ts | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/services/codefixes/helpers.ts b/src/services/codefixes/helpers.ts index ad037622a97f6..ebb58bae13c09 100644 --- a/src/services/codefixes/helpers.ts +++ b/src/services/codefixes/helpers.ts @@ -621,11 +621,7 @@ function endOfRequiredTypeParameters(checker: TypeChecker, type: GenericType): n const fullTypeArguments = type.typeArguments; const target = type.target; for (let cutoff = 0; cutoff < fullTypeArguments.length; cutoff++) { - const firstTypeToDrop = fullTypeArguments[cutoff]; - // Our trick for figuring out if generics have unnecessary defaults by filling in - // missing type arguments incorrectly guesses that trailing unknowns are unnneceesary. - // Simply bail on trailing unknowns to avoid this case. - if ((firstTypeToDrop.getFlags() | TypeFlags.Unknown) === TypeFlags.Unknown) { + if (target.localTypeParameters?.[cutoff].constraint === undefined) { continue; } const typeArguments = fullTypeArguments.slice(0, cutoff); diff --git a/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts index 4d60ae3144827..0df342ea44cd2 100644 --- a/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts +++ b/tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports59-drops-unneeded-after-unknown.ts @@ -8,11 +8,11 @@ //// verify.codeFix({ - description: "Add return type 'Foo'", + description: "Add return type 'Foo'", index: 0, newFileContent: ` export interface Foo {} -export function g(x: Foo): Foo { return x; } +export function g(x: Foo): Foo { return x; } `, }); \ No newline at end of file