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

RFC-2229: Implement Precise Capture Analysis #78801

Merged
merged 16 commits into from
Nov 17, 2020

Conversation

arora-aman
Copy link
Member

@arora-aman arora-aman commented Nov 6, 2020

This PR introduces

  • Feature gate for RFC-2229 (incomplete) capture_disjoint_field
  • Rustc Attribute to print out the capture analysis rustc_capture_analysis
  • Precise capture analysis

Description of the analysis

  1. If the feature gate is not set then all variables that are not local to the closure will be added to the list of captures. (This is for backcompat)
  2. The rest of the analysis is based entirely on how the captured Places are used within the closure. Precise information (i.e. projections) about the Place is maintained throughout.
  3. To reduce the amount of information we need to keep track of, we do a minimization step. In this step, we determine a list such that no Place within this list represents an ancestor path to another entry in the list. Check Ensure minimal places are captured project-rfc-2229#9 for more detailed examples.
  4. To keep the compiler functional as before we implement a Bridge between the results of this new analysis to existing data structures used for closure captures. Note the new capture analysis results are only part of MaybeTypeckTables that is the information is only available during typeck-ing.

Known issues

  • Statements like let _ = x will make the compiler ICE when used within a closure with the feature enabled. More generally speaking the issue is caused by let statements that create no bindings and are init'ed using a Place expression.

Testing

We removed the code that would handle the case where the feature gate is not set, to enable the feature as default and did a bors try and perf run. More information here: #78762

Thanks

This has been slowly in the works for a while now.
I want to call out @Azhng @ChrisPardy @null-sleep @jenniferwills @logmosier @roxelo for working on this and the previous PRs that led up to this, @nikomatsakis for guiding us.

Closes rust-lang/project-rfc-2229#7
Closes rust-lang/project-rfc-2229#9
Closes rust-lang/project-rfc-2229#6
Closes rust-lang/project-rfc-2229#19

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2020
@arora-aman
Copy link
Member Author

arora-aman commented Nov 6, 2020

cc #76005. This PR implements part of the solution needed. Once writeback is done (rust-lang/project-rfc-2229#18), the issue can hopefully be closed.

@mark-i-m
Copy link
Member

mark-i-m commented Nov 7, 2020

Wow so exciting! Thank you @arora-aman (and Niko and @blitzerr and I'm sure many others I'm missing)!

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't make it all the way through yet :) will read more later, but here are some comments.

compiler/rustc_middle/src/ty/mod.rs Show resolved Hide resolved
compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved
///
/// For closure `fix_s`, (at a high level) the IndexMap will contain:
///
/// Place { V1, [ProjectionKind::Field(Index=0, Variant=0)] } : CaptureKind { E1, ImmutableBorrow }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should show the whole data structure. I think it looks like:

V1 -> [
    Place { V1, [ProjectionKind::Field(Index=0, Variant=0)] } : CaptureKind { E1, ImmutableBorrow },
    Place { V1, [ProjectionKind::Field(Index=1, Variant=0)] } : CaptureKind { E2, MutableBorrow }
]

Right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the output of the first pass (ignoring the init step for backcompat) which doesn't aggregate based on root variable. The aggregation happens as part of min capture pass.

This is later moved to upvars.rs when we implement Min Capture Analysis since we don't store the result of this in the TypeckResults.

compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/context.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Nov 9, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@arora-aman
Copy link
Member Author

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Copy link
Member Author

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and address the comments.

I couldn't seem to reply directly to your comments for some reason.

compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
///
/// For closure `fix_s`, (at a high level) the IndexMap will contain:
///
/// Place { V1, [ProjectionKind::Field(Index=0, Variant=0)] } : CaptureKind { E1, ImmutableBorrow }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the output of the first pass (ignoring the init step for backcompat) which doesn't aggregate based on root variable. The aggregation happens as part of min capture pass.

This is later moved to upvars.rs when we implement Min Capture Analysis since we don't store the result of this in the TypeckResults.

