False positive for the trait_method_added lint with diesel

[weiznich]

Which lint or lints are the issue

trait_method_added

Known issues that might be causing this

• Is the flagged item defined in another crate, or a re-export of such an item? (#638)

Steps to reproduce the bug with the above code

diesel-rs/diesel@f7e9819

Actual Behaviour

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/trait_method_added.ron

Failed in:
  trait method diesel::connection::Connection::set_prepared_statement_cache_size in file /home/weiznich/Documents/rust/diesel/diesel/src/connection/mod.rs:416
  trait method diesel::Connection::set_prepared_statement_cache_size in file /home/weiznich/Documents/rust/diesel/diesel/src/connection/mod.rs:416
  trait method diesel::prelude::Connection::set_prepared_statement_cache_size in file /home/weiznich/Documents/rust/diesel/diesel/src/connection/mod.rs:416

Expected Behaviour

I believe this is a false positive as the Connection trait requires that ConnectionSealed is implemented, which in turn is private (reexport) as long as the unstable i-implement-a-third-party-backend-and-opt-into-breaking-changes feature flag is not enabled.

Verbose Lint Output

cargo semver-checks --features "postgres" --baseline-version 2.2.3 --only-explicit-features --verbose 
     Parsing diesel v2.2.0 (current)
             Features: postgres
    Updating crates.io index
     Locking 23 packages to latest compatible versions
 Documenting diesel v2.2.0 (/home/weiznich/Documents/rust/diesel/diesel)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 2.44s
      Parsed [   2.918s] (current)
     Parsing diesel v2.2.3 (baseline, cached)
      Parsed [   0.166s] (baseline)
    Checking diesel v2.2.3 -> v2.2.0 (patch change)
    Starting 89 checks, 0 unnecessary on 20 threads
        PASS [   0.040s]       major        auto_trait_impl_removed
        PASS [   0.007s]       major        constructible_struct_adds_field
        PASS [   0.009s]       major        constructible_struct_adds_private_field
        PASS [   0.010s]       major        constructible_struct_changed_type
        PASS [   0.037s]       major        derive_trait_impl_removed
        PASS [   0.006s]       major        enum_marked_non_exhaustive
        PASS [   0.004s]       major        enum_missing
        PASS [   0.006s]       minor        enum_must_use_added
        PASS [   0.006s]       major        enum_now_doc_hidden
        PASS [   0.014s]       major        enum_repr_int_changed
        PASS [   0.009s]       major        enum_repr_int_removed
        PASS [   0.006s]       major        enum_repr_transparent_removed
        PASS [   0.008s]       major        enum_struct_variant_field_added
        PASS [   0.005s]       major        enum_struct_variant_field_missing
        PASS [   0.004s]       major        enum_struct_variant_field_now_doc_hidden
        PASS [   0.004s]       major        enum_tuple_variant_changed_kind
        PASS [   0.006s]       major        enum_tuple_variant_field_added
        PASS [   0.005s]       major        enum_tuple_variant_field_missing
        PASS [   0.005s]       major        enum_tuple_variant_field_now_doc_hidden
        PASS [   0.010s]       major        enum_unit_variant_changed_kind
        PASS [   0.014s]       major        enum_variant_added
        PASS [   0.006s]       major        enum_variant_marked_non_exhaustive
        PASS [   0.006s]       major        enum_variant_missing
        PASS [   0.005s]       major        exported_function_changed_abi
        PASS [   0.006s]       major        function_abi_no_longer_unwind
        PASS [   0.009s]       major        function_changed_abi
        PASS [   0.004s]       major        function_const_removed
        PASS [   0.010s]       major        function_export_name_changed
        PASS [   0.009s]       major        function_missing
        PASS [   0.007s]       minor        function_must_use_added
        PASS [   0.009s]       major        function_now_doc_hidden
        PASS [   0.005s]       major        function_parameter_count_changed
        PASS [   0.004s]       major        function_unsafe_added
        PASS [   0.007s]       major        inherent_associated_const_now_doc_hidden
        PASS [   0.006s]       major        inherent_associated_pub_const_missing
        PASS [   0.007s]       major        inherent_method_const_removed
        PASS [   0.007s]       major        inherent_method_missing
        PASS [   0.006s]       minor        inherent_method_must_use_added
        PASS [   0.008s]       major        inherent_method_now_doc_hidden
        PASS [   0.009s]       major        inherent_method_unsafe_added
        PASS [   0.009s]       major        method_parameter_count_changed
        PASS [   0.005s]       major        module_missing
        PASS [   0.004s]       major        pub_module_level_const_missing
        PASS [   0.004s]       major        pub_module_level_const_now_doc_hidden
        PASS [   0.006s]       major        pub_static_missing
        PASS [   0.004s]       major        pub_static_mut_now_immutable
        PASS [   0.005s]       major        pub_static_now_doc_hidden
        PASS [   0.004s]       major        repr_c_removed
        PASS [   0.008s]       major        repr_packed_added
        PASS [   0.006s]       major        repr_packed_removed
        PASS [   0.020s]       major        sized_impl_removed
        PASS [   0.006s]       major        struct_marked_non_exhaustive
        PASS [   0.007s]       major        struct_missing
        PASS [   0.006s]       minor        struct_must_use_added
        PASS [   0.008s]       major        struct_now_doc_hidden
        PASS [   0.006s]       major        struct_pub_field_missing
        PASS [   0.005s]       major        struct_pub_field_now_doc_hidden
        PASS [   0.004s]       major        struct_repr_transparent_removed
        PASS [   0.005s]       major        struct_with_pub_fields_changed_type
        PASS [   0.008s]       major        trait_associated_const_added
        PASS [   0.006s]       major        trait_associated_const_default_removed
        PASS [   0.005s]       major        trait_associated_const_now_doc_hidden
        PASS [   0.008s]       major        trait_associated_type_added
        PASS [   0.006s]       major        trait_associated_type_default_removed
        PASS [   0.005s]       major        trait_associated_type_now_doc_hidden
        FAIL [   0.008s]       major        trait_method_added
        PASS [   0.014s]       major        trait_method_default_impl_removed
        PASS [   0.012s]       major        trait_method_missing
        PASS [   0.011s]       major        trait_method_now_doc_hidden
        PASS [   0.013s]       major        trait_method_unsafe_added
        PASS [   0.008s]       major        trait_method_unsafe_removed
        PASS [   0.007s]       major        trait_missing
        PASS [   0.009s]       minor        trait_must_use_added
        PASS [   0.005s]       major        trait_newly_sealed
        PASS [   0.005s]       major        trait_no_longer_object_safe
        PASS [   0.007s]       major        trait_now_doc_hidden
        PASS [   0.006s]       major        trait_removed_associated_constant
        PASS [   0.011s]       major        trait_removed_associated_type
        PASS [   0.017s]       major        trait_removed_supertrait
        PASS [   0.006s]       major        trait_unsafe_added
        PASS [   0.006s]       major        trait_unsafe_removed
        PASS [   0.013s]       major        tuple_struct_to_plain_struct
        PASS [   0.015s]       minor        type_marked_deprecated
        PASS [   0.006s]       major        union_field_missing
        PASS [   0.004s]       major        union_missing
        PASS [   0.006s]       minor        union_must_use_added
        PASS [   0.005s]       major        union_now_doc_hidden
        PASS [   0.006s]       major        union_pub_field_now_doc_hidden
        PASS [   0.007s]       major        unit_struct_changed_kind
     Checked [   0.060s] 89 checks: 88 pass, 1 fail, 0 warn, 0 skip

--- failure trait_method_added: pub trait method added ---

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/trait_method_added.ron

Failed in:
  trait method diesel::connection::Connection::set_prepared_statement_cache_size in file /home/weiznich/Documents/rust/diesel/diesel/src/connection/mod.rs:416
  trait method diesel::Connection::set_prepared_statement_cache_size in file /home/weiznich/Documents/rust/diesel/diesel/src/connection/mod.rs:416
  trait method diesel::prelude::Connection::set_prepared_statement_cache_size in file /home/weiznich/Documents/rust/diesel/diesel/src/connection/mod.rs:416

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [   3.303s] diesel

Generated System Information

---

Software version

cargo-semver-checks 0.35.0

Operating system

Linux 6.10.9-200.fc40.x86_64

Command-line

/home/weiznich/.cargo/bin/cargo-semver-checks semver-checks --bugreport 

cargo version

> cargo -V 
cargo 1.81.0 (2dbb1af80 2024-08-20)

Compile time information

• Profile: release
• Target triple: x86_64-unknown-linux-gnu
• Family: unix
• OS: linux
• Architecture: x86_64
• Pointer width: 64
• Endian: little
• CPU features: fxsr,sse,sse2
• Host: x86_64-unknown-linux-gnu

Cargo build configuration:

[alias]
xtask = ["run", "--package", "xtask", "--"]

Build Configuration

None that are relevant

Additional Context

None

[obi1kenobi]

Thanks for the report, and sorry for the inconvenience.

I have to say I've never seen a bound like the where Self: OtherTrait before on a trait:

pub trait Connection: SimpleConnection + Sized + Send
where
    Self: ConnectionSealed,

from https://docs.diesel.rs/2.2.x/src/diesel/connection/mod.rs.html#212-217

Just so I make sure we get this right in our test suite, is that semantically different than writing

pub trait Connection: SimpleConnection + ConnectionSealed + Sized + Send

in any way? It seems like Self will end up being ConnectionSealed either way, and knowing that the eventual implementer of Connection has done so successfully means that we know they also implemented ConnectionSealed. So it seems identical to me, and yet somehow our analysis (currently!) disagrees. I'm wondering if that's a bug per se, or if I've missed some case where a where Self: Other bound may behave differently.

[weiznich]

Just so I make sure we get this right in our test suite, is that semantically different than writing

It's shouldn't be semantically different to that variant, although there might have been a reason why I wrote it the other way around. If I remember correctly that might have been related to how rustc resolves traits, but I don't remember any details.

knowing that the eventual implementer of Connection has done so successfully means that we know they also implemented ConnectionSealed.

It's correct to assume that any type that implements Connection must implement ConnectionSealed. The main point here is: You cannot implement ConnectionSealed outside of diesel (at least not without the unstable feature flag or without going through a module named internal, that is marked #[doc(hidden)] several times), as we don't have any wild card impl for ConnectionSealed and Connection is implemented for all concrete types that implement ConnectionSealed. Thinking a bit more about that, it might just be that cargo semver-check fails to see that diesel::internal is not part of the public API?

[obi1kenobi]

I'll double-check the specific case in diesel to be sure, but a priori I doubt the issue is related to the diesel::internal API. We should be handling all those cases properly: we have a truly astonishing number of test cases checking all sorts of cursed edge cases, and nothing I saw in diesel seems that far out of the ordinary there.

99.9% chance this is just a case of "we weren't looking at where Self: Trait clauses in the sealing analysis" which should be an easy fix.

[obi1kenobi]

Having dug into this and then re-read your message, both issues were at play and I had misunderstood your original point.

• We weren't considering where Self: Trait as a possible means of sealing. This is fixed in Traits can be sealed with where Self: SealedTrait predicate as well. trustfall-rustdoc-adapter#497 and will be part of the next cargo-semver-checks release.
• The analysis considers ConnectionSealed not sealed because it's publicly accessible (behind #[doc(hidden)]) at path:

[
    ImportablePath {
        path: Path {
            components: [
                "diesel",
                "internal",
                "derives",
                "multiconnection",
                "ConnectionSealed",
            ],
        },
        modifiers: Modifiers {
            doc_hidden: true,
            deprecated: false,
        },
    },
]

I believe you were trying to tip me off about the #[doc(hidden)]-but-public nature of ConnectionSealed, and I instead misinterpreted that as a concern that we might not be identifying #[doc(hidden)] correctly. Sorry about that! We correctly identify #[doc(hidden)], and the issue is downstream of that: "is ConnectionSealed actually sealed?"

Is a doc(hidden)-but-implementable trait considered sealed?

The canonical answer in Rust is "no" — sealing is a matter of ability not convention. doc(hidden) says that by convention one shouldn't use that trait, not that one cannot use it or implement it.

On the other hand, I sympathize with the use case! It feels like this shouldn't be flagged because people shouldn't implement that trait themselves. If they do, it feels like any breakage should be on them.

To help me figure this out, could you offer more context on why ConnectionSealed is merely doc(hidden) as opposed to pub-in-priv or sealed in some other way?

[weiznich]

On the other hand, I sympathize with the use case! It feels like this shouldn't be flagged because people shouldn't implement that trait themselves. If they do, it feels like any breakage should be on them.

To help me figure this out, could you offer more context on why ConnectionSealed is merely doc(hidden) as opposed to pub-in-priv or sealed in some other way?

Diesel provides a derive which implements this trait, for that it must be reachable via a public path from a third party crate. We generally do not consider code used by the derive macro to be part of the stable API as it's common in the rust ecosystem. These traits are considered to be implementation details, as we always couple the proc macro to a specific diesel version.

[obi1kenobi]

That makes sense. Thank you.

I think we'll need a new set of lints so we can properly capture the nuance here. Something like an error-by-default lint on "this trait is neither sealed nor doc(hidden)-sealed, and it made a breaking change" vs a warn-by-default lint on "this trait is doc(hidden)-sealed but not sealed, and it made a breaking change so be careful even though this is maybe still okay."

If we did the above, then this use case would have gotten a warning from the second lint, and no errors. (Projects can also configure lints to be allow/warn/deny using Cargo.toml config.) Adding a method to a fully-unsealed trait would still error, not warn.

Does that sound reasonable, or would you do something differently?

P.S.: I made a Discord for cargo-semver-checks and you're welcome to join it if you'd like! Here's an invite code that's good for 7 days: https://discord.gg/vf6AGXQU

[weiznich]

That sounds very reasonable. The other solution that I had in my mind is that if you plan to upstream this as official part of cargo you could maybe try to get a cargo or semver-check tool namespace registered in the compiler. After that you could add some sort of attribute so that crate authors can manually annotate parts of the API as private, which then would be considered as "non existent" by semver-check.

[obi1kenobi]

I brought this up at RustConf a couple of weeks ago: tool-oriented item attributes! They'd also be useful for tools like kani etc. The feedback was positive, so I'm sure it'll happen sooner or later.