Merge #1454

1454: Fix `cargo watch` code action filtering r=etaoins a=etaoins

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.

Co-authored-by: Ryan Cumming <[email protected]>

4 files changed, 292 insertions, 107 deletions

diff --git a/editors/code/src/test/utils/diagnotics/SuggestedFix.test.ts b/editors/code/src/test/utils/diagnotics/SuggestedFix.test.ts
new file mode 100644
index 000000000..6c7f436f3
--- /dev/null
+++ b/editors/code/src/test/utils/diagnotics/SuggestedFix.test.ts
@@ -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!');
+        });
+    });
+});
diff --git a/editors/code/src/test/utils/diagnotics/SuggestedFixCollection.test.ts b/editors/code/src/test/utils/diagnotics/SuggestedFixCollection.test.ts
new file mode 100644
index 000000000..f0328893e
--- /dev/null
+++ b/editors/code/src/test/utils/diagnotics/SuggestedFixCollection.test.ts
@@ -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);
+    });
+});
diff --git a/editors/code/src/test/rust_diagnostics.test.ts b/editors/code/src/test/utils/diagnotics/rust.test.ts
index f27c58fe2..b555a4819 100644
--- a/editors/code/src/test/rust_diagnostics.test.ts
+++ b/editors/code/src/test/utils/diagnotics/rust.test.ts
@@ -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
+        );
     });
 });
diff --git a/editors/code/src/test/vscode_diagnostics.test.ts b/editors/code/src/test/utils/diagnotics/vscode.test.ts
index 9c5d812fa..542dec1f5 100644
--- a/editors/code/src/test/vscode_diagnostics.test.ts
+++ b/editors/code/src/test/utils/diagnotics/vscode.test.ts
@@ -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));
-    });
-});