rustc: Remove all "consider using an explicit lifetime parameter" suggestions

[brson]

These give so many incorrect suggestions that having them is
detrimental to the user experience. The compiler should not be
suggesting changes to the code that are wrong - it is infuriating: not
only is the compiler telling you that you don't understand borrowing,
the compiler itself appears to not understand borrowing. It does not
inspire confidence.

r? @nikomatsakis

[bstrie]

cc @jonathandturner

[withoutboats]

I feel like I've found these helpful many more times than they've been wrong, but the compiler giving wrong advice is indeed a frustrating experience that could cause users to believe that the borrow rules are 'not worth it,' 'not good enough,' 'not correct,' or some other negative thing.

Perhaps in the long term when the compiler could rerun the borrowck pass with the explicit annotations, recurring as necessary (and accumulating all of them to make a single suggestion), and only make the suggestion if the resulting code passes?

To provide a counterargument even to that change (though I think the negative experiences overwhelm this), in complicated (but safe) lifetime wrangling code, I have followed the compiler's suggestions to find the lifetime error I actually needed to solve.

[sophiajt]

@withoutboats - do you think it can be fixed rather than being removed?

That is, is there a heuristic for generally when the notes are helpful? Being wrong to me just sounds like a net loss, but if there are times we can detect that they'll be useful and only display them then, that seems like a better improvement than pulling it out altogether

[nikomatsakis]

I think I agree with removing this code. It's bitrotted and dates from a time when the language was quite different (e.g., no 'a: 'b where clauses). The main reason I have not done so, though, is a concern about throwing the baby out with the bathwater.

I know that I have definitely found these suggestions helpful on many occasions. One particularly useful thing they can do is to highlight where there are elided lifetime parameters that you did not know about that are causing you grief.

[nikomatsakis]

That is, is there a heuristic for generally when the notes are helpful? Being wrong to me just sounds like a net loss, but if there are times we can detect that they'll be useful and only display them then, that seems like a better improvement than pulling it out altogether

I gave a sort of "high-level" idea of where I have found them useful, and I know that anecdotally people have told me that they like them, but I think they also mislead a lot. It's a bad sign that e.g. when I see one I usually just ignore it, or I find it's quite common that it's either showing me the same signature I have or making some other non-actionable suggestion. I wish I had more specific examples at hand but I don't. Perhaps if we turn them off we will start to accumulate some, though. =)

That said, I do suspect we could identify the cases where it might be wrong and avoid them. But also the code is (as I wrote before) sort of old and done in a weird way, and I'd rather have a cleaner start. Though I'm not sure what's better. Right now it crawls the HIR and makes modifications and then pretty-prints it, as I recall. We might be able to just do some thing where it finds the parameters whose types can be changed and does a span_label() call with a suggestion and it would be nicer.

[nikomatsakis]

I'd like a bit wider feedback here. cc @rust-lang/docs @rust-lang/compiler

[nrc]

Sometimes these give good suggestions, but it seems to be at 50% or less for me. I would rather they were fixed rather than thrown out, but I guess throwing out seems like an improvement over the status quo.

[GuillaumeGomez]

I'm in favor for a deletion for now.

[arielb1]

I can sort of read it to get the bounds I want. Maybe we should just dump some of these bounds on the user directly? It does tell me when I get a type parameter missing in a method signature and inferred to something random.

[bors]

