From 1bc0cd194b79dbbeed0eb406b7b1fdacacff2fc8 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 28 Sep 2018 15:22:27 -0700 Subject: [PATCH 01/18] Now adding @type to variable declarations, at least Probably everything else, but not as well. --- src/services/codefixes/inferFromUsage.ts | 3 -- src/services/textChanges.ts | 32 +++++++++++++++++-- .../fourslash/codeFixInferFromUsageJS.ts | 12 +++++++ 3 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 tests/cases/fourslash/codeFixInferFromUsageJS.ts diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index ed8939079e3ec..e6fad23661e75 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -26,9 +26,6 @@ namespace ts.codefix { errorCodes, getCodeActions(context) { const { sourceFile, program, span: { start }, errorCode, cancellationToken } = context; - if (isSourceFileJS(sourceFile)) { - return undefined; // TODO: GH#20113 - } const token = getTokenAtPosition(sourceFile, start); let declaration!: Declaration | undefined; diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 756d2799a28cb..f40ad4be2ce8c 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -41,6 +41,12 @@ namespace ts.textChanges { Start } + export enum CommentKind { + Single, + Multi, + Jsdoc, + } + function skipWhitespacesAndLineBreaks(text: string, start: number) { return skipTrivia(text, start, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true); } @@ -315,7 +321,7 @@ namespace ts.textChanges { this.replaceRange(sourceFile, { pos, end: pos }, createToken(modifier), { suffix: " " }); } - public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string): void { + public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string, kind = CommentKind.Single): void { const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); // First try to see if we can put the comment on the previous line. @@ -325,7 +331,10 @@ namespace ts.textChanges { const insertAtLineStart = isValidLocationToAddComment(sourceFile, startPosition); const token = getTouchingToken(sourceFile, insertAtLineStart ? startPosition : position); const indent = sourceFile.text.slice(lineStartPosition, startPosition); - const text = `${insertAtLineStart ? "" : this.newLineCharacter}//${commentText}${this.newLineCharacter}${indent}`; + const completeCommentText = kind === CommentKind.Jsdoc ? `/**${commentText}*/` : + kind === CommentKind.Multi ? `/*${commentText}*/` : + `//${commentText}`; + const text = (insertAtLineStart ? "" : this.newLineCharacter) + completeCommentText + this.newLineCharacter + indent; this.insertText(sourceFile, token.getStart(sourceFile), text); } @@ -339,6 +348,10 @@ namespace ts.textChanges { /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ public tryInsertTypeAnnotation(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { + if (isInJSFile(sourceFile)) { + this.tryInsertJSDocType(sourceFile, node, type); + return; + } let endNode: Node | undefined; if (isFunctionLike(node)) { endNode = findChildOfKind(node, SyntaxKind.CloseParenToken, sourceFile); @@ -355,6 +368,21 @@ namespace ts.textChanges { this.insertNodeAt(sourceFile, endNode.end, type, { prefix: ": " }); } + /** TODO: Try allowing insertion of other things too + * TODO: Might need to only disallow node from being a parameterdecl, propertydecl, propertysig + * (or correctly handle parameterdecl by walking up and adding a param, at least) + */ + public tryInsertJSDocType(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { + if (isParameter(node)) { + // RECUR with node=node.parent + node = node.parent; + } + + const commentText = ` @type {${createPrinter().printNode(EmitHint.Unspecified, type, sourceFile)}} `; + this.insertCommentBeforeLine(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.pos).line, node.pos, commentText, CommentKind.Jsdoc); + // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones + } + public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray): void { // If no `(`, is an arrow function `x => x`, so use the pos of the first parameter const start = (findChildOfKind(node, SyntaxKind.OpenParenToken, sourceFile) || first(node.parameters)).getStart(sourceFile); diff --git a/tests/cases/fourslash/codeFixInferFromUsageJS.ts b/tests/cases/fourslash/codeFixInferFromUsageJS.ts new file mode 100644 index 0000000000000..62c9d1bf90b08 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageJS.ts @@ -0,0 +1,12 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: test.js +////[|var foo;|] +////function f() { +//// foo += 2; +////} + +verify.rangeAfterCodeFix("/** @type {number} */\nvar foo;",/*includeWhiteSpace*/ undefined, /*errorCode*/ undefined, 2); From e34bf548c3d91fe59266eb8d5246fcf5a6146d09 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 28 Sep 2018 15:38:12 -0700 Subject: [PATCH 02/18] Improve @param output and add test It's still bad, but at least it's not wrong. --- src/services/textChanges.ts | 15 ++++--- .../fourslash/codeFixInferFromUsage_allJS.ts | 44 +++++++++++++++++++ 2 files changed, 54 insertions(+), 5 deletions(-) create mode 100644 tests/cases/fourslash/codeFixInferFromUsage_allJS.ts diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index f40ad4be2ce8c..a31a54133aa65 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -373,12 +373,17 @@ namespace ts.textChanges { * (or correctly handle parameterdecl by walking up and adding a param, at least) */ public tryInsertJSDocType(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { - if (isParameter(node)) { + // if (isParameter(node)) { // RECUR with node=node.parent - node = node.parent; - } - - const commentText = ` @type {${createPrinter().printNode(EmitHint.Unspecified, type, sourceFile)}} `; + // node = node.parent; + // } + + // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, the line before might the wrong place, etc) + // TODO: Parameter probably shouldn't need to manually unescape its text + const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); + const commentText = isParameter(node) && isIdentifier(node.name) ? + ` @param {${printed}} ${unescapeLeadingUnderscores(node.name.escapedText)} ` : + ` @type {${printed}} `; this.insertCommentBeforeLine(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.pos).line, node.pos, commentText, CommentKind.Jsdoc); // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones } diff --git a/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts b/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts new file mode 100644 index 0000000000000..d3bf66853f8de --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts @@ -0,0 +1,44 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @strictNullChecks: true +// @Filename: important.js + +////function f(x, y) { +//// x += 0; +//// y += ""; +////} +//// +////function g(z) { +//// return z * 2; +////} +//// +////let x = null; +////function h() { +//// if (!x) x = 2; +////} + +verify.codeFixAll({ + fixId: "inferFromUsage", + fixAllDescription: "Infer all types from usage", + newFileContent: +`/** @param {number} x */ +/** @param {string} y */ +function f(x, y) { + x += 0; + y += ""; +} + +/** @param {number} z */ +function g(z) { + return z * 2; +} + +/** @type {number | null} */ +let x = null; +function h() { + if (!x) x = 2; +}`, +}); From 0f4a800d9847ea3b492447697b9959353c291f58 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Mon, 1 Oct 2018 15:54:04 -0700 Subject: [PATCH 03/18] Add some js/inferFromUsage tests and fixes Also, remove redundant is(Set|Get)Accessor functions. --- src/compiler/checker.ts | 2 +- .../transformers/declarations/diagnostics.ts | 10 ++++---- src/compiler/utilities.ts | 8 ------- src/services/codefixes/inferFromUsage.ts | 20 ++++++++++++---- src/services/textChanges.ts | 24 ++++++++----------- .../fourslash/codeFixInferFromUsageCallJS.ts | 18 ++++++++++++++ .../codeFixInferFromUsageMember2JS.ts | 14 +++++++++++ .../codeFixInferFromUsageMemberJS.ts | 20 ++++++++++++++++ 8 files changed, 83 insertions(+), 33 deletions(-) create mode 100644 tests/cases/fourslash/codeFixInferFromUsageCallJS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 611b6c4d4b075..8802938bfafe5 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -28365,7 +28365,7 @@ namespace ts { function isImplementationOfOverload(node: SignatureDeclaration) { if (nodeIsPresent((node as FunctionLikeDeclaration).body)) { - if (isGetAccessor(node) || isSetAccessor(node)) return false; // Get or set accessors can never be overload implementations, but can have up to 2 signatures + if (isGetAccessorDeclaration(node) || isSetAccessorDeclaration(node)) return false; // Get or set accessors can never be overload implementations, but can have up to 2 signatures const symbol = getSymbolOfNode(node); const signaturesOfSymbol = getSignaturesOfSymbol(symbol); // If this function body corresponds to function with multiple signature, it is implementation of overload diff --git a/src/compiler/transformers/declarations/diagnostics.ts b/src/compiler/transformers/declarations/diagnostics.ts index 53b5750a1edd0..7a61661c1f384 100644 --- a/src/compiler/transformers/declarations/diagnostics.ts +++ b/src/compiler/transformers/declarations/diagnostics.ts @@ -33,8 +33,8 @@ namespace ts { isPropertyDeclaration(node) || isPropertySignature(node) || isBindingElement(node) || - isSetAccessor(node) || - isGetAccessor(node) || + isSetAccessorDeclaration(node) || + isGetAccessorDeclaration(node) || isConstructSignatureDeclaration(node) || isCallSignatureDeclaration(node) || isMethodDeclaration(node) || @@ -50,7 +50,7 @@ namespace ts { } export function createGetSymbolAccessibilityDiagnosticForNodeName(node: DeclarationDiagnosticProducing) { - if (isSetAccessor(node) || isGetAccessor(node)) { + if (isSetAccessorDeclaration(node) || isGetAccessorDeclaration(node)) { return getAccessorNameVisibilityError; } else if (isMethodSignature(node) || isMethodDeclaration(node)) { @@ -126,7 +126,7 @@ namespace ts { if (isVariableDeclaration(node) || isPropertyDeclaration(node) || isPropertySignature(node) || isBindingElement(node) || isConstructorDeclaration(node)) { return getVariableDeclarationTypeVisibilityError; } - else if (isSetAccessor(node) || isGetAccessor(node)) { + else if (isSetAccessorDeclaration(node) || isGetAccessorDeclaration(node)) { return getAccessorDeclarationTypeVisibilityError; } else if (isConstructSignatureDeclaration(node) || isCallSignatureDeclaration(node) || isMethodDeclaration(node) || isMethodSignature(node) || isFunctionDeclaration(node) || isIndexSignatureDeclaration(node)) { @@ -466,4 +466,4 @@ namespace ts { }; } } -} \ No newline at end of file +} diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index d4c28ee94c64d..a11a94a60d6d8 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -6719,14 +6719,6 @@ namespace ts { return node.kind >= SyntaxKind.FirstJSDocTagNode && node.kind <= SyntaxKind.LastJSDocTagNode; } - export function isSetAccessor(node: Node): node is SetAccessorDeclaration { - return node.kind === SyntaxKind.SetAccessor; - } - - export function isGetAccessor(node: Node): node is GetAccessorDeclaration { - return node.kind === SyntaxKind.GetAccessor; - } - /** True if has jsdoc nodes attached to it. */ /* @internal */ // TODO: GH#19856 Would like to return `node is Node & { jsDoc: JSDoc[] }` but it causes long compile times diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index e6fad23661e75..80e96d790b7f8 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -47,7 +47,7 @@ namespace ts.codefix { function getDiagnostic(errorCode: number, token: Node): DiagnosticMessage { switch (errorCode) { case Diagnostics.Parameter_0_implicitly_has_an_1_type.code: - return isSetAccessor(getContainingFunction(token)!) ? Diagnostics.Infer_type_of_0_from_usage : Diagnostics.Infer_parameter_types_from_usage; // TODO: GH#18217 + return isSetAccessorDeclaration(getContainingFunction(token)!) ? Diagnostics.Infer_type_of_0_from_usage : Diagnostics.Infer_parameter_types_from_usage; // TODO: GH#18217 case Diagnostics.Rest_parameter_0_implicitly_has_an_any_type.code: return Diagnostics.Infer_parameter_types_from_usage; default: @@ -56,7 +56,7 @@ namespace ts.codefix { } function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, errorCode: number, program: Program, cancellationToken: CancellationToken, markSeen: NodeSeenTracker): Declaration | undefined { - if (!isParameterPropertyModifier(token.kind) && token.kind !== SyntaxKind.Identifier && token.kind !== SyntaxKind.DotDotDotToken) { + if (!isParameterPropertyModifier(token.kind) && token.kind !== SyntaxKind.Identifier && token.kind !== SyntaxKind.DotDotDotToken && token.kind !== SyntaxKind.ThisKeyword) { return undefined; } @@ -65,10 +65,16 @@ namespace ts.codefix { // Variable and Property declarations case Diagnostics.Member_0_implicitly_has_an_1_type.code: case Diagnostics.Variable_0_implicitly_has_type_1_in_some_locations_where_its_type_cannot_be_determined.code: + // TODO: Here! if ((isVariableDeclaration(parent) && markSeen(parent)) || isPropertyDeclaration(parent) || isPropertySignature(parent)) { // handle bad location annotateVariableDeclaration(changes, sourceFile, parent, program, cancellationToken); return parent; } + if (isPropertyAccessExpression(parent)) { + annotateThisPropertyAssignment(changes, sourceFile, parent, program, cancellationToken); + return parent; + // Try jumping! + } return undefined; case Diagnostics.Variable_0_implicitly_has_an_1_type.code: { @@ -89,7 +95,7 @@ namespace ts.codefix { switch (errorCode) { // Parameter declarations case Diagnostics.Parameter_0_implicitly_has_an_1_type.code: - if (isSetAccessor(containingFunction)) { + if (isSetAccessorDeclaration(containingFunction)) { annotateSetAccessor(changes, sourceFile, containingFunction, program, cancellationToken); return containingFunction; } @@ -105,7 +111,7 @@ namespace ts.codefix { // Get Accessor declarations case Diagnostics.Property_0_implicitly_has_type_any_because_its_get_accessor_lacks_a_return_type_annotation.code: case Diagnostics._0_which_lacks_return_type_annotation_implicitly_has_an_1_return_type.code: - if (isGetAccessor(containingFunction) && isIdentifier(containingFunction.name)) { + if (isGetAccessorDeclaration(containingFunction) && isIdentifier(containingFunction.name)) { annotate(changes, sourceFile, containingFunction, inferTypeForVariableFromUsage(containingFunction.name, program, cancellationToken), program); return containingFunction; } @@ -113,7 +119,7 @@ namespace ts.codefix { // Set Accessor declarations case Diagnostics.Property_0_implicitly_has_type_any_because_its_set_accessor_lacks_a_parameter_type_annotation.code: - if (isSetAccessor(containingFunction)) { + if (isSetAccessorDeclaration(containingFunction)) { annotateSetAccessor(changes, sourceFile, containingFunction, program, cancellationToken); return containingFunction; } @@ -130,6 +136,10 @@ namespace ts.codefix { } } + function annotateThisPropertyAssignment(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: PropertyAccessExpression, program: Program, cancellationToken: CancellationToken): void { + annotate(changes, sourceFile, declaration, inferTypeForVariableFromUsage(declaration.name, program, cancellationToken), program); + } + function isApplicableFunctionForInference(declaration: FunctionLike): declaration is MethodDeclaration | FunctionDeclaration | ConstructorDeclaration { switch (declaration.kind) { case SyntaxKind.FunctionDeclaration: diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index a31a54133aa65..39f79e771631c 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -213,7 +213,7 @@ namespace ts.textChanges { formatContext: formatting.FormatContext; } - export type TypeAnnotatable = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; + export type TypeAnnotatable = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature | PropertyAccessExpression; export class ChangeTracker { private readonly changes: Change[] = []; @@ -347,11 +347,11 @@ namespace ts.textChanges { } /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ - public tryInsertTypeAnnotation(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { - if (isInJSFile(sourceFile)) { - this.tryInsertJSDocType(sourceFile, node, type); - return; + public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode): void { + if (isInJSFile(sourceFile) && topNode.parent.kind !== SyntaxKind.PropertySignature) { + return this.tryInsertJSDocType(sourceFile, topNode, type); } + let node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; let endNode: Node | undefined; if (isFunctionLike(node)) { endNode = findChildOfKind(node, SyntaxKind.CloseParenToken, sourceFile); @@ -373,18 +373,14 @@ namespace ts.textChanges { * (or correctly handle parameterdecl by walking up and adding a param, at least) */ public tryInsertJSDocType(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { - // if (isParameter(node)) { - // RECUR with node=node.parent - // node = node.parent; - // } - - // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, the line before might the wrong place, etc) + // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, the line before might be the wrong place, etc) // TODO: Parameter probably shouldn't need to manually unescape its text const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); - const commentText = isParameter(node) && isIdentifier(node.name) ? - ` @param {${printed}} ${unescapeLeadingUnderscores(node.name.escapedText)} ` : + const commentText = + isParameter(node) && isIdentifier(node.name) ? ` @param {${printed}} ${unescapeLeadingUnderscores(node.name.escapedText)} ` : + isGetAccessorDeclaration(node) ? ` @return {${printed}}` : ` @type {${printed}} `; - this.insertCommentBeforeLine(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.pos).line, node.pos, commentText, CommentKind.Jsdoc); + this.insertCommentBeforeLine(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.pos + 1).line, node.pos, commentText, CommentKind.Jsdoc); // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones } diff --git a/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts b/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts new file mode 100644 index 0000000000000..051446c2ff243 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts @@ -0,0 +1,18 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: test.js +////function wat(b) { +//// b(); +////} + +verify.codeFixAll({ + fixId: "inferFromUsage", + fixAllDescription: "Infer all types from usage", + newFileContent: +`/** @param {() => void} b */ +function wat(b) { + b(); +}`}); diff --git a/tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts b/tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts new file mode 100644 index 0000000000000..acb81d7d03c8f --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts @@ -0,0 +1,14 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js + +/////** @typedef {{ [|p|] }} I */ +/////** @type {I} */ +////var i; +////i.p = 0; + + +verify.rangeAfterCodeFix("p: number;", undefined, undefined, 1); diff --git a/tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts b/tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts new file mode 100644 index 0000000000000..95c64652ff335 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts @@ -0,0 +1,20 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @strictNullChecks: true +// @Filename: important.js +////class C { +//// constructor() { +//// [|this.p|] = undefined; +//// } +//// method() { +//// this.p.push(1) +//// } +////} + + +// Note: Should be number[] | undefined, but inference currently privileges assignments +// over usage (even when the only result is undefined) and infers only undefined. +verify.rangeAfterCodeFix("/** @type {undefined} */\n this.p", undefined, undefined, 2); From be3577cfebeb5cc5c76729829cb656dc9f30805d Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Mon, 1 Oct 2018 18:39:54 -0700 Subject: [PATCH 04/18] Fix @typedef refactor --- src/services/textChanges.ts | 2 +- tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 39f79e771631c..3d53eaf89afc7 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -348,7 +348,7 @@ namespace ts.textChanges { /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode): void { - if (isInJSFile(sourceFile) && topNode.parent.kind !== SyntaxKind.PropertySignature) { + if (isInJSFile(sourceFile) && topNode.kind !== SyntaxKind.PropertySignature) { return this.tryInsertJSDocType(sourceFile, topNode, type); } let node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; diff --git a/tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts b/tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts index acb81d7d03c8f..dc52286a9063e 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts @@ -5,10 +5,10 @@ // @noImplicitAny: true // @Filename: important.js -/////** @typedef {{ [|p|] }} I */ +/////** @typedef {{ [|p |]}} I */ /////** @type {I} */ ////var i; ////i.p = 0; -verify.rangeAfterCodeFix("p: number;", undefined, undefined, 1); +verify.rangeAfterCodeFix("p: number", undefined, undefined, 1); From bf5529bf853c1534bde0a513e46791c282f0ed7d Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Tue, 2 Oct 2018 11:09:25 -0700 Subject: [PATCH 05/18] Emit JSDoc optional parameters By surrounding the parameter name with brackets. It is super, super ugly right now. --- src/harness/fourslash.ts | 8 +++-- src/services/codefixes/inferFromUsage.ts | 23 +++++++------ src/services/textChanges.ts | 20 ++++++++---- ...deFixInferFromUsageMultipleParametersJS.ts | 21 ++++++++++++ ...FixInferFromUsageNumberIndexSignatureJS.ts | 15 +++++++++ .../codeFixInferFromUsageOptionalParamJS.ts | 19 +++++++++++ .../codeFixInferFromUsagePropertyAccessJS.ts | 32 +++++++++++++++++++ 7 files changed, 120 insertions(+), 18 deletions(-) create mode 100644 tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index ccd7aa1d067b8..dc28871d1c0ea 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2516,10 +2516,10 @@ Actual: ${stringify(fullActual)}`); * @param expectedContents The contents of the file after the fixes are applied. * @param fileName The file to check. If not supplied, the current open file is used. */ - public verifyFileAfterCodeFix(expectedContents: string, fileName?: string) { + public verifyFileAfterCodeFix(expectedContents: string, fileName?: string, index?: number) { fileName = fileName ? fileName : this.activeFile.fileName; - this.applyCodeActions(this.getCodeFixes(fileName)); + this.applyCodeActions(this.getCodeFixes(fileName), index); const actualContents: string = this.getFileContent(fileName); if (this.removeWhitespace(actualContents) !== this.removeWhitespace(expectedContents)) { @@ -4388,6 +4388,10 @@ namespace FourSlashInterface { this.state.verifyRangeAfterCodeFix(expectedText, includeWhiteSpace, errorCode, index); } + public fileAfterCodeFix(expectedContents: string, fileName?: string, index?: number) { + this.state.verifyFileAfterCodeFix(expectedContents, fileName, index); + } + public codeFixAll(options: VerifyCodeFixAllOptions): void { this.state.verifyCodeFixAll(options); } diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index 80e96d790b7f8..d840364b03ddd 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -158,15 +158,16 @@ namespace ts.codefix { } const types = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) || - containingFunction.parameters.map(p => isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined); + + containingFunction.parameters.map(p => [isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined, /*isOptionalParameter*/ false] as [Type | undefined, boolean]); // We didn't actually find a set of type inference positions matching each parameter position - if (!types || containingFunction.parameters.length !== types.length) { + if (containingFunction.parameters.length !== types.length) { return; } - zipWith(containingFunction.parameters, types, (parameter, type) => { + zipWith(containingFunction.parameters, types, (parameter, [type, isOptionalParameter]) => { if (!parameter.type && !parameter.initializer) { - annotate(changes, sourceFile, parameter, type, program); + annotate(changes, sourceFile, parameter, type, program, isOptionalParameter); } }); } @@ -180,9 +181,9 @@ namespace ts.codefix { } } - function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program): void { + function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program, isOptionalParameter?: boolean): void { const typeNode = type && getTypeNodeIfAccessible(type, declaration, program.getTypeChecker()); - if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode); + if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode, isOptionalParameter); } function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, checker: TypeChecker): TypeNode | undefined { @@ -210,7 +211,7 @@ namespace ts.codefix { return InferFromReference.inferTypeFromReferences(getReferences(token, program, cancellationToken), program.getTypeChecker(), cancellationToken); } - function inferTypeForParametersFromUsage(containingFunction: FunctionLikeDeclaration, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): (Type | undefined)[] | undefined { + function inferTypeForParametersFromUsage(containingFunction: FunctionLikeDeclaration, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): ([Type | undefined, boolean])[] | undefined { switch (containingFunction.kind) { case SyntaxKind.Constructor: case SyntaxKind.FunctionExpression: @@ -253,7 +254,7 @@ namespace ts.codefix { return getTypeFromUsageContext(usageContext, checker); } - export function inferTypeForParametersFromReferences(references: ReadonlyArray, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): (Type | undefined)[] | undefined { + export function inferTypeForParametersFromReferences(references: ReadonlyArray, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): ([Type | undefined, boolean])[] | undefined { if (references.length === 0) { return undefined; } @@ -269,11 +270,13 @@ namespace ts.codefix { } const isConstructor = declaration.kind === SyntaxKind.Constructor; const callContexts = isConstructor ? usageContext.constructContexts : usageContext.callContexts; + let isOptional = false; return callContexts && declaration.parameters.map((parameter, parameterIndex) => { const types: Type[] = []; const isRest = isRestParameter(parameter); for (const callContext of callContexts) { if (callContext.argumentTypes.length <= parameterIndex) { + isOptional = isInJSFile(declaration); continue; } @@ -287,10 +290,10 @@ namespace ts.codefix { } } if (!types.length) { - return undefined; + return [undefined, false] as [undefined, boolean]; } const type = checker.getWidenedType(checker.getUnionType(types, UnionReduction.Subtype)); - return isRest ? checker.createArrayType(type) : type; + return [isRest ? checker.createArrayType(type) : type, isOptional] as [Type, boolean]; }); } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 3d53eaf89afc7..e2d1130571c43 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -347,11 +347,11 @@ namespace ts.textChanges { } /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ - public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode): void { + public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode, isOptionalParameter?: boolean): void { if (isInJSFile(sourceFile) && topNode.kind !== SyntaxKind.PropertySignature) { - return this.tryInsertJSDocType(sourceFile, topNode, type); + return this.tryInsertJSDocType(sourceFile, topNode, type, isOptionalParameter); } - let node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; + const node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; let endNode: Node | undefined; if (isFunctionLike(node)) { endNode = findChildOfKind(node, SyntaxKind.CloseParenToken, sourceFile); @@ -372,18 +372,26 @@ namespace ts.textChanges { * TODO: Might need to only disallow node from being a parameterdecl, propertydecl, propertysig * (or correctly handle parameterdecl by walking up and adding a param, at least) */ - public tryInsertJSDocType(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { + public tryInsertJSDocType(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode, isOptionalParameter: boolean | undefined): void { // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, the line before might be the wrong place, etc) // TODO: Parameter probably shouldn't need to manually unescape its text const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); const commentText = - isParameter(node) && isIdentifier(node.name) ? ` @param {${printed}} ${unescapeLeadingUnderscores(node.name.escapedText)} ` : - isGetAccessorDeclaration(node) ? ` @return {${printed}}` : + isParameter(node) && isIdentifier(node.name) ? this.printJSDocParameter(printed, node.name, isOptionalParameter) : + isGetAccessorDeclaration(node) ? ` @return {${printed}} ` : ` @type {${printed}} `; this.insertCommentBeforeLine(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.pos + 1).line, node.pos, commentText, CommentKind.Jsdoc); // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones } + private printJSDocParameter(printed: string, name: Identifier, isOptionalParameter: boolean | undefined) { + let printName = unescapeLeadingUnderscores(name.escapedText); + if (isOptionalParameter) { + printName = `[${printName}]`; + } + return ` @param {${printed}} ${printName} `; + } + public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray): void { // If no `(`, is an arrow function `x => x`, so use the pos of the first parameter const start = (findChildOfKind(node, SyntaxKind.OpenParenToken, sourceFile) || first(node.parameters)).getStart(sourceFile); diff --git a/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts b/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts new file mode 100644 index 0000000000000..f29f01be3b7ff --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts @@ -0,0 +1,21 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js + +//// function f([|a, b, c, d, e = 0, ...d |]) { +//// } +//// f(1, "string", { a: 1 }, {shouldNotBeHere: 2}, {shouldNotBeHere: 2}, 3, "string"); + + +verify.fileAfterCodeFix( +`/** @param {number} a */ +/** @param {string} b */ +/** @param {{ a: number; }} c */ +/** @param {{ shouldNotBeHere: number; }} d */ +/** @param {(string | number)[]} d */ +function f(a, b, c, d, e = 0, ...d ) { +} +f(1, "string", { a: 1 }, {shouldNotBeHere: 2}, {shouldNotBeHere: 2}, 3, "string");`, undefined, 6); diff --git a/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts b/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts new file mode 100644 index 0000000000000..428a02f49354d --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts @@ -0,0 +1,15 @@ +/// +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js + +////function f([|a|]) { +//// return a[0] + 1; +////} + +verify.fileAfterCodeFix( +`/** @param {number[]} a */ +function f(a) { + return a[0] + 1; +}`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts b/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts new file mode 100644 index 0000000000000..4286d078abf8b --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts @@ -0,0 +1,19 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js +////function f([|a|]){ +//// a; +////} +////f(); +////f(1); + +verify.fileAfterCodeFix( +`/** @param {number} [a] */ +function f(a) { + a; +} +f(); +f(1);`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts b/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts new file mode 100644 index 0000000000000..b363da5151db6 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts @@ -0,0 +1,32 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js + +////function foo([|a, m, x|]) { +//// a.b.c; +//// +//// var numeric = 0; +//// numeric = m.n(); +//// +//// x.y.z +//// x.y.z.push(0); +//// return x.y.z +////} + +verify.fileAfterCodeFix( +`/** @param {{ b: { c: any; }; }} a */ +/** @param {{ n: () => number; }} m */ +/** @param {{ y: { z: number[]; }; }} x */ +function foo(a, m, x) { + a.b.c; + + var numeric = 0; + numeric = m.n(); + + x.y.z + x.y.z.push(0); + return x.y.z +}`, undefined, 2); From 4ee4d690b44fd3a5619e10f428d247768091c4bf Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Wed, 3 Oct 2018 09:31:54 -0700 Subject: [PATCH 06/18] Get rest of existing tests working --- src/services/codefixes/inferFromUsage.ts | 4 +-- .../codeFixInferFromUsageRestParam2JS.ts | 25 ++++++++++++++ .../codeFixInferFromUsageRestParam3JS.ts | 19 +++++++++++ .../codeFixInferFromUsageRestParamJS.ts | 25 ++++++++++++++ .../fourslash/codeFixInferFromUsageSetter2.ts | 11 ------ .../codeFixInferFromUsageSetterJS.ts | 22 ++++++++++++ ...erFromUsageSetterWithInaccessibleTypeJS.ts | 34 +++++++++++++++++++ ...FixInferFromUsageStringIndexSignatureJS.ts | 21 ++++++++++++ .../codeFixInferFromUsageVariable2JS.ts | 18 ++++++++++ .../codeFixInferFromUsageVariableJS.ts | 14 ++++++++ 10 files changed, 180 insertions(+), 13 deletions(-) create mode 100644 tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts delete mode 100644 tests/cases/fourslash/codeFixInferFromUsageSetter2.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageVariable2JS.ts create mode 100644 tests/cases/fourslash/codeFixInferFromUsageVariableJS.ts diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index d840364b03ddd..6aeaf901d9981 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -166,7 +166,7 @@ namespace ts.codefix { } zipWith(containingFunction.parameters, types, (parameter, [type, isOptionalParameter]) => { - if (!parameter.type && !parameter.initializer) { + if (!(parameter.type || isInJSFile(parameter) && getJSDocType(parameter)) && !parameter.initializer) { annotate(changes, sourceFile, parameter, type, program, isOptionalParameter); } }); @@ -293,7 +293,7 @@ namespace ts.codefix { return [undefined, false] as [undefined, boolean]; } const type = checker.getWidenedType(checker.getUnionType(types, UnionReduction.Subtype)); - return [isRest ? checker.createArrayType(type) : type, isOptional] as [Type, boolean]; + return [isRest ? checker.createArrayType(type) : type, isOptional && !isRest] as [Type, boolean]; }); } diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts new file mode 100644 index 0000000000000..d131612dcf9f5 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts @@ -0,0 +1,25 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js +/////** @param {number} a */ +////function f(a, [|...rest|]){ +//// a; rest; +////} +////f(1); +////f(2, "s1"); +////f(3, false, "s2"); +////f(4, "s1", "s2", false, "s4"); + +verify.fileAfterCodeFix( +`/** @param {number} a */ +/** @param {(string | boolean)[]} rest */ +function f(a, ...rest){ + a; rest; +} +f(1); +f(2, "s1"); +f(3, false, "s2"); +f(4, "s1", "s2", false, "s4");`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts new file mode 100644 index 0000000000000..25cb66087bb85 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts @@ -0,0 +1,19 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js +/////** @param {number} a */ +////function f(a, [|...rest |]){ +//// a; +//// rest.push(22); +////} + +verify.fileAfterCodeFix( +`/** @param {number} a */ +/** @param {number[]} rest */ +function f(a, ...rest){ + a; + rest.push(22); +}`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts new file mode 100644 index 0000000000000..3c80f5df45d87 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts @@ -0,0 +1,25 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js +/////** @param {number} a */ +////function f(a: number, [|...rest|]){ +//// a; rest; +////} +////f(1); +////f(2, "s1"); +////f(3, "s1", "s2"); +////f(3, "s1", "s2", "s3", "s4"); + +verify.fileAfterCodeFix( +`/** @param {number} a */ +/** @param {string[]} rest */ +function f(a: number, ...rest){ + a; rest; +} +f(1); +f(2, "s1"); +f(3, "s1", "s2"); +f(3, "s1", "s2", "s3", "s4");`, undefined, 4); diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetter2.ts b/tests/cases/fourslash/codeFixInferFromUsageSetter2.ts deleted file mode 100644 index d058bc2787c7d..0000000000000 --- a/tests/cases/fourslash/codeFixInferFromUsageSetter2.ts +++ /dev/null @@ -1,11 +0,0 @@ -/// - -// @noImplicitAny: true -////class C { -//// set [|x(v)|] { -//// v; -//// } -////} -////(new C).x = 1; - -verify.rangeAfterCodeFix("x(v: number)", undefined, undefined, 1); \ No newline at end of file diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts new file mode 100644 index 0000000000000..a3e1272a58dd9 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts @@ -0,0 +1,22 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @Filename: important.js +////class C { +//// set [|x(v)|] { +//// v; +//// } +////} +////(new C).x = 1; + +verify.fileAfterCodeFix( +` +class C { + /** @param {number} v */ + set x(v) { + v; + } +} +(new C).x = 1;`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts new file mode 100644 index 0000000000000..a085c9dc384a7 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts @@ -0,0 +1,34 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noEmit: true +// @noImplicitAny: true + +// @Filename: /promise.d.ts +////interface Promise { +////} +////declare var Promise: Promise; + +// @Filename: /a.js +////export class D {} +////export default new D(); + +// @Filename: /b.js +////export class C { +//// set [|x|](val) { val; } +//// method() { this.x = import("./a"); } +////} + +goTo.file("/b.js"); +verify.codeFix({ + index: 2, + description: "Infer type of 'x' from usage", + newFileContent: +`export class C { + /** @param {Promise} val */ + set x(val) { val; } + method() { this.x = import("./a"); } +}`, +}); + diff --git a/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts b/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts new file mode 100644 index 0000000000000..31c91b7934c91 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts @@ -0,0 +1,21 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noEmit: true +// @noImplicitAny: true +// @Filename: important.js + +////function f([|a|]) { +//// return a['hi']; +////} + +verify.codeFix({ + index: 2, + description: "Infer parameter types from usage", + newFileContent: +`/** @param {{ [x: string]: any; }} a */ +function f(a) { + return a['hi']; +}`, +}); diff --git a/tests/cases/fourslash/codeFixInferFromUsageVariable2JS.ts b/tests/cases/fourslash/codeFixInferFromUsageVariable2JS.ts new file mode 100644 index 0000000000000..0e88a30b96bee --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageVariable2JS.ts @@ -0,0 +1,18 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noEmit: true +// @noImplicitAny: true +// @Filename: important.js +////[|var x; +////function f() { +//// x++; +////}|] + +verify.rangeAfterCodeFix(`/** @type {number} */ +var x; +function f() { + x++; +} +`, /*includeWhiteSpace*/ undefined, /*errorCode*/ undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageVariableJS.ts b/tests/cases/fourslash/codeFixInferFromUsageVariableJS.ts new file mode 100644 index 0000000000000..8e96fd0891616 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageVariableJS.ts @@ -0,0 +1,14 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noEmit: true +// @noImplicitAny: true +// @Filename: important.js + +////[|var x;|] +////function f() { +//// x++; +////} + +verify.rangeAfterCodeFix("/** @type {number } */\nvar x;", /*includeWhiteSpace*/ undefined, /*errorCode*/ undefined, 2); From ae062b2fe98d14d0eedbc2d8aab10f9763704b74 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 5 Oct 2018 09:58:37 -0700 Subject: [PATCH 07/18] Correct location of comments --- src/services/textChanges.ts | 31 ++++++++++++++----- .../codeFixInferFromUsageMemberJS.ts | 12 ++++++- .../codeFixInferFromUsageSingleLineClassJS.ts | 17 ++++++++++ 3 files changed, 52 insertions(+), 8 deletions(-) create mode 100644 tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 79a5731fca7a4..41a4f3b1af5a3 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -348,6 +348,15 @@ namespace ts.textChanges { this.insertText(sourceFile, token.getStart(sourceFile), text); } + public insertCommentThenNewline(sourceFile: SourceFile, character: number, position: number, commentText: string, kind = CommentKind.Single): void { + const token = getTouchingToken(sourceFile, position); + const completeCommentText = kind === CommentKind.Jsdoc ? `/**${commentText}*/` : + kind === CommentKind.Multi ? `/*${commentText}*/` : + `//${commentText}`; + const text = completeCommentText + this.newLineCharacter + " ".repeat(character); + this.insertText(sourceFile, token.getStart(sourceFile), text); + } + public replaceRangeWithText(sourceFile: SourceFile, range: TextRange, text: string) { this.changes.push({ kind: ChangeKind.Text, sourceFile, range, text }); } @@ -382,15 +391,23 @@ namespace ts.textChanges { * TODO: Might need to only disallow node from being a parameterdecl, propertydecl, propertysig * (or correctly handle parameterdecl by walking up and adding a param, at least) */ - public tryInsertJSDocType(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode, isOptionalParameter: boolean | undefined): void { - // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, the line before might be the wrong place, etc) + public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode, isOptionalParameter: boolean | undefined): void { + // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, etc) // TODO: Parameter probably shouldn't need to manually unescape its text const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); - const commentText = - isParameter(node) && isIdentifier(node.name) ? this.printJSDocParameter(printed, node.name, isOptionalParameter) : - isGetAccessorDeclaration(node) ? ` @return {${printed}} ` : - ` @type {${printed}} `; - this.insertCommentBeforeLine(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.pos + 1).line, node.pos, commentText, CommentKind.Jsdoc); + let commentText; + if (isParameter(node) && isIdentifier(node.name)) { + commentText = this.printJSDocParameter(printed, node.name, isOptionalParameter); + node = node.parent; + } + else if (isGetAccessorDeclaration(node)) { + commentText = ` @return {${printed}} `; + } + else { + commentText = ` @type {${printed}} `; + node = node.parent; + } + this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart()).character, node.getStart(), commentText, CommentKind.Jsdoc); // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones } diff --git a/tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts b/tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts index 95c64652ff335..cb86347cb9f1e 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageMemberJS.ts @@ -17,4 +17,14 @@ // Note: Should be number[] | undefined, but inference currently privileges assignments // over usage (even when the only result is undefined) and infers only undefined. -verify.rangeAfterCodeFix("/** @type {undefined} */\n this.p", undefined, undefined, 2); +verify.fileAfterCodeFix( +`class C { + constructor() { + /** @type {undefined} */ + this.p = undefined; + } + method() { + this.p.push(1) + } +} +`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts new file mode 100644 index 0000000000000..99163fc737c31 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts @@ -0,0 +1,17 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noEmit: true +// @noImplicitAny: true +// @Filename: important.js +////class C {m(x) {return x;}} +////var c = new C() +////c.m(1) + +verify.fileAfterCodeFix( +` +class C {/** @param {number} x */ + m(x) {return x;}} +var c = new C() +c.m(1)`, undefined, 2); From 69dec3f7ad975ff94a0587e3ce8593f4f36a6d18 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 5 Oct 2018 14:25:36 -0700 Subject: [PATCH 08/18] Handle @param blocks 1. Format multiple params nicely in a single-multiline block. 2. Generate only params that haven't already been documented. Existing documentation is not touched. --- src/services/codefixes/inferFromUsage.ts | 40 +++++++++++++++---- src/services/textChanges.ts | 32 +++++++++------ .../fourslash/codeFixInferFromUsageCallJS.ts | 4 +- ...deFixInferFromUsageMultipleParametersJS.ts | 12 +++--- ...FixInferFromUsageNumberIndexSignatureJS.ts | 4 +- .../codeFixInferFromUsageOptionalParamJS.ts | 4 +- ...FixInferFromUsagePartialParameterListJS.ts | 29 ++++++++++++++ .../codeFixInferFromUsagePropertyAccessJS.ts | 8 ++-- .../codeFixInferFromUsageRestParam2JS.ts | 4 +- .../codeFixInferFromUsageRestParam3JS.ts | 4 +- .../codeFixInferFromUsageRestParamJS.ts | 4 +- .../codeFixInferFromUsageSetterJS.ts | 4 +- ...erFromUsageSetterWithInaccessibleTypeJS.ts | 4 +- .../codeFixInferFromUsageSingleLineClassJS.ts | 4 +- ...FixInferFromUsageStringIndexSignatureJS.ts | 4 +- .../fourslash/codeFixInferFromUsage_allJS.ts | 10 +++-- 16 files changed, 131 insertions(+), 40 deletions(-) create mode 100644 tests/cases/fourslash/codeFixInferFromUsagePartialParameterListJS.ts diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index 6aeaf901d9981..67dba4ca9961a 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -165,11 +165,29 @@ namespace ts.codefix { return; } - zipWith(containingFunction.parameters, types, (parameter, [type, isOptionalParameter]) => { - if (!(parameter.type || isInJSFile(parameter) && getJSDocType(parameter)) && !parameter.initializer) { - annotate(changes, sourceFile, parameter, type, program, isOptionalParameter); + if (isInJSFile(containingFunction)) { + annotateJSDocParameters(changes, sourceFile, containingFunction.parameters, types, program); + } + else { + zipWith(containingFunction.parameters, types, (parameter, typair) => { + if (!parameter.type && !parameter.initializer) { + annotate(changes, sourceFile, parameter, typair[0], program); + } + }); + } + } + + function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, params: NodeArray, types: [Type | undefined, boolean][], program: Program): void { + const triples = []; + for (let i = 0; i < params.length; i++) { + const param = params[i]; + const [t, opt] = types[i]; + const typeNode = t && getTypeNodeIfAccessible(t, param, program.getTypeChecker()); + if (!(param.type || isInJSFile(param) && getJSDocType(param)) && !param.initializer && typeNode) { + triples.push([param, typeNode, opt] as [ParameterDeclaration, TypeNode, boolean]); } - }); + } + changes.tryInsertJSDocParams(sourceFile, triples); } function annotateSetAccessor(changes: textChanges.ChangeTracker, sourceFile: SourceFile, setAccessorDeclaration: SetAccessorDeclaration, program: Program, cancellationToken: CancellationToken): void { @@ -177,13 +195,21 @@ namespace ts.codefix { if (param && isIdentifier(setAccessorDeclaration.name) && isIdentifier(param.name)) { const type = inferTypeForVariableFromUsage(setAccessorDeclaration.name, program, cancellationToken) || inferTypeForVariableFromUsage(param.name, program, cancellationToken); - annotate(changes, sourceFile, param, type, program); + if (isInJSFile(setAccessorDeclaration)) { + const typeNode = type && getTypeNodeIfAccessible(type, param, program.getTypeChecker()); + if (typeNode) { + changes.tryInsertJSDocParams(sourceFile, [[param, typeNode, /*isOptionalParameter*/ false]]); + } + } + else { + annotate(changes, sourceFile, param, type, program); + } } } - function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program, isOptionalParameter?: boolean): void { + function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program): void { const typeNode = type && getTypeNodeIfAccessible(type, declaration, program.getTypeChecker()); - if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode, isOptionalParameter); + if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode); } function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, checker: TypeChecker): TypeNode | undefined { diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 41a4f3b1af5a3..d826fa1e25124 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -365,10 +365,24 @@ namespace ts.textChanges { this.replaceRangeWithText(sourceFile, createRange(pos), text); } + public tryInsertJSDocParams(sourceFile: SourceFile, annotations: [ParameterDeclaration, TypeNode, boolean][]) { + const parent = annotations[0][0].parent; + const indent = getLineAndCharacterOfPosition(sourceFile, parent.getStart()).character; + let commentText = "\n"; + for (const [node, type, isOptionalParameter] of annotations) { + if (isIdentifier(node.name)) { + const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); + commentText += this.printJSDocParameter(indent, printed, node.name, isOptionalParameter); + } + } + commentText += " ".repeat(indent + 1); + this.insertCommentThenNewline(sourceFile, indent, parent.getStart(), commentText, CommentKind.Jsdoc); + } + /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ - public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode, isOptionalParameter?: boolean): void { + public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode): void { if (isInJSFile(sourceFile) && topNode.kind !== SyntaxKind.PropertySignature) { - return this.tryInsertJSDocType(sourceFile, topNode, type, isOptionalParameter); + return this.tryInsertJSDocType(sourceFile, topNode, type); } const node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; let endNode: Node | undefined; @@ -391,16 +405,10 @@ namespace ts.textChanges { * TODO: Might need to only disallow node from being a parameterdecl, propertydecl, propertysig * (or correctly handle parameterdecl by walking up and adding a param, at least) */ - public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode, isOptionalParameter: boolean | undefined): void { - // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, etc) - // TODO: Parameter probably shouldn't need to manually unescape its text + public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void { const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); let commentText; - if (isParameter(node) && isIdentifier(node.name)) { - commentText = this.printJSDocParameter(printed, node.name, isOptionalParameter); - node = node.parent; - } - else if (isGetAccessorDeclaration(node)) { + if (isGetAccessorDeclaration(node)) { commentText = ` @return {${printed}} `; } else { @@ -411,12 +419,12 @@ namespace ts.textChanges { // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones } - private printJSDocParameter(printed: string, name: Identifier, isOptionalParameter: boolean | undefined) { + private printJSDocParameter(indent: number, printed: string, name: Identifier, isOptionalParameter: boolean | undefined) { let printName = unescapeLeadingUnderscores(name.escapedText); if (isOptionalParameter) { printName = `[${printName}]`; } - return ` @param {${printed}} ${printName} `; + return " ".repeat(indent) + ` * @param {${printed}} ${printName}\n`; } public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray): void { diff --git a/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts b/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts index 051446c2ff243..fa7eb5ee67130 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts @@ -12,7 +12,9 @@ verify.codeFixAll({ fixId: "inferFromUsage", fixAllDescription: "Infer all types from usage", newFileContent: -`/** @param {() => void} b */ +`/** + * @param {() => void} b + */ function wat(b) { b(); }`}); diff --git a/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts b/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts index f29f01be3b7ff..8f544df432369 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts @@ -11,11 +11,13 @@ verify.fileAfterCodeFix( -`/** @param {number} a */ -/** @param {string} b */ -/** @param {{ a: number; }} c */ -/** @param {{ shouldNotBeHere: number; }} d */ -/** @param {(string | number)[]} d */ +`/** + * @param {number} a + * @param {string} b + * @param {{ a: number; }} c + * @param {{ shouldNotBeHere: number; }} d + * @param {(string | number)[]} d + */ function f(a, b, c, d, e = 0, ...d ) { } f(1, "string", { a: 1 }, {shouldNotBeHere: 2}, {shouldNotBeHere: 2}, 3, "string");`, undefined, 6); diff --git a/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts b/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts index 428a02f49354d..b6b8fe4c25fc8 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts @@ -9,7 +9,9 @@ ////} verify.fileAfterCodeFix( -`/** @param {number[]} a */ +`/** + * @param {number[]} a + */ function f(a) { return a[0] + 1; }`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts b/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts index 4286d078abf8b..cc00c1ddb5d75 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts @@ -11,7 +11,9 @@ ////f(1); verify.fileAfterCodeFix( -`/** @param {number} [a] */ +`/** + * @param {number} [a] + */ function f(a) { a; } diff --git a/tests/cases/fourslash/codeFixInferFromUsagePartialParameterListJS.ts b/tests/cases/fourslash/codeFixInferFromUsagePartialParameterListJS.ts new file mode 100644 index 0000000000000..67795b6a17818 --- /dev/null +++ b/tests/cases/fourslash/codeFixInferFromUsagePartialParameterListJS.ts @@ -0,0 +1,29 @@ +/// + +// @allowJs: true +// @checkJs: true +// @noImplicitAny: true +// @strictNullChecks: true +// @Filename: important.js +/////** +//// * @param {*} y +//// */ +////function f(x, y, z) { +//// return x +////} +////f(1, 2, 3) + +verify.fileAfterCodeFix( +` +/** + * @param {*} y + */ +/** + * @param {number} x + * @param {number} z + */ +function f(x, y, z) { + return x +} +f(1, 2, 3) +`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts b/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts index b363da5151db6..a52c66312fc00 100644 --- a/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts @@ -17,9 +17,11 @@ ////} verify.fileAfterCodeFix( -`/** @param {{ b: { c: any; }; }} a */ -/** @param {{ n: () => number; }} m */ -/** @param {{ y: { z: number[]; }; }} x */ +`/** + * @param {{ b: { c: any; }; }} a + * @param {{ n: () => number; }} m + * @param {{ y: { z: number[]; }; }} x + */ function foo(a, m, x) { a.b.c; diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts index d131612dcf9f5..0504816c59724 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts @@ -15,7 +15,9 @@ verify.fileAfterCodeFix( `/** @param {number} a */ -/** @param {(string | boolean)[]} rest */ +/** + * @param {(string | boolean)[]} rest + */ function f(a, ...rest){ a; rest; } diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts index 25cb66087bb85..bda7090ba06d3 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts @@ -12,7 +12,9 @@ verify.fileAfterCodeFix( `/** @param {number} a */ -/** @param {number[]} rest */ +/** + * @param {number[]} rest + */ function f(a, ...rest){ a; rest.push(22); diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts index 3c80f5df45d87..36582bfa7dbcb 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts @@ -15,7 +15,9 @@ verify.fileAfterCodeFix( `/** @param {number} a */ -/** @param {string[]} rest */ +/** + * @param {string[]} rest + */ function f(a: number, ...rest){ a; rest; } diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts index a3e1272a58dd9..71621f48239d3 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts @@ -14,7 +14,9 @@ verify.fileAfterCodeFix( ` class C { - /** @param {number} v */ + /** + * @param {number} v + */ set x(v) { v; } diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts index a085c9dc384a7..2c944eb3cc043 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts @@ -26,7 +26,9 @@ verify.codeFix({ description: "Infer type of 'x' from usage", newFileContent: `export class C { - /** @param {Promise} val */ + /** + * @param {Promise} val + */ set x(val) { val; } method() { this.x = import("./a"); } }`, diff --git a/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts index 99163fc737c31..f78a70aa14abf 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts @@ -11,7 +11,9 @@ verify.fileAfterCodeFix( ` -class C {/** @param {number} x */ +class C {/** + * @param {number} x + */ m(x) {return x;}} var c = new C() c.m(1)`, undefined, 2); diff --git a/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts b/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts index 31c91b7934c91..5182e80969e91 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts @@ -14,7 +14,9 @@ verify.codeFix({ index: 2, description: "Infer parameter types from usage", newFileContent: -`/** @param {{ [x: string]: any; }} a */ +`/** + * @param {{ [x: string]: any; }} a + */ function f(a) { return a['hi']; }`, diff --git a/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts b/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts index d3bf66853f8de..00e44660a9848 100644 --- a/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts @@ -24,14 +24,18 @@ verify.codeFixAll({ fixId: "inferFromUsage", fixAllDescription: "Infer all types from usage", newFileContent: -`/** @param {number} x */ -/** @param {string} y */ +`/** + * @param {number} x + * @param {string} y + */ function f(x, y) { x += 0; y += ""; } -/** @param {number} z */ +/** + * @param {number} z + */ function g(z) { return z * 2; } From b469d9f8acc8a9ec46a6fd06cb9ad7b6310e8a89 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 5 Oct 2018 14:32:54 -0700 Subject: [PATCH 09/18] Re-add isGet/SetAccessor -- it is part of the API --- src/compiler/checker.ts | 2 +- src/compiler/transformers/declarations/diagnostics.ts | 8 ++++---- src/compiler/utilities.ts | 7 +++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 831eb328d058b..ead2b14410583 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -28389,7 +28389,7 @@ namespace ts { function isImplementationOfOverload(node: SignatureDeclaration) { if (nodeIsPresent((node as FunctionLikeDeclaration).body)) { - if (isGetAccessorDeclaration(node) || isSetAccessorDeclaration(node)) return false; // Get or set accessors can never be overload implementations, but can have up to 2 signatures + if (isGetAccessor(node) || isSetAccessor(node)) return false; // Get or set accessors can never be overload implementations, but can have up to 2 signatures const symbol = getSymbolOfNode(node); const signaturesOfSymbol = getSignaturesOfSymbol(symbol); // If this function body corresponds to function with multiple signature, it is implementation of overload diff --git a/src/compiler/transformers/declarations/diagnostics.ts b/src/compiler/transformers/declarations/diagnostics.ts index 7a61661c1f384..94d6994f8d2ef 100644 --- a/src/compiler/transformers/declarations/diagnostics.ts +++ b/src/compiler/transformers/declarations/diagnostics.ts @@ -33,8 +33,8 @@ namespace ts { isPropertyDeclaration(node) || isPropertySignature(node) || isBindingElement(node) || - isSetAccessorDeclaration(node) || - isGetAccessorDeclaration(node) || + isSetAccessor(node) || + isGetAccessor(node) || isConstructSignatureDeclaration(node) || isCallSignatureDeclaration(node) || isMethodDeclaration(node) || @@ -50,7 +50,7 @@ namespace ts { } export function createGetSymbolAccessibilityDiagnosticForNodeName(node: DeclarationDiagnosticProducing) { - if (isSetAccessorDeclaration(node) || isGetAccessorDeclaration(node)) { + if (isSetAccessor(node) || isGetAccessor(node)) { return getAccessorNameVisibilityError; } else if (isMethodSignature(node) || isMethodDeclaration(node)) { @@ -126,7 +126,7 @@ namespace ts { if (isVariableDeclaration(node) || isPropertyDeclaration(node) || isPropertySignature(node) || isBindingElement(node) || isConstructorDeclaration(node)) { return getVariableDeclarationTypeVisibilityError; } - else if (isSetAccessorDeclaration(node) || isGetAccessorDeclaration(node)) { + else if (isSetAccessor(node) || isGetAccessor(node)) { return getAccessorDeclarationTypeVisibilityError; } else if (isConstructSignatureDeclaration(node) || isCallSignatureDeclaration(node) || isMethodDeclaration(node) || isMethodSignature(node) || isFunctionDeclaration(node) || isIndexSignatureDeclaration(node)) { diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 3f3c9f6237ab3..94cf71a772fc9 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -6727,6 +6727,13 @@ namespace ts { return !!jsDoc && jsDoc.length > 0; } + export function isSetAccessor(node: Node): node is SetAccessorDeclaration { + return node.kind === SyntaxKind.SetAccessor; + } + export function isGetAccessor(node: Node): node is GetAccessorDeclaration { + return node.kind === SyntaxKind.GetAccessor; + } + /** True if has type node attached to it. */ /* @internal */ export function hasType(node: Node): node is HasType { From 56bcf3f5f2313da7f21c6bc5bc0e61e5219ba9ee Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 5 Oct 2018 14:34:29 -0700 Subject: [PATCH 10/18] Move isSet/GetAccessor back to the original location --- src/compiler/utilities.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 94cf71a772fc9..d7e1e2e629129 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -6719,6 +6719,13 @@ namespace ts { return node.kind >= SyntaxKind.FirstJSDocTagNode && node.kind <= SyntaxKind.LastJSDocTagNode; } + export function isSetAccessor(node: Node): node is SetAccessorDeclaration { + return node.kind === SyntaxKind.SetAccessor; + } + export function isGetAccessor(node: Node): node is GetAccessorDeclaration { + return node.kind === SyntaxKind.GetAccessor; + } + /** True if has jsdoc nodes attached to it. */ /* @internal */ // TODO: GH#19856 Would like to return `node is Node & { jsDoc: JSDoc[] }` but it causes long compile times @@ -6727,13 +6734,6 @@ namespace ts { return !!jsDoc && jsDoc.length > 0; } - export function isSetAccessor(node: Node): node is SetAccessorDeclaration { - return node.kind === SyntaxKind.SetAccessor; - } - export function isGetAccessor(node: Node): node is GetAccessorDeclaration { - return node.kind === SyntaxKind.GetAccessor; - } - /** True if has type node attached to it. */ /* @internal */ export function hasType(node: Node): node is HasType { From a3aae7bb90a44c64493449280769a596cb2dba16 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 5 Oct 2018 14:36:48 -0700 Subject: [PATCH 11/18] Oh no I missed a newline and a space --- src/compiler/utilities.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index d7e1e2e629129..9c6d9002b3c79 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -6722,7 +6722,8 @@ namespace ts { export function isSetAccessor(node: Node): node is SetAccessorDeclaration { return node.kind === SyntaxKind.SetAccessor; } - export function isGetAccessor(node: Node): node is GetAccessorDeclaration { + + export function isGetAccessor(node: Node): node is GetAccessorDeclaration { return node.kind === SyntaxKind.GetAccessor; } From d22961a9ba94a2c564e1622f74ec552684d225f6 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 5 Oct 2018 15:44:33 -0700 Subject: [PATCH 12/18] Switch to an object type --- src/services/codefixes/inferFromUsage.ts | 61 +++++++++++++----------- src/services/textChanges.ts | 18 ++++--- 2 files changed, 41 insertions(+), 38 deletions(-) diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index 67dba4ca9961a..74170d172abcc 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -1,5 +1,11 @@ /* @internal */ namespace ts.codefix { + export interface FakeSymbol { + declaration?: Node; + type?: Type; + typeNode?: TypeNode; + isOptional?: boolean; + } const fixId = "inferFromUsage"; const errorCodes = [ // Variable declarations @@ -65,15 +71,13 @@ namespace ts.codefix { // Variable and Property declarations case Diagnostics.Member_0_implicitly_has_an_1_type.code: case Diagnostics.Variable_0_implicitly_has_type_1_in_some_locations_where_its_type_cannot_be_determined.code: - // TODO: Here! if ((isVariableDeclaration(parent) && markSeen(parent)) || isPropertyDeclaration(parent) || isPropertySignature(parent)) { // handle bad location annotateVariableDeclaration(changes, sourceFile, parent, program, cancellationToken); return parent; } if (isPropertyAccessExpression(parent)) { - annotateThisPropertyAssignment(changes, sourceFile, parent, program, cancellationToken); + annotate(changes, sourceFile, parent, inferTypeForVariableFromUsage(parent.name, program, cancellationToken), program); return parent; - // Try jumping! } return undefined; @@ -136,10 +140,6 @@ namespace ts.codefix { } } - function annotateThisPropertyAssignment(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: PropertyAccessExpression, program: Program, cancellationToken: CancellationToken): void { - annotate(changes, sourceFile, declaration, inferTypeForVariableFromUsage(declaration.name, program, cancellationToken), program); - } - function isApplicableFunctionForInference(declaration: FunctionLike): declaration is MethodDeclaration | FunctionDeclaration | ConstructorDeclaration { switch (declaration.kind) { case SyntaxKind.FunctionDeclaration: @@ -157,37 +157,38 @@ namespace ts.codefix { return; } - const types = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) || - - containingFunction.parameters.map(p => [isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined, /*isOptionalParameter*/ false] as [Type | undefined, boolean]); + const symbols = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) || + containingFunction.parameters.map(p => ({ + declaration: p, + type: isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined + } as FakeSymbol)); // We didn't actually find a set of type inference positions matching each parameter position - if (containingFunction.parameters.length !== types.length) { + if (containingFunction.parameters.length !== symbols.length) { return; } if (isInJSFile(containingFunction)) { - annotateJSDocParameters(changes, sourceFile, containingFunction.parameters, types, program); + annotateJSDocParameters(changes, sourceFile, symbols, program); } else { - zipWith(containingFunction.parameters, types, (parameter, typair) => { + zipWith(containingFunction.parameters, symbols, (parameter, { type }) => { if (!parameter.type && !parameter.initializer) { - annotate(changes, sourceFile, parameter, typair[0], program); + annotate(changes, sourceFile, parameter, type, program); } }); } } - function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, params: NodeArray, types: [Type | undefined, boolean][], program: Program): void { - const triples = []; - for (let i = 0; i < params.length; i++) { - const param = params[i]; - const [t, opt] = types[i]; - const typeNode = t && getTypeNodeIfAccessible(t, param, program.getTypeChecker()); - if (!(param.type || isInJSFile(param) && getJSDocType(param)) && !param.initializer && typeNode) { - triples.push([param, typeNode, opt] as [ParameterDeclaration, TypeNode, boolean]); + function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbols: FakeSymbol[], program: Program): void { + const result = []; + for (const symbol of symbols) { + const param = symbol.declaration as ParameterDeclaration; + const typeNode = symbol.type && getTypeNodeIfAccessible(symbol.type, param, program.getTypeChecker()); + if (typeNode && !param.initializer && !getJSDocType(param)) { + result.push({ ...symbol, typeNode }); } } - changes.tryInsertJSDocParams(sourceFile, triples); + changes.tryInsertJSDocParams(sourceFile, result); } function annotateSetAccessor(changes: textChanges.ChangeTracker, sourceFile: SourceFile, setAccessorDeclaration: SetAccessorDeclaration, program: Program, cancellationToken: CancellationToken): void { @@ -198,7 +199,7 @@ namespace ts.codefix { if (isInJSFile(setAccessorDeclaration)) { const typeNode = type && getTypeNodeIfAccessible(type, param, program.getTypeChecker()); if (typeNode) { - changes.tryInsertJSDocParams(sourceFile, [[param, typeNode, /*isOptionalParameter*/ false]]); + changes.tryInsertJSDocParams(sourceFile, [{ declaration: param, typeNode, isOptional: false }]); } } else { @@ -237,7 +238,7 @@ namespace ts.codefix { return InferFromReference.inferTypeFromReferences(getReferences(token, program, cancellationToken), program.getTypeChecker(), cancellationToken); } - function inferTypeForParametersFromUsage(containingFunction: FunctionLikeDeclaration, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): ([Type | undefined, boolean])[] | undefined { + function inferTypeForParametersFromUsage(containingFunction: FunctionLikeDeclaration, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): FakeSymbol[] | undefined { switch (containingFunction.kind) { case SyntaxKind.Constructor: case SyntaxKind.FunctionExpression: @@ -280,7 +281,7 @@ namespace ts.codefix { return getTypeFromUsageContext(usageContext, checker); } - export function inferTypeForParametersFromReferences(references: ReadonlyArray, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): ([Type | undefined, boolean])[] | undefined { + export function inferTypeForParametersFromReferences(references: ReadonlyArray, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): FakeSymbol[] | undefined { if (references.length === 0) { return undefined; } @@ -316,10 +317,14 @@ namespace ts.codefix { } } if (!types.length) { - return [undefined, false] as [undefined, boolean]; + return { type: undefined, isOptional: false }; } const type = checker.getWidenedType(checker.getUnionType(types, UnionReduction.Subtype)); - return [isRest ? checker.createArrayType(type) : type, isOptional && !isRest] as [Type, boolean]; + return { + type: isRest ? checker.createArrayType(type) : type, + isOptional: isOptional && !isRest, + declaration: parameter + }; }); } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index d826fa1e25124..9e5a9ca471937 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -365,14 +365,15 @@ namespace ts.textChanges { this.replaceRangeWithText(sourceFile, createRange(pos), text); } - public tryInsertJSDocParams(sourceFile: SourceFile, annotations: [ParameterDeclaration, TypeNode, boolean][]) { - const parent = annotations[0][0].parent; + public tryInsertJSDocParams(sourceFile: SourceFile, annotations: codefix.FakeSymbol[]) { + const parent = (annotations[0].declaration!).parent; const indent = getLineAndCharacterOfPosition(sourceFile, parent.getStart()).character; let commentText = "\n"; - for (const [node, type, isOptionalParameter] of annotations) { + for (const { declaration, typeNode: type, isOptional } of annotations) { + const node = declaration as ParameterDeclaration; if (isIdentifier(node.name)) { - const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); - commentText += this.printJSDocParameter(indent, printed, node.name, isOptionalParameter); + const printed = createPrinter().printNode(EmitHint.Unspecified, type!, sourceFile); + commentText += this.printJSDocParameter(indent, printed, node.name, isOptional); } } commentText += " ".repeat(indent + 1); @@ -382,6 +383,7 @@ namespace ts.textChanges { /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode): void { if (isInJSFile(sourceFile) && topNode.kind !== SyntaxKind.PropertySignature) { + // TODO: Lift this out into callers (and maybe tryInsertJSDocType too?) return this.tryInsertJSDocType(sourceFile, topNode, type); } const node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; @@ -401,10 +403,6 @@ namespace ts.textChanges { this.insertNodeAt(sourceFile, endNode.end, type, { prefix: ": " }); } - /** TODO: Try allowing insertion of other things too - * TODO: Might need to only disallow node from being a parameterdecl, propertydecl, propertysig - * (or correctly handle parameterdecl by walking up and adding a param, at least) - */ public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void { const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); let commentText; @@ -415,8 +413,8 @@ namespace ts.textChanges { commentText = ` @type {${printed}} `; node = node.parent; } + // before this: move this to inferFromUsage? this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart()).character, node.getStart(), commentText, CommentKind.Jsdoc); - // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones } private printJSDocParameter(indent: number, printed: string, name: Identifier, isOptionalParameter: boolean | undefined) { From 3cc99ea9059dff09d420b92d10075bb27ec2aaf6 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 5 Oct 2018 16:29:53 -0700 Subject: [PATCH 13/18] A lot of cleanup More to come, though. annotate is only called in annotateVariableDeclaration where we don't know whether we're in JS or not. --- src/services/codefixes/inferFromUsage.ts | 54 ++++++++++++-------- src/services/textChanges.ts | 63 +++++++++++------------- 2 files changed, 62 insertions(+), 55 deletions(-) diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index 74170d172abcc..e13aebaa0afb3 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -1,11 +1,5 @@ /* @internal */ namespace ts.codefix { - export interface FakeSymbol { - declaration?: Node; - type?: Type; - typeNode?: TypeNode; - isOptional?: boolean; - } const fixId = "inferFromUsage"; const errorCodes = [ // Variable declarations @@ -76,7 +70,11 @@ namespace ts.codefix { return parent; } if (isPropertyAccessExpression(parent)) { - annotate(changes, sourceFile, parent, inferTypeForVariableFromUsage(parent.name, program, cancellationToken), program); + const type = inferTypeForVariableFromUsage(parent.name, program, cancellationToken); + const typeNode = type && getTypeNodeIfAccessible(type, parent, program.getTypeChecker()); + if (typeNode) { + changes.tryInsertJSDocType(sourceFile, parent, typeNode); + } return parent; } return undefined; @@ -161,7 +159,7 @@ namespace ts.codefix { containingFunction.parameters.map(p => ({ declaration: p, type: isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined - } as FakeSymbol)); + } as ParameterInference)); // We didn't actually find a set of type inference positions matching each parameter position if (containingFunction.parameters.length !== symbols.length) { return; @@ -171,24 +169,24 @@ namespace ts.codefix { annotateJSDocParameters(changes, sourceFile, symbols, program); } else { - zipWith(containingFunction.parameters, symbols, (parameter, { type }) => { - if (!parameter.type && !parameter.initializer) { - annotate(changes, sourceFile, parameter, type, program); + for (const { declaration, type } of symbols) { + if (declaration && !declaration.type && !declaration.initializer) { + annotate(changes, sourceFile, declaration, type, program); } - }); + } } } - function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbols: FakeSymbol[], program: Program): void { + function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbols: ParameterInference[], program: Program): void { const result = []; for (const symbol of symbols) { - const param = symbol.declaration as ParameterDeclaration; + const param = symbol.declaration; const typeNode = symbol.type && getTypeNodeIfAccessible(symbol.type, param, program.getTypeChecker()); if (typeNode && !param.initializer && !getJSDocType(param)) { result.push({ ...symbol, typeNode }); } } - changes.tryInsertJSDocParams(sourceFile, result); + changes.tryInsertJSDocParameters(sourceFile, result); } function annotateSetAccessor(changes: textChanges.ChangeTracker, sourceFile: SourceFile, setAccessorDeclaration: SetAccessorDeclaration, program: Program, cancellationToken: CancellationToken): void { @@ -199,7 +197,7 @@ namespace ts.codefix { if (isInJSFile(setAccessorDeclaration)) { const typeNode = type && getTypeNodeIfAccessible(type, param, program.getTypeChecker()); if (typeNode) { - changes.tryInsertJSDocParams(sourceFile, [{ declaration: param, typeNode, isOptional: false }]); + changes.tryInsertJSDocParameters(sourceFile, [{ declaration: param, typeNode }]); } } else { @@ -210,7 +208,16 @@ namespace ts.codefix { function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program): void { const typeNode = type && getTypeNodeIfAccessible(type, declaration, program.getTypeChecker()); - if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode); + + if (typeNode) { + if (isInJSFile(sourceFile) && declaration.kind !== SyntaxKind.PropertySignature) { + // TODO: Lift this out into callers (and maybe tryInsertJSDocType too?) + changes.tryInsertJSDocType(sourceFile, declaration, typeNode); + } + else { + changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode); + } + } } function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, checker: TypeChecker): TypeNode | undefined { @@ -238,7 +245,7 @@ namespace ts.codefix { return InferFromReference.inferTypeFromReferences(getReferences(token, program, cancellationToken), program.getTypeChecker(), cancellationToken); } - function inferTypeForParametersFromUsage(containingFunction: FunctionLikeDeclaration, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): FakeSymbol[] | undefined { + function inferTypeForParametersFromUsage(containingFunction: FunctionLikeDeclaration, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): ParameterInference[] | undefined { switch (containingFunction.kind) { case SyntaxKind.Constructor: case SyntaxKind.FunctionExpression: @@ -254,6 +261,13 @@ namespace ts.codefix { } } + export interface ParameterInference { + declaration: ParameterDeclaration; + type?: Type; + typeNode?: TypeNode; + isOptional?: boolean; + } + namespace InferFromReference { interface CallContext { argumentTypes: Type[]; @@ -281,7 +295,7 @@ namespace ts.codefix { return getTypeFromUsageContext(usageContext, checker); } - export function inferTypeForParametersFromReferences(references: ReadonlyArray, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): FakeSymbol[] | undefined { + export function inferTypeForParametersFromReferences(references: ReadonlyArray, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): ParameterInference[] | undefined { if (references.length === 0) { return undefined; } @@ -317,7 +331,7 @@ namespace ts.codefix { } } if (!types.length) { - return { type: undefined, isOptional: false }; + return { declaration: parameter }; } const type = checker.getWidenedType(checker.getUnionType(types, UnionReduction.Subtype)); return { diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 9e5a9ca471937..1c64fabc202d4 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -41,12 +41,6 @@ namespace ts.textChanges { Start } - export enum CommentKind { - Single, - Multi, - Jsdoc, - } - function skipWhitespacesAndLineBreaks(text: string, start: number) { return skipTrivia(text, start, /*stopAfterLineBreak*/ false, /*stopAtComments*/ true); } @@ -213,7 +207,7 @@ namespace ts.textChanges { formatContext: formatting.FormatContext; } - export type TypeAnnotatable = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature | PropertyAccessExpression; + export type TypeAnnotatable = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; export class ChangeTracker { private readonly changes: Change[] = []; @@ -331,7 +325,7 @@ namespace ts.textChanges { this.insertNodeAt(sourceFile, pos, createToken(modifier), { prefix: " " }); } - public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string, kind = CommentKind.Single): void { + public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string): void { const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); // First try to see if we can put the comment on the previous line. @@ -341,19 +335,13 @@ namespace ts.textChanges { const insertAtLineStart = isValidLocationToAddComment(sourceFile, startPosition); const token = getTouchingToken(sourceFile, insertAtLineStart ? startPosition : position); const indent = sourceFile.text.slice(lineStartPosition, startPosition); - const completeCommentText = kind === CommentKind.Jsdoc ? `/**${commentText}*/` : - kind === CommentKind.Multi ? `/*${commentText}*/` : - `//${commentText}`; - const text = (insertAtLineStart ? "" : this.newLineCharacter) + completeCommentText + this.newLineCharacter + indent; + const text = `${insertAtLineStart ? "" : this.newLineCharacter}//${commentText}${this.newLineCharacter}${indent}`; this.insertText(sourceFile, token.getStart(sourceFile), text); } - public insertCommentThenNewline(sourceFile: SourceFile, character: number, position: number, commentText: string, kind = CommentKind.Single): void { + public insertCommentThenNewline(sourceFile: SourceFile, character: number, position: number, commentText: string): void { const token = getTouchingToken(sourceFile, position); - const completeCommentText = kind === CommentKind.Jsdoc ? `/**${commentText}*/` : - kind === CommentKind.Multi ? `/*${commentText}*/` : - `//${commentText}`; - const text = completeCommentText + this.newLineCharacter + " ".repeat(character); + const text = "/**" + commentText + "*/" + this.newLineCharacter + this.repeatString(" ", character); this.insertText(sourceFile, token.getStart(sourceFile), text); } @@ -365,28 +353,25 @@ namespace ts.textChanges { this.replaceRangeWithText(sourceFile, createRange(pos), text); } - public tryInsertJSDocParams(sourceFile: SourceFile, annotations: codefix.FakeSymbol[]) { - const parent = (annotations[0].declaration!).parent; + public tryInsertJSDocParameters(sourceFile: SourceFile, parameters: codefix.ParameterInference[]) { + if (parameters.length === 0) { + return; + } + const parent = parameters[0].declaration.parent; const indent = getLineAndCharacterOfPosition(sourceFile, parent.getStart()).character; let commentText = "\n"; - for (const { declaration, typeNode: type, isOptional } of annotations) { - const node = declaration as ParameterDeclaration; - if (isIdentifier(node.name)) { - const printed = createPrinter().printNode(EmitHint.Unspecified, type!, sourceFile); - commentText += this.printJSDocParameter(indent, printed, node.name, isOptional); + for (const { declaration, typeNode, isOptional } of parameters) { + if (isIdentifier(declaration.name)) { + const printed = createPrinter().printNode(EmitHint.Unspecified, typeNode!, sourceFile); + commentText += this.printJSDocParameter(indent, printed, declaration.name, isOptional); } } - commentText += " ".repeat(indent + 1); - this.insertCommentThenNewline(sourceFile, indent, parent.getStart(), commentText, CommentKind.Jsdoc); + commentText += this.repeatString(" ", indent + 1); + this.insertCommentThenNewline(sourceFile, indent, parent.getStart(), commentText); } /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */ - public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode): void { - if (isInJSFile(sourceFile) && topNode.kind !== SyntaxKind.PropertySignature) { - // TODO: Lift this out into callers (and maybe tryInsertJSDocType too?) - return this.tryInsertJSDocType(sourceFile, topNode, type); - } - const node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; + public tryInsertTypeAnnotation(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void { let endNode: Node | undefined; if (isFunctionLike(node)) { endNode = findChildOfKind(node, SyntaxKind.CloseParenToken, sourceFile); @@ -413,8 +398,16 @@ namespace ts.textChanges { commentText = ` @type {${printed}} `; node = node.parent; } - // before this: move this to inferFromUsage? - this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart()).character, node.getStart(), commentText, CommentKind.Jsdoc); + this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart()).character, node.getStart(), commentText); + } + + /** Should only be used to repeat strings a small number of times */ + private repeatString(s: string, n: number) { + let sum = ""; + for (let i = 0; i < n; i++) { + sum += s; + } + return sum; } private printJSDocParameter(indent: number, printed: string, name: Identifier, isOptionalParameter: boolean | undefined) { @@ -422,7 +415,7 @@ namespace ts.textChanges { if (isOptionalParameter) { printName = `[${printName}]`; } - return " ".repeat(indent) + ` * @param {${printed}} ${printName}\n`; + return this.repeatString(" ", indent) + ` * @param {${printed}} ${printName}\n`; } public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray): void { From 31db1ce9ae942e45c1d23fde73301730ac512e6a Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Mon, 8 Oct 2018 08:54:41 -0700 Subject: [PATCH 14/18] Move and delegate to annotateJSDocParameters --- src/services/codefixes/inferFromUsage.ts | 33 ++++++++++-------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index e13aebaa0afb3..4f5ae9a5a1bea 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -177,28 +177,13 @@ namespace ts.codefix { } } - function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbols: ParameterInference[], program: Program): void { - const result = []; - for (const symbol of symbols) { - const param = symbol.declaration; - const typeNode = symbol.type && getTypeNodeIfAccessible(symbol.type, param, program.getTypeChecker()); - if (typeNode && !param.initializer && !getJSDocType(param)) { - result.push({ ...symbol, typeNode }); - } - } - changes.tryInsertJSDocParameters(sourceFile, result); - } - function annotateSetAccessor(changes: textChanges.ChangeTracker, sourceFile: SourceFile, setAccessorDeclaration: SetAccessorDeclaration, program: Program, cancellationToken: CancellationToken): void { const param = firstOrUndefined(setAccessorDeclaration.parameters); if (param && isIdentifier(setAccessorDeclaration.name) && isIdentifier(param.name)) { const type = inferTypeForVariableFromUsage(setAccessorDeclaration.name, program, cancellationToken) || inferTypeForVariableFromUsage(param.name, program, cancellationToken); if (isInJSFile(setAccessorDeclaration)) { - const typeNode = type && getTypeNodeIfAccessible(type, param, program.getTypeChecker()); - if (typeNode) { - changes.tryInsertJSDocParameters(sourceFile, [{ declaration: param, typeNode }]); - } + annotateJSDocParameters(changes, sourceFile, [{ declaration: param, type }], program); } else { annotate(changes, sourceFile, param, type, program); @@ -208,10 +193,8 @@ namespace ts.codefix { function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program): void { const typeNode = type && getTypeNodeIfAccessible(type, declaration, program.getTypeChecker()); - if (typeNode) { if (isInJSFile(sourceFile) && declaration.kind !== SyntaxKind.PropertySignature) { - // TODO: Lift this out into callers (and maybe tryInsertJSDocType too?) changes.tryInsertJSDocType(sourceFile, declaration, typeNode); } else { @@ -220,7 +203,19 @@ namespace ts.codefix { } } - function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, checker: TypeChecker): TypeNode | undefined { + function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbols: ParameterInference[], program: Program): void { + const result = []; + for (const symbol of symbols) { + const param = symbol.declaration; + const typeNode = symbol.type && getTypeNodeIfAccessible(symbol.type, param, program.getTypeChecker()); + if (typeNode && !param.initializer && !getJSDocType(param)) { + result.push({ ...symbol, typeNode }); + } + } + changes.tryInsertJSDocParameters(sourceFile, result); + } + + function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, checker: TypeChecker): TypeNode | undefined { let typeIsAccessible = true; const notAccessible = () => { typeIsAccessible = false; }; const res = checker.typeToTypeNode(type, enclosingScope, /*flags*/ undefined, { From edcb30d6d7ee8c3f371b07acbdc3a46456dea716 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Mon, 8 Oct 2018 15:09:46 -0700 Subject: [PATCH 15/18] Address PR comments --- src/services/codefixes/inferFromUsage.ts | 34 ++++++++++-------------- src/services/textChanges.ts | 31 ++++++++++----------- 2 files changed, 28 insertions(+), 37 deletions(-) diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts index 4f5ae9a5a1bea..e2a2ecef4c4ec 100644 --- a/src/services/codefixes/inferFromUsage.ts +++ b/src/services/codefixes/inferFromUsage.ts @@ -155,21 +155,18 @@ namespace ts.codefix { return; } - const symbols = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) || - containingFunction.parameters.map(p => ({ + const parameterInferences = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) || + containingFunction.parameters.map(p => ({ declaration: p, type: isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined - } as ParameterInference)); - // We didn't actually find a set of type inference positions matching each parameter position - if (containingFunction.parameters.length !== symbols.length) { - return; - } + })); + Debug.assert(containingFunction.parameters.length === parameterInferences.length); if (isInJSFile(containingFunction)) { - annotateJSDocParameters(changes, sourceFile, symbols, program); + annotateJSDocParameters(changes, sourceFile, parameterInferences, program); } else { - for (const { declaration, type } of symbols) { + for (const { declaration, type } of parameterInferences) { if (declaration && !declaration.type && !declaration.initializer) { annotate(changes, sourceFile, declaration, type, program); } @@ -203,15 +200,12 @@ namespace ts.codefix { } } - function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, symbols: ParameterInference[], program: Program): void { - const result = []; - for (const symbol of symbols) { - const param = symbol.declaration; - const typeNode = symbol.type && getTypeNodeIfAccessible(symbol.type, param, program.getTypeChecker()); - if (typeNode && !param.initializer && !getJSDocType(param)) { - result.push({ ...symbol, typeNode }); - } - } + function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parameterInferences: ParameterInference[], program: Program): void { + const result = mapDefined(parameterInferences, inference => { + const param = inference.declaration; + const typeNode = inference.type && getTypeNodeIfAccessible(inference.type, param, program.getTypeChecker()); + return typeNode && !param.initializer && !getJSDocType(param) ? { ...inference, typeNode } : undefined; + }); changes.tryInsertJSDocParameters(sourceFile, result); } @@ -256,7 +250,7 @@ namespace ts.codefix { } } - export interface ParameterInference { + interface ParameterInference { declaration: ParameterDeclaration; type?: Type; typeNode?: TypeNode; @@ -306,10 +300,10 @@ namespace ts.codefix { } const isConstructor = declaration.kind === SyntaxKind.Constructor; const callContexts = isConstructor ? usageContext.constructContexts : usageContext.callContexts; - let isOptional = false; return callContexts && declaration.parameters.map((parameter, parameterIndex) => { const types: Type[] = []; const isRest = isRestParameter(parameter); + let isOptional = false; for (const callContext of callContexts) { if (callContext.argumentTypes.length <= parameterIndex) { isOptional = isInJSFile(declaration); diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 1c64fabc202d4..08ea956a766a8 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -209,6 +209,12 @@ namespace ts.textChanges { export type TypeAnnotatable = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature; + interface JSDocParameter { + declaration: ParameterDeclaration; + typeNode: TypeNode; + isOptional?: boolean; + } + export class ChangeTracker { private readonly changes: Change[] = []; private readonly newFiles: { readonly oldFile: SourceFile | undefined, readonly fileName: string, readonly statements: ReadonlyArray }[] = []; @@ -341,7 +347,7 @@ namespace ts.textChanges { public insertCommentThenNewline(sourceFile: SourceFile, character: number, position: number, commentText: string): void { const token = getTouchingToken(sourceFile, position); - const text = "/**" + commentText + "*/" + this.newLineCharacter + this.repeatString(" ", character); + const text = "/**" + commentText + "*/" + this.newLineCharacter + repeatString(" ", character); this.insertText(sourceFile, token.getStart(sourceFile), text); } @@ -353,7 +359,7 @@ namespace ts.textChanges { this.replaceRangeWithText(sourceFile, createRange(pos), text); } - public tryInsertJSDocParameters(sourceFile: SourceFile, parameters: codefix.ParameterInference[]) { + public tryInsertJSDocParameters(sourceFile: SourceFile, parameters: JSDocParameter[]) { if (parameters.length === 0) { return; } @@ -362,11 +368,11 @@ namespace ts.textChanges { let commentText = "\n"; for (const { declaration, typeNode, isOptional } of parameters) { if (isIdentifier(declaration.name)) { - const printed = createPrinter().printNode(EmitHint.Unspecified, typeNode!, sourceFile); + const printed = changesToText.getNonformattedText(typeNode, sourceFile, this.newLineCharacter).text; commentText += this.printJSDocParameter(indent, printed, declaration.name, isOptional); } } - commentText += this.repeatString(" ", indent + 1); + commentText += repeatString(" ", indent + 1); this.insertCommentThenNewline(sourceFile, indent, parent.getStart(), commentText); } @@ -398,16 +404,7 @@ namespace ts.textChanges { commentText = ` @type {${printed}} `; node = node.parent; } - this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart()).character, node.getStart(), commentText); - } - - /** Should only be used to repeat strings a small number of times */ - private repeatString(s: string, n: number) { - let sum = ""; - for (let i = 0; i < n; i++) { - sum += s; - } - return sum; + this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart(sourceFile)).character, node.getStart(sourceFile), commentText); } private printJSDocParameter(indent: number, printed: string, name: Identifier, isOptionalParameter: boolean | undefined) { @@ -415,7 +412,7 @@ namespace ts.textChanges { if (isOptionalParameter) { printName = `[${printName}]`; } - return this.repeatString(" ", indent) + ` * @param {${printed}} ${printName}\n`; + return repeatString(" ", indent) + ` * @param {${printed}} ${printName}\n`; } public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray): void { @@ -859,7 +856,7 @@ namespace ts.textChanges { } /** Note: output node may be mutated input node. */ - function getNonformattedText(node: Node, sourceFile: SourceFile | undefined, newLineCharacter: string): { text: string, node: Node } { + export function getNonformattedText(node: Node, sourceFile: SourceFile | undefined, newLineCharacter: string): { text: string, node: Node } { const writer = new Writer(newLineCharacter); const newLine = newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed; createPrinter({ newLine, neverAsciiEscape: true }, writer).writeNode(EmitHint.Unspecified, node, sourceFile, writer); @@ -1211,7 +1208,7 @@ namespace ts.textChanges { if (parent.kind === SyntaxKind.CatchClause) { // TODO: There's currently no unused diagnostic for this, could be a suggestion - changes.deleteNodeRange(sourceFile, findChildOfKind(parent, SyntaxKind.OpenParenToken, sourceFile)!, findChildOfKind(parent, SyntaxKind.CloseParenToken, sourceFile)!); + changes.deleteNodeRange(sourceFile, findChildOfKind(parent, SyntaxKind.OpenParenToken, sourceFile)!, findChildOfKind(parent, SyntaxKind.CloseParenToken, sourceFile)!); return; } From d129e32b0a05b996a86c969cfc39383727aa2293 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Mon, 8 Oct 2018 16:03:59 -0700 Subject: [PATCH 16/18] Lint: newline problems!!!! --- src/services/textChanges.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 08ea956a766a8..604646d6bef78 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -1208,7 +1208,7 @@ namespace ts.textChanges { if (parent.kind === SyntaxKind.CatchClause) { // TODO: There's currently no unused diagnostic for this, could be a suggestion - changes.deleteNodeRange(sourceFile, findChildOfKind(parent, SyntaxKind.OpenParenToken, sourceFile)!, findChildOfKind(parent, SyntaxKind.CloseParenToken, sourceFile)!); + changes.deleteNodeRange(sourceFile, findChildOfKind(parent, SyntaxKind.OpenParenToken, sourceFile)!, findChildOfKind(parent, SyntaxKind.CloseParenToken, sourceFile)!); return; } From e3cf7875e445160ea2cc560787b356cff0c1f4d1 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Tue, 9 Oct 2018 10:00:21 -0700 Subject: [PATCH 17/18] Switch another call to getNonformattedText --- src/services/textChanges.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 604646d6bef78..0d45039f5b181 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -395,7 +395,7 @@ namespace ts.textChanges { } public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void { - const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile); + const printed = changesToText.getNonformattedText(type, sourceFile, this.newLineCharacter).text; let commentText; if (isGetAccessorDeclaration(node)) { commentText = ` @return {${printed}} `; From 5066880828c48e6e1233338b318c274f49de1909 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Tue, 9 Oct 2018 10:12:40 -0700 Subject: [PATCH 18/18] Update baseline missed after merge --- .../codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts index 2c944eb3cc043..7620b969234ac 100644 --- a/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts +++ b/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts @@ -27,7 +27,7 @@ verify.codeFix({ newFileContent: `export class C { /** - * @param {Promise} val + * @param {Promise} val */ set x(val) { val; } method() { this.x = import("./a"); }