Skip to content

Commit 6c9a83d

Browse files
committed
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)
1 parent ccdd688 commit 6c9a83d

File tree

3 files changed

+31
-7
lines changed

3 files changed

+31
-7
lines changed

src/server/session.ts

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1367,7 +1367,7 @@ namespace ts.server {
13671367
emptyArray;
13681368
}
13691369

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

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

1388-
private getSuggestionDiagnosticsSync(args: protocol.SuggestionDiagnosticsSyncRequestArgs): readonly protocol.Diagnostic[] | readonly protocol.DiagnosticWithLinePosition[] {
1388+
private getSuggestionDiagnosticsSync(args: protocol.SuggestionDiagnosticsSyncRequestArgs) {
13891389
const { configFile } = this.getConfigFileAndProject(args);
13901390
if (configFile) {
13911391
// Currently there are no info diagnostics for config files.
@@ -2198,7 +2198,25 @@ namespace ts.server {
21982198
const scriptInfo = project.getScriptInfoForNormalizedPath(file)!;
21992199
const { startPosition, endPosition } = this.getStartAndEndPosition(args, scriptInfo);
22002200

2201-
const codeActions = project.getLanguageService().getCodeFixesAtPosition(file, startPosition, endPosition, args.errorCodes, this.getFormatOptions(file), this.getPreferences(file));
2201+
let codeActions: readonly CodeFixAction[];
2202+
try {
2203+
codeActions = project.getLanguageService().getCodeFixesAtPosition(file, startPosition, endPosition, args.errorCodes, this.getFormatOptions(file), this.getPreferences(file));
2204+
}
2205+
catch(e) {
2206+
const ls = project.getLanguageService();
2207+
const existingDiagCodes = [
2208+
...ls.getSyntacticDiagnostics(file),
2209+
...ls.getSemanticDiagnostics(file),
2210+
...ls.getSuggestionDiagnostics(file)
2211+
].map(d =>
2212+
decodedTextSpanIntersectsWith(startPosition, endPosition - startPosition, d.start!, d.length!)
2213+
&& d.code);
2214+
const badCode = args.errorCodes.find(c => !existingDiagCodes.includes(c));
2215+
if (badCode !== undefined) {
2216+
e.message = `BADCLIENT: Bad error code, ${badCode} not found in range ${startPosition}..${endPosition} (found: ${existingDiagCodes.join(", ")}); could have caused this error:\n${e.message}`;
2217+
}
2218+
throw e;
2219+
}
22022220
return simplifiedResult ? codeActions.map(codeAction => this.mapCodeFixAction(codeAction)) : codeActions;
22032221
}
22042222

src/services/codefixes/fixUnusedIdentifier.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,8 +237,10 @@ namespace ts.codefix {
237237
if (isParameter(parent)) {
238238
tryDeleteParameter(changes, sourceFile, parent, checker, sourceFiles, program, cancellationToken, isFixAll);
239239
}
240-
else if (!isFixAll || !(isIdentifier(token) && FindAllReferences.Core.isSymbolReferencedInFile(token, checker, sourceFile))) {
241-
changes.delete(sourceFile, isImportClause(parent) ? token : isComputedPropertyName(parent) ? parent.parent : parent);
240+
else if (!(isFixAll && isIdentifier(token) && FindAllReferences.Core.isSymbolReferencedInFile(token, checker, sourceFile))) {
241+
const node = isImportClause(parent) ? token : isComputedPropertyName(parent) ? parent.parent : parent;
242+
Debug.assert(node !== sourceFile, "should not delete whole source file");
243+
changes.delete(sourceFile, node);
242244
}
243245
}
244246

src/services/textChanges.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1361,7 +1361,11 @@ namespace ts.textChanges {
13611361
break;
13621362

13631363
default:
1364-
if (isImportClause(node.parent) && node.parent.name === node) {
1364+
if (!node.parent) {
1365+
// a misbehaving client can reach here with the SourceFile node
1366+
deleteNode(changes, sourceFile, node);
1367+
}
1368+
else if (isImportClause(node.parent) && node.parent.name === node) {
13651369
deleteDefaultImport(changes, sourceFile, node.parent);
13661370
}
13671371
else if (isCallExpression(node.parent) && contains(node.parent.arguments, node)) {

0 commit comments

Comments
 (0)