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

refactor uses of freevars to use the upvar list #60205

Closed
nikomatsakis opened this issue Apr 23, 2019 · 6 comments
Closed

refactor uses of freevars to use the upvar list #60205

nikomatsakis opened this issue Apr 23, 2019 · 6 comments
Assignees
Labels
E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 23, 2019

As part of #53488, we need to refactor code that relies on the freevars helper to figure out what things a closure captures. The freevars function just lists the "base variables" that a closure uses. This used to be equivalent to the sets of captures but that is what we are trying to change.

@blitzerr already introduced a new field to the TypeckTables, the upvar_list. This maps from each closure to the list of upvars captured by it. These upvars are identified by a UpvarId -- eventually, though, they will be expected to contain additional information, so that a closure like || foo(&x.y.z) might record that it captures x.y.z and not just x.

There is a list of freevars uses in this dropbox paper document. This issue should be extended with a list of the uses. I'll also add some mentoring instructions for specific cases to try and get things rolling.

There is a Zulip topic for discussion of this issue.

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Apr 23, 2019
@nikomatsakis
Copy link
Contributor Author

Mentoring instructions

The first use is in make_mirror_unadjusted:

let upvars = cx.tcx.with_freevars(expr_node_id, |freevars| {

In this code, we zip two lists:

  • the list of free variables freevars
  • the types of the upvars (captured variables) from the closure (substs.upvar_tys(def_id, cx.tcx))

These lists are meant to contain the same number of items. I think we want to change this to iterate over the upvar_uses. To start, we need to get the TypeckTables -- in this particular case, those are available from cx.tables(). We need to index the upvar_list with the closure's def-id, which is available here as the variable def_id (defined here). So we can get the list of upvars by doing cx.tables().upvar_list[&def_id] instead of calling cx.tcx.with_freevars(expr_node_id, |freevars|..).

Next problem: the old code was giving us a vector of hir::FreeVar values, but the upvar_list field will be giving us a Vec<UpvarId>. Hmm. However, if you look at what we do with each free variable (fv), you'll see that we call capture_freevar:

fn capture_freevar<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>,
closure_expr: &'tcx hir::Expr,
freevar: &hir::Freevar,
freevar_ty: Ty<'tcx>)
-> ExprRef<'tcx> {

which in turn uses the parameter freevar to... create an UpvarId:

let var_hir_id = cx.tcx.hir().node_to_hir_id(freevar.var_id());
let upvar_id = ty::UpvarId {
var_path: ty::UpvarPath { hir_id: var_hir_id },
closure_expr_id: cx.tcx.hir().local_def_id_from_hir_id(closure_expr.hir_id).to_local(),
};

So, basically, we can now pass in the UpvarId directly, which is easier. But there is one complication: the hir::Upvar included a Def value that was given to convert_var. That is actually kind of annoying, as we no longer have "ready access" to such a Def. However, I think we have all the pieces we need to construct one. We want a Def::Upvar, which needs three things:

  • a NodeId for the variable that was captured -- we can get this by invoking hir_to_node_id with the hir_id from the UpvarId.
  • an index -- this is the index within the upvar_list, so we can pass that in by modifying the loop above
  • the closure's node-id -- we can get that by calling as_local_node_id on the closure def-id (which the calling function already has)

A nicer refactoring, though, might be to extract out the handling of upvars from convert_var into a helper which does not expect a Def, but instead expects the HirId (not NodeId) of the variable being captured, index, and closure-def-id. Then we could just invoke the function directly with those things. The convert_var function could then invoke this helper after doing the conversions it is already doing.

@csmoe csmoe self-assigned this Apr 24, 2019
@JohnTitor
Copy link
Member

I'd like to work on this.

@hbina
Copy link
Contributor

hbina commented Oct 3, 2019

Is this issue still available to be picked up? I would like to work on it.

@csmoe
Copy link
Member

csmoe commented Oct 3, 2019

@hbina the instructions above is kind of out-date. rfc-2229 is paused now, you can subscribe to this workgroup in complier-team repo to get the latest update once it restarts.

@hbina
Copy link
Contributor

hbina commented Oct 3, 2019

@csmoe oh, I didn't know thats a thing. Heading over there now!

@nikomatsakis
Copy link
Contributor Author

I think this issue is kind of outdated, closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants