Repository navigation
NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663
Description
We have made a lot of progress on NLL diagnostics
So much so that it behooves us to take a step back and reevaluate where we stand with respect to overall diagnostic quality.
As part of that, we should do another round of review of the differences between AST borrowck diagnostic output and NLL borrowck diagnostic output.
- see NLL: must review all .nll.stderr files #49862 for the previous round of review here
Update: I finished the review, and have put the cases where NLL is not up to par into "buckets." Each bucket has a linked comment on this issue (and, in some cases, a separate issue that is linked from that comment and from the description here).
Here are all the buckets (with links to each specific comment):
- When a bucket is no longer relevant (either because the issue has been fixed or we've decided its not actually an issue), it will be struckout (
like so).
Reacted by Rémy Rakic, Santiago Pastorino, Ashkan Kiani, matthewjasper, Esteban Kuber and David Wood
Activity
pnkfelix commentedon Jul 24, 2018
see #49862 for the previous round of review here
pnkfelix commentedon Jul 25, 2018
In the details, here's a list of all 226 of the current
.nll.stderrfiles, along with a checkmark if I looked at it and a summary of my finding for it. After I go through the whole list I'll make buckets to put them into.Details
that are not immediately visible when one just sees
x += x.we may be committing to no longer having the same level of
box insensitivity in a number of the cases listed here.
movekeyword on closures #51169movekeyword on closures #51169matchergnomics).confusing and strictly worse than AST-borrowck. (A StorageEnd for
a temp holding a
&-borrow is not a use.)so at least this might be fixable by adjusting some rustc
control-flow?
(Should be easy to add this to NLL.)
dictalternative test expression
(perhaps due to test weakness?)
&muthere"&'a mut String&'a mut String&'z mut selfof its suggestion&mut Zinstead of&'a mut Selfin its suggestion (original was&'a Self)&mut [1, 2, 3, 4]"is actually valid; I guess that does not get promoted to static storage.)
Tmay not live long enough"(is it masked by the earlier reported error under NLL?)
xthat is causing closure to borrow itxthat is causing closure to borrow it+ <lifetime>to theimpl Iteratorreturn type(which might be a big point of this ui test?)
'a(which is a moreprecise bound on the validity of
x...)'xbut does not have any span that includes that lifetime.conterror which adds'rto&T.region error due to nll" would add value?)
match foo { ... }; would be better tojust highlight input to
matchin common case.id()up to aletbinding to extend its lifetime.'awould probably be a good idea.FOOis dropped, but that terminologydoesn't make sense for statics
maincannot have generic parameters.(But maybe should be revised in test to not do that.)
id()up to aletbinding to extend its lifetime.id()up to aletbinding to extend its lifetime.moveto force closure to take ownership ofv'aand'bbut does not have any span that includes those lifetimes.'arequired." message'arequired." message'arequired." message'arequired." message'arequired." messagemoveto force closure to take ownershipmoveto force closure to take ownership&mut Own<Point>&mut Own<Point>fwithin the closurethat causes the erroneous move.
young(perhaps test weakness?)letbinding to extend temps' lifetimes.NLL might be leaving out one of the errors. Or AST-borrowck might be redundantly reporting one.
NLL might be leaving out one of the errors. Or AST-borrowck might be redundantly reporting one.
(not sure if there's a "test weakness" to be removed in this case)
(not sure if there's a "test weakness" to be removed in this case)
subsequent one is revealed. So NLL remains sound; it just is a
question of whether we always expect both errors to be reported
when they occur in the same function body.
See NLL lets borrowck observe drop order for
let (a, b);#51036See NLL lets borrowck observe drop order for
let (a, b);#51036See NLL lets borrowck observe drop order for
let (a, b);#51036&mut std::string::Stringprovided a more specific message about it being captured by a
trait object (I presume its that object's destructor that is
source of the use referenced here).
here is due to a later loop iteration.
AST-borrowck output is also not 100% sorted w.r.t. span here)
&mut &mut i32pnkfelix commentedon Jul 27, 2018
(I've almost finished the initial review; there are just 32 more diffs for me to look at. But I think it will have to wait until Monday.)
pnkfelix commentedon Jul 30, 2018
I have finished my initial scan of all 226 files. (See details link in above comment.)
I have tagged 152 of the cases as "on par" with AST-borrowck. That leaves 74 cases that are not up to par.
Next, I need to take all of the 74 cases where NLL is not up to par with AST-borrowck, and organize those cases into buckets, so that we get a better idea of how many root issues remain to be resolved.
pnkfelix commentedon Jul 30, 2018
I'm going to try making a separate comment for each bucket that I identified; that way each bucket will have a specific URL that one can link within this issue. I'm going to skip the cases where NLL is clearer at (or above) par when compared to AST-borrowck.
pnkfelix commentedon Jul 30, 2018
On Par, Assuming Faith In RFC 2005
There are two cases where NLL is on par, as long as you are a believer in the new
matchergonomics. I.e., the error produced by NLL and the suggestion it makes relies heavily on rust-lang/rfcs#2005 . In several cases we have stopped suggesting to the user that they inject areforref mut, and instead we suggest that they stop dereferencing thematchinput itself (and then presumably the match ergonomics will often handle implicitly turning the pattern identifiers into references).These are the cases:
rust/src/test/ui/borrowck/borrowck-move-error-with-note.nll.stderr
Lines 1 to 8 in fefe816
rust/src/test/ui/issue-40402-ref-hints/issue-40402-2.nll.stderr
Lines 1 to 10 in fefe816
pnkfelix commentedon Jul 30, 2018
pnkfelix commentedon Jul 30, 2018
Better Diagnostic for Trait Object Capture
In https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/span/regions-close-over-type-parameter-2.nll.stderr NLL is on par but it would be nice if instead of "borrow later used here" we provided a more specific message about it being captured by a trait object (I presume its that object's destructor that is source of the use referenced here).
Has PR: #54848
pnkfelix commentedon Jul 30, 2018
pnkfelix commentedon Jul 30, 2018
Missing
movesuggestion: #51169These tests all would benefit from a fix for #51169
pnkfelix commentedon Jul 30, 2018
51 remaining items
Rollup merge of rust-lang#54831 - davidtwco:issue-52663-struct-field-…
Rollup merge of rust-lang#54848 - davidtwco:issue-52663-trait-object,…
Auto merge of #54831 - davidtwco:issue-52663-struct-field-suggestion,…
Auto merge of #54848 - davidtwco:issue-52663-trait-object, r=nikomats…
Rollup merge of rust-lang#54825 - davidtwco:issue-52663-deref-raw-poi…
Rollup merge of rust-lang#54825 - davidtwco:issue-52663-deref-raw-poi…
pnkfelix commentedon Oct 24, 2018
closing as I am starting on #54528 now.
Removed overlapping_spans.{rs,stderr,nll.stderr}.