Merge #5553

5553: Add fix ranges for diagnostics r=matklad a=SomeoneToIgnore

A follow-up of https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/Less.20red.20in.20the.20code

Now diagnostics can apply fixes in a range that's different from the range used to highlight the diagnostics.
Previous logic did not consider the fix range, having both ranges equal, which could cause a lot of red noise in the editor.
Now, the fix range gets used with the fix, the diagnostics range is used for everything else which allows to improve the error highlighting.

before:
<img width="191" alt="image" src="https://user-images.githubusercontent.com/2690773/88590727-df9a6a00-d063-11ea-97ed-9809c1c5e6e6.png">
after:
<img width="222" alt="image" src="https://user-images.githubusercontent.com/2690773/88590734-e1fcc400-d063-11ea-9b7c-25701cbd5352.png">

`MissingFields` and `MissingPatFields` diagnostics now have the fix range as `ast::RecordFieldList` of the expression with an error (as it was before this PR), and the diagnostics range as a `ast::Path` of the expression, if it's present (do you have any example of `ast::Expr::RecordLit` that has no path btw?).
The rest of the diagnostics have both ranges equal, same as it was before this PR.

Co-authored-by: Kirill Bulatov <[email protected]>

3 files changed, 59 insertions, 91 deletions

diff --git a/crates/ra_hir_ty/src/diagnostics.rs b/crates/ra_hir_ty/src/diagnostics.rs
index 977c0525b..7ab7f79db 100644
--- a/crates/ra_hir_ty/src/diagnostics.rs
+++ b/crates/ra_hir_ty/src/diagnostics.rs
@@ -6,10 +6,10 @@ mod unsafe_check;
 use std::any::Any;
 
 use hir_def::DefWithBodyId;
-use hir_expand::diagnostics::{AstDiagnostic, Diagnostic, DiagnosticSink};
-use hir_expand::{db::AstDatabase, name::Name, HirFileId, InFile};
+use hir_expand::diagnostics::{Diagnostic, DiagnosticSink};
+use hir_expand::{name::Name, HirFileId, InFile};
 use ra_prof::profile;
-use ra_syntax::{ast, AstNode, AstPtr, SyntaxNodePtr};
+use ra_syntax::{ast, AstPtr, SyntaxNodePtr};
 use stdx::format_to;
 
 use crate::db::HirDatabase;
@@ -37,7 +37,7 @@ impl Diagnostic for NoSuchField {
         "no such field".to_string()
     }
 
-    fn source(&self) -> InFile<SyntaxNodePtr> {
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
         InFile::new(self.file, self.field.clone().into())
     }
 
@@ -46,20 +46,11 @@ impl Diagnostic for NoSuchField {
     }
 }
 
-impl AstDiagnostic for NoSuchField {
-    type AST = ast::RecordExprField;
-
-    fn ast(&self, db: &dyn AstDatabase) -> Self::AST {
-        let root = db.parse_or_expand(self.source().file_id).unwrap();
-        let node = self.source().value.to_node(&root);
-        ast::RecordExprField::cast(node).unwrap()
-    }
-}
-
 #[derive(Debug)]
 pub struct MissingFields {
     pub file: HirFileId,
-    pub field_list: AstPtr<ast::RecordExprFieldList>,
+    pub field_list_parent: AstPtr<ast::RecordExpr>,
+    pub field_list_parent_path: Option<AstPtr<ast::Path>>,
     pub missed_fields: Vec<Name>,
 }
 
@@ -71,28 +62,28 @@ impl Diagnostic for MissingFields {
         }
         buf
     }
-    fn source(&self) -> InFile<SyntaxNodePtr> {
-        InFile { file_id: self.file, value: self.field_list.clone().into() }
+
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
+        InFile {
+            file_id: self.file,
+            value: self
+                .field_list_parent_path
+                .clone()
+                .map(SyntaxNodePtr::from)
+                .unwrap_or_else(|| self.field_list_parent.clone().into()),
+        }
     }
+
     fn as_any(&self) -> &(dyn Any + Send + 'static) {
         self
     }
 }
 
-impl AstDiagnostic for MissingFields {
-    type AST = ast::RecordExprFieldList;
-
-    fn ast(&self, db: &dyn AstDatabase) -> Self::AST {
-        let root = db.parse_or_expand(self.source().file_id).unwrap();
-        let node = self.source().value.to_node(&root);
-        ast::RecordExprFieldList::cast(node).unwrap()
-    }
-}
-
 #[derive(Debug)]
 pub struct MissingPatFields {
     pub file: HirFileId,
-    pub field_list: AstPtr<ast::RecordPatFieldList>,
+    pub field_list_parent: AstPtr<ast::RecordPat>,
+    pub field_list_parent_path: Option<AstPtr<ast::Path>>,
     pub missed_fields: Vec<Name>,
 }
 
@@ -104,8 +95,15 @@ impl Diagnostic for MissingPatFields {
         }
         buf
     }
