From 5fe3b75677197964f458f1c7468b383af68f965a Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Thu, 1 Feb 2024 23:22:24 +0300 Subject: [PATCH 01/11] incorrect case diagnostics for trait name --- crates/hir-ty/src/diagnostics/decl_check.rs | 50 +++++++++++++++++-- .../src/handlers/incorrect_case.rs | 3 +- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 78f2005e67..464743135b 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -20,7 +20,7 @@ use hir_def::{ hir::{Pat, PatId}, src::HasSource, AdtId, AttrDefId, ConstId, EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, - StaticId, StructId, + StaticId, StructId, TraitId, }; use hir_expand::{ name::{AsName, Name}, @@ -79,12 +79,13 @@ pub enum IdentType { Enum, Field, Function, + Module, Parameter, StaticVariable, Structure, + Trait, Variable, Variant, - Module, } impl fmt::Display for IdentType { @@ -94,12 +95,13 @@ impl fmt::Display for IdentType { IdentType::Enum => "Enum", IdentType::Field => "Field", IdentType::Function => "Function", + IdentType::Module => "Module", IdentType::Parameter => "Parameter", IdentType::StaticVariable => "Static variable", IdentType::Structure => "Structure", + IdentType::Trait => "Trait", IdentType::Variable => "Variable", IdentType::Variant => "Variant", - IdentType::Module => "Module", }; repr.fmt(f) @@ -136,6 +138,7 @@ impl<'a> DeclValidator<'a> { pub(super) fn validate_item(&mut self, item: ModuleDefId) { match item { ModuleDefId::ModuleId(module_id) => self.validate_module(module_id), + ModuleDefId::TraitId(trait_id) => self.validate_trait(trait_id), ModuleDefId::FunctionId(func) => self.validate_func(func), ModuleDefId::AdtId(adt) => self.validate_adt(adt), ModuleDefId::ConstId(const_id) => self.validate_const(const_id), @@ -283,6 +286,47 @@ impl<'a> DeclValidator<'a> { } } + fn validate_trait(&mut self, trait_id: TraitId) { + // Check whether non-snake case identifiers are allowed for this trait. + if self.allowed(trait_id.into(), allow::NON_CAMEL_CASE_TYPES, false) { + return; + } + + // Check the trait name. + let data = self.db.trait_data(trait_id); + let trait_name = data.name.display(self.db.upcast()).to_string(); + let trait_name_replacement = to_camel_case(&trait_name).map(|new_name| Replacement { + current_name: data.name.clone(), + suggested_text: new_name, + expected_case: CaseType::UpperCamelCase, + }); + + if let Some(replacement) = trait_name_replacement { + let trait_loc = trait_id.lookup(self.db.upcast()); + let trait_src = trait_loc.source(self.db.upcast()); + + let Some(ast_ptr) = trait_src.value.name() else { + never!( + "Replacement ({:?}) was generated for a trait without a name: {:?}", + replacement, + trait_src + ); + return; + }; + + let diagnostic = IncorrectCase { + file: trait_src.file_id, + ident_type: IdentType::Trait, + ident: AstPtr::new(&ast_ptr), + expected_case: replacement.expected_case, + ident_text: trait_name, + suggested_text: replacement.suggested_text, + }; + + self.sink.push(diagnostic); + } + } + fn validate_func(&mut self, func: FunctionId) { let data = self.db.function_data(func); if matches!(func.lookup(self.db.upcast()).container, ItemContainerId::ExternBlockId(_)) { diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index f5a6aa1197..02f93ca9b8 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -462,12 +462,13 @@ extern { } #[test] - fn bug_traits_arent_checked() { + fn incorrect_trait_and_assoc_item_names() { // 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 { + // ^^^^^^^^^ 💡 warn: Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait` fn BAD_FUNCTION(); fn BadFunction(); } From 6d2d6323ac991786d857bf1cec55bf406249e825 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Fri, 2 Feb 2024 02:11:43 +0300 Subject: [PATCH 02/11] incorrect case diagnostics for trait assoc functions and consts --- crates/hir/src/lib.rs | 11 +++++++++++ crates/ide-diagnostics/src/handlers/incorrect_case.rs | 11 +++++++---- .../src/handlers/mismatched_arg_count.rs | 5 +++-- crates/ide-diagnostics/src/tests.rs | 11 +++++++++++ 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 1e21045e98..a741da78d9 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -563,6 +563,17 @@ impl Module { for diag in db.trait_data_with_diagnostics(t.id).1.iter() { emit_def_diagnostic(db, acc, diag); } + + for item in t.items(db) { + let def: DefWithBody = match item { + AssocItem::Function(it) => it.into(), + AssocItem::Const(it) => it.into(), + AssocItem::TypeAlias(_) => continue, + }; + + def.diagnostics(db, acc); + } + acc.extend(def.diagnostics(db)) } ModuleDef::Adt(adt) => { diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index 02f93ca9b8..8b247c4558 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -388,14 +388,13 @@ mod F { #[test] fn complex_ignore() { - // FIXME: this should trigger errors for the second case. check_diagnostics( r#" trait T { fn a(); } struct U {} impl T for U { fn a() { - #[allow(non_snake_case)] + #[allow(non_snake_case, non_upper_case_globals)] trait __BitFlagsOk { const HiImAlsoBad: u8 = 2; fn Dirty(&self) -> bool { false } @@ -403,7 +402,9 @@ impl T for U { trait __BitFlagsBad { const HiImAlsoBad: u8 = 2; + // ^^^^^^^^^^^ 💡 warn: Constant `HiImAlsoBad` should have UPPER_SNAKE_CASE name, e.g. `HI_IM_ALSO_BAD` fn Dirty(&self) -> bool { false } + // ^^^^^💡 warn: Function `Dirty` should have snake_case name, e.g. `dirty` } } } @@ -463,14 +464,16 @@ extern { #[test] fn incorrect_trait_and_assoc_item_names() { - // 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 { // ^^^^^^^^^ 💡 warn: Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait` + const bad_const: u8; + // ^^^^^^^^^ 💡 warn: Constant `bad_const` should have UPPER_SNAKE_CASE name, e.g. `BAD_CONST` fn BAD_FUNCTION(); + // ^^^^^^^^^^^^ 💡 warn: Function `BAD_FUNCTION` should have snake_case name, e.g. `bad_function` fn BadFunction(); + // ^^^^^^^^^^^ 💡 warn: Function `BadFunction` should have snake_case name, e.g. `bad_function` } "#, ); diff --git a/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs b/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs index 66ebf59350..9e9822818e 100644 --- a/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs +++ b/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs @@ -103,7 +103,7 @@ fn invalid_args_range( #[cfg(test)] mod tests { - use crate::tests::check_diagnostics; + use crate::tests::{check_diagnostics, check_diagnostics_with_disabled}; #[test] fn simple_free_fn_zero() { @@ -197,7 +197,7 @@ fn f() { fn method_unknown_receiver() { // note: this is incorrect code, so there might be errors on this in the // future, but we shouldn't emit an argument count diagnostic here - check_diagnostics( + check_diagnostics_with_disabled( r#" trait Foo { fn method(&self, arg: usize) {} } @@ -206,6 +206,7 @@ fn f() { x.method(); } "#, + std::iter::once("unused_variables".to_string()), ); } diff --git a/crates/ide-diagnostics/src/tests.rs b/crates/ide-diagnostics/src/tests.rs index f394a491b5..610d047cd6 100644 --- a/crates/ide-diagnostics/src/tests.rs +++ b/crates/ide-diagnostics/src/tests.rs @@ -89,6 +89,16 @@ pub(crate) fn check_diagnostics(ra_fixture: &str) { check_diagnostics_with_config(config, ra_fixture) } +#[track_caller] +pub(crate) fn check_diagnostics_with_disabled( + ra_fixture: &str, + disabled: impl Iterator, +) { + let mut config = DiagnosticsConfig::test_sample(); + config.disabled.extend(disabled); + check_diagnostics_with_config(config, ra_fixture) +} + #[track_caller] pub(crate) fn check_diagnostics_with_config(config: DiagnosticsConfig, ra_fixture: &str) { let (db, files) = RootDatabase::with_many_files(ra_fixture); @@ -175,6 +185,7 @@ fn minicore_smoke_test() { let mut config = DiagnosticsConfig::test_sample(); // This should be ignored since we conditionally remove code which creates single item use with braces config.disabled.insert("unused_braces".to_string()); + config.disabled.insert("unused_variables".to_string()); check_diagnostics_with_config(config, &source); } From 0ffc088439fbbb31992016cc9cc348c2156d19b4 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Fri, 2 Feb 2024 04:12:50 +0300 Subject: [PATCH 03/11] no incorrect case diagnostics for trait impl assoc functions and consts except for pats in fn bodies --- crates/hir-ty/src/diagnostics/decl_check.rs | 70 ++++++++++++++----- .../src/handlers/incorrect_case.rs | 33 ++++++++- 2 files changed, 84 insertions(+), 19 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 464743135b..7ff06bb218 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -328,8 +328,8 @@ impl<'a> DeclValidator<'a> { } fn validate_func(&mut self, func: FunctionId) { - let data = self.db.function_data(func); - if matches!(func.lookup(self.db.upcast()).container, ItemContainerId::ExternBlockId(_)) { + let container = func.lookup(self.db.upcast()).container; + if matches!(container, ItemContainerId::ExternBlockId(_)) { cov_mark::hit!(extern_func_incorrect_case_ignored); return; } @@ -339,23 +339,48 @@ impl<'a> DeclValidator<'a> { return; } - // Check the function name. - let function_name = data.name.display(self.db.upcast()).to_string(); - let fn_name_replacement = to_lower_snake_case(&function_name).map(|new_name| Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::LowerSnakeCase, - }); + // Check whether function is an associated item of a trait implementation + let is_trait_impl_assoc_fn = self.is_trait_impl_container(container); - let body = self.db.body(func.into()); + // Check the function name. + if !is_trait_impl_assoc_fn { + let data = self.db.function_data(func); + let function_name = data.name.display(self.db.upcast()).to_string(); + let fn_name_replacement = + to_lower_snake_case(&function_name).map(|new_name| Replacement { + current_name: data.name.clone(), + suggested_text: new_name, + expected_case: CaseType::LowerSnakeCase, + }); + // If there is at least one element to spawn a warning on, + // go to the source map and generate a warning. + if let Some(fn_name_replacement) = fn_name_replacement { + self.create_incorrect_case_diagnostic_for_func(func, fn_name_replacement); + } + } else { + cov_mark::hit!(trait_impl_assoc_func_name_incorrect_case_ignored); + } // Check the patterns inside the function body. - // This includes function parameters. + // This includes function parameters if it's not an associated function + // of a trait implementation. + let body = self.db.body(func.into()); let pats_replacements = body .pats .iter() .filter_map(|(pat_id, pat)| match pat { - Pat::Bind { id, .. } => Some((pat_id, &body.bindings[*id].name)), + Pat::Bind { id, .. } => { + // Filter out parameters if it's an associated function + // of a trait implementation. + if is_trait_impl_assoc_fn + && body.params.iter().any(|param_id| *param_id == pat_id) + { + cov_mark::hit!(trait_impl_assoc_func_param_incorrect_case_ignored); + None + } else { + Some((pat_id, &body.bindings[*id].name)) + } + } _ => None, }) .filter_map(|(id, bind_name)| { @@ -371,12 +396,6 @@ impl<'a> DeclValidator<'a> { )) }) .collect(); - - // If there is at least one element to spawn a warning on, go to the source map and generate a warning. - if let Some(fn_name_replacement) = fn_name_replacement { - self.create_incorrect_case_diagnostic_for_func(func, fn_name_replacement); - } - self.create_incorrect_case_diagnostic_for_variables(func, pats_replacements); } @@ -732,6 +751,12 @@ impl<'a> DeclValidator<'a> { } fn validate_const(&mut self, const_id: ConstId) { + let container = const_id.lookup(self.db.upcast()).container; + if self.is_trait_impl_container(container) { + cov_mark::hit!(trait_impl_assoc_const_incorrect_case_ignored); + return; + } + let data = self.db.const_data(const_id); if self.allowed(const_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) { @@ -819,4 +844,13 @@ impl<'a> DeclValidator<'a> { self.sink.push(diagnostic); } + + fn is_trait_impl_container(&self, container_id: ItemContainerId) -> bool { + if let ItemContainerId::ImplId(impl_id) = container_id { + if self.db.impl_trait(impl_id).is_some() { + return true; + } + } + false + } } diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index 8b247c4558..84929c63d1 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -52,7 +52,7 @@ fn fixes(ctx: &DiagnosticsContext<'_>, d: &hir::IncorrectCase) -> Option Date: Fri, 2 Feb 2024 13:16:49 +0300 Subject: [PATCH 04/11] incorrect case diagnostics for type aliases --- crates/hir-ty/src/diagnostics/decl_check.rs | 53 ++++++++++++++++++- crates/hir/src/lib.rs | 32 ++++++----- .../src/handlers/incorrect_case.rs | 6 +++ 3 files changed, 76 insertions(+), 15 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 7ff06bb218..4909b82f88 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -20,7 +20,7 @@ use hir_def::{ hir::{Pat, PatId}, src::HasSource, AdtId, AttrDefId, ConstId, EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, - StaticId, StructId, TraitId, + StaticId, StructId, TraitId, TypeAliasId, }; use hir_expand::{ name::{AsName, Name}, @@ -84,6 +84,7 @@ pub enum IdentType { StaticVariable, Structure, Trait, + TypeAlias, Variable, Variant, } @@ -100,6 +101,7 @@ impl fmt::Display for IdentType { IdentType::StaticVariable => "Static variable", IdentType::Structure => "Structure", IdentType::Trait => "Trait", + IdentType::TypeAlias => "Type alias", IdentType::Variable => "Variable", IdentType::Variant => "Variant", }; @@ -143,6 +145,7 @@ impl<'a> DeclValidator<'a> { ModuleDefId::AdtId(adt) => self.validate_adt(adt), ModuleDefId::ConstId(const_id) => self.validate_const(const_id), ModuleDefId::StaticId(static_id) => self.validate_static(static_id), + ModuleDefId::TypeAliasId(type_alias_id) => self.validate_type_alias(type_alias_id), _ => (), } } @@ -845,6 +848,54 @@ impl<'a> DeclValidator<'a> { self.sink.push(diagnostic); } + fn validate_type_alias(&mut self, type_alias_id: TypeAliasId) { + let container = type_alias_id.lookup(self.db.upcast()).container; + if self.is_trait_impl_container(container) { + cov_mark::hit!(trait_impl_assoc_type_incorrect_case_ignored); + return; + } + + // Check whether non-snake case identifiers are allowed for this type alias. + if self.allowed(type_alias_id.into(), allow::NON_CAMEL_CASE_TYPES, false) { + return; + } + + // Check the type alias name. + let data = self.db.type_alias_data(type_alias_id); + let type_alias_name = data.name.display(self.db.upcast()).to_string(); + let type_alias_name_replacement = + to_camel_case(&type_alias_name).map(|new_name| Replacement { + current_name: data.name.clone(), + suggested_text: new_name, + expected_case: CaseType::UpperCamelCase, + }); + + if let Some(replacement) = type_alias_name_replacement { + let type_alias_loc = type_alias_id.lookup(self.db.upcast()); + let type_alias_src = type_alias_loc.source(self.db.upcast()); + + let Some(ast_ptr) = type_alias_src.value.name() else { + never!( + "Replacement ({:?}) was generated for a type alias without a name: {:?}", + replacement, + type_alias_src + ); + return; + }; + + let diagnostic = IncorrectCase { + file: type_alias_src.file_id, + ident_type: IdentType::TypeAlias, + ident: AstPtr::new(&ast_ptr), + expected_case: replacement.expected_case, + ident_text: type_alias_name, + suggested_text: replacement.suggested_text, + }; + + self.sink.push(diagnostic); + } + } + fn is_trait_impl_container(&self, container_id: ItemContainerId) -> bool { if let ItemContainerId::ImplId(impl_id) = container_id { if self.db.impl_trait(impl_id).is_some() { diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index a741da78d9..997cb43643 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -565,13 +565,7 @@ impl Module { } for item in t.items(db) { - let def: DefWithBody = match item { - AssocItem::Function(it) => it.into(), - AssocItem::Const(it) => it.into(), - AssocItem::TypeAlias(_) => continue, - }; - - def.diagnostics(db, acc); + item.diagnostics(db, acc); } acc.extend(def.diagnostics(db)) @@ -741,13 +735,7 @@ impl Module { } for &item in &db.impl_data(impl_def.id).items { - let def: DefWithBody = match AssocItem::from(item) { - AssocItem::Function(it) => it.into(), - AssocItem::Const(it) => it.into(), - AssocItem::TypeAlias(_) => continue, - }; - - def.diagnostics(db, acc); + AssocItem::from(item).diagnostics(db, acc); } } } @@ -2662,6 +2650,22 @@ impl AssocItem { _ => None, } } + + pub fn diagnostics(self, db: &dyn HirDatabase, acc: &mut Vec) { + match self { + AssocItem::Function(func) => { + DefWithBody::from(func).diagnostics(db, acc); + } + AssocItem::Const(const_) => { + DefWithBody::from(const_).diagnostics(db, acc); + } + AssocItem::TypeAlias(type_alias) => { + for diag in hir_ty::diagnostics::incorrect_case(db, type_alias.id.into()) { + acc.push(diag.into()); + } + } + } + } } impl HasVisibility for AssocItem { diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index 84929c63d1..59031e44cc 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -470,6 +470,8 @@ trait BAD_TRAIT { // ^^^^^^^^^ 💡 warn: Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait` const bad_const: u8; // ^^^^^^^^^ 💡 warn: Constant `bad_const` should have UPPER_SNAKE_CASE name, e.g. `BAD_CONST` + type BAD_TYPE; + // ^^^^^^^^ 💡 warn: Type alias `BAD_TYPE` should have CamelCase name, e.g. `BadType` fn BAD_FUNCTION(); // ^^^^^^^^^^^^ 💡 warn: Function `BAD_FUNCTION` should have snake_case name, e.g. `bad_function` fn BadFunction(); @@ -482,6 +484,7 @@ trait BAD_TRAIT { #[test] fn no_diagnostics_for_trait_impl_assoc_items_except_pats_in_body() { cov_mark::check!(trait_impl_assoc_const_incorrect_case_ignored); + cov_mark::check!(trait_impl_assoc_type_incorrect_case_ignored); cov_mark::check_count!(trait_impl_assoc_func_name_incorrect_case_ignored, 2); cov_mark::check!(trait_impl_assoc_func_param_incorrect_case_ignored); check_diagnostics_with_disabled( @@ -490,6 +493,8 @@ trait BAD_TRAIT { // ^^^^^^^^^ 💡 warn: Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait` const bad_const: u8; // ^^^^^^^^^ 💡 warn: Constant `bad_const` should have UPPER_SNAKE_CASE name, e.g. `BAD_CONST` + type BAD_TYPE; + // ^^^^^^^^ 💡 warn: Type alias `BAD_TYPE` should have CamelCase name, e.g. `BadType` fn BAD_FUNCTION(BAD_PARAM: u8); // ^^^^^^^^^^^^ 💡 warn: Function `BAD_FUNCTION` should have snake_case name, e.g. `bad_function` // ^^^^^^^^^ 💡 warn: Parameter `BAD_PARAM` should have snake_case name, e.g. `bad_param` @@ -499,6 +504,7 @@ trait BAD_TRAIT { impl BAD_TRAIT for () { const bad_const: u8 = 1; + type BAD_TYPE = (); fn BAD_FUNCTION(BAD_PARAM: u8) { let BAD_VAR = 10; // ^^^^^^^ 💡 warn: Variable `BAD_VAR` should have snake_case name, e.g. `bad_var` From 46d79e21b5e2f5d9ce86a22d209453fcc8205372 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Fri, 2 Feb 2024 15:50:00 +0300 Subject: [PATCH 05/11] add allow and deny attribute tests for incorrect case diagnostics for traits --- .../src/handlers/incorrect_case.rs | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index 59031e44cc..ecba53a1b8 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -503,10 +503,10 @@ trait BAD_TRAIT { } impl BAD_TRAIT for () { - const bad_const: u8 = 1; + const bad_const: u8 = 0; type BAD_TYPE = (); fn BAD_FUNCTION(BAD_PARAM: u8) { - let BAD_VAR = 10; + let BAD_VAR = 0; // ^^^^^^^ 💡 warn: Variable `BAD_VAR` should have snake_case name, e.g. `bad_var` } fn BadFunction() {} @@ -560,6 +560,14 @@ pub const some_const: u8 = 10; #[allow(non_upper_case_globals)] pub static SomeStatic: u8 = 10; + +#[allow(non_snake_case, non_camel_case_types, non_upper_case_globals)] +trait BAD_TRAIT { + const bad_const: u8; + type BAD_TYPE; + fn BAD_FUNCTION(BAD_PARAM: u8); + fn BadFunction(); +} "#, ); } @@ -619,6 +627,20 @@ pub const some_const: u8 = 10; #[deny(non_upper_case_globals)] pub static SomeStatic: u8 = 10; //^^^^^^^^^^ 💡 error: Static variable `SomeStatic` should have UPPER_SNAKE_CASE name, e.g. `SOME_STATIC` + +#[deny(non_snake_case, non_camel_case_types, non_upper_case_globals)] +trait BAD_TRAIT { + // ^^^^^^^^^ 💡 error: Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait` + const bad_const: u8; + // ^^^^^^^^^ 💡 error: Constant `bad_const` should have UPPER_SNAKE_CASE name, e.g. `BAD_CONST` + type BAD_TYPE; + // ^^^^^^^^ 💡 error: Type alias `BAD_TYPE` should have CamelCase name, e.g. `BadType` + fn BAD_FUNCTION(BAD_PARAM: u8); + // ^^^^^^^^^^^^ 💡 error: Function `BAD_FUNCTION` should have snake_case name, e.g. `bad_function` + // ^^^^^^^^^ 💡 error: Parameter `BAD_PARAM` should have snake_case name, e.g. `bad_param` + fn BadFunction(); + // ^^^^^^^^^^^ 💡 error: Function `BadFunction` should have snake_case name, e.g. `bad_function` +} "#, ); } From c0071ace5a13b93e7570d7b05bcbc9ddc721833c Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Sat, 3 Feb 2024 13:06:52 +0300 Subject: [PATCH 06/11] refactor `hir-ty::diagnostics::decl_check` --- crates/hir-ty/src/diagnostics/decl_check.rs | 447 ++++++-------------- 1 file changed, 139 insertions(+), 308 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 4909b82f88..75492ec9fe 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -17,6 +17,7 @@ use std::fmt; use hir_def::{ data::adt::VariantData, + db::DefDatabase, hir::{Pat, PatId}, src::HasSource, AdtId, AttrDefId, ConstId, EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, @@ -248,45 +249,25 @@ impl<'a> DeclValidator<'a> { // Check the module name. let Some(module_name) = module_id.name(self.db.upcast()) else { return }; - let module_name_replacement = + let Some(module_name_replacement) = module_name.as_str().and_then(to_lower_snake_case).map(|new_name| Replacement { current_name: module_name, suggested_text: new_name, expected_case: CaseType::LowerSnakeCase, - }); - - if let Some(module_name_replacement) = module_name_replacement { - let module_data = &module_id.def_map(self.db.upcast())[module_id.local_id]; - let module_src = module_data.declaration_source(self.db.upcast()); - - if let Some(module_src) = module_src { - let ast_ptr = match module_src.value.name() { - Some(name) => name, - None => { - never!( - "Replacement ({:?}) was generated for a module without a name: {:?}", - module_name_replacement, - module_src - ); - return; - } - }; - - let diagnostic = IncorrectCase { - file: module_src.file_id, - ident_type: IdentType::Module, - ident: AstPtr::new(&ast_ptr), - expected_case: module_name_replacement.expected_case, - ident_text: module_name_replacement - .current_name - .display(self.db.upcast()) - .to_string(), - suggested_text: module_name_replacement.suggested_text, - }; - - self.sink.push(diagnostic); - } - } + }) + else { + return; + }; + let module_data = &module_id.def_map(self.db.upcast())[module_id.local_id]; + let Some(module_src) = module_data.declaration_source(self.db.upcast()) else { + return; + }; + self.create_incorrect_case_diagnostic_for_ast_node( + module_name_replacement, + module_src.file_id, + &module_src.value, + IdentType::Module, + ); } fn validate_trait(&mut self, trait_id: TraitId) { @@ -297,37 +278,12 @@ impl<'a> DeclValidator<'a> { // Check the trait name. let data = self.db.trait_data(trait_id); - let trait_name = data.name.display(self.db.upcast()).to_string(); - let trait_name_replacement = to_camel_case(&trait_name).map(|new_name| Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::UpperCamelCase, - }); - - if let Some(replacement) = trait_name_replacement { - let trait_loc = trait_id.lookup(self.db.upcast()); - let trait_src = trait_loc.source(self.db.upcast()); - - let Some(ast_ptr) = trait_src.value.name() else { - never!( - "Replacement ({:?}) was generated for a trait without a name: {:?}", - replacement, - trait_src - ); - return; - }; - - let diagnostic = IncorrectCase { - file: trait_src.file_id, - ident_type: IdentType::Trait, - ident: AstPtr::new(&ast_ptr), - expected_case: replacement.expected_case, - ident_text: trait_name, - suggested_text: replacement.suggested_text, - }; - - self.sink.push(diagnostic); - } + self.create_incorrect_case_diagnostic_for_item_name( + trait_id, + &data.name, + CaseType::UpperCamelCase, + IdentType::Trait, + ); } fn validate_func(&mut self, func: FunctionId) { @@ -348,18 +304,12 @@ impl<'a> DeclValidator<'a> { // Check the function name. if !is_trait_impl_assoc_fn { let data = self.db.function_data(func); - let function_name = data.name.display(self.db.upcast()).to_string(); - let fn_name_replacement = - to_lower_snake_case(&function_name).map(|new_name| Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::LowerSnakeCase, - }); - // If there is at least one element to spawn a warning on, - // go to the source map and generate a warning. - if let Some(fn_name_replacement) = fn_name_replacement { - self.create_incorrect_case_diagnostic_for_func(func, fn_name_replacement); - } + self.create_incorrect_case_diagnostic_for_item_name( + func, + &data.name, + CaseType::LowerSnakeCase, + IdentType::Function, + ); } else { cov_mark::hit!(trait_impl_assoc_func_name_incorrect_case_ignored); } @@ -399,47 +349,12 @@ impl<'a> DeclValidator<'a> { )) }) .collect(); - self.create_incorrect_case_diagnostic_for_variables(func, pats_replacements); - } - - /// Given the information about incorrect names in the function declaration, looks up into the source code - /// for exact locations and adds diagnostics into the sink. - fn create_incorrect_case_diagnostic_for_func( - &mut self, - func: FunctionId, - fn_name_replacement: Replacement, - ) { - let fn_loc = func.lookup(self.db.upcast()); - let fn_src = fn_loc.source(self.db.upcast()); - - // Diagnostic for function name. - let ast_ptr = match fn_src.value.name() { - Some(name) => name, - None => { - never!( - "Replacement ({:?}) was generated for a function without a name: {:?}", - fn_name_replacement, - fn_src - ); - return; - } - }; - - let diagnostic = IncorrectCase { - file: fn_src.file_id, - ident_type: IdentType::Function, - ident: AstPtr::new(&ast_ptr), - expected_case: fn_name_replacement.expected_case, - ident_text: fn_name_replacement.current_name.display(self.db.upcast()).to_string(), - suggested_text: fn_name_replacement.suggested_text, - }; - - self.sink.push(diagnostic); + self.create_incorrect_case_diagnostic_for_func_variables(func, pats_replacements); } /// Given the information about incorrect variable names, looks up into the source code /// for exact locations and adds diagnostics into the sink. - fn create_incorrect_case_diagnostic_for_variables( + fn create_incorrect_case_diagnostic_for_func_variables( &mut self, func: FunctionId, pats_replacements: Vec<(PatId, Replacement)>, @@ -460,10 +375,6 @@ impl<'a> DeclValidator<'a> { Some(parent) => parent, None => continue, }; - let name_ast = match ident_pat.name() { - Some(name_ast) => name_ast, - None => continue, - }; let is_param = ast::Param::can_cast(parent.kind()); @@ -481,16 +392,12 @@ impl<'a> DeclValidator<'a> { let ident_type = if is_param { IdentType::Parameter } else { IdentType::Variable }; - let diagnostic = IncorrectCase { - file: source_ptr.file_id, + self.create_incorrect_case_diagnostic_for_ast_node( + replacement, + source_ptr.file_id, + &ident_pat, ident_type, - ident: AstPtr::new(&name_ast), - expected_case: replacement.expected_case, - ident_text: replacement.current_name.display(self.db.upcast()).to_string(), - suggested_text: replacement.suggested_text, - }; - - self.sink.push(diagnostic); + ); } } } @@ -504,20 +411,17 @@ impl<'a> DeclValidator<'a> { let non_snake_case_allowed = self.allowed(struct_id.into(), allow::NON_SNAKE_CASE, false); // Check the structure name. - let struct_name = data.name.display(self.db.upcast()).to_string(); - let struct_name_replacement = if !non_camel_case_allowed { - to_camel_case(&struct_name).map(|new_name| Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::UpperCamelCase, - }) - } else { - None - }; + if !non_camel_case_allowed { + self.create_incorrect_case_diagnostic_for_item_name( + struct_id, + &data.name, + CaseType::UpperCamelCase, + IdentType::Structure, + ); + } // Check the field names. let mut struct_fields_replacements = Vec::new(); - if !non_snake_case_allowed { if let VariantData::Record(fields) = data.variant_data.as_ref() { for (_, field) in fields.iter() { @@ -535,54 +439,27 @@ impl<'a> DeclValidator<'a> { } // If there is at least one element to spawn a warning on, go to the source map and generate a warning. - self.create_incorrect_case_diagnostic_for_struct( + self.create_incorrect_case_diagnostic_for_struct_fields( struct_id, - struct_name_replacement, struct_fields_replacements, ); } - /// Given the information about incorrect names in the struct declaration, looks up into the source code - /// for exact locations and adds diagnostics into the sink. - fn create_incorrect_case_diagnostic_for_struct( + /// Given the information about incorrect names for struct fields, + /// looks up into the source code for exact locations and adds diagnostics into the sink. + fn create_incorrect_case_diagnostic_for_struct_fields( &mut self, struct_id: StructId, - struct_name_replacement: Option, struct_fields_replacements: Vec, ) { // XXX: Only look at sources if we do have incorrect names. - if struct_name_replacement.is_none() && struct_fields_replacements.is_empty() { + if struct_fields_replacements.is_empty() { return; } let struct_loc = struct_id.lookup(self.db.upcast()); let struct_src = struct_loc.source(self.db.upcast()); - if let Some(replacement) = struct_name_replacement { - let ast_ptr = match struct_src.value.name() { - Some(name) => name, - None => { - never!( - "Replacement ({:?}) was generated for a structure without a name: {:?}", - replacement, - struct_src - ); - return; - } - }; - - let diagnostic = IncorrectCase { - file: struct_src.file_id, - ident_type: IdentType::Structure, - ident: AstPtr::new(&ast_ptr), - expected_case: replacement.expected_case, - ident_text: replacement.current_name.display(self.db.upcast()).to_string(), - suggested_text: replacement.suggested_text, - }; - - self.sink.push(diagnostic); - } - let struct_fields_list = match struct_src.value.field_list() { Some(ast::FieldList::RecordFieldList(fields)) => fields, _ => { @@ -638,15 +515,15 @@ impl<'a> DeclValidator<'a> { } // Check the enum name. - let enum_name = data.name.display(self.db.upcast()).to_string(); - let enum_name_replacement = to_camel_case(&enum_name).map(|new_name| Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::UpperCamelCase, - }); + self.create_incorrect_case_diagnostic_for_item_name( + enum_id, + &data.name, + CaseType::UpperCamelCase, + IdentType::Enum, + ); // Check the field names. - let enum_fields_replacements = data + let enum_variants_replacements = data .variants .iter() .filter_map(|(_, name)| { @@ -659,54 +536,24 @@ impl<'a> DeclValidator<'a> { .collect(); // If there is at least one element to spawn a warning on, go to the source map and generate a warning. - self.create_incorrect_case_diagnostic_for_enum( - enum_id, - enum_name_replacement, - enum_fields_replacements, - ) + self.create_incorrect_case_diagnostic_for_enum_variants(enum_id, enum_variants_replacements) } - /// Given the information about incorrect names in the struct declaration, looks up into the source code - /// for exact locations and adds diagnostics into the sink. - fn create_incorrect_case_diagnostic_for_enum( + /// Given the information about incorrect names for enum variants, + /// looks up into the source code for exact locations and adds diagnostics into the sink. + fn create_incorrect_case_diagnostic_for_enum_variants( &mut self, enum_id: EnumId, - enum_name_replacement: Option, enum_variants_replacements: Vec, ) { // XXX: only look at sources if we do have incorrect names - if enum_name_replacement.is_none() && enum_variants_replacements.is_empty() { + if enum_variants_replacements.is_empty() { return; } let enum_loc = enum_id.lookup(self.db.upcast()); let enum_src = enum_loc.source(self.db.upcast()); - if let Some(replacement) = enum_name_replacement { - let ast_ptr = match enum_src.value.name() { - Some(name) => name, - None => { - never!( - "Replacement ({:?}) was generated for a enum without a name: {:?}", - replacement, - enum_src - ); - return; - } - }; - - let diagnostic = IncorrectCase { - file: enum_src.file_id, - ident_type: IdentType::Enum, - ident: AstPtr::new(&ast_ptr), - expected_case: replacement.expected_case, - ident_text: replacement.current_name.display(self.db.upcast()).to_string(), - suggested_text: replacement.suggested_text, - }; - - self.sink.push(diagnostic); - } - let enum_variants_list = match enum_src.value.variant_list() { Some(variants) => variants, _ => { @@ -760,47 +607,20 @@ impl<'a> DeclValidator<'a> { return; } - let data = self.db.const_data(const_id); - if self.allowed(const_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) { return; } - let name = match &data.name { - Some(name) => name, - None => return, - }; - - let const_name = name.to_smol_str(); - let replacement = if let Some(new_name) = to_upper_snake_case(&const_name) { - Replacement { - current_name: name.clone(), - suggested_text: new_name, - expected_case: CaseType::UpperSnakeCase, - } - } else { - // Nothing to do here. + let data = self.db.const_data(const_id); + let Some(name) = &data.name else { return; }; - - let const_loc = const_id.lookup(self.db.upcast()); - let const_src = const_loc.source(self.db.upcast()); - - let ast_ptr = match const_src.value.name() { - Some(name) => name, - None => return, - }; - - let diagnostic = IncorrectCase { - file: const_src.file_id, - ident_type: IdentType::Constant, - ident: AstPtr::new(&ast_ptr), - expected_case: replacement.expected_case, - ident_text: replacement.current_name.display(self.db.upcast()).to_string(), - suggested_text: replacement.suggested_text, - }; - - self.sink.push(diagnostic); + self.create_incorrect_case_diagnostic_for_item_name( + const_id, + name, + CaseType::UpperSnakeCase, + IdentType::Constant, + ); } fn validate_static(&mut self, static_id: StaticId) { @@ -814,38 +634,12 @@ impl<'a> DeclValidator<'a> { return; } - let name = &data.name; - - let static_name = name.to_smol_str(); - let replacement = if let Some(new_name) = to_upper_snake_case(&static_name) { - Replacement { - current_name: name.clone(), - suggested_text: new_name, - expected_case: CaseType::UpperSnakeCase, - } - } else { - // Nothing to do here. - return; - }; - - let static_loc = static_id.lookup(self.db.upcast()); - let static_src = static_loc.source(self.db.upcast()); - - let ast_ptr = match static_src.value.name() { - Some(name) => name, - None => return, - }; - - let diagnostic = IncorrectCase { - file: static_src.file_id, - ident_type: IdentType::StaticVariable, - ident: AstPtr::new(&ast_ptr), - expected_case: replacement.expected_case, - ident_text: replacement.current_name.display(self.db.upcast()).to_string(), - suggested_text: replacement.suggested_text, - }; - - self.sink.push(diagnostic); + self.create_incorrect_case_diagnostic_for_item_name( + static_id, + &data.name, + CaseType::UpperSnakeCase, + IdentType::StaticVariable, + ); } fn validate_type_alias(&mut self, type_alias_id: TypeAliasId) { @@ -862,38 +656,75 @@ impl<'a> DeclValidator<'a> { // Check the type alias name. let data = self.db.type_alias_data(type_alias_id); - let type_alias_name = data.name.display(self.db.upcast()).to_string(); - let type_alias_name_replacement = - to_camel_case(&type_alias_name).map(|new_name| Replacement { - current_name: data.name.clone(), - suggested_text: new_name, - expected_case: CaseType::UpperCamelCase, - }); + self.create_incorrect_case_diagnostic_for_item_name( + type_alias_id, + &data.name, + CaseType::UpperCamelCase, + IdentType::TypeAlias, + ); + } - if let Some(replacement) = type_alias_name_replacement { - let type_alias_loc = type_alias_id.lookup(self.db.upcast()); - let type_alias_src = type_alias_loc.source(self.db.upcast()); + fn create_incorrect_case_diagnostic_for_item_name( + &mut self, + item_id: L, + name: &Name, + expected_case: CaseType, + ident_type: IdentType, + ) where + N: AstNode + HasName + fmt::Debug, + S: HasSource, + L: Lookup = dyn DefDatabase + 'a>, + { + let to_expected_case_type = match expected_case { + CaseType::LowerSnakeCase => to_lower_snake_case, + CaseType::UpperSnakeCase => to_upper_snake_case, + CaseType::UpperCamelCase => to_camel_case, + }; + let Some(replacement) = to_expected_case_type(&name.to_smol_str()).map(|new_name| { + Replacement { current_name: name.clone(), suggested_text: new_name, expected_case } + }) else { + return; + }; - let Some(ast_ptr) = type_alias_src.value.name() else { - never!( - "Replacement ({:?}) was generated for a type alias without a name: {:?}", - replacement, - type_alias_src - ); - return; - }; + let item_loc = item_id.lookup(self.db.upcast()); + let item_src = item_loc.source(self.db.upcast()); + self.create_incorrect_case_diagnostic_for_ast_node( + replacement, + item_src.file_id, + &item_src.value, + ident_type, + ); + } - let diagnostic = IncorrectCase { - file: type_alias_src.file_id, - ident_type: IdentType::TypeAlias, - ident: AstPtr::new(&ast_ptr), - expected_case: replacement.expected_case, - ident_text: type_alias_name, - suggested_text: replacement.suggested_text, - }; + fn create_incorrect_case_diagnostic_for_ast_node( + &mut self, + replacement: Replacement, + file_id: HirFileId, + node: &T, + ident_type: IdentType, + ) where + T: AstNode + HasName + fmt::Debug, + { + let Some(name_ast) = node.name() else { + never!( + "Replacement ({:?}) was generated for a {:?} without a name: {:?}", + replacement, + ident_type, + node + ); + return; + }; - self.sink.push(diagnostic); - } + let diagnostic = IncorrectCase { + file: file_id, + ident_type, + ident: AstPtr::new(&name_ast), + expected_case: replacement.expected_case, + ident_text: replacement.current_name.display(self.db.upcast()).to_string(), + suggested_text: replacement.suggested_text, + }; + + self.sink.push(diagnostic); } fn is_trait_impl_container(&self, container_id: ItemContainerId) -> bool { From 23aa872f3cb4d409c69e44afc2951390fdbe1c81 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Sat, 3 Feb 2024 16:37:58 +0300 Subject: [PATCH 07/11] refactor `hir-ty::diagnostics::decl_check` for function bodies --- crates/hir-ty/src/diagnostics/decl_check.rs | 126 +++++++++----------- 1 file changed, 55 insertions(+), 71 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 75492ec9fe..8432f5a0ad 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -16,12 +16,9 @@ mod case_conv; use std::fmt; use hir_def::{ - data::adt::VariantData, - db::DefDatabase, - hir::{Pat, PatId}, - src::HasSource, - AdtId, AttrDefId, ConstId, EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, - StaticId, StructId, TraitId, TypeAliasId, + data::adt::VariantData, db::DefDatabase, hir::Pat, src::HasSource, AdtId, AttrDefId, ConstId, + EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, StaticId, StructId, + TraitId, TypeAliasId, }; use hir_expand::{ name::{AsName, Name}, @@ -298,11 +295,9 @@ impl<'a> DeclValidator<'a> { return; } - // Check whether function is an associated item of a trait implementation - let is_trait_impl_assoc_fn = self.is_trait_impl_container(container); - // Check the function name. - if !is_trait_impl_assoc_fn { + // Skipped if function is an associated item of a trait implementation. + if !self.is_trait_impl_container(container) { let data = self.db.function_data(func); self.create_incorrect_case_diagnostic_for_item_name( func, @@ -315,91 +310,80 @@ impl<'a> DeclValidator<'a> { } // Check the patterns inside the function body. - // This includes function parameters if it's not an associated function - // of a trait implementation. + self.validate_func_body(func); + } + + /// Check incorrect names for patterns inside the function body. + /// This includes function parameters except for trait implementation associated functions. + fn validate_func_body(&mut self, func: FunctionId) { + // Check whether function is an associated item of a trait implementation + let container = func.lookup(self.db.upcast()).container; + let is_trait_impl_assoc_fn = self.is_trait_impl_container(container); + let body = self.db.body(func.into()); - let pats_replacements = body + let mut pats_replacements = body .pats .iter() .filter_map(|(pat_id, pat)| match pat { Pat::Bind { id, .. } => { - // Filter out parameters if it's an associated function - // of a trait implementation. + // Filter out parameters for trait implementation associated functions. if is_trait_impl_assoc_fn && body.params.iter().any(|param_id| *param_id == pat_id) { cov_mark::hit!(trait_impl_assoc_func_param_incorrect_case_ignored); None } else { - Some((pat_id, &body.bindings[*id].name)) + let bind_name = &body.bindings[*id].name; + let replacement = Replacement { + current_name: bind_name.clone(), + suggested_text: to_lower_snake_case(&bind_name.to_smol_str())?, + expected_case: CaseType::LowerSnakeCase, + }; + Some((pat_id, replacement)) } } _ => None, }) - .filter_map(|(id, bind_name)| { - Some(( - id, - Replacement { - current_name: bind_name.clone(), - suggested_text: to_lower_snake_case( - &bind_name.display(self.db.upcast()).to_string(), - )?, - expected_case: CaseType::LowerSnakeCase, - }, - )) - }) - .collect(); - self.create_incorrect_case_diagnostic_for_func_variables(func, pats_replacements); - } + .peekable(); - /// Given the information about incorrect variable names, looks up into the source code - /// for exact locations and adds diagnostics into the sink. - fn create_incorrect_case_diagnostic_for_func_variables( - &mut self, - func: FunctionId, - pats_replacements: Vec<(PatId, Replacement)>, - ) { // XXX: only look at source_map if we do have missing fields - if pats_replacements.is_empty() { + if pats_replacements.peek().is_none() { return; } let (_, source_map) = self.db.body_with_source_map(func.into()); - for (id, replacement) in pats_replacements { - if let Ok(source_ptr) = source_map.pat_syntax(id) { - if let Some(ptr) = source_ptr.value.cast::() { - let root = source_ptr.file_syntax(self.db.upcast()); - let ident_pat = ptr.to_node(&root); - let parent = match ident_pat.syntax().parent() { - Some(parent) => parent, - None => continue, - }; + let Ok(source_ptr) = source_map.pat_syntax(id) else { + continue; + }; + let Some(ptr) = source_ptr.value.cast::() else { + continue; + }; + let root = source_ptr.file_syntax(self.db.upcast()); + let ident_pat = ptr.to_node(&root); + let Some(parent) = ident_pat.syntax().parent() else { + continue; + }; - let is_param = ast::Param::can_cast(parent.kind()); - - // We have to check that it's either `let var = ...` or `var @ Variant(_)` statement, - // because e.g. match arms are patterns as well. - // In other words, we check that it's a named variable binding. - let is_binding = ast::LetStmt::can_cast(parent.kind()) - || (ast::MatchArm::can_cast(parent.kind()) - && ident_pat.at_token().is_some()); - if !(is_param || is_binding) { - // This pattern is not an actual variable declaration, e.g. `Some(val) => {..}` match arm. - continue; - } - - let ident_type = - if is_param { IdentType::Parameter } else { IdentType::Variable }; - - self.create_incorrect_case_diagnostic_for_ast_node( - replacement, - source_ptr.file_id, - &ident_pat, - ident_type, - ); - } + let is_param = ast::Param::can_cast(parent.kind()); + // We have to check that it's either `let var = ...` or `var @ Variant(_)` statement, + // because e.g. match arms are patterns as well. + // In other words, we check that it's a named variable binding. + let is_binding = ast::LetStmt::can_cast(parent.kind()) + || (ast::MatchArm::can_cast(parent.kind()) && ident_pat.at_token().is_some()); + if !(is_param || is_binding) { + // This pattern is not an actual variable declaration, e.g. `Some(val) => {..}` match arm. + continue; } + + let ident_type = if is_param { IdentType::Parameter } else { IdentType::Variable }; + + self.create_incorrect_case_diagnostic_for_ast_node( + replacement, + source_ptr.file_id, + &ident_pat, + ident_type, + ); } } From 8e29104178688d8b8672f5a64661751f5b6d0de1 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Sat, 3 Feb 2024 17:11:56 +0300 Subject: [PATCH 08/11] refactor `hir-ty::diagnostics::decl_check` for struct fields --- crates/hir-ty/src/diagnostics/decl_check.rs | 128 +++++++++----------- 1 file changed, 58 insertions(+), 70 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 8432f5a0ad..0401c70958 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -388,14 +388,11 @@ impl<'a> DeclValidator<'a> { } fn validate_struct(&mut self, struct_id: StructId) { - let data = self.db.struct_data(struct_id); - + // Check the structure name. let non_camel_case_allowed = self.allowed(struct_id.into(), allow::NON_CAMEL_CASE_TYPES, false); - let non_snake_case_allowed = self.allowed(struct_id.into(), allow::NON_SNAKE_CASE, false); - - // Check the structure name. if !non_camel_case_allowed { + let data = self.db.struct_data(struct_id); self.create_incorrect_case_diagnostic_for_item_name( struct_id, &data.name, @@ -405,88 +402,79 @@ impl<'a> DeclValidator<'a> { } // Check the field names. - let mut struct_fields_replacements = Vec::new(); - if !non_snake_case_allowed { - if let VariantData::Record(fields) = data.variant_data.as_ref() { - for (_, field) in fields.iter() { - let field_name = field.name.display(self.db.upcast()).to_string(); - if let Some(new_name) = to_lower_snake_case(&field_name) { - let replacement = Replacement { - current_name: field.name.clone(), - suggested_text: new_name, - expected_case: CaseType::LowerSnakeCase, - }; - struct_fields_replacements.push(replacement); - } - } - } - } - - // If there is at least one element to spawn a warning on, go to the source map and generate a warning. - self.create_incorrect_case_diagnostic_for_struct_fields( - struct_id, - struct_fields_replacements, - ); + self.validate_struct_fields(struct_id); } - /// Given the information about incorrect names for struct fields, - /// looks up into the source code for exact locations and adds diagnostics into the sink. - fn create_incorrect_case_diagnostic_for_struct_fields( - &mut self, - struct_id: StructId, - struct_fields_replacements: Vec, - ) { + /// Check incorrect names for struct fields. + fn validate_struct_fields(&mut self, struct_id: StructId) { + if self.allowed(struct_id.into(), allow::NON_SNAKE_CASE, false) { + return; + } + + let data = self.db.struct_data(struct_id); + let VariantData::Record(fields) = data.variant_data.as_ref() else { + return; + }; + let mut struct_fields_replacements = fields + .iter() + .filter_map(|(_, field)| { + to_lower_snake_case(&field.name.to_smol_str()).map(|new_name| Replacement { + current_name: field.name.clone(), + suggested_text: new_name, + expected_case: CaseType::LowerSnakeCase, + }) + }) + .peekable(); + // XXX: Only look at sources if we do have incorrect names. - if struct_fields_replacements.is_empty() { + if struct_fields_replacements.peek().is_none() { return; } let struct_loc = struct_id.lookup(self.db.upcast()); let struct_src = struct_loc.source(self.db.upcast()); - let struct_fields_list = match struct_src.value.field_list() { - Some(ast::FieldList::RecordFieldList(fields)) => fields, - _ => { - always!( - struct_fields_replacements.is_empty(), - "Replacements ({:?}) were generated for a structure fields which had no fields list: {:?}", - struct_fields_replacements, - struct_src - ); - return; - } + let Some(ast::FieldList::RecordFieldList(struct_fields_list)) = + struct_src.value.field_list() + else { + always!( + struct_fields_replacements.peek().is_none(), + "Replacements ({:?}) were generated for a structure fields \ + which had no fields list: {:?}", + struct_fields_replacements.collect::>(), + struct_src + ); + return; }; let mut struct_fields_iter = struct_fields_list.fields(); - for field_to_rename in struct_fields_replacements { + for field_replacement in struct_fields_replacements { // We assume that parameters in replacement are in the same order as in the // actual params list, but just some of them (ones that named correctly) are skipped. - let ast_ptr = loop { - match struct_fields_iter.next().and_then(|field| field.name()) { - Some(field_name) => { - if field_name.as_name() == field_to_rename.current_name { - break field_name; - } - } - None => { - never!( - "Replacement ({:?}) was generated for a structure field which was not found: {:?}", - field_to_rename, struct_src - ); - return; + let field = loop { + if let Some(field) = struct_fields_iter.next() { + let Some(field_name) = field.name() else { + continue; + }; + if field_name.as_name() == field_replacement.current_name { + break field; } + } else { + never!( + "Replacement ({:?}) was generated for a structure field \ + which was not found: {:?}", + field_replacement, + struct_src + ); + return; } }; - let diagnostic = IncorrectCase { - file: struct_src.file_id, - ident_type: IdentType::Field, - ident: AstPtr::new(&ast_ptr), - expected_case: field_to_rename.expected_case, - ident_text: field_to_rename.current_name.display(self.db.upcast()).to_string(), - suggested_text: field_to_rename.suggested_text, - }; - - self.sink.push(diagnostic); + self.create_incorrect_case_diagnostic_for_ast_node( + field_replacement, + struct_src.file_id, + &field, + IdentType::Field, + ); } } From d761d86f03fdcb147571cee3a381653d9293c6db Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Sat, 3 Feb 2024 17:36:11 +0300 Subject: [PATCH 09/11] refactor `hir-ty::diagnostics::decl_check` for enum variants --- crates/hir-ty/src/diagnostics/decl_check.rs | 95 ++++++++++----------- 1 file changed, 43 insertions(+), 52 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 0401c70958..e8abebeba4 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -494,81 +494,72 @@ impl<'a> DeclValidator<'a> { IdentType::Enum, ); - // Check the field names. - let enum_variants_replacements = data + // Check the variant names. + self.validate_enum_variants(enum_id) + } + + /// Check incorrect names for enum variants. + fn validate_enum_variants(&mut self, enum_id: EnumId) { + let data = self.db.enum_data(enum_id); + let mut enum_variants_replacements = data .variants .iter() .filter_map(|(_, name)| { - Some(Replacement { + to_camel_case(&name.to_smol_str()).map(|new_name| Replacement { current_name: name.clone(), - suggested_text: to_camel_case(&name.to_smol_str())?, + suggested_text: new_name, expected_case: CaseType::UpperCamelCase, }) }) - .collect(); + .peekable(); - // If there is at least one element to spawn a warning on, go to the source map and generate a warning. - self.create_incorrect_case_diagnostic_for_enum_variants(enum_id, enum_variants_replacements) - } - - /// Given the information about incorrect names for enum variants, - /// looks up into the source code for exact locations and adds diagnostics into the sink. - fn create_incorrect_case_diagnostic_for_enum_variants( - &mut self, - enum_id: EnumId, - enum_variants_replacements: Vec, - ) { // XXX: only look at sources if we do have incorrect names - if enum_variants_replacements.is_empty() { + if enum_variants_replacements.peek().is_none() { return; } let enum_loc = enum_id.lookup(self.db.upcast()); let enum_src = enum_loc.source(self.db.upcast()); - let enum_variants_list = match enum_src.value.variant_list() { - Some(variants) => variants, - _ => { - always!( - enum_variants_replacements.is_empty(), - "Replacements ({:?}) were generated for a enum variants which had no fields list: {:?}", - enum_variants_replacements, - enum_src - ); - return; - } + let Some(enum_variants_list) = enum_src.value.variant_list() else { + always!( + enum_variants_replacements.peek().is_none(), + "Replacements ({:?}) were generated for enum variants \ + which had no fields list: {:?}", + enum_variants_replacements, + enum_src + ); + return; }; let mut enum_variants_iter = enum_variants_list.variants(); - for variant_to_rename in enum_variants_replacements { + for variant_replacement in enum_variants_replacements { // We assume that parameters in replacement are in the same order as in the // actual params list, but just some of them (ones that named correctly) are skipped. - let ast_ptr = loop { - match enum_variants_iter.next().and_then(|v| v.name()) { - Some(variant_name) => { - if variant_name.as_name() == variant_to_rename.current_name { - break variant_name; - } - } - None => { - never!( - "Replacement ({:?}) was generated for a enum variant which was not found: {:?}", - variant_to_rename, enum_src - ); - return; + let variant = loop { + if let Some(variant) = enum_variants_iter.next() { + let Some(variant_name) = variant.name() else { + continue; + }; + if variant_name.as_name() == variant_replacement.current_name { + break variant; } + } else { + never!( + "Replacement ({:?}) was generated for an enum variant \ + which was not found: {:?}", + variant_replacement, + enum_src + ); + return; } }; - let diagnostic = IncorrectCase { - file: enum_src.file_id, - ident_type: IdentType::Variant, - ident: AstPtr::new(&ast_ptr), - expected_case: variant_to_rename.expected_case, - ident_text: variant_to_rename.current_name.display(self.db.upcast()).to_string(), - suggested_text: variant_to_rename.suggested_text, - }; - - self.sink.push(diagnostic); + self.create_incorrect_case_diagnostic_for_ast_node( + variant_replacement, + enum_src.file_id, + &variant, + IdentType::Variant, + ); } } From 33b3b6dbf91d67cb9a724ccf268f990c89429665 Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Mon, 5 Feb 2024 01:05:39 +0300 Subject: [PATCH 10/11] refactor: disable "unused" lint using parameter name --- .../ide-diagnostics/src/handlers/mismatched_arg_count.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs b/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs index 9e9822818e..41c762c85b 100644 --- a/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs +++ b/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs @@ -103,7 +103,7 @@ fn invalid_args_range( #[cfg(test)] mod tests { - use crate::tests::{check_diagnostics, check_diagnostics_with_disabled}; + use crate::tests::check_diagnostics; #[test] fn simple_free_fn_zero() { @@ -197,16 +197,15 @@ fn f() { fn method_unknown_receiver() { // note: this is incorrect code, so there might be errors on this in the // future, but we shouldn't emit an argument count diagnostic here - check_diagnostics_with_disabled( + check_diagnostics( r#" -trait Foo { fn method(&self, arg: usize) {} } +trait Foo { fn method(&self, _arg: usize) {} } fn f() { let x; x.method(); } "#, - std::iter::once("unused_variables".to_string()), ); } From b45ee8281181dfee3b63e36541b6d07e3f10942d Mon Sep 17 00:00:00 2001 From: davidsemakula Date: Mon, 5 Feb 2024 01:31:28 +0300 Subject: [PATCH 11/11] incorrect case diagnostics for param names of trait impl assoc functions --- crates/hir-ty/src/diagnostics/decl_check.rs | 26 +++++-------------- .../src/handlers/incorrect_case.rs | 2 +- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index e8abebeba4..38eb3371e3 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -316,31 +316,19 @@ impl<'a> DeclValidator<'a> { /// Check incorrect names for patterns inside the function body. /// This includes function parameters except for trait implementation associated functions. fn validate_func_body(&mut self, func: FunctionId) { - // Check whether function is an associated item of a trait implementation - let container = func.lookup(self.db.upcast()).container; - let is_trait_impl_assoc_fn = self.is_trait_impl_container(container); - let body = self.db.body(func.into()); let mut pats_replacements = body .pats .iter() .filter_map(|(pat_id, pat)| match pat { Pat::Bind { id, .. } => { - // Filter out parameters for trait implementation associated functions. - if is_trait_impl_assoc_fn - && body.params.iter().any(|param_id| *param_id == pat_id) - { - cov_mark::hit!(trait_impl_assoc_func_param_incorrect_case_ignored); - None - } else { - let bind_name = &body.bindings[*id].name; - let replacement = Replacement { - current_name: bind_name.clone(), - suggested_text: to_lower_snake_case(&bind_name.to_smol_str())?, - expected_case: CaseType::LowerSnakeCase, - }; - Some((pat_id, replacement)) - } + let bind_name = &body.bindings[*id].name; + let replacement = Replacement { + current_name: bind_name.clone(), + suggested_text: to_lower_snake_case(&bind_name.to_smol_str())?, + expected_case: CaseType::LowerSnakeCase, + }; + Some((pat_id, replacement)) } _ => None, }) diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index ecba53a1b8..96fbb4468f 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -486,7 +486,6 @@ trait BAD_TRAIT { cov_mark::check!(trait_impl_assoc_const_incorrect_case_ignored); cov_mark::check!(trait_impl_assoc_type_incorrect_case_ignored); cov_mark::check_count!(trait_impl_assoc_func_name_incorrect_case_ignored, 2); - cov_mark::check!(trait_impl_assoc_func_param_incorrect_case_ignored); check_diagnostics_with_disabled( r#" trait BAD_TRAIT { @@ -506,6 +505,7 @@ impl BAD_TRAIT for () { const bad_const: u8 = 0; type BAD_TYPE = (); fn BAD_FUNCTION(BAD_PARAM: u8) { + // ^^^^^^^^^ 💡 warn: Parameter `BAD_PARAM` should have snake_case name, e.g. `bad_param` let BAD_VAR = 0; // ^^^^^^^ 💡 warn: Variable `BAD_VAR` should have snake_case name, e.g. `bad_var` }