From 0bfda4fb7d25cceefb08292676a47c96a82e409d Mon Sep 17 00:00:00 2001 From: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com> Date: Thu, 18 Apr 2024 17:18:34 -0400 Subject: [PATCH] Impl item removals should lint if the fallback item is not public API. (#763) --- .../inherent_associated_pub_const_missing.ron | 10 +++-- src/lints/inherent_method_missing.ron | 1 + .../new/src/lib.rs | 14 +++++++ .../old/src/lib.rs | 20 ++++++++++ .../inherent_method_missing/new/src/lib.rs | 27 +++++++++++++ .../inherent_method_missing/old/src/lib.rs | 39 +++++++++++++++++++ ...nt_associated_pub_const_missing.output.ron | 13 ++++++- .../inherent_method_missing.output.ron | 12 ++++++ 8 files changed, 132 insertions(+), 4 deletions(-) diff --git a/src/lints/inherent_associated_pub_const_missing.ron b/src/lints/inherent_associated_pub_const_missing.ron index 011a27f3..b72ffb8f 100644 --- a/src/lints/inherent_associated_pub_const_missing.ron +++ b/src/lints/inherent_associated_pub_const_missing.ron @@ -19,7 +19,7 @@ SemverQuery( inherent_impl { public_api_eligible @filter(op: "=", value: ["$true"]) - + associated_constant { associated_constant: name @output @tag public_api_eligible @filter(op: "=", value: ["$true"]) @@ -28,7 +28,7 @@ SemverQuery( filename @output begin_line @output } - } + } } } } @@ -49,8 +49,12 @@ SemverQuery( name @filter(op: "=", value: ["%associated_constant"]) } } - + impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) { + # We only consider falling back to a trait impl's constant + # if the impl of that trait is public API. + public_api_eligible @filter(op: "=", value: ["$true"]) + implemented_trait { trait { associated_constant { diff --git a/src/lints/inherent_method_missing.ron b/src/lints/inherent_method_missing.ron index c41071e5..7eca6b8b 100644 --- a/src/lints/inherent_method_missing.ron +++ b/src/lints/inherent_method_missing.ron @@ -49,6 +49,7 @@ SemverQuery( # an inherently-implemented method to a trait is not necessarily # a breaking change, so we don't want to report it. impl @fold @transform(op: "count") @filter(op: "=", value: ["$zero"]) { + public_api_eligible @filter(op: "=", value: ["$true"]) method { visibility_limit @filter(op: "one_of", value: ["$public_or_default"]) name @filter(op: "=", value: ["%method_name"]) diff --git a/test_crates/inherent_associated_pub_const_missing/new/src/lib.rs b/test_crates/inherent_associated_pub_const_missing/new/src/lib.rs index dcf65622..95d54881 100644 --- a/test_crates/inherent_associated_pub_const_missing/new/src/lib.rs +++ b/test_crates/inherent_associated_pub_const_missing/new/src/lib.rs @@ -74,3 +74,17 @@ pub struct ExampleB; impl TraitB for ExampleB { const N_B: i64 = 0; } + +// Test for when an inherent const is implemented as trait's const, +// but the `impl Trait` block is not public API. In this case, we report the lint +// since falling back to the trait implementation would require using non-public API. +pub trait TraitC { + const N_C: i64; +} + +pub struct ExampleC; + +#[doc(hidden)] +impl TraitC for ExampleC { + const N_C: i64 = 0; +} diff --git a/test_crates/inherent_associated_pub_const_missing/old/src/lib.rs b/test_crates/inherent_associated_pub_const_missing/old/src/lib.rs index 3ee0834c..07cbb9ec 100644 --- a/test_crates/inherent_associated_pub_const_missing/old/src/lib.rs +++ b/test_crates/inherent_associated_pub_const_missing/old/src/lib.rs @@ -102,3 +102,23 @@ impl ExampleB { } impl TraitB for ExampleB {} + + +// Test for when an inherent const is implemented as trait's const, +// but the `impl Trait` block is not public API. In this case, we report the lint +// since falling back to the trait implementation would require using non-public API. +pub trait TraitC { + const N_C: i64; +} + +pub struct ExampleC; + +impl ExampleC { + // This const gets removed. + pub const N_C: i64 = ::N_C; +} + +#[doc(hidden)] +impl TraitC for ExampleC { + const N_C: i64 = 0; +} diff --git a/test_crates/inherent_method_missing/new/src/lib.rs b/test_crates/inherent_method_missing/new/src/lib.rs index 13eb456e..15e63233 100644 --- a/test_crates/inherent_method_missing/new/src/lib.rs +++ b/test_crates/inherent_method_missing/new/src/lib.rs @@ -13,3 +13,30 @@ pub trait Bar { impl Bar for Foo { fn moved_method(&self) {} } + + + +// Test that the removal of an inherent method `next()` does not trigger `inherent_method_missing` +// if the equivalent method `Iterator::next()` is available and public API. +pub struct MyIter; + +impl Iterator for MyIter { + type Item = i64; + + fn next(&mut self) -> Option { + None + } +} + +// Test that the removal of an inherent method `next()` triggers `inherent_method_missing` +// if there is an equivalent method `Iterator::next()` but isn't public API due to `#[doc(hidden)]`. +pub struct SecretlyIter; + +#[doc(hidden)] +impl Iterator for SecretlyIter { + type Item = i64; + + fn next(&mut self) -> Option { + None + } +} diff --git a/test_crates/inherent_method_missing/old/src/lib.rs b/test_crates/inherent_method_missing/old/src/lib.rs index a8911f45..7c6755d9 100644 --- a/test_crates/inherent_method_missing/old/src/lib.rs +++ b/test_crates/inherent_method_missing/old/src/lib.rs @@ -12,3 +12,42 @@ impl Foo { pub fn moved_method(&self) {} } + +// Test that the removal of an inherent method `next()` does not trigger `inherent_method_missing` +// if the equivalent method `Iterator::next()` is available and public API. +pub struct MyIter; + +impl MyIter { + // This method is getting removed. + pub fn next(&mut self) -> Option<::Item> { + ::next() + } +} + +impl Iterator for MyIter { + type Item = i64; + + fn next(&mut self) -> Option { + None + } +} + +// Test that the removal of an inherent method `next()` triggers `inherent_method_missing` +// if there is an equivalent method `Iterator::next()` that isn't public API via `#[doc(hidden)]`. +pub struct SecretlyIter; + +impl SecretlyIter { + // This method is getting removed. + pub fn next(&mut self) -> Option<::Item> { + ::next() + } +} + +#[doc(hidden)] +impl Iterator for SecretlyIter { + type Item = i64; + + fn next(&mut self) -> Option { + None + } +} diff --git a/test_outputs/inherent_associated_pub_const_missing.output.ron b/test_outputs/inherent_associated_pub_const_missing.output.ron index 93f27952..79114d33 100644 --- a/test_outputs/inherent_associated_pub_const_missing.output.ron +++ b/test_outputs/inherent_associated_pub_const_missing.output.ron @@ -10,7 +10,7 @@ "span_begin_line": Uint64(6), "span_filename": String("src/lib.rs"), "visibility_limit": String("public"), - }, + }, { "associated_constant": String("PublicConstantRenameA"), "name": String("StructA"), @@ -33,5 +33,16 @@ "span_filename": String("src/lib.rs"), "visibility_limit": String("public"), }, + { + "associated_constant": String("N_C"), + "name": String("ExampleC"), + "path": List([ + String("inherent_associated_pub_const_missing"), + String("ExampleC"), + ]), + "span_begin_line": Uint64(118), + "span_filename": String("src/lib.rs"), + "visibility_limit": String("public"), + }, ], } diff --git a/test_outputs/inherent_method_missing.output.ron b/test_outputs/inherent_method_missing.output.ron index 63c711d7..2f138d3e 100644 --- a/test_outputs/inherent_method_missing.output.ron +++ b/test_outputs/inherent_method_missing.output.ron @@ -50,6 +50,18 @@ "span_filename": String("src/lib.rs"), "visibility_limit": String("public"), }, + { + "method_name": String("next"), + "method_visibility": String("public"), + "name": String("SecretlyIter"), + "path": List([ + String("inherent_method_missing"), + String("SecretlyIter"), + ]), + "span_begin_line": Uint64(41), + "span_filename": String("src/lib.rs"), + "visibility_limit": String("public"), + }, ], "./test_crates/inherent_method_unsafe_added/": [ {