From af30111b1c8af7abdaaa52120e24e088f15ff25b Mon Sep 17 00:00:00 2001 From: Campbell Date: Sat, 13 Jul 2024 21:54:22 +1200 Subject: [PATCH] feat: Add incorrect case diagnostics for enum variant fields and all variables --- crates/hir-ty/src/diagnostics/decl_check.rs | 93 +++++++++++-- .../src/handlers/incorrect_case.rs | 128 +++++++++++++++--- .../src/handlers/missing_match_arms.rs | 4 +- 3 files changed, 194 insertions(+), 31 deletions(-) diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs index 15ecf9aafc..1e6cb85d3b 100644 --- a/crates/hir-ty/src/diagnostics/decl_check.rs +++ b/crates/hir-ty/src/diagnostics/decl_check.rs @@ -17,8 +17,8 @@ use std::fmt; use hir_def::{ data::adt::VariantData, db::DefDatabase, hir::Pat, src::HasSource, AdtId, AttrDefId, ConstId, - EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, StaticId, StructId, - TraitId, TypeAliasId, + EnumId, EnumVariantId, FunctionId, ItemContainerId, Lookup, ModuleDefId, ModuleId, StaticId, + StructId, TraitId, TypeAliasId, }; use hir_expand::{ name::{AsName, Name}, @@ -353,17 +353,16 @@ impl<'a> DeclValidator<'a> { 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. + let is_shorthand = ast::RecordPatField::cast(parent.clone()) + .map(|parent| parent.name_ref().is_none()) + .unwrap_or_default(); + if is_shorthand { + // We don't check shorthand field patterns, such as 'field' in `Thing { field }`, + // since the shorthand isn't the declaration. continue; } + let is_param = ast::Param::can_cast(parent.kind()); let ident_type = if is_param { IdentType::Parameter } else { IdentType::Variable }; self.create_incorrect_case_diagnostic_for_ast_node( @@ -489,6 +488,11 @@ impl<'a> DeclValidator<'a> { /// Check incorrect names for enum variants. fn validate_enum_variants(&mut self, enum_id: EnumId) { let data = self.db.enum_data(enum_id); + + for (variant_id, _) in data.variants.iter() { + self.validate_enum_variant_fields(*variant_id); + } + let mut enum_variants_replacements = data .variants .iter() @@ -551,6 +555,75 @@ impl<'a> DeclValidator<'a> { } } + /// Check incorrect names for fields of enum variant. + fn validate_enum_variant_fields(&mut self, variant_id: EnumVariantId) { + let variant_data = self.db.enum_variant_data(variant_id); + let VariantData::Record(fields) = variant_data.variant_data.as_ref() else { + return; + }; + let mut variant_field_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 variant_field_replacements.peek().is_none() { + return; + } + + let variant_loc = variant_id.lookup(self.db.upcast()); + let variant_src = variant_loc.source(self.db.upcast()); + + let Some(ast::FieldList::RecordFieldList(variant_fields_list)) = + variant_src.value.field_list() + else { + always!( + variant_field_replacements.peek().is_none(), + "Replacements ({:?}) were generated for an enum variant \ + which had no fields list: {:?}", + variant_field_replacements.collect::>(), + variant_src + ); + return; + }; + let mut variant_variants_iter = variant_fields_list.fields(); + for field_replacement in variant_field_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) = variant_variants_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 an enum variant field \ + which was not found: {:?}", + field_replacement, + variant_src + ); + return; + } + }; + + self.create_incorrect_case_diagnostic_for_ast_node( + field_replacement, + variant_src.file_id, + &field, + IdentType::Field, + ); + } + } + fn validate_const(&mut self, const_id: ConstId) { let container = const_id.lookup(self.db.upcast()).container; if self.is_trait_impl_container(container) { diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs index a0fad7c850..18a95f0963 100644 --- a/crates/ide-diagnostics/src/handlers/incorrect_case.rs +++ b/crates/ide-diagnostics/src/handlers/incorrect_case.rs @@ -332,6 +332,7 @@ impl someStruct { check_diagnostics( r#" enum Option { Some, None } +use Option::{Some, None}; #[allow(unused)] fn main() { @@ -344,24 +345,6 @@ fn main() { ); } - #[test] - fn non_let_bind() { - check_diagnostics( - r#" -enum Option { Some, None } - -#[allow(unused)] -fn main() { - match Option::None { - SOME_VAR @ None => (), - // ^^^^^^^^ 💡 warn: Variable `SOME_VAR` should have snake_case name, e.g. `some_var` - Some => (), - } -} -"#, - ); - } - #[test] fn allow_attributes_crate_attr() { check_diagnostics( @@ -427,7 +410,12 @@ fn qualify() { #[test] // Issue #8809. fn parenthesized_parameter() { - check_diagnostics(r#"fn f((O): _) { _ = O; }"#) + check_diagnostics( + r#" +fn f((_O): u8) {} + // ^^ 💡 warn: Variable `_O` should have snake_case name, e.g. `_o` +"#, + ) } #[test] @@ -766,4 +754,106 @@ mod Foo; "#, ) } + + #[test] + fn test_field_shorthand() { + check_diagnostics( + r#" +struct Foo { _nonSnake: u8 } + // ^^^^^^^^^ 💡 warn: Field `_nonSnake` should have snake_case name, e.g. `_non_snake` +fn func(Foo { _nonSnake }: Foo) {} +"#, + ); + } + + #[test] + fn test_match() { + check_diagnostics( + r#" +enum Foo { Variant { nonSnake1: u8 } } + // ^^^^^^^^^ 💡 warn: Field `nonSnake1` should have snake_case name, e.g. `non_snake1` +fn func() { + match (Foo::Variant { nonSnake1: 1 }) { + Foo::Variant { nonSnake1: _nonSnake2 } => {}, + // ^^^^^^^^^^ 💡 warn: Variable `_nonSnake2` should have snake_case name, e.g. `_non_snake2` + } +} +"#, + ); + + check_diagnostics( + r#" +struct Foo(u8); + +fn func() { + match Foo(1) { + Foo(_nonSnake) => {}, + // ^^^^^^^^^ 💡 warn: Variable `_nonSnake` should have snake_case name, e.g. `_non_snake` + } +} +"#, + ); + + check_diagnostics( + r#" +fn main() { + match 1 { + _Bad1 @ _Bad2 => {} + // ^^^^^ 💡 warn: Variable `_Bad1` should have snake_case name, e.g. `_bad1` + // ^^^^^ 💡 warn: Variable `_Bad2` should have snake_case name, e.g. `_bad2` + } +} +"#, + ); + check_diagnostics( + r#" +fn main() { + match 1 { _Bad1 => () } + // ^^^^^ 💡 warn: Variable `_Bad1` should have snake_case name, e.g. `_bad1` +} +"#, + ); + + check_diagnostics( + r#" +enum Foo { V1, V2 } +use Foo::V1; + +fn main() { + match V1 { + _Bad1 @ V1 => {}, + // ^^^^^ 💡 warn: Variable `_Bad1` should have snake_case name, e.g. `_bad1` + Foo::V2 => {} + } +} +"#, + ); + } + + #[test] + fn test_for_loop() { + check_diagnostics( + r#" +//- minicore: iterators +fn func() { + for _nonSnake in [] {} + // ^^^^^^^^^ 💡 warn: Variable `_nonSnake` should have snake_case name, e.g. `_non_snake` +} +"#, + ); + + check_fix( + r#" +//- minicore: iterators +fn func() { + for nonSnake$0 in [] { nonSnake; } +} +"#, + r#" +fn func() { + for non_snake in [] { non_snake; } +} +"#, + ); + } } diff --git a/crates/ide-diagnostics/src/handlers/missing_match_arms.rs b/crates/ide-diagnostics/src/handlers/missing_match_arms.rs index 6d0119fb57..97296278c3 100644 --- a/crates/ide-diagnostics/src/handlers/missing_match_arms.rs +++ b/crates/ide-diagnostics/src/handlers/missing_match_arms.rs @@ -608,8 +608,8 @@ fn main() { // `Never` is deliberately not defined so that it's an uninferred type. // We ignore these to avoid triggering bugs in the analysis. match Option::::None { - None => (), - Some(never) => match never {}, + Option::None => (), + Option::Some(never) => match never {}, } match Option::::None { Option::Some(_never) => {},