Fix cargo watch code action filtering

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.

Author: etaoins

editors/code/src/commands/cargo_watch.ts

@@ -5,16 +5,15 @@ import * as vscode from 'vscode';
 
 import { Server } from '../server';
 import { terminate } from '../utils/processes';
+import { LineBuffer } from './line_buffer';
+import { StatusDisplay } from './watch_status';
+
 import {
     mapRustDiagnosticToVsCode,
     RustDiagnostic
-} from '../utils/rust_diagnostics';
-import {
-    areCodeActionsEqual,
-    areDiagnosticsEqual
-} from '../utils/vscode_diagnostics';
-import { LineBuffer } from './line_buffer';
-import { StatusDisplay } from './watch_status';
+} from '../utils/diagnostics/rust';
+import SuggestedFixCollection from '../utils/diagnostics/SuggestedFixCollection';
+import { areDiagnosticsEqual } from '../utils/diagnostics/vscode';
 
 export function registerCargoWatchProvider(
     subscriptions: vscode.Disposable[]
@@ -42,16 +41,13 @@ export function registerCargoWatchProvider(
     return provider;
 }
 
-export class CargoWatchProvider
-    implements vscode.Disposable, vscode.CodeActionProvider {
+export class CargoWatchProvider implements vscode.Disposable {
     private readonly diagnosticCollection: vscode.DiagnosticCollection;
     private readonly statusDisplay: StatusDisplay;
     private readonly outputChannel: vscode.OutputChannel;
 
-    private codeActions: {
-        [fileUri: string]: vscode.CodeAction[];
-    };
-    private readonly codeActionDispose: vscode.Disposable;
+    private suggestedFixCollection: SuggestedFixCollection;
+    private codeActionDispose: vscode.Disposable;
 
     private cargoProcess?: child_process.ChildProcess;
 
@@ -66,13 +62,14 @@ export class CargoWatchProvider
             'Cargo Watch Trace'
         );
 
-        // Register code actions for rustc's suggested fixes
-        this.codeActions = {};
+        // Track `rustc`'s suggested fixes so we can convert them to code actions
+        this.suggestedFixCollection = new SuggestedFixCollection();
         this.codeActionDispose = vscode.languages.registerCodeActionsProvider(
             [{ scheme: 'file', language: 'rust' }],
-            this,
+            this.suggestedFixCollection,
             {
-                providedCodeActionKinds: [vscode.CodeActionKind.QuickFix]
+                providedCodeActionKinds:
+                    SuggestedFixCollection.PROVIDED_CODE_ACTION_KINDS
             }
         );
     }
@@ -156,13 +153,6 @@ export class CargoWatchProvider
         this.codeActionDispose.dispose();
     }
 
-    public provideCodeActions(
-        document: vscode.TextDocument
-    ): vscode.ProviderResult<Array<vscode.Command | vscode.CodeAction>> {
-        const documentActions = this.codeActions[document.uri.toString()];
-        return documentActions || [];
-    }
-
     private logInfo(line: string) {
         if (Server.config.cargoWatchOptions.trace === 'verbose') {
             this.outputChannel.append(line);
@@ -181,7 +171,7 @@ export class CargoWatchProvider
     private parseLine(line: string) {
         if (line.startsWith('[Running')) {
             this.diagnosticCollection.clear();
-            this.codeActions = {};
+            this.suggestedFixCollection.clear();
             this.statusDisplay.show();
         }
 
@@ -225,7 +215,7 @@ export class CargoWatchProvider
                 return;
             }
 
-            const { location, diagnostic, codeActions } = mapResult;
+            const { location, diagnostic, suggestedFixes } = mapResult;
             const fileUri = location.uri;
 
             const diagnostics: vscode.Diagnostic[] = [
@@ -236,37 +226,22 @@ export class CargoWatchProvider
             const isDuplicate = diagnostics.some(d =>
                 areDiagnosticsEqual(d, diagnostic)
             );
-
             if (isDuplicate) {
                 return;
             }
 
             diagnostics.push(diagnostic);
             this.diagnosticCollection!.set(fileUri, diagnostics);
 
-            if (codeActions.length) {
-                const fileUriString = fileUri.toString();
-                const existingActions = this.codeActions[fileUriString] || [];
-
-                for (const newAction of codeActions) {
-                    const existingAction = existingActions.find(existing =>
-                        areCodeActionsEqual(existing, newAction)
+            if (suggestedFixes.length) {
+                for (const suggestedFix of suggestedFixes) {
+                    this.suggestedFixCollection.addSuggestedFixForDiagnostic(
+                        suggestedFix,
+                        diagnostic
                     );
-
-                    if (existingAction) {
-                        if (!existingAction.diagnostics) {
-                            existingAction.diagnostics = [];
-                        }
-                        // This action also applies to this diagnostic
-                        existingAction.diagnostics.push(diagnostic);
-                    } else {
-                        newAction.diagnostics = [diagnostic];
-                        existingActions.push(newAction);
-                    }
                 }
 
                 // Have VsCode query us for the code actions
-                this.codeActions[fileUriString] = existingActions;
                 vscode.commands.executeCommand(
                     'vscode.executeCodeActionProvider',
                     fileUri,

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!');
+        });
+    });
+});

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);
+    });
+});