Skip to content

inferFromUsage codefix now emits JSDoc in JS files #27610

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Oct 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
1bc0cd1
Now adding @type to variable declarations, at least
sandersn Sep 28, 2018
e34bf54
Improve @param output and add test
sandersn Sep 28, 2018
0f4a800
Add some js/inferFromUsage tests and fixes
sandersn Oct 1, 2018
be3577c
Fix @typedef refactor
sandersn Oct 2, 2018
bf5529b
Emit JSDoc optional parameters
sandersn Oct 2, 2018
4ee4d69
Get rest of existing tests working
sandersn Oct 3, 2018
9ae4a99
Merge branch 'master' into js/infer-from-usage
sandersn Oct 4, 2018
09ae612
Merge branch 'master' into js/infer-from-usage
sandersn Oct 4, 2018
e99220f
Merge branch 'master' into js/infer-from-usage
sandersn Oct 5, 2018
ae062b2
Correct location of comments
sandersn Oct 5, 2018
69dec3f
Handle @param blocks
sandersn Oct 5, 2018
b469d9f
Re-add isGet/SetAccessor -- it is part of the API
sandersn Oct 5, 2018
56bcf3f
Move isSet/GetAccessor back to the original location
sandersn Oct 5, 2018
84b08c0
Merge branch 'master' into js/infer-from-usage
sandersn Oct 5, 2018
a3aae7b
Oh no I missed a newline and a space
sandersn Oct 5, 2018
d22961a
Switch to an object type
sandersn Oct 5, 2018
3cc99ea
A lot of cleanup
sandersn Oct 5, 2018
31db1ce
Move and delegate to annotateJSDocParameters
sandersn Oct 8, 2018
6b0be8b
Merge branch 'master' into js/infer-from-usage
sandersn Oct 8, 2018
edcb30d
Address PR comments
sandersn Oct 8, 2018
d129e32
Lint: newline problems!!!!
sandersn Oct 8, 2018
e3cf787
Switch another call to getNonformattedText
sandersn Oct 9, 2018
bc32d3c
Merge branch 'master' into js/infer-from-usage
sandersn Oct 9, 2018
5066880
Update baseline missed after merge
sandersn Oct 9, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/transformers/declarations/diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -466,4 +466,4 @@ namespace ts {
};
}
}
}
}
8 changes: 6 additions & 2 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
}
Expand Down
94 changes: 69 additions & 25 deletions src/services/codefixes/inferFromUsage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ namespace ts.codefix {
errorCodes,
getCodeActions(context) {
const { sourceFile, program, span: { start }, errorCode, cancellationToken, host } = context;
if (isSourceFileJS(sourceFile)) {
return undefined; // TODO: GH#20113
}

const token = getTokenAtPosition(sourceFile, start);
let declaration!: Declaration | undefined;
Expand All @@ -50,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
Copy link

@ghost ghost Oct 8, 2018

Choose a reason for hiding this comment

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

Why do we have both of these -- isSetAccessor and isSetAccessorDeclaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason, but removing one would change the public API, so I decided not do it in this PR.

case Diagnostics.Rest_parameter_0_implicitly_has_an_any_type.code:
return Diagnostics.Infer_parameter_types_from_usage;
default:
Expand All @@ -59,7 +56,7 @@ namespace ts.codefix {
}

function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, errorCode: number, program: Program, cancellationToken: CancellationToken, markSeen: NodeSeenTracker, host: LanguageServiceHost): 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;
}

Expand All @@ -72,6 +69,14 @@ namespace ts.codefix {
annotateVariableDeclaration(changes, sourceFile, parent, program, host, cancellationToken);
return parent;
}
if (isPropertyAccessExpression(parent)) {
const type = inferTypeForVariableFromUsage(parent.name, program, cancellationToken);
const typeNode = type && getTypeNodeIfAccessible(type, parent, program, host);
if (typeNode) {
changes.tryInsertJSDocType(sourceFile, parent, typeNode);
}
return parent;
}
return undefined;

case Diagnostics.Variable_0_implicitly_has_an_1_type.code: {
Expand All @@ -92,7 +97,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, host, cancellationToken);
return containingFunction;
}
Expand All @@ -108,15 +113,15 @@ 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, host);
return containingFunction;
}
return undefined;

// 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, host, cancellationToken);
return containingFunction;
}
Expand Down Expand Up @@ -150,35 +155,61 @@ namespace ts.codefix {
return;
}

const types = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) ||
containingFunction.parameters.map(p => isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined);
// We didn't actually find a set of type inference positions matching each parameter position
if (!types || containingFunction.parameters.length !== types.length) {
return;
}
const parameterInferences = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) ||
containingFunction.parameters.map<ParameterInference>(p => ({
declaration: p,
type: isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined
}));
Debug.assert(containingFunction.parameters.length === parameterInferences.length);

zipWith(containingFunction.parameters, types, (parameter, type) => {
if (!parameter.type && !parameter.initializer) {
annotate(changes, sourceFile, parameter, type, program, host);
if (isInJSFile(containingFunction)) {
annotateJSDocParameters(changes, sourceFile, parameterInferences, program, host);
}
else {
for (const { declaration, type } of parameterInferences) {
if (declaration && !declaration.type && !declaration.initializer) {
annotate(changes, sourceFile, declaration, type, program, host);
}
}
});
}
}

function annotateSetAccessor(changes: textChanges.ChangeTracker, sourceFile: SourceFile, setAccessorDeclaration: SetAccessorDeclaration, program: Program, host: LanguageServiceHost, 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);
annotate(changes, sourceFile, param, type, program, host);
if (isInJSFile(setAccessorDeclaration)) {
annotateJSDocParameters(changes, sourceFile, [{ declaration: param, type }], program, host);
}
else {
annotate(changes, sourceFile, param, type, program, host);
}
}
}

function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program, host: LanguageServiceHost): void {
const typeNode = type && getTypeNodeIfAccessible(type, declaration, program, host);
if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode);
if (typeNode) {
if (isInJSFile(sourceFile) && declaration.kind !== SyntaxKind.PropertySignature) {
changes.tryInsertJSDocType(sourceFile, declaration, typeNode);
}
else {
changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode);
}
}
}

function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode | undefined {
function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parameterInferences: ParameterInference[], program: Program, host: LanguageServiceHost): void {
const result = mapDefined(parameterInferences, inference => {
const param = inference.declaration;
const typeNode = inference.type && getTypeNodeIfAccessible(inference.type, param, program, host);
return typeNode && !param.initializer && !getJSDocType(param) ? { ...inference, typeNode } : undefined;
});
changes.tryInsertJSDocParameters(sourceFile, result);
}

function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, program: Program, host: LanguageServiceHost): TypeNode | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

[](start = 0, length = 3)

This looks unintentional.

