-
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
NLL: bad error message when converting anonymous lifetime to 'static
#51536
NLL: bad error message when converting anonymous lifetime to 'static
#51536
Conversation
'static
'static
This PR isn't ready yet (at time of writing and creating the WIP PR), but it completes the majority of the initial steps for this. A function has been added that computes all the constraint paths from a region - the shortest of these paths is then categorized, sorted and labelled onto an error. Improvements are required with the categorization, I was unsure what categories were required and how best to work them out. Discussion related to this PR can be found on Zulip, in |
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 |
Ping from triage @nikomatsakis! This PR needs your review. |
☔ The latest upstream changes (presumably #51460) made this pull request unmergeable. Please resolve the merge conflicts. |
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 looking great so far! I guess the question we have to answer now is to focus in on some specific tests and try to push the output to something we are happy with.
let mut end_regions: Vec<RegionVid> = Vec::new(); | ||
|
||
// When we've still got points to visit... | ||
while !next.is_empty() { |
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.
Nit: while let Some(current) = next.pop() {
is nicer :)
return Ordering::Equal; | ||
} | ||
|
||
match (self, other) { |
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.
Can we not just #[derive(Ord)]
here? I think that would work fine...?
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.
(We would want to comment that the order is significant)
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 considered doing that, but it would have meant that variants at the top of the definition were considered most significant - which seemed a little too implicit? - so I went with this instead. I'll change it to #[derive(Ord)]
.
@@ -1096,6 +1136,32 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |||
}).is_some() | |||
} | |||
|
|||
/// This function classifies a constraint from a location. | |||
fn classify_constraint(&self, location: Location, mir: &Mir<'tcx>) -> ConstraintCategory { |
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.
as an aside, it feels pretty clear we should think about breaking this logic out into its own module at some point. Doesn't have to be in this PR.
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'll make an error_reporting
module and move relevant functions into it.
if categorized_path.len() > 0 { | ||
let blame_constraint = &categorized_path[0]; | ||
|
||
let mut diag = infcx.tcx.sess.struct_span_err( |
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.
can I see the output of this code for any of our tests?
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.
For the test we focused on in the pair programming session, this is the current output - it just labels spans with the assigned categories as you had suggested as a first step:
error: Cast
--> src/test/ui/underscore-lifetime/dyn-trait-underscore.rs:18:5
|
16 | fn a<T>(items: &[T]) -> Box<dyn Iterator<Item=&T>> {
| ____________________________________________________-
17 | | // ^^^^^^^^^^^^^^^^^^^^^ bound *here* defaults to `'static`
18 | | Box::new(items.iter()) //~ ERROR cannot infer an appropriate lifetime
| | ^^^^^^^^^^^^^^^^^^^^^^
| | |
| | Cast
| | Cast
19 | | }
| |_- Cast
I don't know if it's great, because we end up with a lot of Cast
. Though, not all tests are like that.
c34548b
to
c28eebd
Compare
Rebased and addressed the above comments. |
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 |
Ping from triage @davidtwco! There are test failures you should fix. |
'static
'static
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.
looks good but for the nit that error messages in compiler should begin with lowercase letter
@bors r+ |
📌 Commit 06b35ef87cadb18366d119496239bb1a75ac7a4f has been approved by |
☔ The latest upstream changes (presumably #51139) made this pull request unmergeable. Please resolve the merge conflicts. |
@davidtwco needs rebase :( |
06b35ef
to
ac8aef2
Compare
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 |
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 |
☔ The latest upstream changes (presumably #51855) made this pull request unmergeable. Please resolve the merge conflicts. |
We are not currently using it for anything; even polonius just uses the `from_location`.
… Categorization needs a bit of work.
37b0cc6
to
c0c4741
Compare
@bors r+ |
📌 Commit c0c4741 has been approved by |
…vements, r=nikomatsakis NLL: bad error message when converting anonymous lifetime to `'static` Contributes to #46983. This PR doesn't introduce fantastic errors, but it should hopefully lay some groundwork for diagnostic improvements. r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
refactor and cleanup region errors for NLL This is a WIP commit. It simplifies some of the code from #51536 and extends a few more steps towards the errors that @davidtwco and I were shooting for. These are intended as a replacement for the general "unable to infer lifetime" messages -- one that is actually actionable. We're certainly not there yet, but the overall shape hopefully gets a bit clearer. I'm thinking about trying to open up an internals thread to sketch out the overall plan and perhaps discuss how to get the wording right, which special cases to handle, etc. r? @estebank cc @davidtwco
Contributes to #46983. This PR doesn't introduce fantastic errors, but it should hopefully lay some groundwork for diagnostic improvements.
r? @nikomatsakis