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

Rebalancing coherence. #1023

Merged
merged 1 commit into from
Mar 31, 2015
Merged

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 27, 2015

I recently realized that our current trait system contains a forward compatibility hazard concerned with negative reasoning. The TL;DR is that negative reasoning (that is, the compiler saying that "doing X is legal if the trait T is NOT implemented for some type U") can easily make it impossible to add impls of any traits in a backwards compatible way. The most obvious example of negative reasoning are negative trait bounds, which have been proposed in a rather nicely written RFC. However, even without negative bounds, the trait system as currently implemented already has some amount of negative reasoning, in the form of the coherence system.

This RFC is fairly simple proposal that tries to strike a good balance between parent and child crates, in terms of permitting parent crates to expand but also giving child crates lots of freedom to define the impls they need. However, it does involve tightening coherence so it is somewhat more restrictive (the current rules are designed to permit as much as possible in the child crates; but this winds up limiting the parent crates).

Some prior discussion is on this internals thread, although it's mostly me talking to myself.

Rendered view

[edited to link to final rendered version]


## Summary

This RFC proposes two rule changes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You list three things after this.

@quantheory
Copy link
Contributor

I don't know that I have thought through the implications enough to be strongly for or against, but I do want to point out that without higher-kinded types, it's sometimes hard to work generically over types of references. This encourages library developers to define interfaces that use mainly &T and &mut T, but if/when HKT lands, using some generic Ref<'a, T> would become more feasible, and possibly more popular for presenting "views" of data. Those objects would presumably have to be fundamental in order to be used similarly to &T.

Question on a different topic: Should Deref be fundamental purely due to the special status it has in the language? I believe that the current rules regarding coercion, autoref, and autoderef for method calls are such that adding a Deref impl is not a breaking change (e.g. we don't bother with an autoderef for a method call if we've already found an applicable impl). But it's not completely obvious to me that this is always the case (or that we want to require this into the indefinite future).

@theemathas
Copy link

I like the general idea, but there are some corner cases, especially with #[fundamental]. I will add some line notes for those.

BTW, I believe that the specifics of #[fundamental] should be moved to the Detailed Design section.

with the following meaning:

- A `#[fundamental]` type `Foo` is one where implementing a blanket
impl over `Foo` is a breaking change. As described, `&` and `&mut` are

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly is a "blanket impl"? Does something like impl<T> SomeTrait for Foo<T, Bar> {...} count?

@pnkfelix
Copy link
Member

cc me

@nikomatsakis
Copy link
Contributor Author

Just to answer the question that was asked a few times as to why the specific set of "fundamental" traits was chosen:

  1. I'm not sure if Sized is actually needed. I added it early on in the code and I forget why. I'll try taking it out to see what happens.
  2. I was trying to resist the temptation to mark a lot of traits as fundamental. I'd prefer to make none. The reason is that every trait we mark as fundamental is an additional constraint that any future generalization has to support. It might be that we find #[fundamental] completely adequate as a scheme for allowing one to balance negative reasoning and possible future expansion, but we may also find some other nifty approach, and I wanted to minimize the set of things that any possible future approach would have to support.

That said, I could certainly see some logic for marking all the "overload" traits (Deref, Fn, Index, Add, etc) as fundamental just because it's a coherent set that should be easy to remember. The fact is that the big step is going from zero fundamental traits to one; going from one to N is somewhat less scary. Still, we can easily expand the set of fundamental traits in a backwards compatible way, so I don't feel a ton of pressure to figure this out this second.

@cristicbz
Copy link

I noticed in the alternative sections, you talk about specialization and bring up the problem of associated types (brilliant catch by the way). I'm curious about this paragraph (my emphasis):

For example, I experimented with a "forward proofing" rule which required that, if one impl is more specialized than another according to this definition, then all of their associated type bindings must match. This was an experiment to see whether it was possible to write some kind of "rules" for how associated types should be based on their inputs. Unfortunately, this proved to be untenable. There are numerous trait patterns that we use all the time where this constraint does not hold.

My use case of specialisation, like you mention earlier in that doc, would be to provide optimised implementations, like your zip example. In that case we would certainly want associated types to match (or at least be covariant). Essentially we want it to be transparent that you're using a specialised impl and adding specialisations should not be a breaking change.

I can't think of any of the "numerous trait patterns" that you mention. I'm sure you're actually right, but could you please elaborate for posterity? The specialisation solution would have made me really happy as it seems both less ad-hoc and less restrictive than the solution proposed in the RFC.

@theemathas
Copy link

@nikomatsakis The main reason I asked about the overloading traits is that some operators perform autoderef / autoref (to be precise, Index, IndexMut, Fn, FnMut, FnOnce), while Deref and DerefMut control autoderef. I am not sure about the impact of this on method resolution.

PS You still have not answered about "what is a blanket impl".

@aturon
Copy link
Member

aturon commented Mar 31, 2015

After much discussion internally, on the internals post, this thread, and the weekly meeting, this RFC has been approved. It is unfortunate to have to move so quickly on a deep change like this, but the existing system of coherence was simply not in a shippable state without it. The design presented here manages to avoid breaking code while not making strong commitments -- the proposed attribute can be left feature-gated, and eventually replaced with an entirely different mechanism or stabilized as-is, depending on our experience.

