Open obi1kenobi opened 1 month ago
cc @ehuss this might be of interest — it appears to be a new set of SemVer hazards that might bear mentioning in the cargo reference's SemVer section.
Thanks for the ping! Just to clarify, is this just a variant of trait-object-safety? That section explicitly says "adding an item", but could be extended to be "adding, or changing an existing item to be non-object-safe"?
Also, one of the todo items in https://github.com/rust-lang/cargo/issues/8736 is to clarify what changes are allowed to a trait item. The old RFC just said no "non-trivial" changes, but didn't define what "trivial" meant. I think adding or removing trait bounds would by default be non-trivial, though I can see where there could be some subtle exceptions.
Just to clarify, is this just a variant of trait-object-safety?
I don't believe so. Even with the Self: Sized
bound on trait Example
's method, the trait itself remains object safe.
In other words, the following code compiles just fine: playground
pub trait Example {
fn method(&mut self) -> Option<i64> where Self: Sized;
}
fn demo(value: &mut dyn Example) {}
Any other trait items that do not require Self: Sized
would continue to be usable, so this could be extended to a realistic, non-trivial example as well.
Also, one of the todo items in https://github.com/rust-lang/cargo/issues/8736 is to clarify what changes are allowed to a trait item. The old RFC just said no "non-trivial" changes, but didn't define what "trivial" meant. I think adding or removing trait bounds would by default be non-trivial, though I can see where there could be some subtle exceptions.
The edge cases are annoyingly difficult to pin down. I've tried and given up several times — it's a couple-week-long rabbit hole minimum, in my estimation.
For example, partially-sealed traits complicate everything. Methods that cannot be called from outside the trait's crate, or methods that cannot be overridden from outside the trait's crate, both make it quite complex to determine when changes to bounds could possibly affect a downstream crate.
This is one of the things I'd love to sit down and properly work out if I can find some funding to make it my full-time job for a few months. If the Rust project or Foundation might be willing to sponsor that work, I'd be happy to do it and even work out how to make the rules machine-checkable in cargo-semver-checks as well.
As requested. These examples don't change object safety but are still breaking changes.
Example 1: Removing Self: Sized
from an associated type is a breaking change.
// Now that you can omit `where Self: Sized` associated types from `dyn Trait`,
// it is a breaking change to remove `Self: Sized` from associated types of
// object-safe traits
pub trait Trait {
type Assoc where Self: Sized;
}
// If the `Self: Sized` bound on `Assoc` is removed, this must change to be
// `dyn Trait<Assoc = SomeConcreteType>`
pub fn example(_: &dyn Trait) {}
Example 2: removing Self: Sized
from self: Self
receiver methods is a breaking change.
pub trait Trait {
fn consume(self) where Self: Sized;
fn other(&self) {}
}
// Check that it's object safe
pub fn example(_: &dyn Trait) {}
// How do you implement `Trait` for non-sized types?
impl Trait for str {
// We can't just omit the method (yet)...
// https://github.com/rust-lang/rfcs/issues/2829
// This errors because you can't pass unsized types by value...
// fn consume(self) {}
// This errors becuase trivial bounds isn't implemented yet...
// https://github.com/rust-lang/rust/issues/48214
// fn consume(self) where Self: Sized {}
// But this workaround for trivial bounds works
// (you still can't call the method)
fn consume(self) where for<'a> Self: Sized {}
}
// But if you remove `where Self: Sized` from the trait, you will break the
// implementation because it is "no longer as general" as the trait.
//
// N.b. the reference says that a "receiver type of `Self` (i.e. `self`)
// implies [`where Self: Sized`]", but as this example demonstrates, that is
// not true. There is some other special carve-out that leaves traits that
// have methods with `self` receivers object safe.
//
// https://doc.rust-lang.org/nightly/reference/items/traits.html#object-safety
//
// I.e. also comment out `impl Trait for str` and the `&dyn Trait` will still
// compile.
(As far as I know, there is currently no way to implement a trait with a self: Self
receiver that lacks a where Self: Sized
bound for unsized types. Perhaps suggesting the where Self: Sized
bound should be a lint...)
Amazing, thank you! A few follow-up questions to make sure I understand all the nuances and can implement the lints accurately:
dyn Trait<Assoc = SomeConcreteType>
instead of just dyn Trait
coming from a linguistic necessity, or from a shortcoming of existing tooling? As you know, Rust's SemVer rules say that breakage from tooling shortcomings are not major. Inference failures that require explicitly specifying types are specifically called out in the API evolution RFC as breaking but non-major, and I'd like to figure out if this one might be in that same bucket too.Self: Sized
bound would not be breaking? I.e. should the lint be "removed trait method bound" or "removed trait method bound on non-sealed trait"?where
bound on trait methods excludes the method from the dyn
-callable portion of the trait? In other words, I believe the new rule is "if the trait method uses where
, you can't call it on dyn Trait
."If you add the bound, the associated type is no longer defined for unsized implementors, and that is a breaking change.
// Uncomment the two `Self: Sized` lines and `main` will break
trait Trait {
type Assoc: Default
where
//Self: Sized,
;
}
impl Trait for str {
type Assoc = ()
where
// This workaround is needed for associated types too...
//for<'a> Self: Sized,
;
}
fn main() {
// ...but the workaround doesn't make the associated type usable
let _ = <<str as Trait>::Assoc>::default();
}
It's linguistic necessity, both due to the reasoning here and the fact that things like method ABI and return types differ between erased base types with different associated types.
Example 2 breaks all unsized implementors, local or downstream. If the trait is sealed and all implementations were for Sized
types, I don't think the example matters. If there were implementations for unsized types, they must have also removed those. That's a breaking change on its own, but perhaps you detect that independently.
It's definitely not "all where
clauses opt the method out of being dyn
-dispatchable". It looks closer to "where TypeInvolvingSelf: NonAutoTrait
make a method non-object safe". (The PR author would be a better person to ask about the precise rules.) So now a where
bound involving Self
in that way on a method is like having a generic type parameter or using Self
in a non-receiver argument or in return position -- your trait is not object safe unless you opt the method out of dyn
-dispatch by also adding where Self: Sized
.
Note that this is more targeted than merely mentioning Self
. where
clauses on associated types are unaffected, for example. It also doesn't apply to lifetime bounds. Or auto-trait bounds. (Again, you should ask the PR author for the precise details.) Example:
#![deny(where_clauses_object_safety)]
use core::fmt::Debug;
trait Trait {
type Assoc;
// These are object safe (before and after the PR)
fn foo_1(&self) where Self: Send;
fn foo_2(&self) where Self::Assoc: Debug;
fn foo_3<'a>(&self, _: &'a str) where Self: 'a;
// This is no longer object safe after the PR
// fn bar(&self) where Self: Debug;
// You can still opt it out of `dyn`-dispatch with `Self: Sized`
// so that the trait as a whole is object safe
fn bar(&self) where Self: Debug + Sized;
}
fn example(_: &dyn Trait<Assoc = ()>) {}
(I'm also assuming the PR matches the lint, since the PR is not in the Playground Beta or Nightly apparently.)
Rust appears to allow trait items (so far, types and methods, in the future possibly also constants) to be excluded from trait objects by applying
Self: Sized
bounds like so:As far as I can tell, it is not required to repeat
where Self: Sized
on the items of trait implementations, so the breakage has to come from attempts to call or use the defined functionality, rather than being immediately caused by a definition lacking awhere Self: Sized
bound.SemVer hazards:
where Self: Sized
is generally a major breaking change for trait methods.where Self: Sized
a major breaking change for trait methods? If so, how?where Self: Sized
a major breaking change for trait associated types by itself, meaning without a simultaneouswhere Self: Sized
bound change on a trait method as well? If so, how?Adding
where Self: Sized
to a trait method is breakingplayground
Caveat: if the method is made non-callable from outside its own crate (i.e. the trait is partially-sealed as described in this post), then this is not a major breaking change since no external crate could have called the method and been broken by its signature change.
This is currently not expressible in a Trustfall query, so it cannot be linted without further work on the query schema.