From 428a6ff5b8bad2c80a3522599195bf2a393f744e Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 11:10:07 +0100 Subject: Move cargo watch functionality to separate crate --- crates/ra_cargo_watch/Cargo.toml | 17 + crates/ra_cargo_watch/src/conv.rs | 280 +++++++++ .../snapshots/test__snap_clippy_pass_by_ref.snap | 85 +++ .../test__snap_handles_macro_location.snap | 46 ++ ...st__snap_rustc_incompatible_type_for_trait.snap | 46 ++ .../test__snap_rustc_mismatched_type.snap | 46 ++ .../test__snap_rustc_unused_variable.snap | 70 +++ ...est__snap_rustc_wrong_number_of_parameters.snap | 65 ++ crates/ra_cargo_watch/src/conv/test.rs | 698 +++++++++++++++++++++ crates/ra_cargo_watch/src/lib.rs | 345 ++++++++++ 10 files changed, 1698 insertions(+) create mode 100644 crates/ra_cargo_watch/Cargo.toml create mode 100644 crates/ra_cargo_watch/src/conv.rs create mode 100644 crates/ra_cargo_watch/src/conv/snapshots/test__snap_clippy_pass_by_ref.snap create mode 100644 crates/ra_cargo_watch/src/conv/snapshots/test__snap_handles_macro_location.snap create mode 100644 crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_incompatible_type_for_trait.snap create mode 100644 crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_mismatched_type.snap create mode 100644 crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_unused_variable.snap create mode 100644 crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_wrong_number_of_parameters.snap create mode 100644 crates/ra_cargo_watch/src/conv/test.rs create mode 100644 crates/ra_cargo_watch/src/lib.rs (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/Cargo.toml b/crates/ra_cargo_watch/Cargo.toml new file mode 100644 index 000000000..bcc4648ff --- /dev/null +++ b/crates/ra_cargo_watch/Cargo.toml @@ -0,0 +1,17 @@ +[package] +edition = "2018" +name = "ra_cargo_watch" +version = "0.1.0" +authors = ["rust-analyzer developers"] + +[dependencies] +crossbeam-channel = "0.4" +lsp-types = { version = "0.67.0", features = ["proposed"] } +log = "0.4.3" +cargo_metadata = "0.9.1" +jod-thread = "0.1.0" +parking_lot = "0.10.0" + +[dev-dependencies] +insta = "0.12.0" +serde_json = "1.0" \ No newline at end of file diff --git a/crates/ra_cargo_watch/src/conv.rs b/crates/ra_cargo_watch/src/conv.rs new file mode 100644 index 000000000..3bd4bf7a5 --- /dev/null +++ b/crates/ra_cargo_watch/src/conv.rs @@ -0,0 +1,280 @@ +//! This module provides the functionality needed to convert diagnostics from +//! `cargo check` json format to the LSP diagnostic format. +use cargo_metadata::diagnostic::{ + Applicability, Diagnostic as RustDiagnostic, DiagnosticLevel, DiagnosticSpan, + DiagnosticSpanMacroExpansion, +}; +use lsp_types::{ + Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, DiagnosticTag, Location, + NumberOrString, Position, Range, Url, +}; +use std::{fmt::Write, path::PathBuf}; + +#[cfg(test)] +mod test; + +/// Converts a Rust level string to a LSP severity +fn map_level_to_severity(val: DiagnosticLevel) -> Option { + match val { + DiagnosticLevel::Ice => Some(DiagnosticSeverity::Error), + DiagnosticLevel::Error => Some(DiagnosticSeverity::Error), + DiagnosticLevel::Warning => Some(DiagnosticSeverity::Warning), + DiagnosticLevel::Note => Some(DiagnosticSeverity::Information), + DiagnosticLevel::Help => Some(DiagnosticSeverity::Hint), + DiagnosticLevel::Unknown => None, + } +} + +/// Check whether a file name is from macro invocation +fn is_from_macro(file_name: &str) -> bool { + file_name.starts_with('<') && file_name.ends_with('>') +} + +/// Converts a Rust macro span to a LSP location recursively +fn map_macro_span_to_location( + span_macro: &DiagnosticSpanMacroExpansion, + workspace_root: &PathBuf, +) -> Option { + if !is_from_macro(&span_macro.span.file_name) { + return Some(map_span_to_location(&span_macro.span, workspace_root)); + } + + if let Some(expansion) = &span_macro.span.expansion { + return map_macro_span_to_location(&expansion, workspace_root); + } + + None +} + +/// Converts a Rust span to a LSP location +fn map_span_to_location(span: &DiagnosticSpan, workspace_root: &PathBuf) -> Location { + if is_from_macro(&span.file_name) && span.expansion.is_some() { + let expansion = span.expansion.as_ref().unwrap(); + if let Some(macro_range) = map_macro_span_to_location(&expansion, workspace_root) { + return macro_range; + } + } + + let mut file_name = workspace_root.clone(); + file_name.push(&span.file_name); + let uri = Url::from_file_path(file_name).unwrap(); + + let range = Range::new( + Position::new(span.line_start as u64 - 1, span.column_start as u64 - 1), + Position::new(span.line_end as u64 - 1, span.column_end as u64 - 1), + ); + + Location { uri, range } +} + +/// Converts a secondary Rust span to a LSP related information +/// +/// If the span is unlabelled this will return `None`. +fn map_secondary_span_to_related( + span: &DiagnosticSpan, + workspace_root: &PathBuf, +) -> Option { + if let Some(label) = &span.label { + let location = map_span_to_location(span, workspace_root); + Some(DiagnosticRelatedInformation { location, message: label.clone() }) + } else { + // Nothing to label this with + None + } +} + +/// Determines if diagnostic is related to unused code +fn is_unused_or_unnecessary(rd: &RustDiagnostic) -> bool { + if let Some(code) = &rd.code { + match code.code.as_str() { + "dead_code" | "unknown_lints" | "unreachable_code" | "unused_attributes" + | "unused_imports" | "unused_macros" | "unused_variables" => true, + _ => false, + } + } else { + false + } +} + +/// Determines if diagnostic is related to deprecated code +fn is_deprecated(rd: &RustDiagnostic) -> bool { + if let Some(code) = &rd.code { + match code.code.as_str() { + "deprecated" => true, + _ => false, + } + } else { + false + } +} + +#[derive(Debug)] +pub struct SuggestedFix { + pub title: String, + pub location: Location, + pub replacement: String, + pub applicability: Applicability, + pub diagnostics: Vec, +} + +impl std::cmp::PartialEq for SuggestedFix { + fn eq(&self, other: &SuggestedFix) -> bool { + if self.title == other.title + && self.location == other.location + && self.replacement == other.replacement + { + // Applicability doesn't impl PartialEq... + match (&self.applicability, &other.applicability) { + (Applicability::MachineApplicable, Applicability::MachineApplicable) => true, + (Applicability::HasPlaceholders, Applicability::HasPlaceholders) => true, + (Applicability::MaybeIncorrect, Applicability::MaybeIncorrect) => true, + (Applicability::Unspecified, Applicability::Unspecified) => true, + _ => false, + } + } else { + false + } + } +} + +enum MappedRustChildDiagnostic { + Related(DiagnosticRelatedInformation), + SuggestedFix(SuggestedFix), + MessageLine(String), +} + +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() + }; + + MappedRustChildDiagnostic::SuggestedFix(SuggestedFix { + title, + location, + replacement: suggested_replacement.clone(), + applicability: span.suggestion_applicability.clone().unwrap_or(Applicability::Unknown), + diagnostics: vec![], + }) + } else { + MappedRustChildDiagnostic::Related(DiagnosticRelatedInformation { + location, + message: rd.message.clone(), + }) + } +} + +#[derive(Debug)] +pub(crate) struct MappedRustDiagnostic { + pub location: Location, + pub diagnostic: Diagnostic, + pub suggested_fixes: Vec, +} + +/// Converts a Rust root diagnostic to LSP form +/// +/// This flattens the Rust diagnostic by: +/// +/// 1. Creating a LSP 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 `None` +pub(crate) fn map_rust_diagnostic_to_lsp( + rd: &RustDiagnostic, + workspace_root: &PathBuf, +) -> Option { + let primary_span = rd.spans.iter().find(|s| s.is_primary)?; + + let location = map_span_to_location(&primary_span, workspace_root); + + let severity = map_level_to_severity(rd.level); + let mut primary_span_label = primary_span.label.as_ref(); + + let mut source = String::from("rustc"); + let mut code = rd.code.as_ref().map(|c| c.code.clone()); + if let Some(code_val) = &code { + // See if this is an RFC #2103 scoped lint (e.g. from Clippy) + let scoped_code: Vec<&str> = code_val.split("::").collect(); + if scoped_code.len() == 2 { + source = String::from(scoped_code[0]); + code = Some(String::from(scoped_code[1])); + } + } + + let mut related_information = vec![]; + let mut tags = vec![]; + + for secondary_span in rd.spans.iter().filter(|s| !s.is_primary) { + let related = map_secondary_span_to_related(secondary_span, workspace_root); + if let Some(related) = related { + related_information.push(related); + } + } + + let mut suggested_fixes = vec![]; + let mut message = rd.message.clone(); + for child in &rd.children { + let child = map_rust_child_diagnostic(&child, workspace_root); + match child { + MappedRustChildDiagnostic::Related(related) => related_information.push(related), + MappedRustChildDiagnostic::SuggestedFix(suggested_fix) => { + suggested_fixes.push(suggested_fix) + } + MappedRustChildDiagnostic::MessageLine(message_line) => { + write!(&mut message, "\n{}", message_line).unwrap(); + + // These secondary messages usually duplicate the content of the + // primary span label. + primary_span_label = None; + } + } + } + + if let Some(primary_span_label) = primary_span_label { + write!(&mut message, "\n{}", primary_span_label).unwrap(); + } + + if is_unused_or_unnecessary(rd) { + tags.push(DiagnosticTag::Unnecessary); + } + + if is_deprecated(rd) { + tags.push(DiagnosticTag::Deprecated); + } + + let diagnostic = Diagnostic { + range: location.range, + severity, + code: code.map(NumberOrString::String), + source: Some(source), + message, + related_information: if !related_information.is_empty() { + Some(related_information) + } else { + None + }, + tags: if !tags.is_empty() { Some(tags) } else { None }, + }; + + Some(MappedRustDiagnostic { location, diagnostic, suggested_fixes }) +} diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_clippy_pass_by_ref.snap b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_clippy_pass_by_ref.snap new file mode 100644 index 000000000..cb0920914 --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_clippy_pass_by_ref.snap @@ -0,0 +1,85 @@ +--- +source: crates/ra_cargo_watch/src/conv/test.rs +expression: diag +--- +MappedRustDiagnostic { + location: Location { + uri: "file:///test/compiler/mir/tagset.rs", + range: Range { + start: Position { + line: 41, + character: 23, + }, + end: Position { + line: 41, + character: 28, + }, + }, + }, + diagnostic: Diagnostic { + range: Range { + start: Position { + line: 41, + character: 23, + }, + end: Position { + line: 41, + character: 28, + }, + }, + severity: Some( + Warning, + ), + code: Some( + String( + "trivially_copy_pass_by_ref", + ), + ), + source: Some( + "clippy", + ), + message: "this argument is passed by reference, but would be more efficient if passed by value\n#[warn(clippy::trivially_copy_pass_by_ref)] implied by #[warn(clippy::all)]\nfor further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref", + related_information: Some( + [ + DiagnosticRelatedInformation { + location: Location { + uri: "file:///test/compiler/lib.rs", + range: Range { + start: Position { + line: 0, + character: 8, + }, + end: Position { + line: 0, + character: 19, + }, + }, + }, + message: "lint level defined here", + }, + ], + ), + tags: None, + }, + suggested_fixes: [ + SuggestedFix { + title: "consider passing by value instead: \'self\'", + location: Location { + uri: "file:///test/compiler/mir/tagset.rs", + range: Range { + start: Position { + line: 41, + character: 23, + }, + end: Position { + line: 41, + character: 28, + }, + }, + }, + replacement: "self", + applicability: Unspecified, + diagnostics: [], + }, + ], +} diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_handles_macro_location.snap b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_handles_macro_location.snap new file mode 100644 index 000000000..19510ecc1 --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_handles_macro_location.snap @@ -0,0 +1,46 @@ +--- +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: 1, + character: 4, + }, + end: Position { + line: 1, + character: 26, + }, + }, + }, + diagnostic: Diagnostic { + range: Range { + start: Position { + line: 1, + character: 4, + }, + end: Position { + line: 1, + character: 26, + }, + }, + severity: Some( + Error, + ), + code: Some( + String( + "E0277", + ), + ), + source: Some( + "rustc", + ), + message: "can\'t compare `{integer}` with `&str`\nthe trait `std::cmp::PartialEq<&str>` is not implemented for `{integer}`", + related_information: None, + tags: None, + }, + suggested_fixes: [], +} diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_incompatible_type_for_trait.snap b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_incompatible_type_for_trait.snap new file mode 100644 index 000000000..cf683e4b6 --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_incompatible_type_for_trait.snap @@ -0,0 +1,46 @@ +--- +source: crates/ra_cargo_watch/src/conv/test.rs +expression: diag +--- +MappedRustDiagnostic { + location: Location { + uri: "file:///test/compiler/ty/list_iter.rs", + range: Range { + start: Position { + line: 51, + character: 4, + }, + end: Position { + line: 51, + character: 47, + }, + }, + }, + diagnostic: Diagnostic { + range: Range { + start: Position { + line: 51, + character: 4, + }, + end: Position { + line: 51, + character: 47, + }, + }, + severity: Some( + Error, + ), + code: Some( + String( + "E0053", + ), + ), + source: Some( + "rustc", + ), + message: "method `next` has an incompatible type for trait\nexpected type `fn(&mut ty::list_iter::ListIterator<\'list, M>) -> std::option::Option<&ty::Ref>`\n found type `fn(&ty::list_iter::ListIterator<\'list, M>) -> std::option::Option<&\'list ty::Ref>`", + related_information: None, + tags: None, + }, + suggested_fixes: [], +} diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_mismatched_type.snap b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_mismatched_type.snap new file mode 100644 index 000000000..8c1483c74 --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_mismatched_type.snap @@ -0,0 +1,46 @@ +--- +source: crates/ra_cargo_watch/src/conv/test.rs +expression: diag +--- +MappedRustDiagnostic { + location: Location { + uri: "file:///test/runtime/compiler_support.rs", + range: Range { + start: Position { + line: 47, + character: 64, + }, + end: Position { + line: 47, + character: 69, + }, + }, + }, + diagnostic: Diagnostic { + range: Range { + start: Position { + line: 47, + character: 64, + }, + end: Position { + line: 47, + character: 69, + }, + }, + severity: Some( + Error, + ), + code: Some( + String( + "E0308", + ), + ), + source: Some( + "rustc", + ), + message: "mismatched types\nexpected usize, found u32", + related_information: None, + tags: None, + }, + suggested_fixes: [], +} diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_unused_variable.snap b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_unused_variable.snap new file mode 100644 index 000000000..eb5a2247b --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_unused_variable.snap @@ -0,0 +1,70 @@ +--- +source: crates/ra_cargo_watch/src/conv/test.rs +expression: diag +--- +MappedRustDiagnostic { + location: Location { + uri: "file:///test/driver/subcommand/repl.rs", + range: Range { + start: Position { + line: 290, + character: 8, + }, + end: Position { + line: 290, + character: 11, + }, + }, + }, + diagnostic: Diagnostic { + range: Range { + start: Position { + line: 290, + character: 8, + }, + end: Position { + line: 290, + character: 11, + }, + }, + severity: Some( + Warning, + ), + code: Some( + String( + "unused_variables", + ), + ), + source: Some( + "rustc", + ), + message: "unused variable: `foo`\n#[warn(unused_variables)] on by default", + related_information: None, + tags: Some( + [ + Unnecessary, + ], + ), + }, + suggested_fixes: [ + SuggestedFix { + title: "consider prefixing with an underscore: \'_foo\'", + location: Location { + uri: "file:///test/driver/subcommand/repl.rs", + range: Range { + start: Position { + line: 290, + character: 8, + }, + end: Position { + line: 290, + character: 11, + }, + }, + }, + replacement: "_foo", + applicability: MachineApplicable, + diagnostics: [], + }, + ], +} diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_wrong_number_of_parameters.snap b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_wrong_number_of_parameters.snap new file mode 100644 index 000000000..2f4518931 --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_wrong_number_of_parameters.snap @@ -0,0 +1,65 @@ +--- +source: crates/ra_cargo_watch/src/conv/test.rs +expression: diag +--- +MappedRustDiagnostic { + location: Location { + uri: "file:///test/compiler/ty/select.rs", + range: Range { + start: Position { + line: 103, + character: 17, + }, + end: Position { + line: 103, + character: 29, + }, + }, + }, + diagnostic: Diagnostic { + range: Range { + start: Position { + line: 103, + character: 17, + }, + end: Position { + line: 103, + character: 29, + }, + }, + severity: Some( + Error, + ), + code: Some( + String( + "E0061", + ), + ), + source: Some( + "rustc", + ), + message: "this function takes 2 parameters but 3 parameters were supplied\nexpected 2 parameters", + related_information: Some( + [ + DiagnosticRelatedInformation { + location: Location { + uri: "file:///test/compiler/ty/select.rs", + range: Range { + start: Position { + line: 218, + character: 4, + }, + end: Position { + line: 230, + character: 5, + }, + }, + }, + message: "defined here", + }, + ], + ), + tags: None, + }, + suggested_fixes: [], +} diff --git a/crates/ra_cargo_watch/src/conv/test.rs b/crates/ra_cargo_watch/src/conv/test.rs new file mode 100644 index 000000000..69a07be11 --- /dev/null +++ b/crates/ra_cargo_watch/src/conv/test.rs @@ -0,0 +1,698 @@ +use crate::*; + +fn parse_diagnostic(val: &str) -> cargo_metadata::diagnostic::Diagnostic { + serde_json::from_str::(val).unwrap() +} + +#[test] +fn snap_rustc_incompatible_type_for_trait() { + let diag = parse_diagnostic( + r##"{ + "message": "method `next` has an incompatible type for trait", + "code": { + "code": "E0053", + "explanation": "\nThe parameters of any trait method must match between a trait implementation\nand the trait definition.\n\nHere are a couple examples of this error:\n\n```compile_fail,E0053\ntrait Foo {\n fn foo(x: u16);\n fn bar(&self);\n}\n\nstruct Bar;\n\nimpl Foo for Bar {\n // error, expected u16, found i16\n fn foo(x: i16) { }\n\n // error, types differ in mutability\n fn bar(&mut self) { }\n}\n```\n" + }, + "level": "error", + "spans": [ + { + "file_name": "compiler/ty/list_iter.rs", + "byte_start": 1307, + "byte_end": 1350, + "line_start": 52, + "line_end": 52, + "column_start": 5, + "column_end": 48, + "is_primary": true, + "text": [ + { + "text": " fn next(&self) -> Option<&'list ty::Ref> {", + "highlight_start": 5, + "highlight_end": 48 + } + ], + "label": "types differ in mutability", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [ + { + "message": "expected type `fn(&mut ty::list_iter::ListIterator<'list, M>) -> std::option::Option<&ty::Ref>`\n found type `fn(&ty::list_iter::ListIterator<'list, M>) -> std::option::Option<&'list ty::Ref>`", + "code": null, + "level": "note", + "spans": [], + "children": [], + "rendered": null + } + ], + "rendered": "error[E0053]: method `next` has an incompatible type for trait\n --> compiler/ty/list_iter.rs:52:5\n |\n52 | fn next(&self) -> Option<&'list ty::Ref> {\n | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ types differ in mutability\n |\n = note: expected type `fn(&mut ty::list_iter::ListIterator<'list, M>) -> std::option::Option<&ty::Ref>`\n found type `fn(&ty::list_iter::ListIterator<'list, M>) -> std::option::Option<&'list ty::Ref>`\n\n" + } + "##, + ); + + 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); +} + +#[test] +fn snap_rustc_unused_variable() { + let diag = parse_diagnostic( + r##"{ +"message": "unused variable: `foo`", +"code": { + "code": "unused_variables", + "explanation": null +}, +"level": "warning", +"spans": [ + { + "file_name": "driver/subcommand/repl.rs", + "byte_start": 9228, + "byte_end": 9231, + "line_start": 291, + "line_end": 291, + "column_start": 9, + "column_end": 12, + "is_primary": true, + "text": [ + { + "text": " let foo = 42;", + "highlight_start": 9, + "highlight_end": 12 + } + ], + "label": null, + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } +], +"children": [ + { + "message": "#[warn(unused_variables)] on by default", + "code": null, + "level": "note", + "spans": [], + "children": [], + "rendered": null + }, + { + "message": "consider prefixing with an underscore", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "driver/subcommand/repl.rs", + "byte_start": 9228, + "byte_end": 9231, + "line_start": 291, + "line_end": 291, + "column_start": 9, + "column_end": 12, + "is_primary": true, + "text": [ + { + "text": " let foo = 42;", + "highlight_start": 9, + "highlight_end": 12 + } + ], + "label": null, + "suggested_replacement": "_foo", + "suggestion_applicability": "MachineApplicable", + "expansion": null + } + ], + "children": [], + "rendered": null + } +], +"rendered": "warning: unused variable: `foo`\n --> driver/subcommand/repl.rs:291:9\n |\n291 | let foo = 42;\n | ^^^ help: consider prefixing with an underscore: `_foo`\n |\n = note: #[warn(unused_variables)] on by default\n\n" +}"##, + ); + + 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); +} + +#[test] +fn snap_rustc_wrong_number_of_parameters() { + let diag = parse_diagnostic( + r##"{ +"message": "this function takes 2 parameters but 3 parameters were supplied", +"code": { + "code": "E0061", + "explanation": "\nThe number of arguments passed to a function must match the number of arguments\nspecified in the function signature.\n\nFor example, a function like:\n\n```\nfn f(a: u16, b: &str) {}\n```\n\nMust always be called with exactly two arguments, e.g., `f(2, \"test\")`.\n\nNote that Rust does not have a notion of optional function arguments or\nvariadic functions (except for its C-FFI).\n" +}, +"level": "error", +"spans": [ + { + "file_name": "compiler/ty/select.rs", + "byte_start": 8787, + "byte_end": 9241, + "line_start": 219, + "line_end": 231, + "column_start": 5, + "column_end": 6, + "is_primary": false, + "text": [ + { + "text": " pub fn add_evidence(", + "highlight_start": 5, + "highlight_end": 25 + }, + { + "text": " &mut self,", + "highlight_start": 1, + "highlight_end": 19 + }, + { + "text": " target_poly: &ty::Ref,", + "highlight_start": 1, + "highlight_end": 41 + }, + { + "text": " evidence_poly: &ty::Ref,", + "highlight_start": 1, + "highlight_end": 43 + }, + { + "text": " ) {", + "highlight_start": 1, + "highlight_end": 8 + }, + { + "text": " match target_poly {", + "highlight_start": 1, + "highlight_end": 28 + }, + { + "text": " ty::Ref::Var(tvar, _) => self.add_var_evidence(tvar, evidence_poly),", + "highlight_start": 1, + "highlight_end": 81 + }, + { + "text": " ty::Ref::Fixed(target_ty) => {", + "highlight_start": 1, + "highlight_end": 43 + }, + { + "text": " let evidence_ty = evidence_poly.resolve_to_ty();", + "highlight_start": 1, + "highlight_end": 65 + }, + { + "text": " self.add_evidence_ty(target_ty, evidence_poly, evidence_ty)", + "highlight_start": 1, + "highlight_end": 76 + }, + { + "text": " }", + "highlight_start": 1, + "highlight_end": 14 + }, + { + "text": " }", + "highlight_start": 1, + "highlight_end": 10 + }, + { + "text": " }", + "highlight_start": 1, + "highlight_end": 6 + } + ], + "label": "defined here", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + }, + { + "file_name": "compiler/ty/select.rs", + "byte_start": 4045, + "byte_end": 4057, + "line_start": 104, + "line_end": 104, + "column_start": 18, + "column_end": 30, + "is_primary": true, + "text": [ + { + "text": " self.add_evidence(target_fixed, evidence_fixed, false);", + "highlight_start": 18, + "highlight_end": 30 + } + ], + "label": "expected 2 parameters", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } +], +"children": [], +"rendered": "error[E0061]: this function takes 2 parameters but 3 parameters were supplied\n --> compiler/ty/select.rs:104:18\n |\n104 | self.add_evidence(target_fixed, evidence_fixed, false);\n | ^^^^^^^^^^^^ expected 2 parameters\n...\n219 | / pub fn add_evidence(\n220 | | &mut self,\n221 | | target_poly: &ty::Ref,\n222 | | evidence_poly: &ty::Ref,\n... |\n230 | | }\n231 | | }\n | |_____- defined here\n\n" +}"##, + ); + + 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); +} + +#[test] +fn snap_clippy_pass_by_ref() { + let diag = parse_diagnostic( + r##"{ +"message": "this argument is passed by reference, but would be more efficient if passed by value", +"code": { + "code": "clippy::trivially_copy_pass_by_ref", + "explanation": null +}, +"level": "warning", +"spans": [ + { + "file_name": "compiler/mir/tagset.rs", + "byte_start": 941, + "byte_end": 946, + "line_start": 42, + "line_end": 42, + "column_start": 24, + "column_end": 29, + "is_primary": true, + "text": [ + { + "text": " pub fn is_disjoint(&self, other: Self) -> bool {", + "highlight_start": 24, + "highlight_end": 29 + } + ], + "label": null, + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } +], +"children": [ + { + "message": "lint level defined here", + "code": null, + "level": "note", + "spans": [ + { + "file_name": "compiler/lib.rs", + "byte_start": 8, + "byte_end": 19, + "line_start": 1, + "line_end": 1, + "column_start": 9, + "column_end": 20, + "is_primary": true, + "text": [ + { + "text": "#![warn(clippy::all)]", + "highlight_start": 9, + "highlight_end": 20 + } + ], + "label": null, + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } + ], + "children": [], + "rendered": null + }, + { + "message": "#[warn(clippy::trivially_copy_pass_by_ref)] implied by #[warn(clippy::all)]", + "code": null, + "level": "note", + "spans": [], + "children": [], + "rendered": null + }, + { + "message": "for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref", + "code": null, + "level": "help", + "spans": [], + "children": [], + "rendered": null + }, + { + "message": "consider passing by value instead", + "code": null, + "level": "help", + "spans": [ + { + "file_name": "compiler/mir/tagset.rs", + "byte_start": 941, + "byte_end": 946, + "line_start": 42, + "line_end": 42, + "column_start": 24, + "column_end": 29, + "is_primary": true, + "text": [ + { + "text": " pub fn is_disjoint(&self, other: Self) -> bool {", + "highlight_start": 24, + "highlight_end": 29 + } + ], + "label": null, + "suggested_replacement": "self", + "suggestion_applicability": "Unspecified", + "expansion": null + } + ], + "children": [], + "rendered": null + } +], +"rendered": "warning: this argument is passed by reference, but would be more efficient if passed by value\n --> compiler/mir/tagset.rs:42:24\n |\n42 | pub fn is_disjoint(&self, other: Self) -> bool {\n | ^^^^^ help: consider passing by value instead: `self`\n |\nnote: lint level defined here\n --> compiler/lib.rs:1:9\n |\n1 | #![warn(clippy::all)]\n | ^^^^^^^^^^^\n = note: #[warn(clippy::trivially_copy_pass_by_ref)] implied by #[warn(clippy::all)]\n = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#trivially_copy_pass_by_ref\n\n" +}"##, + ); + + 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); +} + +#[test] +fn snap_rustc_mismatched_type() { + let diag = parse_diagnostic( + r##"{ +"message": "mismatched types", +"code": { + "code": "E0308", + "explanation": "\nThis error occurs when the compiler was unable to infer the concrete type of a\nvariable. It can occur for several cases, the most common of which is a\nmismatch in the expected type that the compiler inferred for a variable's\ninitializing expression, and the actual type explicitly assigned to the\nvariable.\n\nFor example:\n\n```compile_fail,E0308\nlet x: i32 = \"I am not a number!\";\n// ~~~ ~~~~~~~~~~~~~~~~~~~~\n// | |\n// | initializing expression;\n// | compiler infers type `&str`\n// |\n// type `i32` assigned to variable `x`\n```\n" +}, +"level": "error", +"spans": [ + { + "file_name": "runtime/compiler_support.rs", + "byte_start": 1589, + "byte_end": 1594, + "line_start": 48, + "line_end": 48, + "column_start": 65, + "column_end": 70, + "is_primary": true, + "text": [ + { + "text": " let layout = alloc::Layout::from_size_align_unchecked(size, align);", + "highlight_start": 65, + "highlight_end": 70 + } + ], + "label": "expected usize, found u32", + "suggested_replacement": null, + "suggestion_applicability": null, + "expansion": null + } +], +"children": [], +"rendered": "error[E0308]: mismatched types\n --> runtime/compiler_support.rs:48:65\n |\n48 | let layout = alloc::Layout::from_size_align_unchecked(size, align);\n | ^^^^^ expected usize, found u32\n\n" +}"##, + ); + + 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); +} + +#[test] +fn snap_handles_macro_location() { + let diag = parse_diagnostic( + r##"{ +"rendered": "error[E0277]: can't compare `{integer}` with `&str`\n --> src/main.rs:2:5\n |\n2 | assert_eq!(1, \"love\");\n | ^^^^^^^^^^^^^^^^^^^^^^ no implementation for `{integer} == &str`\n |\n = help: the trait `std::cmp::PartialEq<&str>` is not implemented for `{integer}`\n = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)\n\n", +"children": [ + { + "children": [], + "code": null, + "level": "help", + "message": "the trait `std::cmp::PartialEq<&str>` is not implemented for `{integer}`", + "rendered": null, + "spans": [] + } +], +"code": { + "code": "E0277", + "explanation": "\nYou tried to use a type which doesn't implement some trait in a place which\nexpected that trait. Erroneous code example:\n\n```compile_fail,E0277\n// here we declare the Foo trait with a bar method\ntrait Foo {\n fn bar(&self);\n}\n\n// we now declare a function which takes an object implementing the Foo trait\nfn some_func(foo: T) {\n foo.bar();\n}\n\nfn main() {\n // we now call the method with the i32 type, which doesn't implement\n // the Foo trait\n some_func(5i32); // error: the trait bound `i32 : Foo` is not satisfied\n}\n```\n\nIn order to fix this error, verify that the type you're using does implement\nthe trait. Example:\n\n```\ntrait Foo {\n fn bar(&self);\n}\n\nfn some_func(foo: T) {\n foo.bar(); // we can now use this method since i32 implements the\n // Foo trait\n}\n\n// we implement the trait on the i32 type\nimpl Foo for i32 {\n fn bar(&self) {}\n}\n\nfn main() {\n some_func(5i32); // ok!\n}\n```\n\nOr in a generic context, an erroneous code example would look like:\n\n```compile_fail,E0277\nfn some_func(foo: T) {\n println!(\"{:?}\", foo); // error: the trait `core::fmt::Debug` is not\n // implemented for the type `T`\n}\n\nfn main() {\n // We now call the method with the i32 type,\n // which *does* implement the Debug trait.\n some_func(5i32);\n}\n```\n\nNote that the error here is in the definition of the generic function: Although\nwe only call it with a parameter that does implement `Debug`, the compiler\nstill rejects the function: It must work with all possible input types. In\norder to make this example compile, we need to restrict the generic type we're\naccepting:\n\n```\nuse std::fmt;\n\n// Restrict the input type to types that implement Debug.\nfn some_func(foo: T) {\n println!(\"{:?}\", foo);\n}\n\nfn main() {\n // Calling the method is still fine, as i32 implements Debug.\n some_func(5i32);\n\n // This would fail to compile now:\n // struct WithoutDebug;\n // some_func(WithoutDebug);\n}\n```\n\nRust only looks at the signature of the called function, as such it must\nalready specify all requirements that will be used for every type parameter.\n" +}, +"level": "error", +"message": "can't compare `{integer}` with `&str`", +"spans": [ + { + "byte_end": 155, + "byte_start": 153, + "column_end": 33, + "column_start": 31, + "expansion": { + "def_site_span": { + "byte_end": 940, + "byte_start": 0, + "column_end": 6, + "column_start": 1, + "expansion": null, + "file_name": "<::core::macros::assert_eq macros>", + "is_primary": false, + "label": null, + "line_end": 36, + "line_start": 1, + "suggested_replacement": null, + "suggestion_applicability": null, + "text": [ + { + "highlight_end": 35, + "highlight_start": 1, + "text": "($ left : expr, $ right : expr) =>" + }, + { + "highlight_end": 3, + "highlight_start": 1, + "text": "({" + }, + { + "highlight_end": 33, + "highlight_start": 1, + "text": " match (& $ left, & $ right)" + }, + { + "highlight_end": 7, + "highlight_start": 1, + "text": " {" + }, + { + "highlight_end": 34, + "highlight_start": 1, + "text": " (left_val, right_val) =>" + }, + { + "highlight_end": 11, + "highlight_start": 1, + "text": " {" + }, + { + "highlight_end": 46, + "highlight_start": 1, + "text": " if ! (* left_val == * right_val)" + }, + { + "highlight_end": 15, + "highlight_start": 1, + "text": " {" + }, + { + "highlight_end": 25, + "highlight_start": 1, + "text": " panic !" + }, + { + "highlight_end": 57, + "highlight_start": 1, + "text": " (r#\"assertion failed: `(left == right)`" + }, + { + "highlight_end": 16, + "highlight_start": 1, + "text": " left: `{:?}`," + }, + { + "highlight_end": 18, + "highlight_start": 1, + "text": " right: `{:?}`\"#," + }, + { + "highlight_end": 47, + "highlight_start": 1, + "text": " & * left_val, & * right_val)" + }, + { + "highlight_end": 15, + "highlight_start": 1, + "text": " }" + }, + { + "highlight_end": 11, + "highlight_start": 1, + "text": " }" + }, + { + "highlight_end": 7, + "highlight_start": 1, + "text": " }" + }, + { + "highlight_end": 42, + "highlight_start": 1, + "text": " }) ; ($ left : expr, $ right : expr,) =>" + }, + { + "highlight_end": 49, + "highlight_start": 1, + "text": "({ $ crate :: assert_eq ! ($ left, $ right) }) ;" + }, + { + "highlight_end": 53, + "highlight_start": 1, + "text": "($ left : expr, $ right : expr, $ ($ arg : tt) +) =>" + }, + { + "highlight_end": 3, + "highlight_start": 1, + "text": "({" + }, + { + "highlight_end": 37, + "highlight_start": 1, + "text": " match (& ($ left), & ($ right))" + }, + { + "highlight_end": 7, + "highlight_start": 1, + "text": " {" + }, + { + "highlight_end": 34, + "highlight_start": 1, + "text": " (left_val, right_val) =>" + }, + { + "highlight_end": 11, + "highlight_start": 1, + "text": " {" + }, + { + "highlight_end": 46, + "highlight_start": 1, + "text": " if ! (* left_val == * right_val)" + }, + { + "highlight_end": 15, + "highlight_start": 1, + "text": " {" + }, + { + "highlight_end": 25, + "highlight_start": 1, + "text": " panic !" + }, + { + "highlight_end": 57, + "highlight_start": 1, + "text": " (r#\"assertion failed: `(left == right)`" + }, + { + "highlight_end": 16, + "highlight_start": 1, + "text": " left: `{:?}`," + }, + { + "highlight_end": 22, + "highlight_start": 1, + "text": " right: `{:?}`: {}\"#," + }, + { + "highlight_end": 72, + "highlight_start": 1, + "text": " & * left_val, & * right_val, $ crate :: format_args !" + }, + { + "highlight_end": 33, + "highlight_start": 1, + "text": " ($ ($ arg) +))" + }, + { + "highlight_end": 15, + "highlight_start": 1, + "text": " }" + }, + { + "highlight_end": 11, + "highlight_start": 1, + "text": " }" + }, + { + "highlight_end": 7, + "highlight_start": 1, + "text": " }" + }, + { + "highlight_end": 6, + "highlight_start": 1, + "text": " }) ;" + } + ] + }, + "macro_decl_name": "assert_eq!", + "span": { + "byte_end": 38, + "byte_start": 16, + "column_end": 27, + "column_start": 5, + "expansion": null, + "file_name": "src/main.rs", + "is_primary": false, + "label": null, + "line_end": 2, + "line_start": 2, + "suggested_replacement": null, + "suggestion_applicability": null, + "text": [ + { + "highlight_end": 27, + "highlight_start": 5, + "text": " assert_eq!(1, \"love\");" + } + ] + } + }, + "file_name": "<::core::macros::assert_eq macros>", + "is_primary": true, + "label": "no implementation for `{integer} == &str`", + "line_end": 7, + "line_start": 7, + "suggested_replacement": null, + "suggestion_applicability": null, + "text": [ + { + "highlight_end": 33, + "highlight_start": 31, + "text": " if ! (* left_val == * right_val)" + } + ] + } +] +}"##, + ); + + 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); +} diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs new file mode 100644 index 000000000..c86386610 --- /dev/null +++ b/crates/ra_cargo_watch/src/lib.rs @@ -0,0 +1,345 @@ +//! cargo_check provides the functionality needed to run `cargo check` or +//! another compatible command (f.x. clippy) in a background thread and provide +//! LSP diagnostics based on the output of the command. +use cargo_metadata::Message; +use crossbeam_channel::{select, unbounded, Receiver, RecvError, Sender, TryRecvError}; +use lsp_types::{ + Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd, + WorkDoneProgressReport, +}; +use parking_lot::RwLock; +use std::{ + collections::HashMap, + path::PathBuf, + process::{Command, Stdio}, + sync::Arc, + thread::JoinHandle, + time::Instant, +}; + +mod conv; + +use crate::conv::{map_rust_diagnostic_to_lsp, MappedRustDiagnostic, SuggestedFix}; + +#[derive(Clone, Debug)] +pub struct CheckOptions { + pub enable: bool, + pub args: Vec, + pub command: String, + pub all_targets: bool, +} + +/// CheckWatcher wraps the shared state and communication machinery used for +/// running `cargo check` (or other compatible command) and providing +/// diagnostics based on the output. +#[derive(Debug)] +pub struct CheckWatcher { + pub task_recv: Receiver, + pub cmd_send: Sender, + pub shared: Arc>, + handle: JoinHandle<()>, +} + +impl CheckWatcher { + pub fn new(options: &CheckOptions, workspace_root: PathBuf) -> CheckWatcher { + let options = options.clone(); + let shared = Arc::new(RwLock::new(CheckWatcherSharedState::new())); + + let (task_send, task_recv) = unbounded::(); + let (cmd_send, cmd_recv) = unbounded::(); + let shared_ = shared.clone(); + let handle = std::thread::spawn(move || { + let mut check = CheckWatcherState::new(options, workspace_root, shared_); + check.run(&task_send, &cmd_recv); + }); + + CheckWatcher { task_recv, cmd_send, handle, shared } + } + + /// Schedule a re-start of the cargo check worker. + pub fn update(&self) { + self.cmd_send.send(CheckCommand::Update).unwrap(); + } +} + +pub struct CheckWatcherState { + options: CheckOptions, + workspace_root: PathBuf, + running: bool, + watcher: WatchThread, + last_update_req: Option, + shared: Arc>, +} + +#[derive(Debug)] +pub struct CheckWatcherSharedState { + diagnostic_collection: HashMap>, + suggested_fix_collection: HashMap>, +} + +impl CheckWatcherSharedState { + fn new() -> CheckWatcherSharedState { + CheckWatcherSharedState { + diagnostic_collection: HashMap::new(), + suggested_fix_collection: HashMap::new(), + } + } + + /// Clear the cached diagnostics, and schedule updating diagnostics by the + /// server, to clear stale results. + pub fn clear(&mut self, task_send: &Sender) { + let cleared_files: Vec = self.diagnostic_collection.keys().cloned().collect(); + + self.diagnostic_collection.clear(); + self.suggested_fix_collection.clear(); + + for uri in cleared_files { + task_send.send(CheckTask::Update(uri.clone())).unwrap(); + } + } + + pub fn diagnostics_for(&self, uri: &Url) -> Option<&[Diagnostic]> { + self.diagnostic_collection.get(uri).map(|d| d.as_slice()) + } + + pub fn fixes_for(&self, uri: &Url) -> Option<&[SuggestedFix]> { + self.suggested_fix_collection.get(uri).map(|d| d.as_slice()) + } + + fn add_diagnostic(&mut self, file_uri: Url, diagnostic: Diagnostic) { + let diagnostics = self.diagnostic_collection.entry(file_uri).or_default(); + + // If we're building multiple targets it's possible we've already seen this diagnostic + let is_duplicate = diagnostics.iter().any(|d| are_diagnostics_equal(d, &diagnostic)); + if is_duplicate { + return; + } + + diagnostics.push(diagnostic); + } + + fn add_suggested_fix_for_diagnostic( + &mut self, + mut suggested_fix: SuggestedFix, + diagnostic: &Diagnostic, + ) { + let file_uri = suggested_fix.location.uri.clone(); + let file_suggestions = self.suggested_fix_collection.entry(file_uri).or_default(); + + let existing_suggestion: Option<&mut SuggestedFix> = + file_suggestions.iter_mut().find(|s| s == &&suggested_fix); + if let Some(existing_suggestion) = existing_suggestion { + // The existing suggestion also applies to this new diagnostic + existing_suggestion.diagnostics.push(diagnostic.clone()); + } else { + // We haven't seen this suggestion before + suggested_fix.diagnostics.push(diagnostic.clone()); + file_suggestions.push(suggested_fix); + } + } +} + +#[derive(Debug)] +pub enum CheckTask { + /// Request a update of the given files diagnostics + Update(Url), + + /// Request check progress notification to client + Status(WorkDoneProgress), +} + +pub enum CheckCommand { + /// Request re-start of check thread + Update, +} + +impl CheckWatcherState { + pub fn new( + options: CheckOptions, + workspace_root: PathBuf, + shared: Arc>, + ) -> CheckWatcherState { + let watcher = WatchThread::new(&options, &workspace_root); + CheckWatcherState { + options, + workspace_root, + running: false, + watcher, + last_update_req: None, + shared, + } + } + + pub fn run(&mut self, task_send: &Sender, cmd_recv: &Receiver) { + self.running = true; + while self.running { + select! { + recv(&cmd_recv) -> cmd => match cmd { + Ok(cmd) => self.handle_command(cmd), + Err(RecvError) => { + // Command channel has closed, so shut down + self.running = false; + }, + }, + recv(self.watcher.message_recv) -> msg => match msg { + Ok(msg) => self.handle_message(msg, task_send), + Err(RecvError) => {}, + } + }; + + if self.should_recheck() { + self.last_update_req.take(); + self.shared.write().clear(task_send); + + self.watcher.cancel(); + self.watcher = WatchThread::new(&self.options, &self.workspace_root); + } + } + } + + fn should_recheck(&mut self) -> bool { + if let Some(_last_update_req) = &self.last_update_req { + // We currently only request an update on save, as we need up to + // date source on disk for cargo check to do it's magic, so we + // don't really need to debounce the requests at this point. + return true; + } + false + } + + fn handle_command(&mut self, cmd: CheckCommand) { + match cmd { + CheckCommand::Update => self.last_update_req = Some(Instant::now()), + } + } + + fn handle_message(&mut self, msg: CheckEvent, task_send: &Sender) { + match msg { + CheckEvent::Begin => { + task_send + .send(CheckTask::Status(WorkDoneProgress::Begin(WorkDoneProgressBegin { + title: "Running 'cargo check'".to_string(), + cancellable: Some(false), + message: None, + percentage: None, + }))) + .unwrap(); + } + + CheckEvent::End => { + task_send + .send(CheckTask::Status(WorkDoneProgress::End(WorkDoneProgressEnd { + message: None, + }))) + .unwrap(); + } + + CheckEvent::Msg(Message::CompilerArtifact(msg)) => { + task_send + .send(CheckTask::Status(WorkDoneProgress::Report(WorkDoneProgressReport { + cancellable: Some(false), + message: Some(msg.target.name), + percentage: None, + }))) + .unwrap(); + } + + CheckEvent::Msg(Message::CompilerMessage(msg)) => { + let map_result = + match map_rust_diagnostic_to_lsp(&msg.message, &self.workspace_root) { + Some(map_result) => map_result, + None => return, + }; + + let MappedRustDiagnostic { location, diagnostic, suggested_fixes } = map_result; + let file_uri = location.uri.clone(); + + if !suggested_fixes.is_empty() { + for suggested_fix in suggested_fixes { + self.shared + .write() + .add_suggested_fix_for_diagnostic(suggested_fix, &diagnostic); + } + } + self.shared.write().add_diagnostic(file_uri, diagnostic); + + task_send.send(CheckTask::Update(location.uri)).unwrap(); + } + + CheckEvent::Msg(Message::BuildScriptExecuted(_msg)) => {} + CheckEvent::Msg(Message::Unknown) => {} + } + } +} + +/// WatchThread exists to wrap around the communication needed to be able to +/// run `cargo check` without blocking. Currently the Rust standard library +/// doesn't provide a way to read sub-process output without blocking, so we +/// have to wrap sub-processes output handling in a thread and pass messages +/// back over a channel. +struct WatchThread { + message_recv: Receiver, + cancel_send: Sender<()>, +} + +enum CheckEvent { + Begin, + Msg(cargo_metadata::Message), + End, +} + +impl WatchThread { + fn new(options: &CheckOptions, workspace_root: &PathBuf) -> WatchThread { + let mut args: Vec = vec![ + options.command.clone(), + "--message-format=json".to_string(), + "--manifest-path".to_string(), + format!("{}/Cargo.toml", workspace_root.to_string_lossy()), + ]; + if options.all_targets { + args.push("--all-targets".to_string()); + } + args.extend(options.args.iter().cloned()); + + let (message_send, message_recv) = unbounded(); + let (cancel_send, cancel_recv) = unbounded(); + let enabled = options.enable; + std::thread::spawn(move || { + if !enabled { + return; + } + + let mut command = Command::new("cargo") + .args(&args) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("couldn't launch cargo"); + + message_send.send(CheckEvent::Begin).unwrap(); + for message in cargo_metadata::parse_messages(command.stdout.take().unwrap()) { + match cancel_recv.try_recv() { + Ok(()) | Err(TryRecvError::Disconnected) => { + command.kill().expect("couldn't kill command"); + } + Err(TryRecvError::Empty) => (), + } + + message_send.send(CheckEvent::Msg(message.unwrap())).unwrap(); + } + message_send.send(CheckEvent::End).unwrap(); + }); + WatchThread { message_recv, cancel_send } + } + + fn cancel(&self) { + let _ = self.cancel_send.send(()); + } +} + +fn are_diagnostics_equal(left: &Diagnostic, right: &Diagnostic) -> bool { + left.source == right.source + && left.severity == right.severity + && left.range == right.range + && left.message == right.message +} -- cgit v1.2.3 From a2d10694ccfcd12dad8796fc86966ea10ca3fc01 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 11:31:25 +0100 Subject: Consistent, hopefully robust, shutdown/cancelation story for cargo check subprocess --- crates/ra_cargo_watch/src/lib.rs | 66 ++++++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 16 deletions(-) (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index c86386610..70afd7f8a 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -2,7 +2,7 @@ //! another compatible command (f.x. clippy) in a background thread and provide //! LSP diagnostics based on the output of the command. use cargo_metadata::Message; -use crossbeam_channel::{select, unbounded, Receiver, RecvError, Sender, TryRecvError}; +use crossbeam_channel::{select, unbounded, Receiver, RecvError, Sender}; use lsp_types::{ Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd, WorkDoneProgressReport, @@ -191,7 +191,8 @@ impl CheckWatcherState { self.last_update_req.take(); self.shared.write().clear(task_send); - self.watcher.cancel(); + // By replacing the watcher, we drop the previous one which + // causes it to shut down automatically. self.watcher = WatchThread::new(&self.options, &self.workspace_root); } } @@ -277,9 +278,11 @@ impl CheckWatcherState { /// doesn't provide a way to read sub-process output without blocking, so we /// have to wrap sub-processes output handling in a thread and pass messages /// back over a channel. +/// The correct way to dispose of the thread is to drop it, on which the +/// sub-process will be killed, and the thread will be joined. struct WatchThread { + handle: Option>, message_recv: Receiver, - cancel_send: Sender<()>, } enum CheckEvent { @@ -302,9 +305,8 @@ impl WatchThread { args.extend(options.args.iter().cloned()); let (message_send, message_recv) = unbounded(); - let (cancel_send, cancel_recv) = unbounded(); let enabled = options.enable; - std::thread::spawn(move || { + let handle = std::thread::spawn(move || { if !enabled { return; } @@ -316,24 +318,56 @@ impl WatchThread { .spawn() .expect("couldn't launch cargo"); - message_send.send(CheckEvent::Begin).unwrap(); + // If we trigger an error here, we will do so in the loop instead, + // which will break out of the loop, and continue the shutdown + let _ = message_send.send(CheckEvent::Begin); + for message in cargo_metadata::parse_messages(command.stdout.take().unwrap()) { - match cancel_recv.try_recv() { - Ok(()) | Err(TryRecvError::Disconnected) => { - command.kill().expect("couldn't kill command"); + let message = match message { + Ok(message) => message, + Err(err) => { + log::error!("Invalid json from cargo check, ignoring: {}", err); + continue; } - Err(TryRecvError::Empty) => (), - } + }; - message_send.send(CheckEvent::Msg(message.unwrap())).unwrap(); + match message_send.send(CheckEvent::Msg(message)) { + Ok(()) => {} + Err(_err) => { + // The send channel was closed, so we want to shutdown + break; + } + } } - message_send.send(CheckEvent::End).unwrap(); + + // We can ignore any error here, as we are already in the progress + // of shutting down. + let _ = message_send.send(CheckEvent::End); + + // It is okay to ignore the result, as it only errors if the process is already dead + let _ = command.kill(); + + // Again, we don't care about the exit status so just ignore the result + let _ = command.wait(); }); - WatchThread { message_recv, cancel_send } + WatchThread { handle: Some(handle), message_recv } } +} - fn cancel(&self) { - let _ = self.cancel_send.send(()); +impl std::ops::Drop for WatchThread { + fn drop(&mut self) { + if let Some(handle) = self.handle.take() { + // Replace our reciever with dummy one, so we can drop and close the + // one actually communicating with the thread + let recv = std::mem::replace(&mut self.message_recv, crossbeam_channel::never()); + + // Dropping the original reciever initiates thread sub-process shutdown + drop(recv); + + // Join the thread, it should finish shortly. We don't really care + // whether it panicked, so it is safe to ignore the result + let _ = handle.join(); + } } } -- cgit v1.2.3 From 02ce5bbf6b46a67c3fc12b964de204b8130f1b10 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 11:43:05 +0100 Subject: Shutdown/cancelation story for main cargo watch thread --- crates/ra_cargo_watch/src/lib.rs | 47 +++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 15 deletions(-) (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index 70afd7f8a..4af26ff8c 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -32,12 +32,13 @@ pub struct CheckOptions { /// CheckWatcher wraps the shared state and communication machinery used for /// running `cargo check` (or other compatible command) and providing /// diagnostics based on the output. +/// The spawned thread is shut down when this struct is dropped. #[derive(Debug)] pub struct CheckWatcher { pub task_recv: Receiver, pub cmd_send: Sender, pub shared: Arc>, - handle: JoinHandle<()>, + handle: Option>, } impl CheckWatcher { @@ -52,8 +53,7 @@ impl CheckWatcher { let mut check = CheckWatcherState::new(options, workspace_root, shared_); check.run(&task_send, &cmd_recv); }); - - CheckWatcher { task_recv, cmd_send, handle, shared } + CheckWatcher { task_recv, cmd_send, handle: Some(handle), shared } } /// Schedule a re-start of the cargo check worker. @@ -62,13 +62,21 @@ impl CheckWatcher { } } -pub struct CheckWatcherState { - options: CheckOptions, - workspace_root: PathBuf, - running: bool, - watcher: WatchThread, - last_update_req: Option, - shared: Arc>, +impl std::ops::Drop for CheckWatcher { + fn drop(&mut self) { + if let Some(handle) = self.handle.take() { + // Replace our reciever with dummy one, so we can drop and close the + // one actually communicating with the thread + let recv = std::mem::replace(&mut self.task_recv, crossbeam_channel::never()); + + // Dropping the original reciever finishes the thread loop + drop(recv); + + // Join the thread, it should finish shortly. We don't really care + // whether it panicked, so it is safe to ignore the result + let _ = handle.join(); + } + } } #[derive(Debug)] @@ -153,6 +161,14 @@ pub enum CheckCommand { Update, } +struct CheckWatcherState { + options: CheckOptions, + workspace_root: PathBuf, + watcher: WatchThread, + last_update_req: Option, + shared: Arc>, +} + impl CheckWatcherState { pub fn new( options: CheckOptions, @@ -163,7 +179,6 @@ impl CheckWatcherState { CheckWatcherState { options, workspace_root, - running: false, watcher, last_update_req: None, shared, @@ -171,19 +186,21 @@ impl CheckWatcherState { } pub fn run(&mut self, task_send: &Sender, cmd_recv: &Receiver) { - self.running = true; - while self.running { + loop { select! { recv(&cmd_recv) -> cmd => match cmd { Ok(cmd) => self.handle_command(cmd), Err(RecvError) => { // Command channel has closed, so shut down - self.running = false; + break; }, }, recv(self.watcher.message_recv) -> msg => match msg { Ok(msg) => self.handle_message(msg, task_send), - Err(RecvError) => {}, + Err(RecvError) => { + // Task channel has closed, so shut down + break; + }, } }; -- cgit v1.2.3 From 4d33835a34b4ccdfc4ea34b8f3f8ff61d092a543 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 11:47:09 +0100 Subject: Cargo fmt run --- crates/ra_cargo_watch/src/lib.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index 4af26ff8c..d17edd775 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -176,13 +176,7 @@ impl CheckWatcherState { shared: Arc>, ) -> CheckWatcherState { let watcher = WatchThread::new(&options, &workspace_root); - CheckWatcherState { - options, - workspace_root, - watcher, - last_update_req: None, - shared, - } + CheckWatcherState { options, workspace_root, watcher, last_update_req: None, shared } } pub fn run(&mut self, task_send: &Sender, cmd_recv: &Receiver) { -- cgit v1.2.3 From 59837c75f44f4b1b6a09e8db7e59a7bbd78d074a Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 11:57:00 +0100 Subject: Add doc comment to module --- crates/ra_cargo_watch/src/conv/test.rs | 2 ++ 1 file changed, 2 insertions(+) (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/src/conv/test.rs b/crates/ra_cargo_watch/src/conv/test.rs index 69a07be11..6817245c2 100644 --- a/crates/ra_cargo_watch/src/conv/test.rs +++ b/crates/ra_cargo_watch/src/conv/test.rs @@ -1,3 +1,5 @@ +//! This module contains the large and verbose snapshot tests for the +//! conversions between `cargo check` json and LSP diagnostics. use crate::*; fn parse_diagnostic(val: &str) -> cargo_metadata::diagnostic::Diagnostic { -- cgit v1.2.3 From ed84c85aef859e97ab355e78bf77f435689f25b7 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 12:42:18 +0100 Subject: Fix shutdown behavoir of main cargo-watch thread. Even though this didn't error, it became clear to me that it was closing the wrong channel, resulting in the child thread never finishing. --- crates/ra_cargo_watch/src/lib.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index d17edd775..11b624abf 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -36,7 +36,7 @@ pub struct CheckOptions { #[derive(Debug)] pub struct CheckWatcher { pub task_recv: Receiver, - pub cmd_send: Sender, + pub cmd_send: Option>, pub shared: Arc>, handle: Option>, } @@ -53,23 +53,24 @@ impl CheckWatcher { let mut check = CheckWatcherState::new(options, workspace_root, shared_); check.run(&task_send, &cmd_recv); }); - CheckWatcher { task_recv, cmd_send, handle: Some(handle), shared } + CheckWatcher { task_recv, cmd_send: Some(cmd_send), handle: Some(handle), shared } } /// Schedule a re-start of the cargo check worker. pub fn update(&self) { - self.cmd_send.send(CheckCommand::Update).unwrap(); + if let Some(cmd_send) = &self.cmd_send { + cmd_send.send(CheckCommand::Update).unwrap(); + } } } impl std::ops::Drop for CheckWatcher { fn drop(&mut self) { if let Some(handle) = self.handle.take() { - // Replace our reciever with dummy one, so we can drop and close the - // one actually communicating with the thread - let recv = std::mem::replace(&mut self.task_recv, crossbeam_channel::never()); + // Take the sender out of the option + let recv = self.cmd_send.take(); - // Dropping the original reciever finishes the thread loop + // Dropping the sender finishes the thread loop drop(recv); // Join the thread, it should finish shortly. We don't really care -- cgit v1.2.3 From c732f215cb31e9f022090b8d0212f6ea9c134c11 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 12:43:14 +0100 Subject: Don't finish main cargo watch thread when subprocess finishes. --- crates/ra_cargo_watch/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index 11b624abf..d683d43d2 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -193,8 +193,7 @@ impl CheckWatcherState { recv(self.watcher.message_recv) -> msg => match msg { Ok(msg) => self.handle_message(msg, task_send), Err(RecvError) => { - // Task channel has closed, so shut down - break; + // Watcher finished, do nothing. }, } }; -- cgit v1.2.3 From 899dbebd02b41b12d89c9f485e85208b39b81932 Mon Sep 17 00:00:00 2001 From: Emil Lauridsen Date: Fri, 27 Dec 2019 17:29:02 +0100 Subject: Fix busy-waiting issue in main cargo watch thread --- crates/ra_cargo_watch/src/lib.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'crates/ra_cargo_watch') diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs index d683d43d2..78250f910 100644 --- a/crates/ra_cargo_watch/src/lib.rs +++ b/crates/ra_cargo_watch/src/lib.rs @@ -2,7 +2,7 @@ //! another compatible command (f.x. clippy) in a background thread and provide //! LSP diagnostics based on the output of the command. use cargo_metadata::Message; -use crossbeam_channel::{select, unbounded, Receiver, RecvError, Sender}; +use crossbeam_channel::{never, select, unbounded, Receiver, RecvError, Sender}; use lsp_types::{ Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd, WorkDoneProgressReport, @@ -193,7 +193,9 @@ impl CheckWatcherState { recv(self.watcher.message_recv) -> msg => match msg { Ok(msg) => self.handle_message(msg, task_send), Err(RecvError) => { - // Watcher finished, do nothing. + // Watcher finished, replace it with a never channel to + // avoid busy-waiting. + std::mem::replace(&mut self.watcher.message_recv, never()); }, } }; @@ -370,7 +372,7 @@ impl std::ops::Drop for WatchThread { if let Some(handle) = self.handle.take() { // Replace our reciever with dummy one, so we can drop and close the // one actually communicating with the thread - let recv = std::mem::replace(&mut self.message_recv, crossbeam_channel::never()); + let recv = std::mem::replace(&mut self.message_recv, never()); // Dropping the original reciever initiates thread sub-process shutdown drop(recv); -- cgit v1.2.3