Skip to content

Drop unnecessary type arguments in the isolated declarations quick fix #59665

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 13 commits into from
Sep 16, 2024
Merged
1 change: 1 addition & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1640,6 +1640,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
getBaseTypeOfLiteralType,
getWidenedType,
getWidenedLiteralType,
fillMissingTypeArguments,
getTypeFromTypeNode: nodeIn => {
const node = getParseTreeNode(nodeIn, isTypeNode);
return node ? getTypeFromTypeNode(node) : errorType;
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5394,6 +5394,7 @@ export interface TypeChecker {
/** @internal */ isTypeParameterPossiblyReferenced(tp: TypeParameter, node: Node): boolean;
/** @internal */ typeHasCallOrConstructSignatures(type: Type): boolean;
/** @internal */ getSymbolFlags(symbol: Symbol): SymbolFlags;
/** @internal */ fillMissingTypeArguments(typeArguments: readonly Type[], typeParameters: readonly TypeParameter[] | undefined, minTypeArgumentCount: number, isJavaScriptImplicitAny: boolean): Type[];
}

/** @internal */
Expand Down
11 changes: 8 additions & 3 deletions src/services/codefixes/fixMissingTypeAnnotationOnExports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import {
createImportAdder,
eachDiagnostic,
registerCodeFix,
typeNodeToAutoImportableTypeNode,
typePredicateToAutoImportableTypeNode,
typeToAutoImportableTypeNode,
typeToMinimizedReferenceType,
} from "../_namespaces/ts.codefix.js";
import {
ArrayBindingPattern,
Expand Down Expand Up @@ -1096,9 +1097,9 @@ function withContext<T>(
return emptyInferenceResult;
}

function typeToTypeNode(type: Type, enclosingDeclaration: Node, flags = NodeBuilderFlags.None) {
function typeToTypeNode(type: Type, enclosingDeclaration: Node, flags = NodeBuilderFlags.None): TypeNode | undefined {
let isTruncated = false;
const result = typeToAutoImportableTypeNode(typeChecker, importAdder, type, enclosingDeclaration, scriptTarget, declarationEmitNodeBuilderFlags | flags, declarationEmitInternalNodeBuilderFlags, {
const minimizedTypeNode = typeToMinimizedReferenceType(typeChecker, type, enclosingDeclaration, declarationEmitNodeBuilderFlags | flags, declarationEmitInternalNodeBuilderFlags, {
moduleResolverHost: program,
trackSymbol() {
return true;
Expand All @@ -1107,6 +1108,10 @@ function withContext<T>(
isTruncated = true;
},
});
if (!minimizedTypeNode) {
return undefined;
}
const result = typeNodeToAutoImportableTypeNode(minimizedTypeNode, importAdder, scriptTarget);
return isTruncated ? factory.createKeywordTypeNode(SyntaxKind.AnyKeyword) : result;
}

Expand Down
46 changes: 45 additions & 1 deletion src/services/codefixes/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
flatMap,
FunctionDeclaration,
FunctionExpression,
GenericType,
GetAccessorDeclaration,
getAllAccessorDeclarations,
getCheckFlags,
Expand Down Expand Up @@ -59,6 +60,7 @@ import {
isSetAccessorDeclaration,
isStringLiteral,
isTypeNode,
isTypeReferenceNode,
isTypeUsableAsPropertyName,
isYieldExpression,
LanguageServiceHost,
Expand Down Expand Up @@ -595,7 +597,15 @@ function createTypeParameterName(index: number) {

/** @internal */
export function typeToAutoImportableTypeNode(checker: TypeChecker, importAdder: ImportAdder, type: Type, contextNode: Node | undefined, scriptTarget: ScriptTarget, flags?: NodeBuilderFlags, internalFlags?: InternalNodeBuilderFlags, tracker?: SymbolTracker): TypeNode | undefined {
let typeNode = checker.typeToTypeNode(type, contextNode, flags, internalFlags, tracker);
const typeNode = checker.typeToTypeNode(type, contextNode, flags, internalFlags, tracker);
if (!typeNode) {
return undefined;
}
return typeNodeToAutoImportableTypeNode(typeNode, importAdder, scriptTarget);
}

/** @internal */
export function typeNodeToAutoImportableTypeNode(typeNode: TypeNode, importAdder: ImportAdder, scriptTarget: ScriptTarget): TypeNode | undefined {
if (typeNode && isImportTypeNode(typeNode)) {
const importableReference = tryGetAutoImportableReferenceFromTypeNode(typeNode, scriptTarget);
if (importableReference) {
Expand All @@ -608,6 +618,40 @@ export function typeToAutoImportableTypeNode(checker: TypeChecker, importAdder:
return getSynthesizedDeepClone(typeNode);
}

function endOfRequiredTypeParameters(checker: TypeChecker, type: GenericType): number {
Debug.assert(type.typeArguments);
const fullTypeArguments = type.typeArguments;
const target = type.target;
for (let cutoff = 0; cutoff < fullTypeArguments.length; cutoff++) {
const typeArguments = fullTypeArguments.slice(0, cutoff);
const filledIn = checker.fillMissingTypeArguments(typeArguments, target.typeParameters, cutoff, /*isJavaScriptImplicitAny*/ false);
if (filledIn.every((fill, i) => fill === fullTypeArguments[i])) {
return cutoff;
}
}
// If we make it all the way here, all the type arguments are required.
return fullTypeArguments.length;
}

/** @internal */
export function typeToMinimizedReferenceType(checker: TypeChecker, type: Type, contextNode: Node | undefined, flags?: NodeBuilderFlags, internalFlags?: InternalNodeBuilderFlags, tracker?: SymbolTracker): TypeNode | undefined {
let typeNode = checker.typeToTypeNode(type, contextNode, flags, internalFlags, tracker);
if (!typeNode) {
return undefined;
}
if (isTypeReferenceNode(typeNode)) {
const genericType = type as GenericType;
if (genericType.typeArguments && typeNode.typeArguments) {
const cutoff = endOfRequiredTypeParameters(checker, genericType);
if (cutoff < typeNode.typeArguments.length) {
const newTypeArguments = factory.createNodeArray(typeNode.typeArguments.slice(0, cutoff));
typeNode = factory.updateTypeReferenceNode(typeNode, typeNode.typeName, newTypeArguments);
}
}
}
return typeNode;
}

/** @internal */
export function typePredicateToAutoImportableTypeNode(checker: TypeChecker, importAdder: ImportAdder, typePredicate: TypePredicate, contextNode: Node | undefined, scriptTarget: ScriptTarget, flags?: NodeBuilderFlags, internalFlags?: InternalNodeBuilderFlags, tracker?: SymbolTracker): TypeNode | undefined {
let typePredicateNode = checker.typePredicateToTypePredicateNode(typePredicate, contextNode, flags, internalFlags, tracker);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path='fourslash.ts'/>

// @isolatedDeclarations: true
// @declaration: true
// @lib: es2015
////let x: Iterator<number>;
////export const y = x;

verify.codeFix({
description: "Add annotation of type 'Iterator<number>'",
index: 0,
newFileContent:
`let x: Iterator<number>;
export const y: Iterator<number> = x;`,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/// <reference path='fourslash.ts'/>

// @isolatedDeclarations: true
// @declaration: true

////export interface Foo<T, U = T[]> {}
////export function foo(x: Foo<string>) {
//// return x;
////}

verify.codeFix({
description: "Add return type 'Foo<string>'",
index: 0,
newFileContent:
`export interface Foo<T, U = T[]> {}
export function foo(x: Foo<string>): Foo<string> {
return x;
}`,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/// <reference path='fourslash.ts'/>

// In the abstract, we might prefer the inferred return type annotation to
// be identical to the parameter type (with 2 type parameters).
// Our current heuristic to avoid overly complex types in this case creates
// "overly simple" types, but this tradeoff seems reasonable.

// @isolatedDeclarations: true
// @declaration: true
////export interface Foo<T, U = T[]> {}
////export function foo(x: Foo<string, string[]>) {
//// return x;
////}

verify.codeFix({
description: "Add return type 'Foo<string>'",
index: 0,
newFileContent:
`export interface Foo<T, U = T[]> {}
export function foo(x: Foo<string, string[]>): Foo<string> {
return x;
}`,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference path='fourslash.ts'/>

// Our current heursitic to avoid overly verbose generic types
// doesn't handle generic types nested inside other types.

// @isolatedDeclarations: true
// @declaration: true
////export interface Foo<T, U = T[]> {}
////export function foo(x: Map<number, Foo<string>>) {
//// return x;
////}

verify.codeFix({
description: "Add return type 'Map<number, Foo<string, string[]>>'",
index: 0,
newFileContent:
`export interface Foo<T, U = T[]> {}
export function foo(x: Map<number, Foo<string>>): Map<number, Foo<string, string[]>> {
return x;
}`,
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path='fourslash.ts'/>

// @isolatedDeclarations: true
// @declaration: true
// @lib: es2015
//// export function foo(x: Generator<number>) {
//// return x;
//// }

verify.codeFix({
description: "Add return type 'Generator<number>'",
index: 0,
newFileContent:
`export function foo(x: Generator<number>): Generator<number> {
Copy link
Member

Choose a reason for hiding this comment

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

To be clear I was meaning to write function *foo() { } here and then see what happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, OK, added.

It's not technically related to the rest of this PR since it gets inferred a precise return that needs all 3 type arguments, but it's a good case to include in the test suite anyway.

return x;
}`
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path='fourslash.ts'/>

// @isolatedDeclarations: true
// @declaration: true
// @lib: es2015
//// export function *foo() {
//// yield 5;
//// }

verify.codeFix({
description: "Add return type 'Generator<number, void, unknown>'",
index: 0,
newFileContent:
`export function *foo(): Generator<number, void, unknown> {
yield 5;
}`
});