Skip to content
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

Re-Rebalancing Coherence #2451

Merged
merged 5 commits into from Oct 28, 2018
Merged

Conversation

@sgrif
Copy link
Contributor

@sgrif sgrif commented May 30, 2018

RFC #1023 introduced rules that exclude impls which should clearly be
valid. It also used some ambiguous language around what is a breaking
change, that ended up being completely ignored and contradicted by #1105.
This RFC seeks to clarify what is or isn't a breaking change when it
comes to implementing an existing trait, and conservatively expands the
orphan rules to allow impls which do not violate coherence, and fit
within the original goals of #1023.

Rendered
Tracking issue

RFC #1023 introduced rules that exclude impls which should clearly be
valid. It also used some ambiguous language around what is a breaking
change, that ended up being completely ignored and contradicted by #1105.
This RFC seeks to clarify what is or isn't a breaking change when it
comes to implementing an existing trait, and conservatively expands the
orphan rules to allow impls which do not violate coherence, and fit
within the original goals of #1023.
@Centril Centril added the T-lang label May 30, 2018
@Diggsey
Copy link
Contributor

@Diggsey Diggsey commented May 30, 2018

Honestly, this RFC is the clearest explanation of the coherence rules I've seen to date - the proposed rules actually seem simpler to understand with less special casing.

Is it necessarily true that all blanket impls are a breaking change? At the very least, new traits can obviously have blanket impls backwards compatibly, but even for existing traits, aren't there any cases where a blanket impl can be safely added?

