Skip to content
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.

Sign up for GitHub

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

inferFromUsage: use ChangeTracker and typeToTypeNode #22379

Merged
3 commits merged into from
Mar 7, 2018
Merged

inferFromUsage: use ChangeTracker and typeToTypeNode #22379

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9ebf05
inferFromUsage: use ChangeTracker and typeToTypeNode
Mar 6, 2018
e58d270
Use typeToTypeNode with SymbolTracker
Mar 7, 2018
095ce46
Add comment
Mar 7, 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
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2793,6 +2793,7 @@ namespace ts {

/** Note that the resulting nodes cannot be checked. */
typeToTypeNode(type: Type, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): TypeNode;
/* @internal */ typeToTypeNode(type: Type, enclosingDeclaration?: Node, flags?: NodeBuilderFlags, tracker?: SymbolTracker): TypeNode; // tslint:disable-line unified-signatures
/** Note that the resulting nodes cannot be checked. */
signatureToSignatureDeclaration(signature: Signature, kind: SyntaxKind, enclosingDeclaration?: Node, flags?: NodeBuilderFlags): SignatureDeclaration & {typeArguments?: NodeArray<TypeNode>} | undefined;
/** Note that the resulting nodes cannot be checked. */
Expand Down
176 changes: 69 additions & 107 deletions src/services/codefixes/inferFromUsage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,27 +24,26 @@ namespace ts.codefix {
];
registerCodeFix({
errorCodes,
getCodeActions({ sourceFile, program, span: { start }, errorCode, cancellationToken }) {
getCodeActions(context) {
const { sourceFile, program, span: { start }, errorCode, cancellationToken } = context;
if (isSourceFileJavaScript(sourceFile)) {
return undefined; // TODO: GH#20113
}

const token = getTokenAtPosition(sourceFile, start, /*includeJsDocComment*/ false);
const fix = getFix(sourceFile, token, errorCode, program, cancellationToken);
if (!fix) return undefined;

const { declaration, textChanges } = fix;
const name = getNameOfDeclaration(declaration);
const description = formatStringFromArgs(getLocaleSpecificMessage(getDiagnostic(errorCode, token)), [name.getText()]);
return [{ description, changes: [{ fileName: sourceFile.fileName, textChanges }], fixId }];
let declaration!: Declaration;
const changes = textChanges.ChangeTracker.with(context, changes => { declaration = doChange(changes, sourceFile, token, errorCode, program, cancellationToken); });
if (changes.length === 0) return undefined;
const name = getNameOfDeclaration(declaration).getText();
const description = formatStringFromArgs(getLocaleSpecificMessage(getDiagnostic(errorCode, token)), [name]);
return [{ description, changes, fixId }];
},
fixIds: [fixId],
getAllCodeActions(context) {
const { sourceFile, program, cancellationToken } = context;
const seenFunctions = createMap<true>();
return codeFixAllWithTextChanges(context, errorCodes, (changes, err) => {
const fix = getFix(sourceFile, getTokenAtPosition(err.file!, err.start!, /*includeJsDocComment*/ false), err.code, program, cancellationToken, seenFunctions);
if (fix) changes.push(...fix.textChanges);
return codeFixAll(context, errorCodes, (changes, err) => {
doChange(changes, sourceFile, getTokenAtPosition(err.file!, err.start!, /*includeJsDocComment*/ false), err.code, program, cancellationToken, seenFunctions);
});
},
});
Expand All @@ -60,12 +59,7 @@ namespace ts.codefix {
}
}

interface Fix {
readonly declaration: Declaration;
readonly textChanges: TextChange[];
}

function getFix(sourceFile: SourceFile, token: Node, errorCode: number, program: Program, cancellationToken: CancellationToken, seenFunctions?: Map<true>): Fix | undefined {
function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, token: Node, errorCode: number, program: Program, cancellationToken: CancellationToken, seenFunctions?: Map<true>): Declaration | undefined {
if (!isAllowedTokenKind(token.kind)) {
return undefined;
}
Expand All @@ -74,11 +68,15 @@ 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:
return getCodeActionForVariableDeclaration(<PropertyDeclaration | PropertySignature | VariableDeclaration>token.parent, program, cancellationToken);
annotateVariableDeclaration(changes, sourceFile, <PropertyDeclaration | PropertySignature | VariableDeclaration>token.parent, program, cancellationToken);
return token.parent as Declaration;

case Diagnostics.Variable_0_implicitly_has_an_1_type.code: {
const symbol = program.getTypeChecker().getSymbolAtLocation(token);
return symbol && symbol.valueDeclaration && getCodeActionForVariableDeclaration(<VariableDeclaration>symbol.valueDeclaration, program, cancellationToken);
if (symbol && symbol.valueDeclaration) {
annotateVariableDeclaration(changes, sourceFile, <VariableDeclaration>symbol.valueDeclaration, program, cancellationToken);
return symbol.valueDeclaration;
}
}
}

Expand All @@ -91,22 +89,34 @@ namespace ts.codefix {
// Parameter declarations
case Diagnostics.Parameter_0_implicitly_has_an_1_type.code:
if (isSetAccessor(containingFunction)) {
return getCodeActionForSetAccessor(containingFunction, program, cancellationToken);
annotateSetAccessor(changes, sourceFile, containingFunction, program, cancellationToken);
return containingFunction;
}
// falls through
case Diagnostics.Rest_parameter_0_implicitly_has_an_any_type.code:
return !seenFunctions || addToSeen(seenFunctions, getNodeId(containingFunction))
? getCodeActionForParameters(cast(token.parent, isParameter), containingFunction, sourceFile, program, cancellationToken)
: undefined;
if (!seenFunctions || addToSeen(seenFunctions, getNodeId(containingFunction))) {
const param = cast(token.parent, isParameter);
annotateParameters(changes, param, containingFunction, sourceFile, program, cancellationToken);
return param;
}
return undefined;

// 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:
return isGetAccessor(containingFunction) ? getCodeActionForGetAccessor(containingFunction, sourceFile, program, cancellationToken) : undefined;
if (isGetAccessor(containingFunction) && isIdentifier(containingFunction.name)) {
annotate(changes, sourceFile, containingFunction, inferTypeForVariableFromUsage(containingFunction.name, program, cancellationToken), program);
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:
return isSetAccessor(containingFunction) ? getCodeActionForSetAccessor(containingFunction, program, cancellationToken) : undefined;
if (isSetAccessor(containingFunction)) {
annotateSetAccessor(changes, sourceFile, containingFunction, program, cancellationToken);
return containingFunction;
}
return undefined;

default:
return Debug.fail(String(errorCode));
Expand All @@ -127,10 +137,10 @@ namespace ts.codefix {
}
}

function getCodeActionForVariableDeclaration(declaration: VariableDeclaration | PropertyDeclaration | PropertySignature, program: Program, cancellationToken: CancellationToken): Fix | undefined {
if (!isIdentifier(declaration.name)) return undefined;
const type = inferTypeForVariableFromUsage(declaration.name, program, cancellationToken);
return makeFix(declaration, declaration.name.getEnd(), type, program);
function annotateVariableDeclaration(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: VariableDeclaration | PropertyDeclaration | PropertySignature, program: Program, cancellationToken: CancellationToken): void {
if (isIdentifier(declaration.name)) {
annotate(changes, sourceFile, declaration, inferTypeForVariableFromUsage(declaration.name, program, cancellationToken), program);
}
}

function isApplicableFunctionForInference(declaration: FunctionLike): declaration is MethodDeclaration | FunctionDeclaration | ConstructorDeclaration {
Expand All @@ -145,54 +155,51 @@ namespace ts.codefix {
return false;
}

function getCodeActionForParameters(parameterDeclaration: ParameterDeclaration, containingFunction: FunctionLike, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): Fix | undefined {
function annotateParameters(changes: textChanges.ChangeTracker, parameterDeclaration: ParameterDeclaration, containingFunction: FunctionLike, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): void {
if (!isIdentifier(parameterDeclaration.name) || !isApplicableFunctionForInference(containingFunction)) {
return undefined;
return;
}

const types = inferTypeForParametersFromUsage(containingFunction, sourceFile, program, cancellationToken) ||
containingFunction.parameters.map(p => isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, program, cancellationToken) : undefined);
if (!types) return undefined;

// We didn't actually find a set of type inference positions matching each parameter position
if (containingFunction.parameters.length !== types.length) {
return undefined;
if (!types || containingFunction.parameters.length !== types.length) {
return;
}

const textChanges = arrayFrom(mapDefinedIterator(zipToIterator(containingFunction.parameters, types), ([parameter, type]) =>
type && !parameter.type && !parameter.initializer ? makeChange(containingFunction, parameter.end, type, program) : undefined));
return textChanges.length ? { declaration: parameterDeclaration, textChanges } : undefined;
}

function getCodeActionForSetAccessor(setAccessorDeclaration: SetAccessorDeclaration, program: Program, cancellationToken: CancellationToken): Fix | undefined {
const setAccessorParameter = setAccessorDeclaration.parameters[0];
if (!setAccessorParameter || !isIdentifier(setAccessorDeclaration.name) || !isIdentifier(setAccessorParameter.name)) {
return undefined;
}

const type = inferTypeForVariableFromUsage(setAccessorDeclaration.name, program, cancellationToken) ||
inferTypeForVariableFromUsage(setAccessorParameter.name, program, cancellationToken);
return makeFix(setAccessorParameter, setAccessorParameter.name.getEnd(), type, program);
zipWith(containingFunction.parameters, types, (parameter, type) => {
if (!parameter.type && !parameter.initializer) {
annotate(changes, sourceFile, parameter, type, program);
}
});
}

function getCodeActionForGetAccessor(getAccessorDeclaration: GetAccessorDeclaration, sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): Fix | undefined {
if (!isIdentifier(getAccessorDeclaration.name)) {
return undefined;
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);
annotate(changes, sourceFile, param, type, program);
}

const type = inferTypeForVariableFromUsage(getAccessorDeclaration.name, program, cancellationToken);
const closeParenToken = findChildOfKind(getAccessorDeclaration, SyntaxKind.CloseParenToken, sourceFile);
return makeFix(getAccessorDeclaration, closeParenToken.getEnd(), type, program);
}

function makeFix(declaration: Declaration, start: number, type: Type | undefined, program: Program): Fix | undefined {
const change = makeChange(declaration, start, type, program);
return change && { declaration, textChanges: [change] };
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.insertTypeAnnotation(sourceFile, declaration, typeNode);
}

function makeChange(declaration: Declaration, start: number, type: Type | undefined, program: Program): TextChange | undefined {
const typeString = type && typeToString(type, declaration, program.getTypeChecker());
return typeString === undefined ? undefined : createTextChangeFromStartLength(start, 0, `: ${typeString}`);
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, {
trackSymbol: (symbol, declaration, meaning) => {
typeIsAccessible = typeIsAccessible && checker.isSymbolAccessible(symbol, declaration, meaning, /*shouldComputeAliasToMarkVisible*/ false).accessibility === SymbolAccessibility.Accessible;
},
reportInaccessibleThisError: notAccessible,
reportPrivateInBaseOfClassExpression: notAccessible,
reportInaccessibleUniqueSymbolError: notAccessible,
});
return typeIsAccessible ? res : undefined;
}

function getReferences(token: PropertyName | Token<SyntaxKind.ConstructorKeyword>, program: Program, cancellationToken: CancellationToken): ReadonlyArray<Identifier> {
Expand Down Expand Up @@ -221,51 +228,6 @@ namespace ts.codefix {
}
}

function getTypeAccessiblityWriter(checker: TypeChecker): EmitTextWriter {
let str = "";
let typeIsAccessible = true;

const writeText: (text: string) => void = text => str += text;
return {
getText: () => typeIsAccessible ? str : undefined,
writeKeyword: writeText,
writeOperator: writeText,
writePunctuation: writeText,
writeSpace: writeText,
writeStringLiteral: writeText,
writeParameter: writeText,
writeProperty: writeText,
writeSymbol: writeText,
write: writeText,
writeTextOfNode: writeText,
rawWrite: writeText,
writeLiteral: writeText,
getTextPos: () => 0,
getLine: () => 0,
getColumn: () => 0,
getIndent: () => 0,
isAtStartOfLine: () => false,
writeLine: () => writeText(" "),
increaseIndent: noop,
decreaseIndent: noop,
clear: () => { str = ""; typeIsAccessible = true; },
trackSymbol: (symbol, declaration, meaning) => {
if (checker.isSymbolAccessible(symbol, declaration, meaning, /*shouldComputeAliasToMarkVisible*/ false).accessibility !== SymbolAccessibility.Accessible) {
typeIsAccessible = false;
}
},
reportInaccessibleThisError: () => { typeIsAccessible = false; },
reportPrivateInBaseOfClassExpression: () => { typeIsAccessible = false; },
reportInaccessibleUniqueSymbolError: () => { typeIsAccessible = false; }
};
}

function typeToString(type: Type, enclosingDeclaration: Declaration, checker: TypeChecker): string {
const writer = getTypeAccessiblityWriter(checker);
checker.writeType(type, enclosingDeclaration, /*flags*/ undefined, writer);
return writer.getText();
}

namespace InferFromReference {
interface CallContext {
argumentTypes: Type[];
Expand Down
9 changes: 9 additions & 0 deletions src/services/textChanges.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ namespace ts.textChanges {
formatContext: formatting.FormatContext;
}

export type TypeAnnotatable = SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature;

export class ChangeTracker {
private readonly changes: Change[] = [];
private readonly deletedNodesInLists: true[] = []; // Stores ids of nodes in lists that we already deleted. Used to avoid deleting `, ` twice in `a, b`.
Expand Down Expand Up @@ -343,6 +345,13 @@ namespace ts.textChanges {
this.replaceRange(sourceFile, { pos, end: pos }, createToken(modifier), { suffix: " " });
}

public insertTypeAnnotation(sourceFile: SourceFile, node: TypeAnnotatable, type: TypeNode): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

so why not just replaceNode with a node with a type annotation?

Copy link
Author

Choose a reason for hiding this comment

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

To avoid an issue like #22358 -- we should be adding new text but not reformatting existing text

Copy link
Contributor

Choose a reason for hiding this comment

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

i see.. we should add a comment for that then..

const end = (isFunctionLike(node)
? findChildOfKind(node, SyntaxKind.CloseParenToken, sourceFile)!
: node.kind !== SyntaxKind.VariableDeclaration && node.questionToken ? node.questionToken : node.name).end;
this.insertNodeAt(sourceFile, end, type, { prefix: ": " });
}

private getOptionsForInsertNodeBefore(before: Node, doubleNewlines: boolean): ChangeNodeOptions {
if (isStatement(before) || isClassElement(before)) {
return { suffix: doubleNewlines ? this.newLineCharacter + this.newLineCharacter : this.newLineCharacter };
Expand Down
6 changes: 3 additions & 3 deletions tests/cases/fourslash/codeFixInferFromUsageInaccessibleTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
/// <reference path='fourslash.ts' />

// @noImplicitAny: true
////function f1([|a |]) { }
////function f1(a) { }
////function h1() {
//// class C { p: number };
//// f1({ ofTypeC: new C() });
////}
////
////function f2([|a |]) { }
////function f2(a) { }
////function h2() {
//// interface I { a: number }
//// interface I { a: number }
//// var i: I = {a : 1};
//// f2(i);
//// f2(2);
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/typeToStringCrashInCodeFix.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
// @noImplicitAny: true
//// function f(y, z = { p: y[

verify.getAndApplyCodeFix();
verify.not.codeFixAvailable();