11 KiB

Raw Permalink Blame History

Feature Name: do_not_recommend
Start Date: 2018-04-07
RFC PR: rust-lang/rfcs#2397
Rust Issue: rust-lang/rust#51992

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<T: Iterator> 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

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 satisfied, it tells you posts::id: SelectableExpression<users::table> is not satisfied.

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<T: Iterator> 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:

for i in (1, 2, 3) {
    println!("{}", i);
}

They 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<diesel::sql_types::Integer>` for `&str`

Even worse, since the body of impl<T: Expression> AsExpression<T::SqlType> 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<a_table::columns::id, &str>`
   = 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<a_table::columns::id, &str>`
   = note: required because of the requirements on the impl of `diesel::query_dsl::filter_dsl::FilterDsl<diesel::expression::operators::Eq<a_table::columns::id, &str>>` for `diesel::query_builder::SelectStatement<a_table::table>`

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<Sqlite> 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

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<T: Foo> 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

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

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

The vast majority of cases where this would be used are for traits and impls that look very similar to Iterator and impl<T: Iterator> IntoIterator for T. We could potentially instead try to improve the compiler's diagnostics without this attribute, to detect those cases.

Prior art

The author is not aware of any prior art regarding this feature.

Unresolved questions

What other names could we go with besides #[do_not_recommend]?

11 KiB Raw Permalink Blame History