Merge #9237

9237: internal: move diagnostics infra to hir r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <[email protected]>

4 files changed, 13 insertions, 612 deletions

diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs
index f3236bc06..407273943 100644
--- a/crates/hir_ty/src/diagnostics.rs
+++ b/crates/hir_ty/src/diagnostics.rs
@@ -4,17 +4,14 @@ mod match_check;
 mod unsafe_check;
 mod decl_check;
 
-use std::{any::Any, fmt};
+use std::fmt;
 
 use base_db::CrateId;
 use hir_def::ModuleDefId;
-use hir_expand::{HirFileId, InFile};
-use syntax::{ast, AstPtr, SyntaxNodePtr};
+use hir_expand::HirFileId;
+use syntax::{ast, AstPtr};
 
-use crate::{
-    db::HirDatabase,
-    diagnostics_sink::{Diagnostic, DiagnosticCode, DiagnosticSink},
-};
+use crate::db::HirDatabase;
 
 pub use crate::diagnostics::{
     expr::{
@@ -27,11 +24,11 @@ pub fn validate_module_item(
     db: &dyn HirDatabase,
     krate: CrateId,
     owner: ModuleDefId,
-    sink: &mut DiagnosticSink<'_>,
-) {
+) -> Vec<IncorrectCase> {
     let _p = profile::span("validate_module_item");
-    let mut validator = decl_check::DeclValidator::new(db, krate, sink);
+    let mut validator = decl_check::DeclValidator::new(db, krate);
     validator.validate_item(owner);
+    validator.sink
 }
 
 #[derive(Debug)]
@@ -99,142 +96,3 @@ pub struct IncorrectCase {
     pub ident_text: String,
     pub suggested_text: String,
 }
-
-impl Diagnostic for IncorrectCase {
-    fn code(&self) -> DiagnosticCode {
-        DiagnosticCode("incorrect-ident-case")
-    }
-
-    fn message(&self) -> String {
-        format!(
-            "{} `{}` should have {} name, e.g. `{}`",
-            self.ident_type,
-            self.ident_text,
-            self.expected_case.to_string(),
-            self.suggested_text
-        )
-    }
-
-    fn display_source(&self) -> InFile<SyntaxNodePtr> {
-        InFile::new(self.file, self.ident.clone().into())
-    }
-
-    fn as_any(&self) -> &(dyn Any + Send + 'static) {
-        self
-    }
-
-    fn is_experimental(&self) -> bool {
-        true
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use base_db::{fixture::WithFixture, FileId, SourceDatabase, SourceDatabaseExt};
-    use hir_def::{db::DefDatabase, AssocItemId, ModuleDefId};
-    use hir_expand::db::AstDatabase;
-    use rustc_hash::FxHashMap;
-    use syntax::{TextRange, TextSize};
-
-    use crate::{
-        diagnostics::validate_module_item,
-        diagnostics_sink::{Diagnostic, DiagnosticSinkBuilder},
-        test_db::TestDB,
-    };
-
-    impl TestDB {
-        fn diagnostics<F: FnMut(&dyn Diagnostic)>(&self, mut cb: F) {
-            let crate_graph = self.crate_graph();
-            for krate in crate_graph.iter() {
-                let crate_def_map = self.crate_def_map(krate);
-
-                let mut fns = Vec::new();
-                for (module_id, _) in crate_def_map.modules() {
-                    for decl in crate_def_map[module_id].scope.declarations() {
-                        let mut sink = DiagnosticSinkBuilder::new().build(&mut cb);
-                        validate_module_item(self, krate, decl, &mut sink);
-
-                        if let ModuleDefId::FunctionId(f) = decl {
-                            fns.push(f)
-                        }
-                    }
-
-                    for impl_id in crate_def_map[module_id].scope.impls() {
-                        let impl_data = self.impl_data(impl_id);
-                        for item in impl_data.items.iter() {
-                            if let AssocItemId::FunctionId(f) = item {
-                                let mut sink = DiagnosticSinkBuilder::new().build(&mut cb);
-                                validate_module_item(
-                                    self,
-                                    krate,
-                                    ModuleDefId::FunctionId(*f),
-                                    &mut sink,
-                                );
-                                fns.push(*f)
-                            }
-                        }
-                    }
-                }
-            }
-        }
-    }
-
-    pub(crate) fn check_diagnostics(ra_fixture: &str) {
-        let db = TestDB::with_files(ra_fixture);
-        let annotations = db.extract_annotations();
-
-        let mut actual: FxHashMap<FileId, Vec<(TextRange, String)>> = FxHashMap::default();
-        db.diagnostics(|d| {
-            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();
-            actual.entry(file_id).or_default().push((range, message));
-        });
-
-        for (file_id, diags) in actual.iter_mut() {
-            diags.sort_by_key(|it| it.0.start());
-            let text = db.file_text(*file_id);
-            // For multiline spans, place them on line start
-            for (range, content) in diags {
-                if text[*range].contains('\n') {
-                    *range = TextRange::new(range.start(), range.start() + TextSize::from(1));
-                    *content = format!("... {}", content);
-                }
-            }
-        }
-
-        assert_eq!(annotations, actual);
-    }
-
-    #[test]
-    fn import_extern_crate_clash_with_inner_item() {
-        // This is more of a resolver test, but doesn't really work with the hir_def testsuite.
-
-        check_diagnostics(
-            r#"
-//- /lib.rs crate:lib deps:jwt
-mod permissions;
-
-use permissions::jwt;
-
-fn f() {
-    fn inner() {}
-    jwt::Claims {}; // should resolve to the local one with 0 fields, and not get a diagnostic
-}
-
-//- /permissions.rs
-pub mod jwt  {
-    pub struct Claims {}
-}
-
-//- /jwt/lib.rs crate:jwt
-pub struct Claims {
-    field: u8,
-}
-        "#,
-        );
-    }
-}
diff --git a/crates/hir_ty/src/diagnostics/decl_check.rs b/crates/hir_ty/src/diagnostics/decl_check.rs
index cfb5d7320..ca452e879 100644
--- a/crates/hir_ty/src/diagnostics/decl_check.rs
+++ b/crates/hir_ty/src/diagnostics/decl_check.rs
@@ -29,7 +29,6 @@ use syntax::{
 use crate::{
     db::HirDatabase,
     diagnostics::{decl_check::case_conv::*, CaseType, IdentType, IncorrectCase},
-    diagnostics_sink::DiagnosticSink,
 };
 
 mod allow {
@@ -40,10 +39,10 @@ mod allow {
     pub(super) const NON_CAMEL_CASE_TYPES: &str = "non_camel_case_types";
 }
 
-pub(super) struct DeclValidator<'a, 'b> {
+pub(super) struct DeclValidator<'a> {
     db: &'a dyn HirDatabase,
     krate: CrateId,
-    sink: &'a mut DiagnosticSink<'b>,
+    pub(super) sink: Vec<IncorrectCase>,
 }
 
 #[derive(Debug)]
@@ -53,13 +52,9 @@ struct Replacement {
     expected_case: CaseType,
 }
 
-impl<'a, 'b> DeclValidator<'a, 'b> {
-    pub(super) fn new(
-        db: &'a dyn HirDatabase,
-        krate: CrateId,
-        sink: &'a mut DiagnosticSink<'b>,
-    ) -> DeclValidator<'a, 'b> {
-        DeclValidator { db, krate, sink }
+impl<'a> DeclValidator<'a> {
+    pub(super) fn new(db: &'a dyn HirDatabase, krate: CrateId) -> DeclValidator<'a> {
+        DeclValidator { db, krate, sink: Vec::new() }
     }
 
     pub(super) fn validate_item(&mut self, item: ModuleDefId) {
@@ -121,7 +116,6 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
     fn validate_func(&mut self, func: FunctionId) {
         let data = self.db.function_data(func);
         if data.is_in_extern_block() {
-            cov_mark::hit!(extern_func_incorrect_case_ignored);
             return;
         }
 
@@ -131,7 +125,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
         for (_, block_def_map) in body.blocks(self.db.upcast()) {
             for (_, module) in block_def_map.modules() {
                 for def_id in module.scope.declarations() {
-                    let mut validator = DeclValidator::new(self.db, self.krate, self.sink);
+                    let mut validator = DeclValidator::new(self.db, self.krate);
                     validator.validate_item(def_id);
                 }
             }
@@ -578,7 +572,6 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
     fn validate_static(&mut self, static_id: StaticId) {
         let data = self.db.static_data(static_id);
         if data.is_extern {
-            cov_mark::hit!(extern_static_incorrect_case_ignored);
             return;
         }
 
@@ -623,343 +616,3 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
         self.sink.push(diagnostic);
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use crate::diagnostics::tests::check_diagnostics;
-
-    #[test]
-    fn incorrect_function_name() {
-        check_diagnostics(
-            r#"
-fn NonSnakeCaseName() {}
-// ^^^^^^^^^^^^^^^^ Function `NonSnakeCaseName` should have snake_case name, e.g. `non_snake_case_name`
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_function_params() {
-        check_diagnostics(
-            r#"
-fn foo(SomeParam: u8) {}
-    // ^^^^^^^^^ Parameter `SomeParam` should have snake_case name, e.g. `some_param`
-
-fn foo2(ok_param: &str, CAPS_PARAM: u8) {}
-                     // ^^^^^^^^^^ Parameter `CAPS_PARAM` should have snake_case name, e.g. `caps_param`
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_variable_names() {
-        check_diagnostics(
-            r#"
-fn foo() {
-    let SOME_VALUE = 10;
-     // ^^^^^^^^^^ Variable `SOME_VALUE` should have snake_case name, e.g. `some_value`
-    let AnotherValue = 20;
-     // ^^^^^^^^^^^^ Variable `AnotherValue` should have snake_case name, e.g. `another_value`
-}
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_struct_names() {
-        check_diagnostics(
-            r#"
-struct non_camel_case_name {}
-    // ^^^^^^^^^^^^^^^^^^^ Structure `non_camel_case_name` should have CamelCase name, e.g. `NonCamelCaseName`
-
-struct SCREAMING_CASE {}
-    // ^^^^^^^^^^^^^^ Structure `SCREAMING_CASE` should have CamelCase name, e.g. `ScreamingCase`
-"#,
-        );
-    }
-
-    #[test]
-    fn no_diagnostic_for_camel_cased_acronyms_in_struct_name() {
-        check_diagnostics(
-            r#"
-struct AABB {}
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_struct_field() {
-        check_diagnostics(
-            r#"
-struct SomeStruct { SomeField: u8 }
-                 // ^^^^^^^^^ Field `SomeField` should have snake_case name, e.g. `some_field`
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_enum_names() {
-        check_diagnostics(
-            r#"
-enum some_enum { Val(u8) }
-  // ^^^^^^^^^ Enum `some_enum` should have CamelCase name, e.g. `SomeEnum`
-
-enum SOME_ENUM
-  // ^^^^^^^^^ Enum `SOME_ENUM` should have CamelCase name, e.g. `SomeEnum`
-"#,
-        );
-    }
-
-    #[test]
-    fn no_diagnostic_for_camel_cased_acronyms_in_enum_name() {
-        check_diagnostics(
-            r#"
-enum AABB {}
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_enum_variant_name() {
-        check_diagnostics(
-            r#"
-enum SomeEnum { SOME_VARIANT(u8) }
-             // ^^^^^^^^^^^^ Variant `SOME_VARIANT` should have CamelCase name, e.g. `SomeVariant`
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_const_name() {
-        check_diagnostics(
-            r#"
-const some_weird_const: u8 = 10;
-   // ^^^^^^^^^^^^^^^^ Constant `some_weird_const` should have UPPER_SNAKE_CASE name, e.g. `SOME_WEIRD_CONST`
-
-fn func() {
-    const someConstInFunc: &str = "hi there";
-       // ^^^^^^^^^^^^^^^ Constant `someConstInFunc` should have UPPER_SNAKE_CASE name, e.g. `SOME_CONST_IN_FUNC`
-
-}
-"#,
-        );
-    }
-
-    #[test]
-    fn incorrect_static_name() {
-        check_diagnostics(
-            r#"
-static some_weird_const: u8 = 10;
-    // ^^^^^^^^^^^^^^^^ Static variable `some_weird_const` should have UPPER_SNAKE_CASE name, e.g. `SOME_WEIRD_CONST`
-
-fn func() {
-    static someConstInFunc: &str = "hi there";
-        // ^^^^^^^^^^^^^^^ Static variable `someConstInFunc` should have UPPER_SNAKE_CASE name, e.g. `SOME_CONST_IN_FUNC`
-}
-"#,
-        );
-    }
-
-    #[test]
-    fn fn_inside_impl_struct() {
-        check_diagnostics(
-            r#"
-struct someStruct;
-    // ^^^^^^^^^^ Structure `someStruct` should have CamelCase name, e.g. `SomeStruct`
-
-impl someStruct {
-    fn SomeFunc(&self) {
-    // ^^^^^^^^ Function `SomeFunc` should have snake_case name, e.g. `some_func`
-        static someConstInFunc: &str = "hi there";
-            // ^^^^^^^^^^^^^^^ Static variable `someConstInFunc` should have UPPER_SNAKE_CASE name, e.g. `SOME_CONST_IN_FUNC`
-        let WHY_VAR_IS_CAPS = 10;
-         // ^^^^^^^^^^^^^^^ Variable `WHY_VAR_IS_CAPS` should have snake_case name, e.g. `why_var_is_caps`
-    }
-}
-"#,
-        );
-    }
-
-    #[test]
-    fn no_diagnostic_for_enum_varinats() {
-        check_diagnostics(
-            r#"
-enum Option { Some, None }
-
-fn main() {
-    match Option::None {
-        None => (),
-        Some => (),
-    }
-}
-"#,
-        );
-    }
-
-    #[test]
-    fn non_let_bind() {
-        check_diagnostics(
-            r#"
-enum Option { Some, None }
-
-fn main() {
-    match Option::None {
-        SOME_VAR @ None => (),
-     // ^^^^^^^^ Variable `SOME_VAR` should have snake_case name, e.g. `some_var`
-        Some => (),
-    }
-}
-"#,
-        );
-    }
-
-    #[test]
-    fn allow_attributes() {
-        check_diagnostics(
-            r#"
-#[allow(non_snake_case)]
-fn NonSnakeCaseName(SOME_VAR: u8) -> u8{
-    // cov_flags generated output from elsewhere in this file
-    extern "C" {
-        #[no_mangle]
-        static lower_case: u8;
-    }
-
-    let OtherVar = SOME_VAR + 1;
-    OtherVar
-}
-
-#[allow(nonstandard_style)]
-mod CheckNonstandardStyle {
-    fn HiImABadFnName() {}
-}
-
-#[allow(bad_style)]
-mod CheckBadStyle {
-    fn HiImABadFnName() {}
-}
-
-mod F {
-    #![allow(non_snake_case)]
-    fn CheckItWorksWithModAttr(BAD_NAME_HI: u8) {}
-}
-
-#[allow(non_snake_case, non_camel_case_types)]
-pub struct some_type {
-    SOME_FIELD: u8,
-    SomeField: u16,
-}
-
-#[allow(non_upper_case_globals)]
-pub const some_const: u8 = 10;
-
-#[allow(non_upper_case_globals)]
-pub static SomeStatic: u8 = 10;
-    "#,
-        );
-    }
-
-    #[test]
-    fn allow_attributes_crate_attr() {
-        check_diagnostics(
-            r#"
-#![allow(non_snake_case)]
-
-mod F {
-    fn CheckItWorksWithCrateAttr(BAD_NAME_HI: u8) {}
-}
-    "#,
-        );
-    }
-
-    #[test]
-    #[ignore]
-    fn bug_trait_inside_fn() {
-        // FIXME:
-        // This is broken, and in fact, should not even be looked at by this
-        // lint in the first place. There's weird stuff going on in the
-        // collection phase.
-        // It's currently being brought in by:
-        // * validate_func on `a` recursing into modules
-        // * then it finds the trait and then the function while iterating
-        //   through modules
-        // * then validate_func is called on Dirty
-        // * ... which then proceeds to look at some unknown module taking no
-        //   attrs from either the impl or the fn a, and then finally to the root
-        //   module
-        //
-        // It should find the attribute on the trait, but it *doesn't even see
-        // the trait* as far as I can tell.
-
-        check_diagnostics(
-            r#"
-trait T { fn a(); }
-struct U {}
-impl T for U {
-    fn a() {
-        // this comes out of bitflags, mostly
-        #[allow(non_snake_case)]
-        trait __BitFlags {
-            const HiImAlsoBad: u8 = 2;
-            #[inline]
-            fn Dirty(&self) -> bool {
-                false
-            }
-        }
-
-    }
-}
-    "#,
-        );
-    }
-
-    #[test]
-    #[ignore]
-    fn bug_traits_arent_checked() {
-        // FIXME: Traits and functions in traits aren't currently checked by
-        // r-a, even though rustc will complain about them.
-        check_diagnostics(
-            r#"
-trait BAD_TRAIT {
-    // ^^^^^^^^^ Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait`
-    fn BAD_FUNCTION();
-    // ^^^^^^^^^^^^ Function `BAD_FUNCTION` should have snake_case name, e.g. `bad_function`
-    fn BadFunction();
-    // ^^^^^^^^^^^^ Function `BadFunction` should have snake_case name, e.g. `bad_function`
-}
-    "#,
-        );
-    }
-
-    #[test]
-    fn ignores_extern_items() {
-        cov_mark::check!(extern_func_incorrect_case_ignored);
-        cov_mark::check!(extern_static_incorrect_case_ignored);
-        check_diagnostics(
-            r#"
-extern {
-    fn NonSnakeCaseName(SOME_VAR: u8) -> u8;
-    pub static SomeStatic: u8 = 10;
-}
-            "#,
-        );
-    }
-
-    #[test]
-    fn infinite_loop_inner_items() {
-        check_diagnostics(
-            r#"
-fn qualify() {
-    mod foo {
-        use super::*;
-    }
-}
-            "#,
-        )
-    }
-
-    #[test] // Issue #8809.
-    fn parenthesized_parameter() {
-        check_diagnostics(r#"fn f((O): _) {}"#)
-    }
-}
diff --git a/crates/hir_ty/src/diagnostics_sink.rs b/crates/hir_ty/src/diagnostics_sink.rs
deleted file mode 100644
index 084fa8b06..000000000
--- a/crates/hir_ty/src/diagnostics_sink.rs
+++ /dev/null
@@ -1,109 +0,0 @@
-//! 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 used 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 hir_expand::InFile;
-use syntax::SyntaxNodePtr;
-
-#[derive(Copy, Clone, Debug, PartialEq)]
-pub struct DiagnosticCode(pub &'static str);
-
-impl DiagnosticCode {
-    pub fn as_str(&self) -> &str {
-        self.0
-    }
-}
-
-pub trait Diagnostic: Any + Send + Sync + fmt::Debug + 'static {
-    fn code(&self) -> DiagnosticCode;
-    fn message(&self) -> String;
-    /// Source element that triggered the diagnostics.
-    ///
-    /// Note that this should reflect "semantics", rather than specific span we
-    /// want to highlight. When rendering the diagnostics into an error message,
-    /// the IDE will fetch the `SyntaxNode` and will narrow the span
-    /// appropriately.
-    fn display_source(&self) -> InFile<SyntaxNodePtr>;
-    fn as_any(&self) -> &(dyn Any + Send + 'static);
-    fn is_experimental(&self) -> bool {
-        false
-    }
-}
-
-pub struct DiagnosticSink<'a> {
-    callbacks: Vec<Box<dyn FnMut(&dyn Diagnostic) -> Result<(), ()> + 'a>>,
-    filters: Vec<Box<dyn FnMut(&dyn Diagnostic) -> bool + 'a>>,
-    default_callback: Box<dyn FnMut(&dyn Diagnostic) + 'a>,
-}
-
-impl<'a> DiagnosticSink<'a> {
-    pub fn push(&mut self, d: impl Diagnostic) {
-        let d: &dyn Diagnostic = &d;
-        self._push(d);
-    }
-
-    fn _push(&mut self, d: &dyn Diagnostic) {
-        for filter in &mut self.filters {
-            if !filter(d) {
-                return;
-            }
-        }
-        for cb in &mut self.callbacks {
-            match cb(d) {
-                Ok(()) => return,
-                Err(()) => (),
-            }
-        }
-        (self.default_callback)(d)
-    }
-}
-
-pub struct DiagnosticSinkBuilder<'a> {
-    callbacks: Vec<Box<dyn FnMut(&dyn Diagnostic) -> Result<(), ()> + 'a>>,
-    filters: Vec<Box<dyn FnMut(&dyn Diagnostic) -> bool + 'a>>,
-}
-
-impl<'a> DiagnosticSinkBuilder<'a> {
-    pub fn new() -> Self {
-        Self { callbacks: Vec::new(), filters: Vec::new() }
-    }
-
-    pub fn filter<F: FnMut(&dyn Diagnostic) -> bool + 'a>(mut self, cb: F) -> Self {
-        self.filters.push(Box::new(cb));
-        self
-    }
-
-    pub fn on<D: Diagnostic, F: FnMut(&D) + 'a>(mut self, mut cb: F) -> Self {
-        let cb = move |diag: &dyn Diagnostic| match diag.as_any().downcast_ref::<D>() {
-            Some(d) => {
-                cb(d);
-                Ok(())
-            }
-            None => Err(()),
-        };
-        self.callbacks.push(Box::new(cb));
-        self
-    }
-
-    pub fn build<F: FnMut(&dyn Diagnostic) + 'a>(self, default_callback: F) -> DiagnosticSink<'a> {
-        DiagnosticSink {
-            callbacks: self.callbacks,
-            filters: self.filters,
-            default_callback: Box::new(default_callback),
-        }
-    }
-}
diff --git a/crates/hir_ty/src/lib.rs b/crates/hir_ty/src/lib.rs
index 0c6b19653..128cae830 100644
--- a/crates/hir_ty/src/lib.rs
+++ b/crates/hir_ty/src/lib.rs
@@ -21,7 +21,6 @@ mod utils;
 mod walk;
 pub mod db;
 pub mod diagnostics;
-pub mod diagnostics_sink;
 pub mod display;
 pub mod method_resolution;
 pub mod primitive;