compiler/rustc_middle/src/ty/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved
if upvars.map_or(body_owner_is_closure, |upvars| !upvars.contains_key(var_hir_id)) {
// The nested closure might be capturing the current (enclosing) closure's local variables.
// We check if the root variable is ever mentioned within the enclosing closure, if not
// then for the current body (if it's a closure) these aren't captures, we will ignore them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have logic like this before -- what changed? I'm not sure I quite understand the example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess overall I'm confused about the role of body_owner_is_closure... can you clarify a bit?

Copy link
Member Author

@arora-aman arora-aman Nov 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we had two structures, closure_captures and upvar_capture_map. We would read what is captured from the first and check the capture kind in the latter. ExprUseVisitor only affects upvar_capture_map so it didn't matter if it contains extra entries since the extra entries (captures of the nested closure that are local variables of the enclosing) aren't listed as captures they aren't read.

We have now aggregated the two maps in one, so we need to be more careful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason body_owner_is_closure is required is because Clippy might use this over a function body, in which case we want to report this back but we don't have upvars because the body is not a closure.

|| {
println!("{}", p.x);
//~^ ERROR: Capturing p[(0, 0)] -> ImmBorrow
//~^^ ERROR: Min Capture p[(0, 0)] -> ImmBorrow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this outer closure have to capture p[(1, 0)], so that it can be given to the inner closure when that inner closure is created?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enclosing closure captures that, we capture it via MutBorrow which happens in the nested closure. (See the double error thrown for p[1, 0] thrown within c2.

//~^^^^^ ERROR: Capturing p[(1, 0)] -> MutBorrow
//~^^^^^^ ERROR: Min Capture p[(1, 0)] -> MutBorrow
c2();
println!("{}", p.y);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not to mention, I guess, the direct use of p.y here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an ImmBorrow. The nested closure uses it via MutBorrow, so we escalate to that. (Since the outer closure must capture p.y with at least the same CaptureKind as the nested closure).

if upvars.map_or(body_owner_is_closure, |upvars| !upvars.contains_key(var_hir_id)) {
// The nested closure might be capturing the current (enclosing) closure's local variables.
// We check if the root variable is ever mentioned within the enclosing closure, if not
// then for the current body (if it's a closure) these aren't captures, we will ignore them.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason body_owner_is_closure is required is because Clippy might use this over a function body, in which case we want to report this back but we don't have upvars because the body is not a closure.


let upvar_id = if body_owner_is_closure {
// Mark the place to be captured by the enclosing closure
ty::UpvarId::new(*var_hir_id, self.body_owner)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the main body which contains this closure (whose captures we are currently processing) is a Closure itself then we want to reflect that in the UpvarId that the closure capturing the Place now is the enclosing closure.

We had similar logic before where we would fake create a place using cat_captured_var which is now removed (diff at the bottom of this file or if this link works). Just one thing to note about that is that it calls into mc which is being from the perspective of the body owner here and already has the body_owner def id. It gets passed to mc in ExprUseVisitor::new. We just now keep a copy of that variable in expr_use_visitor.

Copy link
Member Author

@arora-aman arora-aman Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also critical to do because upvar_capture_map is indexed with UpvarId which essentially contains (var_hir_id, closure_local_def_id), so want to ensure that the variable is captured with respect to the enclosing closure.

In the case where the body (that contains this closure) being processed isn't a closure, we don't update because if we did, we will be setting the closure_def_id within UpvarId to a non-closure id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so there is a small problem here, we initially had

    fn cat_captured_var(
        &mut self,
        closure_hir_id: hir::HirId,
        closure_span: Span,
        var_id: hir::HirId,
    ) -> mc::McResult<PlaceWithHirId<'tcx>> {
        // Create the place for the variable being borrowed, from the
        // perspective of the creator (parent) of the closure.
        let var_ty = self.mc.node_ty(var_id)?;
        self.mc.cat_res(closure_hir_id, closure_span, var_ty, Res::Local(var_id))
    }

this here, which would do 2 things:

  • Create a fake Place with no projections,
  • but more importantly, select the PlaceBase based on if the variable was local/upvar to the parent or not.

Essentially the else case needs to a PlaceBase::Local(var_hir_id).

  • The if doesn't change because we know if the body owner is a closure then the currently being processed place is captured by it. (The check before we iterate the min captures for a variable help with that)
  • This else changes to be a Place::Local. Since from the perspective of a non-closure body owner, it's always going to be a local variable.

This is the reason Clippy tests are failing.

Just a side note I would've expected bors try to have caught this, but I guess it runs a smaller set of a test suite?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think bors try runs any tests tbh, and it can succeed even if the PR can't build the some of the tools, for example. You can make it run tests but that's more involved and t-infra could explain: IIRC you have to add a temporary commit in your PR to modify the CI toml files, turning the try builder into a test running builder, then try that and remove the commit before landing

Copy link
Member

@lqd lqd Nov 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should also mention that usually the tools' tests don't cause the PR to fail, it just changes the "toolstate" (in this case, clippy would have gone from "test pass" to "test fail", some other tools could regress as well), and the failing tool's team can then fix that (with the PR author's help). This time we're in the week to cut the new beta, and no PR regressing the tools can land during this period. If you can't fix the clippy tests, this PR can still land next week: https://forge.rust-lang.org/index.html#no-tools-breakage-week

Copy link
Member Author

@arora-aman arora-aman Nov 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing the discussion on Zulip t-infra

@arora-aman
Copy link
Member Author

@nikomatsakis updated :)

@bors
Copy link
Contributor

bors commented Nov 10, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

arora-aman and others added 13 commits November 10, 2020 20:44
Co-authored-by: Archer Zhang <archer.xn@gmail.com>
Signed-off-by: Aman Arora <me@aman-arora.com>
Co-authored-by: Dhruv Jauhar <dhruvjhr@gmail.com>
Co-authored-by: Dhruv Jauhar <dhruvjhr@gmail.com>
Co-authored-by: Chris Pardy <chrispardy36@gmail.com>
Co-authored-by: Logan Mosier <logmosier@gmail.com>
Co-authored-by: Chris Pardy <chrispardy36@gmail.com>
Co-authored-by: Logan Mosier <logmosier@gmail.com>
Co-authored-by: Jenny Wills <wills.jenniferg@gmail.com>
Co-authored-by: Aman Arora <me@aman-arora.com>
@arora-aman
Copy link
Member Author

arora-aman commented Nov 11, 2020

Added a test for enums in closures with the rebase.

@arora-aman
Copy link
Member Author

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I understand the tests now. This is looking great! I left one nit.

self.tcx.has_attr(closure_def_id, sym::rustc_capture_analysis)
}

