Auto merge of #17588 - CamWass:more-rename, r=Veykril

feat: Add incorrect case diagnostics for enum variant fields and all variables/params

Updates the incorrect case diagnostic to check:

1. Fields of enum variants. Example:
```rust
enum Foo {
    Variant { nonSnake: u8 }
}
```
2. All variable bindings, instead of just let bindings and certain match arm patters. Examples:
```rust
match 1 { nonSnake => () }
match 1 { nonSnake @ 1 => () }
match 1 { nonSnake1 @ nonSnake2 => () } // slightly cursed, but these both introduce new
                                        // bindings that are bound to the same value.

const ONE: i32 = 1;
match 1 { nonSnake @ ONE } //  ONE is ignored since it is not a binding

match Some(1) { Some(nonSnake) => () }

struct Foo { field: u8 }
match (Foo { field: 1 } ) {
    Foo { field: nonSnake } => ();
}

struct Foo { nonSnake: u8 } // diagnostic here, at definition
match (Foo { nonSnake: 1 } ) { // no diagnostic here...
    Foo { nonSnake } => ();    // ...or here, since these are not where the name is introduced
}

for nonSnake in [] {}

struct Foo(u8);
for Foo(nonSnake) in [] {}
```
3. All parameter bindings, instead of just top-level binding identifiers. Examples:
```rust
fn func(nonSnake: u8) {} // worked before

struct Foo { field: u8 }
fn func(Foo { field: nonSnake }: Foo) {} // now get diagnostic for nonSnake
```

This is accomplished by changing the way binding identifier patterns are filtered:
- Previously, all binding idents were skipped, except a few classes of "good" binding locations that were checked.
- Now, all binding idents are checked, except field shorthands which are skipped.

Moving from a whitelist to a blacklist potentially makes the analysis more brittle:
If new pattern types are added in the future where ident pats don't introduce new names, then they may incorrectly create diagnostics.

But the benefit of the blacklist approach is simplicity: I think a whitelist approach would need to recursively visit patterns to collect renaming candidates?

Author: bors

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::<Vec<_>>(),
+                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) {

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; }
+}
+"#,
+        );
+    }
 }

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::<Never>::None {
-        None => (),
-        Some(never) => match never {},
+        Option::None => (),
+        Option::Some(never) => match never {},
     }
     match Option::<Never>::None {
         Option::Some(_never) => {},