user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix cargo watch code action filtering

Browse source 
There are two issues with the implementation of `provideCodeActions`
introduced in #1439:

1. We're returning the code action based on the file its diagnostic is
   in; not the file the suggested fix is in. I'm not sure how often
   fixes are suggested cross-file but it's something we should handle.

2. We're not filtering code actions based on the passed range. The means
   if there is any suggestion in a file we'll show an action for every
   line of the file. I naively thought that VS Code would filter for us
   but that was wrong.

Unfortunately the VS Code `CodeAction` object is very complex - it can
handle edits across multiple files, run commands, etc. This makes it
complex to check them for equality or see if any of their edits
intersects with a specified range.

To make it easier to work with suggestions this introduces a
`SuggestedFix` model object and a `SuggestFixCollection` code action
provider. This is a layer between the raw Rust JSON and VS Code's
`CodeAction`s. I was reluctant to introduce another layer of abstraction
here but my attempt to work directly with VS Code's model objects was
worse.
This commit is contained in:
Ryan Cumming 2019-06-27 21:30:23 +10:00
parent 0e1912de52
commit abc0784e57
10 changed files with 495 additions and 254 deletions
Download patch file Download diff file Expand all files Collapse all files

133
editors/code/src/test/utils/diagnotics/SuggestedFix.test.ts Normal file
View file

@ -0,0 +1,133 @@
import * as assert from 'assert';
import * as vscode from 'vscode';
import { SuggestionApplicability } from '../../../utils/diagnostics/rust';
import SuggestedFix from '../../../utils/diagnostics/SuggestedFix';
const location1 = new vscode.Location(
vscode.Uri.file('/file/1'),
new vscode.Range(new vscode.Position(1, 2), new vscode.Position(3, 4))
);
const location2 = new vscode.Location(
vscode.Uri.file('/file/2'),
new vscode.Range(new vscode.Position(5, 6), new vscode.Position(7, 8))
);
describe('SuggestedFix', () => {
describe('isEqual', () => {
it('should treat identical instances as equal', () => {
const suggestion1 = new SuggestedFix(
'Replace me!',
location1,
'With this!'
);
const suggestion2 = new SuggestedFix(
'Replace me!',
location1,
'With this!'
);
assert(suggestion1.isEqual(suggestion2));
});
it('should treat instances with different titles as inequal', () => {
const suggestion1 = new SuggestedFix(
'Replace me!',
location1,
'With this!'
);
const suggestion2 = new SuggestedFix(
'Not the same title!',
location1,
'With this!'
);
assert(!suggestion1.isEqual(suggestion2));
});
it('should treat instances with different replacements as inequal', () => {
const suggestion1 = new SuggestedFix(
'Replace me!',
location1,
'With this!'
);
const suggestion2 = new SuggestedFix(
'Replace me!',
location1,
'With something else!'
);
assert(!suggestion1.isEqual(suggestion2));
});
it('should treat instances with different locations as inequal', () => {
const suggestion1 = new SuggestedFix(
'Replace me!',
location1,
'With this!'
);
const suggestion2 = new SuggestedFix(
'Replace me!',
location2,
'With this!'
);
assert(!suggestion1.isEqual(suggestion2));
});
it('should treat instances with different applicability as inequal', () => {
const suggestion1 = new SuggestedFix(
'Replace me!',
location1,
'With this!',
SuggestionApplicability.MachineApplicable
);
const suggestion2 = new SuggestedFix(
'Replace me!',
location2,
'With this!',
SuggestionApplicability.HasPlaceholders
);
assert(!suggestion1.isEqual(suggestion2));
});
});
describe('toCodeAction', () => {
it('should map a simple suggestion', () => {
const suggestion = new SuggestedFix(
'Replace me!',
location1,
'With this!'
);
const codeAction = suggestion.toCodeAction();
assert.strictEqual(codeAction.kind, vscode.CodeActionKind.QuickFix);
assert.strictEqual(codeAction.title, 'Replace me!');
assert.strictEqual(codeAction.isPreferred, false);
const edit = codeAction.edit;
if (!edit) {
return assert.fail('Code Action edit unexpectedly missing');
}
const editEntries = edit.entries();
assert.strictEqual(editEntries.length, 1);
const [[editUri, textEdits]] = editEntries;
assert.strictEqual(editUri.toString(), location1.uri.toString());
assert.strictEqual(textEdits.length, 1);
const [textEdit] = textEdits;
assert(textEdit.range.isEqual(location1.range));
assert.strictEqual(textEdit.newText, 'With this!');
});
});
});

