Stream: t-compiler/wg-nll

Topic: #53488 closure and disjoint fields


nikomatsakis (Oct 29 2018 at 17:27, on Zulip):

So @blitzerr regarding #53488, I think maybe the first thing to do is to spend a bit of time looking at how "upvar inference" and closure desugaring works now. Actually, I wonder if you'd be interested in trying to write up some material in the rustc-guide about it? What I can imagine is: you + me do a video call to talk it over (which we can record and post) and you try to distill some of that into a chapter, which we can then edit together. Thoughts? (Feel free to say no if you don't feel comfortable writing -- the video call may be a good idea regardless)

blitzerr (Oct 29 2018 at 17:31, on Zulip):

Absolutely, I will look up the code and pen down my understanding in the rustc guide. :+1:

nikomatsakis (Oct 29 2018 at 17:32, on Zulip):

Do you want to try and schedule a video call, @blitzerr ?

blitzerr (Oct 29 2018 at 17:32, on Zulip):

Sure, we can definitely do a video call and get started with it.

blitzerr (Oct 29 2018 at 17:32, on Zulip):

Sure, how do I schedule a call?

blitzerr (Oct 29 2018 at 17:32, on Zulip):

Google calendar or something ?

nikomatsakis (Oct 29 2018 at 17:32, on Zulip):

I'm not sure what time would work for me this week. I guess the first question is what time zone you are in :)

blitzerr (Oct 29 2018 at 17:33, on Zulip):

Seattle so PST :slight_smile:

nikomatsakis (Oct 29 2018 at 17:33, on Zulip):

Ah, ok

blitzerr (Oct 29 2018 at 17:33, on Zulip):

I know you are Boston

nikomatsakis (Oct 29 2018 at 17:33, on Zulip):

let me check my calendar and send you a privmsg I guess

blitzerr (Oct 29 2018 at 17:33, on Zulip):

sure

nikomatsakis (Oct 30 2018 at 17:46, on Zulip):

in case you are curious, @blitzerr and I did a "pair programming" session discussing how upvar analysis works for closures; you can watch it here

Jake Goulding (Oct 30 2018 at 18:45, on Zulip):

I want there to be an "upvar" joke equivalent to "updog"

blitzerr (Nov 02 2018 at 15:30, on Zulip):

@nikomatsakis
I posted a PR for rustc guide (just the various modes of closure and their MIR de-sugaring) here
https://github.com/rust-lang-nursery/rustc-guide/pull/225

The issue is all the de-sugaring looks the same. Everywhere the variable is moved. Should I be looking at a different file ?

blitzerr (Nov 02 2018 at 15:31, on Zulip):

I just put up the PR so that I can start a discussion. I will make it more sane as we go along. :slight_smile:

blitzerr (Nov 02 2018 at 20:53, on Zulip):
1.
fn main() {
  let x = 10;
  foo(|| println!("Hi {}", x))
}
_3 = [closure@test.rs:9:9: 11:6] { x: move _4 };

2.
fn main() {
  let x = 10;
  foo(|| {
    x += 1;
    println!("Hi {}", x)
  })
}
_3 = [closure@test.rs:9:9: 11:6] { x: move _4 }; // bb0[6]: scope 1 at test.rs:9:9: 11:6

In both the cases, I see that the MIR is exactly the same and the variable is being moved. I was thinking that in first case it should be an immutable borrow (or whatever the MIR version of it), in the second case it should be a mutable borrow and then in the 3rd case (not shown here but the case of where X is a Vec and the closure calls drop(x), there it should be a move).

blitzerr (Nov 02 2018 at 20:53, on Zulip):

@nikomatsakis is my understanding incorrect ?

blitzerr (Nov 07 2018 at 04:36, on Zulip):

$ rust-lldb build/x86_64-apple-darwin/stage1/bin/rustc (lldb) command script import "/Users/blitz/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/etc/lldb_rust_formatters.py" (lldb) type summary add --no-value --python-function lldb_rust_formatters.print_val -x ".*" --category Rust (lldb) type category enable Rust (lldb) target create "build/x86_64-apple-darwin/stage1/bin/rustc" Current executable set to 'build/x86_64-apple-darwin/stage1/bin/rustc' (x86_64). (lldb) b upvar.rs:107 Breakpoint 1: where = librustc_typeck-343623f2c64cba7c.dylibrustc_typeck::check::upvar::_$LT$impl$u20$rustc_typeck..check..FnCtxt$LT$$u27$a$C$$u20$$u27$gcx$C$$u20$$u27$tcx$GT$$GT$::analyze_closure::h65ebe12563fafb96 + 225 at upvar.rs:107, address = 0x00000000000f65e1
(lldb) r ../../progs/mut/mut.rs
Process 14184 launched: '/Users/blitz/rustc-dev/rust/build/x86_64-apple-darwin/stage1/bin/rustc' (x86_64)
Process 14184 stopped

blitzerr (Nov 09 2018 at 18:31, on Zulip):

Hi @nikomatsakis

nikomatsakis (Nov 12 2018 at 17:51, on Zulip):

@blitzerr :wave:

blitzerr (Nov 12 2018 at 18:26, on Zulip):

@nikomatsakis Welcome back. I read your blog post. Looks like you had a very productive time

blitzerr (Nov 12 2018 at 18:27, on Zulip):

I posted a PR but that's just the first-cut, RustFmt on upvar.rs, before I modify the code as you prefer.

blitzerr (Nov 12 2018 at 18:35, on Zulip):
pub struct UpvarId {
     pub var_id: hir::HirId,
     pub closure_expr_id: LocalDefId,
}

I was thinking that instead of wrapping the UpvarId inside another struct, I was thinking how about we wrap pub var_id: hir::HirId inside another called VarPath or something?

blitzerr (Nov 13 2018 at 21:59, on Zulip):

:wave: @nikomatsakis
Any inputs ?

nikomatsakis (Nov 13 2018 at 22:00, on Zulip):

well, I R+'d your Pr, I think?

nikomatsakis (Nov 13 2018 at 22:00, on Zulip):

but on to the next question

nikomatsakis (Nov 13 2018 at 22:00, on Zulip):

that sounds reasonable

nikomatsakis (Nov 13 2018 at 22:00, on Zulip):

how about we wrap pub var_id: hir::HirId inside another called VarPath or something

that in particular

blitzerr (Nov 15 2018 at 05:21, on Zulip):

Another pull: https://github.com/rust-lang/rust/pull/55953

blitzerr (Dec 04 2018 at 13:34, on Zulip):

Hi @nikomatsakis . This is the topic thread.

nikomatsakis (Dec 06 2018 at 13:57, on Zulip):

ok so picking this up again @blitzerr :)

nikomatsakis (Dec 06 2018 at 13:58, on Zulip):

I don't recall, you did land some kind of refactoring PR, right?

blitzerr (Dec 06 2018 at 13:58, on Zulip):

:tada:

blitzerr (Dec 06 2018 at 13:59, on Zulip):

https://github.com/rust-lang/rust/pull/55953/files

blitzerr (Dec 06 2018 at 14:00, on Zulip):

So basically separating out hir_id into a struct of it's own

nikomatsakis (Dec 06 2018 at 14:01, on Zulip):

right ok

nikomatsakis (Dec 06 2018 at 14:01, on Zulip):

so

nikomatsakis (Dec 06 2018 at 14:01, on Zulip):

here is a question

nikomatsakis (Dec 06 2018 at 14:01, on Zulip):

we should try to identify some cases that are particularly easy basically

nikomatsakis (Dec 06 2018 at 14:01, on Zulip):

so we can try to tackle those first

nikomatsakis (Dec 06 2018 at 14:02, on Zulip):

really @blitzerr this job is not particularly hard

nikomatsakis (Dec 06 2018 at 14:02, on Zulip):

conceptually :P

nikomatsakis (Dec 06 2018 at 14:02, on Zulip):

(famous last words)

blitzerr (Dec 06 2018 at 14:02, on Zulip):

Right.
I remember you saying we start with tuples first

nikomatsakis (Dec 06 2018 at 14:02, on Zulip):

maybe a good place to start is trying to just identify the paths we would capture (but not actually capture them)

nikomatsakis (Dec 06 2018 at 14:02, on Zulip):

ah, I was smart

nikomatsakis (Dec 06 2018 at 14:03, on Zulip):

but I think trying to identify the paths is good

nikomatsakis (Dec 06 2018 at 14:03, on Zulip):

also

nikomatsakis (Dec 06 2018 at 14:03, on Zulip):

one technique we sometimes use for this is to add some kind of special, perm-unstable attribute like #[rustc_dump_closure_paths]

nikomatsakis (Dec 06 2018 at 14:03, on Zulip):

which then causes the tests to emit special errors at each closure containing interesting information

nikomatsakis (Dec 06 2018 at 14:04, on Zulip):

I was thinking we could add such an attribute and then make some tests like

#[rustc_dump_closure_paths]
fn main() {
    let x = (22, 44);
    let y = || x.0;
    println!("{}", y());
}

this would presently emit an error like "capturing x" on the closure

nikomatsakis (Dec 06 2018 at 14:04, on Zulip):

then the job is to write the code that will look at the HIR and try to decide that x.0 would be a better path to capture :P

nikomatsakis (Dec 06 2018 at 14:04, on Zulip):

(sorry, just thinking out loud)

blitzerr (Dec 06 2018 at 14:05, on Zulip):

Makes sense.

nikomatsakis (Dec 06 2018 at 14:05, on Zulip):

there is another place that might be a better starting point

blitzerr (Dec 06 2018 at 14:05, on Zulip):

I will try that out

nikomatsakis (Dec 06 2018 at 14:05, on Zulip):

right now there is a fair amount of code that is using tcx.with_freevars

