Skip to content
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 diagnostics replaced nice closure errors w/ indecipherable free region errors #52572

Merged
merged 6 commits into from Jul 22, 2018

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link
Member

davidtwco commented Jul 20, 2018

Fixes #51027.

r? @nikomatsakis

@davidtwco davidtwco changed the title NLL diagnostics replaced nice closure errors w/ indecipherable free region errors WIP: NLL diagnostics replaced nice closure errors w/ indecipherable free region errors Jul 20, 2018

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Jul 20, 2018

As of submission, this PR produces the following error (or equivalent):

error: borrowed data escapes outside of closure
  --> issue-45983.rs:17:27
   |
16 |     let mut x = None;
   |         ----- `x` is declared here, outside of the closure body
17 |     give_any(|y| x = Some(y));:
   |               -  ^^^^^^^^^^^ `y` escapes the closure body here
   |               |
   |               `y` is a reference that is only valid in the closure body

It fixes the following tests mentioned in the issue:

  • borrowck/issue-45983.rs
  • borrowck/issue-7573.rs
  • borrowck/regions-escape-bound-fn-2.rs
  • borrowck/regions-escape-bound-fn.rs
  • borrowck/regions-escape-unboxed-closure.rs
  • closure-expected-type/expect-region-supply-region.rs
  • error-codes/E0621-does-not-trigger-for-closures.rs (not sure how to approach)
}
},
ProjectionElem::Deref => {
if recurse {

This comment has been minimized.

@davidtwco

davidtwco Jul 20, 2018

Author Member

I wasn't sure about adding this parameter (recurse) - without it, a handful of unrelated tests start throwing duplicate errors with shorter spans, but without considering the place within the deref I don't get some of the tests that currently work.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

Hmm. So the behavior when recurse is true seems mostly correct to me -- that is, if you have a variable that is captured "by reference" (and not "by value"), then x would be desugared to *self.x. However, I think we would only then want to recurse through a single deref. Maybe this is the cause of the other tests behaving strangely?

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

I've just checked and it seems like only doing a single deref still exhibits this behaviour.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Pushed a fix for this.

self.get_argument_index_for_region(tcx, fr)
.map(|index| self.get_argument_name_and_span_for_region(mir, index))
})
.unwrap_or_else(|| span_bug!(mir.span, "can't find var name for free region {:?}", fr))

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

I'm a bit nervous about this case. Based on the other code I see, it seems like it is possible for the region to appear in the closure output, for example. I'll try to build your branch and produce an example though because it's hard for me to predict in advance.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Most recent pushed commit introduces changes that ICE due to this in some tests.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Another commit now stops those ICEs by falling back to previous error handling.

(Some(upvar_name), upvar_span)
}

/// Get argument index for a region.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

I would word this as:


Search the argument types for one that references fr (which should be a free region). Returns Some(_) with the index of the input if one is found.

NB: In the case of a clsoure, the index is indexing into the signature as seen by the user--- in particular, index 0 is not the implicit self parameter.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Pushed a commit that addresses this.

Some(upvar_index)
}

/// Get upvar name and span for a region.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

I would say something like:


Given the index of an upvar, finds its name (if any) and the span from where it was declared.


Question: Why does this return an Option? From what I can tell, it is always Some?

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

I had it return an Option so that the types where the same in the and_then sections of the function above. I'll change this to use a .map(...) in that function rather than change the return type of this one.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Pushed a commit that addresses this.

.unwrap_or_else(|| span_bug!(mir.span, "can't find var name for free region {:?}", fr))
}

/// Get upvar index for a region.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

I would say something like:


Search the upvars (if any) to find one that references fr. Return its index.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Pushed a commit that addresses this.

Some(argument_index)
}

/// Get argument name and span for a region.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

I would reword this similar to the fn for upvars.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Pushed a commit that addresses this.

@@ -224,30 +235,75 @@ impl<'tcx> RegionInferenceContext<'tcx> {

// Get a span
let (category, span) = categorized_path.first().unwrap();

match category {
ConstraintCategory::AssignmentToUpvar =>

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

This is interesting. Looking for an assignment to an upvar is not quite taking the approach I expected, but it also makes sense. I guess though that this code won't fire for something like:

fn foo(x: &mut Vec<&u32>) {
  bar(|y| x.push(y));
}

fn bar(_: impl FnOnce(&u32)) { }

Right?

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 21, 2018

Contributor

Seems ok, though I can think we can address this case via a similar set of errors, if we generalize the conditions somewhat away from assignment and more into looking at the kinds of regions involved. (Or maybe this is just another sort of case to specialize)

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

This is similar to the issue-7573.rs case that isn't yet handled. I messed around with making another variant for ConstraintCategory that would go through to the report_closure_error branch.

This comment has been minimized.

@davidtwco

davidtwco Jul 21, 2018

Author Member

Fixed with most recent commit.

@nikomatsakis nikomatsakis changed the title WIP: NLL diagnostics replaced nice closure errors w/ indecipherable free region errors NLL diagnostics replaced nice closure errors w/ indecipherable free region errors Jul 21, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jul 22, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 22, 2018

📌 Commit 58eee34 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 22, 2018

☔️ The latest upstream changes (presumably #52359) made this pull request unmergeable. Please resolve the merge conflicts.

davidtwco added some commits Jul 20, 2018

@davidtwco davidtwco force-pushed the davidtwco:issue-51027 branch from 58eee34 to c64db00 Jul 22, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jul 22, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 22, 2018

📌 Commit c64db00 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jul 22, 2018

@bors p=1

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jul 22, 2018

Giving high priority, as improving this case is a EP2 blocker.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 22, 2018

⌛️ Testing commit c64db00 with merge 32772fd...

bors added a commit that referenced this pull request Jul 22, 2018

Auto merge of #52572 - davidtwco:issue-51027, r=nikomatsakis
NLL diagnostics replaced nice closure errors w/ indecipherable free region errors

Fixes #51027.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 32772fd to master...

@bors bors merged commit c64db00 into rust-lang:master Jul 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-51027 branch Jul 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.