Skip to content

Commit

Permalink
fix: component import quick-fix with "did you mean" diagnostics (#2373)
Browse files Browse the repository at this point in the history
* fix: component import quick-fix when undeclared variable might be a typo

* fix: combine fix sometime affect next document synchronization

* fix-all as well

* wording
  • Loading branch information
jasonlyu123 authored May 17, 2024
1 parent 3147c81 commit 7409890
Show file tree
Hide file tree
Showing 5 changed files with 185 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,8 @@ export class CodeActionsProviderImpl implements CodeActionsProvider {
if (
diagnostic.range.start.line < 0 ||
diagnostic.range.end.line < 0 ||
diagnostic.code !== DiagnosticCode.CANNOT_FIND_NAME
(diagnostic.code !== DiagnosticCode.CANNOT_FIND_NAME &&
diagnostic.code !== DiagnosticCode.CANNOT_FIND_NAME_X_DID_YOU_MEAN_Y)
) {
continue;
}
Expand Down Expand Up @@ -313,10 +314,13 @@ export class CodeActionsProviderImpl implements CodeActionsProvider {
const virtualDoc = new Document(virtualUri, newText);
virtualDoc.openedByClient = true;
// let typescript know about the virtual document
await this.lsAndTsDocResolver.getSnapshot(virtualDoc);
// getLSAndTSDoc instead of getSnapshot so that project dirty state is correctly tracked by us
// otherwise, sometime the applied code fix might not be picked up by the language service
// because we think the project is still dirty and doesn't update the project version
await this.lsAndTsDocResolver.getLSAndTSDoc(virtualDoc);

return {
virtualDoc: new Document(virtualUri, newText),
virtualDoc,
insertedNames: names
};
}
Expand Down Expand Up @@ -559,7 +563,9 @@ export class CodeActionsProviderImpl implements CodeActionsProvider {
const end = tsDoc.offsetAt(tsDoc.getGeneratedPosition(range.end));
const errorCodes: number[] = context.diagnostics.map((diag) => Number(diag.code));
const cannotFindNameDiagnostic = context.diagnostics.filter(
(diagnostic) => diagnostic.code === DiagnosticCode.CANNOT_FIND_NAME
(diagnostic) =>
diagnostic.code === DiagnosticCode.CANNOT_FIND_NAME ||
diagnostic.code === DiagnosticCode.CANNOT_FIND_NAME_X_DID_YOU_MEAN_Y
);

const formatCodeSettings = await this.configManager.getFormatCodeSettingsForFile(
Expand All @@ -579,9 +585,14 @@ export class CodeActionsProviderImpl implements CodeActionsProvider {
formatCodeSettings
)
: undefined;
codeFixes =
// either-or situation
codeFixes || [

// either-or situation when it's not a "did you mean" fix
if (
codeFixes === undefined ||
errorCodes.includes(DiagnosticCode.CANNOT_FIND_NAME_X_DID_YOU_MEAN_Y)
) {
codeFixes ??= [];
codeFixes = codeFixes.concat(
...lang.getCodeFixesAtPosition(
tsDoc.filePath,
start,
Expand All @@ -599,7 +610,8 @@ export class CodeActionsProviderImpl implements CodeActionsProvider {
userPreferences,
formatCodeSettings
)
];
);
}

const snapshots = new SnapshotMap(this.lsAndTsDocResolver);
snapshots.set(tsDoc.filePath, tsDoc);
Expand Down Expand Up @@ -847,7 +859,11 @@ export class CodeActionsProviderImpl implements CodeActionsProvider {
if (codeFix.fixName === FIX_IMPORT_FIX_NAME) {
const allCannotFindNameDiagnostics = lang
.getSemanticDiagnostics(fileName)
.filter((diagnostic) => diagnostic.code === DiagnosticCode.CANNOT_FIND_NAME);
.filter(
(diagnostic) =>
diagnostic.code === DiagnosticCode.CANNOT_FIND_NAME ||
diagnostic.code === DiagnosticCode.CANNOT_FIND_NAME_X_DID_YOU_MEAN_Y
);

if (allCannotFindNameDiagnostics.length < 2) {
checkedFixIds.add(codeFix.fixId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export enum DiagnosticCode {
MISSING_PROP = 2741, // "Property '..' is missing in type '..' but required in type '..'."
NO_OVERLOAD_MATCHES_CALL = 2769, // "No overload matches this call"
CANNOT_FIND_NAME = 2304, // "Cannot find name 'xxx'"
CANNOT_FIND_NAME_X_DID_YOU_MEAN_Y = 2552, // "Cannot find name '...' Did you mean '...'?"
EXPECTED_N_ARGUMENTS = 2554, // Expected {0} arguments, but got {1}.
DEPRECATED_SIGNATURE = 6387 // The signature '..' of '..' is deprecated
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { LSAndTSDocResolver } from '../../../../src/plugins/typescript/LSAndTSDo
import { __resetCache } from '../../../../src/plugins/typescript/service';
import { pathToUrl } from '../../../../src/utils';
import { recursiveServiceWarmup } from '../test-utils';
import { DiagnosticCode } from '../../../../src/plugins/typescript/features/DiagnosticsProvider';

const testDir = path.join(__dirname, '..');
const indent = ' '.repeat(4);
Expand Down Expand Up @@ -64,7 +65,7 @@ describe('CodeActionsProvider', function () {
uri: pathToUrl(filePath),
text: harmonizeNewLines(ts.sys.readFile(filePath) || '')
});
return { provider, document, docManager };
return { provider, document, docManager, lsAndTsDocResolver };
}

it('provides quickfix', async () => {
Expand Down Expand Up @@ -661,6 +662,81 @@ describe('CodeActionsProvider', function () {
]);
});

it('provides quickfix for component import with "did you mean" diagnostics', async () => {
const { provider, document } = setup('codeaction-component-import.svelte');

const codeActions = await provider.getCodeActions(
document,
Range.create(Position.create(4, 1), Position.create(4, 6)),
{
diagnostics: [
{
code: 2552,
message: "Cannot find name 'Empty'. Did you mean 'EMpty'?",
range: Range.create(Position.create(4, 1), Position.create(4, 6)),
source: 'ts'
}
],
only: [CodeActionKind.QuickFix]
}
);

(<TextDocumentEdit>codeActions[0]?.edit?.documentChanges?.[0])?.edits.forEach(
(edit) => (edit.newText = harmonizeNewLines(edit.newText))
);

assert.deepStrictEqual(codeActions, <CodeAction[]>[
{
edit: {
documentChanges: [
{
edits: [
{
newText: harmonizeNewLines(
`\n${indent}import Empty from "../empty.svelte";\n`
),
range: {
end: Position.create(0, 18),
start: Position.create(0, 18)
}
}
],
textDocument: {
uri: getUri('codeaction-component-import.svelte'),
version: null
}
}
]
},
kind: 'quickfix',
title: 'Add import from "../empty.svelte"'
},
{
edit: {
documentChanges: [
{
edits: [
{
newText: 'EMpty',
range: {
end: Position.create(4, 6),
start: Position.create(4, 1)
}
}
],
textDocument: {
uri: getUri('codeaction-component-import.svelte'),
version: null
}
}
]
},
kind: 'quickfix',
title: "Change spelling to 'EMpty'"
}
]);
});

it('remove import inline with script tag', async () => {
const { provider, document } = setup('remove-imports-inline.svelte');

Expand Down Expand Up @@ -863,13 +939,15 @@ describe('CodeActionsProvider', function () {
});

it('provide quick fix to fix all missing import component', async () => {
const { provider, document } = setup('codeaction-custom-fix-all-component.svelte');
const { provider, document, docManager, lsAndTsDocResolver } = setup(
'codeaction-custom-fix-all-component.svelte'
);

const range = Range.create(Position.create(4, 1), Position.create(4, 15));
const codeActions = await provider.getCodeActions(document, range, {
diagnostics: [
{
code: 2304,
code: DiagnosticCode.CANNOT_FIND_NAME,
message: "Cannot find name 'FixAllImported'.",
range: range,
source: 'ts'
Expand Down Expand Up @@ -912,6 +990,73 @@ describe('CodeActionsProvider', function () {
}
]
});

// fix-all has some "creative" workaround. Testing if it won't affect the document synchronization after applying the fix
docManager.updateDocument(
document,
resolvedFixAll.edit.documentChanges[0].edits.map((edit) => ({
range: edit.range,
text: edit.newText
}))
);

const { lang, tsDoc } = await lsAndTsDocResolver.getLSAndTSDoc(document);
const cannotFindNameDiagnostics = lang
.getSemanticDiagnostics(tsDoc.filePath)
.filter((diagnostic) => diagnostic.code === DiagnosticCode.CANNOT_FIND_NAME);
assert.strictEqual(cannotFindNameDiagnostics.length, 0);
});

it('provide quick fix to fix all missing import component with "did you mean" diagnostics', async () => {
const { provider, document } = setup('codeaction-custom-fix-all-component4.svelte');

const range = Range.create(Position.create(4, 1), Position.create(4, 15));
const codeActions = await provider.getCodeActions(document, range, {
diagnostics: [
{
code: DiagnosticCode.CANNOT_FIND_NAME_X_DID_YOU_MEAN_Y,
message: "Cannot find name 'FixAllImported'. Did you mean 'FixAllImported3'?",
range: range,
source: 'ts'
}
],
only: [CodeActionKind.QuickFix]
});

const fixAll = codeActions.find((action) => action.data);
const resolvedFixAll = await provider.resolveCodeAction(document, fixAll!);

(<TextDocumentEdit>resolvedFixAll?.edit?.documentChanges?.[0])?.edits.forEach(
(edit) => (edit.newText = harmonizeNewLines(edit.newText))
);

assert.deepStrictEqual(resolvedFixAll.edit, {
documentChanges: [
{
edits: [
{
newText:
`\n${indent}import FixAllImported from \"./importing/FixAllImported.svelte\";\n` +
`${indent}import FixAllImported2 from \"./importing/FixAllImported2.svelte\";\n`,
range: {
start: {
character: 18,
line: 0
},
end: {
character: 18,
line: 0
}
}
}
],
textDocument: {
uri: getUri('codeaction-custom-fix-all-component4.svelte'),
version: null
}
}
]
});
});

it('provide quick fix to fix all missing import component without duplicate (script)', async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script lang="ts">
class EMpty {}
</script>

<Empty />
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<script lang="ts">
class FixAllImported3 {}
</script>

<FixAllImported />
<FixAllImported2 />

0 comments on commit 7409890

Please sign in to comment.