False positive for the `trait_method_added` lint with diesel

weiznich commented 2 days ago

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

https://github.com/diesel-rs/diesel/commit/f7e9819303877b5a255143c4b32fa1e5745aaad0

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 commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

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 commented 1 day ago

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 https://github.com/obi1kenobi/trustfall-rustdoc-adapter/pull/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?

obi1kenobi / cargo-semver-checks