Skip to content

fix: report incorrect-ident-case for inner items #15320

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 21, 2023
Merged

fix: report incorrect-ident-case for inner items #15320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b53a078
Report incorrect case for fn inner items
lowr Jul 21, 2023
33b7b45
Report incorrect case for inner items within all bodies
lowr Jul 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 37 additions & 32 deletions crates/hir-ty/src/diagnostics/decl_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand All @@ -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
}
Expand Down Expand Up @@ -120,7 +115,6 @@ pub struct IncorrectCase {

pub(super) struct DeclValidator<'a> {
db: &'a dyn HirDatabase,
krate: CrateId,
pub(super) sink: Vec<IncorrectCase>,
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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,
})
})
Expand Down Expand Up @@ -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;
}
Expand All @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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);
}
}
}
}
}
9 changes: 2 additions & 7 deletions crates/hir/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
Expand Down Expand Up @@ -1820,7 +1815,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())
}
}
Expand Down
96 changes: 96 additions & 0 deletions crates/ide-diagnostics/src/handlers/incorrect_case.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
},
}
"#,
);
}
}