-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better lifetime error message #56479
Conversation
src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs
Outdated
Show resolved
Hide resolved
Hmm... Is this necessarily about proving that references are valid, or whether the lifetimes are the correct ones? The new error message seems less accurate in some cases that what we have right now. |
An unsatisfied lifetime constraint usually (always?) indicates that there may be a reference somewhere that may cause memory unsafety (e.g. by living too long). Or am I missing something? |
@mark-i-m sure; but that reference may be baked into some struct which is then baked into another struct... such that the lifetime that caused the error is actually used quite deeply in the type? In other words it's far removed? |
Hmm... I can see your point, but I'm not sure how to fix it. Should we make the message more vague (e.g. "unable to prove memory safety")? |
@mark-i-m My point is that the current diagnostic talks about lifetimes whereas the proposed one talks about references; talking about lifetimes seems more direct here; could we reword it to retain lifetimes somehow but still make it more friendly? For example:
This one is probably not great, but do you see what I mean? |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
How about some of the following?
|
Of the top of my head I like:
(assuming there is a function header...? ^.^) but... I have sucked up too much air; I'll defer to @estebank's judgement on diagnostics :) |
Although I'm partial to "bro, you're lifetimes are all wonky", I'd prefer "lifetime may not live long enough" as it's closer to the wording elsewhere for other cases. My ideal solution would be to rework the message depending on which of the possible situations we're facing. For example, I'd like it to be something along the lines of "borrow being returned doesn't live long enough". In context:
|
So we do that currently, but this is the fallback message if we can do one of the more specialized messages. |
In general, it would be helpful to me if you would run |
@Centril After thinking about it, "implementation does not satisfy lifetime restrictions from function header" seems perhaps a bit context-dependent. It might be possible to change some errors to do that, but I'm not sure all unsatisfied constraints come from a function header. So for now, I will stick with @estebank's suggestion. @nikomatsakis Ah, that's a good idea. I will run |
@nikomatsakis Pushed. |
We need to keep iterating over these error messages, but I'm happy with this PR as is as I find the new wording less jargon-ny. @nikomatsakis what do you think? |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I don't understand the CI failure. Everything is passing locally. Don't I just need to rebase or something? |
The weird thing is that the corresponding .rs file doesn’t have any //ERROR comments. |
Or maybe I just missed it or did a weird rebase or something |
This comment has been minimized.
This comment has been minimized.
I'm torn. I agree the new message has less jargon. On the other hand, I'm not really sure what a lifetime "living long enough" should mean, and I'm interested in seeing what we can do to move away from this "lifetime" and "live long enough" sort of terminology, which I think is generally sort of confusing. Still, I don't really have any superior suggestions. =) |
Perhaps we can talk about "Object
|
In my mind, this means their is a reference somewhere that we can't prove is valid everywhere it is used, which is what I put in the original change in this PR. Something about the intuition of "X might not be valid everywhere it is used" seems like the intuition we want to convey IMHO. |
☔ The latest upstream changes (presumably #57108) made this pull request unmergeable. Please resolve the merge conflicts. |
Better lifetime error message I propose the following error message as more user-friendly r? @nikomatsakis
Better lifetime error message I propose the following error message as more user-friendly r? @nikomatsakis
Failed in #57688 (comment) when combined with other things (not sure what...). I think you need to @bors r- |
I have to go now, and I haven't had a chance to run the full suite with --bless --compare-mode=nll, but the ui tests are updated and the rest are running now. |
@bors r+ |
📌 Commit db2d243 has been approved by |
⌛ Testing commit db2d243 with merge 3035858778ecc1b510052483fe6b955927abd0fb... |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ugh... seems we got a segfault (I would be surprised if this PR was the cause, but still worrying...)? [01:46:20] test vec::test_swap_remove_empty ... ok
[01:46:21] died due to signal 11
[01:46:21] test failed, to rerun pass '--test collectionstests' @bors retry |
Better lifetime error message I propose the following error message as more user-friendly r? @nikomatsakis
☀️ Test successful - checks-travis, status-appveyor |
I propose the following error message as more user-friendly
r? @nikomatsakis