From 4529f1be81b3b1424447908f2446776f44748fcd Mon Sep 17 00:00:00 2001 From: Jade Date: Thu, 8 Apr 2021 04:37:34 -0700 Subject: decl_check: consider outer scopes' allows Fix #8417. Also makes it less noisy about no_mangle annotated stuff the user can do nothing about. Note: this still is broken with bitfield! macros. A repro in an ignore test is included here. I believe this bug is elsewhere, and I don't think I can work around it here. --- crates/hir_ty/src/diagnostics/decl_check.rs | 132 +++++++++++++++++++++++++--- 1 file changed, 122 insertions(+), 10 deletions(-) (limited to 'crates/hir_ty') diff --git a/crates/hir_ty/src/diagnostics/decl_check.rs b/crates/hir_ty/src/diagnostics/decl_check.rs index 1c9f9ede7..31532f327 100644 --- a/crates/hir_ty/src/diagnostics/decl_check.rs +++ b/crates/hir_ty/src/diagnostics/decl_check.rs @@ -35,6 +35,8 @@ use crate::{ }; mod allow { + pub(super) const BAD_STYLE: &str = "bad_style"; + pub(super) const NONSTANDARD_STYLE: &str = "nonstandard_style"; pub(super) const NON_SNAKE_CASE: &str = "non_snake_case"; pub(super) const NON_UPPER_CASE_GLOBAL: &str = "non_upper_case_globals"; pub(super) const NON_CAMEL_CASE_TYPES: &str = "non_camel_case_types"; @@ -83,10 +85,39 @@ impl<'a, 'b> DeclValidator<'a, 'b> { } /// Checks whether not following the convention is allowed for this item. - /// - /// Currently this method doesn't check parent attributes. - fn allowed(&self, id: AttrDefId, allow_name: &str) -> bool { - self.db.attrs(id).by_key("allow").tt_values().any(|tt| tt.to_string().contains(allow_name)) + fn allowed(&self, id: AttrDefId, allow_name: &str, recursing: bool) -> bool { + let is_allowed = |def_id| { + let attrs = self.db.attrs(def_id); + // don't bug the user about directly no_mangle annotated stuff, they can't do anything about it + (!recursing && attrs.by_key("no_mangle").exists()) + || attrs.by_key("allow").tt_values().any(|tt| { + let allows = tt.to_string(); + allows.contains(allow_name) + || allows.contains(allow::BAD_STYLE) + || allows.contains(allow::NONSTANDARD_STYLE) + }) + }; + + is_allowed(id) + // go upwards one step or give up + || match id { + AttrDefId::ModuleId(m) => m.containing_module(self.db.upcast()).map(|v| v.into()), + AttrDefId::FunctionId(f) => Some(f.lookup(self.db.upcast()).container.into()), + AttrDefId::StaticId(sid) => Some(sid.lookup(self.db.upcast()).container.into()), + AttrDefId::ConstId(cid) => Some(cid.lookup(self.db.upcast()).container.into()), + AttrDefId::TraitId(tid) => Some(tid.lookup(self.db.upcast()).container.into()), + AttrDefId::ImplId(iid) => Some(iid.lookup(self.db.upcast()).container.into()), + // These warnings should not explore macro definitions at all + AttrDefId::MacroDefId(_) => None, + // Will never occur under an enum/struct/union/type alias + AttrDefId::AdtId(_) => None, + AttrDefId::FieldId(_) => None, + AttrDefId::EnumVariantId(_) => None, + AttrDefId::TypeAliasId(_) => None, + AttrDefId::GenericParamId(_) => None, + } + .map(|mid| self.allowed(mid, allow_name, true)) + .unwrap_or(false) } fn validate_func(&mut self, func: FunctionId) { @@ -109,7 +140,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { } // Check whether non-snake case identifiers are allowed for this function. - if self.allowed(func.into(), allow::NON_SNAKE_CASE) { + if self.allowed(func.into(), allow::NON_SNAKE_CASE, false) { return; } @@ -328,8 +359,9 @@ impl<'a, 'b> DeclValidator<'a, 'b> { fn validate_struct(&mut self, struct_id: StructId) { let data = self.db.struct_data(struct_id); - let non_camel_case_allowed = self.allowed(struct_id.into(), allow::NON_CAMEL_CASE_TYPES); - let non_snake_case_allowed = self.allowed(struct_id.into(), allow::NON_SNAKE_CASE); + let non_camel_case_allowed = + self.allowed(struct_id.into(), allow::NON_CAMEL_CASE_TYPES, false); + let non_snake_case_allowed = self.allowed(struct_id.into(), allow::NON_SNAKE_CASE, false); // Check the structure name. let struct_name = data.name.to_string(); @@ -461,7 +493,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { let data = self.db.enum_data(enum_id); // Check whether non-camel case names are allowed for this enum. - if self.allowed(enum_id.into(), allow::NON_CAMEL_CASE_TYPES) { + if self.allowed(enum_id.into(), allow::NON_CAMEL_CASE_TYPES, false) { return; } @@ -584,7 +616,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { fn validate_const(&mut self, const_id: ConstId) { let data = self.db.const_data(const_id); - if self.allowed(const_id.into(), allow::NON_UPPER_CASE_GLOBAL) { + if self.allowed(const_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) { return; } @@ -632,7 +664,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { return; } - if self.allowed(static_id.into(), allow::NON_UPPER_CASE_GLOBAL) { + if self.allowed(static_id.into(), allow::NON_UPPER_CASE_GLOBAL, false) { return; } @@ -869,10 +901,36 @@ fn main() { r#" #[allow(non_snake_case)] fn NonSnakeCaseName(SOME_VAR: u8) -> u8{ + // cov_flags generated output from elsewhere in this file + extern "C" { + #[no_mangle] + static lower_case: u8; + } + let OtherVar = SOME_VAR + 1; OtherVar } + #[allow(nonstandard_style)] + mod CheckNonstandardStyle { + fn HiImABadFnName() {} + } + + #[allow(bad_style)] + mod CheckBadStyle { + fn HiImABadFnName() {} + } + + mod F { + #![allow(non_snake_case)] + fn CheckItWorksWithModAttr(BAD_NAME_HI: u8) {} + } + + trait BAD_TRAIT { + fn BAD_FUNCTION(); + fn BadFunction(); + } + #[allow(non_snake_case, non_camel_case_types)] pub struct some_type { SOME_FIELD: u8, @@ -888,6 +946,60 @@ fn main() { ); } + #[test] + fn allow_attributes_crate_attr() { + check_diagnostics( + r#" + #![allow(non_snake_case)] + + mod F { + fn CheckItWorksWithCrateAttr(BAD_NAME_HI: u8) {} + } + + "#, + ); + } + + #[test] + #[ignore] + fn bug_trait_inside_fn() { + // FIXME: + // This is broken, and in fact, should not even be looked at by this + // lint in the first place. There's weird stuff going on in the + // collection phase. + // It's currently being brought in by: + // * validate_func on `a` recursing into modules + // * then it finds the trait and then the function while iterating + // through modules + // * then validate_func is called on Dirty + // * ... which then proceeds to look at some unknown module taking no + // attrs from either the impl or the fn a, and then finally to the root + // module + // + // It should find the attribute on the trait, but it *doesn't even see + // the trait* as far as I can tell. + + check_diagnostics( + r#" +trait T { fn a(); } +struct U {} +impl T for U { + fn a() { + // this comes out of bitflags, mostly + #[allow(non_snake_case)] + trait __BitFlags { + const HiImAlsoBad: u8 = 2; + #[inline] + fn Dirty(&self) -> bool { + false + } + } + + } +}"#, + ); + } + #[test] fn ignores_extern_items() { cov_mark::check!(extern_func_incorrect_case_ignored); -- cgit v1.2.3 From 26d2653dd64f139c89449cf4bab8bac737e930a5 Mon Sep 17 00:00:00 2001 From: Jade Date: Tue, 13 Apr 2021 01:20:00 -0700 Subject: address review feedback --- crates/hir_ty/src/diagnostics/decl_check.rs | 56 ++++++++++++++++++----------- 1 file changed, 35 insertions(+), 21 deletions(-) (limited to 'crates/hir_ty') diff --git a/crates/hir_ty/src/diagnostics/decl_check.rs b/crates/hir_ty/src/diagnostics/decl_check.rs index 31532f327..fb0fc4c91 100644 --- a/crates/hir_ty/src/diagnostics/decl_check.rs +++ b/crates/hir_ty/src/diagnostics/decl_check.rs @@ -926,11 +926,6 @@ fn main() { fn CheckItWorksWithModAttr(BAD_NAME_HI: u8) {} } - trait BAD_TRAIT { - fn BAD_FUNCTION(); - fn BadFunction(); - } - #[allow(non_snake_case, non_camel_case_types)] pub struct some_type { SOME_FIELD: u8, @@ -981,22 +976,41 @@ fn main() { check_diagnostics( r#" -trait T { fn a(); } -struct U {} -impl T for U { - fn a() { - // this comes out of bitflags, mostly - #[allow(non_snake_case)] - trait __BitFlags { - const HiImAlsoBad: u8 = 2; - #[inline] - fn Dirty(&self) -> bool { - false + trait T { fn a(); } + struct U {} + impl T for U { + fn a() { + // this comes out of bitflags, mostly + #[allow(non_snake_case)] + trait __BitFlags { + const HiImAlsoBad: u8 = 2; + #[inline] + fn Dirty(&self) -> bool { + false + } } + } + } + "#, + ); + } + #[test] + #[ignore] + fn bug_traits_arent_checked() { + // FIXME: Traits and functions in traits aren't currently checked by + // r-a, even though rustc will complain about them. + check_diagnostics( + r#" + trait BAD_TRAIT { + // ^^^^^^^^^ Trait `BAD_TRAIT` should have CamelCase name, e.g. `BadTrait` + fn BAD_FUNCTION(); + // ^^^^^^^^^^^^ Function `BAD_FUNCTION` should have snake_case name, e.g. `bad_function` + fn BadFunction(); + // ^^^^^^^^^^^^ Function `BadFunction` should have snake_case name, e.g. `bad_function` } -}"#, + "#, ); } @@ -1006,10 +1020,10 @@ impl T for U { cov_mark::check!(extern_static_incorrect_case_ignored); check_diagnostics( r#" -extern { - fn NonSnakeCaseName(SOME_VAR: u8) -> u8; - pub static SomeStatic: u8 = 10; -} + extern { + fn NonSnakeCaseName(SOME_VAR: u8) -> u8; + pub static SomeStatic: u8 = 10; + } "#, ); } -- cgit v1.2.3