From 79c7dddccce9ea96c4babda337cf0ded1a4c95d8 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 7 Apr 2018 15:57:47 -0600 Subject: [PATCH 1/7] Introduce `#[do_not_recommend]` to control errors for trait impls --- text/0000-do-not-recommend.md | 246 ++++++++++++++++++++++++++++++++++ 1 file changed, 246 insertions(+) create mode 100644 text/0000-do-not-recommend.md diff --git a/text/0000-do-not-recommend.md b/text/0000-do-not-recommend.md new file mode 100644 index 00000000000..a9e9bb68eb3 --- /dev/null +++ b/text/0000-do-not-recommend.md @@ -0,0 +1,246 @@ +- Feature Name: `do_not_recommend` +- Start Date: 2018-04-07 +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +A new attribute can be placed on trait implementations: `#[do_not_recommend]`. +This attribute will cause the compiler to never recommend this impl transitively +as a way to implement another trait. For example, this would be placed on +`impl IntoIterator for T`. The result of this is that when `T: +IntoIterator` fails, the error message will only mention `IntoIterator`. It will +not say "perhaps `Iterator` should be implemented?". + +# Motivation +[motivation]: #motivation + +When a type fails to implement a trait, Rust has the wonderful behavior of +looking at possible *other* trait impls which might cause the trait in question +to be implemented. This is usually a good thing. For example, when using Diesel, +this is why instead of telling you `SelectStatement<{30 page long type}>: +ExecuteDsl is not satisifed`, it instead tells you `posts::id: +SelectableExpression is not satisifed`. + +However, there are times where this behavior actually makes the resulting error +more confusing. There are specific trait impls which almost always cause these +error messages to be more confusing. These are usually (but not always) very +broad blanket impls on traits with names like `IntoFoo` or `AsBar`. One such +problem impl is `impl IntoIterator for T`. + +## `IntoIterator` confusion + +Let's look at the struggles of a hypothetical Python programmer who is getting +into Rust for the first time. In Python, tuples are iterable. So our python +programmer writes this code expecting it to work: + +```rust +for i in (1, 2, 3) { + println!("{}", i); +} +``` + +The get the following error: + +``` +error[E0277]: the trait bound `({integer}, {integer}, {integer}): std::iter::Iterator` is not satisfied + --> src/main.rs:2:14 + | +2 | for x in (1, 2, 3) { + | ^^^^^^^^^ `({integer}, {integer}, {integer})` is not an iterator; maybe try calling `.iter()` or a similar method + | +``` + +This error message is particularly bad for a failed `IntoIterator` constraint. +The only type in `std` which has a method called `iter` that doesn't implement +`IntoIterator` is a fixed sized array. For all of those types, it's generally +more idiomatic to just put an `&` in front of the value. And for this case, +neither one would be helpful even if it worked, since our hero is likely +expecting `x` to be `i32`, not `&i32`. + +Following the advice of the error message, they try calling `.iter` on their +tuple, and get a new error: + +``` +error[E0599]: no method named `iter` found for type `({integer}, {integer}, {integer})` in the current scope + --> src/main.rs:2:24 + | +2 | for x in (1, 2, 3).iter() { + | +``` + +At this point they remember a friend telling them they could see all of the +types that implement some trait in the docs. Tuples clearly aren't the type we +need, so let's see if we can find the type we *do* need. The error has told us +that we need to be looking at `Iterator`, so that's where we look in the docs. + +The implementors section there is... less than helpful. Other than the type +`Map` (which our Rust newbie might incorrectly assume is `HashMap`), nothing +here looks helpful. It's mostly just weird types called `Iter` and weird +nonsense like `RSplitN`. At this point there's no obvious path to resolution. + +If we had pointed them at `IntoIterator` like we should have, then the +implementors section... Well it actually wouldn't have been much more helpful, +since it's mostly just spammed with every single possible size of fixed sized +array. However, that's a completely separate problem, and at the very least vec +and slice, the type they most likely needed to see, are at least *somewhere* on +that page. + +If nothing else, *in this particular case*, there was at least a note saying +"required by `std::iter::IntoIterator::into_iter`". However, the tiny footnote +at the bottom is not where most people look, and as we'll see later, is also not +always there or helpful. + +## Ecosystem Examples + +Let's look at another example from outside the standard library. This is a +problem Diesel has run into numerous times. The most common is with our +`AsExpression` trait. Diesel has a trait called `Expression`, which represents a +fragment of SQL with a known type. There is also a trait called `AsExpression`, +which is used to convert -- for example -- a Rust string into a data structure +representing a `TEXT` SQL expression. Unlike `IntoIterator`, where `Item` is an +associated type, in this case `SqlType` is a type parameter. + +This gets represented in the type system to prevent things like accidentally +trying to compare a string with a text column. Problem code might look like +this: `a_table::id.eq(1)`. However, the error message they get is not so +helpful: + +``` +error[E0277]: the trait bound `str: diesel::Expression` is not satisfied + --> src/lib.rs:14:17 + | +14 | a_table::id.eq("1"); + | ^^ the trait `diesel::Expression` is not implemented for `str` + | + = note: required because of the requirements on the impl of `diesel::Expression` for `&str` + = note: required because of the requirements on the impl of `diesel::expression::AsExpression` for `&str` +``` + +Even worse, since the body of `impl AsExpression for +T` implies that the conversion returns `Self`, rust will continue on assuming +that `&str` is a type that appears in the final AST. This results in our less +than helpful message being even further behind 8 different trait impls that +would never be implemented for `&str` in the first place. + +Once again, we do have this little foot note with the information we care about, +but as soon as we introduce one more layer of indirection, that gets completely +lost. For example, if that code were instead written as +`a_table::table.find("1")`, the full output we see is going to be: + +``` +error[E0277]: the trait bound `str: diesel::Expression` is not satisfied + --> src/lib.rs:14:20 + | +14 | a_table::table.find("1"); + | ^^^^ the trait `diesel::Expression` is not implemented for `str` + | + = note: required because of the requirements on the impl of `diesel::Expression` for `&str` + = note: required because of the requirements on the impl of `diesel::Expression` for `diesel::expression::operators::Eq` + = note: required because of the requirements on the impl of `diesel::EqAll<&str>` for `a_table::columns::id` + = note: required because of the requirements on the impl of `diesel::query_dsl::filter_dsl::FindDsl<&str>` for `a_table::table` + +error[E0277]: the trait bound `str: diesel::expression::NonAggregate` is not satisfied + --> src/lib.rs:14:20 + | +14 | a_table::table.find("1"); + | ^^^^ the trait `diesel::expression::NonAggregate` is not implemented for `str` + | + = note: required because of the requirements on the impl of `diesel::expression::NonAggregate` for `&str` + = note: required because of the requirements on the impl of `diesel::expression::NonAggregate` for `diesel::expression::operators::Eq` + = note: required because of the requirements on the impl of `diesel::query_dsl::filter_dsl::FilterDsl>` for `diesel::query_builder::SelectStatement` +``` + +Nowhere in this output is the *actual* missing trait (`AsExpression`) mentioned, +nor is the type parameter we care about (`sql_types::Integer`), which is *the +most important piece of information* ever mentioned. + +The final motivation for this attribute is actually to *help* Rust give +transitive impls when it currently isn't. The only time Rust will recommend +implementing trait `T` in order to get an implementation of trait `U` is if +there is only one such impl which could potentially apply to your type that +would result in that behavior. + +For example, Diesel has to provide a special impl to insert more than one row at +a time on SQLite, which doesn't have the keywords needed to safely do this in a +single query. However, on older versions of Diesel, if there is something +missing that causes that insert statement to not be valid, Rust will just give +up because it doesn't know if you wanted the "normal way to insert a thing" impl +to apply, or the "insert an iterator on SQLite" impl to apply. In the best case +this would result in "`InsertStatement<{30 page type}>: ExecuteDsl` is +not satisfied", which is not helpful, but at least it's not actively misleading. +In the worst case it would result in "`YourRandomStruct: Iterator` is not +satisfied. Perhaps you need to implement it?" which is just complete nonsense. + +With this annotation, Rust would know that it should *never* recommend the impl +related to `Iterators`, and will always give diagnostics as if the "normal way +to insert a thing" impl were the only one that existed. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +Since the diagnostics around this RFC aren't ever mentioned in a guide, I'm not +sure there would be a guide level explanation, but here goes: + +Let's imagine you have the following traits: + +``` +pub trait Foo { +} + +pub trait Bar { +} + +impl Bar for T { +} +``` + +If you tried to call a function that expects `T: Bar` with a type that does not +implement `Bar`, Rust will helpfully notice that if `T` implemented `Foo`, it +would also implement `Bar`. Because of that, it will recommend that you +implement `Foo` instead of `Bar`. + +This is usually the desired behavior, but in some cases it can result in +confusing error messages. Perhaps when a function expects `Bar` and it's not +implemented, it would never make sense to implement `Foo` for that type. In this +case, we can put `#[do_not_recommend]` above our impl, and Rust will *never* +recommend implementing `Foo` as a way to get to `Bar`. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +During trait resolution, Rust will attempt to lower a query like +`IntoIterator(?T)` into a series of subqueries such as `IntoIterator(?T) :- +Iterator(?T)`. If only one such subquery exists, it will be used for error +diagnostics instead. + +With this RFC, for the purposes of diagnostics only, impls annotated with +`#[do_not_recommend]` will be treated as if they did not exist. This means that +cases where there would have been one subquery will be treated as if there were +0, and cases where there were 2 will be treated as if there were 1. + +# Drawbacks +[drawbacks]: #drawbacks + +While this attribute only affects diagnostics, it is inherently tied to how +trait resolution works. This could potentially complicate work happening on the +trait system today (particularly with regards to chalk). + +# Rationale and alternatives +[alternatives]: #alternatives + +- The vast majority of cases where this would be used are for traits and impls + that look very similar to `Iterator` and `impl IntoIterator for + T`. We could potentially instead try to improve the compiler's diagnostics + without this attribute, to detect chose cases. + +# Prior art +[prior-art]: #prior-art + +The author is not aware of any prior art regarding this feature. + +# Unresolved questions +[unresolved]: #unresolved-questions + +n/a From cb8b6c8497c73389067c7aa68115ae765a3a052b Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 7 Apr 2018 16:00:26 -0600 Subject: [PATCH 2/7] ETOOMANYINSTEADS --- text/0000-do-not-recommend.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-do-not-recommend.md b/text/0000-do-not-recommend.md index a9e9bb68eb3..1a99ef5e9ce 100644 --- a/text/0000-do-not-recommend.md +++ b/text/0000-do-not-recommend.md @@ -20,7 +20,7 @@ When a type fails to implement a trait, Rust has the wonderful behavior of looking at possible *other* trait impls which might cause the trait in question to be implemented. This is usually a good thing. For example, when using Diesel, this is why instead of telling you `SelectStatement<{30 page long type}>: -ExecuteDsl is not satisifed`, it instead tells you `posts::id: +ExecuteDsl is not satisifed`, it tells you `posts::id: SelectableExpression is not satisifed`. However, there are times where this behavior actually makes the resulting error From ef5e09018dfbc80ce5864b26a3fc691d835e81a1 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 7 Apr 2018 16:04:31 -0600 Subject: [PATCH 3/7] ETHEISNOTTHESAMEASTHEYTRYPROOFREADINGNEXTTIME --- text/0000-do-not-recommend.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-do-not-recommend.md b/text/0000-do-not-recommend.md index 1a99ef5e9ce..1a16e6e90be 100644 --- a/text/0000-do-not-recommend.md +++ b/text/0000-do-not-recommend.md @@ -41,7 +41,7 @@ for i in (1, 2, 3) { } ``` -The get the following error: +They get the following error: ``` error[E0277]: the trait bound `({integer}, {integer}, {integer}): std::iter::Iterator` is not satisfied From cc9d6ebe17722b2240e863b5679ddb21d06b0557 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 7 Apr 2018 16:10:36 -0600 Subject: [PATCH 4/7] ETHANKSCENTRIL --- text/0000-do-not-recommend.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-do-not-recommend.md b/text/0000-do-not-recommend.md index 1a16e6e90be..f5b9d2bb9c8 100644 --- a/text/0000-do-not-recommend.md +++ b/text/0000-do-not-recommend.md @@ -233,7 +233,7 @@ trait system today (particularly with regards to chalk). - The vast majority of cases where this would be used are for traits and impls that look very similar to `Iterator` and `impl IntoIterator for T`. We could potentially instead try to improve the compiler's diagnostics - without this attribute, to detect chose cases. + without this attribute, to detect those cases. # Prior art [prior-art]: #prior-art From 0eedcaaca49511fdfb2b97286d3a64b0481048b1 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Fri, 27 Apr 2018 14:58:24 -0600 Subject: [PATCH 5/7] Mention that we need to evaluate the name of the attribute --- text/0000-do-not-recommend.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-do-not-recommend.md b/text/0000-do-not-recommend.md index f5b9d2bb9c8..46e04094c08 100644 --- a/text/0000-do-not-recommend.md +++ b/text/0000-do-not-recommend.md @@ -243,4 +243,4 @@ The author is not aware of any prior art regarding this feature. # Unresolved questions [unresolved]: #unresolved-questions -n/a +- What other names could we go with besides `#[do_not_recommend]`? From a82274afd298b2adbd6c8341f259ac5f6434dbd8 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Mon, 2 Jul 2018 19:09:22 +0200 Subject: [PATCH 6/7] RFC 2397 --- text/{0000-do-not-recommend.md => 2397-do-not-recommend.md} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename text/{0000-do-not-recommend.md => 2397-do-not-recommend.md} (98%) diff --git a/text/0000-do-not-recommend.md b/text/2397-do-not-recommend.md similarity index 98% rename from text/0000-do-not-recommend.md rename to text/2397-do-not-recommend.md index 46e04094c08..daf8cdf96c2 100644 --- a/text/0000-do-not-recommend.md +++ b/text/2397-do-not-recommend.md @@ -1,7 +1,7 @@ - Feature Name: `do_not_recommend` - Start Date: 2018-04-07 -- RFC PR: (leave this empty) -- Rust Issue: (leave this empty) +- RFC PR: [rust-lang/rfcs#2397](https://github.com/rust-lang/rfcs/pull/2397) +- Rust Issue: [rust-lang/rust#](https://github.com/rust-lang/rust/issues/) # Summary [summary]: #summary From 326b29a427333ca9cf29a9b69b03dbfaa11df84a Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Mon, 2 Jul 2018 19:10:17 +0200 Subject: [PATCH 7/7] RFC 2397: Update tracking issue --- text/2397-do-not-recommend.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/2397-do-not-recommend.md b/text/2397-do-not-recommend.md index daf8cdf96c2..cb99bd57e92 100644 --- a/text/2397-do-not-recommend.md +++ b/text/2397-do-not-recommend.md @@ -1,7 +1,7 @@ - Feature Name: `do_not_recommend` - Start Date: 2018-04-07 - RFC PR: [rust-lang/rfcs#2397](https://github.com/rust-lang/rfcs/pull/2397) -- Rust Issue: [rust-lang/rust#](https://github.com/rust-lang/rust/issues/) +- Rust Issue: [rust-lang/rust#51992](https://github.com/rust-lang/rust/issues/51992) # Summary [summary]: #summary