7 files changed, 262 insertions, 58 deletions

diff --git a/crates/hir_ty/src/infer.rs b/crates/hir_ty/src/infer.rs
index e4407ff50..674e9e6f9 100644
--- a/crates/hir_ty/src/infer.rs
+++ b/crates/hir_ty/src/infer.rs
@@ -210,6 +210,7 @@ struct InferenceContext<'a> {
     table: unify::InferenceTable,
     trait_env: Arc<TraitEnvironment>,
     obligations: Vec<DomainGoal>,
+    last_obligations_check: Option<u32>,
     result: InferenceResult,
     /// The return type of the function being inferred, or the closure if we're
     /// currently within one.
@@ -245,6 +246,7 @@ impl<'a> InferenceContext<'a> {
             result: InferenceResult::default(),
             table: unify::InferenceTable::new(),
             obligations: Vec::default(),
+            last_obligations_check: None,
             return_ty: TyKind::Unknown.intern(&Interner), // set in collect_fn_signature
             trait_env: owner
                 .as_generic_def_id()
@@ -334,6 +336,13 @@ impl<'a> InferenceContext<'a> {
     }
 
     fn resolve_obligations_as_possible(&mut self) {
+        if self.last_obligations_check == Some(self.table.revision) {
+            // no change
+            return;
+        }
+        let _span = profile::span("resolve_obligations_as_possible");
+
+        self.last_obligations_check = Some(self.table.revision);
         let obligations = mem::replace(&mut self.obligations, Vec::new());
         for obligation in obligations {
             let in_env = InEnvironment::new(self.trait_env.env.clone(), obligation.clone());
@@ -360,6 +369,11 @@ impl<'a> InferenceContext<'a> {
         }
     }
 
+    fn push_obligation(&mut self, o: DomainGoal) {
+        self.obligations.push(o);
+        self.last_obligations_check = None;
+    }
+
     fn unify(&mut self, ty1: &Ty, ty2: &Ty) -> bool {
         self.table.unify(ty1, ty2)
     }
@@ -408,8 +422,8 @@ impl<'a> InferenceContext<'a> {
                     }),
                     ty: ty.clone(),
                 };
-                self.obligations.push(trait_ref.cast(&Interner));
-                self.obligations.push(alias_eq.cast(&Interner));
+                self.push_obligation(trait_ref.cast(&Interner));
+                self.push_obligation(alias_eq.cast(&Interner));
                 self.resolve_ty_as_possible(ty)
             }
             None => self.err_ty(),
@@ -436,7 +450,7 @@ impl<'a> InferenceContext<'a> {
         let var = self.table.new_type_var();
         let alias_eq = AliasEq { alias: AliasTy::Projection(proj_ty), ty: var.clone() };
         let obligation = alias_eq.cast(&Interner);
-        self.obligations.push(obligation);
+        self.push_obligation(obligation);
         var
     }
 