-    fn source(&self) -> InFile<SyntaxNodePtr> {
-        InFile { file_id: self.file, value: self.field_list.clone().into() }
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
+        InFile {
+            file_id: self.file,
+            value: self
+                .field_list_parent_path
+                .clone()
+                .map(SyntaxNodePtr::from)
+                .unwrap_or_else(|| self.field_list_parent.clone().into()),
+        }
     }
     fn as_any(&self) -> &(dyn Any + Send + 'static) {
         self
@@ -123,7 +121,7 @@ impl Diagnostic for MissingMatchArms {
     fn message(&self) -> String {
         String::from("Missing match arm")
     }
-    fn source(&self) -> InFile<SyntaxNodePtr> {
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
         InFile { file_id: self.file, value: self.match_expr.clone().into() }
     }
     fn as_any(&self) -> &(dyn Any + Send + 'static) {
@@ -141,7 +139,7 @@ impl Diagnostic for MissingOkInTailExpr {
     fn message(&self) -> String {
         "wrap return expression in Ok".to_string()
     }
-    fn source(&self) -> InFile<SyntaxNodePtr> {
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
         InFile { file_id: self.file, value: self.expr.clone().into() }
     }
     fn as_any(&self) -> &(dyn Any + Send + 'static) {
@@ -149,16 +147,6 @@ impl Diagnostic for MissingOkInTailExpr {
     }
 }
 
-impl AstDiagnostic for MissingOkInTailExpr {
-    type AST = ast::Expr;
-
-    fn ast(&self, db: &dyn AstDatabase) -> Self::AST {
-        let root = db.parse_or_expand(self.file).unwrap();
-        let node = self.source().value.to_node(&root);
-        ast::Expr::cast(node).unwrap()
-    }
-}
-
 #[derive(Debug)]
 pub struct BreakOutsideOfLoop {
     pub file: HirFileId,
@@ -169,7 +157,7 @@ impl Diagnostic for BreakOutsideOfLoop {
     fn message(&self) -> String {
         "break outside of loop".to_string()
     }
-    fn source(&self) -> InFile<SyntaxNodePtr> {
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
         InFile { file_id: self.file, value: self.expr.clone().into() }
     }
     fn as_any(&self) -> &(dyn Any + Send + 'static) {
@@ -177,16 +165,6 @@ impl Diagnostic for BreakOutsideOfLoop {
     }
 }
 
-impl AstDiagnostic for BreakOutsideOfLoop {
-    type AST = ast::Expr;
-
-    fn ast(&self, db: &dyn AstDatabase) -> Self::AST {
-        let root = db.parse_or_expand(self.file).unwrap();
-        let node = self.source().value.to_node(&root);
-        ast::Expr::cast(node).unwrap()
-    }
-}
-
 #[derive(Debug)]
 pub struct MissingUnsafe {
     pub file: HirFileId,
@@ -197,7 +175,7 @@ impl Diagnostic for MissingUnsafe {
     fn message(&self) -> String {
         format!("This operation is unsafe and requires an unsafe function or block")
     }
-    fn source(&self) -> InFile<SyntaxNodePtr> {
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
         InFile { file_id: self.file, value: self.expr.clone().into() }
     }
     fn as_any(&self) -> &(dyn Any + Send + 'static) {
@@ -205,16 +183,6 @@ impl Diagnostic for MissingUnsafe {
     }
 }
 
-impl AstDiagnostic for MissingUnsafe {
-    type AST = ast::Expr;
-
-    fn ast(&self, db: &dyn AstDatabase) -> Self::AST {
-        let root = db.parse_or_expand(self.source().file_id).unwrap();
-        let node = self.source().value.to_node(&root);
-        ast::Expr::cast(node).unwrap()
-    }
-}
-
 #[derive(Debug)]
 pub struct MismatchedArgCount {
     pub file: HirFileId,
@@ -228,7 +196,7 @@ impl Diagnostic for MismatchedArgCount {
         let s = if self.expected == 1 { "" } else { "s" };
         format!("Expected {} argument{}, found {}", self.expected, s, self.found)
     }
-    fn source(&self) -> InFile<SyntaxNodePtr> {
+    fn display_source(&self) -> InFile<SyntaxNodePtr> {
         InFile { file_id: self.file, value: self.call_expr.clone().into() }
     }
     fn as_any(&self) -> &(dyn Any + Send + 'static) {
@@ -239,19 +207,13 @@ impl Diagnostic for MismatchedArgCount {
     }
 }
 
-impl AstDiagnostic for MismatchedArgCount {
-    type AST = ast::CallExpr;
-    fn ast(&self, db: &dyn AstDatabase) -> Self::AST {
-        let root = db.parse_or_expand(self.source().file_id).unwrap();
-        let node = self.source().value.to_node(&root);
-        ast::CallExpr::cast(node).unwrap()
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use hir_def::{db::DefDatabase, AssocItemId, ModuleDefId};
-    use hir_expand::diagnostics::{Diagnostic, DiagnosticSinkBuilder};
+    use hir_expand::{
+        db::AstDatabase,
+        diagnostics::{Diagnostic, DiagnosticSinkBuilder},
+    };
     use ra_db::{fixture::WithFixture, FileId, SourceDatabase, SourceDatabaseExt};
     use ra_syntax::{TextRange, TextSize};
     use rustc_hash::FxHashMap;
@@ -296,9 +258,11 @@ mod tests {
 
         let mut actual: FxHashMap<FileId, Vec<(TextRange, String)>> = FxHashMap::default();
         db.diagnostics(|d| {
-            // FXIME: macros...
-            let file_id = d.source().file_id.original_file(&db);
-            let range = d.syntax_node(&db).text_range();
+            let src = d.display_source();
+            let root = db.parse_or_expand(src.file_id).unwrap();
+            // FIXME: macros...
+            let file_id = src.file_id.original_file(&db);
+            let range = src.value.to_node(&root).text_range();
             let message = d.message().to_owned();
             actual.entry(file_id).or_default().push((range, message));
         });
@@ -326,8 +290,8 @@ struct S { foo: i32, bar: () }
 impl S {
     fn new() -> S {
         S {
-        //^... Missing structure fields:
-        //|    - bar
+      //^ Missing structure fields:
+      //|    - bar
             foo: 92,
             baz: 62,
           //^^^^^^^ no such field
@@ -448,8 +412,8 @@ impl Foo {
 struct S { foo: i32, bar: () }
 fn baz(s: S) {
     let S { foo: _ } = s;
-        //^^^^^^^^^^ Missing structure fields:
-        //         | - bar
+      //^ Missing structure fields:
+      //| - bar
 }
 "#,
         );
diff --git a/crates/ra_hir_ty/src/diagnostics/expr.rs b/crates/ra_hir_ty/src/diagnostics/expr.rs
index 95bbf2d95..51adcecaf 100644
--- a/crates/ra_hir_ty/src/diagnostics/expr.rs
+++ b/crates/ra_hir_ty/src/diagnostics/expr.rs
@@ -100,8 +100,8 @@ impl<'a, 'b> ExprValidator<'a, 'b> {
 
         if let Ok(source_ptr) = source_map.expr_syntax(id) {
             let root = source_ptr.file_syntax(db.upcast());
-            if let ast::Expr::RecordExpr(record_lit) = &source_ptr.value.to_node(&root) {
-                if let Some(field_list) = record_lit.record_expr_field_list() {
+            if let ast::Expr::RecordExpr(record_expr) = &source_ptr.value.to_node(&root) {
+                if let Some(_) = record_expr.record_expr_field_list() {
                     let variant_data = variant_data(db.upcast(), variant_def);
                     let missed_fields = missed_fields
                         .into_iter()
@@ -109,7 +109,8 @@ impl<'a, 'b> ExprValidator<'a, 'b> {
                         .collect();
                     self.sink.push(MissingFields {
                         file: source_ptr.file_id,
-                        field_list: AstPtr::new(&field_list),
+                        field_list_parent: AstPtr::new(&record_expr),
+                        field_list_parent_path: record_expr.path().map(|path| AstPtr::new(&path)),
                         missed_fields,
                     })
                 }
@@ -131,7 +132,7 @@ impl<'a, 'b> ExprValidator<'a, 'b> {
             if let Some(expr) = source_ptr.value.as_ref().left() {
                 let root = source_ptr.file_syntax(db.upcast());
                 if let ast::Pat::RecordPat(record_pat) = expr.to_node(&root) {
-                    if let Some(field_list) = record_pat.record_pat_field_list() {
+                    if let Some(_) = record_pat.record_pat_field_list() {
                         let variant_data = variant_data(db.upcast(), variant_def);
                         let missed_fields = missed_fields
                             .into_iter()
@@ -139,7 +140,10 @@ impl<'a, 'b> ExprValidator<'a, 'b> {
                             .collect();
                         self.sink.push(MissingPatFields {
                             file: source_ptr.file_id,
-                            field_list: AstPtr::new(&field_list),
+                            field_list_parent: AstPtr::new(&record_pat),
+                            field_list_parent_path: record_pat
+                                .path()
+                                .map(|path| AstPtr::new(&path)),
                             missed_fields,
                         })
                     }
diff --git a/crates/ra_hir_ty/src/diagnostics/match_check.rs b/crates/ra_hir_ty/src/diagnostics/match_check.rs
index 507edcb7d..deca244db 100644
--- a/crates/ra_hir_ty/src/diagnostics/match_check.rs
+++ b/crates/ra_hir_ty/src/diagnostics/match_check.rs
@@ -1161,15 +1161,15 @@ fn main() {
         //^ Missing match arm
     match a {
         Either::A { } => (),
-                //^^^ Missing structure fields:
-                //  | - foo
+      //^^^^^^^^^ Missing structure fields:
+      //        | - foo
         Either::B => (),
     }
     match a {
         //^ Missing match arm
         Either::A { } => (),
-    }           //^^^ Missing structure fields:
-                //  | - foo
+    } //^^^^^^^^^ Missing structure fields:
+      //        | - foo
 
     match a {
         Either::A { foo: true } => (),