Skip to content

Commit

Permalink
deleteDeclaration: don't crash on the top node.
Browse files Browse the repository at this point in the history
A misbehaved client can sometimes cause the server to reach
`deleteDeclaration` with the SourceFile, and it will crash due to no
`node.parent`.  I couldn't find a good way to create a test for it, but
I could trigger it manually by having a file with just a `,`, and
sending an explicit `getCodeFixes` command to the server with
`errorCodes: [6133]`.

Do three things to improve this:

1. `textChanges.ts`: if we get here with the root node, delete it
   instead of failing.

2. `fixUnusedIdentifier.ts`: check that we don't `delete` a node that is
   the whole source file, so the error is more focused (should have more
   similar failure stacks).

3. `session.ts`: when there was any failure in `getCodeFixes`, check if
   the input had a diag code that does not appear in the requested text
   range, and throw an error saying that the failure is probably a
   result of a bad request.

Closes microsoft#33726 (probably not fixing it, but making it easier to find the
cause)
  • Loading branch information
elibarzilay committed Mar 22, 2021
1 parent ccdd688 commit b5cc450
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 13 deletions.
42 changes: 32 additions & 10 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,11 @@ namespace ts.server {
typesMapLocation?: string;
}

type IncludePositionDiagnostics<IncludePos extends boolean> =
IncludePos extends true ? readonly protocol.DiagnosticWithLinePosition[]
: IncludePos extends false ? readonly protocol.Diagnostic[]
: readonly (protocol.DiagnosticWithLinePosition | protocol.Diagnostic)[];

export class Session<TMessage = string> implements EventSender {
private readonly gcTimer: GcTimer;
protected projectService: ProjectService;
Expand Down Expand Up @@ -1194,18 +1199,19 @@ namespace ts.server {
});
}

