Skip to content

Omnibus fixes for telemetry-sourced crashes #20545

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 6 commits into from
Dec 13, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3951,7 +3951,12 @@ namespace ts {
writePunctuation(writer, SyntaxKind.CloseBracketToken);
writePunctuation(writer, SyntaxKind.ColonToken);
writeSpace(writer);
buildTypeDisplay(info.type, writer, enclosingDeclaration, globalFlags, symbolStack);
if (info.type) {
buildTypeDisplay(info.type, writer, enclosingDeclaration, globalFlags, symbolStack);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, I believe this will be deprecated by #18860. Good catch!

}
else {
writeKeyword(writer, SyntaxKind.AnyKeyword);
}
writePunctuation(writer, SyntaxKind.SemicolonToken);
writer.writeLine();
}
Expand Down
22 changes: 19 additions & 3 deletions src/services/codefixes/inferFromUsage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ namespace ts.codefix {
return undefined;
}

const containingFunction = getContainingFunction(token);
switch (errorCode) {
// Variable and Property declarations
case Diagnostics.Member_0_implicitly_has_an_1_type.code:
Expand All @@ -81,6 +80,13 @@ namespace ts.codefix {
const symbol = program.getTypeChecker().getSymbolAtLocation(token);
return symbol && symbol.valueDeclaration && getCodeActionForVariableDeclaration(<VariableDeclaration>symbol.valueDeclaration, sourceFile, program, cancellationToken);
}
}

const containingFunction = getContainingFunction(token);
if (containingFunction === undefined) {
return undefined;
}
switch (errorCode) {

// Parameter declarations
case Diagnostics.Parameter_0_implicitly_has_an_1_type.code:
Expand Down Expand Up @@ -148,6 +154,11 @@ namespace ts.codefix {
containingFunction.parameters.map(p => isIdentifier(p.name) ? inferTypeForVariableFromUsage(p.name, sourceFile, 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;
}

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;
Expand Down Expand Up @@ -191,8 +202,9 @@ namespace ts.codefix {
sourceFile,
token.getStart(sourceFile));

Debug.assert(!!references, "Found no references!");
Debug.assert(references.length === 1, "Found more references than expected");
if (!references || references.length !== 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Is the references.length !== 1 required? Seems weird to expect an array of exactly 1 object. If there are 2, is it better to return an empty array than ignore the 2nd unexpected element? Is it guaranteed/expected to always return 1 element in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

This variable is probably badly named. references is not an array of references to a symbol, it's (effectively) an array of arrays; each element of the outer array refers to a different symbol (as could occur if you had a type and a value with the same name used in a position where either meaning might be valid, such as an import statement), and each subelement is the references to that symbol. In this case it would be very unusual (IOW we have no idea what's going on or what the right thing to do is) if we found more than one symbol matching a certain property.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to remove the asserts or leave them? There is minimal user impact either way (though throwing errors probably isn't good for perf), since we merely end up swallowing the error and writing it as part of the failure response.

return [];
}

return references[0].references.map(r => <Identifier>getTokenAtPosition(program.getSourceFile(r.fileName), r.textSpan.start, /*includeJsDocComment*/ false));
}
Expand Down Expand Up @@ -281,6 +293,10 @@ namespace ts.codefix {
}

export function inferTypeForParametersFromReferences(references: Identifier[], declaration: FunctionLikeDeclaration, checker: TypeChecker, cancellationToken: CancellationToken): (Type | undefined)[] | undefined {
if (references.length === 0) {
return undefined;
}

if (declaration.parameters) {
const usageContext: UsageContext = {};
for (const reference of references) {
Expand Down
2 changes: 1 addition & 1 deletion src/services/symbolDisplay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ namespace ts.SymbolDisplay {
addNewLineIfDisplayPartsExist();
if (symbolKind) {
pushTypePart(symbolKind);
if (!some(symbol.declarations, d => isArrowFunction(d) || (isFunctionExpression(d) || isClassExpression(d)) && !d.name)) {
if (symbol && !some(symbol.declarations, d => isArrowFunction(d) || (isFunctionExpression(d) || isClassExpression(d)) && !d.name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So if symbol === undefined we will just leave the name blank? Is that desirable behavior? Should we perhaps instead fail to build the display completely?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code path builds either anonymous or non-anonymous declarations (e.g. function (x) or function y(x)), so it's already regularly producing non-named symbol displays. In the case where the originating type doesn't have a symbol, it's sort of de facto anonymous anyway so it seems like this won't ever produce any particularly weird output.

displayParts.push(spacePart());
addFullSymbolName(symbol);
}
Expand Down
9 changes: 9 additions & 0 deletions tests/cases/fourslash/incompleteFunctionCallCodefix.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/// <reference path='fourslash.ts' />

// @noImplicitAny: true
//// function f(/*1*/x) {
//// }
//// f(

verify.not.codeFixAvailable([]);

7 changes: 7 additions & 0 deletions tests/cases/fourslash/incompleteFunctionCallCodefix2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference path='fourslash.ts' />

// @noImplicitAny: true
//// function f(new C(100, 3, undefined)

verify.not.codeFixAvailable([]);

7 changes: 7 additions & 0 deletions tests/cases/fourslash/incompleteFunctionCallCodefix3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/// <reference path='fourslash.ts' />

// @noImplicitAny: true
//// function ...q) {}} f(10);

verify.not.codeFixAvailable([]);

6 changes: 6 additions & 0 deletions tests/cases/fourslash/typeToStringCrashInCodeFix.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/// <reference path="fourslash.ts" />

// @noImplicitAny: true
//// function f(y, z = { p: y[

verify.getAndApplyCodeFix();