nikomatsakis (Dec 06 2018 at 14:05, on Zulip):

this function is problematic

blitzerr (Dec 06 2018 at 14:07, on Zulip):

Okay. So we need to change that as well

nikomatsakis (Dec 06 2018 at 14:07, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/ty/struct.TyCtxt.html#method.with_freevars

nikomatsakis (Dec 06 2018 at 14:08, on Zulip):

the reason it's problematic is that

nikomatsakis (Dec 06 2018 at 14:11, on Zulip):

(sorry, slicing up a few conversations)

nikomatsakis (Dec 06 2018 at 14:11, on Zulip):

well basically that it returns a slice of variables

blitzerr (Dec 06 2018 at 14:11, on Zulip):

You mean because it takes the NodeId ?

nikomatsakis (Dec 06 2018 at 14:11, on Zulip):

that is also ungreat ;)

nikomatsakis (Dec 06 2018 at 14:11, on Zulip):

but basically all code that uses it is implicitly assuming that closures capture entire variables

nikomatsakis (Dec 06 2018 at 14:11, on Zulip):

and not paths

nikomatsakis (Dec 06 2018 at 14:12, on Zulip):

it's not that much code that uses it:

/home/nmatsakis/.cargo/bin/rg --no-heading --color never '\.with_freevars\('
src/librustc_passes/rvalue_promotion.rs:474:            if v.tcx.with_freevars(e.id, |fv| !fv.is_empty()) {
src/librustc_typeck/collect.rs:1102:        tcx.with_freevars(node_id, |fv| {
src/librustc_mir/build/mod.rs:609:    let upvar_decls: Vec<_> = tcx.with_freevars(fn_id, |freevars| {
src/librustc_mir/borrow_check/error_reporting.rs:1651:                        .with_freevars(node_id, |fv| fv[field.index()]);
src/librustc_mir/borrow_check/error_reporting.rs:2423:            let var_span = self.infcx.tcx.with_freevars(
src/librustc/mir/mod.rs:2371:                            tcx.with_freevars(node_id, |freevars| {
src/librustc/mir/mod.rs:2389:                            tcx.with_freevars(node_id, |freevars| {
src/librustc/util/ppaux.rs:1221:                        tcx.with_freevars(node_id, |freevars| {
src/librustc/util/ppaux.rs:1261:                        tcx.with_freevars(node_id, |freevars| {
src/librustc/middle/liveness.rs:475:        ir.tcx.with_freevars(expr.id, |freevars| {
src/librustc/middle/expr_use_visitor.rs:936:        self.tcx().with_freevars(closure_expr.id, |freevars| {
src/librustc_typeck/check/upvar.rs:134:        self.tcx.with_freevars(closure_node_id, |freevars| {
src/librustc_typeck/check/upvar.rs:244:        tcx.with_freevars(closure_id, |freevars| {
src/librustc_typeck/check/coercion.rs:701:            ty::FnPtr(_) if self.tcx.with_freevars(node_id_a, |v| v.is_empty()) => {
src/librustc_mir/hair/cx/expr.rs:530:            let upvars = cx.tcx.with_freevars(expr.id, |freevars| {
nikomatsakis (Dec 06 2018 at 14:12, on Zulip):

but e.g. all the code in librustc_mir needs to be generalized, and I'm sure most of the rest

nikomatsakis (Dec 06 2018 at 14:12, on Zulip):

it would be kind of good to look at those uses and try to write out what they are doing I guess

nikomatsakis (Dec 06 2018 at 14:13, on Zulip):

@blitzerr I wonder if the thing for us to do is to schedule another hour-long slot next week or something

nikomatsakis (Dec 06 2018 at 14:13, on Zulip):

I'm probably not going to have enough time at this second

nikomatsakis (Dec 06 2018 at 14:13, on Zulip):

but clearly we need to carve out a bit of time

blitzerr (Dec 06 2018 at 14:15, on Zulip):

Sure.
We can have another call. Meanwhile I will try to figure out what the other callers use it for

blitzerr (Dec 06 2018 at 14:15, on Zulip):

Thanks for looking into this on a busy all :hand: week :slight_smile:

blitzerr (Dec 06 2018 at 14:25, on Zulip):

Is this all that uses the entire upvar node or expects it? We have to get an exhaustive list. I wonder how.

nikomatsakis (Dec 06 2018 at 14:33, on Zulip):

I think this is a pretty exhaustive list

nikomatsakis (Dec 06 2018 at 14:33, on Zulip):

well

nikomatsakis (Dec 06 2018 at 14:34, on Zulip):

the other place you might check would be things that invoke upvar_tys. I suspect that most of them are also calling freevars in the same vicinity, or else don't really need to be changed, but still.

blitzerr (Dec 11 2018 at 22:34, on Zulip):

@nikomatsakis Let me know when you will have time for a call/ discussion

blitzerr (Dec 12 2018 at 21:21, on Zulip):

@nikomatsakis :wave:

nikomatsakis (Dec 12 2018 at 21:40, on Zulip):

ok @blitzerr so -- I have a few minutes before my next meeting but not a ton

nikomatsakis (Dec 12 2018 at 21:40, on Zulip):

I definitely still think some refactoring around this area is good

nikomatsakis (Dec 12 2018 at 21:41, on Zulip):

it seems like we're not quite at the point of knowing what to do

nikomatsakis (Dec 12 2018 at 21:41, on Zulip):

let me see if in the next 10 minutes or so I can sketch out a plan

nikomatsakis (Dec 12 2018 at 21:48, on Zulip):

digging around I am starting to get some idea of the range of problems we'll have :P

nikomatsakis (Dec 12 2018 at 21:48, on Zulip):

but I guess a starting point would be to identify some of the places we use freevars and perhaps convert them to use data withdrawn from TypeckTables instead

nikomatsakis (Dec 12 2018 at 21:49, on Zulip):

let's start with exactly one example

nikomatsakis (Dec 12 2018 at 21:49, on Zulip):

this code during MIR construction iterates over the freevariables using with_freevars

nikomatsakis (Dec 12 2018 at 21:50, on Zulip):

for each free variable, it looks up the borrow mode using the upvar_capture method here:

            let var_id = fv.var_id();
            let var_hir_id = tcx.hir().node_to_hir_id(var_id);
            let closure_expr_id = tcx.hir().local_def_id(fn_id);
            let capture = hir.tables().upvar_capture(ty::UpvarId {
                var_path: ty::UpvarPath {hir_id: var_hir_id},
                closure_expr_id: LocalDefId::from_def_id(closure_expr_id),
});
nikomatsakis (Dec 12 2018 at 21:50, on Zulip):

it'd be nice if we didn't use with_freevars but instead were able to get the list of free var capture into from hir.tables()

nikomatsakis (Dec 12 2018 at 21:51, on Zulip):

this might require however adding another field to TypeckTables, something like

pub upvar_list: ty::UpvarListMap<'tcx>,

where

pub type UpvarListMap<'tcx> = FxHashMap<DefId, Vec<UpvarId>>
nikomatsakis (Dec 12 2018 at 21:54, on Zulip):

so maybe a good first step is just adding this field to TypeckTables, and populating it. I imagine we would populate it right around this code. Basically in this loop we could be pushing the UpvarId values we create into a vector, so that we can store that vector into the map we added to the tables, sort of like this -- but after the loop, once the vector is complete.

nikomatsakis (Dec 12 2018 at 21:56, on Zulip):

ok, @blitzerr, that's all I have time for for now -- what we should probably do is open an issue and I can move these notes to there. The issue would be something like "refactor uses of freevar" I guess. I can try to file one later.

blitzerr (Dec 12 2018 at 21:58, on Zulip):

@nikomatsakis Thanks a lot. I will create an issue and add your notes and then you can add more to that. Thanks a lot for your time

nikomatsakis (Dec 12 2018 at 22:26, on Zulip):

I hope @blitzerr some of that made sense =)

blitzerr (Dec 12 2018 at 22:40, on Zulip):

@nikomatsakis what you said makes sense in terms of the steps but I don't get why it's bad for the current code to loop over with_freevars ? The only thing I can think of is having it in hir.tables() is cleaner ?

blitzerr (Dec 13 2018 at 04:20, on Zulip):

@nikomatsakis
https://github.com/rust-lang/rust/issues/53553
Is this similar to what you were thinking ? Can we use this issue or we would like to create our own ?

nikomatsakis (Dec 13 2018 at 15:25, on Zulip):

@nikomatsakis what you said makes sense in terms of the steps but I don't get why it's bad for the current code to loop over with_freevars ? The only thing I can think of is having it in hir.tables() is cleaner ?

well, in the future, we expect closures to capture not only free variables, but entire paths. Most code using freevars will not be compatible with that assumption.

nikomatsakis (Dec 13 2018 at 15:26, on Zulip):

Is this similar to what you were thinking ? Can we use this issue or we would like to create our own ?

Similar, yes, though different. Seems like a good place to discuss, at leaset.

blitzerr (Dec 13 2018 at 16:49, on Zulip):

@nikomatsakis okay, I will add your words to the issue #53553.

nikomatsakis (Dec 13 2018 at 16:51, on Zulip):

This is actually a sort of complication we have to work through — the current setup relies on knowing the types of all captured variables in the closure before type-check runs. I have to go revisit the RFC and see what we discussed there in this regard.

blitzerr (Dec 13 2018 at 16:55, on Zulip):

You mean in working through #53488 we will also solve #53553 ?

blitzerr (Dec 17 2018 at 01:00, on Zulip):

@nikomatsakis Created a new issue : https://github.com/rust-lang/rust/issues/56905.

blitzerr (Dec 17 2018 at 01:10, on Zulip):

https://github.com/rust-lang/rust/pull/56906/files

blitzerr (Dec 17 2018 at 17:20, on Zulip):

@nikomatsakis
:wait_one_second:

nikomatsakis (Dec 17 2018 at 17:22, on Zulip):

Saw it, great!

nikomatsakis (Dec 17 2018 at 17:22, on Zulip):

I'll try to add some more notes, not sure if I can get to that today, but I've added it to my list

blitzerr (Dec 18 2018 at 03:48, on Zulip):

Thanks a lot @nikomatsakis for reviewing. I have a question related to your comment :D

nikomatsakis (Dec 18 2018 at 15:23, on Zulip):

@blitzerr responded

blitzerr (Dec 18 2018 at 21:59, on Zulip):

Thanks a lot @nikomatsakis

blitzerr (Dec 18 2018 at 21:59, on Zulip):

I will post an update tonight.

nikomatsakis (Dec 18 2018 at 22:00, on Zulip):

great, I will try to post some further comments about next steps

blitzerr (Dec 18 2018 at 22:01, on Zulip):

@nikomatsakis When you have a chance if you can look at https://github.com/rust-lang/rustc-guide/pull/225, it would be great or else we can push it after we wrap up the #53488 as well. Whatever works.

blitzerr (Dec 18 2018 at 22:01, on Zulip):

great, I will try to post some further comments about next steps

That will be awesome.

blitzerr (Dec 18 2018 at 22:15, on Zulip):

@nikomatsakis If we are thinking the same thing, the next step will be to replace the calls to with_freevars with the map we introduced ?

nikomatsakis (Dec 18 2018 at 22:25, on Zulip):

@blitzerr right -- but not all of them. I think I highlighted one good candidate to start.

nikomatsakis (Dec 18 2018 at 22:26, on Zulip):

there are some bigger questions we need to think through though that I've not had time to ponder

blitzerr (Dec 18 2018 at 22:27, on Zulip):

@nikomatsakis Maybe if you tell me what the big questions are, I can think of something and then you can tell me I am wrong :smiley:

nikomatsakis (Dec 18 2018 at 22:29, on Zulip):

specifically, the problem has to do with the way we handle "closure substs"

nikomatsakis (Dec 18 2018 at 22:29, on Zulip):

the current scheme is explained here, in the comments

nikomatsakis (Dec 18 2018 at 22:30, on Zulip):

the problem is that this scheme relies on knowing the set of captured variables

nikomatsakis (Dec 18 2018 at 22:30, on Zulip):

and it creates one generic parameter for each of them

nikomatsakis (Dec 18 2018 at 22:30, on Zulip):

but if we are going to be capturing paths, and we don't know those paths until type check...

nikomatsakis (Dec 18 2018 at 22:31, on Zulip):

we might need to adjust that scheme

nikomatsakis (Dec 18 2018 at 22:31, on Zulip):

since I think we have to know the number of parameters etc before typeck completes

nikomatsakis (Dec 18 2018 at 22:31, on Zulip):

but maybe not

nikomatsakis (Dec 18 2018 at 22:31, on Zulip):

or maybe we can figure out the set of captured paths syntactically without needing to know typeck results; but I doubt it

blitzerr (Dec 18 2018 at 23:46, on Zulip):

@nikomatsakis

or maybe we can figure out the set of captured paths syntactically without needing to know typeck results; but I doubt it

Sounds like doing the same thing twice.
But its good to run into problems. I will learn more. :crab:

blitzerr (Dec 19 2018 at 04:57, on Zulip):

@nikomatsakis
https://github.com/rust-lang/rust/pull/56906/commits/d7519544694c1866ba4bf32cd307ed6f670e1267

blitzerr (Dec 21 2018 at 04:08, on Zulip):

@nikomatsakis Maybe I made a mistake here. I should have used closure_node_id instead of closure_def_id as the key to the uppvar_list.
I say that because, there is a step to get to closure_def_id from closure_hir_id
let (closure_def_id, substs) = match self.node_ty(closure_hir_id).sty {

This provides ast::NodeId and therefore using closure_node_id as the key to upvar_list we can get the list of upvar directly or else we have to use
tcx.hir().local_def_id(fn_id) to get closure_def_id and then use that to arrive at the list of upvars. Do you agree ?

blitzerr (Dec 22 2018 at 23:14, on Zulip):

@nikomatsakis
This is my first attempt at using the new data structure we created in the mir creation of a function. I have a question for you, to get unblocked. My question is in the PR as a comment

blitzerr (Dec 22 2018 at 23:16, on Zulip):

Also, I think I messed up by commit/git somehow and now I see this being posted as part of the last PR and not as a new PR as I would have hoped. Can someone please educate me on how to solve this ?

nikomatsakis (Dec 26 2018 at 18:10, on Zulip):

@blitzerr you should definitely not use the closure node-id

nikomatsakis (Dec 26 2018 at 18:10, on Zulip):

node-ids are kind of deprecated

nikomatsakis (Dec 26 2018 at 18:11, on Zulip):

I'll take a look at your comment though

blitzerr (Dec 26 2018 at 18:22, on Zulip):

@nikomatsakis
Thanks a lot.

nikomatsakis (Dec 26 2018 at 18:22, on Zulip):

@blitzerr is this the comment you were referring to?

blitzerr (Dec 26 2018 at 18:24, on Zulip):

Yup that's it @nikomatsakis

nikomatsakis (Dec 26 2018 at 18:25, on Zulip):

ok gimme a sec but I am confused -- which NodeId are you referring to, in that code, that is causing you problems?

nikomatsakis (Dec 26 2018 at 18:26, on Zulip):

is it fn_id?

nikomatsakis (Dec 26 2018 at 18:28, on Zulip):

oh, I guess you mean this part:

var_id is needed because further down, in this
             });
         // closure, it is used to find the Node::Binding to update the UpvarDecl, which will be
             let by_ref = match capture {
         // eventually returned by the closure. So the other way would be, if we can get the
                 ty::UpvarCapture::ByValue => false,
         // Node::Binding, given an HirId, we can also solve this.
blitzerr (Dec 26 2018 at 18:42, on Zulip):

Right, that's where I am stuck @nikomatsakis

blitzerr (Dec 26 2018 at 21:32, on Zulip):

@nikomatsakis That is one issue and the other one is what should upvar_list's key be ?
Whereever we use with_freevars, I see the code snippets use ast::NodeId and therefore I was suggesting using NodeId as the key but if it is deprecated, then maybe we should use BodyId or DefId I think. The last PR used DefId as the key, do you think that's better that the BodyId ?

blitzerr (Dec 26 2018 at 21:35, on Zulip):

This brings us the second issue, no matter what we end up with, we will need NodeId to work with as that is what the hir().find() expects here
if let Some(Node::Binding(pat)) = tcx.hir().find(var_id)

nikomatsakis (Dec 27 2018 at 12:43, on Zulip):

@blitzerr ok I'll leave some tips today, sorry, got distracted yesterday

nikomatsakis (Dec 27 2018 at 13:54, on Zulip):

@blitzerr see this comment -- tl;dr you can freely interconvert between node-id and hir-id

nikomatsakis (Dec 27 2018 at 13:54, on Zulip):

I was at first going to propose a more extensive refactoring, but it seems fine for now

blitzerr (Dec 27 2018 at 15:51, on Zulip):

Ohh man! I missed that utility function ! Embarrassing

blitzerr (Dec 27 2018 at 15:51, on Zulip):

@nikomatsakis
Regarding the second question, what do you think we should use as the key in our upvar_list ?

nikomatsakis (Dec 27 2018 at 15:59, on Zulip):

@blitzerr I would prefer DefId

nikomatsakis (Dec 27 2018 at 15:59, on Zulip):

I think we use DefId most places

nikomatsakis (Dec 27 2018 at 15:59, on Zulip):

also, a BodyId doesn't work across crates

blitzerr (Dec 27 2018 at 16:04, on Zulip):

Sounds good !

blitzerr (Dec 27 2018 at 16:05, on Zulip):

@nikomatsakis
What is the way you start another PR on your fork?
If you see this PR got started on a previous PR that was already merged. How do I clean this up or just add on top of this one ?

nikomatsakis (Dec 27 2018 at 16:07, on Zulip):

@blitzerr well if that PR was already merged, then just create a new branch

nikomatsakis (Dec 27 2018 at 16:07, on Zulip):

e.g., git checkout -b my-new-branch

nikomatsakis (Dec 27 2018 at 16:07, on Zulip):

and push the new branch to your repository

nikomatsakis (Dec 27 2018 at 16:07, on Zulip):

then do a rebase atop rust-lang/master

nikomatsakis (Dec 27 2018 at 16:08, on Zulip):

leaving just the new commits

blitzerr (Dec 27 2018 at 17:56, on Zulip):

thanks a lot @nikomatsakis

blitzerr (Dec 28 2018 at 15:15, on Zulip):

@nikomatsakis
I see there are ways to get a local defId from NodeId but is there a way to get global DefId from the NodeId ?

nikomatsakis (Dec 28 2018 at 15:16, on Zulip):

@blitzerr not all node-ids have a def-id, but yes you can use opt_local_def_id

nikomatsakis (Dec 28 2018 at 15:17, on Zulip):

docs

nikomatsakis (Dec 28 2018 at 15:17, on Zulip):

well, not much docs there :)

blitzerr (Dec 28 2018 at 15:17, on Zulip):

But the upvar_list is keyed on global DefId

nikomatsakis (Dec 28 2018 at 15:18, on Zulip):

I'm not sure what you mean

nikomatsakis (Dec 28 2018 at 15:18, on Zulip):

a DefId is a DefId

nikomatsakis (Dec 28 2018 at 15:18, on Zulip):

ah, I guess it's the name "local def-id" that is confusing?

nikomatsakis (Dec 28 2018 at 15:19, on Zulip):

in this context, that just means: a DefId where the crate field is LOCAL_CRATE

nikomatsakis (Dec 28 2018 at 15:19, on Zulip):

but once you have a DefId, it is canonical and can always be compared against other DefId values

blitzerr (Dec 28 2018 at 16:20, on Zulip):

Ahh! Thanks for clarifying @nikomatsakis
I was thinking there are two flavors one local to crate and one global. You got it, the name local_def_id was what lead me there :grinning:

blitzerr (Dec 29 2018 at 19:55, on Zulip):

PR

blitzerr (Dec 29 2018 at 19:57, on Zulip):

This leads to ice because for some reason, the upvar_List when checked inside librustc_mir is empty.

Matthew Jasper (Dec 29 2018 at 20:00, on Zulip):

You might have to modify src/librustc_typeck/check/writeback.rs to handle upvar lists as well.

Matthew Jasper (Dec 29 2018 at 20:03, on Zulip):

The only parts of TypeckTables that get persisted are the parts stored there.

blitzerr (Dec 29 2018 at 20:05, on Zulip):

Thank you @Matthew Jasper
I didn't know about that.

blitzerr (Dec 29 2018 at 20:07, on Zulip):

Is that a specific function I need to add this new list to ?

blitzerr (Dec 29 2018 at 20:08, on Zulip):

@Matthew Jasper
can you give a brief description about how the writeback struct and friends works ?

Matthew Jasper (Dec 29 2018 at 20:12, on Zulip):

OK, so typeck_tables_of takes the def id of an item that is local to the current crate, and has a body (a function, constant or static), type-infers/type-checks the body, and returns the TypeckTables for that item, which contains all of the inferred information.

blitzerr (Dec 29 2018 at 20:16, on Zulip):

Cool. Thank you so much.

Matthew Jasper (Dec 29 2018 at 20:18, on Zulip):

During type inference, partially inferred types are represented using Type variables (ty::Infer). These don't appear in the final TypeckTables since all of the types should have been inferred once typeck_tables_of is done.

Matthew Jasper (Dec 29 2018 at 20:21, on Zulip):

When type inference is running however, having to update the typeck tables every time a new type is inferred would be unreasonably slow, so instead all of the replacement happens at the end in resolve_type_vars_in_body, which creates a new TypeTables which doesn't contain any inference types.

Matthew Jasper (Dec 29 2018 at 20:23, on Zulip):

As UpvarListMap doesn't contain any types, you should be able to copy it to the new table in resolve_type_vars_in_body. As an aside UpvarListMap doesn't appear to use it's lifetime parameter.

blitzerr (Dec 30 2018 at 05:03, on Zulip):

Sorry @Matthew Jasper I didn't see the last few messages. Thanks a lot for the detailed explanation. Very helpful.
Regarding lifetime parameter, do you think I should remove it then ?

Matthew Jasper (Dec 30 2018 at 10:42, on Zulip):

Yes, unless you're going to add a use of it later.

nikomatsakis (Jan 02 2019 at 21:04, on Zulip):

@blitzerr is the PR updated now?

nikomatsakis (Jan 02 2019 at 21:06, on Zulip):

looks like no

blitzerr (Jan 02 2019 at 21:06, on Zulip):

@nikomatsakis
Not with the changes to copy the tables inside typeck_tables_of

nikomatsakis (Jan 02 2019 at 21:06, on Zulip):

ok, should be a small diff

blitzerr (Jan 02 2019 at 21:07, on Zulip):

@nikomatsakis ya and I can check if that makes the panic go away.

blitzerr (Jan 02 2019 at 21:10, on Zulip):

@nikomatsakis Also, as an aside, we can add eprintln! in the rustc-guide as a way of debugging rustc. I found it very helpful

nikomatsakis (Jan 02 2019 at 21:12, on Zulip):

we...could...I never use it myself

nikomatsakis (Jan 02 2019 at 21:12, on Zulip):

I usually just add debug!

nikomatsakis (Jan 02 2019 at 21:12, on Zulip):

in fact, I would not recommend eprintln!

nikomatsakis (Jan 02 2019 at 21:13, on Zulip):

because it prints out when you are bootstrapping

nikomatsakis (Jan 02 2019 at 21:13, on Zulip):

I find adding debug! or info! is better (the latter can be useful to distinguish the things you've just added from other debug! calls already in the compiler)

nikomatsakis (Jan 02 2019 at 21:13, on Zulip):

but either way we should have some "debugging tips", I know we've talked about adding that before

nikomatsakis (Jan 02 2019 at 21:16, on Zulip):

(and in said section we could mention eprintln! as an easy way, though it has downsides)

blitzerr (Jan 02 2019 at 21:17, on Zulip):

The debug statements are printed when you use the rustc to compile a new file
RUST_LOG=rustc::traits rustc +local my-file.rs
Like here we are using the already built rustc to compile my-file.rs and generating the debug logs. In my case, the compiler would panic, while being built (./x.py build ...). In that case debug won't help right ?

nikomatsakis (Jan 02 2019 at 21:28, on Zulip):

ah, that is true, though you can use RUST_LOG when running x.py

nikomatsakis (Jan 02 2019 at 21:29, on Zulip):

in general tips for dealing with crashes during bootstrapping is a good topic

nikomatsakis (Jan 02 2019 at 21:31, on Zulip):

but you are definitely correct that eprintln! may be particularly useful in that scenario

centril (Jan 02 2019 at 21:32, on Zulip):

@nikomatsakis FYI: there's dbg! now, https://doc.rust-lang.org/nightly/std/macro.dbg.html

nikomatsakis (Jan 02 2019 at 22:29, on Zulip):

@blitzerr I left a few notes (link) regarding some of the steps we will have to do in this closure refactoring. Those weren't more "internal notes to myself", but we should discuss, as I think it suggests a few more refactorings to try out =)

nikomatsakis (Jan 02 2019 at 22:29, on Zulip):

the good news is that I think the path is starting to become clear

nikomatsakis (Jan 02 2019 at 22:29, on Zulip):

and you will be the world's expert on closures by the time we're done :P

blitzerr (Jan 02 2019 at 22:36, on Zulip):

ah, that is true, though you can use RUST_LOG when running x.py

Can you tell me how and I can add that to rustc-guide as well

blitzerr (Jan 02 2019 at 22:53, on Zulip):

@nikomatsakis

blitzerr (Jan 02 2019 at 23:01, on Zulip):

Thanks for the notes ! I will read them tonight

csmoe (Jan 02 2019 at 23:06, on Zulip):

@blitzerr debugging chapter from https://rust-lang.github.io/rustc-guide/compiler-debugging.html

blitzerr (Jan 02 2019 at 23:12, on Zulip):

@csmoe
I see lines like this $ RUST_LOG=rustc::traits rustc +local my-file.rs
which is invoking it after the compiler is built and not while building it using x.py

blitzerr (Jan 02 2019 at 23:13, on Zulip):

@csmoe Let me know if you are seeing something that I missed ?

csmoe (Jan 02 2019 at 23:14, on Zulip):

oops, rust_log only works with built rustc.

blitzerr (Jan 02 2019 at 23:16, on Zulip):

Right but looks like @nikomatsakis has a trick to make it work while it is being built

blitzerr (Jan 03 2019 at 04:09, on Zulip):

@nikomatsakis New commit

nikomatsakis (Jan 03 2019 at 16:08, on Zulip):

@blitzerr you just need to do RUST_LOG=rustc::traits ./x.py ...

blitzerr (Jan 03 2019 at 16:54, on Zulip):

Thanks

blitzerr (Jan 03 2019 at 17:13, on Zulip):

@nikomatsakis
I think, one way to remove the usage of Freevars struct will be for the with_freevars function to be able to call the closure provided with the upvar_list and then all callers code can change to accommodate to use the UpvarId instead of Freevar. But that said, it will have to be an all at once change instead of the way we are currently doing it, tackling one with_freevar call at a time.

blitzerr (Jan 03 2019 at 17:19, on Zulip):

Not sure if you reviewed the code, but looking at your comments on the issue, I think we have a direction for the next change. There are two methods you suggest, have you made your mind on which one ? Maybe we want to do the tuples based one ? When you suggest that the closure can keep a list of all the upvars it is referencing, we don't really need to right ? I think we have to extend the UpvarId struct to keep track of which fields in aggregate datatypes are borrowed by the closure ?

blitzerr (Jan 03 2019 at 22:10, on Zulip):

@nikomatsakis :point_up:

nikomatsakis (Jan 04 2019 at 19:05, on Zulip):

I would prefer @blitzerr to migrate callers of the with_freevars code one by one, since then it can be done incrementally — but also because there may be some callers that should not be migrated, I'm not sure

nikomatsakis (Jan 04 2019 at 19:05, on Zulip):

I think the next refactoring I would pursue though is indeed to introduce a tuple of type variables for the upvar types

nikomatsakis (Jan 04 2019 at 19:06, on Zulip):

I wonder @blitzerr if you want to try to do that one -- or get started on it, anyways -- doing a pair programming session

nikomatsakis (Jan 04 2019 at 19:06, on Zulip):

I feel like it looks "not that hard" to me, but I could imagine if you are not that familiar with the code it would be hard to know what to do

blitzerr (Jan 04 2019 at 19:31, on Zulip):

@nikomatsakis
Sure we can pick up the tuple introduction part. A pair programming sounds like a great idea. I want to do that ever since I read this

nikomatsakis (Jan 04 2019 at 19:53, on Zulip):

@blitzerr then we need to schedule a slot :)

blitzerr (Jan 04 2019 at 19:55, on Zulip):

@nikomatsakis Awesome ! How does your schedule look like next week ?

blitzerr (Jan 07 2019 at 17:38, on Zulip):

@nikomatsakis New commit. Any chance you can take a look at that ?

blitzerr (Jan 07 2019 at 17:39, on Zulip):

It's the same one from last week, no changes

nikomatsakis (Jan 07 2019 at 18:49, on Zulip):

from a brief glance it looks good to me :)

nikomatsakis (Jan 08 2019 at 19:15, on Zulip):

@blitzerr posted a review

blitzerr (Jan 08 2019 at 19:25, on Zulip):

Thank you for your time @nikomatsakis . I will submit an update for this one tonight.

blitzerr (Jan 09 2019 at 03:26, on Zulip):

@nikomatsakis update

blitzerr (Jan 09 2019 at 03:26, on Zulip):

Addressed your comments and posted another update

nikomatsakis (Jan 09 2019 at 14:55, on Zulip):

r+'d

blitzerr (Jan 10 2019 at 18:56, on Zulip):

@nikomatsakis Created New Issue for the work on making the "upvar types" a tuple of the actual upvar types work item

nikomatsakis (Jan 10 2019 at 19:24, on Zulip):

great

blitzerr (Jan 13 2019 at 17:48, on Zulip):

@nikomatsakis
Changes so far
What it addresses

What remains is change the lifetime parameters from tcx: TyCtxt<'_, '_, 'tcx> to tcx: TyCtxt<'_, 'tcx 'tcx>
I want to ask that in this case, the generator takes 'gcx lifetime and changing that to the 'tcx does not look good. Any ideas on how to do it or not do it at all ?

csmoe (Jan 14 2019 at 15:27, on Zulip):

@Santiago Pastorino okay :slight_smile:

blitzerr (Jan 14 2019 at 20:00, on Zulip):

@nikomatsakis :point_up:

nikomatsakis (Jan 15 2019 at 19:30, on Zulip):

@blitzerr nice! will review shortly

nikomatsakis (Jan 15 2019 at 21:00, on Zulip):

@blitzerr left various comments on the commits

blitzerr (Jan 15 2019 at 21:39, on Zulip):

Thanks a lot @nikomatsakis
Regarding naming I agree with the inconsistent part. What do you suggest, we adopt the clear naming (by this I mean, refactor the existing code to adopt the new name) or stick to the old convention ?

blitzerr (Jan 15 2019 at 21:40, on Zulip):

(deleted)

blitzerr (Jan 15 2019 at 21:41, on Zulip):

What remains is change the lifetime parameters from tcx: TyCtxt<'_, '_, 'tcx> to tcx: TyCtxt<'_, 'tcx 'tcx>
I want to ask that in this case, the generator takes 'gcx lifetime and changing that to the 'tcx does not look good. Any ideas on how to do it or not do it at all

blitzerr (Jan 15 2019 at 21:42, on Zulip):

Any ideas on that one :point_up:

blitzerr (Jan 16 2019 at 17:40, on Zulip):

@nikomatsakis
Addressed your comments here

nikomatsakis (Jan 16 2019 at 18:16, on Zulip):

@blitzerr comment -- the logic in ppaux still doesn't seem quite right to me

blitzerr (Jan 16 2019 at 18:27, on Zulip):

I added a comment and maybe we can iterate here more quickly?
"
Are you saying that we can be in a situation where the type is neither a type or an inference variable ?
In that case we will already panic as the code stands in src/librustc/ty/sty.rs(line 389 of this commit)

And this code will do that right? In case of an inference variable the opt_tuple_tys returns an empty list and so the some branch will do the usual running through an empty list ?
"

blitzerr (Jan 16 2019 at 18:27, on Zulip):

@nikomatsakis :point_of_information:

nikomatsakis (Jan 16 2019 at 18:28, on Zulip):

Are you saying that we can be in a situation where the type is neither a type or an inference variable ?

do you mean neither a tuple nor an inference variable? If so, I'm not saying that, it should always be one of those 2 things.

nikomatsakis (Jan 16 2019 at 18:28, on Zulip):

In case of an inference variable the opt_tuple_tys returns an empty list

ah, I missed that maybe

nikomatsakis (Jan 16 2019 at 18:29, on Zulip):

this is the definition of opt_tuple_tys that I see

 /// This is safe to be called even when the upvar type parameters are not
    /// bound and is exactly called for that same reason.
    pub fn opt_tuple_tys(
        self,
        def_id: DefId,
        tcx: TyCtxt<'_, '_, 'tcx>,
    ) -> Option<impl Iterator<Item=Ty<'tcx>> + 'tcx> {
        match self.upvar_tuple_ty_or_infer_var(def_id, tcx).sty {
            ty::Tuple(tys) => Some(tys.iter().cloned()),
            ref _t => None,
nikomatsakis (Jan 16 2019 at 18:29, on Zulip):

looks like it returns None, not an empty loop

nikomatsakis (Jan 16 2019 at 18:30, on Zulip):

which actually I think is the right thing

blitzerr (Jan 16 2019 at 18:32, on Zulip):
pub fn opt_tuple_tys(
        self,
        self,
        def_id: DefId,
        def_id: DefId,
        tcx: TyCtxt<'_, '_, 'tcx>,
        tcx: TyCtxt<'_, '_, 'tcx>,
    ) -> Option<impl Iterator<Item=Ty<'tcx>> + 'tcx> {
    ) -> Option<impl Iterator<Item=Ty<'tcx>> + 'tcx> {
        match self.upvar_tuple_ty_or_infer_var(def_id, tcx).sty {
        match self.upvar_tuple_ty(def_id, tcx).sty {
            ty::Tuple(tys) => Some(tys.iter().cloned()),
            ty::Tuple(tys) => Some(tys.iter().cloned()),
            ref _t => None,
            ty::Infer(_) => Some(ty::List::empty().iter().cloned()),
            _ => bug!("unexpected type")
        }
        }
    }
    }
blitzerr (Jan 16 2019 at 18:32, on Zulip):

This is what I see on the commit

blitzerr (Jan 16 2019 at 18:36, on Zulip):

here
and
here

blitzerr (Jan 16 2019 at 18:37, on Zulip):

@nikomatsakis :point_of_information:

nikomatsakis (Jan 16 2019 at 18:37, on Zulip):

ok

nikomatsakis (Jan 16 2019 at 18:37, on Zulip):

I personally thikn I would still return None for infer

nikomatsakis (Jan 16 2019 at 18:37, on Zulip):

otherwise, you can't tell the difference between "no upvars"

nikomatsakis (Jan 16 2019 at 18:37, on Zulip):

and "upvars not yet known"

nikomatsakis (Jan 16 2019 at 18:38, on Zulip):

but at the caller site, if you don't care about that difference (which we probably don't),

nikomatsakis (Jan 16 2019 at 18:38, on Zulip):

we can just do opt_tuple_tys().unwrap_or(&[]) or something

nikomatsakis (Jan 16 2019 at 18:38, on Zulip):

specifically I think I would return Option<&[Ty<'tcx>]>

blitzerr (Jan 16 2019 at 18:39, on Zulip):

The upvars tuple vs not inferred yet can be obtained from the list being filled vs empty.

blitzerr (Jan 16 2019 at 18:39, on Zulip):

But I do get your point.

nikomatsakis (Jan 16 2019 at 18:39, on Zulip):

I'm not sure how you can tell it is filled if the list is empty

nikomatsakis (Jan 16 2019 at 18:39, on Zulip):

also, why return Option if you never return None =)

blitzerr (Jan 16 2019 at 18:40, on Zulip):

That's true :slight_smile:

blitzerr (Jan 16 2019 at 18:40, on Zulip):

So you want it to not return an iterator anymore ?

blitzerr (Jan 16 2019 at 18:41, on Zulip):

@nikomatsakis Is that because it will save us from cloning ?

nikomatsakis (Jan 16 2019 at 18:42, on Zulip):

I would return a slice just because it's more convenient for everyone

blitzerr (Jan 16 2019 at 18:43, on Zulip):

I get your suggestions now. Sorry for the confusion. Unfortunately, I can only implement this tonight :frown:

blitzerr (Jan 16 2019 at 18:43, on Zulip):

I agree with you @nikomatsakis The syntax is much nicer :slight_smile:

blitzerr (Jan 16 2019 at 18:46, on Zulip):

@nikomatsakis While I have your attention, any thoughts on

What remains is change the lifetime parameters from tcx: TyCtxt&lt;'_, '_, 'tcx&gt; to tcx: TyCtxt&lt;'_, 'tcx 'tcx&gt;
I want to ask that in this case, the generator takes 'gcx lifetime and changing that to the 'tcx does not look good. Any ideas on how to do it or not do it at all ?
nikomatsakis (Jan 16 2019 at 19:02, on Zulip):

@blitzerr er that is hard to read

nikomatsakis (Jan 16 2019 at 19:02, on Zulip):

what's the question exactly?

nikomatsakis (Jan 16 2019 at 19:03, on Zulip):

maybe re-paste it without the "```"

blitzerr (Jan 17 2019 at 03:14, on Zulip):

@nikomatsakis
As we discussed

blitzerr (Jan 17 2019 at 04:46, on Zulip):
error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
   --> src/librustc/ty/sty.rs:549:9
    |
549 |         impl Iterator<Item=Ty<'tcx>> + 'a
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: hidden type `std::iter::Chain<impl std::iter::Iterator, std::iter::Once<&'gcx ty::TyS<'gcx>>>` captures the lifetime 'gcx as defined on the impl at 532:10
   --> src/librustc/ty/sty.rs:532:10
    |
532 | impl<'a, 'gcx, 'tcx> GeneratorSubsts<'tcx> {
    |          ^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0700`.
error: Could not compile `rustc`.

I get this error when I make this change

-    pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, '_, 'tcx>) ->
+    pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, 'tcx, 'tcx>) ->
blitzerr (Jan 17 2019 at 04:51, on Zulip):

This is what I meant. Sorry for the confusion and late response.

nikomatsakis (Jan 17 2019 at 13:54, on Zulip):

Yeah so that's one of the least favorite impl Trait limitations :)

nikomatsakis (Jan 17 2019 at 13:54, on Zulip):

there are workarounds but...before we go there...

nikomatsakis (Jan 17 2019 at 13:56, on Zulip):

maybe we can return a slice?

nikomatsakis (Jan 17 2019 at 13:57, on Zulip):

btw @blitzerr what branch are you working from on your fork?

nikomatsakis (Jan 17 2019 at 13:58, on Zulip):

upvar-tuple, I guess

nikomatsakis (Jan 17 2019 at 13:59, on Zulip):

anyway so yeah I suspect we can make this a slice now

nikomatsakis (Jan 17 2019 at 13:59, on Zulip):

I would just do that

nikomatsakis (Jan 17 2019 at 13:59, on Zulip):
pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, 'tcx, 'tcx>)  -> &[Ty<'tcx>]
blitzerr (Jan 17 2019 at 14:09, on Zulip):

@nikomatsakis
You guessed it right, upvar-tuple it is. :slight_smile:

blitzerr (Jan 17 2019 at 14:13, on Zulip):

We are returning a slice in the opt_tuple_ty function now ( latest change). We also want to return slices from upvar_tys ?

blitzerr (Jan 17 2019 at 14:13, on Zulip):

@nikomatsakis :wait_one_second:

nikomatsakis (Jan 17 2019 at 14:54, on Zulip):

@blitzerr right

blitzerr (Jan 17 2019 at 15:15, on Zulip):

@nikomatsakis maybe tonight I shall have it :slight_smile:

blitzerr (Jan 18 2019 at 12:30, on Zulip):

@nikomatsakis changing the return type of upvar_tys from impl Iterator<Item=Ty<'tcx>> + 'tcx to &'tcx[Ty<'tcx>], is it really required to fix the lifetime error? This change will be at quite a few places

blitzerr (Jan 18 2019 at 12:58, on Zulip):

Also a change like

     pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, '_, 'tcx>) ->
-        impl Iterator<Item=Ty<'tcx>> + 'tcx
+        &'tcx[Ty<'tcx>]
     {
         match self.upvar_tuple_ty(def_id, tcx).sty {
-            ty::Tuple(tys) => tys.iter().cloned(),
+            ty::Tuple(tys) => tys,
             ref t => bug!("upvar_tuple_ty is not a valid tuple: {:?}", t),
         }
     }

throws errors like

error[E0271]: type mismatch resolving `<std::iter::Once<&ty::TyS<'_>> as std::iter::IntoIterator>::Item == &&ty::TyS<'_>`
   --> src/librustc/ty/sty.rs:551:44
    |
551 |         self.upvar_tys(def_id, tcx).iter().chain(iter::once(tcx.types.u32))
    |                                            ^^^^^ expected struct `ty::TyS`, found reference
    |
    = note: expected type `&ty::TyS<'_>`
               found type `&&ty::TyS<'_>`

error[E0271]: type mismatch resolving `<std::iter::Chain<std::slice::Iter<'_, &ty::TyS<'_>>, std::iter::Once<&ty::TyS<'_>>> as std::iter::Iterator>::Item == &'tcx ty::TyS<'tcx>`
   --> src/librustc/ty/sty.rs:549:9
    |
549 |         impl Iterator<Item=Ty<'tcx>> + 'a
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected reference, found struct `ty::TyS`
    |
    = note: expected type `&&ty::TyS<'_>`
               found type `&'tcx ty::TyS<'tcx>`
    = note: the return type of a function must have a statically known size

error[E0271]: type mismatch resolving `<std::iter::Once<&ty::TyS<'_>> as std::iter::Iterator>::Item == &&ty::TyS<'_>`
   --> src/librustc/ty/sty.rs:549:9
    |
549 |         impl Iterator<Item=Ty<'tcx>> + 'a
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `ty::TyS`, found reference
    |
    = note: expected type `&ty::TyS<'_>`
               found type `&&ty::TyS<'_>`

I can guess why the compiler is unhappy but is there an idiomatic way, it can be re-written with least effort to make the compiler happy ?

nikomatsakis (Jan 18 2019 at 14:42, on Zulip):

yeah, you'll need to add .iter() and maybe .iter().cloned() into various places...

nikomatsakis (Jan 18 2019 at 14:42, on Zulip):

it's not necessary to fix

nikomatsakis (Jan 18 2019 at 14:42, on Zulip):

I just thought it might be nicer to return a slice

nikomatsakis (Jan 18 2019 at 14:42, on Zulip):

but maybe not

blitzerr (Jan 19 2019 at 05:36, on Zulip):

@nikomatsakis here we go.

blitzerr (Jan 19 2019 at 05:52, on Zulip):
   pub fn upvar_tys(self, tcx: TyCtxt<'_, '_, 'tcx>) -> impl Iterator<Item = Ty<'tcx>> + 'tcx {
    ¦   match self {
    ¦   ¦   DefiningTy::Closure(def_id, substs) => {
    ¦   ¦   ¦   Either::Left(substs.upvar_tys(def_id, tcx).iter().cloned())
    ¦   ¦   }
    ¦   ¦   DefiningTy::Generator(def_id, substs, _) => {
    ¦   ¦   ¦   Either::Right(Either::Left(substs.upvar_tys(def_id, tcx).iter().cloned()))
    ¦   ¦   }
    ¦   ¦   DefiningTy::FnDef(..) | DefiningTy::Const(..) => {
    ¦   ¦   ¦   Either::Right(Either::Right(iter::empty()))
    ¦   ¦   }
    ¦   }
    }

There is still this function in the nll/universal_regions.rs which returns an iterator. Its the branch returning the iter::empty that's messing things up. Couldn't think of an easy way to address this

nikomatsakis (Jan 22 2019 at 16:21, on Zulip):

@blitzerr I'm a bit unsure what you are asking :)

nikomatsakis (Jan 22 2019 at 16:21, on Zulip):

oh, like, how can that fn be converted to return a slice?

nikomatsakis (Jan 22 2019 at 16:22, on Zulip):

you can just return &[] from that final branch

nikomatsakis (Jan 22 2019 at 16:22, on Zulip):
pub fn upvar_tys(self, tcx: TyCtxt<'_, '_, 'tcx>) -> &[Ty<'tcx>] {
    ¦   match self {
    ¦   ¦   DefiningTy::Closure(def_id, substs) => substs.upvar_tys(def_id, tcx),
    ¦   ¦   DefiningTy::Generator(def_id, substs, _) => substs.upvar_tys(def_id, tcx),
    ¦   ¦   DefiningTy::FnDef(..) | DefiningTy::Const(..) => &[],
    ¦   }
    }
nikomatsakis (Jan 22 2019 at 16:23, on Zulip):

this works because [] is a constant and hence &[] can be assigned 'static lifetime

blitzerr (Jan 22 2019 at 18:27, on Zulip):

@nikomatsakis Awesome. Thank you.

blitzerr (Jan 22 2019 at 18:29, on Zulip):

Yeah so that's one of the least favorite impl Trait limitations :slight_smile:

I think we are ready for the next phase. You were talking of a workaround. So, we can do that and move to the next phase of #53488

nikomatsakis (Jan 22 2019 at 18:29, on Zulip):

@blitzerr you'll have to remind me -- workaround for what?

nikomatsakis (Jan 22 2019 at 18:29, on Zulip):

iirc, the next phase was going to be changing the closure substs etc to actually have a tuple

nikomatsakis (Jan 22 2019 at 18:30, on Zulip):

instead of now, where it has N parameters, and then we synthesize the tuple

nikomatsakis (Jan 22 2019 at 18:30, on Zulip):

right?

blitzerr (Jan 22 2019 at 18:32, on Zulip):

That is the logical next step but before that we wanted to make sure (at compilation time) that people don't end up calling upvar_tys from the inference code. So, basically

-    pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, '_, 'tcx>) ->
+    pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, 'tcx, 'tcx>) ->
blitzerr (Jan 22 2019 at 18:32, on Zulip):
error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds
   --> src/librustc/ty/sty.rs:549:9
    |
549 |         impl Iterator<Item=Ty<'tcx>> + 'a
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
note: hidden type `std::iter::Chain<impl std::iter::Iterator, std::iter::Once<&'gcx ty::TyS<'gcx>>>` captures the lifetime 'gcx as defined on the impl at 532:10
   --> src/librustc/ty/sty.rs:532:10
    |
532 | impl<'a, 'gcx, 'tcx> GeneratorSubsts<'tcx> {
    |          ^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0700`.
error: Could not compile `rustc`.

Gives me this error

nikomatsakis (Jan 22 2019 at 18:33, on Zulip):

ah I see

nikomatsakis (Jan 22 2019 at 18:33, on Zulip):

and you don't want to remove the impl Iterator, right?

blitzerr (Jan 22 2019 at 18:33, on Zulip):

there are workarounds but...before we go there...

To which you said :point_up:

nikomatsakis (Jan 22 2019 at 18:34, on Zulip):

so the problem here is that the type returned for the impl Trait is only allowed to reference the lifetimes that appear in the impl Iterator<Item=Ty<'tcx>> + 'a type itself -- namely 'tcx and 'a

blitzerr (Jan 22 2019 at 18:34, on Zulip):

Ohh no, I value your opinion :slight_smile:

nikomatsakis (Jan 22 2019 at 18:34, on Zulip):

but here we are capturing the TyCtxt<'a, 'gcx, 'tcx>, which includes 'gcx

blitzerr (Jan 22 2019 at 18:34, on Zulip):

I already removed all the iterator returns

nikomatsakis (Jan 22 2019 at 18:34, on Zulip):

there is this weird trait called Captures that can be used as a workaround

nikomatsakis (Jan 22 2019 at 18:34, on Zulip):

oh, well, then I don't understand the error

blitzerr (Jan 22 2019 at 18:35, on Zulip):

Except for one, which you just told me how to do

nikomatsakis (Jan 22 2019 at 18:35, on Zulip):

ah ok

nikomatsakis (Jan 22 2019 at 18:35, on Zulip):

well in any case you can do impl Iterator<...> + Captures<'gcx> + 'a

blitzerr (Jan 22 2019 at 18:35, on Zulip):

Tonight, I will check that again.

nikomatsakis (Jan 22 2019 at 18:35, on Zulip):

captures trait: https://doc.rust-lang.org/nightly/nightly-rustc/rustc/util/captures/trait.Captures.html

nikomatsakis (Jan 22 2019 at 18:35, on Zulip):

it's basically a hack -- a "dummy" requirement just to let you mention 'gcx

blitzerr (Jan 22 2019 at 18:35, on Zulip):

You think, after changing iterator to slice ref, that error should go away ?

nikomatsakis (Jan 22 2019 at 18:35, on Zulip):

but I sort of prefer the slice personally

nikomatsakis (Jan 22 2019 at 18:35, on Zulip):

at least I think I do

nikomatsakis (Jan 22 2019 at 18:36, on Zulip):

You think, after changing iterator to slice ref, that error should go away ?

that immediate error yes

nikomatsakis (Jan 22 2019 at 18:36, on Zulip):

I would perhaps expect other errors :)

blitzerr (Jan 22 2019 at 18:36, on Zulip):

:D

blitzerr (Jan 22 2019 at 18:38, on Zulip):

Okay. So then I will make the last change you suggest and try changing TyCtxt<'_, '_, 'tcx>) to TyCtxt<'_, 'tcx, 'tcx>) and see what new errors I get.

blitzerr (Jan 22 2019 at 18:40, on Zulip):

@nikomatsakis
Then we might be good for the next steps. We should discuss that soon

blitzerr (Jan 22 2019 at 18:45, on Zulip):

Thanks for explaining the error. I haven't written much Rust other than my compiler contributions which aren't many either. :grinning:

nikomatsakis (Jan 22 2019 at 20:46, on Zulip):

great, let me know how it goes!

blitzerr (Jan 23 2019 at 04:24, on Zulip):

Making the change

-    pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, '_, 'tcx>) ->
+    pub fn upvar_tys(self, def_id: DefId, tcx: TyCtxt<'_, 'tcx, 'tcx>) ->

gives the errors

error: unsatisfied lifetime constraints=====================>        ] 107/125: rustc
   --> src/librustc/ty/sty.rs:550:9
    |
531 | impl<'a, 'gcx, 'tcx> GeneratorSubsts<'tcx> {
    |          ----  ---- lifetime `'tcx` defined here
    |          |
    |          lifetime `'gcx` defined here
...
550 |         self.upvar_tys(def_id, tcx).iter().cloned().chain(iter::once(tcx.types.u32))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ argument requires that `'tcx` must outlive `'gcx`
blitzerr (Jan 23 2019 at 04:26, on Zulip):

@nikomatsakis In this function I think I need to change the argument
tcx: TyCtxt<'a, 'gcx, 'tcx> to tcx: TyCtxt<'a, 'tcx, 'tcx> but not sure if that is the right way

impl<'a, 'gcx, 'tcx> GeneratorSubsts<'tcx> {
    pub fn pre_transforms_tys(self, def_id: DefId, tcx: TyCtxt<'a, 'gcx, 'tcx>) ->
       impl Iterator<Item=Ty<'tcx>> + 'a
    {
       self.upvar_tys(def_id, tcx).iter().cloned().chain(iter::once(tcx.types.u32))
    }
...
nikomatsakis (Jan 23 2019 at 16:54, on Zulip):

@blitzerr it looks like changing that signature may cause trouble

nikomatsakis (Jan 23 2019 at 16:54, on Zulip):

it may not be necessary to change to 'tcx, 'tcx

nikomatsakis (Jan 23 2019 at 16:54, on Zulip):

that does guarantee that we use this outside of an infernce context

nikomatsakis (Jan 23 2019 at 16:54, on Zulip):

but

nikomatsakis (Jan 23 2019 at 16:54, on Zulip):

we could also enforce that dynamically (by asserting)

nikomatsakis (Jan 23 2019 at 16:55, on Zulip):

in particular, pre_transforms_ty is used in the NLL type-check (which does have an inference context)

nikomatsakis (Jan 23 2019 at 16:55, on Zulip):

however, we could also just rewrite that function -- and perhaps move it into the NLL type_check/mod.rs file, the only place it is invoked -- to match against the tuple and extract the contents

nikomatsakis (Jan 23 2019 at 16:56, on Zulip):

the point being that we know that, when we are in the NLL type-check, these types should be fully inferred (we do have an active inference context then, but we are inferring different things)

blitzerr (Jan 23 2019 at 16:56, on Zulip):

We are enforcing it dynamically (as in the latest change)

blitzerr (Jan 23 2019 at 16:56, on Zulip):

So you suggest we don't mess with the signature ?

nikomatsakis (Jan 23 2019 at 16:57, on Zulip):

I'm torn -- it might not be worth the trouble

nikomatsakis (Jan 23 2019 at 16:57, on Zulip):

we could leave it for a FIXME

blitzerr (Jan 23 2019 at 16:57, on Zulip):

:joy:

nikomatsakis (Jan 23 2019 at 16:57, on Zulip):

I think this is what I would actually do :)

nikomatsakis (Jan 23 2019 at 16:57, on Zulip):

because i'd like to press on and get to the harder bits

nikomatsakis (Jan 23 2019 at 16:57, on Zulip):

and we can come back and clean this part up later

blitzerr (Jan 23 2019 at 16:57, on Zulip):

Sounds good.

nikomatsakis (Jan 23 2019 at 16:58, on Zulip):

do you want to try and schedule another pairing session? might be useful to get into the next step

blitzerr (Jan 23 2019 at 16:58, on Zulip):

I will leave a fixme and move to the next steps

blitzerr (Jan 23 2019 at 16:58, on Zulip):

Absolutely

blitzerr (Jan 23 2019 at 16:58, on Zulip):

How does this week look to you ?

blitzerr (Jan 23 2019 at 16:58, on Zulip):

Any particular day you prefer ?

nikomatsakis (Jan 23 2019 at 17:07, on Zulip):

good question

nikomatsakis (Jan 23 2019 at 17:08, on Zulip):

I could maybe do later today for a bit

nikomatsakis (Jan 23 2019 at 17:08, on Zulip):

and/or some time on Friday is prob next best option

blitzerr (Jan 23 2019 at 17:21, on Zulip):

How about Friday 12:30 - 1:30 pm your time ?

blitzerr (Jan 23 2019 at 17:27, on Zulip):

@nikomatsakis :wait_one_second:

nikomatsakis (Jan 23 2019 at 19:18, on Zulip):

@blitzerr I could do that

blitzerr (Jan 23 2019 at 19:19, on Zulip):

Cool, will set something up

nikomatsakis (Jan 23 2019 at 19:19, on Zulip):

ok, if you can send me an invite to nmatsakis@mozilla.com would be appreciated

blitzerr (Jan 23 2019 at 19:24, on Zulip):

@nikomatsakis I just did :D

blitzerr (Jan 23 2019 at 19:25, on Zulip):

Let me know of you get it and if the video url looks good to you. I used the i-heart-rust extension that you generally use.

nikomatsakis (Jan 25 2019 at 19:03, on Zulip):

@blitzerr the new video is uploading to YouTube now

blitzerr (Jan 25 2019 at 19:04, on Zulip):

@nikomatsakis Awesome :+1:
Thanks a lot for your time. Very grateful :pray:

nikomatsakis (Jan 25 2019 at 19:04, on Zulip):

Likewise =)

nikomatsakis (Jan 25 2019 at 19:42, on Zulip):

@blitzerr video uploaded

blitzerr (Jan 25 2019 at 20:02, on Zulip):

Thank you @nikomatsakis

nikomatsakis (Feb 01 2019 at 15:34, on Zulip):

@blitzerr how is it going with this?

blitzerr (Feb 01 2019 at 15:47, on Zulip):

Sorry @nikomatsakis
It hasn't been going as fast as we would like it to be. Haven't had much time this week at all in the evenings. I am planning to find some time this weekend. Sorry again for not making any progress.

nikomatsakis (Feb 01 2019 at 15:52, on Zulip):

@blitzerr no pressure, just wanted to check in

blitzerr (Feb 01 2019 at 16:03, on Zulip):

Thanks @nikomatsakis

blitzerr (Feb 03 2019 at 20:11, on Zulip):

@nikomatsakis

thread '[ui] ui/closures/closure-move-sync.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3245:9

---- [ui] ui/closures/closure-no-fn-1.rs stdout ----
diff of stderr:

5      |                             ^^^^^^^^^^^^^^^^^^^^^ expected fn pointer, found closure
6      |
7      = note: expected type `fn(u8) -> u8`
-                 found type `[closure@$DIR/closure-no-fn-1.rs:6:29: 6:50 a:_]`
+                 found type `[closure@$DIR/closure-no-fn-1.rs:6:29: 6:50]`
9
10  error: aborting due to previous error
11

If we look at this error, we somehow removed a variable from the closure name [closure@$DIR/closure-no-fn-1.rs:6:29: 6:50 a:_] to [closure@$DIR/closure-no-fn-1.rs:6:29: 6:50]

blitzerr (Feb 03 2019 at 20:15, on Zulip):

generators errors are more wordy, and I don't understand them

---- [ui] ui/generator/auto-trait-regions.rs stdout ----
diff of stderr:

-   error: implementation of `Foo` is not general enough
-     --> $DIR/auto-trait-regions.rs:30:5
+   error[E0597]: borrowed value does not live long enough
+     --> $DIR/auto-trait-regions.rs:44:24
3      |
-   LL |     assert_foo(gen);
-      |     ^^^^^^^^^^
+   LL |         let a = A(&mut true, &mut true, No);
+      |                        ^^^^                - temporary value dropped here while still borrowed
+      |                        |
+      |                        temporary value does not live long enough
+   ...
+   LL |     };
+      |     - temporary value needs to live until here
6      |
-      = note: `&'0 OnlyFooIfStaticRef` must implement `Foo` for any lifetime `'0`
-      = note: but `&'1 OnlyFooIfStaticRef` only implements `Foo` for the lifetime `'1`
+      = note: consider using a `let` binding to increase its lifetime
9
-   error: implementation of `Foo` is not general enough
-     --> $DIR/auto-trait-regions.rs:48:5
+   error[E0597]: borrowed value does not live long enough
+     --> $DIR/auto-trait-regions.rs:44:35
12     |
-   LL |     assert_foo(gen);
-      |     ^^^^^^^^^^
+   LL |         let a = A(&mut true, &mut true, No);
+      |                                   ^^^^     - temporary value dropped here while still borrowed
+      |                                   |
+      |                                   temporary value does not live long enough
+   ...
+   LL |     };
+      |     - temporary value needs to live until here
15     |
-      = note: `A<'0, '1>` must implement `Foo` for any two lifetimes `'0` and `'1`
-      = note: but `A<'_, '2>` only implements `Foo` for the lifetime `'2`
+      = note: consider using a `let` binding to increase its lifetime
18
-   error: aborting due to 2 previous errors
+   error[E0626]: borrow may still be in use when generator yields
+     --> $DIR/auto-trait-regions.rs:44:24
+      |
+   LL |         let a = A(&mut true, &mut true, No);
+      |                        ^^^^
+   LL |         yield;
+      |         ----- possible yield occurs here
20
+   error[E0626]: borrow may still be in use when generator yields
+     --> $DIR/auto-trait-regions.rs:44:35
+      |
+   LL |         let a = A(&mut true, &mut true, No);
+      |                                   ^^^^
+   LL |         yield;
+      |         ----- possible yield occurs here
+
+   error: aborting due to 4 previous errors
+
+   Some errors occurred: E0597, E0626.
+   For more information about an error, try `rustc --explain E0597`.
21
blitzerr (Feb 03 2019 at 20:46, on Zulip):

For the closure related failures, looking at the code, looks like it comes from
format!("[closure@{:?}]", tcx.hir().span(node_id)) in src/librustc/mir/mod.rs. So might be that the span no longer gives you that param.

blitzerr (Feb 14 2019 at 23:13, on Zulip):

@nikomatsakis :wave:
Based on some of your post after all hands, I assume you are re-organizing things that you had on your plate. What happens to this ? :slight_smile:

nikomatsakis (Feb 15 2019 at 20:52, on Zulip):

@blitzerr heh, a good questionm, I've been wondering about that very question!

nikomatsakis (Feb 15 2019 at 20:53, on Zulip):

At the moment, I'm still hoping to work with you on it, but it might be better for both of us if we could find another mentor.

blitzerr (Feb 15 2019 at 21:10, on Zulip):

@nikomatsakis sure, whatever helps you be more efficient.

blitzerr (Feb 15 2019 at 21:11, on Zulip):

I was also wondering if for the less fortunate, who were not invited, will there be a blog post or rust internal post with the big bullet points from the all hands? :grinning:

nikomatsakis (Feb 22 2019 at 18:36, on Zulip):

@blitzerr yeah I want to make one of those. Also we should try to talk, maybe next week?

blitzerr (Feb 22 2019 at 18:39, on Zulip):

@nikomatsakis My availability will be a little spotty till mid March. I will be travelling. But if we can sync async, then I can try to make some progress on the issue.

If you want a talk today, I will be available.

blitzerr (Mar 02 2019 at 04:53, on Zulip):

@nikomatsakis
I think that now you have some time after the region bug fix so I would like to draw your attention to this. :slight_smile:

blitzerr (Mar 02 2019 at 04:54, on Zulip):

One of the ui-tests compiled with rustc with our changes

rustc +stage1 src/test/ui/not-clone-closure.rs
error[E0277]: the trait bound `S: std::clone::Clone` is not satisfied in `[closure@src/test/ui/not-clone-closure.rs:7:17: 9:6 a:S]`
  --> src/test/ui/not-clone-closure.rs:11:23
   |
11 |     let hello = hello.clone(); //~ ERROR the trait bound `S: std::clone::Clone` is not satisfied
   |                       ^^^^^ within `[closure@src/test/ui/not-clone-closure.rs:7:17: 9:6 a:S]`, the trait `std::clone::Clone` is not implemented for `S`
   |
   = note: required because it appears within the type `(S,)`
   = note: required because it appears within the type `[closure@src/test/ui/not-clone-closure.rs:7:17: 9:6 a:S]`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
blitzerr (Mar 02 2019 at 04:54, on Zulip):

The same file compiled with nightly rustc

rustc +nightly src/test/ui/not-clone-closure.rs
error[E0277]: the trait bound `S: std::clone::Clone` is not satisfied in `[closure@src/test/ui/not-clone-closure.rs:7:17: 9:6 a:S]`
  --> src/test/ui/not-clone-closure.rs:11:23
   |
11 |     let hello = hello.clone(); //~ ERROR the trait bound `S: std::clone::Clone` is not satisfied
   |                       ^^^^^ within `[closure@src/test/ui/not-clone-closure.rs:7:17: 9:6 a:S]`, the trait `std::clone::Clone` is not implemented for `S`
   |
   = note: required because it appears within the type `[closure@src/test/ui/not-clone-closure.rs:7:17: 9:6 a:S]`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
blitzerr (Mar 02 2019 at 04:55, on Zulip):

The difference is the presence of this extra line
= note: required because it appears within the type (S,)``

blitzerr (Mar 02 2019 at 05:07, on Zulip):

contents of the test file

cat src/test/ui/not-clone-closure.rs
// Check that closures do not implement `Clone` if their environment is not `Clone`.

struct S(i32);

fn main() {
    let a = S(5);
    let hello = move || {
        println!("Hello {}", a.0);
    };

    let hello = hello.clone(); //~ ERROR the trait bound `S: std::clone::Clone` is not satisfied
}
blitzerr (Mar 02 2019 at 05:14, on Zulip):

So I am trying to step through but I might be doing something wrong as instead of stack I am getting assembly

Process 11116 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = instruction step over
    frame #0: 0x000000010006b8d0 rustc`main + 224
rustc`main:
->  0x10006b8d0 <+224>: callq  0x10042e498               ; symbol stub for: mprotect
    0x10006b8d5 <+229>: testl  %eax, %eax
    0x10006b8d7 <+231>: jne    0x10006bac3               ; <+723>
    0x10006b8dd <+237>: movq   0x4ec764(%rip), %r15      ; std::sys::unix::thread::guard::PAGE_SIZE::h7a87b6bd0741865a (.llvm.14947831840210026976)
Target 0: (rustc) stopped.
nikomatsakis (Mar 04 2019 at 20:49, on Zulip):

@blitzerr it's not clear that this is something you are doing wrong

nikomatsakis (Mar 04 2019 at 20:50, on Zulip):

Are you saying that your branch added the line in question, or removed it?

nikomatsakis (Mar 04 2019 at 20:50, on Zulip):

I can certainly imagine how this diff might occur, not sure that I am worried about it, basically

nikomatsakis (Mar 04 2019 at 20:50, on Zulip):

I imagine your branch added the new line?

simulacrum (Mar 04 2019 at 20:51, on Zulip):

That does appear to be the case (i.e., nightly does not have the line). Seems like an improvement to me :)

nikomatsakis (Mar 04 2019 at 20:52, on Zulip):

debatable, but I think it's ok either way

nikomatsakis (Mar 04 2019 at 20:52, on Zulip):

what's happening is that, in the trait selection code, there is this code that enumeates all the "components"

nikomatsakis (Mar 04 2019 at 20:52, on Zulip):

of a given typ

nikomatsakis (Mar 04 2019 at 20:53, on Zulip):

in the case of closures, it used to produce 1 component per upvar (captured variable)

nikomatsakis (Mar 04 2019 at 20:53, on Zulip):

we changed it @blitzerr to produce a tuple of all upvar types

nikomatsakis (Mar 04 2019 at 20:53, on Zulip):

and that is the line you are now seeing

nikomatsakis (Mar 04 2019 at 20:53, on Zulip):

we could probably make it go away by inspecting the type variable representing the upvar types and -- if it is known to be a tuple -- inlining its contents

nikomatsakis (Mar 04 2019 at 20:54, on Zulip):

(it could also be an unresolved type variable)

blitzerr (Mar 05 2019 at 02:56, on Zulip):

@nikomatsakis , that makes sense.

(it could also be an unresolved type variable)

Do you think we will be error checking before type resolution in some cases ?

nikomatsakis (Mar 05 2019 at 14:47, on Zulip):

ps, further convesation on this topic can move to #t-compiler/wg-rfc-2229

Last update: Nov 21 2019 at 23:30UTC