diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 78f2005e676..38eb3371e3c 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -16,11 +16,9 @@ use std::fmt; use hir_def::{ - data::adt::VariantData, - hir::{Pat, PatId}, - src::HasSource, - AdtId, AttrDefId, ConstId, EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, - StaticId, StructId, + 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}, @@ -79,12 +77,14 @@ pub enum IdentType { Enum, Field, Function, + Module, Parameter, StaticVariable, Structure, + Trait, + TypeAlias, Variable, Variant, - Module, } impl fmt::Display for IdentType { @@ -94,12 +94,14 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { 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::TypeAlias => "Type alias", IdentType::Variable => "Variable", IdentType::Variant => "Variant", - IdentType::Module => "Module", }; repr.fmt(f) @@ -136,10 +138,12 @@ pub(super) fn new(db: &'a dyn HirDatabase) -> 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), ModuleDefId::StaticId(static_id) => self.validate_static(static_id), + ModuleDefId::TypeAliasId(type_alias_id) => self.validate_type_alias(type_alias_id), _ => (), } } @@ -242,50 +246,46 @@ fn validate_module(&mut self, module_id: ModuleId) { // 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, - }); + }) + 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, + ); + } - 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); - } + 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); + self.create_incorrect_case_diagnostic_for_item_name( + trait_id, + &data.name, + CaseType::UpperCamelCase, + IdentType::Trait, + ); } 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; } @@ -296,270 +296,173 @@ fn validate_func(&mut self, func: FunctionId) { } // 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, - }); - - let body = self.db.body(func.into()); + // 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, + &data.name, + CaseType::LowerSnakeCase, + IdentType::Function, + ); + } else { + cov_mark::hit!(trait_impl_assoc_func_name_incorrect_case_ignored); + } // Check the patterns inside the function body. - // This includes function parameters. - let pats_replacements = body + 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) { + let body = self.db.body(func.into()); + let mut 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, .. } => { + 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(); + .peekable(); - // 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); - } - - /// 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); - } - - /// 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( - &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 name_ast = match ident_pat.name() { - Some(name_ast) => name_ast, - 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 }; - - let diagnostic = IncorrectCase { - file: source_ptr.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); - } + 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, + ); } } 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. - 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 - }; - - // 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 !non_camel_case_allowed { + let data = self.db.struct_data(struct_id); + self.create_incorrect_case_diagnostic_for_item_name( + struct_id, + &data.name, + CaseType::UpperCamelCase, + IdentType::Structure, + ); } - // 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( - struct_id, - struct_name_replacement, - struct_fields_replacements, - ); + // Check the field names. + self.validate_struct_fields(struct_id); } - /// 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( - &mut self, - struct_id: StructId, - struct_name_replacement: Option, - 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_name_replacement.is_none() && 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()); - if let Some(replacement) = struct_name_replacement { - let ast_ptr = match struct_src.value.name() { - Some(name) => name, - None => { + 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_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 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 without a name: {:?}", - replacement, + "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::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, - _ => { - 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 mut struct_fields_iter = struct_fields_list.fields(); - for field_to_rename 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 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, + ); } } @@ -572,163 +475,103 @@ fn validate_enum(&mut self, enum_id: EnumId) { } // 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 + // 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( - enum_id, - enum_name_replacement, - enum_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_enum( - &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.peek().is_none() { 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 => { + 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_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 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 a enum without a name: {:?}", - replacement, + "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::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, - _ => { - 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 mut enum_variants_iter = enum_variants_list.variants(); - for variant_to_rename 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 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, + ); } } fn validate_const(&mut self, const_id: ConstId) { - let data = self.db.const_data(const_id); + 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; + } 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) { @@ -742,32 +585,91 @@ fn validate_static(&mut self, static_id: StaticId) { return; } - let name = &data.name; + self.create_incorrect_case_diagnostic_for_item_name( + static_id, + &data.name, + CaseType::UpperSnakeCase, + IdentType::StaticVariable, + ); + } - 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. + 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); + self.create_incorrect_case_diagnostic_for_item_name( + type_alias_id, + &data.name, + CaseType::UpperCamelCase, + IdentType::TypeAlias, + ); + } + + 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 static_loc = static_id.lookup(self.db.upcast()); - let static_src = static_loc.source(self.db.upcast()); + 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 ast_ptr = match static_src.value.name() { - Some(name) => name, - None => return, + 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; }; let diagnostic = IncorrectCase { - file: static_src.file_id, - ident_type: IdentType::StaticVariable, - ident: AstPtr::new(&ast_ptr), + 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, @@ -775,4 +677,13 @@ fn validate_static(&mut self, static_id: StaticId) { 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/hir/src/lib.rs b/crates/hir/src/lib.rs index dbfacb2caa0..b2a793e53d0 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -563,6 +563,11 @@ pub fn diagnostics(self, db: &dyn HirDatabase, acc: &mut Vec) { for diag in db.trait_data_with_diagnostics(t.id).1.iter() { emit_def_diagnostic(db, acc, diag); } + + for item in t.items(db) { + item.diagnostics(db, acc); + } + acc.extend(def.diagnostics(db)) } ModuleDef::Adt(adt) => { @@ -730,13 +735,7 @@ pub fn diagnostics(self, db: &dyn HirDatabase, acc: &mut Vec) { } 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); } } } @@ -2651,6 +2650,22 @@ pub fn as_type_alias(self) -> Option { _ => 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 f5a6aa11979..96fbb4468f5 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 bool { false } @@ -403,7 +402,9 @@ fn Dirty(&self) -> bool { false } 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` } } } @@ -462,19 +463,59 @@ macro_rules! m { } #[test] - 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. + fn incorrect_trait_and_assoc_item_names() { 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` + 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(); + // ^^^^^^^^^^^ 💡 warn: Function `BadFunction` should have snake_case name, e.g. `bad_function` } "#, ); } + #[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); + check_diagnostics_with_disabled( + 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` + 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` + fn BadFunction(); + // ^^^^^^^^^^^ 💡 warn: Function `BadFunction` should have snake_case name, e.g. `bad_function` +} + +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` + } + fn BadFunction() {} +} + "#, + std::iter::once("unused_variables".to_string()), + ); + } + #[test] fn allow_attributes() { check_diagnostics( @@ -519,6 +560,14 @@ pub struct some_type { #[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(); +} "#, ); } @@ -578,6 +627,20 @@ pub struct some_type { #[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` +} "#, ); } diff --git a/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs b/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs index 66ebf593505..41c762c85b2 100644 --- a/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs +++ b/crates/ide-diagnostics/src/handlers/mismatched_arg_count.rs @@ -199,7 +199,7 @@ fn method_unknown_receiver() { // future, but we shouldn't emit an argument count diagnostic here check_diagnostics( r#" -trait Foo { fn method(&self, arg: usize) {} } +trait Foo { fn method(&self, _arg: usize) {} } fn f() { let x; diff --git a/crates/ide-diagnostics/src/tests.rs b/crates/ide-diagnostics/src/tests.rs index d8a796e01b1..c6f4d6be76c 100644 --- a/crates/ide-diagnostics/src/tests.rs +++ b/crates/ide-diagnostics/src/tests.rs @@ -186,6 +186,7 @@ fn check(minicore: MiniCore) { 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); }