☔ The latest upstream changes (presumably #37269) made this pull request unmergeable. Please resolve the merge conflicts.

[nagisa]

I’m in of a similar mind as @withoutboats: I’ve had a multiple cases where suggestion from this class helped me out to figure out the problem at hand. While the solution in most cases ended up not being exactly what suggestion was telling me to use, it often helped me to understand what the compiler thought under the hood and figure out how my code was wrong easier.

[nikomatsakis]

I feel like what all of us experienced users are saying is: the suggestions per se are not helpful, but they expose helpful information. Seems like we should replace this with something more targeted that does expose that information. I'd feel much better about removing if we had someone dedicated to doing that work --- and I'd love to work with them, "I have thoughts" -- but I am not sure what to do in the interim. I definitely dislike giving misleading or wrong suggestions.

[nikomatsakis]

As a starting point, more examples where the messages were helpful would be good.

[eddyb]

@nikomatsakis Lowest cost change (that can be backported to beta) would be adding "This is what a working signature might look like (please report incorrect suggestions at https://github.com/rust-lang/lang/issues):".

[eddyb]

@nikomatsakis @jonathandturner See #37363 for an example of a case with an easy fix.

[nikomatsakis]

Had some brief discussion with @brson on IRC. I think both of us are still inclined to remove, unless someone wants to take up the active task of improving. That said, I suspect there are some simple heuristics that would guide us to better suggestions. It would help if we had a corpus of actual errors people encounter in practice.

My biggest hesitation here results from the fact that the base errors do tend to be the worst we give. Another place where a corpus would be ultra-helpful! Most of my attempts to think about improvements get stuck around here, trying to gather up data.

[steveklabnik]

My thoughts:

1. False positives here are really bad
2. I'm not sure how often this is right vs wrong for me, but when it's wrong, it's very wrong.
3. Misleading new people in this way is brutal
4. as such, I'd lean towards removal. Not mis-leading people is worth more than the convenience of the compiler suggesting what's right occasionally.
5. I would also like to see a version of this feature that works, possibly in the way @withoutboats mentions. But until that comes along, better to remove.

[sophiajt]

+1 on "remove and file an issue to follow-up with better design"

[peschkaj]

👍 to what Jonathan said

On Thu, Nov 3, 2016, 07:41 Jonathan Turner notifications@github.com wrote:

+1 on "remove and file an issue to follow-up with better design"

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#37057 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEXksPNWVI3nhd-mTIdX1qLkYhbEkeAks5q6fK1gaJpZM4KSDxt
.

Jeremiah Peschka

[withoutboats]

I agree with @steveklabnik. I think this feature as is should be removed; I just think suggesting explicit annotations is a good idea to pursue and we shouldn't take the position that we tried this and it doesn't work.

(What I've found rather frustrating recently is that it will suggest adding lifetime annotations to impls of foreign traits. Come on compiler, you know I can't do that.)

[eddyb]

What I've found rather frustrating recently is that it will suggest adding lifetime annotations to impls of foreign traits.

#37481 should fix that. Also, rewording the message to make the compiler seem less confident in its suggestions would replace user pain with bug reports along the lines of "compiler suggestion didn't work".

[alexcrichton]

Sounds like this is leaning towards removal, @brson want to rebase and @nikomatsakis want to r+?

[nikomatsakis]

@alexcrichton yeah, I've been vacillating on how to decide how to reach a decision here. =) But it does seem like the majority of people are in favor of removal, so I'm still inclined to go that way.

[eddyb]

I'm worried about the potential fallaciousness of "a redesign from scratch won't have these problems".
In general, I'd prefer we word suggestions as "they might work, if not consider filing an issue".
That way the compiler is not overconfident all the time, the users get to try something and we learn.

[arielb1]

@nikomatsakis

I am generally able to read the desired bounds out of the suggestions. I think that just dumping the transitive reduction of the requested lifetime bounds would be OK.

[nikomatsakis]

So ok I am quite frustrated that this PR is still open. We need to make a decision. Let me summarize as best I can the arguments on either side:

In favor of removal:

• Suggestion code is old and hard to maintain and it makes false assumptions (i.e., it was written before 'a: 'b was an option, so it will never suggest that)
• Suggestions are frequently wrong, and that outweights their value. E.g., @steveklabnik writes "as such, I'd lean towards removal. Not mis-leading people is worth more than the convenience of the compiler suggesting what's right occasionally."

In favor of keeping:

• Many people feel it is right more than it is wrong
• Even when it is wrong, it can help you to understand what the compiler "is thinking under the hood" (from [@nagisa's comment](While the solution in most cases ended up not being exactly what suggestion was telling me to use, it often helped me to understand what the compiler thought under the hood and figure out how my code was wrong easier.))
  • e.g., I have found it helpful for identifying elided lifetimes I was overlooking
• The general fear that we will not replace it in a timely fashion, or that the replacement will have similar flaws

Some concerns:

• We don't have a strong "corpus" of realistic errors where this helps and realistic ones where it hurts
  • though there are a series of tests for the feature, which is a start; but they are out of date and only concern the "works" cases

Possible courses of action:

• Reword to indicate that the suggestion may be wrong and ask users to file bugs, this could help to build a corpus
  • perhaps something like "= note: suggestions are in beta, please leave comments at [this URL]", though that would make more sense if it were placed on a new, improved system
• Revoke and file a bug for follow-up action (roughly embodied by this PR):
  • perhaps trying to summarize the constraints, per @arielb1's suggestion
  • or perhaps taking the current approach, but being more limited, for example trying to more narrowly target the cases where we know it works (no named lifetimes around, etc)
  • this seems to be the majority of opinion based on existing comments, but certainly not universal

My ideal course of action here would probably be to back out the existing code, but identify someone who is motivated to work on improving it (I'd be happy to mentor). This overlaps with the exiting complaints about lifetime inference errors.

Seem like a reasonable summary?

[brson]

Tests pass locally.

[nikomatsakis]

@bors r+

[bors]

📌 Commit a2735c0 has been approved by nikomatsakis

[nikomatsakis]

OK, this means I have to get serious about planning out improved region errors.

People who contributed to this discussion: if you can please add candidate errors to this etherpad, I would appreciate it. I have been trying to get a good categorization of different classes of errors that occur.

[alexcrichton]

Testing something, but I plan to reopen

[alexcrichton]

Yup, originally the PR contained the character "…" which apparently caused homu to choke. I've now updated the PR description to have three literal dots.

wtf homu

[alexcrichton]

@bors: r=nikomatsakis

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: Make backtraces work on Windows GNU targets again. #39234

[bors]

📌 Commit a2735c0 has been approved by nikomatsakis

[bors]

⌛ Testing commit a2735c0 with merge 154c202...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 154c202 to master...

[shepmaster]

A recent Stack Overflow question was improved when we added the "consider using an explicit lifetime parameter as shown" error. I thought that the OP wasn't reading the error messages, but perhaps they were simply using beta / nightly (although the timeline gets complicated).

I'm pretty sure this hint is an example of @nikomatsakis comment about "locally right, globally wrong", but making the suggested change in OP's original code helped shift the error message to something else that might be more immediately resolvable.

I've removed external dependencies from that example and added it to the etherpad. I'm sad to see this hint going away, and I look forward to the replacement!

[sophiajt]

@shepmaster - the plan is to replace it with something better. If you have ideas for improvement, I'd love to hear them (since it probably falls to me to help design the replacement)

[nikomatsakis]

@shepmaster thanks for adding that; I definitely want to get started on some replacements right away.