fn log_closure_capture_info(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is logging every capture...

Copy link
Member Author

@arora-aman arora-aman Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the function to express the intent more clearly

}
}

fn log_closure_min_capture_info(&self, closure_def_id: DefId, closure_span: Span) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

....and this is logging the minimum captures ...?

let output_str = format!("Capturing {}", capture_str);

let span = capture_info.expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
self.tcx.sess.span_err(span, &output_str);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought I have for how to make this clearer is to add a "note" that indicates which closure this is captured by. Something like:

let mut diag = self.tcx.sess.struct_span_err(span, &output);
diag.span_note(closure_span, "captured by this closure");
diag.emit();

@jonas-schievink
Copy link
Contributor

@bors rollup=never

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 14, 2020
…akis

RFC-2229: Implement Precise Capture Analysis

### This PR introduces
- Feature gate for RFC-2229 (incomplete) `capture_disjoint_field`
- Rustc Attribute to print out the capture analysis `rustc_capture_analysis`
- Precise capture analysis

### Description of the analysis
1. If the feature gate is not set then all variables that are not local to the closure will be added to the list of captures. (This is for backcompat)
2. The rest of the analysis is based entirely on how the captured `Place`s are used within the closure. Precise information (i.e. projections) about the `Place` is maintained throughout.
3. To reduce the amount of information we need to keep track of, we do a minimization step. In this step, we determine a list such that no Place within this list represents an ancestor path to another entry in the list.  Check rust-lang/project-rfc-2229#9 for more detailed examples.
4. To keep the compiler functional as before we implement a Bridge between the results of this new analysis to existing data structures used for closure captures. Note the new capture analysis results are only part of MaybeTypeckTables that is the information is only available during typeck-ing.

