Skip to content

NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL #53004

@pnkfelix

Description

@pnkfelix
Member

While investigating #52979, I noticed some of the diagnostics from --edition 2018 are different than what either of the test.stderr and test.nll.stderr would lead one to expect to see.

This appears to arise from the implementation of -Z borrowck=migrate, from what I can tell.

For example, on ui/borrowck/borrowck-in-static.rs, we have from AST-borrowck:

error[E0507]: cannot move out of captured outer variable in an `Fn` closure
--> $DIR/borrowck-in-static.rs:15:17
|
LL | let x = Box::new(0);
| - captured outer variable
LL | Box::new(|| x) //~ ERROR cannot move out of captured outer variable
| ^ cannot move out of captured outer variable in an `Fn` closure
error: aborting due to previous error

and from NLL we have:

error[E0507]: cannot move out of captured variable in an `Fn` closure
--> $DIR/borrowck-in-static.rs:15:17
|
LL | Box::new(|| x) //~ ERROR cannot move out of captured outer variable
| ^ cannot move out of captured variable in an `Fn` closure
error: aborting due to previous error

but I am seeing this from -Z borrowck=migrate:

error[E0507]: cannot move out of captured variable in an `Fn` closure
  --> ../src/test/ui/borrowck/borrowck-in-static.rs:15:17
   |
15 |     Box::new(|| x) //~ ERROR cannot move out of captured outer variable
   |                 ^ cannot move out of captured variable in an `Fn` closure

error[E0507]: cannot move out of `x`, as it is a captured variable in a `Fn` closure
  --> ../src/test/ui/borrowck/borrowck-in-static.rs:15:17
   |
15 |     Box::new(|| x) //~ ERROR cannot move out of captured outer variable
   |                 ^
   |                 |
   |                 cannot move out of `x`, as it is a captured variable in a `Fn` closure
   |                 cannot move
   |
help: consider changing this to accept closures that implement `FnMut`
  --> ../src/test/ui/borrowck/borrowck-in-static.rs:15:14
   |
15 |     Box::new(|| x) //~ ERROR cannot move out of captured outer variable
   |              ^^^^

error: aborting due to 2 previous errors

My guess that this is arising because I must have missed at least one case where AST-borrowck signals an error. Thus, when the -Z borrowck=migrate runs the AST-borrowck as a fallback in response to an error encounters during the MIR-borrowck, the user can sometimes see the both sets the errors from both NLL and AST-borrowck.

Activity

changed the title [-]NLL migration mode sometimes prints sum of diagnostics from AST-borrowck and NLL[/-] [+]NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL[/+] on Aug 2, 2018
pnkfelix

pnkfelix commented on Aug 2, 2018

@pnkfelix
MemberAuthor

Actually my hypothesis that this is the sum of the two sets of errors is clearly false given even just the simple example above. Where does -Z borrowck=migrate get the message:

error[E0507]: cannot move out of `x`, as it is a captured variable in a `Fn` closure

when that particular level of detail is not included in either of the other diagnostics presented in the other two files...?

self-assigned this
on Aug 2, 2018
pnkfelix

pnkfelix commented on Aug 3, 2018

@pnkfelix
MemberAuthor

Ah wait, this was actually an expected side-effect of migrate mode, due to this part of #52681

Reservation(wk @ WriteKind::Move)
| Write(wk @ WriteKind::Move)
| Reservation(wk @ WriteKind::StorageDeadOrDrop)
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
| Write(wk @ WriteKind::StorageDeadOrDrop)
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) => {
if let Err(_place_err) = self.is_mutable(place, is_local_mutation_allowed) {
if self.tcx.migrate_borrowck() {
// rust-lang/rust#46908: In pure NLL mode this
// code path should be unreachable (and thus
// we signal an ICE in the else branch
// here). But we can legitimately get here
// under borrowck=migrate mode, so instead of
// ICE'ing we instead report a legitimate
// error (which will then be downgraded to a
// warning by the migrate machinery).
error_access = match wk {
WriteKind::MutableBorrow(_) => AccessKind::MutableBorrow,
WriteKind::Move => AccessKind::Move,
WriteKind::StorageDeadOrDrop |
WriteKind::Mutate => AccessKind::Mutate,
};
self.report_mutability_error(
place,
span,
_place_err,
error_access,
location,
);
} else {
self.tcx.sess.delay_span_bug(
span,
&format!(
"Accessing `{:?}` with the kind `{:?}` shouldn't be possible",
place, kind
),
);
}
}

pnkfelix

pnkfelix commented on Aug 3, 2018

@pnkfelix
MemberAuthor

So there a some follow up questions I now have:

  1. I do remember going through the control-flow for this case and figuring out why it "made sense" that this code path became reachable under migration mode. But I have since then forgotten the justification. It would be good to at least document the justification in a comment.
  2. More upsetting (to me at least): the new error we get here under migration mode, presumably generated via self.report_mutability_error(..), looks slightly better to me than the errors we are getting from AST-borrowck and NLL: it names x, and (more importantly) it provides a hint that you may want to change the code so that it accepts FnMut closures...
pnkfelix

pnkfelix commented on Aug 3, 2018

@pnkfelix
MemberAuthor

Having said that, neither of these follow up questions is sufficiently high priority for me to spend more time on them today. I'm going to unassign myself from this issue, but leave it open so that people can find it if they similarly encounter this arguably bizarre behavior from migration mode (which is turned on in --edition 2018 ...)

removed their assignment
on Aug 3, 2018
pnkfelix

pnkfelix commented on Aug 3, 2018

@pnkfelix
MemberAuthor

although now that I think about it more ... I'm certain that in my original justification for the newly generated diagnostics from cases like borrowck-in-static.rs, I had thought it could only arise in cases where the NLL errors would be downgraded to warnings; i.e. situations where AST-borrowck did not error.

but clearly this is a case where AST-borrowck did error, or at least something in that pipeline errored.

So something is still funky here.

added
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
on Aug 21, 2018
nikomatsakis

nikomatsakis commented on Aug 21, 2018

@nikomatsakis
Contributor

@pnkfelix I am doing a triage sweep and am not sure where to put this -- going to put in Edition Release for now I guess.

added a commit that references this issue on Oct 22, 2018
pnkfelix

pnkfelix commented on Oct 24, 2018

@pnkfelix
MemberAuthor

(I think I'm going to turn this into a metabug and will file individual issues when I encounter specific cases, like #55309)

added
metabugIssues about issues themselves ("bugs about bugs")
on Oct 24, 2018
added a commit that references this issue on Oct 25, 2018
5f1fd68
pnkfelix

pnkfelix commented on Nov 6, 2018

@pnkfelix
MemberAuthor

A lot of the cases here have been addressed, especially by PR #55221.

The 3rd NLL diagnostic review (#54528) contains an updated list of all the diagnostic deviations.

I suspect at this point that (and/or the associated project) should be the canonical database for listing deviations like the ones listed here.

So, I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-NLLArea: Non-lexical lifetimes (NLL)NLL-diagnosticsWorking towards the "diagnostic parity" goalT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.metabugIssues about issues themselves ("bugs about bugs")

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @nikomatsakis@pnkfelix

        Issue actions

          NLL migration mode sometimes prints more diagnostics than either AST-borrowck or NLL · Issue #53004 · rust-lang/rust