From fa402e7898eb367f19f9a94eb464defb2ccd59c8 Mon Sep 17 00:00:00 2001 From: Eli Barzilay Date: Thu, 25 Feb 2021 16:57:48 -0500 Subject: [PATCH] `deleteDeclaration`: don't crash on the top node. 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 #33726 (probably not fixing it, but making it easier to find the cause) --- src/server/session.ts | 23 +++++++++++++++---- src/services/codefixes/fixUnusedIdentifier.ts | 6 +++-- src/services/textChanges.ts | 6 ++++- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/server/session.ts b/src/server/session.ts index dc0d4f42f5c79..46e9a52fb5a44 100644 --- a/src/server/session.ts +++ b/src/server/session.ts @@ -1367,7 +1367,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 @@ -1377,7 +1377,7 @@ 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 @@ -1385,7 +1385,7 @@ namespace ts.server { 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. @@ -2198,7 +2198,22 @@ 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 ls = project.getLanguageService(); + const existingDiagCodes = + flatMap(["getSyntacticDiagnostics", "getSemanticDiagnostics", "getSuggestionDiagnostics"] as const, + getter => ls[getter](file)) + .map(d => decodedTextSpanIntersectsWith(startPosition, endPosition - startPosition, d.start!, d.length!) && d.code); // TODO: GH#18217 + 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; } diff --git a/src/services/codefixes/fixUnusedIdentifier.ts b/src/services/codefixes/fixUnusedIdentifier.ts index db7e0550e9a38..9c755729c20e8 100644 --- a/src/services/codefixes/fixUnusedIdentifier.ts +++ b/src/services/codefixes/fixUnusedIdentifier.ts @@ -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); } } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index adbcfe68bef42..2425ae43d1271 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -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)) {