Tracking issue: Additional checks, both semver and non-semver

obi1kenobi commented 2 years ago

This is a list of all not-yet-implemented checks that would be useful to have. Some of these require new schema and adapter implementations as well, tracked in #241.

In addition to checking for semver violations, there are certain changes that are not breaking and don't even require a minor version, but can still be frustrating in downstream crates without a minor or major version bump. Crates should be able to opt into such warnings on an individual basis.

For example, based on this poll (with small sample size: ~40 respondents), ~40% of users expect that upgrading to a new patch version of a crate should not generate new lints or compiler warnings. The split between expecting a new minor version and a new major version was approximately 3-to-1.

Major version required

[ ] #578
[x] exhaustive enum becomes #[non_exhaustive]: #143
repr(C) plain struct has fields reordered -- two flavors of breakage here
- [ ] if the reordered field was pub, the breakage is "public API and ABI have diverged"
- [ ] if the reordered field wasn't pub, this requires either checking types (#149) or field sizes, to determine if "the new field at the old index is semantically equivalent"
[ ] tuple struct has fields reordered
- requires either checking types (#149), or repr(C) with same problems as above
[ ] tuple enum variant has fields reordered
- requires either checking types (#149), or repr(C) with same problems as above, or repr with primitive type on the enum itself since that's equivalent to repr(C) on all variants: https://doc.rust-lang.org/reference/type-layout.html#primitive-representation-of-enums-with-fields
[ ] reordering the variants of an enum where the ordering is public API: #[derive(PartialOrd)], repr(i8) on the enum, unit variants only enum, etc.
- this will change the runtime behavior of the ordering
- context: https://github.com/rust-lang/rust/issues/51561#issuecomment-397485233
- more details: #305
[x] pub struct pub field removed
[x] pub struct constructible with struct literal adds pub field (#233)
[x] pub struct constructible with struct literal adds non-pub field, and cannot be constructed with a literal from outside its own crate anymore (#233)
[ ] pub struct pub field changes type: #148, blocked on #149
[ ] pub enum variant field changes type: blocked on #149
[x] #554
[x] pub enum struct variant adds field: #238
[x] pub enum struct variant removes field: #153
[ ] pub enum variant discriminant removed
[ ] pub enum variant discriminant changed value
[ ] removed direct re-export of an enum variant: #291
struct with public fields changes to another kind (more details: https://github.com/rust-lang/cargo/pull/10871#issuecomment-1217103363)
- [x] unit struct to plain struct is breaking since the implicit constructor disappears (Rust for Rustaceans, Chapter 3, "Type Modifications", page 51)
- [x] #242
[x] union-related lints: #633
[x] #482
[x] pub fn moved, deleted, or renamed (#22, #23, #24)
[ ] pub fn changed return type: blocked on #149
[x] pub fn added argument
[x] pub fn removed argument
[ ] pub fn changed arguments in a backward-incompatible way
- This one is hard: it's possible to go from e.g. taking &str to taking S: Into<String> without breaking
- when it's not breaking, it requires a minor version
[x] #190
[x] #191
[ ] #503
[ ] ABI breaking changes in #[no_mangle] or #[export_name] functions: #502
[ ] pub method moved into a trait
- even if the trait is pub, it needs to be imported in the scope to have its methods be available
- Make sure to test for both trait-provided (default impl) methods and explicitly implemented methods for the trait. See the test cases added in #24 for example.
[x] repr(C) removed from struct or enum (#25)
[x] repr(transparent) removed from struct or enum (#26, #28)
[x] repr(u*) and repr(i*) changed/removed from enum (#29, #30)
[ ] #74
- breaking because of:
  - https://jack.wrenn.fyi/blog/semver-snares-alignment/
  - https://old.reddit.com/r/rust/comments/kr41sq/semver_snares_sizedness_and_size/
[ ] repr(align(N)) changed to a lower N, if that actually changes the alignment of the type
- It's possible that changing to a smaller N doesn't immediately cause a breaking change, because one of the contained fields has higher alignment requirements and ends up in practice causing the overall type's alignment to remain unchanged.
- Should we warn on this anyway, though? Or should we only warn if alignment has changed in practice?
- The argument for "warn always" is that the contractual promise has changed, and any excess alignment is now a fluke and implementation detail that could change in the future.
- https://doc.rust-lang.org/cargo/reference/semver.html#repr-align-n-change
[ ] repr(align(N)) changed to a higher N, if that actually changes the alignment of the type
- This case is harder -- making a stronger promise than before is only breaking if it in practice imposes a higher requirement. If a field of the type already required higher alignment such that the new alignment is a no-op compared to the old alignment, that's not breaking and we shouldn't report that.
- We shouldn't add a lint here until we can check the alignment of contained fields.
- https://doc.rust-lang.org/cargo/reference/semver.html#repr-align-n-change
[x] #632
[ ] repr(packed(N)) changed to a lower N
- same arguments as repr(align(N)) with lower N discussed above -- we probably want a lint for this
- be careful, because repr(packed) is also valid syntax! the N is implied as N=1 if missing
- https://doc.rust-lang.org/cargo/reference/semver.html#repr-packed-n-change
[ ] repr(packed(N)) changed to a higher N
- same arguments as repr(align(N)) with higher N discussed above -- we should probably hold off until we can get layout info for fields
- https://doc.rust-lang.org/cargo/reference/semver.html#repr-packed-n-change
[x] type is no longer Sized / Send / Sync / Unpin / UnwindSafe / RefUnwindSafe (auto traits) (#31)
[ ] type made Copy (appears to be breaking because of https://github.com/rust-lang/rust/issues/100905 )
[x] #73
[x] #870, including
- non-sealed trait added method
- 294
- non-sealed trait removed associated const default value
- non-sealed trait added associated type
- trait newly became sealed
  - this is partially covered by the "trait is not importable" rule, since one way to seal a trait is to make it unimportable
  - but another way to seal a trait is to make implementing or calling it require a private ZST that downstream crates cannot name, and this is not covered
  - reference: https://fosdem.org/2023/schedule/event/rust_rust_api_design_learnings/ time code ~00:30:00
- [x] #441
  - example breaking change in the real-world: https://github.com/awslabs/smithy-rs/pull/2577/files#diff-82252c5a4ad7e9d34be9254db1afe172d0f01c067b121de20ff7f7f29d7cf223L24
[x] trait removed/renamed associated type
[x] #232
- example of why this is breaking: https://twitter.com/i2talics/status/1559607755975057408
[x] #231
[x] #250
[x] #368
[x] #605
[x] #606
[ ] type no longer implements pub trait
[ ] implementing an existing pub trait for an existing type
- breaking because the trait's methods or associated types on that type may be ambiguous relative to those from traits implemented for that type in another crate (Rust for Rustaceans, chapter 3, pg. 52, "Trait Implementations")
[ ] implementing a new pub trait for an existing type, if the new pub trait is in a prelude module that gets imported with a wildcard
- similar to above, same source (Rust for Rustaceans, chapter 3, pg. 52, "Trait Implementations")
- normally the trait has to be in scope for its methods to be available, but the wildcard import will bring it in scope here
[ ] blanket impl added for an existing trait
- the blanket impl can cause a conflict with a downstream type that also implements the trait on one of its own types, if that type is covered by the blanket impl -- source: Rust for Rustaceans, chapter 2, pg. 30, "Blanket Implementations")
[ ] blanket impl added over a fundamental type (&T, Box etc.)
- similar reasoning as above -- source: Rust for Rustaceans, chapter 2, pg. 30, "Fundamental Types"
[ ] added new implementation of existing trait that does not contain at least one new local type (and that type satisfies the exemption from the orphan rule)
- source: Rust for Rustaceans, chapter 2, pg 31. "Covered Implementations"
[ ] upgrading to new major version of dependency while exporting a type that implements a trait from the dependency (new major version -> "it's not the same trait as before"): https://github.com/libp2p/rust-libp2p/pull/3170#issuecomment-1349688293
[x] #635
changes on associated types / trait bounds
- [ ] #786
- more details in thread here: https://twitter.com/compiler_errors/status/1652410429501771778
- [ ] adding a trait bound on a generic type in a function or type signature
- [ ] adding a trait bound on an associated type
- [ ] removing a trait bound from an associated type
- [ ] adding ?Sized on a trait associated type, breaks fn bad<T: Tr>() -> T::Assoc: https://twitter.com/withoutboats/status/1701274760653533637
- [ ] removing ?Sized on a trait associated type, breaks impls that used an unsized type there
- [ ] removing ?Sized bound from from a generic type in a function or type signature
- [ ] adding ?Sized bound to a generic type in a function or type signature: #532
- [ ] removing a trait bound from return position impl Trait
- [ ] adding ?Sized in return position impl Trait (say -> Box<impl Foo> changing to -> Box<impl Foo + ?Sized>)
- [ ] removing a bound on trait impl: #142
- [ ] adding trait bounds on trait impl
- [ ] all of the above but also on bounds in RTN notation: https://rust-lang.github.io/rfcs/3654-return-type-notation.html
[ ] Auto trait impls for impl Trait in return type.
- Requires a superset of the required schema additions as pub fn changed return type
[ ] #338
[ ] pub type typedef changes the order of generic arguments (regular, lifetime, or const generics) relative to the underlying type
[ ] pub type typedef adds a new generic parameter
[ ] pub type typedef removes a generic parameter
[ ] pub type typedef removes a default value for a generic parameter
[ ] pub type typedef changes a default value for a generic parameter
[ ] adding a new generic parameter without a default to a function/method that was already generic with at least one parameter that isn't impl Trait
- breaks cases where generics are explicitly provided
[ ] removing a generic parameter from a function/method
[ ] adding or removing a generic parameter from a struct/enum/union/trait
- adding generic parameter is only major breaking if it doesn't have a default
- source: https://github.com/obi1kenobi/cargo-semver-checks/issues/5#issuecomment-1424481604
- other source: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-parameter-no-default
[ ] variance of type lifetime parameters changed
- example: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=bb4738835367fd9ef604a8fcd728699b
- source: https://twitter.com/tenellous/status/1620907046911754240
- implementation idea: #480
[ ] addition of Drop implementations breaks const fn use: #930
[ ] addition of Drop implementations changing type parameter lifetime requirements
- source: https://github.com/tokio-rs/tracing/issues/2578
- even changing from "definitely doesn't have Drop" to "contains a generic type that might have an explicit Drop" is major & breaking: https://github.com/dtolnay/syn/issues/1718
  - breaking change: https://github.com/dtolnay/syn/commit/321839c38067f83bba7848e3496db4b031591359
  - fix: https://github.com/dtolnay/syn/pull/1719/files
  - reported upstream: https://github.com/RustCrypto/formats/issues/1471
[ ] any ABI change on a fn pointer argument to a function
- in Rust, ABIs are considered equal by comparing their literal strings
- changing pub fn blah(extern "C" fn()) to pub fn blah(extern "C-unwind" fn()) will almost certainly break people
[x] pub const moved, deleted, or renamed
- changing to pub static is still breaking: cannot use static in const fn; cannot initialize another const with a static like pub const MY_CONST = other::PREV_CONST_NOW_STATIC
- might be good to eventually split this into two lints, one for each case, for the sake of good error messages
[x] pub static moved, deleted, or renamed -- but not changed to pub const
- lint variants: at top level of module / inside an impl block
[ ] pub static changed to pub const, with caveats
- lint variants: at top level of module / inside an impl block
- breaking change if the type has interior mutability -- let foo: &'static T = &MY_UNSAFE_CELL_STATIC is fine but doesn't work if the value is a const instead of static
- breaking change if the type is std::mem::needs_drop::<T>() (not the same as T: Drop -- String is not drop itself but its contents need dropping)
  - breaking change is around lifetime promotion: this doesn't work with const but works fine for static
[ ] public API type that implements serde::Serialize + serde::Deserialize gains new fields that are not #[serde(default)]
- This breaks the serialization format.
- Idea from here: https://twitter.com/ManishEarth/status/1647675422648463360
[ ] Lints in #815
[ ] Lints in #816
[x] #954
[ ] #950
[ ] #946
[ ] #940: "This crate with feature set X used to support no_std use, but in the new release it can't be used in no_std anymore"
- Related to the change here, which is breaking for no_std specifically even though it isn't breaking for supertrait reasons: https://github.com/obi1kenobi/cargo-semver-checks/pull/892#issuecomment-2364056127
Breakage related to function pointer types
- [ ] fn pointer became unsafe
- [ ] fn pointer changed ABI
more lint ideas here: https://github.com/rust-lang/cargo/pull/12169

Minor version recommended

[ ] #57
[x] #159
- Code in downstream crates that did not use a value of that type will get a compiler lint.
- New lints for existing code requires a minor version: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#minor-change-introducing-new-lint-warningserrors
949, including:
- new pub struct added
- pub fields added on pub struct
  - don't report this if the entire struct is new
- new pub enum added
- new pub enum variant added
  - don't report this if the entire enum is new
- pub enum variant discriminant added
- new pub inherent method added
  - don't report this if the entire type is new
- new pub union added
[ ] pub type typedef adds a default value for a generic parameter

Project-defined whether major / minor / patch version required

For example, because they are technically breaking but projects may often treat them as non-major.

[ ] Raising the Minimum Supported Rust Version (MSRV) for the crate
[ ] Changing the size of a type
- Example courtesy of cargo-breaking README file

General opt-in warnings

[ ] Depending on a #[non_exhaustive] type from another crate to remain a 1-ZST usable in #[repr(transparent)]
- This is a semver hazard from the user's side. The other crate's type correctly declared that field additions (including sized ones) are non-breaking.
- Related issue: https://github.com/rust-lang/rust/issues/78586
[ ] pub type is not equivalent to pub use -- enum (or pub use) replaced by pub type is currently breaking because use upstream::Enum::* won't work through pub type
- https://github.com/obi1kenobi/cargo-semver-checks/issues/413
- not a major change, as described here: https://predr.ag/blog/re-exporting-enum-with-type-alias-breaking-change-not-major/
- happened in: https://github.com/time-rs/time/issues/675
- will stop being a hazard if this feature is built: https://github.com/rust-lang/rust/issues/73191
[ ] Crate does not enforce some of the recommended allow-by-default lints that are built into Rust and/or clippy: https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html
- For example, own public types should be Debug i.e. the Rust missing_debug_implementations lint: https://twitter.com/Lucretiel/status/1558287048892637184
[ ] Types with a new() -> Self method should be Default: https://rust-lang.github.io/api-guidelines/interoperability.html
[ ] Don't require FusedIterator in generic bounds, instead use Iterator.fuse(): https://doc.rust-lang.org/std/iter/trait.FusedIterator.html
[ ] Newly adding #[inline] on a pub fn in the public API, since that may have unexpected negative perf impact in some cases (e.g. slower cargo test if that crate is a dependency and compiled with opt-level = 3): https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Cargo.20wizard.3A.20automating.20Cargo.20project.20configuration/near/425816132
[ ] public API type that implements serde::Serialize + serde::Deserialize has private fields
- Because those private fields can be manipulated by serializing, editing, then deserializing.
- Idea from here: https://twitter.com/ManishEarth/status/1647680963122724864

Opt-in warnings for difficult-to-reverse changes

[ ] Removing #[non_exhaustive] from an item
- Per semver, removing #[non_exhaustive] can be done in a patch release, but adding it back would then require a new major version.
[ ] Adding an enum variant in a #[non_exhaustive] enum
- Per semver, adding variants to a non-exhaustive enum can be done in a patch release, but removing them again afterward would require a new major version.
[ ] Removing the last non-pub field in an exhaustive public struct
- Structs that are not #[non_exhaustive] and have only public fields can be constructed with a struct literal. Removing the ability to construct a struct with a struct literal is a breaking change and requires a new major version.
[ ] Making an item importable in more than one way
- If an item is in a pub mod and is also exported with pub use, it can become importable in multiple ways. This is easy to miss. Removing an import path is breaking, so perhaps we should warn that this is happening. Related to #35.
[ ] Making a trait object-safe if it previously was not
- Object safety then becomes part of the API contract, and breaking object safety is semver-major.
[ ] A 1-ZST (1-byte-aligned zero-sized-type) type no longer being a 1-ZST
- This is "possibly breaking" and whether it's breaking or not depends on the intent of the type, and can't be determined programmatically. A #[enforce_1zst] attribute could signal that the type should remain a 1-ZST and that deviations from that are breaking.
- This can break downstream even if the type is #[non_exhaustive], until https://github.com/rust-lang/rust/issues/78586 is resolved and prevents this.
[ ] Leaking or re-exporting another crate's type in one's own API
- for example, having a function that returns a value of another crate's API
- this can cause coupling to the other crate's version, and can be a pain
- there are legitimate reasons to do this sometimes, but it should be an intentional decision and probably worth flagging in review
[ ] Making a type Send/Sync/Sized/Unpin or other auto traits, when it previously wasn't.
- this is possible to do indirectly, e.g. by removing the last field that prevented the type from (auto-)implementing those traits
- reverting this is a breaking change

More checks to triage here

CAD97 commented 2 years ago

Own public types should be Debug:

That's already available in the compiler as #[warn(missing_debug_implementations)], isn't it?

obi1kenobi commented 2 years ago

That's already available in the compiler as #[warn(missing_debug_implementations)], isn't it?

Oh, neat, TIL. It appears to be allowed by default and has to be enforced by manually enabling the check. In that case, perhaps the wish-listed query should be checking that #![deny(missing_debug_implementations)] is set instead.

epage commented 2 years ago

This also gets into a conversation that I think we only had over zulip so good to summarize here.

Especially if we want this in cargo some day, I think we should clearly define the scope.

cargo clippy is meant for linting an API as it exists

cargo semver-checks would be meant for linting changes in an API

missing_debug_implementations is an example of something that imo doesn't belong in cargo semver-checks
Linting that a lint is enabled is both getting a bit meta and again something that should be out of scope

Misc notes

Making it easier to add lints to clippy is a conversation with the clippy folks and they are interested in solving it
User-generated lints in either type of tool shipped with rustup would likely be marked as unstable initially. A path to being stable is dependent on how comfortable people are on stabilizing the query language and the data model which is a large surface area
In the mean time, there could be room for a linter that handles user defined lints.

obi1kenobi commented 2 years ago

One possible way forward would be something like:

Extract the data model components (the Trustfall schema and adapter) into a library crate (essentially #67).
Make cargo-semver-checks be just a set of semver queries + a binary that wraps that library crate to execute those queries.
Make one or more other tools for the other use cases: any queries that don't fit within the current cargo-semver-checks / clippy domains, custom user-specified queries, etc.

That way, we could easily experiment with querying for more things without bloating the scope of cargo-semver-checks and without making the integration into cargo messy.

I think extracting the data model into a library crate is pretty straightforward and I would be happy to do it if that's what we decide is the best path forward.

aDotInTheVoid commented 2 years ago

[ ] Auto trait impls for impl Trait in return type.
- Requires doing pub fn changed return type

obi1kenobi commented 2 years ago

[ ] Auto trait impls for impl Trait in return type.

Requires doing pub fn changed return type

Thanks, added to the list! If you'd like to try your hand at it, this lint is probably easier than pub fn changed return type since the actual check is less complex, and I'd be happy to mentor.

oskgo commented 2 years ago

I think "trait added method" might be a bit more complicated.

The way I see it adding default methods is fine, and even adding non-default methods is fine, so long as the trait cannot be implemented by an external user. This can be the case for example when using a private super trait or blanket impls. Especially sealed traits are a common pattern in Rust.

epage commented 2 years ago

even adding non-default methods is fine,

I believe trait added methods are a minor compatibility break. The standard library team is running into this problem with moving functions from the extension trait in itertools to Iterator which is causing them to write a new feature to support this due to the pervasiveness of itertools.

obi1kenobi commented 2 years ago

Non-defaulted items of any kind in a trait that is implementable outside its crate are semver-major, because any implementers must add the new items: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default

Defaulted items in a trait are trickier. They are definitely at least minor, but could be major as well; some such circumstances are described in the semver reference which shows this as a "possibly-breaking" change: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-default-item

I believe trait added methods are a minor compatibility break. The standard library team is running into this problem with moving functions from the extension trait in itertools to Iterator which is causing them to write a new feature to support this due to the pervasiveness of itertools.

I believe this might be due to the introduced ambiguity between the built-in Iterator trait and its itertools analogue, which is captured in the breaking example of the possibly-breaking entry I linked above.

jplatte commented 2 years ago

it's possible to go from e.g. taking &str to taking S: Into<String> without breaking

This is not true, changing an argument type from a concrete type to a generic will break calls like the_function(foo.into()), which only works for non-generic functions because the parameter type guides type inference. There are cases where changing a parameter types as well as a return type is non-breaking though:

Removing trait bounds on parameter types, e.g. x: impl Foo + Bar to x: impl Foo
Adding trait bounds to an existential return type, e.g. -> impl Foo to -> impl Foo + Send

CAD97 commented 2 years ago

I want to note that while Iterator/Itertools is a good example of the issue, it's a symptom of the wider semver-minor upgrade hazard of adding any new items.

This happens because of how name lookup works in Rust, since Rust allows arbitrary namespace mixins.

Adding an item to a trait is name resolution inference breaking, as it could conflict with another trait item where both traits are implemented for the same type.^[Preventable if the trait is sealed and implemented only for types you control.^[If implemented on upstream types, still potentially breaking, if upstream updates; generally we blame downstream if the inference breakage does not happen without downstream, even if it is triggered by updating upstream.]]
Adding a (public) item to a struct/enum is name resolution inference breaking, as it could conflict with a trait item implemented for the type, changing the name from referring to the trait associated item to referring to the struct/enum associated item.
Adding a (public) item to a module is name resolution inference breaking, as downstream could be glob importing your module's contents and another module's contents which defines the same name, causing the name to be ambiguous between your and the other module.

In other words, in a pedantic mode, semver-checks'd be justified on requiring minor for any new public item. Even weakening generic requirements might cause inference issues, so e.g. --strict-pedantic should probably require a minor bump for any change to the public API's types; IIUC this matches the intent of semver-minor's "new feature" trigger as well, since the API by construction has new API surface.

In practice, API evolution in this manner is necessarily considered perfectly acceptable, and is imho very rarely worth warning for. It's a subjective evaluation of how likely both that a name conflict is possible and that some downstream would have both names in scope simultaneously; in most cases this is reasonably rare because of the convention to avoid glob imports^[If you want a version of the lint which can fire without firing on every API change, consider linting only for new trait associated items reachable through a module called prelude, since that's likely designed for glob importing.], and there's not really a good analytical way to determine the risk of a non-globbed name conflict to provide a lint cutoff better than yes/no.

Iterator is an especially interesting case because it's a language item trait in the prelude. User types don't have this exacerbating factor (being implicitly available everywhere) for this concern.

CAD97 commented 2 years ago

Adding trait bounds to an existential return type, e.g. -> impl Foo to -> impl Foo + Send

Note that RPIT already "leaks" autotraits (Send/Sync/Unpin), so that isn't actually a return type refinement.

Actually refining the return type is not-inference-breaking, though you still run the risk of being name-resolution-breaking (e.g. refining to a concrete type or even adding a new guaranteed trait could cause a name conflict with newly applicable extension traits).

epage commented 2 years ago

In practice, API evolution in this manner is necessarily considered perfectly acceptable, and is imho very rarely worth warning for

The fact that there is a lot of nuance to semver and some parts that are contextual is why I feel like https://github.com/obi1kenobi/cargo-semver-check/issues/58 is going to be important.

jhpratt commented 1 year ago

Here's one not listed: adding a generic (type or const) to a function is a breaking change.

obi1kenobi commented 1 year ago

That one is very interesting, and I'm not exactly sure what to make of it.

It's definitely breaking, no doubts there. But as I've written before, some Rust breaking changes don't require a major version — the API evolution RFC says so: https://rust-lang.github.io/rfcs/1105-api-evolution.html

Is adding a generic to an already-generic function covered under that exception?

Is adding a generic to a function that previously wasn't generic covered?

Would love to get your thoughts @jhpratt! These kinds of existential questions are things we run into a lot here 😅

epage commented 1 year ago

Yes, adding a new generic to a function is a major breaking change. It will break in any case where the type parameters are explicitly provided, like if inference didn't work out. This is true for other API items as well with one exception: if a generic is added to a type or trait but is defaulted, then its a minor breaking change. The explicit type parameters are unchanged but there are cases where inference won't work and it will fail to compile and the API Evolution RFC decided to brush that under the rug and ignore it (I've been bitten by it...)

Also, just because the RFC says something is a minor semver breakage, that doesn't mean the user shouldn't be told as that RFC was written specifically for the stdlib and not as guidance for the ecosystem and even the stdlib sometimes goes to extra lengths to avoid minor breaking changes if the impact is large enough. From what I've heard, they are designing a whole new language feature to allow migrating trait methods from itertools to Iterator without breaking people. Granted, #58 will be important for controlling this.

thomaseizinger commented 1 year ago

type no longer implements pub trait

How hard is this one to implement? I assume this covers things like removing the From impl from an error type? I'd like to try myself on that one.

obi1kenobi commented 1 year ago

type no longer implements pub trait

How hard is this one to implement? I assume this covers things like removing the From impl from an error type? I'd like to try myself on that one.

It does cover removing From from a type. Unfortunately, From is generic, and queries over generics are blocked on #241 as the hardest-to-design bit of schema in that issue. I wouldn't recommend it as a starting point, since it's likely to turn into yak shaving.

A better first issue would be something like #368 where the design is reasonably clear already.

In the meantime, I'm going to migrate the adapter implementation from Trustfall v0.2 to Trustfall v0.3 and take advantage of the massively improved ergonomics therein. If you'd like, I can loop you into that as well!

thomaseizinger commented 1 year ago

Leaking or re-exporting another crate's type in one's own API

* for example, having a function that returns a value of another crate's API

* this can cause coupling to the other crate's version, and can be a pain

* there are legitimate reasons to do this sometimes, but it should be an intentional decision and probably worth flagging in review

Two thoughts regarding this:

It would be great if we could somehow specify the intention that a certain item is meant to be hidden from the public API. For example, it is very easy and common to leak a dependency via a From impl for that dependencies Error type.
A reasonable default for the above could be to lint against all dependencies that are < 1.0 and appear in the public API. For a crate that is itself < 1.0, this could be allowed by default but as soon as you bump to 1.0, it should be a warn. If a crate wants to stabilise their public API, they can then opt-in to that lint ahead of time.

See https://rust-lang.github.io/api-guidelines/necessities.html#public-dependencies-of-a-stable-crate-are-stable-c-stable.

epage commented 1 year ago

It would be great if we could somehow specify the intention that a certain item is meant to be hidden from the public API. For example, it is very easy and common to leak a dependency via a From impl for that dependencies Error type.

Except there is no way to convey this intention to your users. If you implement a public trait on a public type, then that is a compatibility boundary. I avoid From for error types for this very reason.

thomaseizinger commented 1 year ago

It would be great if we could somehow specify the intention that a certain item is meant to be hidden from the public API. For example, it is very easy and common to leak a dependency via a From impl for that dependencies Error type.

Except there is no way to convey this intention to your users. If you implement a public trait on a public type, then that is a compatibility boundary. I avoid From for error types for this very reason.

What I meant was, I want to specify to cargo semver-checks that I want crate XYZ not in my public API. If I make a mistake and still include it, it should generate a warning.

epage commented 1 year ago

For that, long term we want https://rust-lang.github.io/rfcs/1977-public-private-dependencies.html

Nemo157 commented 11 months ago

Would it be useful to have a list of known undetected breakages to test against too? https://github.com/RustCrypto/elliptic-curves/issues/984 isn't detected currently and doesn't appear to match any of the checks in the list, it's something like "trait associated type added new required bound".

obi1kenobi commented 11 months ago

Thanks! I updated the list to add that check together with the analogous one for removing bounds from an associated type.

Would it be useful to have a list of known undetected breakages to test against too?

Could you say more about this? I'm curious what form this list would take, and how it would be related to / different from the list in this issue.

Nemo157 commented 11 months ago

Rather than being a list of checks, just a list of version pairs that have seen known ecosystem breakage, but pass all current checks. Maybe even something that can run in CI automatically to see if they start being detected.

obi1kenobi commented 11 months ago

Sorry, I'm still having a bit of trouble understanding the exact suggestion, and who the target audience is / how they benefit.

Would this list of version pairs be something posted in this issue, or part of cargo-semver-checks itself in some way?

When you say "run in CI automatically," is that referring to cargo-semver-checks' own CI, or in the CI of users of cargo-semver-checks?

Sorry I'm having a hard time following here. If it's easier to "show, not tell" I'd be happy to look at a PR too.

Skgland commented 11 months ago

Based on https://github.com/rust-lang/rfcs/pull/3535#discussion_r1429134787 which I reproduced in https://github.com/Skgland/rust-semver-break.

It is currently possible in some cases to match non-exhaustive structs exhaustively, resulting in a breaking change if such a struct is change to have more states (i.e. by adding a field with more than one value).

This is the case if the struct is StructuralPartialEq (constants of the type can be used as a pattern in match) and all possible values of the struct have an accessible constant.

This appears to be missing from this list, though I dought that it is feasible to detect.

obi1kenobi commented 11 months ago

Wow, that's quite the semver hazard! Thanks for pointing it out.

My preference, as I mentioned in the linked issue, would be to either error or lint on this inside rustc or clippy, since a #[non_exhaustive] type having exhaustive semantics seems to me like an accidental language or compiler bug.

If that doesn't pan out, we can look into our options here and see if we can check for StructuralPartialEq in some way.

parasyte commented 5 months ago

"Auto trait impls for impl Trait in return type" came up recently on URLO: Implicit Unpin on impl Any, breaking change possible

obi1kenobi commented 3 months ago

I have figured out how to properly check if a trait is sealed or not, and I've opened #870 with a list of related lints that are now ready to be implemented!

This took 9 months to get right, and I'm excited it's finally there! 🙌

obi1kenobi / cargo-semver-checks