diff --git a/cargo_pup_lint_config/src/struct_lint/builder.rs b/cargo_pup_lint_config/src/struct_lint/builder.rs index 960132d..82529ca 100644 --- a/cargo_pup_lint_config/src/struct_lint/builder.rs +++ b/cargo_pup_lint_config/src/struct_lint/builder.rs @@ -148,4 +148,10 @@ impl<'a> StructConstraintBuilder<'a> { self.add_rule_internal(StructRule::MustBePublic(self.current_severity)); self } + + /// Add a rule requiring the struct to have pub(crate) visibility + pub fn must_be_pub_crate(mut self) -> Self { + self.add_rule_internal(StructRule::MustBePubCrate(self.current_severity)); + self + } } diff --git a/cargo_pup_lint_config/src/struct_lint/types.rs b/cargo_pup_lint_config/src/struct_lint/types.rs index 68974c5..601157b 100644 --- a/cargo_pup_lint_config/src/struct_lint/types.rs +++ b/cargo_pup_lint_config/src/struct_lint/types.rs @@ -35,10 +35,12 @@ pub enum StructRule { MustBeNamed(String, Severity), /// Enforces that the struct name does not match the specified pattern MustNotBeNamed(String, Severity), - /// Enforces that the struct has private visibility + /// Enforces that the struct has private visibility (not pub, not pub(crate), not pub(super)) MustBePrivate(Severity), - /// Enforces that the struct has public visibility + /// Enforces that the struct has public visibility (pub) MustBePublic(Severity), + /// Enforces that the struct has pub(crate) visibility + MustBePubCrate(Severity), /// Enforces that the struct implements a specific trait ImplementsTrait(String, Severity), /// Logical AND - both rules must pass diff --git a/cargo_pup_lint_impl/src/lints/struct_lint/lint.rs b/cargo_pup_lint_impl/src/lints/struct_lint/lint.rs index 2b6b5d0..bdccf6f 100644 --- a/cargo_pup_lint_impl/src/lints/struct_lint/lint.rs +++ b/cargo_pup_lint_impl/src/lints/struct_lint/lint.rs @@ -186,6 +186,14 @@ declare_variable_severity_lint!( "Struct must have public visibility" ); +declare_variable_severity_lint!( + pub, + STRUCT_LINT_MUST_BE_PUB_CRATE, + STRUCT_LINT_MUST_BE_PUB_CRATE_DENY, + STRUCT_LINT_MUST_BE_PUB_CRATE_WARN, + "Struct must have pub(crate) visibility" +); + impl_lint_pass!(StructLint => [ STRUCT_LINT_MUST_BE_NAMED_DENY, STRUCT_LINT_MUST_BE_NAMED_WARN, @@ -194,7 +202,9 @@ impl_lint_pass!(StructLint => [ STRUCT_LINT_MUST_BE_PRIVATE_DENY, STRUCT_LINT_MUST_BE_PRIVATE_WARN, STRUCT_LINT_MUST_BE_PUBLIC_DENY, - STRUCT_LINT_MUST_BE_PUBLIC_WARN + STRUCT_LINT_MUST_BE_PUBLIC_WARN, + STRUCT_LINT_MUST_BE_PUB_CRATE_DENY, + STRUCT_LINT_MUST_BE_PUB_CRATE_WARN ]); impl ArchitectureLintRule for StructLint { @@ -266,6 +276,22 @@ impl<'tcx> LateLintPass<'tcx> for StructLint { let struct_visibility = ctx.tcx.visibility(def_id); let is_public = struct_visibility == rustc_middle::ty::Visibility::Public; + // Check if there's a visibility keyword (pub, pub(crate), pub(super), etc.) + let has_visibility_keyword = !item.vis_span.is_empty(); + + // Check if visibility is pub(crate): + // - Must have a visibility keyword (not inherited/private) + // - Must be restricted to the crate root + let is_pub_crate = match struct_visibility { + rustc_middle::ty::Visibility::Restricted(restricted_to) => { + has_visibility_keyword && restricted_to.is_crate_root() + } + _ => false, + }; + + // Truly private means no visibility keyword at all (inherited visibility) + let is_private = !has_visibility_keyword; + // Apply rules for rule in &self.struct_rules { match rule { @@ -317,28 +343,64 @@ impl<'tcx> LateLintPass<'tcx> for StructLint { } } StructRule::MustBePrivate(severity) => { - if is_public { + if !is_private { + let visibility_desc = if is_public { + "pub" + } else if is_pub_crate { + "pub(crate)" + } else { + "restricted" // pub(super) or pub(in path) + }; span_lint_and_help( ctx, STRUCT_LINT_MUST_BE_PRIVATE::get_by_severity(*severity), self.name().as_str(), definition_span, - format!("Struct '{item_name}' is public, but must be private"), + format!( + "Struct '{item_name}' has {visibility_desc} visibility, but must be private" + ), None, - "Remove the 'pub' visibility modifier", + "Remove the visibility modifier", ); } } StructRule::MustBePublic(severity) => { if !is_public { + let visibility_desc = if is_pub_crate { + "pub(crate)" + } else { + "private" + }; span_lint_and_help( ctx, STRUCT_LINT_MUST_BE_PUBLIC::get_by_severity(*severity), self.name().as_str(), definition_span, - format!("Struct '{item_name}' is private, but must be public"), + format!( + "Struct '{item_name}' has {visibility_desc} visibility, but must be pub" + ), + None, + "Change the visibility to 'pub'", + ); + } + } + StructRule::MustBePubCrate(severity) => { + if !is_pub_crate { + let visibility_desc = if is_public { + "pub" + } else { + "private" + }; + span_lint_and_help( + ctx, + STRUCT_LINT_MUST_BE_PUB_CRATE::get_by_severity(*severity), + self.name().as_str(), + definition_span, + format!( + "Struct '{item_name}' has {visibility_desc} visibility, but must be pub(crate)" + ), None, - "Add the 'pub' visibility modifier", + "Change the visibility to 'pub(crate)'", ); } } diff --git a/test_app/Cargo.lock b/test_app/Cargo.lock index 1228c1c..aacee3e 100644 --- a/test_app/Cargo.lock +++ b/test_app/Cargo.lock @@ -57,7 +57,7 @@ dependencies = [ [[package]] name = "cargo_pup_common" -version = "0.1.3" +version = "0.1.4" dependencies = [ "anyhow", "cargo_metadata", @@ -68,7 +68,7 @@ dependencies = [ [[package]] name = "cargo_pup_lint_config" -version = "0.1.3" +version = "0.1.4" dependencies = [ "anyhow", "cargo_pup_common", diff --git a/test_app/expected_output b/test_app/expected_output index fe6daa2..eb0b79a 100644 --- a/test_app/expected_output +++ b/test_app/expected_output @@ -1,38 +1,3 @@ -warning: proc macro 'forbidden_proc_macro' is not allowed in this module - --> proc_macro_test/src/lib.rs:6:1 - | -6 | / pub fn forbidden_proc_macro(input: TokenStream) -> TokenStream { -7 | | input -8 | | } - | |_^ - | - = help: Consider moving this item to a different module - = note: Applied by cargo-pup rule 'proc_macro_restriction_test'. - = note: `#[warn(module_denied_items)]` on by default - -warning: proc macro attribute 'forbidden_attr_macro' is not allowed in this module - --> proc_macro_test/src/lib.rs:12:1 - | -12 | / pub fn forbidden_attr_macro(_attr: TokenStream, item: TokenStream) -> TokenStream { -13 | | item -14 | | } - | |_^ - | - = help: Consider moving this item to a different module - = note: Applied by cargo-pup rule 'proc_macro_restriction_test'. - -warning: proc macro derive 'forbidden_derive_macro' is not allowed in this module - --> proc_macro_test/src/lib.rs:18:1 - | -18 | / pub fn forbidden_derive_macro(input: TokenStream) -> TokenStream { -19 | | input -20 | | } - | |_^ - | - = help: Consider moving this item to a different module - = note: Applied by cargo-pup rule 'proc_macro_restriction_test'. - -warning: `proc_macro_test` (lib) generated 3 warnings warning: unused macro definition: `forbidden_macro` --> src/macro_restriction/mod.rs:4:14 | @@ -139,19 +104,6 @@ warning: module 'nested' is not allowed in this module = help: Consider moving this item to a different module = note: Applied by cargo-pup rule 'item_type_restrictions'. -warning: declarative macro 'forbidden_macro' is not allowed in this module - --> src/macro_restriction/mod.rs:4:1 - | -4 | / macro_rules! forbidden_macro { -5 | | () => { -6 | | println!("This macro is forbidden!"); -7 | | }; -8 | | } - | |_^ - | - = help: Consider moving this item to a different module - = note: Applied by cargo-pup rule 'macro_restriction_test'. - warning: Wildcard imports are not allowed --> src/module_usage/mod.rs:5:1 | @@ -230,13 +182,13 @@ warning: Struct must match pattern '.*MyTraitImpl$', found 'MyBadlyNamedThing' = note: Applied by cargo-pup rule 'trait_restrictions'. = note: `#[warn(struct_lint_must_be_named)]` on by default -warning: Struct 'MyBadlyNamedThing' is public, but must be private +warning: Struct 'MyBadlyNamedThing' has pub visibility, but must be private --> src/trait_impl/mod.rs:17:1 | 17 | pub struct MyBadlyNamedThing {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: Remove the 'pub' visibility modifier + = help: Remove the visibility modifier = note: Applied by cargo-pup rule 'trait_restrictions'. = note: `#[warn(struct_lint_must_be_private)]` on by default @@ -259,104 +211,5 @@ error: Function 'set_height' is forbidden by lint rule = help: Remove this function to satisfy the architectural rule = note: Applied by cargo-pup rule 'builder_style_set_consuming_forbidden'. -error: Function 'forbidden_async_function' is forbidden by lint rule - --> src/async_functions/mod.rs:8:1 - | -8 | pub async fn forbidden_async_function() -> String { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: Remove this function to satisfy the architectural rule - = note: Applied by cargo-pup rule 'async_functions_forbidden'. - -error: Function 'async_result_function' is forbidden by lint rule - --> src/async_functions/mod.rs:13:1 - | -13 | pub async fn async_result_function() -> Result { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: Remove this function to satisfy the architectural rule - = note: Applied by cargo-pup rule 'async_functions_forbidden'. - -error: Function 'process_async' is forbidden by lint rule - --> src/async_functions/mod.rs:33:5 - | -33 | pub async fn process_async(&self) -> Result { - | ^^^^^^^^^^^^^^^^^^ - | - = help: Remove this function to satisfy the architectural rule - = note: Applied by cargo-pup rule 'async_functions_forbidden'. - -error: Function 'process_item' is forbidden by lint rule - --> src/async_functions/mod.rs:55:5 - | -55 | async fn process_item(&self, item: String) -> Result { - | ^^^^^^^^^^^^^^^^^ - | - = help: Remove this function to satisfy the architectural rule - = note: Applied by cargo-pup rule 'async_functions_forbidden'. - -warning: Function allocates heap memory: calls allocating function: std::boxed::Box::::new - --> src/no_allocation.rs:12:5 - | -12 | Box::new(42) - | ^^^^^^^^^^^^ - | - = help: Remove heap allocations to satisfy the NoAllocation rule - = note: Applied by cargo-pup rule 'no_allocation_check'. - -warning: Function allocates heap memory: calls allocating function: std::vec::Vec::::new - --> src/no_allocation.rs:17:5 - | -17 | Vec::new() - | ^^^^^^^^^^ - | - = help: Remove heap allocations to satisfy the NoAllocation rule - = note: Applied by cargo-pup rule 'no_allocation_check'. - -warning: Function allocates heap memory: calls allocating function: std::string::ToString::to_string - --> src/no_allocation.rs:22:5 - | -22 | x.to_string() - | ^^^^^^^^^^^^^ - | - = help: Remove heap allocations to satisfy the NoAllocation rule - = note: Applied by cargo-pup rule 'no_allocation_check'. - -warning: Function allocates heap memory: calls function that allocates: no_allocation::helper_that_allocates - --> src/no_allocation.rs:27:5 - | -27 | helper_that_allocates(x) - | ^^^^^^^^^^^^^^^^^^^^^^^^ - | - = help: Remove heap allocations to satisfy the NoAllocation rule - = note: Applied by cargo-pup rule 'no_allocation_check'. - -warning: Function allocates heap memory: calls allocating function: std::vec::Vec::::new - --> src/no_allocation.rs:32:5 - | -32 | Vec::new() - | ^^^^^^^^^^ - | - = help: Remove heap allocations to satisfy the NoAllocation rule - = note: Applied by cargo-pup rule 'no_allocation_check'. - -warning: Function allocates heap memory: calls function that allocates: no_allocation::deep_helper - --> src/no_allocation.rs:36:5 - | -36 | deep_helper() - | ^^^^^^^^^^^^^ - | - = help: Remove heap allocations to satisfy the NoAllocation rule - = note: Applied by cargo-pup rule 'no_allocation_check'. - -warning: Function allocates heap memory: calls function that allocates: no_allocation::middle_helper - --> src/no_allocation.rs:41:5 - | -41 | middle_helper() - | ^^^^^^^^^^^^^^^ - | - = help: Remove heap allocations to satisfy the NoAllocation rule - = note: Applied by cargo-pup rule 'no_allocation_check'. - -warning: `test_app` (bin "test_app") generated 27 warnings -error: could not compile `test_app` (bin "test_app") due to 6 previous errors; 27 warnings emitted +warning: `test_app` (bin "test_app") generated 19 warnings +error: could not compile `test_app` (bin "test_app") due to 2 previous errors; 19 warnings emitted diff --git a/test_app/pup.ron b/test_app/pup.ron index 861a48e..bff15b4 100644 --- a/test_app/pup.ron +++ b/test_app/pup.ron @@ -70,6 +70,7 @@ rules: [ MustBeNamed(".*MyTraitImpl$", Warn), MustBePrivate(Warn), + ImplementsTrait("test_app::trait_impl::MyTrait", Warn), ], )), Function(( @@ -79,62 +80,19 @@ ResultErrorMustImplementError(Warn), ], )), - Module(( - name: "macro_restriction_test", - matches: Module("^test_app::macro_restriction$"), - rules: [ - DeniedItems( - items: ["declarative_macro"], - severity: Warn, - ), - ], - )), - Module(( - name: "proc_macro_restriction_test", - matches: Module("^proc_macro_test$"), - rules: [ - DeniedItems( - items: ["proc_macro", "proc_macro_attribute", "proc_macro_derive"], - severity: Warn, - ), - ], - )), Function(( name: "builder_style_with_consuming_forbidden", - matches: AndMatches( - NameRegex("^with_.*"), - ReturnsType(SelfValue) - ), + matches: AndMatches(NameRegex("^with_.*"), ReturnsType(SelfValue)), rules: [ MustNotExist(Error), ], )), Function(( name: "builder_style_set_consuming_forbidden", - matches: AndMatches( - NameRegex("^set_.*"), - ReturnsType(SelfValue) - ), + matches: AndMatches(NameRegex("^set_.*"), ReturnsType(SelfValue)), rules: [ MustNotExist(Error), ], )), - Function(( - name: "async_functions_forbidden", - matches: AndMatches( - InModule("^test_app::async_functions$"), - IsAsync - ), - rules: [ - MustNotExist(Error), - ], - )), - Function(( - name: "no_allocation_check", - matches: InModule("^test_app::no_allocation$"), - rules: [ - NoAllocation(Warn), - ], - )), ], ) \ No newline at end of file diff --git a/tests/ui/struct_lint_new/pub_crate_visibility.rs b/tests/ui/struct_lint_new/pub_crate_visibility.rs new file mode 100644 index 0000000..cf689a6 --- /dev/null +++ b/tests/ui/struct_lint_new/pub_crate_visibility.rs @@ -0,0 +1,21 @@ +// This product includes software developed at Datadog (https://www.datadoghq.com/) Copyright 2024 Datadog, Inc. + +//@compile-flags: --crate-name test_pub_crate_visibility +//@compile-flags: --crate-type lib + +// This test verifies that the StructRule::MustBePubCrate rule works correctly + +// Testing MustBePubCrate rule - struct that is pub but should be pub(crate) +pub struct CrateInternal { //~ ERROR: Struct 'CrateInternal' has pub visibility, but must be pub(crate) + field: i32, +} + +// Testing MustBePubCrate rule - struct that is private but should be pub(crate) +struct AlsoInternal { //~ ERROR: Struct 'AlsoInternal' has private visibility, but must be pub(crate) + value: String, +} + +// This should not trigger any warnings/errors (pub(crate), as required) +pub(crate) struct CorrectlyPubCrate { + data: Vec, +} diff --git a/tests/ui/struct_lint_new/pub_crate_visibility.stderr b/tests/ui/struct_lint_new/pub_crate_visibility.stderr new file mode 100644 index 0000000..59e4cb1 --- /dev/null +++ b/tests/ui/struct_lint_new/pub_crate_visibility.stderr @@ -0,0 +1,21 @@ +error: Struct 'CrateInternal' has pub visibility, but must be pub(crate) + --> tests/ui/struct_lint_new/pub_crate_visibility.rs:9:1 + | +LL | pub struct CrateInternal { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: Change the visibility to 'pub(crate)' + = note: Applied by cargo-pup rule 'must_be_pub_crate_test'. + = note: `#[deny(struct_lint_must_be_pub_crate)]` on by default + +error: Struct 'AlsoInternal' has private visibility, but must be pub(crate) + --> tests/ui/struct_lint_new/pub_crate_visibility.rs:14:1 + | +LL | struct AlsoInternal { + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: Change the visibility to 'pub(crate)' + = note: Applied by cargo-pup rule 'must_be_pub_crate_test'. + +error: aborting due to 2 previous errors + diff --git a/tests/ui/struct_lint_new/pup.ron b/tests/ui/struct_lint_new/pup.ron index 6859c23..59e0827 100644 --- a/tests/ui/struct_lint_new/pup.ron +++ b/tests/ui/struct_lint_new/pup.ron @@ -107,6 +107,20 @@ MustBePrivate(Error), // All InternalModel structs must be private ] ) + ), + + // Test MustBePubCrate rule - only apply to test_pub_crate_visibility crate + Struct( + ( + name: "must_be_pub_crate_test", + matches: AndMatches( + Name("test_pub_crate_visibility"), // Match based on crate name + Name(".*Internal"), // Match structs ending with Internal + ), + rules: [ + MustBePubCrate(Error), // Struct must be pub(crate) + ] + ) ) ] ) \ No newline at end of file diff --git a/tests/ui/struct_lint_new/visibility_rules.rs b/tests/ui/struct_lint_new/visibility_rules.rs index 40ced37..8770314 100644 --- a/tests/ui/struct_lint_new/visibility_rules.rs +++ b/tests/ui/struct_lint_new/visibility_rules.rs @@ -6,7 +6,7 @@ // This test verifies that the StructRule::MustBePrivate and StructRule::MustBePublic rules work correctly // Testing MustBePrivate rule: -pub struct InternalData { //~ ERROR: Struct 'InternalData' is public, but must be private +pub struct InternalData { //~ ERROR: Struct 'InternalData' has pub visibility, but must be private field: i32, } @@ -16,7 +16,7 @@ struct CorrectlyPrivate { } // Testing MustBePublic rule: -struct HiddenApi { //~ WARN: Struct 'HiddenApi' is private, but must be public +struct HiddenApi { //~ WARN: Struct 'HiddenApi' has private visibility, but must be pub id: u64, } @@ -26,7 +26,7 @@ pub struct CorrectlyPublic { } // Also testing name-pattern + visibility combination -pub struct InternalModel { //~ ERROR: Struct 'InternalModel' is public, but must be private +pub struct InternalModel { //~ ERROR: Struct 'InternalModel' has pub visibility, but must be private sensitive_data: String, } diff --git a/tests/ui/struct_lint_new/visibility_rules.stderr b/tests/ui/struct_lint_new/visibility_rules.stderr index 00b805b..aa0fbb7 100644 --- a/tests/ui/struct_lint_new/visibility_rules.stderr +++ b/tests/ui/struct_lint_new/visibility_rules.stderr @@ -1,30 +1,30 @@ -error: Struct 'InternalData' is public, but must be private +error: Struct 'InternalData' has pub visibility, but must be private --> tests/ui/struct_lint_new/visibility_rules.rs:9:1 | LL | pub struct InternalData { | ^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: Remove the 'pub' visibility modifier + = help: Remove the visibility modifier = note: Applied by cargo-pup rule 'must_be_private_test'. = note: `#[deny(struct_lint_must_be_private)]` on by default -warning: Struct 'HiddenApi' is private, but must be public +warning: Struct 'HiddenApi' has private visibility, but must be pub --> tests/ui/struct_lint_new/visibility_rules.rs:19:1 | LL | struct HiddenApi { | ^^^^^^^^^^^^^^^^^^ | - = help: Add the 'pub' visibility modifier + = help: Change the visibility to 'pub' = note: Applied by cargo-pup rule 'must_be_public_test'. = note: `#[warn(struct_lint_must_be_public)]` on by default -error: Struct 'InternalModel' is public, but must be private +error: Struct 'InternalModel' has pub visibility, but must be private --> tests/ui/struct_lint_new/visibility_rules.rs:29:1 | LL | pub struct InternalModel { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = help: Remove the 'pub' visibility modifier + = help: Remove the visibility modifier = note: Applied by cargo-pup rule 'name_and_visibility_test'. error: aborting due to 2 previous errors; 1 warning emitted