Auto merge of #15320 - lowr:fix/incorrect-name-case-for-inner-items, r=HKalbasi

bors · bors · commit 59d35d24a74c · 2023-07-21T06:41:30.000Z
Report `incorrect-ident-case` for inner items Fixes #15319 Although we have been collecting the diagnostics for inner items within function bodies, we were discarding them and never reported to the users. This PR makes sure that they are all reported and additionally collects the diagnostics for inner items within const bodies, static bodies, and enum variant bodies.
diff --git a/crates/hir-ty/src/diagnostics/decl_check.rs b/crates/hir-ty/src/diagnostics/decl_check.rs
@@ -14,13 +14,12 @@ mod case_conv;
 
 use std::fmt;
 
-use base_db::CrateId;
 use hir_def::{
     data::adt::VariantData,
     hir::{Pat, PatId},
     src::HasSource,
-    AdtId, AttrDefId, ConstId, EnumId, FunctionId, ItemContainerId, Lookup, ModuleDefId, StaticId,
-    StructId,
+    AdtId, AttrDefId, ConstId, DefWithBodyId, EnumId, EnumVariantId, FunctionId, ItemContainerId,
+    Lookup, ModuleDefId, StaticId, StructId,
 };
 use hir_expand::{
     name::{AsName, Name},
@@ -44,13 +43,9 @@ mod allow {
     pub(super) const NON_CAMEL_CASE_TYPES: &str = "non_camel_case_types";
 }
 
-pub fn incorrect_case(
-    db: &dyn HirDatabase,
-    krate: CrateId,
-    owner: ModuleDefId,
-) -> Vec<IncorrectCase> {
+pub fn incorrect_case(db: &dyn HirDatabase, owner: ModuleDefId) -> Vec<IncorrectCase> {
     let _p = profile::span("validate_module_item");
-    let mut validator = DeclValidator::new(db, krate);
+    let mut validator = DeclValidator::new(db);
     validator.validate_item(owner);
     validator.sink
 }
@@ -120,7 +115,6 @@ pub struct IncorrectCase {
 
 pub(super) struct DeclValidator<'a> {
     db: &'a dyn HirDatabase,
-    krate: CrateId,
     pub(super) sink: Vec<IncorrectCase>,
 }
 
@@ -132,8 +126,8 @@ struct Replacement {
 }
 
 impl<'a> DeclValidator<'a> {
-    pub(super) fn new(db: &'a dyn HirDatabase, krate: CrateId) -> DeclValidator<'a> {
-        DeclValidator { db, krate, sink: Vec::new() }
+    pub(super) fn new(db: &'a dyn HirDatabase) -> DeclValidator<'a> {
+        DeclValidator { db, sink: Vec::new() }
     }
 
     pub(super) fn validate_item(&mut self, item: ModuleDefId) {
@@ -195,8 +189,7 @@ impl<'a> DeclValidator<'a> {
                 AttrDefId::TypeAliasId(_) => None,
                 AttrDefId::GenericParamId(_) => None,
             }
-            .map(|mid| self.allowed(mid, allow_name, true))
-            .unwrap_or(false)
+            .is_some_and(|mid| self.allowed(mid, allow_name, true))
     }
 
     fn validate_func(&mut self, func: FunctionId) {
@@ -206,17 +199,7 @@ impl<'a> DeclValidator<'a> {
             return;
         }
 
-        let body = self.db.body(func.into());
-
-        // Recursively validate inner scope items, such as static variables and constants.
-        for (_, block_def_map) in body.blocks(self.db.upcast()) {
-            for (_, module) in block_def_map.modules() {
-                for def_id in module.scope.declarations() {
-                    let mut validator = DeclValidator::new(self.db, self.krate);
-                    validator.validate_item(def_id);
-                }
-            }
-        }
+        self.validate_body_inner_items(func.into());
 
         // Check whether non-snake case identifiers are allowed for this function.
         if self.allowed(func.into(), allow::NON_SNAKE_CASE, false) {
@@ -231,6 +214,8 @@ impl<'a> DeclValidator<'a> {
             expected_case: CaseType::LowerSnakeCase,
         });
 
+        let body = self.db.body(func.into());
+
         // Check the patterns inside the function body.
         // This includes function parameters.
         let pats_replacements = body
@@ -496,6 +481,11 @@ impl<'a> DeclValidator<'a> {
     fn validate_enum(&mut self, enum_id: EnumId) {
         let data = self.db.enum_data(enum_id);
 
+        for (local_id, _) in data.variants.iter() {
+            let variant_id = EnumVariantId { parent: enum_id, local_id };
+            self.validate_body_inner_items(variant_id.into());
+        }
+
         // Check whether non-camel case names are allowed for this enum.
         if self.allowed(enum_id.into(), allow::NON_CAMEL_CASE_TYPES, false) {
             return;
@@ -512,13 +502,11 @@ impl<'a> DeclValidator<'a> {
         // Check the field names.
         let enum_fields_replacements = data
             .variants
-            .iter()
-            .filter_map(|(_, variant)| {
+            .values()
+            .filter_map(|variant| {
                 Some(Replacement {
                     current_name: variant.name.clone(),
-                    suggested_text: to_camel_case(
-                        &variant.name.display(self.db.upcast()).to_string(),
-                    )?,
+                    suggested_text: to_camel_case(&variant.name.to_smol_str())?,
                     expected_case: CaseType::UpperCamelCase,
                 })
             })
@@ -622,6 +610,8 @@ impl<'a> DeclValidator<'a> {
     fn validate_const(&mut self, const_id: ConstId) {
         let data = self.db.const_data(const_id);
 
+        self.validate_body_inner_items(const_id.into());
+
         if self.allowed(const_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) {
             return;
         }
@@ -631,7 +621,7 @@ impl<'a> DeclValidator<'a> {
             None => return,
         };
 
-        let const_name = name.display(self.db.upcast()).to_string();
+        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(),
@@ -670,13 +660,15 @@ impl<'a> DeclValidator<'a> {
             return;
         }
 
+        self.validate_body_inner_items(static_id.into());
+
         if self.allowed(static_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) {
             return;
         }
 
         let name = &data.name;
 
-        let static_name = name.display(self.db.upcast()).to_string();
+        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(),
@@ -707,4 +699,17 @@ impl<'a> DeclValidator<'a> {
 
         self.sink.push(diagnostic);
     }
+
+    // FIXME: We don't currently validate names within `DefWithBodyId::InTypeConstId`.
+    /// Recursively validates inner scope items, such as static variables and constants.
+    fn validate_body_inner_items(&mut self, body_id: DefWithBodyId) {
+        let body = self.db.body(body_id);
+        for (_, block_def_map) in body.blocks(self.db.upcast()) {
+            for (_, module) in block_def_map.modules() {
+                for def_id in module.scope.declarations() {
+                    self.validate_item(def_id);
+                }
+            }
+        }
+    }
 }
diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs
@@ -378,19 +378,14 @@ impl ModuleDef {
             ModuleDef::BuiltinType(_) | ModuleDef::Macro(_) => return Vec::new(),
         };
 
-        let module = match self.module(db) {
-            Some(it) => it,
-            None => return Vec::new(),
-        };
-
         let mut acc = Vec::new();
 
         match self.as_def_with_body() {
             Some(def) => {
                 def.diagnostics(db, &mut acc);
             }
             None => {
-                for diag in hir_ty::diagnostics::incorrect_case(db, module.id.krate(), id) {
+                for diag in hir_ty::diagnostics::incorrect_case(db, id) {
                     acc.push(diag.into())
                 }
             }
@@ -1831,7 +1826,7 @@ impl DefWithBody {
             // FIXME: don't ignore diagnostics for in type const
             DefWithBody::InTypeConst(_) => return,
         };
-        for diag in hir_ty::diagnostics::incorrect_case(db, krate, def.into()) {
+        for diag in hir_ty::diagnostics::incorrect_case(db, def.into()) {
             acc.push(diag.into())
         }
     }
diff --git a/crates/ide-diagnostics/src/handlers/incorrect_case.rs b/crates/ide-diagnostics/src/handlers/incorrect_case.rs
@@ -545,4 +545,100 @@ pub static SomeStatic: u8 = 10;
     "#,
         );
     }
+
+    #[test]
+    fn fn_inner_items() {
+        check_diagnostics(
+            r#"
+fn main() {
+    const foo: bool = true;
+        //^^^ 💡 warn: Constant `foo` should have UPPER_SNAKE_CASE name, e.g. `FOO`
+    static bar: bool = true;
+         //^^^ 💡 warn: Static variable `bar` should have UPPER_SNAKE_CASE name, e.g. `BAR`
+    fn BAZ() {
+     //^^^ 💡 warn: Function `BAZ` should have snake_case name, e.g. `baz`
+        const foo: bool = true;
+            //^^^ 💡 warn: Constant `foo` should have UPPER_SNAKE_CASE name, e.g. `FOO`
+        static bar: bool = true;
+             //^^^ 💡 warn: Static variable `bar` should have UPPER_SNAKE_CASE name, e.g. `BAR`
+        fn BAZ() {
+         //^^^ 💡 warn: Function `BAZ` should have snake_case name, e.g. `baz`
+            let INNER_INNER = 42;
+              //^^^^^^^^^^^ 💡 warn: Variable `INNER_INNER` should have snake_case name, e.g. `inner_inner`
+        }
+
+        let INNER_LOCAL = 42;
+          //^^^^^^^^^^^ 💡 warn: Variable `INNER_LOCAL` should have snake_case name, e.g. `inner_local`
+    }
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn const_body_inner_items() {
+        check_diagnostics(
+            r#"
+const _: () = {
+    static bar: bool = true;
+         //^^^ 💡 warn: Static variable `bar` should have UPPER_SNAKE_CASE name, e.g. `BAR`
+    fn BAZ() {}
+     //^^^ 💡 warn: Function `BAZ` should have snake_case name, e.g. `baz`
+
+    const foo: () = {
+        //^^^ 💡 warn: Constant `foo` should have UPPER_SNAKE_CASE name, e.g. `FOO`
+        const foo: bool = true;
+            //^^^ 💡 warn: Constant `foo` should have UPPER_SNAKE_CASE name, e.g. `FOO`
+        static bar: bool = true;
+             //^^^ 💡 warn: Static variable `bar` should have UPPER_SNAKE_CASE name, e.g. `BAR`
+        fn BAZ() {}
+         //^^^ 💡 warn: Function `BAZ` should have snake_case name, e.g. `baz`
+    };
+};
+"#,
+        );
+    }
+
+    #[test]
+    fn static_body_inner_items() {
+        check_diagnostics(
+            r#"
+static FOO: () = {
+    const foo: bool = true;
+        //^^^ 💡 warn: Constant `foo` should have UPPER_SNAKE_CASE name, e.g. `FOO`
+    fn BAZ() {}
+     //^^^ 💡 warn: Function `BAZ` should have snake_case name, e.g. `baz`
+
+    static bar: () = {
+         //^^^ 💡 warn: Static variable `bar` should have UPPER_SNAKE_CASE name, e.g. `BAR`
+        const foo: bool = true;
+            //^^^ 💡 warn: Constant `foo` should have UPPER_SNAKE_CASE name, e.g. `FOO`
+        static bar: bool = true;
+             //^^^ 💡 warn: Static variable `bar` should have UPPER_SNAKE_CASE name, e.g. `BAR`
+        fn BAZ() {}
+         //^^^ 💡 warn: Function `BAZ` should have snake_case name, e.g. `baz`
+    };
+};
+"#,
+        );
+    }
+
+    #[test]
+    fn enum_variant_body_inner_item() {
+        check_diagnostics(
+            r#"
+enum E {
+    A = {
+        const foo: bool = true;
+            //^^^ 💡 warn: Constant `foo` should have UPPER_SNAKE_CASE name, e.g. `FOO`
+        static bar: bool = true;
+             //^^^ 💡 warn: Static variable `bar` should have UPPER_SNAKE_CASE name, e.g. `BAR`
+        fn BAZ() {}
+         //^^^ 💡 warn: Function `BAZ` should have snake_case name, e.g. `baz`
+        42
+    },
+}
+"#,
+        );
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -378,19 +378,14 @@ impl ModuleDef {`
`378`	`378`	`ModuleDef::BuiltinType(_) \| ModuleDef::Macro(_) => return Vec::new(),`
`379`	`379`	`};`
`380`	`380`
`381`		`- let module = match self.module(db) {`
`382`		`- Some(it) => it,`
`383`		`- None => return Vec::new(),`
`384`		`- };`
`385`		`-`
`386`	`381`	`let mut acc = Vec::new();`
`387`	`382`
`388`	`383`	`match self.as_def_with_body() {`
`389`	`384`	`Some(def) => {`
`390`	`385`	`def.diagnostics(db, &mut acc);`
`391`	`386`	`}`
`392`	`387`	`None => {`
`393`		`- for diag in hir_ty::diagnostics::incorrect_case(db, module.id.krate(), id) {`
	`388`	`+ for diag in hir_ty::diagnostics::incorrect_case(db, id) {`
`394`	`389`	`acc.push(diag.into())`
`395`	`390`	`}`
`396`	`391`	`}`
`@@ -1831,7 +1826,7 @@ impl DefWithBody {`
`1831`	`1826`	`// FIXME: don't ignore diagnostics for in type const`
`1832`	`1827`	`DefWithBody::InTypeConst(_) => return,`
`1833`	`1828`	`};`
`1834`		`- for diag in hir_ty::diagnostics::incorrect_case(db, krate, def.into()) {`
	`1829`	`+ for diag in hir_ty::diagnostics::incorrect_case(db, def.into()) {`
`1835`	`1830`	`acc.push(diag.into())`
`1836`	`1831`	`}`
`1837`	`1832`	`}`