diff --git a/text/0000-re-rebalancing-coherence.md b/text/0000-re-rebalancing-coherence.md new file mode 100644 index 00000000000..abde10e4cb9 --- /dev/null +++ b/text/0000-re-rebalancing-coherence.md @@ -0,0 +1,299 @@ +- Feature Name: `re_rebalancing_coherence` +- Start Date: 2018-05-30 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +This RFC seeks to clarify some ambiguity from [RFC #1023], and expands it to +allow type parameters to appear in the type for which the trait is being +implemented, regardless of whether a local type appears before them. More +concretely, it allows `impl ForeignTrait for ForeignType` to be +written. + +# Motivation +[motivation]: #motivation + +For better or worse, we allow implementing foreign traits for foreign types. For +example, `impl From for Vec` is something any crate can write, even +though `From` is a foreign trait, and `Vec` is a foreign type. However, under +the current coherence rules, we do not allow `impl From for Vec`. + +There's no good reason for this restriction. Fundamentally, allowing `for +Vec` requires all the same restrictions as allowing `Vec`. +Disallowing type parameters to appear in the target type restricts how crates +can be extended. + +Consider an example from Diesel. Diesel constructs an AST which represents a SQL +query, and then provides a trait to construct the final SQL. Because different +databases have different syntax, this trait is generic over the backend being +used. Diesel wants to support third party crates which add new AST nodes, as +well as crates which add support for new backends. The current rules make it +impossible to support both. + +The Oracle database requires special syntax for inserting multiple records in a +single query. However, the impl required for this is invalid today. `impl<'a, T, +U> QueryFragment for BatchInsert<'a, T, U>`. There is no reason for this +impl to be rejected. The only impl that Diesel could add which would conflict +with it would look like `impl<'a, T> QueryFragment for BatchInsert<'a, Type1, +Type2>`. Adding such an impl is already considered a major breaking change by +[RFC #1023], which we'll expand on below. + +For some traits, this can be worked around by flipping the self type with the +type parameter to the trait. Diesel has done that in the past (e.g. +`T: NativeSqlType` became `DB: HasSqlType`). However, that wouldn't work +for this case. A crate which adds a new AST node would no longer be able to +implement the required trait for all backends. For example, a crate which added +the `LOWER` function from SQL (which is supported by all databases) would not be +able to write `impl QueryFragment> for DB`. + +Unless we expand the orphan rules, use cases like this one will never be +possible, and a crate like Diesel will never be able to be designed in a +completely extensible fashion. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +## Definitions + +Local Trait: A trait which was defined in the current crate. Whether a trait is +local or not has nothing to do with type parameters. Given `trait Foo`, +`Foo` is always local, regardless of the types used for `T` or `U`. + +Local Type: A struct, enum, or union which was defined in the current crate. +This is not affected by type parameters. `struct Foo` is considered local, but +`Vec` is not. `LocalType` is local. Type aliases do not affect +locality. + +Covered Type: A type which appears as a parameter to another type. For example, +`T` is uncovered, but `Vec` is covered. This is only relevant for type +parameters. + +Blanket Impl: Any implementation where a type appears uncovered. `impl Foo +for T`, `impl Bar for T`, `impl Bar> for T`, and `impl Bar +for Vec` are considered blanket impls. However, `impl Bar> for +Vec` is not a blanket impl, as all types which appear in this impl are +covered by `Vec`. + +Fundamental Type: A type for which you cannot add a blanket impl backwards +compatibly. This includes `&`, `&mut`, and `Box`. Any time a type `T` is +considered local, `&T`, `&mut T`, and `Box` are also considered local. +Fundamental types cannot cover other types. Any time the term "covered type" is +used, `&T`, `&mut T`, and `Box` are not considered covered. + +## What is coherence and why do we care? + +Let's start with a quick refresher on coherence and the orphan rules. Coherence +means that for any given trait and type, there is one specific implementation +that applies. This is important for Rust to be easy to reason about. When you +write `::trait_method`, the compiler needs to know what actual +implementation to use. + +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. + +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. +New features like specialization allow more than one impl to apply, but for any +given type and trait, there will always be exactly one which is most specific, +and deterministically be chosen. + +An important piece of enforcing coherence is restricting "orphan impls". An impl +is orphaned if it is implementing a trait you don't own for a type you don't +own. Rust's rules around this balance two separate, but related goals: + +- Ensuring that two crates can't write impls that would overlap (e.g. no crate + other than `std` can write `impl From for Vec`. If they could, + your program might stop compiling just by using two crates with an overlapping + impl). +- Restricting the impls that can be written so crates can add implementations + for traits/types they do own without worrying about breaking downstream + crates. + +## Teaching users + +This change isn't something that would end up in a guide, and is mostly +communicated through error messages. The most common one seen is [E0210]. The +text of that error will be changed to approximate the following: + +[E0210]: https://doc.rust-lang.org/error-index.html#E0210 + +> Generally speaking, Rust only permits implementing a trait for a type if either +> the trait or type were defined in your program. However, Rust allows a limited +> number of impls that break this rule, if they follow certain rules. This error +> indicates a violation of one of those rules. +> +> A trait is considered local when {definition given above}. A type is considered +> local when {definition given above}. +> +> When implementing a foreign trait for a foreign type, the trait must have one or +> more type parameters. A type local to your crate must appear before any use of +> any type parameters. This means that `impl ForeignTrait, T> for +> ForeignType` is valid, but `impl ForeignTrait> for +> ForeignType` is not. +> +> The reason that Rust considers order at all is to ensure that your +> implementation does not conflict with one from another crate. Without this rule, +> you could write `impl ForeignTrait for ForeignType`, and +> another crate could write `impl ForeignTrait for ForeignType`, +> which would overlap. For that reason, we require that your local type come +> before the type parameter, since the only alternative would be disallowing these +> implementations at all. + +Additionally, the case of `impl ForeignTrait for T` should be +special cased, and given its own error message, which approximates the +following: + +> This error indicates an attempt to implement a trait from another crate for a +> type parameter. +> +> Rust requires that for any given trait and any given type, there is at most one +> implmentation of that trait. An important piece of this is that we disallow +> implementing a trait from another crate for a type parameter. +> +> Rust's orphan rule always permits an impl if either the trait or the type being +> implemented are local to the current crate. Therefore, we can't allow `impl +> ForeignTrait for T`, because it might conflict with another crate +> writing `impl ForeignTrait for LocalTypeCrateB`, which we will always +> permit. + +Finally, [RFC #1105] states that implementing any non-fundamental trait for an +existing type is not a breaking change. This directly condradicts [RFC #1023], +which is entirely based around "blanket impls" being breaking changes. +Regardless of whether the changes proposed to the orphan rules in this proposal +are accepted, a blanket impl being a breaking change *must* be true today. Given +that the compiler currently accepts `impl From for Vec`, adding +`impl From for Vec` must be considered a major breaking change. + +As such, [RFC #1105] is amended to remove the statement that implementing a +non-fundamental trait is a minor breaking change, and states that adding any +blanket impl for an existing trait is a major breaking change, using the +definition of blanket impl given above. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +## Concrete orphan rules + +Assumes the same definitions [as above](#definitions). + +Given `impl Trait 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`. + +Note that this is not just written as the removal of `T0` from the rules defined +in [RFC #1023]. We have the special case rule that `Target` must not be an +uncovered type parameter, because we want to continue to reject `impl +ForeignTrait for T`. This must continue to be rejected to avoid +sibling crates being able to conflict, as this impl would conflict with `impl +ForeignTrait for LocalTypeInCrateB`. + +Under this proposal, the orphan rules continue to work generally as they did +before, with one notable exception; We will permit `impl +ForeignTrait for ForeignType`. This is completley valid under the +forward compatibility rules set in [RFC #1023]. We can demonstrate that this is +the case with the following: + +- Any valid impl of `ForeignTrait` in a child crate must reference at least one + type that is local to the child crate. +- The only way a parent crate can reference the type of a child crate is with a + type parameter. +- For the impl in child crate to overlap with an impl in parent crate, the type + parameter must be uncovered. +- Adding any impl with an uncovered type parameter is considered a major + breaking change. + +We can also demonstrate that it is impossible for two sibling crates to write +conflicting impls, with or without this proposal. + +- Any valid impl of `ForeignTrait` in a child crate must reference at least one + type that is local to the child crate. +- The only way a local type of sibling crate A could overlap with a type used in + an impl from sibling crate B is if sibling crate B used a type parameter +- Any type parameter used by sibling crate B must be preceded by a local type +- Sibling crate A could not possibly name a type from sibling crate B, thus that + parameter can never overlap. + +## Effects on parent crates + +[RFC #1023] is amended to state that adding a new impl to an existing trait is +considered a breaking change if, given `impl Trait for T0`, +any type `T0...Tn` is an uncovered type parameter (is any of `P1...Pn`). + +This clarification is true regardless of whether the changes in this proposal +are accepted or not. Given that the compiler currently accepts `impl From for +Vec`, adding the impl `impl From for Vec` *must* be considered a +major breaking change. Note that the problem is `From`, not `Vec`. Adding +`impl From for Vec` is not a breaking change. + +# Drawbacks +[drawbacks]: #drawbacks + +The current rules around coherence are complex and hard to explain. While this +proposal feels like a natural extension of the current rules, and something many +expect to work, it does make them slightly more complex. + +The orphan rules are often taught as "for an impl `impl Trait for Type`, either +Trait or Type must be local to your crate". While this has never been actually +true, it's a reasonable hand-wavy explanation, and this gets us even further +from it. Even though `impl From for Vec<()>` has always been accepted, +`impl From for Vec` *feels* even less local. While `Vec<()>` only +applies to `std`, `Vec` now applies to types from `std` and any other crate. + +# Rationale and alternatives +[alternatives]: #alternatives + +- Rework coherence even more deeply. The rules around the orphan rule are + complex and hard to explain. Even `--explain E0210` doesn't actually try to + give the rationale behind them, and just states the fairly arcane formula from + the original RFC. While this proposal is a natural extension of the current + rules, and something that many expect to "just work", it ultimately makes them + even more complex. + + In particular, this keeps the "ordering" rule. It still serves *a* purpose + with this proposal, but much less of one. By keeping it, we are able to allow + `impl SomeTrait for ForeignType`, because no sibling crate + can write an overlapping impl. However, this is not something that the + majority of library authors are aware of, and requires API designers to order + their type parameters based on how likely they are to be overidden by other + crates. + + We could instead provide a mechanism for traits to opt into a redesigned + coherence system, and potentially default to that in a future edition. + However, that would likely cause a lot of confusion in the community. This + proposal is a strict addition to the set of impls which are allowed with the + current rules, without an increase in risk or impls which are breaking + changes. It seems like a reasonably conservative move, even if we eventually + want to overhaul coherence. + +- Get rid of the orphan rule entirely. A long standing pain point for crates + like Diesel has been integration with other crates. Diesel doesn't want to + care about chrono, and chrono doesn't want to care about Diesel. A database + access library shouldn't dictate your choice of time libraries, vice versa. + + However, due to the way Rust works today, one of them has to. Nobody can + create a `diesel-chrono` crate due to the orphan rule. Maybe if we just + allowed crates to have incompatible impls, and set a standard of "don't write + orphan impls unless that's the entire point of your crate", it wouldn't + actually be that bad. + +# Unresolved questions +[unresolved]: #unresolved-questions + +- Are there additional implementations which are clearly acceptable under the + current restrictions, which are disallowed with this extension? Should we + allow them if so? + +[RFC #1023]: /~https://github.com/rust-lang/rfcs/blob/master/text/1023-rebalancing-coherence.md +[RFC #1105]: /~https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md