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. --- editors/code/src/utils/diagnostics/SuggestedFix.ts | 67 ++++++ .../utils/diagnostics/SuggestedFixCollection.ts | 74 +++++++ editors/code/src/utils/diagnostics/rust.ts | 235 ++++++++++++++++++++ editors/code/src/utils/diagnostics/vscode.ts | 14 ++ editors/code/src/utils/rust_diagnostics.ts | 236 --------------------- editors/code/src/utils/vscode_diagnostics.ts | 73 ------- 6 files changed, 390 insertions(+), 309 deletions(-) create mode 100644 editors/code/src/utils/diagnostics/SuggestedFix.ts create mode 100644 editors/code/src/utils/diagnostics/SuggestedFixCollection.ts create mode 100644 editors/code/src/utils/diagnostics/rust.ts create mode 100644 editors/code/src/utils/diagnostics/vscode.ts delete mode 100644 editors/code/src/utils/rust_diagnostics.ts delete mode 100644 editors/code/src/utils/vscode_diagnostics.ts (limited to 'editors/code/src/utils') diff --git a/editors/code/src/utils/diagnostics/SuggestedFix.ts b/editors/code/src/utils/diagnostics/SuggestedFix.ts new file mode 100644 index 000000000..b1be2a225 --- /dev/null +++ b/editors/code/src/utils/diagnostics/SuggestedFix.ts @@ -0,0 +1,67 @@ +import * as vscode from 'vscode'; + +import { SuggestionApplicability } from './rust'; + +/** + * Model object for text replacements suggested by the Rust compiler + * + * This is an intermediate form between the raw `rustc` JSON and a + * `vscode.CodeAction`. It's optimised for the use-cases of + * `SuggestedFixCollection`. + */ +export default class SuggestedFix { + public readonly title: string; + public readonly location: vscode.Location; + public readonly replacement: string; + public readonly applicability: SuggestionApplicability; + + /** + * Diagnostics this suggested fix could resolve + */ + public diagnostics: vscode.Diagnostic[]; + + constructor( + title: string, + location: vscode.Location, + replacement: string, + applicability: SuggestionApplicability = SuggestionApplicability.Unspecified + ) { + this.title = title; + this.location = location; + this.replacement = replacement; + this.applicability = applicability; + this.diagnostics = []; + } + + /** + * Determines if this suggested fix is equivalent to another instance + */ + public isEqual(other: SuggestedFix): boolean { + return ( + this.title === other.title && + this.location.range.isEqual(other.location.range) && + this.replacement === other.replacement && + this.applicability === other.applicability + ); + } + + /** + * Converts this suggested fix to a VS Code Quick Fix code action + */ + public toCodeAction(): vscode.CodeAction { + const codeAction = new vscode.CodeAction( + this.title, + vscode.CodeActionKind.QuickFix + ); + + const edit = new vscode.WorkspaceEdit(); + edit.replace(this.location.uri, this.location.range, this.replacement); + codeAction.edit = edit; + + codeAction.isPreferred = + this.applicability === SuggestionApplicability.MachineApplicable; + + codeAction.diagnostics = [...this.diagnostics]; + return codeAction; + } +} diff --git a/editors/code/src/utils/diagnostics/SuggestedFixCollection.ts b/editors/code/src/utils/diagnostics/SuggestedFixCollection.ts new file mode 100644 index 000000000..3b0bf7468 --- /dev/null +++ b/editors/code/src/utils/diagnostics/SuggestedFixCollection.ts @@ -0,0 +1,74 @@ +import * as vscode from 'vscode'; +import SuggestedFix from './SuggestedFix'; + +/** + * Collection of suggested fixes across multiple documents + * + * This stores `SuggestedFix` model objects and returns them via the + * `vscode.CodeActionProvider` interface. + */ +export default class SuggestedFixCollection + implements vscode.CodeActionProvider { + public static PROVIDED_CODE_ACTION_KINDS = [vscode.CodeActionKind.QuickFix]; + + private suggestedFixes: Map; + + constructor() { + this.suggestedFixes = new Map(); + } + + /** + * Clears all suggested fixes across all documents + */ + public clear(): void { + this.suggestedFixes = new Map(); + } + + /** + * Adds a suggested fix for the given diagnostic + * + * Some suggested fixes will appear in multiple diagnostics. For example, + * forgetting a `mut` on a variable will suggest changing the delaration on + * every mutable usage site. If the suggested fix has already been added + * this method will instead associate the existing fix with the new + * diagnostic. + */ + public addSuggestedFixForDiagnostic( + suggestedFix: SuggestedFix, + diagnostic: vscode.Diagnostic + ): void { + const fileUriString = suggestedFix.location.uri.toString(); + const fileSuggestions = this.suggestedFixes.get(fileUriString) || []; + + const existingSuggestion = fileSuggestions.find(s => + s.isEqual(suggestedFix) + ); + + if (existingSuggestion) { + // The existing suggestion also applies to this new diagnostic + existingSuggestion.diagnostics.push(diagnostic); + } else { + // We haven't seen this suggestion before + suggestedFix.diagnostics.push(diagnostic); + fileSuggestions.push(suggestedFix); + } + + this.suggestedFixes.set(fileUriString, fileSuggestions); + } + + /** + * Filters suggested fixes by their document and range and converts them to + * code actions + */ + public provideCodeActions( + document: vscode.TextDocument, + range: vscode.Range + ): vscode.CodeAction[] { + const documentUriString = document.uri.toString(); + + const suggestedFixes = this.suggestedFixes.get(documentUriString); + return (suggestedFixes || []) + .filter(({ location }) => location.range.intersection(range)) + .map(suggestedEdit => suggestedEdit.toCodeAction()); + } +} diff --git a/editors/code/src/utils/diagnostics/rust.ts b/editors/code/src/utils/diagnostics/rust.ts new file mode 100644 index 000000000..d16576eb1 --- /dev/null +++ b/editors/code/src/utils/diagnostics/rust.ts @@ -0,0 +1,235 @@ +import * as path from 'path'; +import * as vscode from 'vscode'; + +import SuggestedFix from './SuggestedFix'; + +export enum SuggestionApplicability { + MachineApplicable = 'MachineApplicable', + HasPlaceholders = 'HasPlaceholders', + MaybeIncorrect = 'MaybeIncorrect', + Unspecified = 'Unspecified' +} + +// Reference: +// https://github.com/rust-lang/rust/blob/master/src/libsyntax/json.rs +export interface RustDiagnosticSpan { + line_start: number; + line_end: number; + column_start: number; + column_end: number; + is_primary: boolean; + file_name: string; + label?: string; + suggested_replacement?: string; + suggestion_applicability?: SuggestionApplicability; +} + +export interface RustDiagnostic { + spans: RustDiagnosticSpan[]; + rendered: string; + message: string; + level: string; + code?: { + code: string; + }; + children: RustDiagnostic[]; +} + +export interface MappedRustDiagnostic { + location: vscode.Location; + diagnostic: vscode.Diagnostic; + suggestedFixes: SuggestedFix[]; +} + +interface MappedRustChildDiagnostic { + related?: vscode.DiagnosticRelatedInformation; + suggestedFix?: SuggestedFix; + messageLine?: string; +} + +/** + * Converts a Rust level string to a VsCode severity + */ +function mapLevelToSeverity(s: string): vscode.DiagnosticSeverity { + if (s === 'error') { + return vscode.DiagnosticSeverity.Error; + } + if (s.startsWith('warn')) { + return vscode.DiagnosticSeverity.Warning; + } + return vscode.DiagnosticSeverity.Information; +} + +/** + * Converts a Rust span to a VsCode location + */ +function mapSpanToLocation(span: RustDiagnosticSpan): vscode.Location { + const fileName = path.join(vscode.workspace.rootPath!, span.file_name); + const fileUri = vscode.Uri.file(fileName); + + const range = new vscode.Range( + new vscode.Position(span.line_start - 1, span.column_start - 1), + new vscode.Position(span.line_end - 1, span.column_end - 1) + ); + + return new vscode.Location(fileUri, range); +} + +/** + * Converts a secondary Rust span to a VsCode related information + * + * If the span is unlabelled this will return `undefined`. + */ +function mapSecondarySpanToRelated( + span: RustDiagnosticSpan +): vscode.DiagnosticRelatedInformation | undefined { + if (!span.label) { + // Nothing to label this with + return; + } + + const location = mapSpanToLocation(span); + return new vscode.DiagnosticRelatedInformation(location, span.label); +} + +/** + * Determines if diagnostic is related to unused code + */ +function isUnusedOrUnnecessary(rd: RustDiagnostic): boolean { + if (!rd.code) { + return false; + } + + return [ + 'dead_code', + 'unknown_lints', + 'unused_attributes', + 'unused_imports', + 'unused_macros', + 'unused_variables' + ].includes(rd.code.code); +} + +/** + * Converts a Rust child diagnostic to a VsCode related information + * + * This can have three outcomes: + * + * 1. If this is no primary span this will return a `noteLine` + * 2. If there is a primary span with a suggested replacement it will return a + * `codeAction`. + * 3. If there is a primary span without a suggested replacement it will return + * a `related`. + */ +function mapRustChildDiagnostic(rd: RustDiagnostic): MappedRustChildDiagnostic { + const span = rd.spans.find(s => s.is_primary); + + if (!span) { + // `rustc` uses these spanless children as a way to print multi-line + // messages + return { messageLine: rd.message }; + } + + // If we have a primary span use its location, otherwise use the parent + const location = mapSpanToLocation(span); + + // We need to distinguish `null` from an empty string + if (span && typeof span.suggested_replacement === 'string') { + // Include our replacement in the title unless it's empty + const title = span.suggested_replacement + ? `${rd.message}: \`${span.suggested_replacement}\`` + : rd.message; + + return { + suggestedFix: new SuggestedFix( + title, + location, + span.suggested_replacement, + span.suggestion_applicability + ) + }; + } else { + const related = new vscode.DiagnosticRelatedInformation( + location, + rd.message + ); + + return { related }; + } +} + +/** + * Converts a Rust root diagnostic to VsCode form + * + * This flattens the Rust diagnostic by: + * + * 1. Creating a `vscode.Diagnostic` with the root message and primary span. + * 2. Adding any labelled secondary spans to `relatedInformation` + * 3. Categorising child diagnostics as either `SuggestedFix`es, + * `relatedInformation` or additional message lines. + * + * If the diagnostic has no primary span this will return `undefined` + */ +export function mapRustDiagnosticToVsCode( + rd: RustDiagnostic +): MappedRustDiagnostic | undefined { + const primarySpan = rd.spans.find(s => s.is_primary); + if (!primarySpan) { + return; + } + + const location = mapSpanToLocation(primarySpan); + const secondarySpans = rd.spans.filter(s => !s.is_primary); + + const severity = mapLevelToSeverity(rd.level); + + const vd = new vscode.Diagnostic(location.range, rd.message, severity); + + let source = 'rustc'; + let code = rd.code && rd.code.code; + if (code) { + // See if this is an RFC #2103 scoped lint (e.g. from Clippy) + const scopedCode = code.split('::'); + if (scopedCode.length === 2) { + [source, code] = scopedCode; + } + } + + vd.source = source; + vd.code = code; + vd.relatedInformation = []; + + for (const secondarySpan of secondarySpans) { + const related = mapSecondarySpanToRelated(secondarySpan); + if (related) { + vd.relatedInformation.push(related); + } + } + + const suggestedFixes = []; + for (const child of rd.children) { + const { related, suggestedFix, messageLine } = mapRustChildDiagnostic( + child + ); + + if (related) { + vd.relatedInformation.push(related); + } + if (suggestedFix) { + suggestedFixes.push(suggestedFix); + } + if (messageLine) { + vd.message += `\n${messageLine}`; + } + } + + if (isUnusedOrUnnecessary(rd)) { + vd.tags = [vscode.DiagnosticTag.Unnecessary]; + } + + return { + location, + diagnostic: vd, + suggestedFixes + }; +} diff --git a/editors/code/src/utils/diagnostics/vscode.ts b/editors/code/src/utils/diagnostics/vscode.ts new file mode 100644 index 000000000..d8b85b720 --- /dev/null +++ b/editors/code/src/utils/diagnostics/vscode.ts @@ -0,0 +1,14 @@ +import * as vscode from 'vscode'; + +/** Compares two `vscode.Diagnostic`s for equality */ +export function areDiagnosticsEqual( + left: vscode.Diagnostic, + right: vscode.Diagnostic +): boolean { + return ( + left.source === right.source && + left.severity === right.severity && + left.range.isEqual(right.range) && + left.message === right.message + ); +} diff --git a/editors/code/src/utils/rust_diagnostics.ts b/editors/code/src/utils/rust_diagnostics.ts deleted file mode 100644 index 3c524cb37..000000000 --- a/editors/code/src/utils/rust_diagnostics.ts +++ /dev/null @@ -1,236 +0,0 @@ -import * as path from 'path'; -import * as vscode from 'vscode'; - -// Reference: -// https://github.com/rust-lang/rust/blob/master/src/libsyntax/json.rs -export interface RustDiagnosticSpan { - line_start: number; - line_end: number; - column_start: number; - column_end: number; - is_primary: boolean; - file_name: string; - label?: string; - suggested_replacement?: string; - suggestion_applicability?: - | 'MachineApplicable' - | 'HasPlaceholders' - | 'MaybeIncorrect' - | 'Unspecified'; -} - -export interface RustDiagnostic { - spans: RustDiagnosticSpan[]; - rendered: string; - message: string; - level: string; - code?: { - code: string; - }; - children: RustDiagnostic[]; -} - -export interface MappedRustDiagnostic { - location: vscode.Location; - diagnostic: vscode.Diagnostic; - codeActions: vscode.CodeAction[]; -} - -interface MappedRustChildDiagnostic { - related?: vscode.DiagnosticRelatedInformation; - codeAction?: vscode.CodeAction; - messageLine?: string; -} - -/** - * Converts a Rust level string to a VsCode severity - */ -function mapLevelToSeverity(s: string): vscode.DiagnosticSeverity { - if (s === 'error') { - return vscode.DiagnosticSeverity.Error; - } - if (s.startsWith('warn')) { - return vscode.DiagnosticSeverity.Warning; - } - return vscode.DiagnosticSeverity.Information; -} - -/** - * Converts a Rust span to a VsCode location - */ -function mapSpanToLocation(span: RustDiagnosticSpan): vscode.Location { - const fileName = path.join(vscode.workspace.rootPath!, span.file_name); - const fileUri = vscode.Uri.file(fileName); - - const range = new vscode.Range( - new vscode.Position(span.line_start - 1, span.column_start - 1), - new vscode.Position(span.line_end - 1, span.column_end - 1) - ); - - return new vscode.Location(fileUri, range); -} - -/** - * Converts a secondary Rust span to a VsCode related information - * - * If the span is unlabelled this will return `undefined`. - */ -function mapSecondarySpanToRelated( - span: RustDiagnosticSpan -): vscode.DiagnosticRelatedInformation | undefined { - if (!span.label) { - // Nothing to label this with - return; - } - - const location = mapSpanToLocation(span); - return new vscode.DiagnosticRelatedInformation(location, span.label); -} - -/** - * Determines if diagnostic is related to unused code - */ -function isUnusedOrUnnecessary(rd: RustDiagnostic): boolean { - if (!rd.code) { - return false; - } - - return [ - 'dead_code', - 'unknown_lints', - 'unused_attributes', - 'unused_imports', - 'unused_macros', - 'unused_variables' - ].includes(rd.code.code); -} - -/** - * Converts a Rust child diagnostic to a VsCode related information - * - * This can have three outcomes: - * - * 1. If this is no primary span this will return a `noteLine` - * 2. If there is a primary span with a suggested replacement it will return a - * `codeAction`. - * 3. If there is a primary span without a suggested replacement it will return - * a `related`. - */ -function mapRustChildDiagnostic(rd: RustDiagnostic): MappedRustChildDiagnostic { - const span = rd.spans.find(s => s.is_primary); - - if (!span) { - // `rustc` uses these spanless children as a way to print multi-line - // messages - return { messageLine: rd.message }; - } - - // If we have a primary span use its location, otherwise use the parent - const location = mapSpanToLocation(span); - - // We need to distinguish `null` from an empty string - if (span && typeof span.suggested_replacement === 'string') { - const edit = new vscode.WorkspaceEdit(); - edit.replace(location.uri, location.range, span.suggested_replacement); - - // Include our replacement in the label unless it's empty - const title = span.suggested_replacement - ? `${rd.message}: \`${span.suggested_replacement}\`` - : rd.message; - - const codeAction = new vscode.CodeAction( - title, - vscode.CodeActionKind.QuickFix - ); - - codeAction.edit = edit; - codeAction.isPreferred = - span.suggestion_applicability === 'MachineApplicable'; - - return { codeAction }; - } else { - const related = new vscode.DiagnosticRelatedInformation( - location, - rd.message - ); - - return { related }; - } -} - -/** - * Converts a Rust root diagnostic to VsCode form - * - * This flattens the Rust diagnostic by: - * - * 1. Creating a `vscode.Diagnostic` with the root message and primary span. - * 2. Adding any labelled secondary spans to `relatedInformation` - * 3. Categorising child diagnostics as either Quick Fix actions, - * `relatedInformation` or additional message lines. - * - * If the diagnostic has no primary span this will return `undefined` - */ -export function mapRustDiagnosticToVsCode( - rd: RustDiagnostic -): MappedRustDiagnostic | undefined { - const codeActions = []; - - const primarySpan = rd.spans.find(s => s.is_primary); - if (!primarySpan) { - return; - } - - const location = mapSpanToLocation(primarySpan); - const secondarySpans = rd.spans.filter(s => !s.is_primary); - - const severity = mapLevelToSeverity(rd.level); - - const vd = new vscode.Diagnostic(location.range, rd.message, severity); - - let source = 'rustc'; - let code = rd.code && rd.code.code; - if (code) { - // See if this is an RFC #2103 scoped lint (e.g. from Clippy) - const scopedCode = code.split('::'); - if (scopedCode.length === 2) { - [source, code] = scopedCode; - } - } - - vd.source = source; - vd.code = code; - vd.relatedInformation = []; - - for (const secondarySpan of secondarySpans) { - const related = mapSecondarySpanToRelated(secondarySpan); - if (related) { - vd.relatedInformation.push(related); - } - } - - for (const child of rd.children) { - const { related, codeAction, messageLine } = mapRustChildDiagnostic( - child - ); - - if (related) { - vd.relatedInformation.push(related); - } - if (codeAction) { - codeActions.push(codeAction); - } - if (messageLine) { - vd.message += `\n${messageLine}`; - } - } - - if (isUnusedOrUnnecessary(rd)) { - vd.tags = [vscode.DiagnosticTag.Unnecessary]; - } - - return { - location, - diagnostic: vd, - codeActions - }; -} diff --git a/editors/code/src/utils/vscode_diagnostics.ts b/editors/code/src/utils/vscode_diagnostics.ts deleted file mode 100644 index 9d763c8d6..000000000 --- a/editors/code/src/utils/vscode_diagnostics.ts +++ /dev/null @@ -1,73 +0,0 @@ -import * as vscode from 'vscode'; - -/** Compares two `vscode.Diagnostic`s for equality */ -export function areDiagnosticsEqual( - left: vscode.Diagnostic, - right: vscode.Diagnostic -): boolean { - return ( - left.source === right.source && - left.severity === right.severity && - left.range.isEqual(right.range) && - left.message === right.message - ); -} - -/** Compares two `vscode.TextEdit`s for equality */ -function areTextEditsEqual( - left: vscode.TextEdit, - right: vscode.TextEdit -): boolean { - if (!left.range.isEqual(right.range)) { - return false; - } - - if (left.newText !== right.newText) { - return false; - } - - return true; -} - -/** Compares two `vscode.CodeAction`s for equality */ -export function areCodeActionsEqual( - left: vscode.CodeAction, - right: vscode.CodeAction -): boolean { - if ( - left.kind !== right.kind || - left.title !== right.title || - !left.edit || - !right.edit - ) { - return false; - } - - const leftEditEntries = left.edit.entries(); - const rightEditEntries = right.edit.entries(); - - if (leftEditEntries.length !== rightEditEntries.length) { - return false; - } - - for (let i = 0; i < leftEditEntries.length; i++) { - const [leftUri, leftEdits] = leftEditEntries[i]; - const [rightUri, rightEdits] = rightEditEntries[i]; - - if (leftUri.toString() !== rightUri.toString()) { - return false; - } - - if (leftEdits.length !== rightEdits.length) { - return false; - } - - for (let j = 0; j < leftEdits.length; j++) { - if (!areTextEditsEqual(leftEdits[j], rightEdits[j])) { - return false; - } - } - } - - return true; -} -- cgit v1.2.3 From 50c6ab709e38b63fb1e3ee5db40426ef131cbd71 Mon Sep 17 00:00:00 2001 From: Ryan Cumming Date: Sat, 29 Jun 2019 19:46:20 +1000 Subject: Comment on the key of `suggestedFixes` This isn't immediately obvious without looking at the users of the map --- editors/code/src/utils/diagnostics/SuggestedFixCollection.ts | 3 +++ 1 file changed, 3 insertions(+) (limited to 'editors/code/src/utils') diff --git a/editors/code/src/utils/diagnostics/SuggestedFixCollection.ts b/editors/code/src/utils/diagnostics/SuggestedFixCollection.ts index 3b0bf7468..132ce12f8 100644 --- a/editors/code/src/utils/diagnostics/SuggestedFixCollection.ts +++ b/editors/code/src/utils/diagnostics/SuggestedFixCollection.ts @@ -11,6 +11,9 @@ export default class SuggestedFixCollection implements vscode.CodeActionProvider { public static PROVIDED_CODE_ACTION_KINDS = [vscode.CodeActionKind.QuickFix]; + /** + * Map of document URI strings to suggested fixes + */ private suggestedFixes: Map; constructor() { -- cgit v1.2.3