### Known issues
- Statements like `let _ = x` will make the compiler ICE when used within a closure with the feature enabled. More generally speaking the issue is caused by `let` statements that create no bindings and are init'ed using a Place expression.

### Testing
We removed the code that would handle the case where the feature gate is not set, to enable the feature as default and did a bors try and perf run. More information here: rust-lang#78762

### Thanks
This has been slowly in the works for a while now.
I want to call out `@Azhng` `@ChrisPardy` `@null-sleep` `@jenniferwills` `@logmosier` `@roxelo` for working on this and the previous PRs that led up to this, `@nikomatsakis` for guiding us.

Closes rust-lang/project-rfc-2229#7
Closes rust-lang/project-rfc-2229#9
Closes rust-lang/project-rfc-2229#6
Closes rust-lang/project-rfc-2229#19

r? `@nikomatsakis`
@bors
Copy link
Contributor

bors commented Nov 14, 2020

⌛ Testing commit c50e57f with merge e914809e6f7c1a822b4b51001a92f3ed6fbc09b6...

@bors
Copy link
Contributor

bors commented Nov 14, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 14, 2020
@lqd
Copy link
Member

lqd commented Nov 14, 2020

Some of the blessed tests seem to cause the failure (or a transient problem on CI as the failures appear confusing to t-infra)

@arora-aman
Copy link
Member Author

arora-aman commented Nov 14, 2020

Clippy tests are failing. I didn't bless them and they shouldn't be blessed. This is an actual error. Part of the problem is explained in the thread above. The fix there didn't resolve the issue when I tried locally so I need to investigate more to get a complete picture.

@arora-aman
Copy link
Member Author

arora-aman commented Nov 15, 2020

Clippy tests are failing. I didn't bless them and they shouldn't be blessed. This is an actual error. Part of the problem is explained in the thread above. The fix there didn't resolve the issue when I tried locally so I need to investigate more to get a complete picture.

The other part of the problem is that Clippy uses ExprUseVisitor after Typeck writeback. Since we don't writeback closure_min_captures just yet, we don't see any captures for the closures when Clippy walks captures for closures. This also prevents us from hitting the other issue I mentioned.

For example for white_let_on_iterator::issue3670 (one of the failing tests)

   7   │   unvars_mentioned=Some(
   8   │     {
   9   │         HirId {
  10   │             owner: DefId(0:3 ~ while_let_on_iterator[317d]::issue3670),
  11   │             local_id: 2,
  12   │         }: Upvar {
  13   │             span: $DIR/while_let_on_iterator.rs:14:34: 14:38 (#0),
  14   │         },
  15   │     },
  16   │ ) closure_captures={
  17   │     DefId(0:4 ~ while_let_on_iterator[317d]::issue3670::{closure#0}): {
  18   │         HirId {
  19   │             owner: DefId(0:3 ~ while_let_on_iterator[317d]::issue3670),
  20   │             local_id: 2,
  21   │         }: UpvarId(HirId { owner: DefId(0:3 ~ while_let_on_iterator[317d]::issue3670), local_id: 2 };`ite
       │ r`;DefId(0:4 ~ while_let_on_iterator[317d]::issue3670::{closure#0})),
  22   │     },
  23   │ }
  24   │  min_caps={}

Clippy uses `ExprUseVisitor` and atleast in some cases it runs
after writeback.

We currently don't writeback the min_capture results of closure
capture analysis since no place within the compiler itself uses it.

In the short term to fix clippy we add a fallback when walking captures
of a closure to check if closure_capture analysis has any entries in it.

Writeback for closure_min_captures will be implemented in
rust-lang/project-rfc-2229#18
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2020

📌 Commit 40dfe1e has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2020
@bors
Copy link
Contributor

bors commented Nov 17, 2020

⌛ Testing commit 40dfe1e with merge b5c37e8...

@bors
Copy link
Contributor

bors commented Nov 17, 2020

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing b5c37e8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 17, 2020
@bors bors merged commit b5c37e8 into rust-lang:master Nov 17, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 17, 2020
@arora-aman arora-aman deleted the min_capture branch November 17, 2020 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
10 participants