-
-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tracking issue: Additional checks, both semver and non-semver #5
Comments
That's already available in the compiler as |
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 |
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.
Misc notes
|
One possible way forward would be something like:
That way, we could easily experiment with querying for more things without bloating the scope of 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. |
|
Thanks, added to the list! If you'd like to try your hand at it, this lint is probably easier than |
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. |
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 |
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 this might be due to the introduced ambiguity between the built-in |
This is not true, changing an argument type from a concrete type to a generic will break calls like
|
I want to note that while This happens because of how name lookup works in Rust, since Rust allows arbitrary namespace mixins.
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. 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
|
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). |
The fact that there is a lot of nuance to semver and some parts that are contextual is why I feel like #58 is going to be important. |
It does cover removing 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! |
Two thoughts regarding this:
|
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 |
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. |
For that, long term we want https://rust-lang.github.io/rfcs/1977-public-private-dependencies.html |
Would it be useful to have a list of known undetected breakages to test against too? RustCrypto/elliptic-curves#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". |
Thanks! I updated the list to add that check together with the analogous one for removing bounds from an associated type.
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. |
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. |
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 When you say "run in CI automatically," is that referring to 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. |
Based on rust-lang/rfcs#3535 (comment) which I reproduced in https://github.com/Skgland/rust-semver-break. It is currently possible in some cases to match non-exhaustive structs exhaustively, 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. |
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 If that doesn't pan out, we can look into our options here and see if we can check for |
"Auto trait impls for |
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! 🙌 |
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
#[doc(hidden)]
i.e. no longer public API #578#[non_exhaustive]
: New lint: Exhaustive pub enum becomes#[non_exhaustive]
#143repr(C)
plain struct has fields reordered -- two flavors of breakage herepub
, the breakage is "public API and ABI have diverged"pub
, this requires either checking types (Represent type information in the Trustfall schema #149) or field sizes, to determine if "the new field at the old index is semantically equivalent"repr(C)
with same problems as aboverepr(C)
with same problems as above, orrepr
with primitive type on the enum itself since that's equivalent torepr(C)
on all variants: https://doc.rust-lang.org/reference/type-layout.html#primitive-representation-of-enums-with-fields#[derive(PartialOrd)]
,repr(i8)
on the enum, unit variants only enum, etc.union
types #633pub fn
moved, deleted, or renamed (Semver-check for functions being removed. #22, Check for removal of methods and associated functions. #23, Fix for moving method to trait #24)pub fn
changed return type: blocked on Represent type information in the Trustfall schema #149pub fn
added argumentpub fn
removed argumentpub fn
changed arguments in a backward-incompatible way&str
to takingS: Into<String>
without breakingpub fn
stops beingconst
#190pub fn
becomesunsafe
#191#[no_mangle]
or#[export_name]
functions: New lints: functions with explicit ABI export names #502repr(C)
removed from struct or enum (Check for repr(C) removal from structs or enums. #25)repr(transparent)
removed from struct or enum (Check for repr(transparent) removal where it is public ABI. #26, Support PhantomData markers in #[repr(transparent)] check. #28)repr(u*)
andrepr(i*)
changed/removed from enum (Check for #[repr(i*)]/#[repr(u*)] semver violations. #29, Check for changes in #[repr(i/usize)] enums as well. #30)repr(align(...))
added or removed on struct, enum, or union #74repr(align(N))
changed to a lowerN
, if that actually changes the alignment of the typeN
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.repr(align(N))
changed to a higherN
, if that actually changes the alignment of the typerepr(packed)
added or removed on a struct or union #632repr(packed(N))
changed to a lowerN
repr(align(N))
with lowerN
discussed above -- we probably want a lint for thisrepr(packed)
is also valid syntax! theN
is implied asN=1
if missingrepr(packed(N))
changed to a higherN
repr(align(N))
with higherN
discussed above -- we should probably hold off until we can get layout info for fieldsSized / Send / Sync / Unpin / UnwindSafe / RefUnwindSafe
(auto traits) (Check Send/Sync/Sized/Unpin/[Ref]UnwindSafe impls. #31)Copy
(appears to be breaking because of Closure returned asimpl FnOnce
incorrectly borrows captured used-by-value iff it isCopy
rust-lang/rust#100905 )pub trait
moved, deleted, or renamed #73pub trait
method default implementation removed #294unsafe
#231unsafe
trait becomes safe #250unsafe
#605unsafe
#606prelude
module that gets imported with a wildcard&T, Box
etc.)where Self: Sized
bounds on trait associated types and methods #786?Sized
on a trait associated type, breaksfn bad<T: Tr>() -> T::Assoc
: https://twitter.com/withoutboats/status/1701274760653533637?Sized
on a trait associated type, breaks impls that used an unsized type there?Sized
bound from from a generic type in a function or type signature?Sized
bound to a generic type in a function or type signature: Changing?Sized
related trait bounds is not detected #532impl Trait
?Sized
in return positionimpl Trait
(say-> Box<impl Foo>
changing to-> Box<impl Foo + ?Sized>
)impl Trait
in return type.pub fn changed return type
pub type
hides struct's implicit constructor #338pub type
typedef changes the order of generic arguments (regular, lifetime, or const generics) relative to the underlying typepub type
typedef adds a new generic parameterpub type
typedef removes a generic parameterpub type
typedef removes a default value for a generic parameterpub type
typedef changes a default value for a generic parameterimpl Trait
Drop
implementations breaksconst fn
use: AddingDrop
impl to a type used in a public API type is a SemVer hazard #930Drop
implementations changing type parameter lifetime requirementsDrop
" to "contains a generic type that might have an explicitDrop
" is major & breaking: Breaking change toGenerics::lifetimes
in v2.0.73 dtolnay/syn#1718pub fn blah(extern "C" fn())
topub fn blah(extern "C-unwind" fn())
will almost certainly break peoplepub const
moved, deleted, or renamedpub static
is still breaking: cannot usestatic
inconst fn
; cannot initialize another const with a static likepub const MY_CONST = other::PREV_CONST_NOW_STATIC
pub static
moved, deleted, or renamed -- but not changed topub const
impl
blockpub static
changed topub const
, with caveatsimpl
blocklet foo: &'static T = &MY_UNSAFE_CELL_STATIC
is fine but doesn't work if the value is aconst
instead ofstatic
std::mem::needs_drop::<T>()
(not the same asT: Drop
--String
is not drop itself but its contents need dropping)const
but works fine forstatic
serde::Serialize + serde::Deserialize
gains new fields that are not#[serde(default)]
union
types #950no_std
use #940: "This crate with feature set X used to supportno_std
use, but in the new release it can't be used inno_std
anymore"no_std
specifically even though it isn't breaking for supertrait reasons: feat: addtrait_added_supertrait
#892 (comment)unsafe
Minor version recommended
#[must_use]
on an item #159pub type
typedef adds a default value for a generic parameterProject-defined whether major / minor / patch version required
For example, because they are technically breaking but projects may often treat them as non-major.
cargo-breaking
README fileGeneral opt-in warnings
#[non_exhaustive]
type from another crate to remain a 1-ZST usable in#[repr(transparent)]
pub type
is not equivalent topub use
-- enum (orpub use
) replaced bypub type
is currently breaking becauseuse upstream::Enum::*
won't work throughpub type
FormatItem
is a breaking change time-rs/time#675pub type foo = bar
andpub use bar as foo
interchangable in next edition rust-lang/rust#73191Debug
i.e. the Rustmissing_debug_implementations
lint: https://twitter.com/Lucretiel/status/1558287048892637184new() -> Self
method should beDefault
: https://rust-lang.github.io/api-guidelines/interoperability.htmlFusedIterator
in generic bounds, instead useIterator.fuse()
: https://doc.rust-lang.org/std/iter/trait.FusedIterator.html#[inline]
on apub fn
in the public API, since that may have unexpected negative perf impact in some cases (e.g. slowercargo test
if that crate is a dependency and compiled withopt-level = 3
): https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Cargo.20wizard.3A.20automating.20Cargo.20project.20configuration/near/425816132serde::Serialize + serde::Deserialize
has private fieldsOpt-in warnings for difficult-to-reverse changes
#[non_exhaustive]
from an item#[non_exhaustive]
can be done in a patch release, but adding it back would then require a new major version.#[non_exhaustive]
enumpub
field in an exhaustive public struct#[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.pub mod
and is also exported withpub 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 Unsatisfactory UX when items are no longer importable by the old path #35.#[enforce_1zst]
attribute could signal that the type should remain a 1-ZST and that deviations from that are breaking.#[non_exhaustive]
, until repr(transparent) should not consider non_exhaustive types as 1-ZSTs outside their crate rust-lang/rust#78586 is resolved and prevents this.Send/Sync/Sized/Unpin
or other auto traits, when it previously wasn't.More checks to triage here
The text was updated successfully, but these errors were encountered: