From 821603619344517b29ea5f9de0e1fd48dc8aecb4 Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 9 Sep 2016 21:40:30 +0200 Subject: [PATCH 1/4] Add test for `#[allow(module_inception)]` on the inner module --- tests/compile-fail/module_inception.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/compile-fail/module_inception.rs b/tests/compile-fail/module_inception.rs index 88497c9a6a7d..861ed504c86e 100644 --- a/tests/compile-fail/module_inception.rs +++ b/tests/compile-fail/module_inception.rs @@ -14,4 +14,11 @@ mod foo { } } +// No warning. See . +mod bar { + #[allow(module_inception)] + mod bar { + } +} + fn main() {} From 03fa9748553c4939b76082b60438136ba3a0dfba Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 12 Sep 2016 10:30:42 +0200 Subject: [PATCH 2/4] Let the submodule `#[allow]` in `module_inception` --- clippy_lints/src/enum_variants.rs | 35 +++++++++++++++--- clippy_lints/src/lib.rs | 4 +-- clippy_lints/src/module_inception.rs | 50 -------------------------- tests/compile-fail/module_inception.rs | 12 +++++-- 4 files changed, 41 insertions(+), 60 deletions(-) delete mode 100644 clippy_lints/src/module_inception.rs diff --git a/clippy_lints/src/enum_variants.rs b/clippy_lints/src/enum_variants.rs index 17c3de480480..8211621fa0e0 100644 --- a/clippy_lints/src/enum_variants.rs +++ b/clippy_lints/src/enum_variants.rs @@ -47,8 +47,32 @@ declare_lint! { "type names prefixed/postfixed with their containing module's name" } +/// **What it does:** Checks for modules that have the same name as their parent module +/// +/// **Why is this bad?** A typical beginner mistake is to have `mod foo;` and again `mod foo { .. }` in `foo.rs`. +/// The expectation is that items inside the inner `mod foo { .. }` are then available +/// through `foo::x`, but they are only available through `foo::foo::x`. +/// If this is done on purpose, it would be better to choose a more representative module name. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// // lib.rs +/// mod foo; +/// // foo.rs +/// mod foo { +/// ... +/// } +/// ``` +declare_lint! { + pub MODULE_INCEPTION, + Warn, + "modules that have the same name as their parent module" +} + pub struct EnumVariantNames { - modules: Vec, + modules: Vec<(InternedString, String)>, threshold: u64, } @@ -60,7 +84,7 @@ impl EnumVariantNames { impl LintPass for EnumVariantNames { fn get_lints(&self) -> LintArray { - lint_array!(ENUM_VARIANT_NAMES, STUTTER) + lint_array!(ENUM_VARIANT_NAMES, STUTTER, MODULE_INCEPTION) } } @@ -171,9 +195,12 @@ impl EarlyLintPass for EnumVariantNames { let item_name_chars = item_name.chars().count(); let item_camel = to_camel_case(&item_name); if item.vis == Visibility::Public && !in_macro(cx, item.span) { - if let Some(mod_camel) = self.modules.last() { + if let Some(&(ref mod_name, ref mod_camel)) = self.modules.last() { // constants don't have surrounding modules if !mod_camel.is_empty() { + if mod_name == &item_name { + span_lint(cx, MODULE_INCEPTION, item.span, "item has the same name as its containing module"); + } let matching = partial_match(mod_camel, &item_camel); let rmatching = partial_rmatch(mod_camel, &item_camel); let nchars = mod_camel.chars().count(); @@ -189,6 +216,6 @@ impl EarlyLintPass for EnumVariantNames { if let ItemKind::Enum(ref def, _) = item.node { check_variant(cx, self.threshold, def, &item_name, item_name_chars, item.span); } - self.modules.push(item_camel); + self.modules.push((item_name, item_camel)); } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 36d466ed9c95..46a2a737d7c1 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -96,7 +96,6 @@ pub mod minmax; pub mod misc; pub mod misc_early; pub mod missing_doc; -pub mod module_inception; pub mod mut_mut; pub mod mut_reference; pub mod mutex_atomic; @@ -175,7 +174,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box utils::internal_lints::LintWithoutLintPass::default()); reg.register_late_lint_pass(box types::TypePass); reg.register_late_lint_pass(box booleans::NonminimalBool); - reg.register_early_lint_pass(box module_inception::Pass); reg.register_late_lint_pass(box eq_op::EqOp); reg.register_early_lint_pass(box enum_variants::EnumVariantNames::new(conf.enum_variant_name_threshold)); reg.register_late_lint_pass(box enum_glob_use::EnumGlobUse); @@ -329,6 +327,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { entry::MAP_ENTRY, enum_clike::ENUM_CLIKE_UNPORTABLE_VARIANT, enum_variants::ENUM_VARIANT_NAMES, + enum_variants::MODULE_INCEPTION, eq_op::EQ_OP, escape::BOXED_LOCAL, eta_reduction::REDUNDANT_CLOSURE, @@ -391,7 +390,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { misc_early::REDUNDANT_CLOSURE_CALL, misc_early::UNNEEDED_FIELD_PATTERN, misc_early::ZERO_PREFIXED_LITERAL, - module_inception::MODULE_INCEPTION, mut_reference::UNNECESSARY_MUT_PASSED, mutex_atomic::MUTEX_ATOMIC, needless_bool::BOOL_COMPARISON, diff --git a/clippy_lints/src/module_inception.rs b/clippy_lints/src/module_inception.rs deleted file mode 100644 index 10c8154d100e..000000000000 --- a/clippy_lints/src/module_inception.rs +++ /dev/null @@ -1,50 +0,0 @@ -use rustc::lint::*; -use syntax::ast::*; -use utils::span_lint; - -/// **What it does:** Checks for modules that have the same name as their parent module -/// -/// **Why is this bad?** A typical beginner mistake is to have `mod foo;` and again `mod foo { .. }` in `foo.rs`. -/// The expectation is that items inside the inner `mod foo { .. }` are then available -/// through `foo::x`, but they are only available through `foo::foo::x`. -/// If this is done on purpose, it would be better to choose a more representative module name. -/// -/// **Known problems:** None. -/// -/// **Example:** -/// ```rust -/// // lib.rs -/// mod foo; -/// // foo.rs -/// mod foo { -/// ... -/// } -/// ``` -declare_lint! { - pub MODULE_INCEPTION, - Warn, - "modules that have the same name as their parent module" -} - -pub struct Pass; - -impl LintPass for Pass { - fn get_lints(&self) -> LintArray { - lint_array![MODULE_INCEPTION] - } -} - -impl EarlyLintPass for Pass { - fn check_item(&mut self, cx: &EarlyContext, item: &Item) { - if let ItemKind::Mod(ref module) = item.node { - for sub_item in &module.items { - if let ItemKind::Mod(_) = sub_item.node { - if item.ident == sub_item.ident { - span_lint(cx, MODULE_INCEPTION, sub_item.span, - "module has the same name as its containing module"); - } - } - } - } - } -} diff --git a/tests/compile-fail/module_inception.rs b/tests/compile-fail/module_inception.rs index 861ed504c86e..f6228cbb7950 100644 --- a/tests/compile-fail/module_inception.rs +++ b/tests/compile-fail/module_inception.rs @@ -4,20 +4,26 @@ mod foo { mod bar { - mod bar { //~ ERROR module has the same name as its containing module + pub mod bar { //~ ERROR item has the same name as its containing module mod foo {} } mod foo {} } - mod foo { //~ ERROR module has the same name as its containing module + pub mod foo { //~ ERROR item has the same name as its containing module mod bar {} } } +mod cake { + mod cake { + // no error, since module is not public + } +} + // No warning. See . mod bar { #[allow(module_inception)] - mod bar { + pub mod bar { } } From 12a82b2007b78594369ffd3ce089437dfe62dab5 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 13 Sep 2016 10:19:55 +0200 Subject: [PATCH 3/4] also lint private modules for module_inception, as that is the main issue --- clippy_lints/src/enum_variants.rs | 24 ++++++++++++++---------- tests/compile-fail/module_inception.rs | 12 +++--------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/enum_variants.rs b/clippy_lints/src/enum_variants.rs index 8211621fa0e0..69ec9911d99a 100644 --- a/clippy_lints/src/enum_variants.rs +++ b/clippy_lints/src/enum_variants.rs @@ -194,21 +194,25 @@ impl EarlyLintPass for EnumVariantNames { let item_name = item.ident.name.as_str(); let item_name_chars = item_name.chars().count(); let item_camel = to_camel_case(&item_name); - if item.vis == Visibility::Public && !in_macro(cx, item.span) { + if !in_macro(cx, item.span) { if let Some(&(ref mod_name, ref mod_camel)) = self.modules.last() { // constants don't have surrounding modules if !mod_camel.is_empty() { if mod_name == &item_name { - span_lint(cx, MODULE_INCEPTION, item.span, "item has the same name as its containing module"); + if let ItemKind::Mod(..) = item.node { + span_lint(cx, MODULE_INCEPTION, item.span, "module has the same name as its containing module"); + } } - let matching = partial_match(mod_camel, &item_camel); - let rmatching = partial_rmatch(mod_camel, &item_camel); - let nchars = mod_camel.chars().count(); - if matching == nchars { - span_lint(cx, STUTTER, item.span, &format!("Item name ({}) starts with its containing module's name ({})", item_camel, mod_camel)); - } - if rmatching == nchars { - span_lint(cx, STUTTER, item.span, &format!("Item name ({}) ends with its containing module's name ({})", item_camel, mod_camel)); + if item.vis == Visibility::Public { + let matching = partial_match(mod_camel, &item_camel); + let rmatching = partial_rmatch(mod_camel, &item_camel); + let nchars = mod_camel.chars().count(); + if matching == nchars { + span_lint(cx, STUTTER, item.span, "item name starts with its containing module's name"); + } + if rmatching == nchars { + span_lint(cx, STUTTER, item.span, "item name ends with its containing module's name"); + } } } } diff --git a/tests/compile-fail/module_inception.rs b/tests/compile-fail/module_inception.rs index f6228cbb7950..861ed504c86e 100644 --- a/tests/compile-fail/module_inception.rs +++ b/tests/compile-fail/module_inception.rs @@ -4,26 +4,20 @@ mod foo { mod bar { - pub mod bar { //~ ERROR item has the same name as its containing module + mod bar { //~ ERROR module has the same name as its containing module mod foo {} } mod foo {} } - pub mod foo { //~ ERROR item has the same name as its containing module + mod foo { //~ ERROR module has the same name as its containing module mod bar {} } } -mod cake { - mod cake { - // no error, since module is not public - } -} - // No warning. See . mod bar { #[allow(module_inception)] - pub mod bar { + mod bar { } } From 40ce3a8f1c62ec403ac9790c0d45f964efe2c504 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 13 Sep 2016 10:20:10 +0200 Subject: [PATCH 4/4] add tests for stutter lints --- tests/compile-fail/stutter.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/compile-fail/stutter.rs diff --git a/tests/compile-fail/stutter.rs b/tests/compile-fail/stutter.rs new file mode 100644 index 000000000000..0c99859c10d8 --- /dev/null +++ b/tests/compile-fail/stutter.rs @@ -0,0 +1,14 @@ +#![feature(plugin)] +#![plugin(clippy)] +#![deny(stutter)] +#![allow(dead_code)] + +mod foo { + pub fn foo() {} + pub fn foo_bar() {} //~ ERROR: item name starts with its containing module's name + pub fn bar_foo() {} //~ ERROR: item name ends with its containing module's name + pub struct FooCake {} //~ ERROR: item name starts with its containing module's name + pub enum CakeFoo {} //~ ERROR: item name ends with its containing module's name +} + +fn main() {}