Introduce #[do_not_recommend] to control errors for trait impls

[sgrif]

Rendered

Tracking issue

[Ixrec]

At first glance, #[do_not_recommend] sounded to me like "do not recommend using this impl" which is obviously not what the RFC's talking about. Maybe it's not a big deal, but something like #[do_not_recommend_in_errors] seems like a tolerable length to me considering the target audience for this feature.

[Centril]

🚲🏠 We could call this #[error_no_suggest] which isn't exactly valid English, but is consistent with #[no_mangle] (which is not called do_not_mangle).

[sgrif]

I like both suggestions. I agree the name could use some bikeshedding

[gilescope]

Could we try and draw parallels with how we hide something from rustdoc? I think they use 'hidden' and to me this is conceptually doing a similar thing.

[skade]

Little bikeshed here: the Guide-level explanation has an illustrating code example of the code we don't want, but not of the code we want. That would be:

#[do_not_recommend]
pub trait Foo {
}

pub trait Bar {
}

impl<T: Foo> Bar for T {
}

Is that right?

[jplatte]

I think the #[do_not_recommend] is supposed to go on the impl, not on the trait. AFAICT, it is supposed to convey "don't recommend implementing Foo to get Bar", not "never recommend implementing Foo to get any other impl".

[skade]

Oh, right. And there I am, accidentally showing why it's useful to put the positive usage example in the guide ;).

[aturon]

This is an absolutely beautiful RFC; thank you @sgrif for the care you put into it! I'm very enthusiastic about this avenue for improving our errors, and of course this is a relatively low-risk/commitment feature to add.

The lang team also briefly discussed this RFC in our last meeting and in general the response was positive. So I'm going to go ahead and kick off full review:

@rfcbot fcp merge

[rfcbot]

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Zoxc
• @aturon
• @cramertj
• @eddyb
• @estebank
• @joshtriplett
• @michaelwoerister
• @nagisa
• @nikomatsakis
• @nrc
• @oli-obk
• @petrochenkov
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

I do think it's worth further exploring what the option should be called. (Not trying to bikeshed, just genuinely agreeing with the already expressed sentiment that we want something that makes it clear this attribute controls suggestions in errors, and not, for instance, trait selection or specialization.) I like the idea of #[no_suggest], for instance.

That said, name aside, I agree with the remainder of the RFC. Checking my box...

[scottmcm]

How does this compare to rustc_on_unimplemented? Would the cases that don't want something suggested be happier or sadder to have a custom message?

[sgrif]

This is entirely orthogonal to rustc_on_unimplemented. It can affect whether the trait "suggested" is the one with a custom message or not, which may or may not be desirable depending on the traits and impls involved. That's why the scope of what this affects is very narrow. The case I gave in the body of the RFC around IntoIterator and Iterator is a good example of where the custom error message is actively confusing. However, if the reason some impl were missing was due to Send not being on some type parameter, that's a case where you do likely do want the custom message there.

[estebank]

For a small contrarian opinion, for the presented example

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

we could slightly improve rustc_on_unimplemented to detect tuples (right now it can determine any types, but tuples are special because they do not share a common trait to query for) and instead emit

error[E0277]: can't iterate over tuple `({integer}, {integer}, {integer})`
 --> src/main.rs:2:14
  |