const checker = program.getTypeChecker();
let typeIsAccessible = true;
const notAccessible = () => { typeIsAccessible = false; };
Expand Down Expand Up @@ -212,7 +243,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): ParameterInference[] | undefined {
switch (containingFunction.kind) {
case SyntaxKind.Constructor:
case SyntaxKind.FunctionExpression:
Expand All @@ -228,6 +259,13 @@ namespace ts.codefix {
}
}

interface ParameterInference {
declaration: ParameterDeclaration;
type?: Type;
typeNode?: TypeNode;
isOptional?: boolean;
}

namespace InferFromReference {
interface CallContext {
argumentTypes: Type[];
Expand Down Expand Up @@ -255,7 +293,7 @@ namespace ts.codefix {
return getTypeFromUsageContext(usageContext, checker);
}

export function inferTypeForParametersFromReferences(references: ReadonlyArray<Identifier>, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): (Type | undefined)[] | undefined {
export function inferTypeForParametersFromReferences(references: ReadonlyArray<Identifier>, declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): ParameterInference[] | undefined {
if (references.length === 0) {
return undefined;
}
Expand All @@ -274,8 +312,10 @@ namespace ts.codefix {
return callContexts && declaration.parameters.map((parameter, parameterIndex) => {
const types: Type[] = [];
const isRest = isRestParameter(parameter);
let isOptional = false;
Copy link
Member

Choose a reason for hiding this comment

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

isOptional [](start = 20, length = 10)

I don't understand this flag. I'll drop by.

for (const callContext of callContexts) {
if (callContext.argumentTypes.length <= parameterIndex) {
isOptional = isInJSFile(declaration);
Copy link
Member

Choose a reason for hiding this comment

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

isInJSFile [](start = 37, length = 10)

I assume isInJSFile is very cheap but I would habitually have either pulled it out of the loop or checked isOptional ||.

continue;
}

Expand All @@ -289,10 +329,14 @@ namespace ts.codefix {
}
}
if (!types.length) {
return undefined;
return { declaration: parameter };
}
const type = checker.getWidenedType(checker.getUnionType(types, UnionReduction.Subtype));
return isRest ? checker.createArrayType(type) : type;
return {
type: isRest ? checker.createArrayType(type) : type,
isOptional: isOptional && !isRest,
declaration: parameter
};
});
}

Expand Down
52 changes: 51 additions & 1 deletion src/services/textChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Statement> }[] = [];
Expand Down Expand Up @@ -339,6 +345,12 @@ namespace ts.textChanges {
this.insertText(sourceFile, token.getStart(sourceFile), text);
}

public insertCommentThenNewline(sourceFile: SourceFile, character: number, position: number, commentText: string): void {
const token = getTouchingToken(sourceFile, position);
const text = "/**" + commentText + "*/" + this.newLineCharacter + repeatString(" ", character);
Copy link
Member

Choose a reason for hiding this comment

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

" " [](start = 91, length = 3)

Will the formatting done after the fix is applied convert this to tabs if that's what the user prefers?

Copy link

Choose a reason for hiding this comment

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

What we do for other fixes is to copy exactly the indentation string that they were using -- if their preferred indentation is tab, space, space, tab, space, space, we should copy that. I'll make a new PR unifying our comment code since #27565 has its own separate jsdoc-adding helpers.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are cases where we need to make to new indentation, though, as in the single-line test case that needs to annotate class C {m(x) {return x;}}

this.insertText(sourceFile, token.getStart(sourceFile), text);
}

public replaceRangeWithText(sourceFile: SourceFile, range: TextRange, text: string) {
this.changes.push({ kind: ChangeKind.Text, sourceFile, range, text });
}
Expand All @@ -347,6 +359,23 @@ namespace ts.textChanges {
this.replaceRangeWithText(sourceFile, createRange(pos), text);
}

public tryInsertJSDocParameters(sourceFile: SourceFile, parameters: JSDocParameter[]) {
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, isOptional } of parameters) {
if (isIdentifier(declaration.name)) {
const printed = changesToText.getNonformattedText(typeNode, sourceFile, this.newLineCharacter).text;
commentText += this.printJSDocParameter(indent, printed, declaration.name, isOptional);
}
}
commentText += repeatString(" ", indent + 1);
Copy link
Member

Choose a reason for hiding this comment

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

commentText += repeatString(" ", indent + 1); [](start = 12, length = 45)

This seems like it belongs in insertCommentThenNewline (which would then either need to check for newlines in the text or accept a flag indicating whether or not the */ should be indented).

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, node: TypeAnnotatable, type: TypeNode): void {
let endNode: Node | undefined;
Expand All @@ -365,6 +394,27 @@ namespace ts.textChanges {
this.insertNodeAt(sourceFile, endNode.end, type, { prefix: ": " });
}

public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void {
Copy link
Member

Choose a reason for hiding this comment

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

tryInsertJSDocType [](start = 15, length = 18)

I feel like I'm missing something obvious, but why "try"?

const printed = changesToText.getNonformattedText(type, sourceFile, this.newLineCharacter).text;
let commentText;
if (isGetAccessorDeclaration(node)) {
commentText = ` @return {${printed}} `;
}
else {
commentText = ` @type {${printed}} `;
node = node.parent;
}
this.insertCommentThenNewline(sourceFile, getLineAndCharacterOfPosition(sourceFile, node.getStart(sourceFile)).character, node.getStart(sourceFile), commentText);
}

private printJSDocParameter(indent: number, printed: string, name: Identifier, isOptionalParameter: boolean | undefined) {
let printName = unescapeLeadingUnderscores(name.escapedText);
if (isOptionalParameter) {
printName = `[${printName}]`;
}
return repeatString(" ", indent) + ` * @param {${printed}} ${printName}\n`;
Copy link
Member

Choose a reason for hiding this comment

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

repeatString(" ", indent) [](start = 19, length = 25)

Having these scattered through the code seems fragile. What about passing an array of lines to insertCommentThenNewline and letting it handle breaks and indentation?

}

public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray<TypeParameterDeclaration>): 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);
Expand Down Expand Up @@ -806,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 } {
Copy link
Member

Choose a reason for hiding this comment

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

export [](start = 8, length = 6)

Why is this exported?

const writer = new Writer(newLineCharacter);
const newLine = newLineCharacter === "\n" ? NewLineKind.LineFeed : NewLineKind.CarriageReturnLineFeed;
createPrinter({ newLine, neverAsciiEscape: true }, writer).writeNode(EmitHint.Unspecified, node, sourceFile, writer);
Expand Down
20 changes: 20 additions & 0 deletions tests/cases/fourslash/codeFixInferFromUsageCallJS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// <reference path='fourslash.ts' />

// @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();
}`});
12 changes: 12 additions & 0 deletions tests/cases/fourslash/codeFixInferFromUsageJS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/// <reference path='fourslash.ts' />

// @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);
14 changes: 14 additions & 0 deletions tests/cases/fourslash/codeFixInferFromUsageMember2JS.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/// <reference path='fourslash.ts' />

// @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);
Loading