Stream: wg-async-foundations

Topic: #54716 drop order


davidtwco (Mar 08 2019 at 14:50, on Zulip):

@Taylor Cramer @nikomatsakis So far I've attempted to change the lowering so that:

async fn foo(<pattern>: <ty>) {
    async move {
    }
}

becomes:

async fn foo(__arg0: <ty>) {
    async move {
        let <pattern>: <ty> = __arg0;
    }
}

I did it this way so that I wouldn't need to handle more complex patterns like async fn foo((x, y): (T, T)) {} or even just async fn foo(_: T) {}. I've got that mostly working - I can look at the unpretty'd HIR and see it but I run into failures in rustc/middle/liveness.rs as the Defs associated with the new ids don't quite line up. For example, if I refer to the original name of the binding in the block after my inserted statements, then it has a Def::Upvar rather than Def::Local and __arg0 doesn't have Def::Uvpar.

I'm not sure if I'm just unaware of some helper functions that would make this easier or if I'm taking the wrong approach, I've not really looked at the HIR lowering before so it's all new.

Taylor Cramer (Mar 08 2019 at 15:12, on Zulip):

@davidtwco without looking at the code, I'd guess it's because you changed the parents of those ast nodes without changing how they get seen by the def collector

Taylor Cramer (Mar 08 2019 at 15:13, on Zulip):

I'm not aware of any useful helper functions for this desugaring

Taylor Cramer (Mar 08 2019 at 15:14, on Zulip):

Stuff like this generally requires some id-related hackery

davidtwco (Mar 08 2019 at 15:20, on Zulip):

I see, that's about what I expected.

nikomatsakis (Mar 08 2019 at 21:28, on Zulip):

@davidtwco do you want to post your branch? It seems like you're headed in the right general direction

davidtwco (Mar 09 2019 at 11:05, on Zulip):

I'll clean it up a bit and then push it to a branch.

davidtwco (Mar 10 2019 at 20:47, on Zulip):

@Taylor Cramer @nikomatsakis Apologies for the delay here, I've pushed to this branch.

I ended up realizing that I had to make the rustc_resolve logic and the def collector more aware of the arguments/new statements and that the way I was doing it made that pretty difficult. What I've done instead is introduce a new field to ast::IsAsync and then modify the arguments/block in a few places that it was needed. That got me further into the compiler than where I was before (after a long detour getting the new statements to have ids). It now errors in the MIR gen, but I suspect that is because of failures in type check. I've not had a chance to dig into that though.

I'm not that pleased with the approach it takes. I can't think of a better way, it just feels like the amount of changes it has taken is way more than it should be and there should be somewhere I can make the change to the function's lowering and that would just propagate through the compiler unchanged - I guess that place is probably the parser, but I don't think we'd want that.

davidtwco (Mar 10 2019 at 20:49, on Zulip):

I'm happy to rip out all of it and take a completely different approach if what I've got there is in fact not great. I've learned a bunch from breaking the compiler with those changes.

davidtwco (Mar 10 2019 at 20:54, on Zulip):

When I originally made this topic I’d only modified the lowering code and was making everything there.

davidtwco (Mar 11 2019 at 10:44, on Zulip):

