Impl item removals should lint if the fallback item is not public API. (

#763)
obi1kenobi · Apr 18, 2024 · 0bfda4f · 0bfda4f
1 parent ac0bf46
commit 0bfda4f
Show file tree

Hide file tree

Showing 8 changed files with 132 additions and 4 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 = <Self as TraitC>::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
@@ -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<Self::Item> {
+        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<Self::Item> {
+        None
+    }
+}
diff --git 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<<Self as Iterator>::Item> {
+        <Self as Iterator>::next()
+    }
+}
+
+impl Iterator for MyIter {
+    type Item = i64;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        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<<Self as Iterator>::Item> {
+        <Self as Iterator>::next()
+    }
+}
+
+#[doc(hidden)]
+impl Iterator for SecretlyIter {
+    type Item = i64;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        None
+    }
+}
diff --git 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
@@ -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/": [
         {