From 637c795b3c9a56795977ade0cedbc7a9fb7dc453 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Thu, 12 Mar 2020 15:01:53 +0100 Subject: Correctly handle multi-line fixes from cargo/clippy --- crates/ra_cargo_watch/src/conv.rs | 47 +++----- ...watch__conv__test__snap_clippy_pass_by_ref.snap | 2 +- ...rgo_watch__conv__test__snap_multi_line_fix.snap | 112 +++++++++++++++++ ...ch__conv__test__snap_rustc_unused_variable.snap | 2 +- crates/ra_cargo_watch/src/conv/test.rs | 134 +++++++++++++++++++++ 5 files changed, 267 insertions(+), 30 deletions(-) create mode 100644 crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_multi_line_fix.snap diff --git a/crates/ra_cargo_watch/src/conv.rs b/crates/ra_cargo_watch/src/conv.rs index 0246adfb5..a3f05bede 100644 --- a/crates/ra_cargo_watch/src/conv.rs +++ b/crates/ra_cargo_watch/src/conv.rs @@ -8,6 +8,7 @@ use lsp_types::{ Location, NumberOrString, Position, Range, TextEdit, Url, WorkspaceEdit, }; use std::{ + collections::HashMap, fmt::Write, path::{Component, Path, PathBuf, Prefix}, str::FromStr, @@ -126,44 +127,34 @@ fn map_rust_child_diagnostic( rd: &RustDiagnostic, workspace_root: &PathBuf, ) -> MappedRustChildDiagnostic { - let span: &DiagnosticSpan = match rd.spans.iter().find(|s| s.is_primary) { - Some(span) => span, - None => { - // `rustc` uses these spanless children as a way to print multi-line - // messages - return MappedRustChildDiagnostic::MessageLine(rd.message.clone()); - } - }; - - // If we have a primary span use its location, otherwise use the parent - let location = map_span_to_location(&span, workspace_root); - - if let Some(suggested_replacement) = &span.suggested_replacement { - // Include our replacement in the title unless it's empty - let title = if !suggested_replacement.is_empty() { - format!("{}: '{}'", rd.message, suggested_replacement) - } else { - rd.message.clone() - }; + let spans: Vec<&DiagnosticSpan> = rd.spans.iter().filter(|s| s.is_primary).collect(); + if spans.is_empty() { + // `rustc` uses these spanless children as a way to print multi-line + // messages + return MappedRustChildDiagnostic::MessageLine(rd.message.clone()); + } - let edit = { - let edits = vec![TextEdit::new(location.range, suggested_replacement.clone())]; - let mut edit_map = std::collections::HashMap::new(); - edit_map.insert(location.uri, edits); - WorkspaceEdit::new(edit_map) - }; + let mut edit_map: HashMap> = HashMap::new(); + for &span in &spans { + if let Some(suggested_replacement) = &span.suggested_replacement { + let location = map_span_to_location(span, workspace_root); + let edit = TextEdit::new(location.range, suggested_replacement.clone()); + edit_map.entry(location.uri).or_default().push(edit); + } + } + if !edit_map.is_empty() { MappedRustChildDiagnostic::SuggestedFix(CodeAction { - title, + title: rd.message.clone(), kind: Some("quickfix".to_string()), diagnostics: None, - edit: Some(edit), + edit: Some(WorkspaceEdit::new(edit_map)), command: None, is_preferred: None, }) } else { MappedRustChildDiagnostic::Related(DiagnosticRelatedInformation { - location, + location: map_span_to_location(spans[0], workspace_root), message: rd.message.clone(), }) } diff --git a/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap index 95ca163dc..47801ae79 100644 --- a/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap +++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap @@ -63,7 +63,7 @@ MappedRustDiagnostic { }, fixes: [ CodeAction { - title: "consider passing by value instead: \'self\'", + title: "consider passing by value instead", kind: Some( "quickfix", ), diff --git a/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_multi_line_fix.snap b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_multi_line_fix.snap new file mode 100644 index 000000000..23c4f5a2c --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_multi_line_fix.snap @@ -0,0 +1,112 @@ +--- +source: crates/ra_cargo_watch/src/conv/test.rs +expression: diag +--- +MappedRustDiagnostic { + location: Location { + uri: "file:///test/src/main.rs", + range: Range { + start: Position { + line: 3, + character: 4, + }, + end: Position { + line: 3, + character: 5, + }, + }, + }, + diagnostic: Diagnostic { + range: Range { + start: Position { + line: 3, + character: 4, + }, + end: Position { + line: 3, + character: 5, + }, + }, + severity: Some( + Warning, + ), + code: Some( + String( + "let_and_return", + ), + ), + source: Some( + "clippy", + ), + message: "returning the result of a let binding from a block\n`#[warn(clippy::let_and_return)]` on by default\nfor further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return", + related_information: Some( + [ + DiagnosticRelatedInformation { + location: Location { + uri: "file:///test/src/main.rs", + range: Range { + start: Position { + line: 2, + character: 4, + }, + end: Position { + line: 2, + character: 30, + }, + }, + }, + message: "unnecessary let binding", + }, + ], + ), + tags: None, + }, + fixes: [ + CodeAction { + title: "return the expression directly", + kind: Some( + "quickfix", + ), + diagnostics: None, + edit: Some( + WorkspaceEdit { + changes: Some( + { + "file:///test/src/main.rs": [ + TextEdit { + range: Range { + start: Position { + line: 2, + character: 4, + }, + end: Position { + line: 2, + character: 30, + }, + }, + new_text: "", + }, + TextEdit { + range: Range { + start: Position { + line: 3, + character: 4, + }, + end: Position { + line: 3, + character: 5, + }, + }, + new_text: "(0..10).collect()", + }, + ], + }, + ), + document_changes: None, + }, + ), + command: None, + is_preferred: None, + }, + ], +} diff --git a/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_unused_variable.snap b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_unused_variable.snap index 3e1fe736c..8bab09540 100644 --- a/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_unused_variable.snap +++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_unused_variable.snap @@ -48,7 +48,7 @@ MappedRustDiagnostic { }, fixes: [ CodeAction { - title: "consider prefixing with an underscore: \'_foo\'", + title: "consider prefixing with an underscore", kind: Some( "quickfix", ), diff --git a/crates/ra_cargo_watch/src/conv/test.rs b/crates/ra_cargo_watch/src/conv/test.rs index 6b86525b8..c880dcdc3 100644 --- a/crates/ra_cargo_watch/src/conv/test.rs +++ b/crates/ra_cargo_watch/src/conv/test.rs @@ -936,3 +936,137 @@ fn snap_macro_compiler_error() { let diag = map_rust_diagnostic_to_lsp(&diag, &workspace_root).expect("couldn't map diagnostic"); insta::assert_debug_snapshot!(diag); } + +#[test] +#[cfg(not(windows))] +fn snap_multi_line_fix() { + let diag = parse_diagnostic( + r##"{ + "rendered": "warning: returning the result of a let binding from a block\n --> src/main.rs:4:5\n |\n3 | let a = (0..10).collect();\n | -------------------------- unnecessary let binding\n4 | a\n | ^\n |\n = note: `#[warn(clippy::let_and_return)]` on by default\n = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return\nhelp: return the expression directly\n |\n3 | \n4 | (0..10).collect()\n |\n\n", + "children": [ + { + "children": [], + "code": null, + "level": "note", + "message": "`#[warn(clippy::let_and_return)]` on by default", + "rendered": null, + "spans": [] + }, + { + "children": [], + "code": null, + "level": "help", + "message": "for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return", + "rendered": null, + "spans": [] + }, + { + "children": [], + "code": null, + "level": "help", + "message": "return the expression directly", + "rendered": null, + "spans": [ + { + "byte_end": 55, + "byte_start": 29, + "column_end": 31, + "column_start": 5, + "expansion": null, + "file_name": "src/main.rs", + "is_primary": true, + "label": null, + "line_end": 3, + "line_start": 3, + "suggested_replacement": "", + "suggestion_applicability": "MachineApplicable", + "text": [ + { + "highlight_end": 31, + "highlight_start": 5, + "text": " let a = (0..10).collect();" + } + ] + }, + { + "byte_end": 61, + "byte_start": 60, + "column_end": 6, + "column_start": 5, + "expansion": null, + "file_name": "src/main.rs", + "is_primary": true, + "label": null, + "line_end": 4, + "line_start": 4, + "suggested_replacement": "(0..10).collect()", + "suggestion_applicability": "MachineApplicable", + "text": [ + { + "highlight_end": 6, + "highlight_start": 5, + "text": " a" + } + ] + } + ] + } + ], + "code": { + "code": "clippy::let_and_return", + "explanation": null + }, + "level": "warning", + "message": "returning the result of a let binding from a block", + "spans": [ + { + "byte_end": 55, + "byte_start": 29, + "column_end": 31, + "column_start": 5, + "expansion": null, + "file_name": "src/main.rs", + "is_primary": false, + "label": "unnecessary let binding", + "line_end": 3, + "line_start": 3, + "suggested_replacement": null, + "suggestion_applicability": null, + "text": [ + { + "highlight_end": 31, + "highlight_start": 5, + "text": " let a = (0..10).collect();" + } + ] + }, + { + "byte_end": 61, + "byte_start": 60, + "column_end": 6, + "column_start": 5, + "expansion": null, + "file_name": "src/main.rs", + "is_primary": true, + "label": null, + "line_end": 4, + "line_start": 4, + "suggested_replacement": null, + "suggestion_applicability": null, + "text": [ + { + "highlight_end": 6, + "highlight_start": 5, + "text": " a" + } + ] + } + ] + } + "##, + ); + + let workspace_root = PathBuf::from("/test/"); + let diag = map_rust_diagnostic_to_lsp(&diag, &workspace_root).expect("couldn't map diagnostic"); + insta::assert_debug_snapshot!(diag); +} -- cgit v1.2.3