(also, unrelated to this issue, but I'd appreciate being added to the GitHub group for this WG so I can get pings about things)

nikomatsakis (Mar 11 2019 at 19:20, on Zulip):

(also, unrelated to this issue, but I'd appreciate being added to the GitHub group for this WG so I can get pings about things)

done

nikomatsakis (Mar 11 2019 at 19:20, on Zulip):

@davidtwco I agree that some kind of "early change" feels like all that should be needed

nikomatsakis (Mar 11 2019 at 19:21, on Zulip):

I wonder if there is some way to alter the HIR lowering here

nikomatsakis (Mar 11 2019 at 19:21, on Zulip):

or rather alter the HIR

nikomatsakis (Mar 11 2019 at 19:21, on Zulip):

let me check out your branch

davidtwco (Mar 11 2019 at 19:29, on Zulip):

Altering the HIR lowering is the first thing I tried, and that's still what I'm doing. I found that I also had to make changes to the def collector, collector (IIRC) and resolve to also get it to work, and that those changes weren't trivial as it was introducing whole new statements (which was affecting what got considered a Def::Upvar(..) for example), so I made the statements and arguments in the parser (storing them in the IsAsync::Async alongside the ids that were there previously) and then used those in the HIR lowering and other places.

davidtwco (Mar 11 2019 at 19:29, on Zulip):

Originally it was only the HIR lowering that I was modifying.

nikomatsakis (Mar 11 2019 at 19:33, on Zulip):

I see

davidtwco (Mar 12 2019 at 13:36, on Zulip):

I think I've got this working.

davidtwco (Mar 12 2019 at 13:37, on Zulip):

Right now the drop order is reversed but they're being dropped at the right time.

davidtwco (Mar 12 2019 at 14:42, on Zulip):

So, the drops are now happening at the right time - that is, when polled, not when created - but the actual order isn't the same. It works for simple cases but when I throw a _ binding into the mix it doesn't work. You can see an example here.

davidtwco (Mar 12 2019 at 14:53, on Zulip):

The actual result for the test I have is here (GitHub is being funny and sometimes that returns a 404, it does exist). You can see the drops now happen after the function call but not in the same order as a regular function.

davidtwco (Mar 12 2019 at 16:38, on Zulip):

Submitted #59135.

davidtwco (Mar 14 2019 at 15:34, on Zulip):

Summary (#59135)
Since I've written a bunch above, here's a short summary of the current state of this:

In an attempt to fix the drop order of async fn when there are unused arguments, I'm attempting to introduce replace the function arguments and introduce bindings that move the arguments into the inner closure, while preserving the patterns. For example:

async fn foo(<pattern>: <type>) {
  async move {
  }
} // <-- dropped as you "exit" the fn

// ...becomes...
fn foo(__arg0: <ty>) {
  async move {
    let <pattern> = __arg0;
  } // <-- dropped as you "exit" the async block
}

This was working (except for some extra diagnostic output on one test, see the first bullet point below), but after a rebase it regressed and I've been unable to fix it. There are currently three issues with the PR:

error[E0282]: type annotations needed
  --> ./src/test/run-pass/issue-54716.rs:36:14
   |
36 | async fn foo(x: D, _y: D) {
   | ^ cannot infer type

In brief, the change modifies the ast::IsAsync::Async variant to introduce a Vec<AsyncArgument> containing a ident (__arg0), a statement (let <pat> = __arg0;) and a argument (__arg0: <ty>) for each of the original arguments of the function. There are corresponding changes to the visitors so that these get given ids. There are also a few other smaller changes that let me keep track of whether an argument/statement was introduced by the async fn lowering or not - this can help with diagnostics as in one of the commits.

This is then used in the HIR lowering to, well, lower to the HIR - replacing the arguments of the function with our generated arguments above, and prepending the statements to the body of the function before it is lowered into the generator. But it is also used in the def collector and in name resolution as those need to work out what should be Def::Upvar(..) and other things like that. I decided to modify the AST to keep the arg and stmt so I didn't need to repeat creating them all of those places.

Originally I tried modifying only the HIR and keeping only ids in the AST (as was contained in IsAsync previously), but I was struggling to get this to work - I think it would be a preferable solution (I want to limit the amount of AST changes), but getting the name resolution and def collection to work, given just the ids, was challenging - I found just adding the new statements before they would normally do their thing was much more straightforward.

davidtwco (Mar 14 2019 at 15:34, on Zulip):

I've deleted the messages before my summary as they're just confused ramblings of someone too deep into the type checker to make any sense.

centril (Mar 14 2019 at 17:39, on Zulip):

@davidtwco Does your changes assume that you will always have async fn foo($pat: type) { ... }? What if you only had async fn foo($pat) e.g. async fn foo([x: u8, y]) ?

centril (Mar 14 2019 at 17:39, on Zulip):

could you work with the latter?

davidtwco (Mar 14 2019 at 17:40, on Zulip):

I'm not sure about async fn foo($pat).

davidtwco (Mar 14 2019 at 17:41, on Zulip):

I'll check when I'm next looking at this.

davidtwco (Mar 14 2019 at 17:45, on Zulip):

At the moment, I don't think it would work without the type being specified. But async fn is only available on the 2018 edition and anonymous parameters in trait methods aren't allowed 2018 as far as I know - so I don't think it could come up. There could be a case I'm not thinking of though.

centril (Mar 14 2019 at 17:45, on Zulip):

IOW, I think you would need to infer the type of the pattern on the HIR given only the pattern and the type variables of the function (but also e.g. async fn foo([x: impl Debug, y])) and then lower to _arg0: $inferred_type

davidtwco (Mar 14 2019 at 17:46, on Zulip):

IOW, I think you would need to infer the type of the pattern on the HIR given only the pattern and the type variables of the function (but also e.g. async fn foo([x: impl Debug, y])) and then lower to _arg0: $inferred_type

I don't quite understand what you mean.

centril (Mar 14 2019 at 17:46, on Zulip):

@davidtwco It's not an issue of anonymous parameters, those were of form fn foo($type) not fn foo($pat) -- it's an issue of future language design

davidtwco (Mar 14 2019 at 17:47, on Zulip):

I don't think there's anything in this change that would rule that out, but it wouldn't work with my implementation as it is currently, I think.

centril (Mar 14 2019 at 17:47, on Zulip):

@davidtwco sure; I'm only asking to be sure that it is not ruled out and that the impl can be changed to handle it

centril (Mar 14 2019 at 17:52, on Zulip):

I don't quite understand what you mean.

Consider e.g. async fn foo<T>((x: u8, Wrapping(y: T), z: impl Debug)) { ... } -- you'd need to infer that the first argument is of type (u8, Wrapping<T>, InferredDebugType) such that you could then have:

async fn foo<T>(__arg0: (u8, Wrapping<T>, InferredDebugType)) {
    async move {
        let (x: u8, Wrapping(y: T), z: impl Debug) = __arg0;
    }
}
nikomatsakis (Mar 19 2019 at 15:53, on Zulip):

I think there would be many hurdles to getting that to work -- it feels like we have to rethink the HIR lowering around this point in any case. i.e., @davidtwco's existing PR is kind of stretching the current code to its limits in some sense.

centril (Mar 19 2019 at 15:54, on Zulip):

@nikomatsakis sure; It's something I'd like to see supported eventually tho :slight_smile:

davidtwco (Mar 19 2019 at 15:56, on Zulip):

I’m still looking for some feedback on the PR and with the issue that currently plagues it.

nikomatsakis (Mar 19 2019 at 15:58, on Zulip):

Yeah, I was just skimming it.

nikomatsakis (Mar 19 2019 at 15:58, on Zulip):

I started with the tests, I'll take a look at the details

davidtwco (Mar 19 2019 at 16:04, on Zulip):

I originally had the test make a direct comparison with the equivalent non-async fn which surfaced that the order wouldn’t be the same.

nikomatsakis (Mar 19 2019 at 16:54, on Zulip):

I originally had the test make a direct comparison with the equivalent non-async fn which surfaced that the order wouldn’t be the same.

wait, @davidtwco, say more?

nikomatsakis (Mar 19 2019 at 16:54, on Zulip):

maybe I missed something

nikomatsakis (Mar 19 2019 at 16:54, on Zulip):

where does the order differ?

davidtwco (Mar 19 2019 at 16:57, on Zulip):

This playground link illustrates what I noticed.

davidtwco (Mar 19 2019 at 16:57, on Zulip):

cc @nikomatsakis

davidtwco (Mar 19 2019 at 16:59, on Zulip):

The PR still fixes fixed the fact that the parameters got dropped before polling, but the order wasn't what I expected.

nikomatsakis (Mar 19 2019 at 17:00, on Zulip):

@davidtwco hmm that's...quite interesting

davidtwco (Mar 19 2019 at 17:01, on Zulip):

I figured it was out-of-scope for this PR, and that just getting the drops to happen when polling was enough.

davidtwco (Apr 09 2019 at 19:32, on Zulip):

@Taylor Cramer @nikomatsakis I've re-opened the PR as #59823.

davidtwco (Apr 09 2019 at 19:32, on Zulip):

(apparently you can't re-open PRs that you force-push too)

davidtwco (Apr 09 2019 at 19:33, on Zulip):

There was a single outstanding comment from @nikomatsakis on the previous version regarding naming in the test.

pnkfelix (Apr 10 2019 at 10:26, on Zulip):

so, if I understand correctly, you are interested in the cause of the difference between complex and complex2 ?

pnkfelix (Apr 10 2019 at 10:27, on Zulip):

keep in mind that you never actually run the body of the closure that is capturing __arg0,__arg1, and _arg2 in complex2

pnkfelix (Apr 10 2019 at 10:28, on Zulip):

so the destructuring that occurs within its body is irrelevant. all that is relevant is that it captures all three arguments

pnkfelix (Apr 10 2019 at 10:30, on Zulip):

and then it subsequently drops them, when the closure is dropped, in reverse order: __arg2 ("_y"), __arg1 (which drops its components in-order, due to #16493), and __arg0 ("x")

pnkfelix (Apr 10 2019 at 10:31, on Zulip):

In short, the reason you see a difference between complex and complex2 is due to the combination of #16493 (see also #16661) and the fact that complex is destructuring its second argument into (a, _, _c).

pnkfelix (Apr 10 2019 at 10:33, on Zulip):

The output for complex itself does remain interesting, in that we are trying to drop the arguments in reverse declaration order

pnkfelix (Apr 10 2019 at 10:34, on Zulip):

but we have this weird scenario where, because we have partially destructured the second argument, we end up having that middle "_" component left over

pnkfelix (Apr 10 2019 at 10:35, on Zulip):

And thus you can observe where the whole (unnamed) second argument is being dropped, compared to the bindings it has been destructured into (_c and a).

pnkfelix (Apr 10 2019 at 10:36, on Zulip):

I freely admit that if you had shown me this code ahead of time and warned me about the interactions here, I still would not have been able to predict the order we would get. I would like to believe I might have said "it will be either [_y, _c, a, _, x] or it will be [_y, _, _c, a, x], but I do not know which one it will be."

pnkfelix (Apr 10 2019 at 11:11, on Zulip):

(Hmm and I do now note that RFC 1857 explicitly says it does not attempt to specify drop order for closure captures)

davidtwco (Apr 10 2019 at 11:51, on Zulip):

The playground link with the complex example is just something I stumbled upon dealing with the actual issue. It just shows that after this PR, the order for an async fn won't match the order for an fn, though (with this PR) now all parameters will be dropped when polled and not before.

davidtwco (Apr 10 2019 at 11:52, on Zulip):

This test is the behaviour the PR changes.

davidtwco (Apr 10 2019 at 11:53, on Zulip):

Previously, on a line like this (line 107)...

assert_eq!(*af.borrow(), &[Function, Val("_y"), Val("x")]);

...the order that the function would return would be &[Val("_y"), Function, Val("x")];.

davidtwco (Apr 10 2019 at 11:54, on Zulip):

This PR ensures that all the drops happen after the Function in the output of that test. Not that the order they are in after that Function will be the same as the equivalent fn (which is what the playground showed w/ the desugaring taking place manually).

davidtwco (Apr 10 2019 at 11:55, on Zulip):

Currently the PR fails the test due to a type inference error, but I had that working before when I desugared to let <pat>: <ty> = __arg0; but if <ty> was impl Trait then that didn't work, so I had to omit the <ty>, leading to the type inference error I've been unsuccessful in resolving.

pnkfelix (Apr 10 2019 at 13:20, on Zulip):

The playground link with the complex example is just something I stumbled upon dealing with the actual issue. It just shows that after this PR, the order for an async fn won't match the order for an fn, though (with this PR) now all parameters will be dropped when polled and not before.

I suppose the question is then, is there a way to further revise the desugaring so that the order for an async fn will match fn, even in the complex case?

pnkfelix (Apr 10 2019 at 13:21, on Zulip):

hmm. let me think.

pnkfelix (Apr 10 2019 at 13:24, on Zulip):

There actually is another wrinkle here

pnkfelix (Apr 10 2019 at 13:24, on Zulip):

I don't know about generators in particular, but for closures, RFC 1857 explicitly said that drop-order of captured variables remained unspecified

pnkfelix (Apr 10 2019 at 13:26, on Zulip):

of course we control the compiler etc, so we can ensure whatever order we need to support continues to work.

pnkfelix (Apr 10 2019 at 13:30, on Zulip):

I will say this: I am starting to wonder, based on the broader discussion overall, if there might be value in a lint that detects functions that have any (sub)patterns in their parameters of the form: _ or ref x where the type of the non-moved thing has drop glue.

pnkfelix (Apr 10 2019 at 13:30, on Zulip):

and just says: "this is a weird thing to do! It may not behave in the manner you expect!"

pnkfelix (Apr 10 2019 at 13:33, on Zulip):

if we had such a warning lint in place, I'd probably have fewer objections to proposals such as dropping unused params early

davidtwco (Apr 10 2019 at 13:43, on Zulip):

I suppose the question is then, is there a way to further revise the desugaring so that the order for an async fn will match fn, even in the complex case?

I was happy to leave this for a follow-up once I got the async fn lowering to capture all the arguments. All that is blocking that is the type inference error that the PR runs into.

pnkfelix (Apr 10 2019 at 13:49, on Zulip):

That's entirely sensible.

pnkfelix (Apr 10 2019 at 13:49, on Zulip):

(The follow-up also might not even be feasible anyway, so I definitely wouldn't want to block this PR based on it.)

Taylor Cramer (Apr 10 2019 at 16:29, on Zulip):

@pnkfelix I'm not sure if you saw, but @nikomatsakis was suggesting previously that we could feature-gate unused arguments to async fn

Taylor Cramer (Apr 10 2019 at 16:30, on Zulip):

A warning/lint is an interesting alternative

Taylor Cramer (Apr 10 2019 at 16:30, on Zulip):

that I think would go some ways towards avoiding the "not ready yet" perception

Taylor Cramer (Apr 10 2019 at 16:31, on Zulip):

something like e.g. "warning: drop order of unused arguments to async fn is unstable. consider removing unused argument or explicitly dropping it"

centril (Apr 10 2019 at 17:38, on Zulip):

I'm a big fan of @nikomatsakis's suggestion to feature gate here

centril (Apr 10 2019 at 17:39, on Zulip):

I do like taking a more empiricist approach to this

pnkfelix (Apr 10 2019 at 18:05, on Zulip):

I hadn’t seen the feature gate suggestion. Any of these options sound better than trying to rush to a decision here, IMO

Taylor Cramer (Apr 10 2019 at 19:43, on Zulip):

@centril how do you feel about warn vs. error?

Taylor Cramer (Apr 10 2019 at 19:44, on Zulip):

I think I'd prefer a warning in an effort to make unimplemented!() easier on folks and make the experience feel less "unfinished" by avoiding the suggestion of feature gates

centril (Apr 10 2019 at 20:14, on Zulip):

@Taylor Cramer hmm; I think as long as we decide amongst ourselves that the drop order is not stable then that's fine by me

centril (Apr 10 2019 at 20:14, on Zulip):

but we need to have that agreement :slight_smile:

Taylor Cramer (Apr 10 2019 at 21:10, on Zulip):

@centril sounds good-- want to raise the option on the github issue?

centril (Apr 10 2019 at 21:11, on Zulip):

@Taylor Cramer Since it was your idea, can you do it? :P

Taylor Cramer (Apr 10 2019 at 21:19, on Zulip):

@centril lol sure

davidtwco (Apr 12 2019 at 21:53, on Zulip):

Spent a little bit of time looking into this type inference failure again today. I think my previous ideas about it were wrong and I was digging in the wrong place. I'm still not too sure what the issue is though. I've got this log output from the rustc::traits module (which I'm not too familiar with), I think line 102 is where it starts trying to evaluate a predicate that it can't. But, I don't think that's related to the let <pat> = __arg0; statement that we insert which is what I thought was the problem before.

eddyb (Apr 18 2019 at 15:07, on Zulip):

/me starts tearing hair out

centril (Apr 18 2019 at 15:07, on Zulip):

@eddyb do you like... want to elaborate? :P

eddyb (Apr 18 2019 at 15:08, on Zulip):

@nikomatsakis I think async fn desugaring to -> impl Future { async { ... } } may have been a mistake

eddyb (Apr 18 2019 at 15:09, on Zulip):

doesn't an async closure have the same issue as an async fn?

eddyb (Apr 18 2019 at 15:09, on Zulip):

wrt arguments?

davidtwco (Apr 18 2019 at 15:09, on Zulip):

doesn't an async closure have the same issue as an async fn?

I'm not sure, I haven't tried.

eddyb (Apr 18 2019 at 15:09, on Zulip):

so then I would just state-machine-transform the body of the function

eddyb (Apr 18 2019 at 15:10, on Zulip):

the problem is we need the function (or closure?) to return the generator wrapper

eddyb (Apr 18 2019 at 15:12, on Zulip):

@nikomatsakis @davidtwco I think the easiest hack in the meanwhile is to just know that a function was an async fn, and so a direct generator children of it should capture its MIR arguments

centril (Apr 18 2019 at 15:13, on Zulip):

@eddyb maybe easier, but it also feels like a deeper hack since it is closer to MIR ("the kernel")

davidtwco (Apr 18 2019 at 15:13, on Zulip):

so a direct generator children of it

I don't think I understand what you mean by this.

eddyb (Apr 18 2019 at 15:14, on Zulip):

a generator that is a direct child

davidtwco (Apr 18 2019 at 15:14, on Zulip):

Change how the closure capturing for generators work if the generator was from the lowering of an async function?

eddyb (Apr 18 2019 at 15:14, on Zulip):

basically, yes

davidtwco (Apr 18 2019 at 15:14, on Zulip):

Alright, thanks.

eddyb (Apr 18 2019 at 15:14, on Zulip):

@centril in the long term, I agree

eddyb (Apr 18 2019 at 15:15, on Zulip):

right

davidtwco (Apr 18 2019 at 15:15, on Zulip):

I think the type inference issue that the current approach takes is resolvable though, it was working before. I'm just not that familiar with the type inference in the compiler.

eddyb (Apr 18 2019 at 15:16, on Zulip):

I'm a bit torn tbh, on one hand desugaring is good

eddyb (Apr 18 2019 at 15:16, on Zulip):

on the other hand, it's hard to make everything work

eddyb (Apr 18 2019 at 15:17, on Zulip):

inconsistency between def parenting in AST vs HIR bothers me to no end

eddyb (Apr 18 2019 at 15:17, on Zulip):

wait, locals don't have DefIds anymore, do they?

eddyb (Apr 18 2019 at 15:17, on Zulip):

so what's actually hard about moving the patterns to the closure?

eddyb (Apr 18 2019 at 15:18, on Zulip):

and we can add an expression type to HIR to refer to arguments directly :P

centril (Apr 18 2019 at 15:18, on Zulip):

:pick: :pick: :pick:

davidtwco (Apr 18 2019 at 15:18, on Zulip):

It was complex because I couldn't just change the lowering, I also had to make some changes to name resolution so that things correctly got marked as Def::Upvar(..) - it was easiest to do that by just modifying the AST that we gave to lowering and name resolution with the replaced arguments and new statements. That and after I got it working, I started running into this type inference problem.

eddyb (Apr 18 2019 at 15:19, on Zulip):

oh heh, I see

davidtwco (Apr 18 2019 at 15:19, on Zulip):

I also don't have a great deal of experience with making these sorts of changes to the compiler, so there may be something I'm missing.

eddyb (Apr 18 2019 at 15:19, on Zulip):

upvars are indeed problematic, but you should be able to change the resolution after rustc_resolve ran

eddyb (Apr 18 2019 at 15:20, on Zulip):

like, you can just create HIR that pretends it resolved to Def::Upvar. and modify the "freevars" map

eddyb (Apr 18 2019 at 15:20, on Zulip):

(frankly, we should just compute "freevars" aka "upvars" from a HIR pass, and be done with it)

eddyb (Apr 18 2019 at 15:21, on Zulip):

I don't know how I spent most of today messing around with Static(Mutability) -> Static + StaticMut when I could've done the "resolution" thing that makes reasoning about all of this better

davidtwco (Apr 18 2019 at 15:23, on Zulip):

upvars are indeed problematic, but you should be able to change the resolution after rustc_resolve ran

I tried that at first, but I thought I'd need to have walked forward and updated the Def of anywhere that used a given variable so that it pointed to the local that I insert rather than the upvar.

eddyb (Apr 18 2019 at 15:23, on Zulip):

@davidtwco oh, I see, I was confused at first, heh

eddyb (Apr 18 2019 at 15:24, on Zulip):

@davidtwco okay we need to fix upvars

eddyb (Apr 18 2019 at 15:24, on Zulip):

give me... a few weeks :P

davidtwco (Apr 18 2019 at 15:24, on Zulip):

davidtwco okay we need to fix upvars

I'm more than happy to help and sink some time into this, I'd just need some pointers.

eddyb (Apr 18 2019 at 15:24, on Zulip):

we can make it so rustc_resolve just doesn't compute this anymore, and anything looking at a mention of a local needs to figure out whether it's an upvar or not

eddyb (Apr 18 2019 at 15:25, on Zulip):

well, give me a few hours to do the thing I wanted to do today :P

davidtwco (Apr 18 2019 at 15:25, on Zulip):

well, give me a few hours to do the thing I wanted to do today :P

Sure thing, thanks.

eddyb (Apr 18 2019 at 15:25, on Zulip):

then we can start thinking about simplifying name resolution results and tracking upvars post-HIR-lowering

eddyb (Apr 18 2019 at 15:25, on Zulip):

I guess we can compute them during HIR lowering, actually

eddyb (Apr 18 2019 at 15:26, on Zulip):

which would avoid us having to change every single place Def::Upvar is handled today

eddyb (Apr 19 2019 at 09:39, on Zulip):

@davidtwco so the refactor I was doing yesterday was #60110, now let's get to the actual one I wanted :P

davidtwco (Apr 19 2019 at 09:50, on Zulip):

Awesome.

davidtwco (Apr 19 2019 at 09:53, on Zulip):

What do you have in mind?

eddyb (Apr 19 2019 at 10:57, on Zulip):

actually, I don't even need to do it this way, heh

davidtwco (Apr 21 2019 at 01:09, on Zulip):

@nikomatsakis @Taylor Cramer Fixed all the failures I was having with this PR, #59823, should be good to review (assuming Travis agrees, I'll find out in the morning).

davidtwco (Apr 21 2019 at 09:26, on Zulip):

Turns out Travis didn't agree, but it was only a mir-opt test, so should be fixed now.

nikomatsakis (Apr 22 2019 at 18:45, on Zulip):

@davidtwco nice! Did you keep the overall desugaring approach, or try something different

davidtwco (Apr 22 2019 at 18:57, on Zulip):

@nikomatsakis it’s the same as before, I just fixed a stupid mistake I had made.

davidtwco (Apr 24 2019 at 06:52, on Zulip):

@nikomatsakis @Taylor Cramer with the PR for this having landed, do you think it’s worth trying to address that the drop order doesn’t match the equivalent fn exactly?

centril (Apr 24 2019 at 06:53, on Zulip):

what's the diff?

davidtwco (Apr 24 2019 at 08:08, on Zulip):

@centril playground

davidtwco (Apr 24 2019 at 08:10, on Zulip):

We fixed the "might drop before polling" issue, but it isn't exactly the same as the equivalent fn.

davidtwco (Apr 24 2019 at 08:11, on Zulip):

It's a result of desugaring into a closure body, the same is visible when fn is compared with closures, it just doesn't look as odd because more is different than just adding async.

centril (Apr 24 2019 at 08:27, on Zulip):

@davidtwco so _ is dropped after x for the async version in bar but the sync one is the reverse?

centril (Apr 24 2019 at 08:29, on Zulip):

@davidtwco your second link seems to point to the wrong place

davidtwco (Apr 24 2019 at 08:29, on Zulip):

Pretty much, I think it's a bit stranger with the last example:

async: `[Function, Val("_y"), Val("_c"), Val("a"), Val("x"), Val("_"), Val("_")]`
 sync: `[Function, Val("_y"), Val("_"), Val("_c"), Val("a"), Val("_"), Val("x")]`'

I think it's only underscore bindings that differ.

davidtwco (Apr 24 2019 at 08:30, on Zulip):

davidtwco your second link seems to point to the wrong place

Fixed, thanks.

centril (Apr 24 2019 at 08:36, on Zulip):

Thanks; neat explanations.

I do think this should be fixed (or otherwise feature gated);
Seems like a half-measure otherwise and _ seem just as likely as _x

centril (Apr 24 2019 at 08:37, on Zulip):

cc @eddyb who iirc thought async { .. } might not have been so good after all?

Taylor Cramer (Apr 24 2019 at 14:05, on Zulip):

@centril I think he was coming around to the idea that desugaring was having some complex impacts

Taylor Cramer (Apr 24 2019 at 14:06, on Zulip):

@davidtwco yeah, we should make a new issue to discuss the remaining diff

davidtwco (Apr 24 2019 at 14:17, on Zulip):

@Taylor Cramer #60236

davidtwco (Apr 24 2019 at 14:18, on Zulip):

I wasn't sure whether that would be considered blocking or deferred.

Taylor Cramer (Apr 24 2019 at 14:18, on Zulip):

I'm not sure either

Taylor Cramer (Apr 24 2019 at 14:19, on Zulip):

Seems worth discussing on-thread

Nick Lawrence (Apr 24 2019 at 14:25, on Zulip):

Very new here - but just out of interest, what is the PR that introduced the drop order issue? Just want to read on it

davidtwco (Apr 24 2019 at 14:28, on Zulip):

Well, the drop order issue was a result of the way that async fn was desugared, not any particular PR (I guess outside of the implementation of async fn originally). #54716 is the original issue that reported the problem, #59823 resolved it.

Nick Lawrence (Apr 24 2019 at 14:29, on Zulip):

awesome - ty

eddyb (Apr 24 2019 at 18:08, on Zulip):

@Taylor Cramer @centril ugh, I still haven't gotten around to trying out upvar simplification but I basically think we shouldn't do "upvar analysis" during name resolution, but move it to HIR lowering, which should allow correct handling

eddyb (Apr 24 2019 at 18:09, on Zulip):

and if we don't do any other crazy refactors like adding hir::ExprKind::Local or anything like that, it should be relatively self-contained? complexity would move from rustc_resolve to rustc::hir::lowering

eddyb (Apr 24 2019 at 18:10, on Zulip):

and we can even store the "upvar (capture) list" in the hir::ExprKind::Closure instead of in a separate map, but that's a refactor we don't need to do now

nikomatsakis (Apr 26 2019 at 18:24, on Zulip):

@eddyb can you elaborate a bit more on what you mean?

nikomatsakis (Apr 26 2019 at 18:24, on Zulip):

I don't really understand your plan

nikomatsakis (Apr 26 2019 at 18:25, on Zulip):

But I was thinking that if we do want to match the drop order exactly -- and I think ultimately that's probably what we want -- that we could maybe do it by basically baking in more information onto the closure, making things a bit more special. This may intersect the #t-compiler/wg-rfc-2229 work as well, I suppose.

eddyb (Apr 26 2019 at 18:25, on Zulip):

so the difficulty here is that rustc_resolve is what decides the upvar sets

nikomatsakis (Apr 26 2019 at 18:26, on Zulip):

(Though I think that the drop order from the desugaring we are doing now is the drop order that regular functions should have had =) but I guess that's water under the bridge)

eddyb (Apr 26 2019 at 18:26, on Zulip):

but there's not really any good reason for that other than "doing it when name resolution happens" (which isn't exactly a good reason)

eddyb (Apr 26 2019 at 18:27, on Zulip):

if we move "Def::Local -> Def::Upvar conversion" to rustc::hir::lowering, it'd be easier to perform the change that moves patterns into the closure

nikomatsakis (Apr 26 2019 at 18:27, on Zulip):

well

eddyb (Apr 26 2019 at 18:27, on Zulip):

or, ehm

eddyb (Apr 26 2019 at 18:27, on Zulip):

that's not what causes trouble, just the added arg_i variables?

nikomatsakis (Apr 26 2019 at 18:27, on Zulip):

so one of the things we've been doing

eddyb (Apr 26 2019 at 18:27, on Zulip):

/me starts being confused

eddyb (Apr 26 2019 at 18:28, on Zulip):

either way, I think rustc_resolve shouldn't have that upvar logic baked in

eddyb (Apr 26 2019 at 18:28, on Zulip):

and having explicit capture lists in hir::ExprKind::Closure seems cool to me

nikomatsakis (Apr 26 2019 at 18:28, on Zulip):

perhaps so

eddyb (Apr 26 2019 at 18:28, on Zulip):

(instead of a side table)

nikomatsakis (Apr 26 2019 at 18:28, on Zulip):

and having explicit capture lists in hir::ExprKind::Closure seems cool to me

yeah that seems fine

nikomatsakis (Apr 26 2019 at 18:28, on Zulip):

well

nikomatsakis (Apr 26 2019 at 18:29, on Zulip):

I think a key question is

nikomatsakis (Apr 26 2019 at 18:29, on Zulip):

just why does the fn drop order work out the way it does

eddyb (Apr 26 2019 at 18:29, on Zulip):

and we could even use it to generate captures on unused variables

nikomatsakis (Apr 26 2019 at 18:29, on Zulip):

but I guess the tl;dr of the fix I imagine is that we have to have some kind of explicit logic around this

eddyb (Apr 26 2019 at 18:29, on Zulip):

which way? I'm not aware of anything very weird

nikomatsakis (Apr 26 2019 at 18:30, on Zulip):

well, these do not behave the same, I think:

async fn bar(x: D, _: D) {
    x.1.borrow_mut().push(DropOrder::Function);
}

fn bar_sync(x: D, _: D) {
    x.1.borrow_mut().push(DropOrder::Function);
}
nikomatsakis (Apr 26 2019 at 18:31, on Zulip):

https://github.com/rust-lang/rust/issues/60236 has more links

eddyb (Apr 26 2019 at 18:31, on Zulip):

but that's async fn, which right now doesn't emulate the regular fn drop order at all?

eddyb (Apr 26 2019 at 18:31, on Zulip):

is that example incomplete?

nikomatsakis (Apr 26 2019 at 18:31, on Zulip):

so _ is dropped after x for the async version in bar but the sync one is the reverse?

nikomatsakis (Apr 26 2019 at 18:31, on Zulip):

so, async fn desugars to

nikomatsakis (Apr 26 2019 at 18:32, on Zulip):
async move {
  let x = arg1;
  let _ = arg2;
  ...
}
eddyb (Apr 26 2019 at 18:32, on Zulip):

wait did we already do this?

nikomatsakis (Apr 26 2019 at 18:32, on Zulip):

I sort of thought that's what functions did too

nikomatsakis (Apr 26 2019 at 18:32, on Zulip):

yes

eddyb (Apr 26 2019 at 18:32, on Zulip):

OOOOH

nikomatsakis (Apr 26 2019 at 18:32, on Zulip):

the problem is that it still doesn't quite match

eddyb (Apr 26 2019 at 18:32, on Zulip):

yeah okay so the reason is probably that hack we added?

nikomatsakis (Apr 26 2019 at 18:32, on Zulip):

actually oh I .. hmm

eddyb (Apr 26 2019 at 18:32, on Zulip):

(oops)

nikomatsakis (Apr 26 2019 at 18:32, on Zulip):

I wonder if it's the hack for "single arugment patterns"

nikomatsakis (Apr 26 2019 at 18:32, on Zulip):

in fns

eddyb (Apr 26 2019 at 18:32, on Zulip):

to special-case let x = ...; including

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

is that the hack you meant?

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

yeah

eddyb (Apr 26 2019 at 18:33, on Zulip):

yeah that's what I mean

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

I .. think that was an unintentional side-effect

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

that we stupidly didn't think about

eddyb (Apr 26 2019 at 18:33, on Zulip):

wow

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

"doh"

eddyb (Apr 26 2019 at 18:33, on Zulip):

but it sounds like we're just not putting it into the right scope?

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

one thing I regret from the MIR work, btw

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

was not taking the time to carefully write all this stuff out

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

and make sure we were happy with it :)

nikomatsakis (Apr 26 2019 at 18:33, on Zulip):

but it sounds like we're just not putting it into the right scope?

you mean the x in the regular function case?

eddyb (Apr 26 2019 at 18:34, on Zulip):

something something sufficiently advanced environment for writing a neat executable spec in :P

eddyb (Apr 26 2019 at 18:34, on Zulip):

yeah

nikomatsakis (Apr 26 2019 at 18:34, on Zulip):

was not taking the time to carefully write all this stuff out

to be fair, some people tried to, but we never really followed it all the way through

eddyb (Apr 26 2019 at 18:34, on Zulip):

it sounds like it's getting dropped in the same scope _ is?

nikomatsakis (Apr 26 2019 at 18:34, on Zulip):

correct

nikomatsakis (Apr 26 2019 at 18:34, on Zulip):

I agree that it seems like a bug

nikomatsakis (Apr 26 2019 at 18:34, on Zulip):

but I'm not sure it's a bug we can fix =)

eddyb (Apr 26 2019 at 18:34, on Zulip):

yeah that... should just not happen

eddyb (Apr 26 2019 at 18:34, on Zulip):

I'm surprised it can at all

eddyb (Apr 26 2019 at 18:35, on Zulip):

isn't some of this more recent? can you use godbolt to try versions? I guess that doesn't execute the code (so you'd have to rely on generating very simple assembly that e.g. stores to an atomic static)

eddyb (Apr 26 2019 at 18:35, on Zulip):

I guess I could try, I'm not doing anything else atm (other than playing with those silly diagrams)

nikomatsakis (Apr 26 2019 at 18:36, on Zulip):

I don't know

nikomatsakis (Apr 26 2019 at 18:36, on Zulip):

worth finding out

nikomatsakis (Apr 26 2019 at 18:36, on Zulip):

I mean it is a pretty edge case

eddyb (Apr 26 2019 at 18:43, on Zulip):

wait, I'm confused

eddyb (Apr 26 2019 at 18:43, on Zulip):

you were saying _ is dropped before the named variable?

eddyb (Apr 26 2019 at 18:44, on Zulip):

okay so there are several things interacting here

eddyb (Apr 26 2019 at 18:49, on Zulip):

@nikomatsakis okay I've confirmed that named patterns don't matter https://godbolt.org/z/HIuWzU

eddyb (Apr 26 2019 at 18:51, on Zulip):

frankly I can't get any order other than 2,1 to show up

eddyb (Apr 26 2019 at 18:52, on Zulip):

@nikomatsakis so I think the problem is with closure captures!

eddyb (Apr 26 2019 at 18:52, on Zulip):

try doing this instead:

eddyb (Apr 26 2019 at 18:53, on Zulip):
let _ = arg_N;
// ...
let _ = arg_0;
let pat_0 = arg_0;
// ...
let pat_N = arg_N;
nikomatsakis (Apr 26 2019 at 19:32, on Zulip):

cc @davidtwco :point_up:

nikomatsakis (Apr 26 2019 at 19:32, on Zulip):

you were saying _ is dropped before the named variable?

honestly i'm not sure @eddyb, I didn't dig that deeply

nikomatsakis (Apr 26 2019 at 19:33, on Zulip):

so I think the problem is with closure captures!

OH

nikomatsakis (Apr 26 2019 at 19:33, on Zulip):

you mean because of the order that struct fields are dropped

nikomatsakis (Apr 26 2019 at 19:33, on Zulip):

and how that is different from let

nikomatsakis (Apr 26 2019 at 19:33, on Zulip):

wow, that'd be nifty

davidtwco (Apr 26 2019 at 19:34, on Zulip):

When I spoke to you last @eddyb, the desugaring for async fn to statements hadn’t landed.

davidtwco (Apr 26 2019 at 19:35, on Zulip):

I’ll give that a shot.

davidtwco (Apr 26 2019 at 20:37, on Zulip):
                                /// Check that unused bindings are dropped after the function is polled.
                                async fn foo_async(__arg0: D, __arg1: D)
                                 ->
                                      ::std::future::from_generator(move ||
                                                                        {
                                                                            let _ =
                                                                                __arg1;
                                                                            let _ =
                                                                                __arg0;
                                                                            let x =
                                                                                __arg0;
                                                                            let _y =
                                                                                __arg1;
                                                                            x.1.borrow_mut().push(DropOrder::Function);
                                                                        })

If this is what you meant, it doesn't seem to achieve the desired effect.

nikomatsakis (Apr 26 2019 at 21:07, on Zulip):

what effect is achieved? :)

davidtwco (Apr 26 2019 at 21:09, on Zulip):

It failed on the same assert as before in the test from the issue, where there are only two parameters - x and _.

davidtwco (Apr 26 2019 at 21:09, on Zulip):

They were still the wrong order, so no effect?

davidtwco (Apr 26 2019 at 21:11, on Zulip):
 async: `[Function, Val("x"), Val("_")]`
  sync: `[Function, Val("_"), Val("x")]`
nikomatsakis (Apr 26 2019 at 21:22, on Zulip):

@davidtwco that order is the order in which things get dropped?

davidtwco (Apr 26 2019 at 21:22, on Zulip):

Yeah.

davidtwco (Apr 26 2019 at 21:22, on Zulip):

It's just the assert output but I renamed left and right to async and sync respectively.

nikomatsakis (Apr 26 2019 at 21:22, on Zulip):

and the example is fn bar(x: Blah, _: ...)?

nikomatsakis (Apr 26 2019 at 21:22, on Zulip):

or is it _, x

davidtwco (Apr 26 2019 at 21:23, on Zulip):
async fn bar(x: D, _: D) {
    x.1.borrow_mut().push(DropOrder::Function);
}

fn bar_sync(x: D, _: D) {
    x.1.borrow_mut().push(DropOrder::Function);
}

Comparison of the drop order of these functions.

nikomatsakis (Apr 26 2019 at 21:24, on Zulip):

yep, ok

davidtwco (Apr 26 2019 at 21:27, on Zulip):

And here's the HIR for those functions (since before I posted the foo desugaring, though they're pretty similar):

                                /// Check that underscore patterns are dropped after the function is polled.
                                async fn bar_async(__arg0: D, __arg1: D)
                                 ->
                                      ::std::future::from_generator(move ||
                                                                        {
                                                                            let _ =
                                                                                __arg1;
                                                                            let _ =
                                                                                __arg0;
                                                                            let x =
                                                                                __arg0;
                                                                            let _ =
                                                                                __arg1;
                                                                            x.1.borrow_mut().push(DropOrder::Function);
                                                                        })

                                fn bar_sync(x: D,
                                            _:
                                                D) {
                                                       x.1.borrow_mut().push(DropOrder::Function);
                                                   }
davidtwco (Apr 26 2019 at 21:32, on Zulip):

Interestingly, this appears to work for the simple case when just experimenting w/ closures.

davidtwco (Apr 26 2019 at 21:37, on Zulip):

It could be that in one or two places I'm not visiting in the right order and that's why I don't see it locally.

davidtwco (Apr 26 2019 at 21:40, on Zulip):

Interestingly, this appears to work for the simple case when just experimenting w/ closures.

Scratch that, that test had the right order before too (because it is the reverse of what we have implemented, but the implementation can't be reversed w/out breaking the order for non-_ bindings IIRC).

davidtwco (Apr 26 2019 at 21:44, on Zulip):

I don't think any "simple" change to our order will work because the more complex patterns aren't affected and still come up wrong.

nikomatsakis (Apr 26 2019 at 21:50, on Zulip):

Yeah -- I think that the bug (in some sense) is in the order that fn drop executes

nikomatsakis (Apr 26 2019 at 21:50, on Zulip):

But I have to dig a bit to verify that hypothesis I guess

nikomatsakis (Apr 26 2019 at 21:50, on Zulip):

But by "bug" I mean that it is not equivalent to a simple desugaring

nikomatsakis (Apr 26 2019 at 21:51, on Zulip):

Though I sort of feel like it was "meant" to be

nikomatsakis (Apr 26 2019 at 21:51, on Zulip):

It's a grey area =)

nikomatsakis (Apr 30 2019 at 17:34, on Zulip):

so @davidtwco I have a theory as to what the problem is

nikomatsakis (Apr 30 2019 at 17:34, on Zulip):

I'm not sure if I communicated it to you

nikomatsakis (Apr 30 2019 at 17:35, on Zulip):

basically, when we do MIR construction, we actually desugar complex fn patterns in the same way you are doing manually

nikomatsakis (Apr 30 2019 at 17:36, on Zulip):

at least I think this is true, i'm looking for the code

nikomatsakis (Apr 30 2019 at 17:36, on Zulip):

anyway, I think we have an optimization around fn fo(a: u32) to avoid doing this, because it was adding an extra MIR parameter

nikomatsakis (Apr 30 2019 at 17:36, on Zulip):

but I think this affects the drop order

nikomatsakis (Apr 30 2019 at 17:37, on Zulip):

ah, yes, right here is the code I am talking about

nikomatsakis (Apr 30 2019 at 17:39, on Zulip):

i.e., the problem here is that in the "simple pattern case" we wind up directly reusing the MIR local for the argument

nikomatsakis (Apr 30 2019 at 17:39, on Zulip):

but in the "complex pattern" case we will "declare bindings" separately, and they will drop later

nikomatsakis (Apr 30 2019 at 17:40, on Zulip):

this perhaps suggests that treating "simple paths" in the desugaring would work I think?

davidtwco (Apr 30 2019 at 17:41, on Zulip):

Treating simple paths in what way? I'm not sure I follow.

nikomatsakis (Apr 30 2019 at 17:41, on Zulip):

i.e., right now (iiuc) we desugar

async fn bar(x: D, _: D) {
    x.1.borrow_mut().push(DropOrder::Function);
}

to

fn bar(__arg0: D, __arg1: D) {
  async move {
    let x = __arg0; // moves __arg0 into x
    let _ = __arg1; // no-op
  }
}
nikomatsakis (Apr 30 2019 at 17:41, on Zulip):

as a result, we will first drop the inner binding x, then we will drop __arg1

nikomatsakis (Apr 30 2019 at 17:42, on Zulip):

but in the normal fn case, we don't have the equivalent let x = __arg0 because we "optimized" that case

nikomatsakis (Apr 30 2019 at 17:42, on Zulip):

and instead we map x directly to arg0

nikomatsakis (Apr 30 2019 at 17:43, on Zulip):

so maybe a desugaring like this

// do not rename `x`, because it is a simple pattern, but rename `_` to `__arg1`
fn bar(x: D, __arg1: D) {
  async move {
    // first add a `let _ = X` for each argument `X`:
    let _ = x;  // ensure this is captured
    let _ = __arg1;      // and this

    // then, for each non-simple pattern, add `let PAT = ARG`:
    let _ = __arg1; // no-op
  }
}
nikomatsakis (Apr 30 2019 at 17:43, on Zulip):

I think that would have the intended drop order?

nikomatsakis (Apr 30 2019 at 17:44, on Zulip):

you might have to reverse the order of the first group of let _ = ... statements

nikomatsakis (Apr 30 2019 at 17:44, on Zulip):

does that make sense?

davidtwco (Apr 30 2019 at 17:49, on Zulip):

What I was noticing was that for this:

fn bar(__arg0: D, __arg1: (D, D, D), __arg2: D) -> impl Future {
  future::from_generator(|| {
    let x = __arg0;
    let (a, _, _c) = __arg1;
    let _y = __arg2;
  })
}

We would end up with MIR that looked something like this (apologies for the rough hand-written MIR-esque representation):

fn bar::{{closure}}(_1: [generator __arg0:D __arg1:(D,D,D) __arg2:D]) -> () {
_2: D "x"
_3: D "a"
_4: D "_c"
_5: D "_y"

bb0:
    _2 = _1.1
    _3 = _1.2.1
    // missing local for the `_` binding
    _4 = _1.2.3
   _5 = _1.3

...

bb7:
   drop(_2) -> bb8

bb8:
   drop(_3) -> bb9

// no drop for the underscore binding

bb9:
   drop(_4) -> bb10

bb10:
   drop(_5) -> bb11

// underscore binding dropped with the rest of the generator

bb11:
   drop(_1)
}
davidtwco (Apr 30 2019 at 17:49, on Zulip):

(I just got back home so I'm currently making dinner and putting some washing on, so slightly preoccupied)

davidtwco (Apr 30 2019 at 20:56, on Zulip):

So, I've fixed one of the order differences. I'm not yet sure why it is only one of them.

With this signature:

async fn foobar(x: D, (a, _, _c): (D, D, D), _: D, _y: D) {
}

On nightly currently:

 async: `[Function, Val("_y"), Val("_c"), Val("a"), Val("x"), Val("_"), Val("_")]`
  sync: `[Function, Val("_y"), Val("_"), Val("_c"), Val("a"), Val("_"), Val("x")]`

Locally, after making some changes:

 async: `[Function, Val("_y"), Val("_"), Val("_c"), Val("_"), Val("a"), Val("x")]`,
  sync: `[Function, Val("_y"), Val("_"), Val("_c"), Val("a"), Val("_"), Val("x")]`
davidtwco (Apr 30 2019 at 21:00, on Zulip):

(I've not ran any of the other tests, just using a reduced test case with just the foobar function for now).

davidtwco (Apr 30 2019 at 21:24, on Zulip):

When I look at the drop order in the MIR, then the drop terminators happen in the expected order (now, after adding drops for the _ bindings), and that works for the _ binding that isn't in a more complex pattern, but not the _ in the tuple, even though drop terminator for _ (in the same scope) is before a.

Though, the downsides with this approach are that it just changes the desugaring slightly to get closer to the fn order, it doesn't do anything to try unify the ordering and that it also wouldn't work if you desugared the function manually (which I think is fine?).

davidtwco (Apr 30 2019 at 21:49, on Zulip):

Interestingly, the same thing appears to happen for the x, _ case even though there's no complex pattern in that. I don't have time to investigate any further tonight, but I think this is still reasonable progress.

nikomatsakis (Apr 30 2019 at 22:20, on Zulip):

OK -- did what I wrote make any sense to you, @davidtwco? If I'm right (and I might not be), we ought to be able to match all cases by special-casing "simple patterns"

nikomatsakis (Apr 30 2019 at 22:20, on Zulip):

I should run some experiments

davidtwco (Apr 30 2019 at 22:22, on Zulip):

I haven’t tried it, but I should be able to pretty quickly tomorrow.

davidtwco (Apr 30 2019 at 22:29, on Zulip):

I wasn’t expecting this case to work, but given that it does, you’re probably right and your solution would do it.

Matthew Jasper (Apr 30 2019 at 22:33, on Zulip):

adding drops for the _ bindings

Doesn't this break something like &(_,): &(String,)?

davidtwco (Apr 30 2019 at 23:09, on Zulip):

Doesn't this break something like &(_,): &(String,)?

I've ditched that approach in favor of @nikomatsakis's suggestion.

OK -- did what I wrote make any sense to you, davidtwco? If I'm right (and I might not be), we ought to be able to match all cases by special-casing "simple patterns"

So, I've implemented this. It fixes the simple case where you've got async fn bar(x: D, _: D), but not the more complex foobar case which desugars to:

                                async fn foobar_async(x: D, __arg1: (D, D, D),
                                                      __arg2: D, _y: D)
                                 ->
                                      ::std::future::from_generator(move ||
                                                                        {
                                                                            let _ =
                                                                                _y;
                                                                            let _ =
                                                                                __arg2;
                                                                            let _ =
                                                                                __arg1;
                                                                            let _ =
                                                                                x;
                                                                            let (a,
                                                                                 _,
                                                                                 _c) =
                                                                                __arg1;
                                                                            let _ =
                                                                                __arg2;
                                                                            x.1.borrow_mut().push(DropOrder::Function);
                                                                        })

and gives the result:

 async: `[Function, Val("_c"), Val("a"), Val("_y"), Val("_"), Val("_"), Val("x")]`
  sync: `[Function, Val("_y"), Val("_"), Val("_c"), Val("a"), Val("_"), Val("x")]`
nikomatsakis (Apr 30 2019 at 23:10, on Zulip):

what's the original again?

nikomatsakis (Apr 30 2019 at 23:11, on Zulip):

hmm

nikomatsakis (Apr 30 2019 at 23:11, on Zulip):

I guess my thesis is wrong when it comes to sync drop order

davidtwco (Apr 30 2019 at 23:12, on Zulip):

I'm trying now with the order of the let <pat> = <init> statements reversed just to see what that does.

davidtwco (Apr 30 2019 at 23:14, on Zulip):

No difference.

davidtwco (Apr 30 2019 at 23:14, on Zulip):

I assume the order is determined only by the let _ = <init>; statements.

davidtwco (Apr 30 2019 at 23:15, on Zulip):

Anyway, it's late here, I'll implement any ideas you have sometime tomorrow.

nikomatsakis (May 01 2019 at 09:12, on Zulip):

First, let me check. This function:

fn foobar(x: D, (a, _, c): (D, D, D), _: D, y: D)

drops in the order:

nikomatsakis (May 01 2019 at 09:13, on Zulip):

this suggests that the order is the following:

nikomatsakis (May 01 2019 at 09:14, on Zulip):

if so, then I guess we can desugar to match this order like so:

nikomatsakis (May 01 2019 at 09:17, on Zulip):
// in order over arguments:
let __arg_i = __arg_i;
let pattern_i = __arg_i;

i.e.,

let __arg0 = __arg0;
let x = __arg0;

let __arg1 = __arg1;
let (a, _, c) = __arg1;

let __arg2 = __arg2;
let _ = __arg2;

let __arg3 = __arg3;
let y = __arg3;

we could then go further and special case "simple" patterns, which is really an optimization I think, so that we wind up with

let x = x;

let __arg1 = __arg1;
let (a, _, c) = __arg1;

let __arg2 = __arg2;
let _ = __arg2;

let y = y;
nikomatsakis (May 01 2019 at 09:17, on Zulip):

we ought to be able to test this theory on play ;)

nikomatsakis (May 01 2019 at 09:19, on Zulip):

re-reading the code I cited earlier, this makes sense. The MIR lowering for arguments:

nikomatsakis (May 01 2019 at 09:19, on Zulip):
nikomatsakis (May 01 2019 at 09:20, on Zulip):

when you register things for drop, they wind up being dropped in the reverse order of which they were registered

nikomatsakis (May 01 2019 at 09:20, on Zulip):

@davidtwco -- if you get a chance, see if that desugaring works better in play

davidtwco (May 01 2019 at 09:22, on Zulip):

I'll give that a go in a bit, thanks.

davidtwco (May 01 2019 at 12:15, on Zulip):

@nikomatsakis I think that worked, I modified my PR (it was quicker than using the playground after how I'd modified it last night) and it passed the tests. Will put a PR up shortly.

nikomatsakis (May 01 2019 at 12:19, on Zulip):

@davidtwco :tada: even better, that drop order (i.e., for fns) is...kind of logical.

nikomatsakis (May 01 2019 at 12:19, on Zulip):

We should document it ;)

davidtwco (May 01 2019 at 12:55, on Zulip):

@nikomatsakis #60437

nikomatsakis (May 01 2019 at 12:57, on Zulip):

@davidtwco btw, I've not looked closely at the desugaring bits, but these __arg3 etc variables -- are they "visible" to users? Or are we using hygiene to hide them?

davidtwco (May 01 2019 at 12:57, on Zulip):

I'm not sure. I haven't done anything specific to hide them.

davidtwco (May 01 2019 at 12:58, on Zulip):

I figured as they were being moved into the pattern that it wouldn't be usable, but they might be visible in that the error you get when attempting to use them isn't "this doesn't exist".

nikomatsakis (May 01 2019 at 13:00, on Zulip):

I guess with _ patterns they are still usable

nikomatsakis (May 01 2019 at 13:01, on Zulip):

Can you write a test and maybe file a bug? Seems like something we should try to fix ;)

nikomatsakis (May 01 2019 at 13:07, on Zulip):

(I can do it)

nikomatsakis (May 01 2019 at 13:09, on Zulip):

__arg variables from async fn desugaring are accessible to the user #60438

nikomatsakis (May 01 2019 at 13:09, on Zulip):

I think we should be able to use hygiene here to hide these __arg variables, but I'm not entirely sure how to do that.

nikomatsakis (May 01 2019 at 13:09, on Zulip):

@Vadim Petrochenkov -- any tips for how we can create a __arg variable (during parsing, I believe) that will not be nameable by the user? Is the "gensym" method the thing we want?

nikomatsakis (May 01 2019 at 13:10, on Zulip):

@eddyb may also have thoughts

eddyb (May 01 2019 at 13:10, on Zulip):

yeah, gensym, but ideally you wouldn't go as far back as parsing, ugh

davidtwco (May 01 2019 at 13:11, on Zulip):

We don't introduce it during parsing, we just construct it there otherwise we'd need to construct the same arg/stmt in a few places. It's still not ideal.

eddyb (May 01 2019 at 13:11, on Zulip):

I'm accumulating too many refactors to do, but we should seriously consider creating Def::Upvar out of Def::Local during HIR lowering, so much less of this needs to be handled by rustc_resolve

nikomatsakis (May 01 2019 at 13:12, on Zulip):

I definitely agree that we clearly need some refactoring here :)

nikomatsakis (May 01 2019 at 13:13, on Zulip):

That is, this feels like it was much harder than it should have been

eddyb (May 01 2019 at 13:13, on Zulip):

I also need some prioritization, I started something a week ago and it blew out of control

nikomatsakis (May 01 2019 at 13:13, on Zulip):

We don't introduce it during parsing, we just construct it there otherwise we'd need to construct the same arg/stmt in a few places. It's still not ideal.

@davidtwco -- have you seen gensym?

davidtwco (May 01 2019 at 13:13, on Zulip):

I'm compiling a version with it now.

eddyb (May 01 2019 at 13:13, on Zulip):

and it overlaps enough with this Def::Upvar thing that I wanted to finish that first, but maybe I should've switched gears

davidtwco (May 01 2019 at 13:14, on Zulip):

So, this stops users using it, but there's still a suggestion.

error[E0425]: cannot find value `__arg1` in this scope
  --> /home/david/projects/rust/rust2/src/test/ui/issue-60236.rs:21:16
   |
LL |     assert_eq!(__arg1, (1, 2, 3));
   |                ^^^^^^ help: a local variable with a similar name exists: `__arg1`

error[E0425]: cannot find value `__arg2` in this scope
  --> /home/david/projects/rust/rust2/src/test/ui/issue-60236.rs:22:16
   |
LL |     assert_eq!(__arg2, 4);
   |                ^^^^^^ help: a local variable with a similar name exists: `__arg2`
eddyb (May 01 2019 at 13:14, on Zulip):

lol

eddyb (May 01 2019 at 13:15, on Zulip):

IMO that suggestion shouldn't fire on gensyms

davidtwco (May 01 2019 at 13:15, on Zulip):

I'll keep it and the suggestion fix can be a follow-up.

davidtwco (May 01 2019 at 13:15, on Zulip):

Unless it's really quick to fix I suppose, I should look.

eddyb (May 01 2019 at 13:15, on Zulip):

yeah it should just be if !name.is_gensym() around the "edit distance" comparison

eddyb (May 01 2019 at 13:16, on Zulip):

also, this probably means it fires right now on _1 and use Foo as _;

eddyb (May 01 2019 at 13:16, on Zulip):

if that's the closest name

davidtwco (May 01 2019 at 13:26, on Zulip):

Alright, that's fixed. Will include in the PR.

nikomatsakis (May 01 2019 at 13:46, on Zulip):

Nice job

davidtwco (May 01 2019 at 13:48, on Zulip):

@nikomatsakis Just updated it.

davidtwco (May 01 2019 at 13:49, on Zulip):

And fixed previous comments.

nikomatsakis (May 01 2019 at 13:58, on Zulip):

@davidtwco looks good, though the test looks a bit more complex than necessary. But I guess the "arc-wake" infra might be useful for other tests.

davidtwco (May 01 2019 at 13:58, on Zulip):

I suppose it is. I wasn't really thinking about it, I just took the existing test and deleted parts.

davidtwco (May 01 2019 at 13:58, on Zulip):

I can simplify it if you want.

davidtwco (May 01 2019 at 13:59, on Zulip):

I assume ArcWake will only be useful in run-pass anyway.

nikomatsakis (May 01 2019 at 14:00, on Zulip):

@davidtwco Yeah, let's simplify. Also, I'd probably prefer for all the tests to live in src/test/ui/async-await (including the run-pass test!) but I don't know if we've reached a conclusion on that yet.

nikomatsakis (May 01 2019 at 14:00, on Zulip):

i.e., I think we're still separating run-pass tests sometimes? I remember we went back and forth

davidtwco (May 01 2019 at 14:01, on Zulip):

There are some from the first batch that got moved into ui that remain there, otherwise they are all in run-pass.

davidtwco (May 01 2019 at 14:01, on Zulip):

I think I moved all the RFC 2008 tests together into UI after that though.

davidtwco (May 01 2019 at 14:05, on Zulip):

@nikomatsakis fixed that.

davidtwco (May 01 2019 at 14:05, on Zulip):

PR updated.

nikomatsakis (May 01 2019 at 15:10, on Zulip):

Nice work, @davidtwco!

nikomatsakis (May 01 2019 at 15:10, on Zulip):

Really great to close this one out :)

centril (May 01 2019 at 17:27, on Zulip):

Yea we only use .../ui tests these days for new tests

eddyb (May 01 2019 at 23:35, on Zulip):

@davidtwco so this is the PR I was hoping to finish much earlier: https://github.com/rust-lang/rust/pull/60462

davidtwco (May 02 2019 at 19:12, on Zulip):

@eddyb awesome! looks good.

eddyb (May 03 2019 at 02:03, on Zulip):

yeah, I just need to get to the part where I actually help the async situation :P

eddyb (May 04 2019 at 01:19, on Zulip):

@davidtwco oooh I have some good news! we should be able to kill Res::Upvar completely!

eddyb (May 04 2019 at 02:23, on Zulip):

okay, it's a bit tricky, but it's effectively unnecessary

davidtwco (May 04 2019 at 10:17, on Zulip):

Apologies for all the fallout of the drop order PRs, I'll do better in future.

Taylor Cramer (May 07 2019 at 01:10, on Zulip):

@davidtwco heh, no worries-- it's a tricky mess. They didn't occur to me either!

Taylor Cramer (May 07 2019 at 01:10, on Zulip):

I guess that's good incentive to keep adding lots of good tests

davidtwco (May 09 2019 at 17:02, on Zulip):

#60674

davidtwco (May 09 2019 at 17:02, on Zulip):

Ugh.

nikomatsakis (May 09 2019 at 17:20, on Zulip):

ugh indeed

davidtwco (May 09 2019 at 18:21, on Zulip):

#60676

eddyb (May 29 2019 at 00:13, on Zulip):

https://github.com/rust-lang/rust/pull/61276

eddyb (May 29 2019 at 00:14, on Zulip):

I managed to get back to this refactor and finish it, well, minus the part where async closures are lowered at HIR time

eddyb (May 29 2019 at 00:26, on Zulip):

the painful thing is that it looks like there could've been a version that works in HIR lowering

eddyb (May 29 2019 at 01:27, on Zulip):

I am currently working my way towards a revert of the libsyntax side

eddyb (May 29 2019 at 01:49, on Zulip):

@davidtwco okay I have a relatively clean revert of the AST changes, time to figure out (if I even can) what goes into the HIR side

eddyb (May 29 2019 at 02:19, on Zulip):

hmmm how does all of this work for async closures? AFAICT they don't get any extra capture-forcing shenanigans

eddyb (May 29 2019 at 02:19, on Zulip):

@davidtwco @nikomatsakis is it accidental that async closures don't have the same drop order for their arguments as async fns?

davidtwco (May 29 2019 at 05:21, on Zulip):

I don’t think we looked into async closures at all.

I think that an async closure would match the equivalent closure because the equivalent closure wouldn’t have captured the unused locals either so the drop order would be the same.

It was only because async fn looked like a function that we wanted to capture all the arguments and have the drop order match an fn, I don’t think the same would apply to async closures.

It’s early here and I’ve just started thinking, so I could be wrong.

eddyb (May 29 2019 at 16:31, on Zulip):

not captures, but arguments - i.e. the a, b, c in async |a, b, c|

eddyb (May 29 2019 at 16:31, on Zulip):

if there are no captures, closures and functions should behave the same (cc @Taylor Cramer)

eddyb (May 29 2019 at 16:32, on Zulip):

@davidtwco I have to finish some other stuff, but if you want to have a stab at this: https://github.com/rust-lang/rust/pull/61276/commits/029ba3b27efdf67b9968be1f27be7a2e881d491c#diff-64b696b0ef6ad44140e973801ed82b25R3016

eddyb (May 29 2019 at 16:33, on Zulip):

I moved things around so you get to make both the arguments and expression, yourself, with lower_body

eddyb (May 29 2019 at 16:33, on Zulip):

right now I basically reverted most of the relevant changes, so the tests you added don't pass anymore

davidtwco (May 29 2019 at 16:33, on Zulip):

I just got the email from GitHub, I’ll take a look.

eddyb (May 29 2019 at 16:35, on Zulip):

arguments would need to become the __arg0 etc. and then the original arguments would go into the lets that would be prepended to the body

davidtwco (May 29 2019 at 16:36, on Zulip):

Great, thanks.

eddyb (May 29 2019 at 16:36, on Zulip):

everything should fit nicely into that one function

davidtwco (May 29 2019 at 16:36, on Zulip):

:tada:

eddyb (May 29 2019 at 16:37, on Zulip):

also I expect that ArgSource would need to be drastically changed, or at least the P<Pat> be replaced with a HirId

eddyb (May 29 2019 at 16:37, on Zulip):

as it stands, you're cloning the AST, which is a big nono

eddyb (May 29 2019 at 16:37, on Zulip):

patterns can contain types which can contain expressions which can contain blocks, which can contain items

eddyb (May 29 2019 at 16:38, on Zulip):

cloning items is pretty bad, even if nobody casually stashes an entire module in a pattern

davidtwco (May 29 2019 at 16:38, on Zulip):

Alright, I’ll make sure to do that.

eddyb (May 29 2019 at 16:39, on Zulip):

(yes, macros can do it, but the compiler shouldn't)

eddyb (May 29 2019 at 16:39, on Zulip):

anyway, I'm sorry it took me this long, and that I didn't finish the important part, but all of this stuff is doing my head in

eddyb (May 29 2019 at 16:40, on Zulip):

@davidtwco do you want a rebase, btw?

eddyb (May 29 2019 at 16:40, on Zulip):

before you start messing with this

Taylor Cramer (May 29 2019 at 16:41, on Zulip):

@davidtwco @eddyb async closures aren't being considered for stabilization right now

Taylor Cramer (May 29 2019 at 16:41, on Zulip):

they're really broken

eddyb (May 29 2019 at 16:41, on Zulip):

ah I see

davidtwco (May 29 2019 at 16:41, on Zulip):

anyway, I'm sorry it took me this long, and that I didn't finish the important part, but all of this stuff is doing my head in

No, no, I appreciate you putting the time in to help tidy this up.

davidtwco do you want a rebase, btw?

I can handle that if you want to get onto something else.

Taylor Cramer (May 29 2019 at 16:41, on Zulip):

because you need to be able to borrow from captures in the return future

Taylor Cramer (May 29 2019 at 16:41, on Zulip):

but you cannot today

eddyb (May 29 2019 at 16:41, on Zulip):

I have this in a different worktree so I can just rebase it while I do other stuff

eddyb (May 29 2019 at 16:42, on Zulip):

@Taylor Cramer you mean like fn call(&'a self) -> Something<'a>? similar to the streaming iterator problem?

eddyb (May 29 2019 at 16:44, on Zulip):

because by-ref captures don't run into that, I'm pretty sure || &foo works and it basically just copies the captured reference (but move || &foo doesn't work)

eddyb (May 29 2019 at 16:46, on Zulip):

@davidtwco oh wow the conflict was with my own small s/TraitOrImpl/Assoc PR which I made because I saw that while I was working on this PR

eddyb (May 29 2019 at 16:47, on Zulip):

@davidtwco pushed, I'll let you know if the build fails and I have to fix anything

Taylor Cramer (May 29 2019 at 16:51, on Zulip):

@eddyb that doesn't work if e.g. the argument to the closure is a reference, and you want the return type to depend on the lifetime of the reference (which is always the case with async closures that take a reference arg)

eddyb (May 29 2019 at 16:57, on Zulip):

you mean, like, |a| &*a?

eddyb (May 29 2019 at 16:57, on Zulip):

this... should work just as well with references as it does with functions - only captures have it worse

eddyb (May 29 2019 at 16:58, on Zulip):

or are you referring to the inference of the return type, because you don't have the explicit signature?

Taylor Cramer (May 29 2019 at 17:02, on Zulip):

@eddyb there isn't a way to write the bound for that closure return type so that it allows it to return a type implementing Future that is dependent upon the HRTB input lifetime

eddyb (May 29 2019 at 17:04, on Zulip):

you mean like for<'a> |x: &'a T| -> impl Future<Output = Foo<'a>>?

eddyb (May 29 2019 at 17:04, on Zulip):

both for<...> and -> impl Trait are missing for closures, right?

eddyb (May 29 2019 at 17:06, on Zulip):

ooooh by bound you mean a function can't take such a closure

eddyb (May 29 2019 at 17:07, on Zulip):

because F: for<'a> FnOnce(&'a T) -> X<'a> requires HKT

eddyb (May 29 2019 at 17:08, on Zulip):

and while F: for<'a> FnOnce<(&'a T,)>, for<'a> <F as FnOnce<(&'a T,)>>::Output: Future<Output = Foo<'a>> might work, it's unstable

eddyb (May 29 2019 at 17:10, on Zulip):

this problem is not limited to closures, if you wanted a function generic over an async fn, you'd run into the same issue

Nemo157 (May 30 2019 at 01:26, on Zulip):

@eddyb the common case is for<'a> |x: &'a T| -> impl Future<Output = Foo> + 'a, where the future borrows from the arguments but the resulting value doesn’t

Nemo157 (May 30 2019 at 01:26, on Zulip):

But presumably your case could exist as well

eddyb (May 30 2019 at 01:28, on Zulip):

I see, that still requires -> X<'a> it's just that <X<'a> as Future>::Output doesn't contain 'a

eddyb (May 30 2019 at 01:29, on Zulip):

so with the Fn(...) -> ... sugar, it would seem to require HKT

eddyb (May 30 2019 at 01:29, on Zulip):

(this feels more and more offtopic, whoooops)

davidtwco (May 31 2019 at 18:27, on Zulip):

FYI @eddyb, I started working on this today and I'm making decent progress.

I think it might make more sense if you backed out the async changes from your current PR and then I'll open one soon (hopefully today/tomorrow) that is based on your PR and still has your commit w/ the async reverts.

davidtwco (May 31 2019 at 21:04, on Zulip):

Submitted #61413 with what I have @eddyb.

eddyb (Jun 01 2019 at 07:50, on Zulip):

@davidtwco okay I'll do just that! :D

eddyb (Jun 01 2019 at 15:12, on Zulip):

oops, I got side-tracked by mangling stuff and forgot

eddyb (Jun 01 2019 at 17:53, on Zulip):

@davidtwco I rebased my PR and r=@Vadim Petrochenkov'd it, and I'm now reviewing your PR

eddyb (Jun 01 2019 at 17:53, on Zulip):

the good news is that ArgSource might be almost unnecessary!

eddyb (Jun 01 2019 at 17:54, on Zulip):

scratch that, rustdoc does the same thing everyone else does

eddyb (Jun 01 2019 at 17:55, on Zulip):

so all you need to do is use the same name as the original pattern, if it's a simple name

davidtwco (Jun 01 2019 at 17:55, on Zulip):

I think we already do that.

davidtwco (Jun 01 2019 at 17:55, on Zulip):

Unless I misunderstand what you mean.

eddyb (Jun 01 2019 at 17:55, on Zulip):

I mean the __arg0 thing

davidtwco (Jun 01 2019 at 17:56, on Zulip):

Yeah. We don’t change it to __arg0 if it is a simple binding (not incl ref bindings).

eddyb (Jun 01 2019 at 17:56, on Zulip):

so then you don't need all of that stuff to get the original pattern

eddyb (Jun 01 2019 at 17:57, on Zulip):

actually, this is HIR lowering, you don't even need __arg0, you could probably just leave it sym::invalid or w/e

eddyb (Jun 01 2019 at 17:57, on Zulip):

actually I don't know nowadays what it is

davidtwco (Jun 01 2019 at 17:58, on Zulip):

I had introduced ArgSource before we started keeping the names the same so I’ll give that a shot.

eddyb (Jun 01 2019 at 17:59, on Zulip):

do you need https://github.com/rust-lang/rust/pull/61413/commits/b91cfd9ceda9b580ccfa8a712817c3b7b3f60721#diff-64b696b0ef6ad44140e973801ed82b25R3090?

eddyb (Jun 01 2019 at 18:00, on Zulip):

seems like at least parts of that might be methods on this

eddyb (Jun 01 2019 at 18:01, on Zulip):

(maybe I should also look at how LocalSource is used, it could be similar)

davidtwco (Jun 01 2019 at 18:01, on Zulip):

If I didn’t pass it in then I’d get borrow checking errors for uses of this within the closure when it was captured.

eddyb (Jun 01 2019 at 18:02, on Zulip):

no, I mean, creating HIR by hand like that

eddyb (Jun 01 2019 at 18:03, on Zulip):

instead of this.foo_bar(...) of which there are dozens of helper methods :P

davidtwco (Jun 01 2019 at 18:03, on Zulip):

I wasn’t aware there were any. I’ll take a look in that case.

eddyb (Jun 01 2019 at 18:03, on Zulip):

look at the other desugarings, like for loops or ?

eddyb (Jun 01 2019 at 18:04, on Zulip):

you might want to also copy how they document each part

eddyb (Jun 01 2019 at 18:04, on Zulip):

and generally the style (sorry I didn't mention them earlier)

eddyb (Jun 01 2019 at 18:05, on Zulip):

anyway this looks great and pretty self-contained!

davidtwco (Jun 01 2019 at 18:05, on Zulip):

That’s fine, I should have noticed them, I’ll have almost certainly looked at those functions during this.

eddyb (Jun 01 2019 at 18:05, on Zulip):

oh and you can rebase --onto the current state of my PR, if you want to

davidtwco (Jun 01 2019 at 18:06, on Zulip):

Will do, thanks.

eddyb (Jun 02 2019 at 15:51, on Zulip):

@davidtwco my PR got merged btw

davidtwco (Jun 02 2019 at 15:51, on Zulip):

I saw, just rebased, then will address the feedback and update my PR.

eddyb (Jun 02 2019 at 15:51, on Zulip):

I should not forget to clean up visit_fn in rustc_resolve

eddyb (Jun 02 2019 at 15:52, on Zulip):

basically the caller should be pushing (Assoc)ItemRibKind

eddyb (Jun 02 2019 at 15:55, on Zulip):

although the closure case is annoying, I think we should actually always push a NormalRibKind (i.e. a block-like scope) for the argument bindings since even for item fn's there are things between the item scope and the argument scope (generics and the signaure)

eddyb (Jun 02 2019 at 15:58, on Zulip):

@varkor might like this: if Foo<T, N> starts working (without an anon const expr in the AST), we don't want x: Foo<T, x> to see the name x at all

eddyb (Jun 02 2019 at 20:30, on Zulip):

@davidtwco are there diagnostic uses of {Arg,Local}Source left?

davidtwco (Jun 02 2019 at 21:42, on Zulip):

Not that I’m aware of, no.

eddyb (Jun 03 2019 at 09:57, on Zulip):

@davidtwco so can they be fully removed?

davidtwco (Jun 03 2019 at 09:57, on Zulip):

I thought it might make sense to keep some breadcrumbs that it was introduced by a desugaring.

davidtwco (Jun 03 2019 at 09:57, on Zulip):

Though I suppose the spans might do that.

eddyb (Jun 03 2019 at 09:57, on Zulip):

the expansion span already records that

eddyb (Jun 03 2019 at 09:57, on Zulip):

heh

davidtwco (Jun 03 2019 at 10:00, on Zulip):

I'll remove them in a moment.

davidtwco (Jun 03 2019 at 10:00, on Zulip):

@eddyb Should the LocalSource variant remain since LocalSource already existed?

eddyb (Jun 03 2019 at 10:16, on Zulip):

I guess that's fine

eddyb (Jun 03 2019 at 10:16, on Zulip):

assuming things do check for Normal and suppress some diagnostics etc.

Last update: Nov 18 2019 at 00:45UTC