2 |     for x in (1, 2, 3) {
  |              ^^^^^^^^^ `({integer}, {integer}, {integer})` is not an iterator
  |
  = note: tuples are not iterable, XXX instead
  = note: the trait bound `({integer}, {integer}, {integer}): std::iter::Iterator` is not satisfied

That being said, this attribute could still be useful for non-rustc libraries.

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.

I believe we can query for traits that are implemented, checking for IntoIterator and supplying a better message (this is not an iterator, but if you borrow it with & it will be) should be an easy win.

[sgrif]

That would definitely be a nice addition, but has little to do with the issue at hand. Focusing specifically on IntoIterator, I'd like to re-iterate a point I made in the RFC text. The rustc_on_unimplemented message for Iterator is never helpful in a context expecting IntoIterator. The only type that does not implement IntoIterator, but does have a method called iter is a fixed size array, and in that case it's more idiomatic to put an & in front than to call iter. The error message for Iterator is useful in contexts expecting Iterator, but not IntoIterator.

[Nemo157]

A relevant thread just appeared on u.rl.o: https://users.rust-lang.org/t/experimenting-with-generic-tryfrom-troubles-occur/17113, this is a different context to any of the current examples, specifically having the compiler suggest different trait bounds for a generic parameter. I assume this error could also be adjusted based on the proposed attribute?

[sgrif]

Yes, this would allow that to be fixed if the attribute were placed on the impl<T: From<U>, U> TryFrom<U> for T. Whether that is a case we want to apply this is a separate discussion.

[Centril]

@sgrif Can you note the naming discussion as part of the unresolved questions? Then we can resolve it before stabilizing (or before merging the RFC if we like..).

[oli-obk]

@rfcbot reviewed

[petrochenkov]

As long as @nikomatsakis thinks this is ok from implementation point of view, I'm ok with this too.

[nagisa]

This feels like a very directed solution at a problem that’s not entirely understood. Not necessarily a bad thing, but I feel that if we thought more intensely about this, we might be able to come up with a solution that’s more general.

For example, one of the changes that would probably make this attribute more general is ability to place it on bounds rather than the whole impl (similarly to how we ended up implementing the #[may_dangle] attribute). In that case the library author could select which bounds they want to ignore for the purposes of the diagnostics and keep the current behaviour some other bounds, that the they expect the library user to satisfy.

Is that useful? I don’t know. It is just a first generalization that came to mind.

(/me is unsure whether this is something worth registering a fcp concern over or not)

[sgrif]

@nagisa If anything that sounds more specific not more general. I do think that there's a pattern here in that most examples that this would apply to are Foo/IntoFoo pairs, but I don't think codifying that into the compiler is any better. Ultimately I opted for a very specific solution because I'm not sure the compiler can ever truly figure out "how deep" the author intends it to go, without getting it wrong some of the time.

Having it on the bounds doesn't really help at all in this case, since every single place that someone writes T: IntoIterator<Item = Foo> they'd need to annotate it or get the confusing error.

[cramertj]

Ping @nagisa @estebank @Zoxc FCP here is blocked on approval from one of y'all.

[estebank]

@cramertj for some reason I cannot check the checkbox (nor edit posts in the rfcs repo). Can you go ahead and mark it for me?

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[cramertj]

@estebank Done! For future reference, you can use "@rfcbot reviewed" to check boxes in repos where you don't have permissions.

[Zoxc]

@rfcbot reviewed

[nagisa]

Having it on the bounds doesn't really help at all in this case, since every single place that someone writes T: IntoIterator<Item = Foo> they'd need to annotate it or get the confusing error.

Feels like I was misunderstood, but I haven’t energy to elaborate.

@rfcbot reviewed

[Aehmlo]

I’m sorry if this was mentioned above and I missed it, but what about a name like recommend(never)? This brings up the question of what other values are valid, and maybe we could discuss the validity of #[recommend($priority)] or something. Just a thought; I think the underlying idea here is a great one.

[moxian]

This RFC title randomly caught my eye in the issue list, and I would like to voice my agreement with @Ixrec and @Centril above - the name is confusing and reads very much as "do not recommend using the method". (at least that was my initial impression upon seeing the RFC title).

Can this still be renamed/bikeshedded, or am I too late?

From my side I can suggest #[do_not_autosuggest]. Both #[error_no_suggest] and #[do_not_recommend_in_errors] mentioned above also sound fine to me - they are at the very least cryptic enough to not assume the "do not use" intention at first glance.

[Centril]

@moxian

The RFC explicitly has an unresolved question:

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

This means that as part of stabilizing the feature, there will be a bikeshed which you are welcome to join. :)

[moxian]

So, just to be clear, the name bikeshedding will happen not here, but in a separate "tracking issue" in rust-lang/rust repository, that would be created after this RFC is merged - is that correct?

[Centril]

@moxian Pretty much yeah.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#51992