I hope in the long term there will be some way for the author of the type/trait to customize the coherence rules (like a more advanced version of the #[fundamental] attribute) rather than having to work around them.

@Ixrec
Copy link
Contributor

@Ixrec Ixrec commented May 30, 2018

Honestly, this RFC is the clearest explanation of the coherence rules I've seen to date - the proposed rules actually seem simpler to understand with less special casing.

I'd go farther than that: This is the only explanation/proposal of the actual coherence rules (as opposed to a pre-rigorous understanding of "why coherence matters") that I ever understood, full stop. Now that I'm looking at it, the formal definition in #1023 is just incomplete and ambiguous (although I assume it was intended to mean something similar to what this new RFC proposes).

Given `impl<P1...Pn> Trait<T1...Tn> for Target`, an impl is valid only if at
least one of the following is true:

- `Trait` is a local trait

This comment was marked as off-topic.

@cramertj

cramertj May 30, 2018
Member

Edit: disregard-- this is disallowed because of missing negative reasoning in the conflict checker, not because of coherence.

If i understand correctly, this formulation of the rules also allows things like this:

trait A<P> {}
trait B<P> {}

impl<P> B<P> for i32 {}

impl<P, T: A<P>> B<P> for T {}

The first of these conditions (Trait is a local trait) is satisfied because B<P> is defined in the current crate, so the impl is allowed.

I originally brought up my desire to do this long ago in this thread, and @nikomatsakis pointed out that allowing this impl means that downstream traits have to check for conflicts created by a combination of local and non-local impls, which was undesirable. @nikomatsakis can you elaborate on the issues you thought this would cause?

This comment has been minimized.

@sgrif

sgrif May 30, 2018
Author Contributor

@cramertj This isn't even about the negative reasoning case. It's because this impl is allowed: impl A<LocalStruct> for i32, which would cause impl<P, T: A<P>> B<P> for T to conflict with impl<P> B<P> for i32. We want to continue to allow this for the reasons I've laid out in this RFC. Replace A with QueryFragment ("compile AST to SQL" in Diesel), LocalStruct with "database backend", and i32 with "concrete type representing an AST node requiring special syntax for a single database", and hopefully it's more clear why we want to allow that hypothetical impl to exist

@sgrif
Copy link
Contributor Author

@sgrif sgrif commented May 30, 2018

Is it necessarily true that all blanket impls are a breaking change? At the very least, new traits can obviously have blanket impls backwards compatibly, but even for existing traits, aren't there any cases where a blanket impl can be safely added?

With the set of impls that are accepted today, there is no case where a blanket impl can be added for an existing trait with an existing type. impl<T> SomeTrait<T> for NewType is of course completely valid, since nothing could be depending on NewType yet. While the wording is vague, this is the original intent behind #1023.

I hope in the long term there will be some way for the author of the type/trait to customize the coherence rules

I agree, and I think many others do as well. However, any substantial change to coherence in that direction will require much more work, will almost certainly be completely blocked on chalk, and need to be tied to an edition. My goal with this RFC was for something more conservative that is able to land more quickly, regardless of whether or not we want to develop a more comprehensive solution in the future.

@rosekunkel
Copy link

@rosekunkel rosekunkel commented May 31, 2018

Given impl<P1...Pn> Trait<T1...Tn> for Target, an impl is valid only if ...

Just to be clear, these are not meant to be the same ns, right? It might be more clear as impl<P1...Pn> Trait<T1...Tm> for Target.

@warlord500
Copy link

@warlord500 warlord500 commented May 31, 2018

awesome write up.
however minor nitpick, your use of double negatives was a little confusing in
the phrase below

Target is not an uncovered type parameter (is not any of P1...Pn)

I think you should simplify it to

Target is a covered type parameter(is not any of P1...Pn)

locality.

Covered Type: A type which appears as a parameter to another type. For example,
`T` is uncovered, but `Vec<T>` is covered. This is only relevant for type

This comment has been minimized.

@durka

durka May 31, 2018
Contributor

Should this say that "T is covered in Vec<T>" rather than "Vec<T> is covered"?

This comment has been minimized.

@Centril

Centril Sep 23, 2018
Contributor

I'd like to see this paragraph updated per @durka's notes.

@abonander
Copy link

@abonander abonander commented May 31, 2018

Is it necessarily true that all blanket impls are a breaking change? At the very least, new traits can obviously have blanket impls backwards compatibly, but even for existing traits, aren't there any cases where a blanket impl can be safely added?

Have we given up on specialization? If the new blanket impl is a default impl it can always be added backwards compatibly, right?

In languages without coherence, the compiler has to have some way to choose
which implementation to use when multiple implementations could apply. Scala
does this by having complex scope resolution rules for "implicit" parameters.
Haskell does this by picking one at random.

This comment has been minimized.

@durka

durka May 31, 2018
Contributor

To be fair, it seems that Haskell solves the problem by erroring if you import modules with conflicting impls in them, unless you use a flag which is generally considered a terrible idea, in which case it can choose "arbitrarily".

This comment has been minimized.

@Centril

Centril May 31, 2018
Contributor

Yep. The phrasing is not reflective of actual Haskell development. {-# LANGUAGE IncoherentInstances #-} are pretty much a "never do this" and -fno-warn-orphans is also considered bad practice.

cc @ekmett who gave the "Type Classes vs. the World" talk and @glaebhoerl.

This comment has been minimized.

@Centril

Centril Sep 23, 2018
Contributor

So for fairness towards the Haskell community and for accuracy, please note somehow that Haskell does have the coherence property. If you also want to elaborate on Scala, that's also fine; perhaps expand on this in a section on "Prior art" (which is missing btw...)

@newpavlov
Copy link
Contributor

@newpavlov newpavlov commented May 31, 2018

In digest crate I have traits which describe functionality of cryptographic hash functions. One of the traits is:

pub trait Input {
    fn process(&mut self, input: &[u8]);
}

And ideally I would like to write blank impl for io::Write:

impl<T: Input> io::Write for T {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> { .. }
}

But obviously it can not be done under current coherence rules, thus I have to implement io::Write for all hash functions manually. If I understand correctly this RFC still disallows such implementations. I think a good approach to this particular problem will be "negative trait bounds", which have been proposed several times, but seems without any results. This feature will allow me to write the code like this:

// `!io::Write` bound means that other crates can't implement
// both `Input` and `io::Write` on the same type
pub trait Input: !io::Write {
    fn process(&mut self, input: &[u8]);
}

// But locally we can define blank impl of `io::Write`, because compiler
// guarantees that no one else can implement it for `T: Input`
impl<T: Input> io::Write for T {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> { .. }
}
@abonander
Copy link

@abonander abonander commented May 31, 2018

@newpavlov Why can't you have something like this:

pub struct InputWriter<T>(T);

impl<T: Input> Write for InputWriter<T> {}
@newpavlov
Copy link
Contributor

@newpavlov newpavlov commented May 31, 2018

@abonander
Because io::copy(&mut my_file, InputWriter::new(&mut hasher)) is less convenient and harder to explain/discover than io::copy(&mut my_file, &mut hasher). Plus negative trait bounds have various applications. Also see motivation for #1148 .

@tspiteri
Copy link

@tspiteri tspiteri commented May 31, 2018

Covered Type: A type which appears as a parameter to another type. For example, T is uncovered, but Vec<T> is covered. This is only relevant for type parameters.

Is (T,) considered covered or uncovered? I think it should be covered, but don't think the current definition covers it. This would have implications about allowing for example impl<T, U> ForeignTrait<LocalType> for (T, U).

@sgrif
Copy link
Contributor Author

@sgrif sgrif commented May 31, 2018

@tspiteri (T,) is covered

- `Target` is not an uncovered type parameter (is not any of `P1...Pn`)
- At least one of the types `T1...Tn` must be a local type. Let `Ti` be the
first such type.
- No type parameters `P1...Pn` may appear *anywhere* in `T1...Tn` before `Ti`.

This comment has been minimized.

@ExpHP

ExpHP May 31, 2018

I think "anywhere" is a bit ambiguous, as it could mean "as any of the type arguments" or "within any of the type arguments". (Though from the emphasis placed, and because the term "uncovered" is not used, I assume you mean the latter)

I might forego the emphasis and write

anywhere (even covered)


Rust's solution is to enforce that there is only one impl to choose from at all.
While the rules required to enforce this are quite complex, the result is easy
to reason about, and is generally considered to be quite important for Rust.

This comment has been hidden.

@Centril

Centril May 31, 2018
Contributor

Important yes; easy to reason about... not so much; or at least, I've never felt it was so easy to tell what impls will be accepted and which will be rejected.

This comment has been hidden.

@Diggsey

Diggsey May 31, 2018
Contributor

I think it means that it's easy to reason about the behaviour of the code (because there can only be one possible implementation) rather than the coherence rules themselves being easy to reason about.

This comment has been hidden.

@dwijnand

dwijnand May 31, 2018
Member

Yeah, the result is easy to reason about, but not necessarily the rules.

This comment has been hidden.

@Centril

Centril May 31, 2018
Contributor

Right. That reading does make sense. Cheers.

@leoyvens
Copy link

@leoyvens leoyvens commented Jun 1, 2018

(T,) is covered

@sgrif Are you sure? Tuples are fundamental, iirc

@sgrif
Copy link
Contributor Author

@sgrif sgrif commented Jun 1, 2018

@leodasvacas Tuples are definitely not fundamental

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 22, 2018

Sorry I've been slow to leave comments here. I am basically 👍 on this RFC. I had a few nitpicks on the phrasing, though I think others have mostly left similar comments; I'll have to go over my notes later.


I did also have one particular question: I feel like the rules as stated in the RFC are unnecessarily narrow. The RFC states:

Given impl<P1...Pn> Trait<T1...Tn> for Target, an impl is valid only if at
least one of the following is true:

  • Trait is a local trait
  • Target is a local type
  • All of
    • Target is not an uncovered type parameter (is not any of P1...Pn)
    • At least one of the types T1...Tn must be a local type. Let Ti be the
      first such type.
    • No type parameters P1...Pn may appear anywhere in T1...Tn before Ti.

It surprised me to see the Target type drawn out specially... I suspect we can make a more general change. For example, this change permits

impl ForeignTrait1<LocalType> for ForeignType<T>

but it forbids

impl ForeignTrait2<ForeignType<T>, LocalType> for u32

Granted, the number of traits that include enough type parameters to make this relevant are few.

Nonetheless, I think I would prefer to see the final rule without calling out Target specifically. Something like this:

Given impl<P0...Pn> Trait<T1...Tn> for T0, an impl is valid only if at
least one of the following is true:

  • Trait is a local trait
  • All of
    • At least one of the types T0..=Tn must be a local type. Let Ti be the
      first such type.
    • No type parameters P0..=Pn appear uncovered in T0..Ti (excluding Ti).

The major change from today being that we do not forbid type parameters "deeply" from T0..Ti, but rather just forbid uncovered type parameters.

Is there a reason we can't do this? It seems clear that this creates no more conflict with parent crates than existed before: for them to overlap with us, they must use an uncovered type parameter at position Ti (which you correctly observe would be a semver violation to add, despite some confusing wording elsewhere). (Update: fixed this paragraph, which previously ended mid-sentence.)

In terms of sibling or cousin crates, I think the argument is similar. Assume there are two cousin crates (neither is a (transitive) dependency of the other) with impls of the same trait Trait:

  • Let i and j be the positions where each crate has its local type
    • since Trait is used by both crates, it cannot be located in either crate, as they are not dependencies of one another
  • If i == j, then these two types clearly cannot equate, as they are local to different crates.
  • Otherwise, let i be the lower index, and let us say it is in crate A and call the other crate B
    • Then crate B must have a "remote type" at position i (e.g., Vec<T> and so forth)
    • This cannot equate with the local type in crate A

Is there a flaw in this reasoning?

I guess the question is why we used the "no type parameters at all" rule in the first place. I think this has to do with historical context. It's worth remembering that these rules arose before RFC 1023 ("rebalancing coherence"), and in that setting we didn't have a notion of fundamental types. I remember that we were concerned about the symmetry of impl<T> Foo<T> for A and impl<T> Foo for (A, T). In particular, we wanted to accept something like (Vec<T>, T) but reject (Foo, T), because T was "uncovered" in the latter. Because tuples are not fundamental, this no longer seems to be an issue.


More generally, @sunjay and I have been working over in Chalk land on modeling the coherence rules and I think we're starting to make some nice progress. This work dovetails pretty nicely with this RFC. To keep this comment from getting too crazy long I'll hold off on commenting too much about it here. Maybe they can write more, or maybe I'll drop some tidbits later. The two main things we've been focused on are separating out the "axioms" (i.e., the things we define by fiat) -- from the "theorems" (i.e., the things we believe follow from that), as well as trying to state the rules in terms of chalk predicates. For example, there is a predicate IsUpstream(T) which states if a type is known to be "owned" by an upstream crate and so forth. (We've been considering also the effects of this RFC on such models.)

@ErichDonGubler
Copy link

@ErichDonGubler ErichDonGubler commented Jun 22, 2018

@nikomatsakis: Um, did you mean to finish this sentence with something other than "and"? Your comment was really interesting to read otherwise...

they must use an uncovered type parameter at position Ti, and (as you correctly observe).

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 28, 2018

@ErichDonGubler heh I suppose I did :)

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 28, 2018

Changed to:

Is there a reason we can't do this? It seems clear that this creates no more conflict with parent crates than existed before: for them to overlap with us, they must use an uncovered type parameter at position Ti (which you correctly observe would be a semver violation to add, despite some confusing wording elsewhere). (Update: fixed this paragraph, which previously ended mid-sentence.)

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 30, 2018

An impl of

impl<T> ForeignTrait<LocalType> for ForeignType<T>

Requires that cousin crates are not able to write the following impl

impl<T> ForeignTrait<T> for ForeignType<CousinLocalType>

If ForeignType is #[fundamental] (for example, consider the case where ForeignType = Box), then, by the current rules, the cousin impl is legal, which means the "original" impl must not be.

If ForeignType is not #[fundamental], the current rules forbid the cousin impl, but they don't take it being forbidden into account - i.e. adding #[fundamental] to a type is not a breaking change, while with this RFC, it must be.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 30, 2018

So, taking fundamental types into account, the "new" rules could be:

The predicate we want to define is "trait-ref R is orphan-owned by crate C". We split types into 3 categories:

  1. Types local to C - types that are defined in C
  2. Non-local types known to C - types that are defined in parents of C
  3. Types not known to C - types from sibling crates

The rules are:

  1. An impl for a trait local in C is always orphan-owned by C.
  2. Otherwise, a trait-ref T_0: Foo<T_1, T_2...> (note that T_0 comes before T_1, T_2...) for a trait is orphan-owned by C if there's a local key parameter T_i such that:
    2.a. T_i contains a local type behind an unbroken sequence of #[fundamental] types (which "implicitly" must be known to this crate).
    2.b. For all parameters T_j where j <= i, for every instance of a type not known to C, that instance is behind an unbroken sequence of #[fundamental] types, followed by either a local type or a non-#[fundamental] known type. The non-#[fundamental] known type part is introduced by this RFC.

Connections

An impl impl<T0, T1> Foo<T1> for T0 is orphan-legal in crate C iff for every instantiation of T0/T1, the resulting trait-ref is orphan-owned by C.

Coherence can perform negative reasoning on a trait-ref R if either:

  1. R comes from a #[fundamental] trait, and no unknown crate can own R.
  2. R comes from a non-#[fundamental] trait, and this crate owns R.

This is exactly the future-compatibility rules - adding an impl to a trait is future-compatible if that trait is non-#[fundamental], and no other crate can orphan-own any instantiation of that impl.

Unbroken rules

The "unbroken sequence of #[fundamental] types" part in 2.b. isn't strictly necessary for soundness - it basically says whether we allow this impl:

impl<T> RemoteTrait for FundamentalPair<LocalType<T>, Vec<LocalType<T>>>

However, negative reasoning depends on the orphan rules, so any change in the orphan rules is a breaking change.

Soundness proof

To show that this is sound, suppose that there is a trait-ref R and 2 cousin crates C1 and C2 that impl it. This means R is orphan-owned by both C1 and C2.

Suppose T_i is the key parameter in C1, T_j is the key parameter in C2. WLOG j <= i. By 2.a., T_j contains a C2-local type behind a sequence of #[fundamental] types known to C2.

However, these types can't be local in C1 (because C1 and C2 are cousin crates), and the C2-local type is not known in C1, that type can't be behind either a local type or a non-#[fundamental] known type,

@sgrif
Copy link
Contributor Author

@sgrif sgrif commented Jul 4, 2018

Just so folks know, I'm on vacation but will reply in a few days

@aturon
Copy link
Member

@aturon aturon commented Oct 10, 2018

@nikomatsakis you have one outstanding concern, "clarify-semver". Has this been addressed?

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 17, 2018

@Centril that summary of Haskell's flags sounds like what I remember. I don't think we need to include that quantity of detail in the RFC, however. =)

UPDATE Perhaps including a link to the excellent comment by @goldfirere would be nice though (comment)

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 17, 2018

@rfcbot resolve clarify-semver

Good enough I suppose.

@rfcbot
Copy link

@rfcbot rfcbot commented Oct 17, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link

@rfcbot rfcbot commented Oct 27, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril Centril merged commit 37b657e into rust-lang:master Oct 28, 2018
@Centril
Copy link
Contributor

@Centril Centril commented Oct 28, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#55437

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 1, 2018

In cases like this, should the RFC that this supersedes get a prominent link to the RFC it is superseded by?

@steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment