10 files changed, 145 insertions, 183 deletions

diff --git a/crates/ra_cargo_watch/Cargo.toml b/crates/ra_cargo_watch/Cargo.toml
index e88295539..dd814fc9d 100644
--- a/crates/ra_cargo_watch/Cargo.toml
+++ b/crates/ra_cargo_watch/Cargo.toml
@@ -11,7 +11,8 @@ log = "0.4.3"
 cargo_metadata = "0.9.1"
 jod-thread = "0.1.0"
 parking_lot = "0.10.0"
+serde_json = "1.0.45"
 
 [dev-dependencies]
-insta = "0.12.0"
+insta = "0.13.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
index ac0f1d28a..506370535 100644
--- a/crates/ra_cargo_watch/src/conv.rs
+++ b/crates/ra_cargo_watch/src/conv.rs
@@ -1,12 +1,11 @@
 //! 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,
+    Diagnostic as RustDiagnostic, DiagnosticLevel, DiagnosticSpan, DiagnosticSpanMacroExpansion,
 };
 use lsp_types::{
-    Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, DiagnosticTag, Location,
-    NumberOrString, Position, Range, Url,
+    CodeAction, Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, DiagnosticTag,
+    Location, NumberOrString, Position, Range, TextEdit, Url, WorkspaceEdit,
 };
 use std::{
     fmt::Write,
@@ -117,38 +116,9 @@ fn is_deprecated(rd: &RustDiagnostic) -> bool {
     }
 }
 
-#[derive(Debug)]
-pub struct SuggestedFix {
-    pub title: String,
-    pub location: Location,
-    pub replacement: String,
-    pub applicability: Applicability,
-    pub diagnostics: Vec<Diagnostic>,
-}
-
-impl std::cmp::PartialEq<SuggestedFix> 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),
+    SuggestedFix(CodeAction),
     MessageLine(String),
 }
 
@@ -176,12 +146,20 @@ fn map_rust_child_diagnostic(
             rd.message.clone()
         };
 
-        MappedRustChildDiagnostic::SuggestedFix(SuggestedFix {
+        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)
+        };
+
+        MappedRustChildDiagnostic::SuggestedFix(CodeAction {
             title,
-            location,
-            replacement: suggested_replacement.clone(),
-            applicability: span.suggestion_applicability.clone().unwrap_or(Applicability::Unknown),
-            diagnostics: vec![],
+            kind: Some("quickfix".to_string()),
+            diagnostics: None,
+            edit: Some(edit),
+            command: None,
+            is_preferred: None,
         })
     } else {
         MappedRustChildDiagnostic::Related(DiagnosticRelatedInformation {
@@ -195,7 +173,7 @@ fn map_rust_child_diagnostic(
 pub(crate) struct MappedRustDiagnostic {
     pub location: Location,
     pub diagnostic: Diagnostic,
-    pub suggested_fixes: Vec<SuggestedFix>,
+    pub fixes: Vec<CodeAction>,
 }
 
 /// Converts a Rust root diagnostic to LSP form
@@ -250,15 +228,13 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
         }
     }
 
-    let mut suggested_fixes = vec![];
+    let mut 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::SuggestedFix(code_action) => fixes.push(code_action.into()),
             MappedRustChildDiagnostic::MessageLine(message_line) => {
                 write!(&mut message, "\n{}", message_line).unwrap();
 
@@ -295,7 +271,7 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
         tags: if !tags.is_empty() { Some(tags) } else { None },
     };
 
-    Some(MappedRustDiagnostic { location, diagnostic, suggested_fixes })
+    Some(MappedRustDiagnostic { location, diagnostic, fixes })
 }
 
 /// Returns a `Url` object from a given path, will lowercase drive letters if present.
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/ra_cargo_watch__conv__test__snap_clippy_pass_by_ref.snap
index cb0920914..95ca163dc 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/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
@@ -61,25 +61,39 @@ MappedRustDiagnostic {
         ),
         tags: None,
     },
-    suggested_fixes: [
-        SuggestedFix {
+    fixes: [
+        CodeAction {
             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,
-                    },
+            kind: Some(
+                "quickfix",
+            ),
+            diagnostics: None,
+            edit: Some(
+                WorkspaceEdit {
+                    changes: Some(
+                        {
+                            "file:///test/compiler/mir/tagset.rs": [
+                                TextEdit {
+                                    range: Range {
+                                        start: Position {
+                                            line: 41,
+                                            character: 23,
+                                        },
+                                        end: Position {
+                                            line: 41,
+                                            character: 28,
+                                        },
+                                    },
+                                    new_text: "self",
+                                },
+                            ],
+                        },
+                    ),
+                    document_changes: None,
                 },
-            },
-            replacement: "self",
-            applicability: Unspecified,
-            diagnostics: [],
+            ),
+            command: None,
+            is_preferred: None,
         },
     ],
 }
diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_handles_macro_location.snap b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_handles_macro_location.snap
index 19510ecc1..12eb32df4 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_handles_macro_location.snap
+++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_handles_macro_location.snap
@@ -42,5 +42,5 @@ MappedRustDiagnostic {
         related_information: None,
         tags: None,
     },
-    suggested_fixes: [],
+    fixes: [],
 }
diff --git a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_macro_compiler_error.snap b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_macro_compiler_error.snap
index 92f7eec05..7b83a7cd0 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_macro_compiler_error.snap
+++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_macro_compiler_error.snap
@@ -57,5 +57,5 @@ MappedRustDiagnostic {
         ),
         tags: None,
     },
-    suggested_fixes: [],
+    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/ra_cargo_watch__conv__test__snap_rustc_incompatible_type_for_trait.snap
index cf683e4b6..54679c5db 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_incompatible_type_for_trait.snap
+++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_incompatible_type_for_trait.snap
@@ -42,5 +42,5 @@ MappedRustDiagnostic {
         related_information: None,
         tags: None,
     },
-    suggested_fixes: [],
+    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/ra_cargo_watch__conv__test__snap_rustc_mismatched_type.snap
index 8c1483c74..57df4ceaf 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_mismatched_type.snap
+++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_mismatched_type.snap
@@ -42,5 +42,5 @@ MappedRustDiagnostic {
         related_information: None,
         tags: None,
     },
-    suggested_fixes: [],
+    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/ra_cargo_watch__conv__test__snap_rustc_unused_variable.snap
index eb5a2247b..3e1fe736c 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_unused_variable.snap
+++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_unused_variable.snap
@@ -46,25 +46,39 @@ MappedRustDiagnostic {
             ],
         ),
     },
-    suggested_fixes: [
-        SuggestedFix {
+    fixes: [
+        CodeAction {
             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,
-                    },
+            kind: Some(
+                "quickfix",
+            ),
+            diagnostics: None,
+            edit: Some(
+                WorkspaceEdit {
+                    changes: Some(
+                        {
+                            "file:///test/driver/subcommand/repl.rs": [
+                                TextEdit {
+                                    range: Range {
+                                        start: Position {
+                                            line: 290,
+                                            character: 8,
+                                        },
+                                        end: Position {
+                                            line: 290,
+                                            character: 11,
+                                        },
+                                    },
+                                    new_text: "_foo",
+                                },
+                            ],
+                        },
+                    ),
+                    document_changes: None,
                 },
-            },
-            replacement: "_foo",
-            applicability: MachineApplicable,
-            diagnostics: [],
+            ),
+            command: None,
+            is_preferred: None,
         },
     ],
 }
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/ra_cargo_watch__conv__test__snap_rustc_wrong_number_of_parameters.snap
index 2f4518931..69301078d 100644
--- a/crates/ra_cargo_watch/src/conv/snapshots/test__snap_rustc_wrong_number_of_parameters.snap
+++ b/crates/ra_cargo_watch/src/conv/snapshots/ra_cargo_watch__conv__test__snap_rustc_wrong_number_of_parameters.snap
@@ -61,5 +61,5 @@ MappedRustDiagnostic {
         ),
         tags: None,
     },
-    suggested_fixes: [],
+    fixes: [],
 }
diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs
index 7f4c9280c..f07c34549 100644
--- a/crates/ra_cargo_watch/src/lib.rs
+++ b/crates/ra_cargo_watch/src/lib.rs
@@ -4,22 +4,20 @@
 use cargo_metadata::Message;
 use crossbeam_channel::{never, select, unbounded, Receiver, RecvError, Sender};
 use lsp_types::{
-    Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin, WorkDoneProgressEnd,
-    WorkDoneProgressReport,
+    CodeAction, CodeActionOrCommand, Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin,
+    WorkDoneProgressEnd, WorkDoneProgressReport,
 };
-use parking_lot::RwLock;
 use std::{
-    collections::HashMap,
+    io::{BufRead, BufReader},
     path::PathBuf,
     process::{Command, Stdio},
-    sync::Arc,
     thread::JoinHandle,
     time::Instant,
 };
 
 mod conv;
 
-use crate::conv::{map_rust_diagnostic_to_lsp, MappedRustDiagnostic, SuggestedFix};
+use crate::conv::{map_rust_diagnostic_to_lsp, MappedRustDiagnostic};
 
 pub use crate::conv::url_from_path_with_drive_lowercasing;
 
@@ -38,7 +36,6 @@ pub struct CheckOptions {
 #[derive(Debug)]
 pub struct CheckWatcher {
     pub task_recv: Receiver<CheckTask>,
-    pub state: Arc<RwLock<CheckState>>,
     cmd_send: Option<Sender<CheckCommand>>,
     handle: Option<JoinHandle<()>>,
 }
@@ -46,7 +43,6 @@ pub struct CheckWatcher {
 impl CheckWatcher {
     pub fn new(options: &CheckOptions, workspace_root: PathBuf) -> CheckWatcher {
         let options = options.clone();
-        let state = Arc::new(RwLock::new(CheckState::new()));
 
         let (task_send, task_recv) = unbounded::<CheckTask>();
         let (cmd_send, cmd_recv) = unbounded::<CheckCommand>();
@@ -54,13 +50,12 @@ impl CheckWatcher {
             let mut check = CheckWatcherThread::new(options, workspace_root);
             check.run(&task_send, &cmd_recv);
         });
-        CheckWatcher { task_recv, cmd_send: Some(cmd_send), handle: Some(handle), state }
+        CheckWatcher { task_recv, cmd_send: Some(cmd_send), handle: Some(handle) }
     }
 
     /// Returns a CheckWatcher that doesn't actually do anything
     pub fn dummy() -> CheckWatcher {
-        let state = Arc::new(RwLock::new(CheckState::new()));
-        CheckWatcher { task_recv: never(), cmd_send: None, handle: None, state }
+        CheckWatcher { task_recv: never(), cmd_send: None, handle: None }
     }
 
     /// Schedule a re-start of the cargo check worker.
@@ -88,83 +83,12 @@ impl std::ops::Drop for CheckWatcher {
 }
 
 #[derive(Debug)]
-pub struct CheckState {
-    diagnostic_collection: HashMap<Url, Vec<Diagnostic>>,
-    suggested_fix_collection: HashMap<Url, Vec<SuggestedFix>>,
-}
-
-impl CheckState {
-    fn new() -> CheckState {
-        CheckState {
-            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) -> Vec<Url> {
-        let cleared_files: Vec<Url> = self.diagnostic_collection.keys().cloned().collect();
-        self.diagnostic_collection.clear();
-        self.suggested_fix_collection.clear();
-        cleared_files
-    }
-
-    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())
-    }
-
-    pub fn add_diagnostic_with_fixes(&mut self, file_uri: Url, diagnostic: DiagnosticWithFixes) {
-        for fix in diagnostic.suggested_fixes {
-            self.add_suggested_fix_for_diagnostic(fix, &diagnostic.diagnostic);
-        }
-        self.add_diagnostic(file_uri, diagnostic.diagnostic);
-    }
-
-    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 clearing of all cached diagnostics from the check watcher
     ClearDiagnostics,
 
     /// Request adding a diagnostic with fixes included to a file
-    AddDiagnostic(Url, DiagnosticWithFixes),
+    AddDiagnostic { url: Url, diagnostic: Diagnostic, fixes: Vec<CodeActionOrCommand> },
 
     /// Request check progress notification to client
     Status(WorkDoneProgress),
@@ -216,8 +140,10 @@ impl CheckWatcherThread {
                 self.last_update_req.take();
                 task_send.send(CheckTask::ClearDiagnostics).unwrap();
 
-                // By replacing the watcher, we drop the previous one which
-                // causes it to shut down automatically.
+                // Replace with a dummy watcher first so we drop the original and wait for completion
+                std::mem::replace(&mut self.watcher, WatchThread::dummy());
+
+                // Then create the actual new watcher
                 self.watcher = WatchThread::new(&self.options, &self.workspace_root);
             }
         }
@@ -277,10 +203,17 @@ impl CheckWatcherThread {
                         None => return,
                     };
 
-                let MappedRustDiagnostic { location, diagnostic, suggested_fixes } = map_result;
+                let MappedRustDiagnostic { location, diagnostic, fixes } = map_result;
+                let fixes = fixes
+                    .into_iter()
+                    .map(|fix| {
+                        CodeAction { diagnostics: Some(vec![diagnostic.clone()]), ..fix }.into()
+                    })
+                    .collect();
 
-                let diagnostic = DiagnosticWithFixes { diagnostic, suggested_fixes };
-                task_send.send(CheckTask::AddDiagnostic(location.uri, diagnostic)).unwrap();
+                task_send
+                    .send(CheckTask::AddDiagnostic { url: location.uri, diagnostic, fixes })
+                    .unwrap();
             }
 
             CheckEvent::Msg(Message::BuildScriptExecuted(_msg)) => {}
@@ -292,7 +225,7 @@ impl CheckWatcherThread {
 #[derive(Debug)]
 pub struct DiagnosticWithFixes {
     diagnostic: Diagnostic,
-    suggested_fixes: Vec<SuggestedFix>,
+    fixes: Vec<CodeAction>,
 }
 
 /// WatchThread exists to wrap around the communication needed to be able to
@@ -341,6 +274,7 @@ impl WatchThread {
                 .args(&args)
                 .stdout(Stdio::piped())
                 .stderr(Stdio::null())
+                .stdin(Stdio::null())
                 .spawn()
                 .expect("couldn't launch cargo");
 
@@ -348,15 +282,45 @@ impl WatchThread {
             // 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()) {
+            // We manually read a line at a time, instead of using serde's
+            // stream deserializers, because the deserializer cannot recover
+            // from an error, resulting in it getting stuck, because we try to
+            // be resillient against failures.
+            //
+            // Because cargo only outputs one JSON object per line, we can
+            // simply skip a line if it doesn't parse, which just ignores any
+            // erroneus output.
+            let stdout = BufReader::new(command.stdout.take().unwrap());
+            for line in stdout.lines() {
+                let line = match line {
+                    Ok(line) => line,
+                    Err(err) => {
+                        log::error!("Couldn't read line from cargo: {}", err);
+                        continue;
+                    }
+                };
+
+                let message = serde_json::from_str::<cargo_metadata::Message>(&line);
                 let message = match message {
                     Ok(message) => message,
                     Err(err) => {
-                        log::error!("Invalid json from cargo check, ignoring: {}", err);
+                        log::error!(
+                            "Invalid json from cargo check, ignoring ({}): {:?} ",
+                            err,
+                            line
+                        );
                         continue;
                     }
                 };
 
+                // Skip certain kinds of messages to only spend time on what's useful
+                match &message {
+                    Message::CompilerArtifact(artifact) if artifact.fresh => continue,
+                    Message::BuildScriptExecuted(_) => continue,
+                    Message::Unknown => continue,
+                    _ => {}
+                }
+
                 match message_send.send(CheckEvent::Msg(message)) {
                     Ok(()) => {}
                     Err(_err) => {
@@ -396,10 +360,3 @@ impl std::ops::Drop for WatchThread {
         }
     }
 }
-
-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
-}