Tracking issue

Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 1, 2015
…pnkfelix

This PR implements rust-lang/rfcs#1023. In the process it fixes rust-lang#23086 and rust-lang#23516. A few impls in libcore had to be updated, but the impact is generally pretty minimal. Most of the fallout is in the tests that probed the limits of today's coherence.

I tested and we were able to build the most popular crates along with iron (modulo errors around errors being sendable).

Fixes rust-lang#23918.
Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 1, 2015
…dd-impls, r=pnkfelix

The primary purpose of this PR is to add blanket impls for the `Fn` traits of the following (simplified) form:

    impl<F:Fn> Fn for &F
    impl<F:FnMut> FnMut for &mut F

However, this wound up requiring two changes:

1. A slight hack so that `x()` where `x: &mut F` is translated to `FnMut::call_mut(&mut *x, ())` vs `FnMut::call_mut(&mut x, ())`. This is achieved by just autoderef'ing one time when calling something whose type is `&F` or `&mut F`.
2. Making the infinite recursion test in trait matching a bit more tailored. This involves adding a notion of "matching" types that looks to see if types are potentially unifiable (it's an approximation).

The PR also includes various small refactorings to the inference code that are aimed at moving the unification and other code into a library (I've got that particular change in a branch, these changes just lead the way there by removing unnecessary dependencies between the compiler and the more general unification code). 

Note that per rust-lang/rfcs#1023, adding impls like these would be a breaking change in the future. 

cc @japaric
cc @alexcrichton 
cc @aturon 

Fixes rust-lang#23015.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 1, 2015
This PR implements rust-lang/rfcs#1023. In the process it fixes rust-lang#23086 and rust-lang#23516. A few impls in libcore had to be updated, but the impact is generally pretty minimal. Most of the fallout is in the tests that probed the limits of today's coherence.

I tested and we were able to build the most popular crates along with iron (modulo errors around errors being sendable).

Fixes rust-lang#23918.
barosl added a commit to barosl/rust that referenced this pull request Jun 25, 2015
I found some typos in the upcoming 1.1 release note. I corrected them,
but I wanted to go further. So I wrote a script that checks the
integrity of the Markdown references, and ran it against `RELEASES.md`.

This commit fixes some trivial cases, but also removes the following
"unused" references:

- [`Iterator::cloned`](http://doc.rust-lang.org/nightly/core/iter/trait.Iterator.html#method.cloned)
- [`thread::scoped`](http://static.rust-lang.org/doc/master/std/thread/fn.scoped.html)
- [`Debug` improvements](https://github.com/rust-lang/rfcs/blob/master/text/0640-debug-improvements.md)
- [Rebalancing coherence.](rust-lang/rfcs#1023)

However, I think there's a possibility that these features might need to
get descriptions as well. How do you feel about it?
bors added a commit to rust-lang/rust that referenced this pull request Jun 26, 2015
I found some typos in the upcoming 1.1 release note. I corrected them, but I wanted to go further. So I wrote a script that checks the integrity of the Markdown references, and ran it against `RELEASES.md`.

This commit fixes some trivial cases, but also removes the following "unused" references:

- [`Iterator::cloned`](http://doc.rust-lang.org/nightly/core/iter/trait.Iterator.html#method.cloned)
- [`thread::scoped`](http://static.rust-lang.org/doc/master/std/thread/fn.scoped.html)
- [`Debug` improvements](https://github.com/rust-lang/rfcs/blob/master/text/0640-debug-improvements.md)
- [Rebalancing coherence.](rust-lang/rfcs#1023)

However, I think there's a possibility that these features might need to get descriptions as well. How do you feel about it?
bors added a commit to rust-lang/rust that referenced this pull request Jun 26, 2015
I found some typos in the upcoming 1.1 release note. I corrected them, but I wanted to go further. So I wrote a script that checks the integrity of the Markdown references, and ran it against `RELEASES.md`.

This commit fixes some trivial cases, but also removes the following "unused" references:

- [`Iterator::cloned`](http://doc.rust-lang.org/nightly/core/iter/trait.Iterator.html#method.cloned)
- [`thread::scoped`](http://static.rust-lang.org/doc/master/std/thread/fn.scoped.html)
- [`Debug` improvements](https://github.com/rust-lang/rfcs/blob/master/text/0640-debug-improvements.md)
- [Rebalancing coherence.](rust-lang/rfcs#1023)

However, I think there's a possibility that these features might need to get descriptions as well. How do you feel about it?
sgrif added a commit to sgrif/rfcs that referenced this pull request May 30, 2018
RFC rust-lang#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 rust-lang#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 rust-lang#1023.

[Rendered]
sgrif added a commit to sgrif/rfcs that referenced this pull request May 30, 2018
RFC rust-lang#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 rust-lang#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 rust-lang#1023.
@sgrif sgrif mentioned this pull request May 30, 2018
@Centril Centril added A-traits Trait system related proposals & ideas A-typesystem Type system related proposals & ideas A-trait-coherence Proposals relating to coherence or orphans. labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-coherence Proposals relating to coherence or orphans. A-traits Trait system related proposals & ideas A-typesystem Type system related proposals & ideas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants