Merge #2162

2162: Move diagnostics to hir_expand r=matklad a=matklad



Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/ra_hir/src/code_model.rs

@@ -11,14 +11,16 @@ use hir_def::{
     type_ref::{Mutability, TypeRef},
     CrateModuleId, LocalEnumVariantId, LocalStructFieldId, ModuleId,
 };
-use hir_expand::name::{self, AsName};
+use hir_expand::{
+    diagnostics::DiagnosticSink,
+    name::{self, AsName},
+};
 use ra_db::{CrateId, Edition};
 use ra_syntax::ast::{self, NameOwner, TypeAscriptionOwner};
 
 use crate::{
     adt::VariantDef,
     db::{AstDatabase, DefDatabase, HirDatabase},
-    diagnostics::DiagnosticSink,
     expr::{validation::ExprValidator, Body, BodySourceMap},
     generics::HasGenericParams,
     ids::{

crates/ra_hir/src/code_model/src.rs

@@ -10,7 +10,7 @@ use crate::{
     ModuleSource, Static, Struct, StructField, Trait, TypeAlias, Union,
 };
 
-pub use hir_def::Source;
+pub use hir_expand::Source;
 
 pub trait HasSource {
     type Ast;

crates/ra_hir/src/diagnostics.rs

@@ -1,82 +1,13 @@
 //! FIXME: write short doc here
 
-use std::{any::Any, fmt};
+use std::any::Any;
 
-use ra_syntax::{ast, AstNode, AstPtr, SyntaxNode, SyntaxNodePtr, TextRange};
+use ra_syntax::{ast, AstNode, AstPtr, SyntaxNodePtr};
 use relative_path::RelativePathBuf;
 
-use crate::{db::HirDatabase, HirFileId, Name, Source};
+use crate::{db::AstDatabase, HirFileId, Name, Source};
 
-/// Diagnostic defines hir API for errors and warnings.
-///
-/// It is used as a `dyn` object, which you can downcast to a concrete
-/// diagnostic. DiagnosticSink are structured, meaning that they include rich
-/// information which can be used by IDE to create fixes. DiagnosticSink are
-/// expressed in terms of macro-expanded syntax tree nodes (so, it's a bad idea
-/// to diagnostic in a salsa value).
-///
-/// Internally, various subsystems of hir produce diagnostics specific to a
-/// subsystem (typically, an `enum`), which are safe to store in salsa but do not
-/// include source locations. Such internal diagnostic are transformed into an
-/// instance of `Diagnostic` on demand.
-pub trait Diagnostic: Any + Send + Sync + fmt::Debug + 'static {
-    fn message(&self) -> String;
-    fn source(&self) -> Source<SyntaxNodePtr>;
-    fn highlight_range(&self) -> TextRange {
-        self.source().ast.range()
-    }
-    fn as_any(&self) -> &(dyn Any + Send + 'static);
-}
-
-pub trait AstDiagnostic {
-    type AST;
-    fn ast(&self, db: &impl HirDatabase) -> Self::AST;
-}
-
-impl dyn Diagnostic {
-    pub fn syntax_node(&self, db: &impl HirDatabase) -> SyntaxNode {
-        let node = db.parse_or_expand(self.source().file_id).unwrap();
-        self.source().ast.to_node(&node)
-    }
-
-    pub fn downcast_ref<D: Diagnostic>(&self) -> Option<&D> {
-        self.as_any().downcast_ref()
-    }
-}
-
-pub struct DiagnosticSink<'a> {
-    callbacks: Vec<Box<dyn FnMut(&dyn Diagnostic) -> Result<(), ()> + 'a>>,
-    default_callback: Box<dyn FnMut(&dyn Diagnostic) + 'a>,
-}
-
-impl<'a> DiagnosticSink<'a> {
-    pub fn new(cb: impl FnMut(&dyn Diagnostic) + 'a) -> DiagnosticSink<'a> {
-        DiagnosticSink { callbacks: Vec::new(), default_callback: Box::new(cb) }
-    }
-
-    pub fn on<D: Diagnostic, F: FnMut(&D) + 'a>(mut self, mut cb: F) -> DiagnosticSink<'a> {
-        let cb = move |diag: &dyn Diagnostic| match diag.downcast_ref::<D>() {
-            Some(d) => {
-                cb(d);
-                Ok(())
-            }
-            None => Err(()),
-        };
-        self.callbacks.push(Box::new(cb));
-        self
-    }
-
-    pub(crate) fn push(&mut self, d: impl Diagnostic) {
-        let d: &dyn Diagnostic = &d;
-        for cb in self.callbacks.iter_mut() {
-            match cb(d) {
-                Ok(()) => return,
-                Err(()) => (),
-            }
-        }
-        (self.default_callback)(d)
-    }
-}
+pub use hir_expand::diagnostics::{AstDiagnostic, Diagnostic, DiagnosticSink};
 
 #[derive(Debug)]
 pub struct NoSuchField {
@@ -139,7 +70,7 @@ impl Diagnostic for MissingFields {
 impl AstDiagnostic for MissingFields {
     type AST = ast::RecordFieldList;
 
-    fn ast(&self, db: &impl HirDatabase) -> Self::AST {
+    fn ast(&self, db: &impl AstDatabase) -> Self::AST {
         let root = db.parse_or_expand(self.source().file_id).unwrap();
         let node = self.source().ast.to_node(&root);
         ast::RecordFieldList::cast(node).unwrap()
@@ -167,7 +98,7 @@ impl Diagnostic for MissingOkInTailExpr {
 impl AstDiagnostic for MissingOkInTailExpr {
     type AST = ast::Expr;
 
-    fn ast(&self, db: &impl HirDatabase) -> Self::AST {
+    fn ast(&self, db: &impl AstDatabase) -> Self::AST {
         let root = db.parse_or_expand(self.file).unwrap();
         let node = self.source().ast.to_node(&root);
         ast::Expr::cast(node).unwrap()

crates/ra_hir/src/expr/validation.rs

@@ -3,12 +3,13 @@
 use std::sync::Arc;
 
 use hir_def::path::known;
+use hir_expand::diagnostics::DiagnosticSink;
 use ra_syntax::ast;
 use rustc_hash::FxHashSet;
 
 use crate::{
     db::HirDatabase,
-    diagnostics::{DiagnosticSink, MissingFields, MissingOkInTailExpr},
+    diagnostics::{MissingFields, MissingOkInTailExpr},
     expr::AstPtr,
     ty::{ApplicationTy, InferenceResult, Ty, TypeCtor},
     Adt, Function, Name, Path,

crates/ra_hir/src/lib.rs

@@ -62,7 +62,7 @@ pub use crate::{
     adt::VariantDef,
     code_model::{
         docs::{DocDef, Docs, Documentation},
-        src::{HasBodySource, HasSource, Source},
+        src::{HasBodySource, HasSource},
         Adt, AssocItem, Const, ConstData, Container, Crate, CrateDependency, DefWithBody, Enum,
         EnumVariant, FieldSource, FnData, Function, HasBody, MacroDef, Module, ModuleDef,
         ModuleSource, Static, Struct, StructField, Trait, TypeAlias, Union,
@@ -85,4 +85,4 @@ pub use hir_def::{
     path::{Path, PathKind},
     type_ref::Mutability,
 };
-pub use hir_expand::{either::Either, name::Name};
+pub use hir_expand::{either::Either, name::Name, Source};

crates/ra_hir/src/mock.rs

@@ -2,6 +2,7 @@
 
 use std::{panic, sync::Arc};
 
+use hir_expand::diagnostics::DiagnosticSink;
 use parking_lot::Mutex;
 use ra_cfg::CfgOptions;
 use ra_db::{
@@ -12,7 +13,7 @@ use relative_path::{RelativePath, RelativePathBuf};
 use rustc_hash::FxHashMap;
 use test_utils::{extract_offset, parse_fixture, CURSOR_MARKER};
 
-use crate::{db, debug::HirDebugHelper, diagnostics::DiagnosticSink};
+use crate::{db, debug::HirDebugHelper};
 
 pub const WORKSPACE: SourceRootId = SourceRootId(0);
 

crates/ra_hir/src/nameres.rs

@@ -55,6 +55,7 @@ mod tests;
 use std::sync::Arc;
 
 use hir_def::{builtin_type::BuiltinType, CrateModuleId};
+use hir_expand::diagnostics::DiagnosticSink;
 use once_cell::sync::Lazy;
 use ra_arena::Arena;
 use ra_db::{Edition, FileId};
@@ -65,7 +66,6 @@ use test_utils::tested_by;
 
 use crate::{
     db::{AstDatabase, DefDatabase},
-    diagnostics::DiagnosticSink,
     ids::MacroDefId,
     nameres::diagnostics::DefDiagnostic,
     Adt, AstId, Crate, HirFileId, MacroDef, Module, ModuleDef, Name, Path, PathKind, Trait,
@@ -513,12 +513,13 @@ impl CrateDefMap {
 }
 
 mod diagnostics {
+    use hir_expand::diagnostics::DiagnosticSink;
     use ra_syntax::{ast, AstPtr};
     use relative_path::RelativePathBuf;
 
     use crate::{
         db::{AstDatabase, DefDatabase},
-        diagnostics::{DiagnosticSink, UnresolvedModule},
+        diagnostics::UnresolvedModule,
         nameres::CrateModuleId,
         AstId,
     };

crates/ra_hir/src/ty/infer.rs

@@ -25,7 +25,7 @@ use hir_def::{
     path::known,
     type_ref::{Mutability, TypeRef},
 };
-use hir_expand::name;
+use hir_expand::{diagnostics::DiagnosticSink, name};
 use ra_arena::map::ArenaMap;
 use ra_prof::profile;
 use test_utils::tested_by;
@@ -40,7 +40,6 @@ use crate::{
     adt::VariantDef,
     code_model::TypeAlias,
     db::HirDatabase,
-    diagnostics::DiagnosticSink,
     expr::{BindingAnnotation, Body, ExprId, PatId},
     resolve::{Resolver, TypeNs},
     ty::infer::diagnostics::InferenceDiagnostic,
@@ -719,12 +718,9 @@ impl Expectation {
 }
 
 mod diagnostics {
-    use crate::{
-        db::HirDatabase,
-        diagnostics::{DiagnosticSink, NoSuchField},
-        expr::ExprId,
-        Function, HasSource,
-    };
+    use hir_expand::diagnostics::DiagnosticSink;
+
+    use crate::{db::HirDatabase, diagnostics::NoSuchField, expr::ExprId, Function, HasSource};
 
     #[derive(Debug, PartialEq, Eq, Clone)]
     pub(super) enum InferenceDiagnostic {

crates/ra_hir_def/src/lib.rs

@@ -19,19 +19,13 @@ pub mod nameres;
 
 use std::hash::{Hash, Hasher};
 
-use hir_expand::{ast_id_map::FileAstId, db::AstDatabase, AstId, HirFileId};
+use hir_expand::{ast_id_map::FileAstId, db::AstDatabase, AstId, HirFileId, Source};
 use ra_arena::{impl_arena_id, RawId};
 use ra_db::{salsa, CrateId, FileId};
 use ra_syntax::{ast, AstNode, SyntaxNode};
 
 use crate::{builtin_type::BuiltinType, db::InternDatabase};
 
-#[derive(Debug, PartialEq, Eq, Clone, Copy)]
-pub struct Source<T> {
-    pub file_id: HirFileId,
-    pub ast: T,
-}
-
 pub enum ModuleSource {
     SourceFile(ast::SourceFile),
     Module(ast::Module),
@@ -94,15 +88,6 @@ impl ModuleSource {
     }
 }
 
-impl<T> Source<T> {
-    pub fn map<F: FnOnce(T) -> U, U>(self, f: F) -> Source<U> {
-        Source { file_id: self.file_id, ast: f(self.ast) }
-    }
-    pub fn file_syntax(&self, db: &impl AstDatabase) -> SyntaxNode {
-        db.parse_or_expand(self.file_id).expect("source created from invalid file")
-    }
-}
-
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct ModuleId {
     pub krate: CrateId,

crates/ra_hir_expand/src/diagnostics.rs

@@ -0,0 +1,85 @@
+//! Semantic errors and warnings.
+//!
+//! The `Diagnostic` trait defines a trait object which can represent any
+//! diagnostic.
+//!
+//! `DiagnosticSink` struct is used as an emitter for diagnostic. When creating
+//! a `DiagnosticSink`, you supply a callback which can react to a `dyn
+//! Diagnostic` or to any concrete diagnostic (downcasting is sued internally).
+//!
+//! Because diagnostics store file offsets, it's a bad idea to store them
+//! directly in salsa. For this reason, every hir subsytem defines it's own
+//! strongly-typed closed set of diagnostics which use hir ids internally, are
+//! stored in salsa and do *not* implement the `Diagnostic` trait. Instead, a
+//! subsystem provides a separate, non-query-based API which can walk all stored
+//! values and transform them into instances of `Diagnostic`.
+
+use std::{any::Any, fmt};
+
+use ra_syntax::{SyntaxNode, SyntaxNodePtr, TextRange};
+
+use crate::{db::AstDatabase, Source};
+
+pub trait Diagnostic: Any + Send + Sync + fmt::Debug + 'static {
+    fn message(&self) -> String;
+    fn source(&self) -> Source<SyntaxNodePtr>;
+    fn highlight_range(&self) -> TextRange {
+        self.source().ast.range()
+    }
+    fn as_any(&self) -> &(dyn Any + Send + 'static);
+}
+
+pub trait AstDiagnostic {
+    type AST;
+    fn ast(&self, db: &impl AstDatabase) -> Self::AST;
+}
+
+impl dyn Diagnostic {
+    pub fn syntax_node(&self, db: &impl AstDatabase) -> SyntaxNode {
+        let node = db.parse_or_expand(self.source().file_id).unwrap();
+        self.source().ast.to_node(&node)
+    }
+
+    pub fn downcast_ref<D: Diagnostic>(&self) -> Option<&D> {
+        self.as_any().downcast_ref()
+    }
+}
+
+pub struct DiagnosticSink<'a> {
+    callbacks: Vec<Box<dyn FnMut(&dyn Diagnostic) -> Result<(), ()> + 'a>>,
+    default_callback: Box<dyn FnMut(&dyn Diagnostic) + 'a>,
+}
+
+impl<'a> DiagnosticSink<'a> {
+    /// FIXME: split `new` and `on` into a separate builder type
+    pub fn new(cb: impl FnMut(&dyn Diagnostic) + 'a) -> DiagnosticSink<'a> {
+        DiagnosticSink { callbacks: Vec::new(), default_callback: Box::new(cb) }
+    }
+
+    pub fn on<D: Diagnostic, F: FnMut(&D) + 'a>(mut self, mut cb: F) -> DiagnosticSink<'a> {
+        let cb = move |diag: &dyn Diagnostic| match diag.downcast_ref::<D>() {
+            Some(d) => {
+                cb(d);
+                Ok(())
+            }
+            None => Err(()),
+        };
+        self.callbacks.push(Box::new(cb));
+        self
+    }
+
+    pub fn push(&mut self, d: impl Diagnostic) {
+        let d: &dyn Diagnostic = &d;
+        self._push(d);
+    }
+
+    fn _push(&mut self, d: &dyn Diagnostic) {
+        for cb in self.callbacks.iter_mut() {
+            match cb(d) {
+                Ok(()) => return,
+                Err(()) => (),
+            }
+        }
+        (self.default_callback)(d)
+    }
+}

crates/ra_hir_expand/src/lib.rs

@@ -9,11 +9,15 @@ pub mod ast_id_map;
 pub mod either;
 pub mod name;
 pub mod hygiene;
+pub mod diagnostics;
 
 use std::hash::{Hash, Hasher};
 
 use ra_db::{salsa, CrateId, FileId};
-use ra_syntax::ast::{self, AstNode};
+use ra_syntax::{
+    ast::{self, AstNode},
+    SyntaxNode,
+};
 
 use crate::ast_id_map::FileAstId;
 
@@ -151,3 +155,18 @@ impl<N: AstNode> AstId<N> {
         db.ast_id_map(self.file_id).get(self.file_ast_id).to_node(&root)
     }
 }
+
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+pub struct Source<T> {
+    pub file_id: HirFileId,
+    pub ast: T,
+}
+
+impl<T> Source<T> {
+    pub fn map<F: FnOnce(T) -> U, U>(self, f: F) -> Source<U> {
+        Source { file_id: self.file_id, ast: f(self.ast) }
+    }
+    pub fn file_syntax(&self, db: &impl db::AstDatabase) -> SyntaxNode {
+        db.parse_or_expand(self.file_id).expect("source created from invalid file")
+    }
+}