Skip to content

Commit 6ce82ab

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 fb60c9f commit 6ce82ab

File tree

3 files changed

+31
-7
lines changed

3 files changed

+31
-7
lines changed

Diff for: src/server/session.ts

+22-4
Original file line numberDiff line numberDiff line change
@@ -1393,7 +1393,7 @@ namespace ts.server {
13931393
emptyArray;
13941394
}
13951395

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

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

1414-
private getSuggestionDiagnosticsSync(args: protocol.SuggestionDiagnosticsSyncRequestArgs): readonly protocol.Diagnostic[] | readonly protocol.DiagnosticWithLinePosition[] {
1414+
private getSuggestionDiagnosticsSync(args: protocol.SuggestionDiagnosticsSyncRequestArgs) {
14151415
const { configFile } = this.getConfigFileAndProject(args);
14161416
if (configFile) {
14171417
// Currently there are no info diagnostics for config files.
@@ -2233,7 +2233,25 @@ namespace ts.server {
22332233
const scriptInfo = project.getScriptInfoForNormalizedPath(file)!;
22342234
const { startPosition, endPosition } = this.getStartAndEndPosition(args, scriptInfo);
22352235

2236-
const codeActions = project.getLanguageService().getCodeFixesAtPosition(file, startPosition, endPosition, args.errorCodes, this.getFormatOptions(file), this.getPreferences(file));
2236+
let codeActions: readonly CodeFixAction[];
2237+
try {
2238+
codeActions = project.getLanguageService().getCodeFixesAtPosition(file, startPosition, endPosition, args.errorCodes, this.getFormatOptions(file), this.getPreferences(file));
2239+
}
2240+
catch(e) {
2241+
const ls = project.getLanguageService();
2242+
const existingDiagCodes = [
2243+
...ls.getSyntacticDiagnostics(file),
2244+
...ls.getSemanticDiagnostics(file),
2245+
...ls.getSuggestionDiagnostics(file)
2246+
].map(d =>
2247+
decodedTextSpanIntersectsWith(startPosition, endPosition - startPosition, d.start!, d.length!)
2248+
&& d.code);
2249+
const badCode = args.errorCodes.find(c => !existingDiagCodes.includes(c));
2250+
if (badCode !== undefined) {
2251+
e.message = `BADCLIENT: Bad error code, ${badCode} not found in range ${startPosition}..${endPosition} (found: ${existingDiagCodes.join(", ")}); could have caused this error:\n${e.message}`;
2252+
}
2253+
throw e;
2254+
}
22372255
return simplifiedResult ? codeActions.map(codeAction => this.mapCodeFixAction(codeAction)) : codeActions;
22382256
}
22392257

Diff for: src/services/codefixes/fixUnusedIdentifier.ts

+4-2
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

Diff for: src/services/textChanges.ts

+5-1
Original file line numberDiff line numberDiff line change
@@ -1363,7 +1363,11 @@ namespace ts.textChanges {
13631363
break;
13641364

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

0 commit comments

Comments
 (0)