NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

pnkfelix · 2018-07-24T14:03:26Z

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~~).

Bucket	Rust Issue	Pull Request	Interested Hackers
On Par, Assuming Faith In RFC 2005
~~Sort By Span~~	#51167	#52904	@pnkfelix
Better Diagnostic for Trait Object Capture		#54848	@davidtwco
~~Better Diagnostic When "Later" means "A Future Loop Iteration"~~		#52948	@davidtwco
Missing move suggestion	#51169	#55069	@matthewjasper
~~NLL Spans Show Hypothetical Code~~	#52877	#52907	@pnkfelix
NLL diagnostic for += is too desugared
~~NLL diagnostic is globally bad ... but locally justifiable?~~	#51191	#54720	@davidtwco
~~The "Later used here" diagnostic is not ideal for return values~~		#53177	@matthewjasper
~~Provide span for declaration of captured variables~~		#53164	@davidtwco
Test Weakness (might mean NLL is not being tested enough)			@pnkfelix
~~NLL is missing struct field suggestion~~		#54831	@davidtwco
NLL Diagnostic Redundancy
~~missing lifetime in suggestion~~	#52880	#52883	@estebank
NLL chooses not great span in its "lifetime required" message
~~NLL fails to suggest + bound on trait object~~		#53900 + #54853	@davidtwco
NLL fails to report lifetime mismatch error
~~"does not live long enough" when talking about a (thread-local) static~~		#53131	@davidtwco
~~NLL missing suggestion to extend lifetimes via let bindings~~	#49821	#54088	@matthewjasper
"borrowed content" instead of "dereference of raw pointer"		#54825	@davidtwco
NLL lacks various special case handling of closures		#53088 + #54976	@matthewjasper + @davidtwco
~~NLL didn't fix interaction of local dying with temps in dropck~~	#21114, #22323		@pnkfelix
~~NLL mentions lifetimes that are not included in printed span(s).~~		#52973	@davidtwco
NLL rejects `FnMut` closure where AST-borrowck accepted it	#53040	#54802	@davidtwco
Note and teach elision rules
Need Further Review

The text was updated successfully, but these errors were encountered:

pnkfelix · 2018-07-24T19:44:29Z

see #49862 for the previous round of review here

pnkfelix · 2018-07-25T13:00:51Z

In the details, here's a list of all 226 of the current .nll.stderr files, 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.

src/test/ui/asm-out-assign-imm.nll.stderr
- NLL is stricly better
src/test/ui/augmented-assignments.nll.stderr
- not ideal; the MIR error is a little confusing because its describing micro-ops
  that are not immediately visible when one just sees x += x.
src/test/ui/borrowck/borrowck-box-insensitivity.nll.stderr
- This might represent an actual "regression" in NLL? Or at least
  we may be committing to no longer having the same level of
  box insensitivity in a number of the cases listed here.
- See also MIR-borrowck: behavior around Box<T> is different from AST borrowck #45696
src/test/ui/borrowck/borrowck-escaping-closure-error-1.nll.stderr
- Would benefit from NLL: missing suggestion to use move keyword on closures #51169
src/test/ui/borrowck/borrowck-escaping-closure-error-2.nll.stderr
- Would benefit from NLL: missing suggestion to use move keyword on closures #51169
src/test/ui/borrowck/borrowck-in-static.nll.stderr
- NLL is on par
src/test/ui/borrowck/borrowck-move-error-with-note.nll.stderr
- NLL is "better" here (at least if you are believer in the new match ergnomics).
src/test/ui/borrowck/borrowck-move-out-of-vec-tail.nll.stderr
- NLL is fine as long as you are okay with highlighting hypothetized code.
src/test/ui/borrowck/borrowck-report-with-custom-diagnostic.nll.stderr
- NLL is on par
src/test/ui/borrowck/borrowck-vec-pattern-nesting.nll.stderr
- NLL is fine as long as you are okay with highlighting hypothetized code.
src/test/ui/borrowck/issue-45983.nll.stderr
- NLL is on par
src/test/ui/borrowck/issue-51117.nll.stderr
- NLL is on par
src/test/ui/borrowck/issue-51415.nll.stderr
- NLL is on par
src/test/ui/borrowck/issue-7573.nll.stderr
- NLL is on par
src/test/ui/borrowck/mut-borrow-in-loop.nll.stderr
- NLL is on par
src/test/ui/borrowck/mut-borrow-of-mut-ref.nll.stderr
- NLL is giving a globally misleading hint, but it seems locally acceptable.
src/test/ui/borrowck/mut-borrow-outside-loop.nll.stderr
- NLL is on par
src/test/ui/borrowck/mutability-errors.nll.stderr
- NLL is on par
src/test/ui/borrowck/promote-ref-mut-in-let-issue-46557.nll.stderr
- the "later used here" messaging for the short-lived temp is
  confusing and strictly worse than AST-borrowck. (A StorageEnd for
  a temp holding a &-borrow is not a use.)
- We do have appropriate phrasing in at least one case in this test
  so at least this might be fixable by adjusting some rustc
  control-flow?
src/test/ui/borrowck/regions-escape-bound-fn-2.nll.stderr
- NLL is on par
src/test/ui/borrowck/regions-escape-bound-fn.nll.stderr
- NLL is on par
src/test/ui/borrowck/regions-escape-unboxed-closure.nll.stderr
- NLL is on par
src/test/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.nll.stderr
- AST-borrowck provides span of the actual captureed variable.
  (Should be easy to add this to NLL.)
src/test/ui/closure-expected-type/expect-region-supply-region.nll.stderr
- There are errors (lines 42-47) that are not being reported by NLL here. (Test weakness?)
src/test/ui/closure-immutable-outer-variable.nll.stderr
- NLL is on par
src/test/ui/closure_context/issue-42065.nll.stderr
- AST-borrowck has note that "closure cannot be invoked more than once because it moves dict
src/test/ui/codemap_tests/huge_multispan_highlight.nll.stderr
- NLL is on par
src/test/ui/codemap_tests/issue-11715.nll.stderr
- NLL is on par
src/test/ui/codemap_tests/one_line.nll.stderr
- NLL is on par
src/test/ui/codemap_tests/overlapping_spans.nll.stderr
- This is (still) no longer testing what I believe it was intended to test w.r.t. diagnostics.
- See src/test/ui/const-eval/conditional_array_execution.rs:15 for a potential
  alternative test expression
src/test/ui/codemap_tests/tab_3.nll.stderr
- NLL is on par
src/test/ui/command-line-diagnostics.nll.stderr
- NLL is on par
src/test/ui/const-eval/conditional_array_execution.nll.stderr
- For some reason we are reporting line 19 two extra times under NLL?
src/test/ui/const-eval/dont_promote_unstable_const_fn.nll.stderr
- NLL reports a different set of "temporary value does not live long enough" errors
  (perhaps due to test weakness?)
src/test/ui/const-eval/issue-43197.nll.stderr
- For some reason we are reporting line 24 two extra times under NLL?
src/test/ui/const-eval/issue-44578.nll.stderr
- For some reason we are reporting line 35 two extra times under NLL?
src/test/ui/did_you_mean/issue-31424.nll.stderr
- still missing suggestion to "try removing &mut here"
src/test/ui/did_you_mean/issue-34126.nll.stderr
- NLL is on par
src/test/ui/did_you_mean/issue-34337.nll.stderr
- NLL is on par
src/test/ui/did_you_mean/issue-35937.nll.stderr
- NLL is on par
src/test/ui/did_you_mean/issue-37139.nll.stderr
- NLL is on par
src/test/ui/did_you_mean/issue-38147-1.nll.stderr
- NLL is on par
src/test/ui/did_you_mean/issue-38147-2.nll.stderr
- NLL is missing suggestion to change struct field type to &'a mut String
src/test/ui/did_you_mean/issue-38147-3.nll.stderr
- NLL is missing suggestion to change struct field type to &'a mut String
src/test/ui/did_you_mean/issue-38147-4.nll.stderr
- NLL is on par
src/test/ui/did_you_mean/issue-39544.nll.stderr
- NLL is losing the lifetime in &'z mut self of its suggestion
- NLL is printing &mut Z instead of &'a mut Self in its suggestion (original was &'a Self)
src/test/ui/did_you_mean/issue-40823.nll.stderr
- NLL is on par
- (its suggestion "consider changing this to be a mutable reference: &mut [1, 2, 3, 4]"
  is actually valid; I guess that does not get promoted to static storage.)
src/test/ui/dropck/dropck-eyepatch-extern-crate.nll.stderr
- We're missing lots of errors due to test weakness.
src/test/ui/dropck/dropck-eyepatch-reorder.nll.stderr
- We're missing lots of errors due to test weakness.
src/test/ui/dropck/dropck-eyepatch.nll.stderr
- We're missing lots of errors due to test weakness.
src/test/ui/error-codes/E0017.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0388.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0389.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0499.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0502.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0503.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0504.nll.stderr
- NLL still fails to mention the (special?) role of the closure here.
src/test/ui/error-codes/E0505.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0597.nll.stderr
- NLL is on par
src/test/ui/error-codes/E0621-does-not-trigger-for-closures.nll.stderr
- NLL is more succinct (and clearer?), but also uses "free region" jargon
src/test/ui/existential_types/generic_type_does_not_live_long_enough.nll.stderr
- NLL is missing "error[E0310]: the parameter type T may not live long enough"
  (is it masked by the earlier reported error under NLL?)
src/test/ui/feature-gate-nll.nll.stderr
- expected. Should change to use ignore-compare-mode-nll after that lands.
src/test/ui/fn-closure-mutable-capture.nll.stderr
- NLL is on par
src/test/ui/generator/borrowing.nll.stderr
- NLL is on par
src/test/ui/generator/dropck.nll.stderr
- NLL is on par
src/test/ui/generator/ref-escapes-but-not-over-yield.nll.stderr
- NLL leaves out the "borrowed value needs to live until here" message. (Acceptable?)
src/test/ui/generator/yield-in-args.nll.stderr
- NLL is on par
src/test/ui/generator/yield-while-iterating.nll.stderr
- NLL fails to highlight the use of x that is causing closure to borrow it
src/test/ui/generator/yield-while-ref-reborrowed.nll.stderr
- NLL fails to highlight the use of x that is causing closure to borrow it
src/test/ui/hygiene/fields-move.nll.stderr
- NLL is on par
src/test/ui/hygiene/fields-numeric-borrowck.nll.stderr
- NLL is on par
src/test/ui/impl-trait/static-return-lifetime-infered.nll.stderr
- NLL's message fails to suggest adding + <lifetime> to the impl Iterator return type
  (which might be a big point of this ui test?)
