From fc7b0eed9f0f7ddc4f6c269f4dc2922fb8f83113 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Sun, 21 Jun 2026 17:13:02 +0200 Subject: [PATCH] feat: lints for enum-name and constructor-name repetition --- crates/diagnostics/src/lint.rs | 29 +++ .../ast_walk/checks/enum_variant_names.rs | 69 ++++++ .../passes/lints/ast_walk/checks/helpers.rs | 8 +- .../src/passes/lints/ast_walk/checks/mod.rs | 4 + .../passes/lints/ast_walk/checks/naming.rs | 11 +- .../checks/self_named_constructors.rs | 61 +++++ .../passes/src/passes/lints/ast_walk/mod.rs | 31 +-- tests/ui/lint/mod.rs | 223 ++++++++++++++++++ .../snapshots/enum_variant_names_prefix.snap | 12 + .../snapshots/enum_variant_names_suffix.snap | 12 + .../self_named_constructors_fires.snap | 12 + 11 files changed, 450 insertions(+), 22 deletions(-) create mode 100644 crates/passes/src/passes/lints/ast_walk/checks/enum_variant_names.rs create mode 100644 crates/passes/src/passes/lints/ast_walk/checks/self_named_constructors.rs create mode 100644 tests/ui/lint/snapshots/enum_variant_names_prefix.snap create mode 100644 tests/ui/lint/snapshots/enum_variant_names_suffix.snap create mode 100644 tests/ui/lint/snapshots/self_named_constructors_fires.snap diff --git a/crates/diagnostics/src/lint.rs b/crates/diagnostics/src/lint.rs index 65a0673ba..7c86ee8df 100644 --- a/crates/diagnostics/src/lint.rs +++ b/crates/diagnostics/src/lint.rs @@ -1036,6 +1036,35 @@ pub fn miscased_screaming_snake(span: &Span, suggested_name: &str) -> LisetteDia .with_help(format!("Rename to `{}`", suggested_name)) } +pub fn enum_variant_names( + span: &Span, + enum_name: &str, + is_prefix: bool, + example_old: &str, + example_new: &str, +) -> LisetteDiagnostic { + let affix = if is_prefix { "prefix" } else { "suffix" }; + LisetteDiagnostic::info("Variant names repeat the enum name") + .with_lint_code("enum_variant_names") + .with_span_label(span, "each variant repeats this name") + .with_help(format!( + "Drop the `{enum_name}` {affix} from each variant (`{example_old}` to `{example_new}`). A variant is already written as `{enum_name}.{example_new}`, so the {affix} repeats the enum name." + )) +} + +pub fn self_named_constructors( + span: &Span, + type_name: &str, + method_name: &str, +) -> LisetteDiagnostic { + LisetteDiagnostic::info("Constructor named after its type") + .with_lint_code("self_named_constructors") + .with_span_label(span, "repeats the type name") + .with_help(format!( + "Calling this as `{type_name}.{method_name}()` repeats the type name. Rename it to `new`, the conventional constructor name." + )) +} + pub fn unused_field(span: &Span) -> LisetteDiagnostic { LisetteDiagnostic::warn("Unused field") .with_lint_code("unused_struct_field") diff --git a/crates/passes/src/passes/lints/ast_walk/checks/enum_variant_names.rs b/crates/passes/src/passes/lints/ast_walk/checks/enum_variant_names.rs new file mode 100644 index 000000000..e625bd8de --- /dev/null +++ b/crates/passes/src/passes/lints/ast_walk/checks/enum_variant_names.rs @@ -0,0 +1,69 @@ +use crate::passes::lints::ast_walk::casing::to_snake_case; +use crate::passes::walk::NodeCtx; +use syntax::ast::Expression; + +pub fn check_enum_variant_names(expression: &Expression, ctx: &NodeCtx) { + if ctx.is_d_lis { + return; + } + + let Expression::Enum { + name, + name_span, + variants, + .. + } = expression + else { + return; + }; + + if variants.len() < 2 { + return; + } + + let enum_words = snake_words(name); + if enum_words.is_empty() { + return; + } + + let variant_words: Vec> = variants.iter().map(|v| snake_words(&v.name)).collect(); + + let is_prefix = variant_words + .iter() + .all(|words| words.len() > enum_words.len() && words[..enum_words.len()] == enum_words[..]); + + let is_suffix = !is_prefix + && variant_words.iter().all(|words| { + words.len() > enum_words.len() + && words[words.len() - enum_words.len()..] == enum_words[..] + }); + + if !is_prefix && !is_suffix { + return; + } + + let cut: usize = enum_words.iter().map(|word| word.chars().count()).sum(); + let first = variants[0].name.as_str(); + let first_len = first.chars().count(); + let example_new: String = if is_prefix { + first.chars().skip(cut).collect() + } else { + first.chars().take(first_len - cut).collect() + }; + + ctx.sink.push(diagnostics::lint::enum_variant_names( + name_span, + name, + is_prefix, + first, + &example_new, + )); +} + +fn snake_words(name: &str) -> Vec { + to_snake_case(name) + .split('_') + .filter(|word| !word.is_empty()) + .map(String::from) + .collect() +} diff --git a/crates/passes/src/passes/lints/ast_walk/checks/helpers.rs b/crates/passes/src/passes/lints/ast_walk/checks/helpers.rs index 8cebb6559..292cb155a 100644 --- a/crates/passes/src/passes/lints/ast_walk/checks/helpers.rs +++ b/crates/passes/src/passes/lints/ast_walk/checks/helpers.rs @@ -1,5 +1,5 @@ use syntax::ast::{ - BinaryOperator, Expression, FormatStringPart, Literal, Pattern, Span, UnaryOperator, + BinaryOperator, Binding, Expression, FormatStringPart, Literal, Pattern, Span, UnaryOperator, }; use syntax::types::{SimpleKind, Type, unqualified_name}; @@ -10,6 +10,12 @@ pub(super) use crate::passes::comparison::{ expressions_equivalent, flip_comparison, is_side_effect_free, signed_integer_literal, }; +pub(super) fn first_param_is_self(params: &[Binding]) -> bool { + params.first().is_some_and(|param| { + matches!(¶m.pattern, Pattern::Identifier { identifier, .. } if identifier == "self") + }) +} + pub(super) fn is_float_operand(store: &Store, expression: &Expression) -> bool { let resolved = store.deep_resolve_alias(&expression.get_type()); if let Some(kind) = resolved.underlying_simple_kind() { diff --git a/crates/passes/src/passes/lints/ast_walk/checks/mod.rs b/crates/passes/src/passes/lints/ast_walk/checks/mod.rs index 503e00853..3fa0edfe1 100644 --- a/crates/passes/src/passes/lints/ast_walk/checks/mod.rs +++ b/crates/passes/src/passes/lints/ast_walk/checks/mod.rs @@ -11,6 +11,7 @@ mod dup_arg; mod duplicate_cutset; mod duplicate_logical_operand; mod empty_match_arm; +mod enum_variant_names; mod equal_operands; mod equatable_if_let; mod excess_parens_on_condition; @@ -79,6 +80,7 @@ mod replaceable_with_zero_fill; mod rest_only_slice_pattern; mod self_assignment; mod self_comparison; +mod self_named_constructors; mod single_arm_select; mod single_element_loop; mod type_limit_comparison; @@ -110,6 +112,7 @@ pub use dup_arg::check_dup_arg; pub use duplicate_cutset::check_duplicate_cutset; pub use duplicate_logical_operand::check_duplicate_logical_operand; pub use empty_match_arm::check_empty_match_arm; +pub use enum_variant_names::check_enum_variant_names; pub use equal_operands::check_equal_operands; pub use equatable_if_let::check_equatable_if_let; pub use excess_parens_on_condition::check_excess_parens_on_condition; @@ -179,6 +182,7 @@ pub use replaceable_with_zero_fill::check_replaceable_with_zero_fill; pub use rest_only_slice_pattern::check_rest_only_slice_pattern; pub use self_assignment::check_self_assignment; pub use self_comparison::check_self_comparison; +pub use self_named_constructors::check_self_named_constructors; pub use single_arm_select::check_single_arm_select; pub use single_element_loop::check_single_element_loop; pub use type_limit_comparison::check_type_limit_comparison; diff --git a/crates/passes/src/passes/lints/ast_walk/checks/naming.rs b/crates/passes/src/passes/lints/ast_walk/checks/naming.rs index 02b65a335..99e23e2ee 100644 --- a/crates/passes/src/passes/lints/ast_walk/checks/naming.rs +++ b/crates/passes/src/passes/lints/ast_walk/checks/naming.rs @@ -4,6 +4,8 @@ use syntax::ast::{Expression, Generic, Pattern, Span}; use crate::passes::lints::ast_walk::casing::{is_snake_case, to_pascal_case, to_snake_case}; use crate::passes::walk::NodeCtx; +use super::helpers::first_param_is_self; + pub fn check_expression_naming(expression: &Expression, ctx: &NodeCtx) { let sink = ctx.sink; let is_d_lis = ctx.is_d_lis; @@ -101,13 +103,8 @@ pub fn check_expression_naming(expression: &Expression, ctx: &NodeCtx) { params, .. } => { - if !is_d_lis { - let is_method = params.first().is_some_and(|p| { - matches!(&p.pattern, Pattern::Identifier { identifier, .. } if identifier == "self") - }); - if !is_method { - check_snake_case(name, name_span, "non_snake_case_function", sink); - } + if !is_d_lis && !first_param_is_self(params) { + check_snake_case(name, name_span, "non_snake_case_function", sink); } for generic in generics { diff --git a/crates/passes/src/passes/lints/ast_walk/checks/self_named_constructors.rs b/crates/passes/src/passes/lints/ast_walk/checks/self_named_constructors.rs new file mode 100644 index 000000000..ecabed09b --- /dev/null +++ b/crates/passes/src/passes/lints/ast_walk/checks/self_named_constructors.rs @@ -0,0 +1,61 @@ +use crate::passes::lints::ast_walk::casing::to_snake_case; +use crate::passes::walk::NodeCtx; +use syntax::ast::{Annotation, Expression}; + +use super::helpers::first_param_is_self; + +pub fn check_self_named_constructors(expression: &Expression, ctx: &NodeCtx) { + if ctx.is_d_lis { + return; + } + + let Expression::ImplBlock { + receiver_name, + methods, + .. + } = expression + else { + return; + }; + + if receiver_name.is_empty() { + return; + } + + let expected = to_snake_case(receiver_name); + + for method in methods { + let Expression::Function { + name, + name_span, + params, + return_annotation, + .. + } = method + else { + continue; + }; + + if first_param_is_self(params) { + continue; + } + + if name.as_str() != expected { + continue; + } + + let returns_self = matches!( + return_annotation, + Annotation::Constructor { name: returned, .. } if returned == receiver_name + ); + if !returns_self { + continue; + } + + ctx.sink.push(diagnostics::lint::self_named_constructors( + name_span, + receiver_name, + name, + )); + } +} diff --git a/crates/passes/src/passes/lints/ast_walk/mod.rs b/crates/passes/src/passes/lints/ast_walk/mod.rs index 5a240f34e..e5dd9d8b9 100644 --- a/crates/passes/src/passes/lints/ast_walk/mod.rs +++ b/crates/passes/src/passes/lints/ast_walk/mod.rs @@ -23,12 +23,13 @@ use checks::{ check_bool_literal_comparison, check_collapsible_else_if, check_collapsible_if, check_collapsible_match, check_double_comparison, check_double_negation, check_dup_arg, check_duplicate_cutset, check_duplicate_logical_operand, check_empty_match_arm, - check_equal_operands, check_equatable_if_let, check_excess_parens_on_condition, - check_exit_after_defer, check_expression_naming, check_float_cmp, - check_float_equality_without_abs, check_goos_goarch_comparison, check_identical_if_branches, - check_identical_match_arms, check_ineffective_bit_mask, check_integer_division_to_zero, - check_invisible_in_string_expression, check_invisible_in_string_pattern, check_let_and_return, - check_loop_runs_once, check_lost_cancel, check_lost_query_mutation, check_manual_bytes_equal, + check_enum_variant_names, check_equal_operands, check_equatable_if_let, + check_excess_parens_on_condition, check_exit_after_defer, check_expression_naming, + check_float_cmp, check_float_equality_without_abs, check_goos_goarch_comparison, + check_identical_if_branches, check_identical_match_arms, check_ineffective_bit_mask, + check_integer_division_to_zero, check_invisible_in_string_expression, + check_invisible_in_string_pattern, check_let_and_return, check_loop_runs_once, + check_lost_cancel, check_lost_query_mutation, check_manual_bytes_equal, check_manual_compound_assignment, check_manual_contains, check_manual_equal_fold, check_manual_filter, check_manual_find, check_manual_is_empty, check_manual_map, check_manual_map_or, check_manual_ok_err, check_manual_ok_or, check_manual_option_zip, @@ -44,14 +45,14 @@ use checks::{ check_redundant_guards, check_redundant_operation, check_redundant_pattern_matching, check_redundant_rebinding, check_redundant_slice_bounds, check_redundant_sprintf, check_regexp_in_loop, check_replaceable_with_zero_fill, check_rest_only_slice_pattern, - check_self_assignment, check_self_comparison, check_single_arm_select, - check_single_element_loop, check_type_limit_comparison, check_uninterpolated_fstring, - check_unnecessary_bool, check_unnecessary_first_then_check, check_unnecessary_lazy_evaluations, - check_unnecessary_map_on_constructor, check_unnecessary_min_or_max, - check_unnecessary_range_loop, check_unnecessary_raw_string_expression, - check_unnecessary_raw_string_pattern, check_unnecessary_return, check_unsigned_comparison, - check_verbose_failure_propagation, check_waitgroup_add_in_task, check_while_let_loop, - check_wildcard_in_or_patterns, + check_self_assignment, check_self_comparison, check_self_named_constructors, + check_single_arm_select, check_single_element_loop, check_type_limit_comparison, + check_uninterpolated_fstring, check_unnecessary_bool, check_unnecessary_first_then_check, + check_unnecessary_lazy_evaluations, check_unnecessary_map_on_constructor, + check_unnecessary_min_or_max, check_unnecessary_range_loop, + check_unnecessary_raw_string_expression, check_unnecessary_raw_string_pattern, + check_unnecessary_return, check_unsigned_comparison, check_verbose_failure_propagation, + check_waitgroup_add_in_task, check_while_let_loop, check_wildcard_in_or_patterns, }; static LINT_CHECKS: LazyLock = LazyLock::new(|| { @@ -155,6 +156,8 @@ static LINT_CHECKS: LazyLock = LazyLock::new(|| { check_expression_naming, &[Struct, Enum, TypeAlias, Interface, Function], ), + (check_enum_variant_names, &[Enum]), + (check_self_named_constructors, &[ImplBlock]), (check_replaceable_with_zero_fill, &[StructCall]), (check_lost_query_mutation, &[Call]), (check_lost_cancel, &[Let]), diff --git a/tests/ui/lint/mod.rs b/tests/ui/lint/mod.rs index 0e3c5a227..ef709ec28 100644 --- a/tests/ui/lint/mod.rs +++ b/tests/ui/lint/mod.rs @@ -18910,3 +18910,226 @@ fn main() { "# ); } + +#[test] +fn enum_variant_names_prefix() { + assert_lint_snapshot!( + r#" +enum Color { ColorRed, ColorGreen } + +fn describe(c: Color) -> int { + match c { + Color.ColorRed => 1, + Color.ColorGreen => 2, + } +} + +fn main() { + let _ = describe(Color.ColorRed) +} +"# + ); +} + +#[test] +fn enum_variant_names_suffix() { + assert_lint_snapshot!( + r#" +enum MyError { ParseMyError, IoMyError } + +fn code(e: MyError) -> int { + match e { + MyError.ParseMyError => 1, + MyError.IoMyError => 2, + } +} + +fn main() { + let _ = code(MyError.ParseMyError) +} +"# + ); +} + +#[test] +fn enum_variant_names_no_warning_distinct_variants() { + assert_no_lint_warnings!( + r#" +enum Color { Red, Green } + +fn pick(c: Color) -> int { + match c { + Color.Red => 1, + Color.Green => 2, + } +} + +fn main() { + let _ = pick(Color.Red) +} +"# + ); +} + +#[test] +fn enum_variant_names_no_warning_word_lookalike() { + assert_no_lint_warnings!( + r#" +enum Color { Colorful, Colorize } + +fn pick(c: Color) -> int { + match c { + Color.Colorful => 1, + Color.Colorize => 2, + } +} + +fn main() { + let _ = pick(Color.Colorful) +} +"# + ); +} + +#[test] +fn enum_variant_names_no_warning_partial_share() { + assert_no_lint_warnings!( + r#" +enum Mixed { MixedOne, Two } + +fn pick(m: Mixed) -> int { + match m { + Mixed.MixedOne => 1, + Mixed.Two => 2, + } +} + +fn main() { + let _ = pick(Mixed.MixedOne) +} +"# + ); +} + +#[test] +fn enum_variant_names_no_warning_single_variant() { + assert_no_lint_warnings!( + r#" +enum Wrapper { WrapperInner } + +fn pick(w: Wrapper) -> int { + match w { + Wrapper.WrapperInner => 1, + } +} + +fn main() { + let _ = pick(Wrapper.WrapperInner) +} +"# + ); +} + +#[test] +fn enum_variant_names_no_warning_variant_equals_enum() { + assert_no_lint_warnings!( + r#" +enum Exact { Exact, ExactMore } + +fn pick(e: Exact) -> int { + match e { + Exact.Exact => 1, + Exact.ExactMore => 2, + } +} + +fn main() { + let _ = pick(Exact.Exact) +} +"# + ); +} + +#[test] +fn self_named_constructors_fires() { + assert_lint_snapshot!( + r#" +struct Point { x: int } + +impl Point { + fn point(x: int) -> Point { + Point { x } + } +} + +fn main() { + let p = Point.point(1) + let _ = p.x +} +"# + ); +} + +#[test] +fn self_named_constructors_no_warning_new() { + assert_no_lint_warnings!( + r#" +struct Point { x: int } + +impl Point { + fn new(x: int) -> Point { + Point { x } + } +} + +fn main() { + let p = Point.new(1) + let _ = p.x +} +"# + ); +} + +#[test] +fn self_named_constructors_no_warning_instance_method() { + assert_no_lint_warnings!( + r#" +struct Point { x: int } + +impl Point { + fn point(self) -> int { + self.x + } +} + +fn main() { + let p = Point { x: 1 } + let _ = p.point() +} +"# + ); +} + +#[test] +fn self_named_constructors_no_warning_returns_other_type() { + assert_no_lint_warnings!( + r#" +struct Widget { id: int } + +impl Widget { + fn widget() -> int { + 5 + } + fn build(id: int) -> Widget { + Widget { id } + } +} + +fn main() { + let _ = Widget.widget() + let w = Widget.build(1) + let _ = w.id +} +"# + ); +} diff --git a/tests/ui/lint/snapshots/enum_variant_names_prefix.snap b/tests/ui/lint/snapshots/enum_variant_names_prefix.snap new file mode 100644 index 000000000..9b3b1e5fe --- /dev/null +++ b/tests/ui/lint/snapshots/enum_variant_names_prefix.snap @@ -0,0 +1,12 @@ +--- +source: tests/ui/lint/mod.rs +--- + [info] Variant names repeat the enum name + ╭─[test.lis:2:6] + 1 │ + 2 │ enum Color { ColorRed, ColorGreen } + · ──┬── + · ╰── each variant repeats this name + 3 │ + ╰──── + help: Drop the `Color` prefix from each variant (`ColorRed` to `Red`). A variant is already written as `Color.Red`, so the prefix repeats the enum name · code: [lint.enum_variant_names] diff --git a/tests/ui/lint/snapshots/enum_variant_names_suffix.snap b/tests/ui/lint/snapshots/enum_variant_names_suffix.snap new file mode 100644 index 000000000..e8caef238 --- /dev/null +++ b/tests/ui/lint/snapshots/enum_variant_names_suffix.snap @@ -0,0 +1,12 @@ +--- +source: tests/ui/lint/mod.rs +--- + [info] Variant names repeat the enum name + ╭─[test.lis:2:6] + 1 │ + 2 │ enum MyError { ParseMyError, IoMyError } + · ───┬─── + · ╰── each variant repeats this name + 3 │ + ╰──── + help: Drop the `MyError` suffix from each variant (`ParseMyError` to `Parse`). A variant is already written as `MyError.Parse`, so the suffix repeats the enum name · code: [lint.enum_variant_names] diff --git a/tests/ui/lint/snapshots/self_named_constructors_fires.snap b/tests/ui/lint/snapshots/self_named_constructors_fires.snap new file mode 100644 index 000000000..7e5d8491b --- /dev/null +++ b/tests/ui/lint/snapshots/self_named_constructors_fires.snap @@ -0,0 +1,12 @@ +--- +source: tests/ui/lint/mod.rs +--- + [info] Constructor named after its type + ╭─[test.lis:5:6] + 4 │ impl Point { + 5 │ fn point(x: int) -> Point { + · ──┬── + · ╰── repeats the type name + 6 │ Point { x } + ╰──── + help: Calling this as `Point.point()` repeats the type name. Rename it to `new`, the conventional constructor name · code: [lint.self_named_constructors]