125
editors/code/src/test/utils/diagnotics/SuggestedFixCollection.test.ts Normal file
View file

@ -0,0 +1,125 @@
import * as assert from 'assert';
import * as vscode from 'vscode';
import SuggestedFix from '../../../utils/diagnostics/SuggestedFix';
import SuggestedFixCollection from '../../../utils/diagnostics/SuggestedFixCollection';
const uri1 = vscode.Uri.file('/file/1');
const uri2 = vscode.Uri.file('/file/2');
const mockDocument1 = ({
uri: uri1
} as unknown) as vscode.TextDocument;
const mockDocument2 = ({
uri: uri2
} as unknown) as vscode.TextDocument;
const range1 = new vscode.Range(
new vscode.Position(1, 2),
new vscode.Position(3, 4)
);
const range2 = new vscode.Range(
new vscode.Position(5, 6),
new vscode.Position(7, 8)
);
const diagnostic1 = new vscode.Diagnostic(range1, 'First diagnostic');
const diagnostic2 = new vscode.Diagnostic(range2, 'Second diagnostic');
// This is a mutable object so return a fresh instance every time
function suggestion1(): SuggestedFix {
return new SuggestedFix(
'Replace me!',
new vscode.Location(uri1, range1),
'With this!'
);
}
describe('SuggestedFixCollection', () => {
it('should add a suggestion then return it as a code action', () => {
const suggestedFixes = new SuggestedFixCollection();
suggestedFixes.addSuggestedFixForDiagnostic(suggestion1(), diagnostic1);
// Specify the document and range that exactly matches
const codeActions = suggestedFixes.provideCodeActions(
mockDocument1,
range1
);
assert.strictEqual(codeActions.length, 1);
const [codeAction] = codeActions;
assert.strictEqual(codeAction.title, suggestion1().title);
const { diagnostics } = codeAction;
if (!diagnostics) {
return assert.fail('Diagnostics unexpectedly missing');
}
assert.strictEqual(diagnostics.length, 1);
assert.strictEqual(diagnostics[0], diagnostic1);
});
it('should not return code actions for different ranges', () => {
const suggestedFixes = new SuggestedFixCollection();
suggestedFixes.addSuggestedFixForDiagnostic(suggestion1(), diagnostic1);
const codeActions = suggestedFixes.provideCodeActions(
mockDocument1,
range2
);
assert(!codeActions || codeActions.length === 0);
});
it('should not return code actions for different documents', () => {
const suggestedFixes = new SuggestedFixCollection();
suggestedFixes.addSuggestedFixForDiagnostic(suggestion1(), diagnostic1);
const codeActions = suggestedFixes.provideCodeActions(
mockDocument2,
range1
);
assert(!codeActions || codeActions.length === 0);
});
it('should not return code actions that have been cleared', () => {
const suggestedFixes = new SuggestedFixCollection();
suggestedFixes.addSuggestedFixForDiagnostic(suggestion1(), diagnostic1);
suggestedFixes.clear();
const codeActions = suggestedFixes.provideCodeActions(
mockDocument1,
range1
);
assert(!codeActions || codeActions.length === 0);
});
it('should merge identical suggestions together', () => {
const suggestedFixes = new SuggestedFixCollection();
// Add the same suggestion for two diagnostics
suggestedFixes.addSuggestedFixForDiagnostic(suggestion1(), diagnostic1);
suggestedFixes.addSuggestedFixForDiagnostic(suggestion1(), diagnostic2);
const codeActions = suggestedFixes.provideCodeActions(
mockDocument1,
range1
);
assert.strictEqual(codeActions.length, 1);
const [codeAction] = codeActions;
const { diagnostics } = codeAction;
if (!diagnostics) {
return assert.fail('Diagnostics unexpectedly missing');
}
// We should be associated with both diagnostics
assert.strictEqual(diagnostics.length, 2);
assert.strictEqual(diagnostics[0], diagnostic1);
assert.strictEqual(diagnostics[1], diagnostic2);
});
});

