From 7731714578d4ae6eb7a54ead2e51ae032e9a700a Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sat, 12 Jun 2021 22:05:23 +0300 Subject: internal: move diagnostics infra to hir --- crates/hir_ty/src/diagnostics.rs | 156 +----------- crates/hir_ty/src/diagnostics/decl_check.rs | 359 +--------------------------- crates/hir_ty/src/diagnostics_sink.rs | 109 --------- crates/hir_ty/src/lib.rs | 1 - 4 files changed, 13 insertions(+), 612 deletions(-) delete mode 100644 crates/hir_ty/src/diagnostics_sink.rs (limited to 'crates/hir_ty') 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 { 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 { - 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(&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> = 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, } #[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; - fn as_any(&self) -> &(dyn Any + Send + 'static); - fn is_experimental(&self) -> bool { - false - } -} - -pub struct DiagnosticSink<'a> { - callbacks: Vec Result<(), ()> + 'a>>, - filters: Vec bool + 'a>>, - default_callback: Box, -} - -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 Result<(), ()> + 'a>>, - filters: Vec bool + 'a>>, -} - -impl<'a> DiagnosticSinkBuilder<'a> { - pub fn new() -> Self { - Self { callbacks: Vec::new(), filters: Vec::new() } - } - - pub fn filter bool + 'a>(mut self, cb: F) -> Self { - self.filters.push(Box::new(cb)); - self - } - - pub fn on(mut self, mut cb: F) -> Self { - let cb = move |diag: &dyn Diagnostic| match diag.as_any().downcast_ref::() { - Some(d) => { - cb(d); - Ok(()) - } - None => Err(()), - }; - self.callbacks.push(Box::new(cb)); - self - } - - pub fn build(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; -- cgit v1.2.3