src/test/ui/in-band-lifetimes/impl/dyn-trait.nll.stderr
- NLL's message is succinct, but fails to mention the role of 'a (which is a more
  precise bound on the validity of x...)
src/test/ui/in-band-lifetimes/mismatched.nll.stderr
- NLL is on par
src/test/ui/in-band-lifetimes/mismatched_trait.nll.stderr
- NLL is on par
src/test/ui/in-band-lifetimes/mut_while_borrow.nll.stderr
- NLL is on par
src/test/ui/issue-10291.nll.stderr
- NLL uses "free region" jargon.
- NLL also mentions 'x but does not have any span that includes that lifetime.
src/test/ui/issue-10398.nll.stderr
- NLL is on par
src/test/ui/issue-11192.nll.stderr
- NLL is on par
src/test/ui/issue-11873.nll.stderr
- NLL is on par
src/test/ui/issue-12041.nll.stderr
- NLL is on par
src/test/ui/issue-12470.nll.stderr
- NLL is on par
src/test/ui/issue-12567.nll.stderr
- NLL is missing the error from line 20?
src/test/ui/issue-13058.nll.stderr
- NLL is missing "explicit lifetime required in type of cont error which adds 'r to &T.
- (perhaps this is an instance where fixing the "not reporting
  region error due to nll" would add value?)
src/test/ui/issue-14285.nll.stderr
- NLL is on par
src/test/ui/issue-15034.nll.stderr
- NLL is on par
src/test/ui/issue-16683.nll.stderr
- NLL's error looks very different from AST-borrowcks. Not clear which one is "better" UX.
src/test/ui/issue-16922.nll.stderr
- NLL is on par
src/test/ui/issue-17263.nll.stderr
- We're missing all the errors due to test weakness.
src/test/ui/issue-17385.nll.stderr
- NLL has redundant diagnostics ... once per match arm rather than once per match?
- NLL is reporting span of whole match for a use match foo { ... }; would be better to
  just highlight input to match in common case.
src/test/ui/issue-17545.nll.stderr
- NLL is missing suggestion to pull id() up to a let binding to extend its lifetime.
src/test/ui/issue-17728.nll.stderr
- NLL is failing to report lifetime mismatch error
src/test/ui/issue-17758.nll.stderr
- NLL's error looks very different from AST-borrowcks. Not clear which one is "better" UX.a
- at the very least, mentioning 'a would probably be a good idea.
src/test/ui/issue-17954.ast.nll.stderr
- NLL says that (thread-local) FOO is dropped, but that terminology
  doesn't make sense for statics
src/test/ui/issue-18118.nll.stderr
- NLL is on par
src/test/ui/issue-18566.nll.stderr
- NLL is on par
src/test/ui/issue-18783.nll.stderr
- NLL is on par
src/test/ui/issue-20801.nll.stderr
- NLL is saying "borrowed content" when AST says more specific "dreference of raw pointer"
src/test/ui/issue-21600.nll.stderr
- NLL is on par
src/test/ui/issue-24267-flow-exit.nll.stderr
- NLL is on par
src/test/ui/issue-24357.nll.stderr
- NLL is missing mention of "(into closure)" when it says value is moved
src/test/ui/issue-25579.ast.nll.stderr
- We're missing all the errors. Is this a weak test, or is this an demo of NLL feature?
src/test/ui/issue-25793.nll.stderr
- NLL is missing mention of "(into closure)" when it says value is moved
src/test/ui/issue-2590.nll.stderr
- NLL is on par
src/test/ui/issue-26217.nll.stderr
- NLL is failing to report lifetime mismatch error
- NLL is reporting that main cannot have generic parameters.
  (But maybe should be revised in test to not do that.)
src/test/ui/issue-27592.nll.stderr
- NLL is on par
src/test/ui/issue-30355.nll.stderr
- NLL is on par
src/test/ui/issue-30438-a.nll.stderr
- NLL is missing suggestion to pull id() up to a let binding to extend its lifetime.
src/test/ui/issue-30438-c.nll.stderr
- NLL is on par
src/test/ui/issue-3154.nll.stderr
- NLL is on par
src/test/ui/issue-33819.nll.stderr
- NLL is on par
src/test/ui/issue-36082.ast.nll.stderr
- NLL is missing suggestion to pull id() up to a let binding to extend its lifetime.
src/test/ui/issue-36400.nll.stderr
- NLL is on par
src/test/ui/issue-40288-2.nll.stderr
- NLL is on par
src/test/ui/issue-40288.nll.stderr
- NLL is on par
src/test/ui/issue-40402-ref-hints/issue-40402-1.nll.stderr
- NLL is on par
src/test/ui/issue-40402-ref-hints/issue-40402-2.nll.stderr
- NLL is on par (assuming match ergonomics)
src/test/ui/issue-40510-1.nll.stderr
- These are totally different. No error in AST. No comments. What is this test?
src/test/ui/issue-40510-3.nll.stderr
- These are totally different. No error in AST. No comments. What is this test?
src/test/ui/issue-41139.nll.stderr
- NLL is on par
src/test/ui/issue-41726.nll.stderr
- NLL is on par
src/test/ui/issue-42106.nll.stderr
- NLL is on par
src/test/ui/issue-42344.nll.stderr
- NLL is on par
src/test/ui/issue-42796.nll.stderr
- NLL is on par
src/test/ui/issue-4335.nll.stderr
- NLL is missing suggestion to use move to force closure to take ownership of v
- (also, NLL diagnostic order would be improved if we sorted by span.)
src/test/ui/issue-45199.ast.nll.stderr
- NLL is on par
src/test/ui/issue-46023.ast.nll.stderr
- NLL is on par
src/test/ui/issue-46604.ast.nll.stderr
- NLL is on par
src/test/ui/issue-51244.nll.stderr
- NLL is on par
src/test/ui/issue-52049.nll.stderr
- NLL is on par
src/test/ui/issue-52126-assign-op-invariance.nll.stderr
- NLL is on par
src/test/ui/issue-52213.nll.stderr
- NLL uses "free region" jargon.
- NLL also mentions 'a and 'b but does not have any span that includes those lifetimes.
src/test/ui/issue-6801.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/42701_one_named_and_one_anonymous.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex1-return-one-existing-name-early-bound-in-struct.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else-2.nll.stderr
- NLL chooses not great span in its "lifetime 'a required." message
src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else-3.nll.stderr
- NLL chooses not great span in its "lifetime 'a required." message
src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else-using-impl-2.nll.stderr
- NLL chooses not great span in its "lifetime 'a required." message
src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else-using-impl-3.nll.stderr
- NLL is okay
src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else-using-impl.nll.stderr
- NLL chooses not great span in its "lifetime 'a required." message
src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else.nll.stderr
- NLL chooses not great span in its "lifetime 'a required." message
src/test/ui/lifetime-errors/ex1-return-one-existing-name-return-type-is-anon.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex1-return-one-existing-name-self-is-anon.nll.stderr
- NLL is okay
src/test/ui/lifetime-errors/ex2a-push-one-existing-name-2.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex2a-push-one-existing-name-early-bound.nll.stderr
- NLL reports type inference failure in "unrelated" spot. (Test weakness?)
src/test/ui/lifetime-errors/ex2a-push-one-existing-name.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex2b-push-no-existing-names.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex2c-push-inference-variable.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex2d-push-inference-variable-2.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex2e-push-inference-variable-3.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-2.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-3.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-2.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-3.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-4.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-earlybound-regions.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs-latebound-regions.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-both-are-structs.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-latebound-regions.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct-2.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct-3.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct-4.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-one-is-struct.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-return-type-is-anon.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-self-is-anon.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-using-fn-items.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-using-impl-items.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions-using-trait-objects.nll.stderr
- NLL is on par
src/test/ui/lifetime-errors/ex3-both-anon-regions.nll.stderr
- NLL is on par
src/test/ui/lifetimes/borrowck-let-suggestion.nll.stderr
- NLL is on par
src/test/ui/macros/span-covering-argument-1.nll.stderr
- NLL is on par
src/test/ui/moves-based-on-type-block-bad.nll.stderr
- NLL is on par
src/test/ui/moves-based-on-type-match-bindings.nll.stderr
- NLL is on par
src/test/ui/nll/get_default.nll.stderr
- NLL is on par
src/test/ui/region-borrow-params-issue-29793-small.nll.stderr
- NLL is missing suggestion to use move to force closure to take ownership
- NLL is missing other special handling of describing errors around closures
- This test is a bit too long, in that it makes the diff hard to parse
src/test/ui/regions-nested-fns-2.nll.stderr
- NLL is missing suggestion to use move to force closure to take ownership
- NLL is missing other special handling of describing errors around closures
src/test/ui/rfc-2005-default-binding-mode/borrowck-issue-49631.nll.stderr
- NLL is on par
src/test/ui/rfc-2005-default-binding-mode/enum.nll.stderr
- NLL is on par
src/test/ui/rfc-2005-default-binding-mode/explicit-mut.nll.stderr
- NLL is on par
src/test/ui/span/borrowck-borrow-overloaded-auto-deref-mut.nll.stderr
- NLL seems to be missing occurrences of the E0499 diagnostic
- NLL has left lifetime out of its suggestino of &mut Own<Point>
src/test/ui/span/borrowck-borrow-overloaded-deref-mut.nll.stderr
- NLL has left lifetime out of its suggestino of &mut Own<Point>
src/test/ui/span/borrowck-call-is-borrow-issue-12224.nll.stderr
- NLL is failing to highlight the occurrence of f within the closure
  that causes the erroneous move.
src/test/ui/span/borrowck-call-method-from-mut-aliasable.nll.stderr
- NLL is on par
src/test/ui/span/borrowck-fn-in-const-b.nll.stderr
- NLL is on par
src/test/ui/span/borrowck-let-suggestion-suffixes.nll.stderr
- NLL is missing the error about extent of young (perhaps test weakness?)
- NLL is missing suggestion to use let binding to extend temps' lifetimes.
src/test/ui/span/borrowck-object-mutability.nll.stderr
- NLL is on par
src/test/ui/span/borrowck-ref-into-rvalue.nll.stderr
- NLL is on par
src/test/ui/span/destructor-restrictions.nll.stderr
- NLL is on par
src/test/ui/span/dropck-object-cycle.nll.stderr
- NLL is on par
src/test/ui/span/dropck_arr_cycle_checked.nll.stderr
- NLL is probably on par;
  NLL might be leaving out one of the errors. Or AST-borrowck might be redundantly reporting one.
- (also, NLL diagnostic order would be improved if we sorted by span.)
src/test/ui/span/dropck_direct_cycle_with_drop.nll.stderr
- NLL is on par
src/test/ui/span/dropck_misc_variants.nll.stderr
- NLL is on par
src/test/ui/span/dropck_vec_cycle_checked.nll.stderr
- NLL is probably on par;
  NLL might be leaving out one of the errors. Or AST-borrowck might be redundantly reporting one.
- (also, NLL diagnostic order would be improved if we sorted by span.)
src/test/ui/span/issue-11925.nll.stderr
- NLL is on par
src/test/ui/span/issue-15480.nll.stderr
- NLL is on par
src/test/ui/span/issue-23338-locals-die-before-temps-of-body.nll.stderr
- NLL was expected to fix this; see dropck: track order of destruction for r-value temporaries #22323
src/test/ui/span/issue-24805-dropck-child-has-items-via-parent.nll.stderr
- NLL is on par
src/test/ui/span/issue-24805-dropck-trait-has-items.nll.stderr
- NLL is on par
src/test/ui/span/issue-24895-copy-clone-dropck.nll.stderr
- NLL is on par
src/test/ui/span/issue-25199.nll.stderr
- NLL has one less error, but that might be expected for this test.
  (not sure if there's a "test weakness" to be removed in this case)
src/test/ui/span/issue-26656.nll.stderr
- NLL is on par
src/test/ui/span/issue-29106.nll.stderr
- NLL is on par
src/test/ui/span/issue-36537.nll.stderr
- NLL is on par
src/test/ui/span/issue-40157.nll.stderr
- NLL is on par
src/test/ui/span/issue28498-reject-ex1.nll.stderr
- NLL has one less error, but that might be expected for this test.
  (not sure if there's a "test weakness" to be removed in this case)
- Note: if you remove the line that causes the initial error, the
  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.
src/test/ui/span/issue28498-reject-lifetime-param.nll.stderr
- NLL has one less error, but that is expected for this test.
  See NLL lets borrowck observe drop order for let (a, b); #51036
src/test/ui/span/issue28498-reject-passed-to-fn.nll.stderr
- NLL has one less error, but that is expected for this test.
  See NLL lets borrowck observe drop order for let (a, b); #51036
src/test/ui/span/issue28498-reject-trait-bound.nll.stderr
- NLL has one less error, but that is expected for this test.
  See NLL lets borrowck observe drop order for let (a, b); #51036
src/test/ui/span/mut-arg-hint.nll.stderr
- NLL has left lifetime out of its suggestion of &mut std::string::String
src/test/ui/span/mut-ptr-cant-outlive-ref.nll.stderr
- NLL is on par
src/test/ui/span/range-2.nll.stderr
- NLL is probably on par
- (NLL diagnostic order would be improved if we sorted by span.)
src/test/ui/span/regionck-unboxed-closure-lifetimes.nll.stderr
- NLL is on par
src/test/ui/span/regions-close-over-borrowed-ref-in-obj.nll.stderr
- NLL is on par
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).
src/test/ui/span/regions-escape-loop-via-variable.nll.stderr
- NLL is on par
- but it would be nice if we made it clear that "later use"
  here is due to a later loop iteration.
src/test/ui/span/regions-escape-loop-via-vec.nll.stderr
- NLL is on par
- (NLL diagnostic order might be improved if we sorted by span, but
  AST-borrowck output is also not 100% sorted w.r.t. span here)
src/test/ui/span/regions-infer-borrow-scope-within-loop.nll.stderr
- NLL is on par
src/test/ui/span/send-is-not-static-ensures-scoping.nll.stderr
- NLL is on par
- (NLL diagnostic order would be improved if we sorted by span.)
src/test/ui/span/send-is-not-static-std-sync-2.nll.stderr
- NLL is on par
src/test/ui/span/send-is-not-static-std-sync.nll.stderr
- NLL is on par
src/test/ui/span/slice-borrow.nll.stderr
- NLL is on par
src/test/ui/span/vec-must-not-hide-type-from-dropck.nll.stderr
- NLL is on par
src/test/ui/span/vec_refs_data_with_early_death.nll.stderr
- NLL is on par
- (NLL diagnostic order would be improved if we sorted by span.)
src/test/ui/span/wf-method-late-bound-regions.nll.stderr
- NLL is on par
src/test/ui/trivial-bounds-inconsistent-copy-reborrow.nll.stderr
- NLL has left lifetime out of its suggestion of &mut &mut i32
src/test/ui/underscore-lifetime/dyn-trait-underscore.nll.stderr
- NLL is on par

pnkfelix · 2018-07-27T16:05:25Z

(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 · 2018-07-30T15:26:08Z

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 · 2018-07-30T21:38:42Z

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 · 2018-07-30T21:44:41Z

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 match ergonomics. 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 a ref or ref mut, and instead we suggest that they stop dereferencing the match input 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

    
           error[E0507]: cannot move out of borrowed content 
        
             --> $DIR/borrowck-move-error-with-note.rs:21:11 
        
              | 
        
           LL |     match *f {             //~ ERROR cannot move out of 
        
              |           ^^ 
        
              |           | 
        
              |           cannot move out of borrowed content 
        
              |           help: consider removing this dereference operator: `f`

rust/src/test/ui/issue-40402-ref-hints/issue-40402-2.nll.stderr

Lines 1 to 10 in fefe816

    
           error[E0507]: cannot move out of borrowed content 
        
             --> $DIR/issue-40402-2.rs:15:18 
        
              | 
        
           LL |     let (a, b) = x[0]; //~ ERROR cannot move out of indexed content 
        
              |          -  -    ^^^^ 
        
              |          |  |    | 
        
              |          |  |    cannot move out of borrowed content 
        
              |          |  |    help: consider using a reference instead: `&x[0]` 
        
              |          |  move occurs because b has type `std::string::String`, which does not implement the `Copy` trait 
        
              |          move occurs because a has type `std::string::String`, which does not implement the `Copy` trait

pnkfelix · 2018-07-30T21:48:38Z

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 · 2018-07-30T21:53:19Z

Missing `move` suggestion: #51169

These tests all would benefit from a fix for #51169

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/issue-4335.nll.stderr
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/borrowck/borrowck-escaping-closure-error-1.nll.stderr
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/borrowck/borrowck-escaping-closure-error-2.nll.stderr
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/region-borrow-params-issue-29793-small.nll.stderr
- NLL is also missing other special handling of describing errors around closures
- This test is a bit too long, in that it makes the diff hard to parse
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/regions-nested-fns-2.nll.stderr
- NLL is also missing other special handling of describing errors around closures

pnkfelix · 2018-07-30T21:56:18Z

NLL diagnostic for `+=` is too desugared

In https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/augmented-assignments.nll.stderr the diagnostic is not ideal; the MIR error is a little confusing because its describing micro-ops that are not immediately visible when one just sees x += x.

pnkfelix · 2018-07-30T22:02:50Z

The "Later used here" diagnostic is not ideal for return values

In this:

rust/src/test/ui/borrowck/promote-ref-mut-in-let-issue-46557.nll.stderr

Lines 4 to 13 in fefe816

    
           LL |   fn gimme_static_mut_let() -> &'static mut u32 { 
        
              |  _______________________________________________- 
        
           LL | |     let ref mut x = 1234543; //~ ERROR 
        
              | |                     ^^^^^^^ temporary value does not live long enough 
        
           LL | |     x 
        
           LL | | } 
        
              | | - 
        
              | | | 
        
              | |_temporary value only lives until here 
        
              |   borrow later used here

the "later used here" messaging is confusing and (in my opinion) strictly worse than AST-borrowck.

NLL does have appropriate phrasing in some other cases in this same test file (" borrowed value must be valid for the static lifetime") so at least this might be fixable by adjusting some rustc control-flow?

Edit: improved by #53177.

pnkfelix · 2018-07-30T22:08:43Z

Test Weakness (might mean NLL is not being tested enough)

Here are some tests where the NLL output is missing some errors, but AFAICT it is because the tests need to be updated to make them robust to NLL semantics rather than assuming lexical lifetimes. This may be fixable with something similar to PR #51057

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/closure-expected-type/expect-region-supply-region.nll.stderr has errors (lines 42-47) that are not being reported by NLL here. (Test weakness?)
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/const-eval/dont_promote_unstable_const_fn.nll.stderr reports a different set of "temporary value does not live long enough" errors (perhaps due to test weakness?)
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/dropck/dropck-eyepatch-extern-crate.nll.stderr missing lots of errors due to test weakness.
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/dropck/dropck-eyepatch-reorder.nll.stderr missing lots of errors due to test weakness.
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/dropck/dropck-eyepatch.nll.stderr missing lots of errors due to test weakness.
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/issue-17263.nll.stderr missing all the errors due to test weakness.
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/lifetime-errors/ex2a-push-one-existing-name-early-bound.nll.stderr reports type inference failure in "unrelated" spot. (Test weakness?)
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/codemap_tests/overlapping_spans.nll.stderr is (still) no longer testing what I believe it was intended to test w.r.t. diagnostics.
- See for a potential alternative test expression:
  
  rust/src/test/ui/const-eval/conditional_array_execution.rs
  
  Line 15 in fefe816
  
  const FOO: u32 = [X - Y, Y - X][(X < Y) as usize];

pnkfelix · 2018-07-30T22:16:25Z

NLL Diagnostic Redundancy

In the following tests we report certain lines as errors two extra times each.

And in the following test we have errors once per match arm rather than AST-borrowck's once per match:

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/issue-17385.nll.stderr
- In addition for this test, NLL is reporting span of whole match for a use match foo { ... }; would be better to just highlight input to match in common case.

pnkfelix · 2018-07-30T22:23:20Z

NLL chooses not great span in its "lifetime required" message

All of these tests it seems like NLL is choosing a strictly worse span for is suggestion than the span suggested by AST-borrowck:

pnkfelix · 2018-07-30T22:26:22Z

NLL fails to report lifetime mismatch error

In these cases NLL is failing to report a lifetime mismatch error

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/issue-17728.nll.stderr
https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/issue-26217.nll.stderr
- Here, NLL is reporting that main cannot have generic parameters. (But maybe should be revised in test to not do that in the first place.)

estebank · 2018-09-17T20:17:05Z

Test Weakness (might mean NLL is not being tested enough)

https://github.com/rust-lang/rust/blob/fefe81605d6111faa8dbb3635ab2c51d59de740a/src/test/ui/codemap_tests/overlapping_spans.nll.stderr is (still) no longer testing what I believe it was intended to test w.r.t. diagnostics.

I believe that test can be removed outright. It'd be impossible for a new change to go through that breaks this kind of output without it being picked up by multiple other stderr tests. This is an artifact of the transition period to the "new" output style.

pnkfelix · 2018-09-24T12:43:19Z

Much has changed since this review was done. Not only in the implementation of NLL (which has advanced considerably), but also the test suite, where things like PR #53196 have added a slew of new *.nll.stderr files that need a top-down review.

(For an example of a *.nll.stderr discrepancy injected since this review that had not yet been caught, see the notes that I added here)

So, what to do... I think the right thing is to open an entirely fresh ticket saying we need a third (and hopefully final?) round of review.

Once work on that ticket is initiated, is there any value in leaving this ticket open? Seems doubtful.

pnkfelix · 2018-09-24T12:58:36Z

I'm leaving this ticket open right now, despite having opened #54528, under the theory that until effort on #54528 starts, there is useful information in the table on this ticket that could still guide ongoing efforts to improve NLL.

However, as soon as anyone starts working on the review associated with #54528, they should probably close this ticket (#52663), in order to encourage there be at most one central location for tracking the differences in NLL diagnostic output.

@nikomatsakis

…ime-suggestion-test, r=nikomatsakis Remove unneccessary error from test, revealing NLL error. Part of rust-lang#52663. Removes unnecessary type mismatch error from test that was hiding borrow check error from NLL stderr. r? @nikomatsakis

@nikomatsakis

…suggestion, r=nikomatsakis NLL is missing struct field suggestion Part of rust-lang#52663. This commit adds suggestions to change the definitions of fields in struct definitions from immutable references to mutable references. r? @nikomatsakis cc @pnkfelix

@nikomatsakis

… r=nikomatsakis Better Diagnostic for Trait Object Capture Part of rust-lang#52663. This commit enhances `LaterUseKind` detection to identify when a borrow is captured by a trait object which helps explain why there is a borrow error. r? @nikomatsakis cc @pnkfelix

@nikomatsakis

… r=nikomatsakis NLL is missing struct field suggestion Part of #52663. This commit adds suggestions to change the definitions of fields in struct definitions from immutable references to mutable references. r? @nikomatsakis cc @pnkfelix

@nikomatsakis

…akis Better Diagnostic for Trait Object Capture Part of #52663. This commit enhances `LaterUseKind` detection to identify when a borrow is captured by a trait object which helps explain why there is a borrow error. r? @nikomatsakis cc @pnkfelix

@nikomatsakis

…nter, r=pnkfelix NLL says "borrowed content" instead of more precise "dereference of raw pointer" Part of rust-lang#52663. Previously, move errors involving the dereference of a raw pointer would say "borrowed content". This commit changes it to say "dereference of raw pointer". r? @nikomatsakis cc @pnkfelix

@nikomatsakis

…nter, r=pnkfelix NLL says "borrowed content" instead of more precise "dereference of raw pointer" Part of rust-lang#52663. Previously, move errors involving the dereference of a raw pointer would say "borrowed content". This commit changes it to say "dereference of raw pointer". r? @nikomatsakis cc @pnkfelix

@nikomatsakis

…=nikomatsakis NLL lacks various special case handling of closures Part of #52663. Firstly, this PR extends existing handling of closures to also support generators. Second, this PR adds the note found in the AST when a closure is invoked twice and captures a variable by-value: ```text note: closure cannot be invoked more than once because it moves the variable `dict` out of its environment --> $DIR/issue-42065.rs:16:29 | LL | for (key, value) in dict { | ^^^^ ``` r? @nikomatsakis cc @pnkfelix

pnkfelix · 2018-10-24T10:43:17Z

closing as I am starting on #54528 now.

This is based on the feedback from estebank: """ I believe that test can be removed outright. It'd be impossible for a new change to go through that breaks this kind of output without it being picked up by multiple other `stderr` tests. This is an artifact of the transition period to the "new" output style. """ see: rust-lang#52663 (comment)

pnkfelix added A-NLL Area: Non-lexical lifetimes (NLL) WG-compiler-nll labels Jul 24, 2018

pnkfelix added this to the Rust 2018 Release Candidate milestone Jul 24, 2018

pnkfelix added the NLL-diagnostics Working towards the "diagnostic parity" goal label Jul 24, 2018

pnkfelix mentioned this issue Jul 27, 2018

nll: remove "warning: not reporting region error due to nll" #52768

Closed

This was referenced Jul 30, 2018

NLL: rewrites should include actual source input, not solely the hypothesized rewrites #52877

Closed

NLL: suggested rewritten input types should preserve user-written lifetimes when needed #52880

Closed

This comment has been minimized.

Sign in to view

pnkfelix self-assigned this Sep 11, 2018

pnkfelix changed the title ~~NLL: need a (new) round of review deltas between .stderr and .nll.stderr output~~ NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output Sep 19, 2018

pnkfelix mentioned this issue Sep 24, 2018

[NLL] Be more permissive when checking access due to Match #53438

Merged

pnkfelix mentioned this issue Sep 24, 2018

NLL: need 3rd round of review comparing .stderr and .nll.stderr output #54528

Closed

davidtwco mentioned this issue Oct 10, 2018

NLL lacks various special case handling of closures #54976

Merged

nikomatsakis modified the milestones: Edition 2018 RC 2, Rust 2018 Release Oct 17, 2018

pnkfelix closed this as completed Oct 24, 2018

This was referenced Oct 31, 2018

various ui tests need to be made robust w.r.t NLL migration #55533

Closed

diagnostic testing: need revison to overlapping_spans test that still surfaces under NLL #51194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

pnkfelix commented Jul 24, 2018 •

edited by matthewjasper

Loading

pnkfelix commented Jul 24, 2018

pnkfelix commented Jul 25, 2018 •

edited

Loading

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 30, 2018 •

edited

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

This comment has been minimized.

pnkfelix commented Jul 30, 2018 •

edited by davidtwco

Loading

This comment has been minimized.

pnkfelix commented Jul 30, 2018 •

edited

Loading

This comment has been minimized.

pnkfelix commented Jul 30, 2018

This comment has been minimized.

pnkfelix commented Jul 30, 2018 •

edited by estebank

Loading

This comment has been minimized.

pnkfelix commented Jul 30, 2018 •

edited

Loading

This comment has been minimized.

pnkfelix commented Jul 30, 2018 •

edited

Loading

This comment has been minimized.

pnkfelix commented Jul 30, 2018

This comment has been minimized.

pnkfelix commented Jul 30, 2018

This comment has been minimized.

estebank commented Sep 17, 2018

Test Weakness (might mean NLL is not being tested enough)

pnkfelix commented Sep 24, 2018 •

edited

Loading

pnkfelix commented Sep 24, 2018

pnkfelix commented Oct 24, 2018

NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

Comments

pnkfelix commented Jul 24, 2018 • edited by matthewjasper Loading

pnkfelix commented Jul 24, 2018

pnkfelix commented Jul 25, 2018 • edited Loading

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 30, 2018 • edited Loading

pnkfelix commented Jul 30, 2018 • edited Loading

pnkfelix commented Jul 30, 2018 • edited Loading

On Par, Assuming Faith In RFC 2005

This comment has been minimized.

pnkfelix commented Jul 30, 2018 • edited by davidtwco Loading

Better Diagnostic for Trait Object Capture

This comment has been minimized.

pnkfelix commented Jul 30, 2018 • edited Loading

Missing move suggestion: #51169

This comment has been minimized.

pnkfelix commented Jul 30, 2018

NLL diagnostic for += is too desugared

This comment has been minimized.

pnkfelix commented Jul 30, 2018 • edited by estebank Loading

The "Later used here" diagnostic is not ideal for return values

This comment has been minimized.

pnkfelix commented Jul 30, 2018 • edited Loading

Test Weakness (might mean NLL is not being tested enough)

This comment has been minimized.

pnkfelix commented Jul 30, 2018 • edited Loading

NLL Diagnostic Redundancy

This comment has been minimized.

pnkfelix commented Jul 30, 2018

NLL chooses not great span in its "lifetime required" message

This comment has been minimized.

pnkfelix commented Jul 30, 2018

NLL fails to report lifetime mismatch error

This comment has been minimized.

estebank commented Sep 17, 2018

Test Weakness (might mean NLL is not being tested enough)

pnkfelix commented Sep 24, 2018 • edited Loading

pnkfelix commented Sep 24, 2018

pnkfelix commented Oct 24, 2018

pnkfelix commented Jul 24, 2018 •

edited by matthewjasper

Loading

pnkfelix commented Jul 25, 2018 •

edited

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

pnkfelix commented Jul 30, 2018 •

edited by davidtwco

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

Missing `move` suggestion: #51169

NLL diagnostic for `+=` is too desugared

pnkfelix commented Jul 30, 2018 •

edited by estebank

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

pnkfelix commented Jul 30, 2018 •

edited

Loading

pnkfelix commented Sep 24, 2018 •

edited

Loading