55
editors/code/src/test/rust_diagnostics.test.ts → editors/code/src/test/utils/diagnotics/rust.test.ts
View file

@ -5,14 +5,15 @@ import * as vscode from 'vscode';
import {
MappedRustDiagnostic,
mapRustDiagnosticToVsCode,
RustDiagnostic
} from '../utils/rust_diagnostics';
RustDiagnostic,
SuggestionApplicability
} from '../../../utils/diagnostics/rust';
function loadDiagnosticFixture(name: string): RustDiagnostic {
const jsonText = fs
.readFileSync(
// We're actually in our JavaScript output directory, climb out
`${__dirname}/../../src/test/fixtures/rust-diagnostics/${name}.json`
`${__dirname}/../../../../src/test/fixtures/rust-diagnostics/${name}.json`
)
.toString();
@ -31,7 +32,9 @@ function mapFixtureToVsCode(name: string): MappedRustDiagnostic {
describe('mapRustDiagnosticToVsCode', () => {
it('should map an incompatible type for trait error', () => {
const { diagnostic, codeActions } = mapFixtureToVsCode('error/E0053');
const { diagnostic, suggestedFixes } = mapFixtureToVsCode(
'error/E0053'
);
assert.strictEqual(
diagnostic.severity,
@ -52,12 +55,12 @@ describe('mapRustDiagnosticToVsCode', () => {
// No related information
assert.deepStrictEqual(diagnostic.relatedInformation, []);
// There are no code actions available
assert.strictEqual(codeActions.length, 0);
// There are no suggested fixes
assert.strictEqual(suggestedFixes.length, 0);
});
it('should map an unused variable warning', () => {
const { diagnostic, codeActions } = mapFixtureToVsCode(
const { diagnostic, suggestedFixes } = mapFixtureToVsCode(
'warning/unused_variables'
);
@ -81,18 +84,23 @@ describe('mapRustDiagnosticToVsCode', () => {
// No related information
assert.deepStrictEqual(diagnostic.relatedInformation, []);
// One code action available to prefix the variable
assert.strictEqual(codeActions.length, 1);
const [codeAction] = codeActions;
// One suggested fix available to prefix the variable
assert.strictEqual(suggestedFixes.length, 1);
const [suggestedFix] = suggestedFixes;
assert.strictEqual(
codeAction.title,
suggestedFix.title,
'consider prefixing with an underscore: `_foo`'
);
assert(codeAction.isPreferred);
assert.strictEqual(
suggestedFix.applicability,
SuggestionApplicability.MachineApplicable
);
});
it('should map a wrong number of parameters error', () => {
const { diagnostic, codeActions } = mapFixtureToVsCode('error/E0061');
const { diagnostic, suggestedFixes } = mapFixtureToVsCode(
'error/E0061'
);
assert.strictEqual(
diagnostic.severity,
@ -115,12 +123,12 @@ describe('mapRustDiagnosticToVsCode', () => {
const [related] = relatedInformation;
assert.strictEqual(related.message, 'defined here');
// There are no actions available
assert.strictEqual(codeActions.length, 0);
// There are no suggested fixes
assert.strictEqual(suggestedFixes.length, 0);
});
it('should map a Clippy copy pass by ref warning', () => {
const { diagnostic, codeActions } = mapFixtureToVsCode(
const { diagnostic, suggestedFixes } = mapFixtureToVsCode(
'clippy/trivially_copy_pass_by_ref'
);
@ -149,14 +157,17 @@ describe('mapRustDiagnosticToVsCode', () => {
const [related] = relatedInformation;
assert.strictEqual(related.message, 'lint level defined here');
// One code action available to pass by value
assert.strictEqual(codeActions.length, 1);
const [codeAction] = codeActions;
// One suggested fix to pass by value
assert.strictEqual(suggestedFixes.length, 1);
const [suggestedFix] = suggestedFixes;
assert.strictEqual(
codeAction.title,
suggestedFix.title,
'consider passing by value instead: `self`'
);
// Clippy does not mark this as machine applicable
assert.strictEqual(codeAction.isPreferred, false);
// Clippy does not mark this with any applicability
assert.strictEqual(
suggestedFix.applicability,
SuggestionApplicability.Unspecified
);
});
});

86
editors/code/src/test/vscode_diagnostics.test.ts → editors/code/src/test/utils/diagnotics/vscode.test.ts
View file

@ -1,12 +1,7 @@
import * as assert from 'assert';
import * as vscode from 'vscode';
import {
areCodeActionsEqual,
areDiagnosticsEqual
} from '../utils/vscode_diagnostics';
const uri = vscode.Uri.file('/file/1');
import { areDiagnosticsEqual } from '../../../utils/diagnostics/vscode';
const range1 = new vscode.Range(
new vscode.Position(1, 2),
@ -101,82 +96,3 @@ describe('areDiagnosticsEqual', () => {
assert(!areDiagnosticsEqual(diagnostic1, diagnostic2));
});
});
describe('areCodeActionsEqual', () => {
it('should treat identical actions as equal', () => {
const codeAction1 = new vscode.CodeAction(
'Fix me!',
vscode.CodeActionKind.QuickFix
);
const codeAction2 = new vscode.CodeAction(
'Fix me!',
vscode.CodeActionKind.QuickFix
);
const edit = new vscode.WorkspaceEdit();
edit.replace(uri, range1, 'Replace with this');
codeAction1.edit = edit;
codeAction2.edit = edit;
assert(areCodeActionsEqual(codeAction1, codeAction2));
});
it('should treat actions with different types as inequal', () => {
const codeAction1 = new vscode.CodeAction(
'Fix me!',
vscode.CodeActionKind.Refactor
);
const codeAction2 = new vscode.CodeAction(
'Fix me!',
vscode.CodeActionKind.QuickFix
);
const edit = new vscode.WorkspaceEdit();
edit.replace(uri, range1, 'Replace with this');
codeAction1.edit = edit;
codeAction2.edit = edit;
assert(!areCodeActionsEqual(codeAction1, codeAction2));
});
it('should treat actions with different titles as inequal', () => {
const codeAction1 = new vscode.CodeAction(
'Fix me!',
vscode.CodeActionKind.Refactor
);
const codeAction2 = new vscode.CodeAction(
'Do something different!',
vscode.CodeActionKind.Refactor
);
const edit = new vscode.WorkspaceEdit();
edit.replace(uri, range1, 'Replace with this');
codeAction1.edit = edit;
codeAction2.edit = edit;
assert(!areCodeActionsEqual(codeAction1, codeAction2));
});
it('should treat actions with different edits as inequal', () => {
const codeAction1 = new vscode.CodeAction(
'Fix me!',
vscode.CodeActionKind.Refactor
);
const edit1 = new vscode.WorkspaceEdit();
edit1.replace(uri, range1, 'Replace with this');
codeAction1.edit = edit1;
const codeAction2 = new vscode.CodeAction(
'Fix me!',
vscode.CodeActionKind.Refactor
);
const edit2 = new vscode.WorkspaceEdit();
edit2.replace(uri, range1, 'Replace with this other thing');
codeAction2.edit = edit2;
assert(!areCodeActionsEqual(codeAction1, codeAction2));
});
});