-
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
Disable some nice region errors in NLL mode. #53115
Conversation
This disables all but the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly a request to open up some follow-up issues
@@ -65,10 +65,16 @@ impl<'cx, 'gcx, 'tcx> NiceRegionError<'cx, 'gcx, 'tcx> { | |||
} | |||
|
|||
pub fn try_report(&self) -> Option<ErrorReported> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a try_report_from_nll
method instead, and invoke that from the NLL borrow checker, vs checking the flag. Just seems a bit cleaner, but it may also improve the error messages in some cases below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| -- -- ^ function was supposed to return data with lifetime `'b` but it is returning data with lifetime `'a` | ||
| | | | ||
| | lifetime `'b` defined here | ||
| lifetime `'a` defined here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems strictly better
LL | | ||
LL | if x > y { x } else { y } //~ ERROR lifetime mismatch | ||
| ^^^^^ ...but data from `x` is returned here | ||
| ^^^^^ requires that `'1` must outlive `'a` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting that we highlight the comparison here -- seems like a problem, though obviously not from this change...maybe we should file a bug? Presumably the actual span we should highlight is the use of x
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mentioned on the .nll.stderr
/.stderr
comparison here.
LL | | ||
LL | x //~ ERROR lifetime mismatch | ||
| ^ ...but data from `x` is returned here | ||
| ^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should file a bug here -- I think it'd be nice to add another note here and use this as an opportunity to teach the elision rules. e.g.,
error: unsatisfied lifetime constraints
--> $DIR/ex1-return-one-existing-name-return-type-is-anon.rs:18:5
|
LL | fn foo<'a>(&self, x: &'a i32) -> &i32 {
| -- - - defaults to `&'1 i32` per the elision rules
| | |
| | let's call the lifetime of this reference `'1`
| lifetime `'a` defined here
LL |
LL | x //~ ERROR lifetime mismatch
| ^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here.
| ^ ...but data from `value` flows into `data` here | ||
| -- -- lifetime `'b` defined here | ||
| | | ||
| lifetime `'a` defined here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should file a bug here too -- in general, it's often dissatisfying to highlight the declaration. To do better, though, we'd presumably have to look at the MIR and see if we can identify the parameter. (You could imagine checking if the lifetime only appears in one parameter's type and using that to guess that we should highlight the parameter type, also)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue-52663-lifetimes-not-included-in-span in Zulip and this comment discussed this briefly as a potential part of #52973.
@bors r+ p=1 Giving p=1 because NLL |
📌 Commit 785b06c36804529092d304426f785b5e31cdcee7 has been approved by |
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 |
Oops! Fixed those |
@bors r+ |
📌 Commit 37ba9ca has been approved by |
Disable some nice region errors in NLL mode. Fixes #52739. cc #52742. r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
Fixes #52739. cc #52742.
r? @nikomatsakis