12 files changed, 136 insertions, 123 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 3f4d9204c..f8d26192d 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -845,14 +845,9 @@ checksum = "74490b50b9fbe561ac330df47c08f3f33073d2d00c150f719147d7c54522fa1b"
 
 [[package]]
 name = "proc-macro-hack"
-version = "0.5.12"
+version = "0.5.14"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f918f2b601f93baa836c1c2945faef682ba5b6d4828ecb45eeb7cc3c71b811b4"
-dependencies = [
- "proc-macro2",
- "quote",
- "syn",
-]
+checksum = "fcfdefadc3d57ca21cf17990a28ef4c0f7c61383a28cb7604cf4a18e6ede1420"
 
 [[package]]
 name = "proc-macro2"
@@ -1480,9 +1475,9 @@ checksum = "ab16ced94dbd8a46c82fd81e3ed9a8727dac2977ea869d217bcc4ea1f122e81f"
 
 [[package]]
 name = "syn"
-version = "1.0.16"
+version = "1.0.17"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "123bd9499cfb380418d509322d7a6d52e5315f064fe4b3ad18a53d6b92c07859"
+checksum = "0df0eb663f387145cab623dea85b09c2c5b4b0aef44e945d928e682fce71bb03"
 dependencies = [
  "proc-macro2",
  "quote",
diff --git a/crates/ra_cargo_watch/src/conv.rs b/crates/ra_cargo_watch/src/conv.rs
index c6f8ca329..817543deb 100644
--- a/crates/ra_cargo_watch/src/conv.rs
+++ b/crates/ra_cargo_watch/src/conv.rs
@@ -1,7 +1,8 @@
 //! This module provides the functionality needed to convert diagnostics from
 //! `cargo check` json format to the LSP diagnostic format.
 use cargo_metadata::diagnostic::{
-    Diagnostic as RustDiagnostic, DiagnosticLevel, DiagnosticSpan, DiagnosticSpanMacroExpansion,
+    Applicability, Diagnostic as RustDiagnostic, DiagnosticLevel, DiagnosticSpan,
+    DiagnosticSpanMacroExpansion,
 };
 use lsp_types::{
     CodeAction, Diagnostic, DiagnosticRelatedInformation, DiagnosticSeverity, DiagnosticTag,
@@ -136,10 +137,13 @@ fn map_rust_child_diagnostic(
 
     let mut edit_map: HashMap<Url, Vec<TextEdit>> = 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);
+        match (&span.suggestion_applicability, &span.suggested_replacement) {
+            (Some(Applicability::MachineApplicable), Some(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);
+            }
+            _ => {}
         }
     }
 
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 9e8f4eff4..a59fa84fa 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
@@ -58,44 +58,26 @@ expression: diag
                         },
                         message: "lint level defined here",
                     },
+                    DiagnosticRelatedInformation {
+                        location: Location {
+                            uri: "file:///test/compiler/mir/tagset.rs",
+                            range: Range {
+                                start: Position {
+                                    line: 41,
+                                    character: 23,
+                                },
+                                end: Position {
+                                    line: 41,
+                                    character: 28,
+                                },
+                            },
+                        },
+                        message: "consider passing by value instead",
+                    },
                 ],
             ),
             tags: None,
         },
-        fixes: [
-            CodeAction {
-                title: "consider passing by value instead",
-                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,
-                    },
-                ),
-                command: None,
-                is_preferred: None,
-            },
-        ],
+        fixes: [],
     },
 ]
diff --git a/crates/ra_cargo_watch/src/lib.rs b/crates/ra_cargo_watch/src/lib.rs
index bffe5eb00..7c525c430 100644
--- a/crates/ra_cargo_watch/src/lib.rs
+++ b/crates/ra_cargo_watch/src/lib.rs
@@ -8,9 +8,10 @@ use lsp_types::{
     WorkDoneProgressEnd, WorkDoneProgressReport,
 };
 use std::{
+    error, fmt,
     io::{BufRead, BufReader},
     path::{Path, PathBuf},
-    process::{Child, Command, Stdio},
+    process::{Command, Stdio},
     thread::JoinHandle,
     time::Instant,
 };