private getDiagnosticsWorker(
args: protocol.FileRequestArgs, isSemantic: boolean, selector: (project: Project, file: string) => readonly Diagnostic[], includeLinePosition: boolean
): readonly protocol.DiagnosticWithLinePosition[] | readonly protocol.Diagnostic[] {
private getDiagnosticsWorker<IncludePos extends boolean>(
args: protocol.FileRequestArgs, isSemantic: boolean, selector: (project: Project, file: string) => readonly Diagnostic[], includeLinePosition: IncludePos
): IncludePositionDiagnostics<IncludePos> {
const { project, file } = this.getFileAndProject(args);
if (isSemantic && isDeclarationFileInJSOnlyNonConfiguredProject(project, file)) {
return emptyArray;
return emptyArray as unknown as IncludePositionDiagnostics<IncludePos>;
}
const scriptInfo = project.getScriptInfoForNormalizedPath(file);
const diagnostics = selector(project, file);
return includeLinePosition
return (includeLinePosition
? this.convertToDiagnosticsWithLinePosition(diagnostics, scriptInfo)
: diagnostics.map(d => formatDiag(file, project, d));
: diagnostics.map(d => formatDiag(file, project, d))
) as unknown as IncludePositionDiagnostics<IncludePos>;
}

private getDefinition(args: protocol.FileLocationRequestArgs, simplifiedResult: boolean): readonly protocol.FileSpanWithContext[] | readonly DefinitionInfo[] {
Expand Down Expand Up @@ -1367,7 +1373,7 @@ namespace ts.server {
emptyArray;
}

private getSyntacticDiagnosticsSync(args: protocol.SyntacticDiagnosticsSyncRequestArgs): readonly protocol.Diagnostic[] | readonly protocol.DiagnosticWithLinePosition[] {
private getSyntacticDiagnosticsSync(args: protocol.SyntacticDiagnosticsSyncRequestArgs) {
const { configFile } = this.getConfigFileAndProject(args);
if (configFile) {
// all the config file errors are reported as part of semantic check so nothing to report here
Expand All @@ -1377,15 +1383,15 @@ namespace ts.server {
return this.getDiagnosticsWorker(args, /*isSemantic*/ false, (project, file) => project.getLanguageService().getSyntacticDiagnostics(file), !!args.includeLinePosition);
}

private getSemanticDiagnosticsSync(args: protocol.SemanticDiagnosticsSyncRequestArgs): readonly protocol.Diagnostic[] | readonly protocol.DiagnosticWithLinePosition[] {
private getSemanticDiagnosticsSync(args: protocol.SemanticDiagnosticsSyncRequestArgs) {
const { configFile, project } = this.getConfigFileAndProject(args);
if (configFile) {
return this.getConfigFileDiagnostics(configFile, project!, !!args.includeLinePosition); // TODO: GH#18217
}
return this.getDiagnosticsWorker(args, /*isSemantic*/ true, (project, file) => project.getLanguageService().getSemanticDiagnostics(file).filter(d => !!d.file), !!args.includeLinePosition);
}

private getSuggestionDiagnosticsSync(args: protocol.SuggestionDiagnosticsSyncRequestArgs): readonly protocol.Diagnostic[] | readonly protocol.DiagnosticWithLinePosition[] {
private getSuggestionDiagnosticsSync(args: protocol.SuggestionDiagnosticsSyncRequestArgs) {
const { configFile } = this.getConfigFileAndProject(args);
if (configFile) {
// Currently there are no info diagnostics for config files.
Expand Down Expand Up @@ -2198,7 +2204,23 @@ namespace ts.server {
const scriptInfo = project.getScriptInfoForNormalizedPath(file)!;
const { startPosition, endPosition } = this.getStartAndEndPosition(args, scriptInfo);

const codeActions = project.getLanguageService().getCodeFixesAtPosition(file, startPosition, endPosition, args.errorCodes, this.getFormatOptions(file), this.getPreferences(file));
let codeActions: readonly CodeFixAction[];
try {
codeActions = project.getLanguageService().getCodeFixesAtPosition(file, startPosition, endPosition, args.errorCodes, this.getFormatOptions(file), this.getPreferences(file));
}
catch(e) {
const existingDiagCodes = flatten((["getSyntacticDiagnostics", "getSemanticDiagnostics", "getSuggestionDiagnostics"] as const).map(getter =>
this.getDiagnosticsWorker(args, /*isSemantic*/ getter !== "getSyntacticDiagnostics",
(project, file) => project.getLanguageService()[getter](file),
/*includeLinePosition*/ true)
.filter(d => decodedTextSpanIntersectsWith(startPosition, endPosition - startPosition, d.start, d.length))
.map(d => d.code)));
const badCode = args.errorCodes.find(c => !existingDiagCodes.includes(c));
if (badCode !== undefined) {
e.message = `Bad error code: ${badCode} not found in range ${startPosition}..${endPosition} (found: ${existingDiagCodes.join(", ")}); could have caused this error:\n${e.message}`;
}
throw e;
}
return simplifiedResult ? codeActions.map(codeAction => this.mapCodeFixAction(codeAction)) : codeActions;
}

Expand Down
6 changes: 4 additions & 2 deletions src/services/codefixes/fixUnusedIdentifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,10 @@ namespace ts.codefix {
if (isParameter(parent)) {
tryDeleteParameter(changes, sourceFile, parent, checker, sourceFiles, program, cancellationToken, isFixAll);
}
else if (!isFixAll || !(isIdentifier(token) && FindAllReferences.Core.isSymbolReferencedInFile(token, checker, sourceFile))) {
changes.delete(sourceFile, isImportClause(parent) ? token : isComputedPropertyName(parent) ? parent.parent : parent);
else if (!(isFixAll && isIdentifier(token) && FindAllReferences.Core.isSymbolReferencedInFile(token, checker, sourceFile))) {
const node = isImportClause(parent) ? token : isComputedPropertyName(parent) ? parent.parent : parent;
Debug.assert(node !== sourceFile, "should not delete whole source file");
changes.delete(sourceFile, node);
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/services/textChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1361,7 +1361,11 @@ namespace ts.textChanges {
break;

default:
if (isImportClause(node.parent) && node.parent.name === node) {
if (!node.parent) {
// a misbehaving client can reach here with the SourceFile node
deleteNode(changes, sourceFile, node);
}
else if (isImportClause(node.parent) && node.parent.name === node) {
deleteDefaultImport(changes, sourceFile, node.parent);
}
else if (isCallExpression(node.parent) && contains(node.parent.arguments, node)) {
Expand Down

0 comments on commit b5cc450

Please sign in to comment.