From abc0784e57610a0cceca63301489918015418df6 Mon Sep 17 00:00:00 2001 From: Ryan Cumming Date: Thu, 27 Jun 2019 21:30:23 +1000 Subject: 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. --- .../src/test/utils/diagnotics/SuggestedFix.test.ts | 133 ++++++++++++++++ .../diagnotics/SuggestedFixCollection.test.ts | 125 +++++++++++++++ .../code/src/test/utils/diagnotics/rust.test.ts | 173 +++++++++++++++++++++ .../code/src/test/utils/diagnotics/vscode.test.ts | 98 ++++++++++++ 4 files changed, 529 insertions(+) create mode 100644 editors/code/src/test/utils/diagnotics/SuggestedFix.test.ts create mode 100644 editors/code/src/test/utils/diagnotics/SuggestedFixCollection.test.ts create mode 100644 editors/code/src/test/utils/diagnotics/rust.test.ts create mode 100644 editors/code/src/test/utils/diagnotics/vscode.test.ts (limited to 'editors/code/src/test/utils') 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/utils/diagnotics/rust.test.ts b/editors/code/src/test/utils/diagnotics/rust.test.ts new file mode 100644 index 000000000..b555a4819 --- /dev/null +++ b/editors/code/src/test/utils/diagnotics/rust.test.ts @@ -0,0 +1,173 @@ +import * as assert from 'assert'; +import * as fs from 'fs'; +import * as vscode from 'vscode'; + +import { + MappedRustDiagnostic, + mapRustDiagnosticToVsCode, + 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` + ) + .toString(); + + return JSON.parse(jsonText); +} + +function mapFixtureToVsCode(name: string): MappedRustDiagnostic { + const rd = loadDiagnosticFixture(name); + const mapResult = mapRustDiagnosticToVsCode(rd); + + if (!mapResult) { + return assert.fail('Mapping unexpectedly failed'); + } + return mapResult; +} + +describe('mapRustDiagnosticToVsCode', () => { + it('should map an incompatible type for trait error', () => { + const { diagnostic, suggestedFixes } = mapFixtureToVsCode( + 'error/E0053' + ); + + assert.strictEqual( + diagnostic.severity, + vscode.DiagnosticSeverity.Error + ); + assert.strictEqual(diagnostic.source, 'rustc'); + assert.strictEqual( + diagnostic.message, + [ + `method \`next\` has an incompatible type for trait`, + `expected type \`fn(&mut ty::list_iter::ListIterator<'list, M>) -> std::option::Option<&ty::Ref>\``, + ` found type \`fn(&ty::list_iter::ListIterator<'list, M>) -> std::option::Option<&'list ty::Ref>\`` + ].join('\n') + ); + assert.strictEqual(diagnostic.code, 'E0053'); + assert.strictEqual(diagnostic.tags, undefined); + + // No related information + assert.deepStrictEqual(diagnostic.relatedInformation, []); + + // There are no suggested fixes + assert.strictEqual(suggestedFixes.length, 0); + }); + + it('should map an unused variable warning', () => { + const { diagnostic, suggestedFixes } = mapFixtureToVsCode( + 'warning/unused_variables' + ); + + assert.strictEqual( + diagnostic.severity, + vscode.DiagnosticSeverity.Warning + ); + assert.strictEqual( + diagnostic.message, + [ + 'unused variable: `foo`', + '#[warn(unused_variables)] on by default' + ].join('\n') + ); + assert.strictEqual(diagnostic.code, 'unused_variables'); + assert.strictEqual(diagnostic.source, 'rustc'); + assert.deepStrictEqual(diagnostic.tags, [ + vscode.DiagnosticTag.Unnecessary + ]); + + // No related information + assert.deepStrictEqual(diagnostic.relatedInformation, []); + + // One suggested fix available to prefix the variable + assert.strictEqual(suggestedFixes.length, 1); + const [suggestedFix] = suggestedFixes; + assert.strictEqual( + suggestedFix.title, + 'consider prefixing with an underscore: `_foo`' + ); + assert.strictEqual( + suggestedFix.applicability, + SuggestionApplicability.MachineApplicable + ); + }); + + it('should map a wrong number of parameters error', () => { + const { diagnostic, suggestedFixes } = mapFixtureToVsCode( + 'error/E0061' + ); + + assert.strictEqual( + diagnostic.severity, + vscode.DiagnosticSeverity.Error + ); + assert.strictEqual( + diagnostic.message, + 'this function takes 2 parameters but 3 parameters were supplied' + ); + assert.strictEqual(diagnostic.code, 'E0061'); + assert.strictEqual(diagnostic.source, 'rustc'); + assert.strictEqual(diagnostic.tags, undefined); + + // One related information for the original definition + const relatedInformation = diagnostic.relatedInformation; + if (!relatedInformation) { + return assert.fail('Related information unexpectedly undefined'); + } + assert.strictEqual(relatedInformation.length, 1); + const [related] = relatedInformation; + assert.strictEqual(related.message, 'defined here'); + + // There are no suggested fixes + assert.strictEqual(suggestedFixes.length, 0); + }); + + it('should map a Clippy copy pass by ref warning', () => { + const { diagnostic, suggestedFixes } = mapFixtureToVsCode( + 'clippy/trivially_copy_pass_by_ref' + ); + + assert.strictEqual( + diagnostic.severity, + vscode.DiagnosticSeverity.Warning + ); + assert.strictEqual(diagnostic.source, 'clippy'); + assert.strictEqual( + diagnostic.message, + [ + 'this argument is passed by reference, but would be more efficient if passed by value', + '#[warn(clippy::trivially_copy_pass_by_ref)] implied by #[warn(clippy::all)]', + 'for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref' + ].join('\n') + ); + assert.strictEqual(diagnostic.code, 'trivially_copy_pass_by_ref'); + assert.strictEqual(diagnostic.tags, undefined); + + // One related information for the lint definition + const relatedInformation = diagnostic.relatedInformation; + if (!relatedInformation) { + return assert.fail('Related information unexpectedly undefined'); + } + assert.strictEqual(relatedInformation.length, 1); + const [related] = relatedInformation; + assert.strictEqual(related.message, 'lint level defined here'); + + // One suggested fix to pass by value + assert.strictEqual(suggestedFixes.length, 1); + const [suggestedFix] = suggestedFixes; + assert.strictEqual( + suggestedFix.title, + 'consider passing by value instead: `self`' + ); + // Clippy does not mark this with any applicability + assert.strictEqual( + suggestedFix.applicability, + SuggestionApplicability.Unspecified + ); + }); +}); diff --git a/editors/code/src/test/utils/diagnotics/vscode.test.ts b/editors/code/src/test/utils/diagnotics/vscode.test.ts new file mode 100644 index 000000000..542dec1f5 --- /dev/null +++ b/editors/code/src/test/utils/diagnotics/vscode.test.ts @@ -0,0 +1,98 @@ +import * as assert from 'assert'; +import * as vscode from 'vscode'; + +import { areDiagnosticsEqual } from '../../../utils/diagnostics/vscode'; + +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) +); + +describe('areDiagnosticsEqual', () => { + it('should treat identical diagnostics as equal', () => { + const diagnostic1 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + + const diagnostic2 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + + assert(areDiagnosticsEqual(diagnostic1, diagnostic2)); + }); + + it('should treat diagnostics with different sources as inequal', () => { + const diagnostic1 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + diagnostic1.source = 'rustc'; + + const diagnostic2 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + diagnostic2.source = 'clippy'; + + assert(!areDiagnosticsEqual(diagnostic1, diagnostic2)); + }); + + it('should treat diagnostics with different ranges as inequal', () => { + const diagnostic1 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + + const diagnostic2 = new vscode.Diagnostic( + range2, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + + assert(!areDiagnosticsEqual(diagnostic1, diagnostic2)); + }); + + it('should treat diagnostics with different messages as inequal', () => { + const diagnostic1 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + + const diagnostic2 = new vscode.Diagnostic( + range1, + 'Goodbye!, world!', + vscode.DiagnosticSeverity.Error + ); + + assert(!areDiagnosticsEqual(diagnostic1, diagnostic2)); + }); + + it('should treat diagnostics with different severities as inequal', () => { + const diagnostic1 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Warning + ); + + const diagnostic2 = new vscode.Diagnostic( + range1, + 'Hello, world!', + vscode.DiagnosticSeverity.Error + ); + + assert(!areDiagnosticsEqual(diagnostic1, diagnostic2)); + }); +}); -- cgit v1.2.3