diff --git a/crates/hir_ty/src/infer/expr.rs b/crates/hir_ty/src/infer/expr.rs
index 6279aa572..25ab3ea4c 100644
--- a/crates/hir_ty/src/infer/expr.rs
+++ b/crates/hir_ty/src/infer/expr.rs
@@ -99,7 +99,7 @@ impl<'a> InferenceContext<'a> {
             environment: trait_env,
         });
         if self.db.trait_solve(krate, goal.value).is_some() {
-            self.obligations.push(implements_fn_trait);
+            self.push_obligation(implements_fn_trait);
             let output_proj_ty = crate::ProjectionTy {
                 associated_ty_id: to_assoc_type_id(output_assoc_type),
                 substitution: substs,
@@ -964,7 +964,7 @@ impl<'a> InferenceContext<'a> {
                 let (predicate, binders) =
                     predicate.clone().subst(parameters).into_value_and_skipped_binders();
                 always!(binders == 0); // quantified where clauses not yet handled
-                self.obligations.push(predicate.cast(&Interner));
+                self.push_obligation(predicate.cast(&Interner));
             }
             // add obligation for trait implementation, if this is a trait method
             match def {
@@ -974,7 +974,7 @@ impl<'a> InferenceContext<'a> {
                         // construct a TraitRef
                         let substs =
                             parameters.prefix(generics(self.db.upcast(), trait_.into()).len());
-                        self.obligations.push(
+                        self.push_obligation(
                             TraitRef { trait_id: to_chalk_trait_id(trait_), substitution: substs }
                                 .cast(&Interner),
                         );
diff --git a/crates/hir_ty/src/infer/path.rs b/crates/hir_ty/src/infer/path.rs
index cefa38509..717738789 100644
--- a/crates/hir_ty/src/infer/path.rs
+++ b/crates/hir_ty/src/infer/path.rs
@@ -258,7 +258,7 @@ impl<'a> InferenceContext<'a> {
                             .push(ty.clone())
                             .fill(std::iter::repeat_with(|| self.table.new_type_var()))
                             .build();
-                        self.obligations.push(
+                        self.push_obligation(
                             TraitRef {
                                 trait_id: to_chalk_trait_id(trait_),
                                 substitution: trait_substs.clone(),
diff --git a/crates/hir_ty/src/infer/unify.rs b/crates/hir_ty/src/infer/unify.rs
index 6e7b0f5a6..5ea4b7481 100644
--- a/crates/hir_ty/src/infer/unify.rs
+++ b/crates/hir_ty/src/infer/unify.rs
@@ -231,6 +231,7 @@ pub(crate) struct TypeVariableData {
 pub(crate) struct InferenceTable {
     pub(super) var_unification_table: InPlaceUnificationTable<TypeVarId>,
     pub(super) type_variable_table: TypeVariableTable,
+    pub(super) revision: u32,
 }
 
 impl InferenceTable {
@@ -238,6 +239,7 @@ impl InferenceTable {
         InferenceTable {
             var_unification_table: InPlaceUnificationTable::new(),
             type_variable_table: TypeVariableTable { inner: Vec::new() },
+            revision: 0,
         }
     }
 
@@ -360,7 +362,10 @@ impl InferenceTable {
                 == self.type_variable_table.is_diverging(*tv2) =>
             {
                 // both type vars are unknown since we tried to resolve them
-                self.var_unification_table.union(tv1.to_inner(), tv2.to_inner());
+                if !self.var_unification_table.unioned(tv1.to_inner(), tv2.to_inner()) {
+                    self.var_unification_table.union(tv1.to_inner(), tv2.to_inner());
+                    self.revision += 1;
+                }
                 true
             }
 
@@ -398,6 +403,7 @@ impl InferenceTable {
                     tv.to_inner(),
                     TypeVarValue::Known(other.clone().intern(&Interner)),
                 );
+                self.revision += 1;
                 true
             }
 
diff --git a/crates/rust-analyzer/src/diagnostics/test_data/macro_compiler_error.txt b/crates/rust-analyzer/src/diagnostics/test_data/macro_compiler_error.txt
index f999848a7..c847bbb35 100644
--- a/crates/rust-analyzer/src/diagnostics/test_data/macro_compiler_error.txt
+++ b/crates/rust-analyzer/src/diagnostics/test_data/macro_compiler_error.txt
@@ -13,16 +13,16 @@
         diagnostic: Diagnostic {
             range: Range {
                 start: Position {
-                    line: 264,
+                    line: 271,
                     character: 8,
                 },
                 end: Position {
-                    line: 264,
-                    character: 76,
+                    line: 271,
+                    character: 50,
                 },
             },
             severity: Some(
-                Error,
+                Hint,
             ),
             code: None,
             code_description: None,
@@ -40,18 +40,18 @@
                                 password: None,
                                 host: None,
                                 port: None,
-                                path: "/test/crates/hir_def/src/data.rs",
+                                path: "/test/crates/hir_def/src/path.rs",
                                 query: None,
                                 fragment: None,
                             },
                             range: Range {
                                 start: Position {
-                                    line: 79,
-                                    character: 15,
+                                    line: 264,
+                                    character: 8,
                                 },
                                 end: Position {
-                                    line: 79,
-                                    character: 41,
+                                    line: 264,
+                                    character: 76,
                                 },
                             },
                         },
@@ -87,6 +87,71 @@
                 },
             },
             severity: Some(
+                Hint,
+            ),
+            code: None,
+            code_description: None,
+            source: Some(
+                "rustc",
+            ),
+            message: "Please register your known path in the path module",
+            related_information: Some(
+                [
+                    DiagnosticRelatedInformation {
+                        location: Location {
+                            uri: Url {
+                                scheme: "file",
+                                username: "",
+                                password: None,
+                                host: None,
+                                port: None,
+                                path: "/test/crates/hir_def/src/path.rs",
+                                query: None,
+                                fragment: None,
+                            },
+                            range: Range {
+                                start: Position {
+                                    line: 264,
+                                    character: 8,
+                                },
+                                end: Position {
+                                    line: 264,
+                                    character: 76,
+                                },
+                            },
+                        },
+                        message: "Exact error occurred here",
+                    },
+                ],
+            ),
+            tags: None,
+            data: None,
+        },
+        fixes: [],
+    },
+    MappedRustDiagnostic {
+        url: Url {
+            scheme: "file",
+            username: "",
+            password: None,
+            host: None,
+            port: None,
+            path: "/test/crates/hir_def/src/path.rs",
+            query: None,
+            fragment: None,
+        },
+        diagnostic: Diagnostic {
+            range: Range {
+                start: Position {
+                    line: 264,
+                    character: 8,
+                },
+                end: Position {
+                    line: 264,
+                    character: 76,
+                },
+            },
+            severity: Some(
                 Error,
             ),
             code: None,
@@ -95,7 +160,60 @@
                 "rustc",
             ),
             message: "Please register your known path in the path module",
-            related_information: None,
+            related_information: Some(
+                [
+                    DiagnosticRelatedInformation {
+                        location: Location {
+                            uri: Url {
+                                scheme: "file",
+                                username: "",
+                                password: None,
+                                host: None,
+                                port: None,
+                                path: "/test/crates/hir_def/src/path.rs",
+                                query: None,
+                                fragment: None,
+                            },
+                            range: Range {
+                                start: Position {
+                                    line: 271,
+                                    character: 8,
+                                },
+                                end: Position {
+                                    line: 271,
+                                    character: 50,
+                                },
+                            },
+                        },
+                        message: "Error originated from macro call here",
+                    },
+                    DiagnosticRelatedInformation {
+                        location: Location {
+                            uri: Url {
+                                scheme: "file",
+                                username: "",
+                                password: None,
+                                host: None,
+                                port: None,
+                                path: "/test/crates/hir_def/src/data.rs",
+                                query: None,
+                                fragment: None,
+                            },
+                            range: Range {
+                                start: Position {
+                                    line: 79,
+                                    character: 15,
+                                },
+                                end: Position {
+                                    line: 79,
+                                    character: 41,
+                                },
+                            },
+                        },
+                        message: "Error originated from macro call here",
+                    },
+                ],
+            ),
             tags: None,
             data: None,
         },
diff --git a/crates/rust-analyzer/src/diagnostics/to_proto.rs b/crates/rust-analyzer/src/diagnostics/to_proto.rs
index 76994de71..e2f319f6b 100644
--- a/crates/rust-analyzer/src/diagnostics/to_proto.rs
+++ b/crates/rust-analyzer/src/diagnostics/to_proto.rs
@@ -34,22 +34,14 @@ fn diagnostic_severity(
     Some(res)
 }
 
-/// Check whether a file name is from macro invocation
-fn is_from_macro(file_name: &str) -> bool {
+/// Checks whether a file name is from macro invocation and does not refer to an actual file.
+fn is_dummy_macro_file(file_name: &str) -> bool {
+    // FIXME: current rustc does not seem to emit `<macro file>` files anymore?
     file_name.starts_with('<') && file_name.ends_with('>')
 }
 
-/// Converts a Rust span to a LSP location, resolving macro expansion site if neccesary
-fn location(workspace_root: &Path, span: &DiagnosticSpan) -> lsp_types::Location {
-    let mut span = span.clone();
-    while let Some(expansion) = span.expansion {
-        span = expansion.span;
-    }
-    return location_naive(workspace_root, &span);
-}
-
 /// Converts a Rust span to a LSP location
-fn location_naive(workspace_root: &Path, span: &DiagnosticSpan) -> lsp_types::Location {
+fn location(workspace_root: &Path, span: &DiagnosticSpan) -> lsp_types::Location {
     let file_name = workspace_root.join(&span.file_name);
     let uri = url_from_abs_path(&file_name);
 
@@ -62,7 +54,25 @@ fn location_naive(workspace_root: &Path, span: &DiagnosticSpan) -> lsp_types::Lo
     lsp_types::Location { uri, range }
 }
 
-/// Converts a secondary Rust span to a LSP related inflocation(ormation
+/// Extracts a suitable "primary" location from a rustc diagnostic.
+///
+/// This takes locations pointing into the standard library, or generally outside the current
+/// workspace into account and tries to avoid those, in case macros are involved.
+fn primary_location(workspace_root: &Path, span: &DiagnosticSpan) -> lsp_types::Location {
+    let span_stack = std::iter::successors(Some(span), |span| Some(&span.expansion.as_ref()?.span));
+    for span in span_stack.clone() {
+        let abs_path = workspace_root.join(&span.file_name);
+        if !is_dummy_macro_file(&span.file_name) && abs_path.starts_with(workspace_root) {
+            return location(workspace_root, span);
+        }
+    }
+
+    // Fall back to the outermost macro invocation if no suitable span comes up.
+    let last_span = span_stack.last().unwrap();
+    location(workspace_root, last_span)
+}
+
+/// Converts a secondary Rust span to a LSP related information
 ///
 /// If the span is unlabelled this will return `None`.
 fn diagnostic_related_information(
@@ -231,7 +241,7 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
     primary_spans
         .iter()
         .flat_map(|primary_span| {
-            let location = location(workspace_root, &primary_span);
+            let primary_location = primary_location(workspace_root, &primary_span);
 
             let mut message = message.clone();
             if needs_primary_span_label {
@@ -243,31 +253,47 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
             // Each primary diagnostic span may result in multiple LSP diagnostics.
             let mut diagnostics = Vec::new();
 
-            let mut related_macro_info = None;
+            let mut related_info_macro_calls = vec![];
 
             // If error occurs from macro expansion, add related info pointing to
             // where the error originated
             // Also, we would generate an additional diagnostic, so that exact place of macro
             // will be highlighted in the error origin place.
-            if !is_from_macro(&primary_span.file_name) && primary_span.expansion.is_some() {
-                let in_macro_location = location_naive(workspace_root, &primary_span);
+            let span_stack = std::iter::successors(Some(*primary_span), |span| {
+                Some(&span.expansion.as_ref()?.span)
+            });
+            for (i, span) in span_stack.enumerate() {
+                if is_dummy_macro_file(&span.file_name) {
+                    continue;
+                }
 
-                // Add related information for the main disagnostic.
-                related_macro_info = Some(lsp_types::DiagnosticRelatedInformation {
-                    location: in_macro_location.clone(),
-                    message: "Error originated from macro here".to_string(),
-                });
+                // First span is the original diagnostic, others are macro call locations that
+                // generated that code.
+                let is_in_macro_call = i != 0;
 
+                let secondary_location = location(workspace_root, &span);
+                if secondary_location == primary_location {
+                    continue;
+                }
+                related_info_macro_calls.push(lsp_types::DiagnosticRelatedInformation {
+                    location: secondary_location.clone(),
+                    message: if is_in_macro_call {
+                        "Error originated from macro call here".to_string()
+                    } else {
+                        "Actual error occurred here".to_string()
+                    },
+                });
                 // For the additional in-macro diagnostic we add the inverse message pointing to the error location in code.
                 let information_for_additional_diagnostic =
                     vec![lsp_types::DiagnosticRelatedInformation {
-                        location: location.clone(),
+                        location: primary_location.clone(),
                         message: "Exact error occurred here".to_string(),
                     }];
 
                 let diagnostic = lsp_types::Diagnostic {
-                    range: in_macro_location.range,
-                    severity,
+                    range: secondary_location.range,
+                    // downgrade to hint if we're pointing at the macro
+                    severity: Some(lsp_types::DiagnosticSeverity::Hint),
                     code: code.clone().map(lsp_types::NumberOrString::String),
                     code_description: code_description.clone(),
                     source: Some(source.clone()),
@@ -276,9 +302,8 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
                     tags: if tags.is_empty() { None } else { Some(tags.clone()) },
                     data: None,
                 };
-
                 diagnostics.push(MappedRustDiagnostic {
-                    url: in_macro_location.uri,
+                    url: secondary_location.uri,
                     diagnostic,
                     fixes: Vec::new(),
                 });
@@ -286,23 +311,25 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
 
             // Emit the primary diagnostic.
             diagnostics.push(MappedRustDiagnostic {
-                url: location.uri.clone(),
+                url: primary_location.uri.clone(),
                 diagnostic: lsp_types::Diagnostic {
-                    range: location.range,
+                    range: primary_location.range,
                     severity,
                     code: code.clone().map(lsp_types::NumberOrString::String),
                     code_description: code_description.clone(),
                     source: Some(source.clone()),
                     message,
-                    related_information: if subdiagnostics.is_empty() {
-                        None
-                    } else {
-                        let mut related = subdiagnostics
+                    related_information: {
+                        let info = related_info_macro_calls
                             .iter()
-                            .map(|sub| sub.related.clone())
+                            .cloned()
+                            .chain(subdiagnostics.iter().map(|sub| sub.related.clone()))
                             .collect::<Vec<_>>();
-                        related.extend(related_macro_info);
-                        Some(related)
+                        if info.is_empty() {
+                            None
+                        } else {
+                            Some(info)
+                        }
                     },
                     tags: if tags.is_empty() { None } else { Some(tags.clone()) },
                     data: None,
@@ -314,7 +341,7 @@ pub(crate) fn map_rust_diagnostic_to_lsp(
             // This is useful because they will show up in the user's editor, unlike
             // `related_information`, which just produces hard-to-read links, at least in VS Code.
             let back_ref = lsp_types::DiagnosticRelatedInformation {
-                location,
+                location: primary_location,
                 message: "original diagnostic".to_string(),
             };
             for sub in &subdiagnostics {
diff --git a/docs/dev/style.md b/docs/dev/style.md
index e4a1672ca..c594946be 100644
--- a/docs/dev/style.md
+++ b/docs/dev/style.md
@@ -55,9 +55,9 @@ https://www.tedinski.com/2018/02/06/system-boundaries.html
 We try to be very conservative with usage of crates.io dependencies.
 Don't use small "helper" crates (exception: `itertools` is allowed).
 If there's some general reusable bit of code you need, consider adding it to the `stdx` crate.
+A useful exercise is to read Cargo.lock and see if some of the *transitive* dependencies do not make sense for rust-analyzer.
 
-**Rationale:** keep compile times low, create ecosystem pressure for faster
-compiles, reduce the number of things which might break.
+**Rationale:** keep compile times low, create ecosystem pressure for faster compiles, reduce the number of things which might break.
 
 ## Commit Style
 
@@ -806,9 +806,48 @@ if let Some(expected_type) = ctx.expected_type.as_ref() {
 }
 ```
 
-**Rational:** `match` is almost always more compact.
+**Rationale:** `match` is almost always more compact.
 The `else` branch can get a more precise pattern: `None` or `Err(_)` instead of `_`.
 
+## Helper Functions
+
+Avoid creating singe-use helper functions:
+
+```rust
+// GOOD
+let buf = {
+    let mut buf = get_empty_buf(&mut arena);
+    buf.add_item(item);
+    buf
+};
+
+// BAD
+
+let buf = prepare_buf(&mut arena, item);
+
+...
+
+fn prepare_buf(arena: &mut Arena, item: Item) -> ItemBuf {
+    let mut res = get_empty_buf(&mut arena);
+    res.add_item(item);
+    res
+}
+```
+
+Exception: if you want to make use of `return` or `?`.
+
+**Rationale:** single-use functions change frequently, adding or removing parameters adds churn.
+A block serves just as well to delineate a bit of logic, but has access to all the context.
+Re-using originally single-purpose function often leads to bad coupling.
+
+## Helper Variables
+
+Introduce helper variables freely, especially for multiline conditions.
+
+**Rationale:** like blocks, single-use variables are a cognitively cheap abstraction, as they have access to all the context.
+Extra variables help during debugging, they make it easy to print/view important intermediate results.
+Giving a name to a condition in `if` expression often improves clarity and leads to a nicer formatted code.
+
 ## Token names
 
 Use `T![foo]` instead of `SyntaxKind::FOO_KW`.