From 4c32c86fdab78610a8e2bd4962bd41ca1ef721db Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Wed, 30 May 2018 14:18:58 -0600 Subject: [PATCH 1/5] Re-Rebalancing Coherence 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. --- text/0000-re-rebalancing-coherence.md | 299 ++++++++++++++++++++++++++ 1 file changed, 299 insertions(+) create mode 100644 text/0000-re-rebalancing-coherence.md 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 From 6be59ca8510ab9966b4ea80ab9b57438f6fcb1e3 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Tue, 18 Sep 2018 16:13:00 -0600 Subject: [PATCH 2/5] Address feedback --- text/0000-re-rebalancing-coherence.md | 48 ++++++++++++++++++--------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/text/0000-re-rebalancing-coherence.md b/text/0000-re-rebalancing-coherence.md index abde10e4cb9..345d0761fab 100644 --- a/text/0000-re-rebalancing-coherence.md +++ b/text/0000-re-rebalancing-coherence.md @@ -181,23 +181,21 @@ definition of blanket impl given above. Assumes the same definitions [as above](#definitions). -Given `impl Trait for Target`, an impl is valid only if at +Given `impl Trait for T0`, 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 + - At least one of the types `T0..=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`. + - No uncovered type parameters `P1..=Pn` may appear in `T0..Ti` (excluding + `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`. +The primary change from the rules defined in in [RFC #1023] is that we only +restrict the appearance of *uncovered* type parameters. Once again, it is +important to note that for the purposes of coherence, `#[fundamental]` types are +special. `Box` is not considered covered, and `Box` is considered +local. Under this proposal, the orphan rules continue to work generally as they did before, with one notable exception; We will permit `impl @@ -228,14 +226,34 @@ conflicting impls, with or without this proposal. ## 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`). +considered a breaking change unless, given `impl Trait for +T0`: + +- At least one of the types `T0..=Tn` must be a local type, added in this + revision. Let `Ti` be the first such type. +- No uncovered type parameters `P1..=Pn` appear in `T0..Ti` (excluding `Ti`) + +The more general way to put this rule is: "Adding an impl to an existing trait +is a breaking change if it could possibly conflict with a legal impl in a +downstream crate". 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. +major breaking change. + +To be specific, the following adding any of the following impls would be +considered a breaking change: + +- `impl OldTrait for OldType` +- `impl OldTrait for T` +- `impl OldTrait for ForeignType` + +However, the following impls would not be considered a breaking change: + +- `impl NewTrait for AnyType` +- `impl OldTrait for NewType` +- `impl OldTrait for OldType` # Drawbacks [drawbacks]: #drawbacks From 8474222704ee12f1898863bf2bfb39d98aa46aef Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Fri, 21 Sep 2018 16:44:53 -0600 Subject: [PATCH 3/5] typo --- text/0000-re-rebalancing-coherence.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-re-rebalancing-coherence.md b/text/0000-re-rebalancing-coherence.md index 345d0761fab..e2f9a31412f 100644 --- a/text/0000-re-rebalancing-coherence.md +++ b/text/0000-re-rebalancing-coherence.md @@ -199,7 +199,7 @@ local. 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 +ForeignTrait for ForeignType`. This is completely valid under the forward compatibility rules set in [RFC #1023]. We can demonstrate that this is the case with the following: From f27e769e1505035d0a2bc225993c9683c7005be2 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Tue, 9 Oct 2018 16:50:11 -0600 Subject: [PATCH 4/5] Fix nits --- text/0000-re-rebalancing-coherence.md | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/text/0000-re-rebalancing-coherence.md b/text/0000-re-rebalancing-coherence.md index e2f9a31412f..cf1756064eb 100644 --- a/text/0000-re-rebalancing-coherence.md +++ b/text/0000-re-rebalancing-coherence.md @@ -63,18 +63,18 @@ local or not has nothing to do with type parameters. Given `trait Foo`, 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. +`Vec` is not. `LocalType` is local. Type aliases and trait +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. +`T` is uncovered, but the `T` in `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`. +Vec` is not a blanket impl, as all instances of `T` 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 @@ -93,7 +93,8 @@ 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. +Haskell (when a discouraged flag is enabled) does this by picking an impl +arbitrarily. 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 From d533b8d8125dd12256fe8257c9ca9d116369e7f2 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sun, 28 Oct 2018 11:37:49 +0100 Subject: [PATCH 5/5] RFC 2451 --- ...alancing-coherence.md => 2451-re-rebalancing-coherence.md} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename text/{0000-re-rebalancing-coherence.md => 2451-re-rebalancing-coherence.md} (98%) diff --git a/text/0000-re-rebalancing-coherence.md b/text/2451-re-rebalancing-coherence.md similarity index 98% rename from text/0000-re-rebalancing-coherence.md rename to text/2451-re-rebalancing-coherence.md index cf1756064eb..f4f1625a0d5 100644 --- a/text/0000-re-rebalancing-coherence.md +++ b/text/2451-re-rebalancing-coherence.md @@ -1,7 +1,7 @@ - Feature Name: `re_rebalancing_coherence` - Start Date: 2018-05-30 -- RFC PR: (leave this empty) -- Rust Issue: (leave this empty) +- RFC PR: [rust-lang/rfcs#2451](https://github.com/rust-lang/rfcs/pull/2451) +- Rust Issue: [rust-lang/rust#55437](https://github.com/rust-lang/rust/issues/55437) # Summary [summary]: #summary