@@ -70,10 +71,10 @@ impl std::ops::Drop for CheckWatcher {
     fn drop(&mut self) {
         if let Some(handle) = self.handle.take() {
             // Take the sender out of the option
-            let recv = self.cmd_send.take();
+            let cmd_send = self.cmd_send.take();
 
             // Dropping the sender finishes the thread loop
-            drop(recv);
+            drop(cmd_send);
 
             // Join the thread, it should finish shortly. We don't really care
             // whether it panicked, so it is safe to ignore the result
@@ -246,11 +247,21 @@ enum CheckEvent {
     End,
 }
 
+#[derive(Debug)]
+pub struct CargoError(String);
+
+impl fmt::Display for CargoError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "Cargo failed: {}", self.0)
+    }
+}
+impl error::Error for CargoError {}
+
 pub fn run_cargo(
     args: &[String],
     current_dir: Option<&Path>,
     on_message: &mut dyn FnMut(cargo_metadata::Message) -> bool,
-) -> Child {
+) -> Result<(), CargoError> {
     let mut command = Command::new("cargo");
     if let Some(current_dir) = current_dir {
         command.current_dir(current_dir);
@@ -273,6 +284,8 @@ pub fn run_cargo(
     // simply skip a line if it doesn't parse, which just ignores any
     // erroneus output.
     let stdout = BufReader::new(child.stdout.take().unwrap());
+    let mut read_at_least_one_message = false;
+
     for line in stdout.lines() {
         let line = match line {
             Ok(line) => line,
@@ -291,12 +304,31 @@ pub fn run_cargo(
             }
         };
 
+        read_at_least_one_message = true;
+
         if !on_message(message) {
             break;
         }
     }
 
-    child
+    // It is okay to ignore the result, as it only errors if the process is already dead
+    let _ = child.kill();
+
+    let err_msg = match child.wait() {
+        Ok(exit_code) if !exit_code.success() && !read_at_least_one_message => {
+            // FIXME: Read the stderr to display the reason, see `read2()` reference in PR comment:
+            // https://github.com/rust-analyzer/rust-analyzer/pull/3632#discussion_r395605298
+            format!(
+                "the command produced no valid metadata (exit code: {:?}): cargo {}",
+                exit_code,
+                args.join(" ")
+            )
+        }
+        Err(err) => format!("io error: {:?}", err),
+        Ok(_) => return Ok(()),
+    };
+
+    Err(CargoError(err_msg))
 }
 
 impl WatchThread {
@@ -325,7 +357,7 @@ impl WatchThread {
                 // which will break out of the loop, and continue the shutdown
                 let _ = message_send.send(CheckEvent::Begin);
 
-                let mut child = run_cargo(&args, Some(&workspace_root), &mut |message| {
+                let res = run_cargo(&args, Some(&workspace_root), &mut |message| {
                     // Skip certain kinds of messages to only spend time on what's useful
                     match &message {
                         Message::CompilerArtifact(artifact) if artifact.fresh => return true,
@@ -334,26 +366,19 @@ impl WatchThread {
                         _ => {}
                     }
 
-                    match message_send.send(CheckEvent::Msg(message)) {
-                        Ok(()) => {}
-                        Err(_err) => {
-                            // The send channel was closed, so we want to shutdown
-                            return false;
-                        }
-                    };
-
-                    true
+                    // if the send channel was closed, we want to shutdown
+                    message_send.send(CheckEvent::Msg(message)).is_ok()
                 });
 
+                if let Err(err) = res {
+                    // FIXME: make the `message_send` to be `Sender<Result<CheckEvent, CargoError>>`
+                    // to display user-caused misconfiguration errors instead of just logging them here
+                    log::error!("Cargo watcher failed {:?}", err);
+                }
+
                 // 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 _ = child.kill();
-
-                // Again, we don't care about the exit status so just ignore the result
-                let _ = child.wait();
             }))
         } else {
             None
diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs
index 45e31095c..4b150ef06 100644
--- a/crates/ra_hir/src/code_model.rs
+++ b/crates/ra_hir/src/code_model.rs
@@ -1042,30 +1042,18 @@ impl Type {
     }
 
     pub fn is_bool(&self) -> bool {
-        match &self.ty.value {
-            Ty::Apply(a_ty) => match a_ty.ctor {
-                TypeCtor::Bool => true,
-                _ => false,
-            },
-            _ => false,
-        }
+        matches!(self.ty.value, Ty::Apply(ApplicationTy { ctor: TypeCtor::Bool, .. }))
     }
 
     pub fn is_mutable_reference(&self) -> bool {
-        match &self.ty.value {
-            Ty::Apply(a_ty) => match a_ty.ctor {
-                TypeCtor::Ref(Mutability::Mut) => true,
-                _ => false,
-            },
-            _ => false,
-        }
+        matches!(
+            self.ty.value,
+            Ty::Apply(ApplicationTy { ctor: TypeCtor::Ref(Mutability::Mut), .. })
+        )
     }
 
     pub fn is_unknown(&self) -> bool {
-        match &self.ty.value {
-            Ty::Unknown => true,
-            _ => false,
-        }
+        matches!(self.ty.value, Ty::Unknown)
     }
 
     /// Checks that particular type `ty` implements `std::future::Future`.
diff --git a/crates/ra_hir/src/lib.rs b/crates/ra_hir/src/lib.rs
index 9f59d590c..713d45f48 100644
--- a/crates/ra_hir/src/lib.rs
+++ b/crates/ra_hir/src/lib.rs
@@ -4,6 +4,18 @@
 //! The principal difference between HIR and syntax trees is that HIR is bound
 //! to a particular crate instance. That is, it has cfg flags and features
 //! applied. So, the relation between syntax and HIR is many-to-one.
+//!
+//! HIR is the public API of the all of the compiler logic above syntax trees.
+//! It is written in "OO" style. Each type is self contained (as in, it knows it's
+//! parents and full context). It should be "clean code".
+//!
+//! `ra_hir_*` crates are the implementation of the compiler logic.
+//! They are written in "ECS" style, with relatively little abstractions.
+//! Many types are not self-contained, and explicitly use local indexes, arenas, etc.
+//!
+//! `ra_hir` is what insulates the "we don't know how to actually write an incremental compiler"
+//! from the ide with completions, hovers, etc. It is a (soft, internal) boundary:
+//! https://www.tedinski.com/2018/02/06/system-boundaries.html.
 
 #![recursion_limit = "512"]
 
diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs
index e8c58ed32..3cf0c66ea 100644
--- a/crates/ra_hir_def/src/body/lower.rs
+++ b/crates/ra_hir_def/src/body/lower.rs
@@ -453,7 +453,7 @@ impl ExprCollector<'_> {
                 }
             }
             ast::Expr::MacroCall(e) => {
-                if let Some(name) = is_macro_rules(&e) {
+                if let Some(name) = e.is_macro_rules().map(|it| it.as_name()) {
                     let mac = MacroDefId {
                         krate: Some(self.expander.module.krate),
                         ast_id: Some(self.expander.ast_id(&e)),
@@ -697,16 +697,6 @@ impl ExprCollector<'_> {
     }
 }
 
-fn is_macro_rules(m: &ast::MacroCall) -> Option<Name> {
-    let name = m.path()?.segment()?.name_ref()?.as_name();
-
-    if name == name![macro_rules] {
-        Some(m.name()?.as_name())
-    } else {
-        None
-    }
-}
-
 impl From<ast::BinOp> for BinaryOp {
     fn from(ast_op: ast::BinOp) -> Self {
         match ast_op {
diff --git a/crates/ra_hir_def/src/path.rs b/crates/ra_hir_def/src/path.rs
index 246032c13..904080341 100644
--- a/crates/ra_hir_def/src/path.rs
+++ b/crates/ra_hir_def/src/path.rs
@@ -95,7 +95,7 @@ pub struct Path {
     /// Note that paths like `<Type as Trait>::foo` are desugard to `Trait::<Self=Type>::foo`.
     type_anchor: Option<Box<TypeRef>>,
     mod_path: ModPath,
-    /// Invariant: the same len as self.path.segments
+    /// Invariant: the same len as `self.mod_path.segments`
     generic_args: Vec<Option<Arc<GenericArgs>>>,
 }
 
diff --git a/crates/ra_hir_ty/src/method_resolution.rs b/crates/ra_hir_ty/src/method_resolution.rs
index 69c059ac8..533c6ccfb 100644
--- a/crates/ra_hir_ty/src/method_resolution.rs
+++ b/crates/ra_hir_ty/src/method_resolution.rs
@@ -95,14 +95,14 @@ impl Ty {
         // Types like slice can have inherent impls in several crates, (core and alloc).
         // The corresponding impls are marked with lang items, so we can use them to find the required crates.
         macro_rules! lang_item_crate {
-        ($($name:expr),+ $(,)?) => {{
-            let mut v = ArrayVec::<[LangItemTarget; 2]>::new();
-            $(
-                v.extend(db.lang_item(cur_crate, $name.into()));
-            )+
-            v
-        }};
-    }
+            ($($name:expr),+ $(,)?) => {{
+                let mut v = ArrayVec::<[LangItemTarget; 2]>::new();
+                $(
+                    v.extend(db.lang_item(cur_crate, $name.into()));
+                )+
+                v
+            }};
+        }
 
         let lang_item_targets = match self {
             Ty::Apply(a_ty) => match a_ty.ctor {
diff --git a/crates/ra_ide_db/src/symbol_index.rs b/crates/ra_ide_db/src/symbol_index.rs
index 884359ee3..0f46f93c1 100644
--- a/crates/ra_ide_db/src/symbol_index.rs
+++ b/crates/ra_ide_db/src/symbol_index.rs
@@ -362,6 +362,13 @@ fn to_symbol(node: &SyntaxNode) -> Option<(SmolStr, SyntaxNodePtr, TextRange)> {
             ast::TypeAliasDef(it) => { decl(it) },
             ast::ConstDef(it) => { decl(it) },
             ast::StaticDef(it) => { decl(it) },
+            ast::MacroCall(it) => {
+                if it.is_macro_rules().is_some() {
+                    decl(it)
+                } else {
+                    None
+                }
+            },
             _ => None,
         }
     }
diff --git a/crates/ra_project_model/src/cargo_workspace.rs b/crates/ra_project_model/src/cargo_workspace.rs
index c2857dbfc..c7f9bd873 100644
--- a/crates/ra_project_model/src/cargo_workspace.rs
+++ b/crates/ra_project_model/src/cargo_workspace.rs
@@ -6,7 +6,7 @@ use std::{
 };
 
 use anyhow::{Context, Result};
-use cargo_metadata::{CargoOpt, Message, MetadataCommand, PackageId};
+use cargo_metadata::{BuildScript, CargoOpt, Message, MetadataCommand, PackageId};
 use ra_arena::{Arena, Idx};
 use ra_cargo_watch::run_cargo;
 use ra_db::Edition;
@@ -254,7 +254,7 @@ pub fn load_out_dirs(
         "check".to_string(),
         "--message-format=json".to_string(),
         "--manifest-path".to_string(),
-        format!("{}", cargo_toml.display()),
+        cargo_toml.display().to_string(),
     ];
 
     if cargo_features.all_features {
@@ -263,19 +263,15 @@ pub fn load_out_dirs(
         // FIXME: `NoDefaultFeatures` is mutual exclusive with `SomeFeatures`
         // https://github.com/oli-obk/cargo_metadata/issues/79
         args.push("--no-default-features".to_string());
-    } else if !cargo_features.features.is_empty() {
-        for feature in &cargo_features.features {
-            args.push(feature.clone());
-        }
+    } else {
+        args.extend(cargo_features.features.iter().cloned());
     }
 
-    let mut res = FxHashMap::default();
-    let mut child = run_cargo(&args, cargo_toml.parent(), &mut |message| {
+    let mut acc = FxHashMap::default();
+    let res = run_cargo(&args, cargo_toml.parent(), &mut |message| {
         match message {
-            Message::BuildScriptExecuted(message) => {
-                let package_id = message.package_id;
-                let out_dir = message.out_dir;
-                res.insert(package_id, out_dir);
+            Message::BuildScriptExecuted(BuildScript { package_id, out_dir, .. }) => {
+                acc.insert(package_id, out_dir);
             }
 
             Message::CompilerArtifact(_) => (),
@@ -285,6 +281,9 @@ pub fn load_out_dirs(
         true
     });
 
-    let _ = child.wait();
-    res
+    if let Err(err) = res {
+        log::error!("Failed to load outdirs: {:?}", err);
+    }
+
+    acc
 }
diff --git a/crates/ra_syntax/src/ast/extensions.rs b/crates/ra_syntax/src/ast/extensions.rs
index c3ae8f90e..392731dac 100644
--- a/crates/ra_syntax/src/ast/extensions.rs
+++ b/crates/ra_syntax/src/ast/extensions.rs
@@ -4,7 +4,7 @@
 use itertools::Itertools;
 
 use crate::{
-    ast::{self, child_opt, children, AstNode, AttrInput, SyntaxNode},
+    ast::{self, child_opt, children, AstNode, AttrInput, NameOwner, SyntaxNode},
     SmolStr, SyntaxElement,
     SyntaxKind::*,
     SyntaxToken, T,
@@ -514,3 +514,14 @@ impl ast::Visibility {
         self.syntax().children_with_tokens().any(|it| it.kind() == T![super])
     }
 }
+
+impl ast::MacroCall {
+    pub fn is_macro_rules(&self) -> Option<ast::Name> {
+        let name_ref = self.path()?.segment()?.name_ref()?;
+        if name_ref.text() == "macro_rules" {
+            self.name()
+        } else {
+            None
+        }
+    }
+}