Stream: t-compiler/wg-nll

Topic: #46589 kill borrows after assignment


davidtwco (Dec 06 2018 at 15:20, on Zulip):

@nikomatsakis Picked up this issue today to finish it off, doesn't look like there's much to do. I've got a fix locally that's a really minor change but it isn't what you suggested in the issue, so just want to double check I've not did something too naive.

Am I right in understanding that the original issue was that if you assigned into a variable then that didn't kill any borrows of a projection of that variable? And what wasn't then finished was that if you assigned into a projection, instead of directly into a local, then it still doesn't kill any borrows.

I've got a naive fix that effectively just makes it work for deref projections too - this removes the error on the remaining test case you had in a comment. Am I right in my understanding that we don't actually want all borrows of a local to get killed for other types of projections - eg. field accesses, indexing? It doesn't touch the place conflict code that you suggested (though I did implement the change you suggested before being unable to work out where it fit into the problem).

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

I'll have to refresh my memory, @davidtwco

davidtwco (Dec 06 2018 at 15:58, on Zulip):

No worries, appreciate you taking a look.

davidtwco (Dec 09 2018 at 18:24, on Zulip):

(just submitted a WIP PR with what I had for this, only because I'll forget I did it otherwise)

Matthew Jasper (Dec 09 2018 at 18:56, on Zulip):

Am I right in my understanding that we don't actually want all borrows of a local to get killed for other types of projections - eg. field accesses, indexing?

Definitely not. Assigning to x.fshould not kill loans of *x.g, since this would allow a second mutable borrow of *x.g.

davidtwco (Dec 09 2018 at 19:05, on Zulip):

Definitely not. Assigning to x.fshould not kill loans of *x.g, since this would allow a second mutable borrow of *x.g.

Does that mean my understanding is definitely not correct, or we definitely don't want all borrows killed for other types of projections?

davidtwco (Dec 09 2018 at 19:06, on Zulip):

I'm having a real hard time parsing sentences today and I don't know why.

Matthew Jasper (Dec 09 2018 at 19:15, on Zulip):

We definitely don't want assigning to a projection to kill all borrows of places with the same base.

davidtwco (Dec 10 2018 at 07:45, on Zulip):

I think I realised what was intended by Niko’s notes on the issue last night. Hold off on reviewing until I’ve had a chance to revisit the PR. :face_palm:🏻‍♂️

nikomatsakis (Dec 12 2018 at 20:33, on Zulip):

I think I meant for assignments to x.f to kill all borrows of x.f.*, basically

davidtwco (Dec 12 2018 at 20:35, on Zulip):

The main thing I wasn't sure of was, after generalizing the conflicting places logic like you described, what place should I check for conflict with?

davidtwco (Dec 12 2018 at 20:35, on Zulip):

I've tried a few things and couldn't find something that seemed quite right.

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

remind me @davidtwco, is there a PR? it's a bit out of cache but I can take a look

davidtwco (Dec 12 2018 at 21:01, on Zulip):

There is, but it's with the initial naive fix that didn't do anything with places conflicting.

davidtwco (Dec 12 2018 at 21:01, on Zulip):

I've not pushed my experimenting with the place conflict code route yet.

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

ok well give me a sec and I can maybe remember :)

davidtwco (Dec 12 2018 at 21:05, on Zulip):

I mostly experimented with doing the place conflict with the base local being assigned into and then the actual place that was being assigned into.

davidtwco (Dec 12 2018 at 21:07, on Zulip):

So, _1 and (*(_1.0)) for example. Hoping that it would consider something like _1 and *(_1) in conflict and therefore can be killed and something like _1 and (*(_1.0)) not and therefore not killed. But it didn't quite work out like that - it was close. After tweaking the BorrowKind and AccessDepth parameters a bunch (since they don't really make sense in this context as far as I understand) I got it so that one or the other worked.

davidtwco (Dec 12 2018 at 21:08, on Zulip):

(or something along those lines, I've still got the working directory so I can check)

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

I'm a bit confused

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

ah, you are considering using that function to decide whether something is killed?

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

I think that's probably not the right one to use

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

I think the RFC basically imagined just using a strict notion of "prefix" -- i.e., if you assign to some place P0, then you find all loans of any Place with P0 as a base and kill them

davidtwco (Dec 12 2018 at 21:11, on Zulip):

You mentioned it in the issue, so figured I'd need it somewhere, couldn't think of where initially and then a few days later thought "hmm, maybe it is for deciding to kill things" and experimented with that.

davidtwco (Dec 12 2018 at 21:11, on Zulip):

That's what my understanding was.

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

this sort of kills more than necessary, but in some of those cases the assignment itself is illegal

davidtwco (Dec 12 2018 at 21:11, on Zulip):

I just tried to find a way to use that function.

davidtwco (Dec 12 2018 at 21:11, on Zulip):

Since it was mentioned.

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

well

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

let me re-read the issue :)

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

ok, ok, right

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

I'm trying to decide if I was right to mention that function

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

in other words: what I wrote is all true

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

well I guess it's just that we don't have an efficient way to compare places per se

davidtwco (Dec 12 2018 at 21:14, on Zulip):

I tried to look through Gitter to see if there was some context of how it would be used that the issue assumed as known, but couldn't see anything.

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

apart from places_conflict

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

but places_conflict is really written (as I wrote) for a different purpose:

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

it is used to say: if you have a loan of the place P and an access to the place P2, does that access potentially overlap with the borrowed place P?

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

naturally that wants to be conservative: if P2 may overlap P, it returns true

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

anyway I suspect you just want to write a separate function in the end

davidtwco (Dec 12 2018 at 21:16, on Zulip):

Yeah, I think I've implemented giving that function a bias toward overlapping or not (at least for the j[x] case).

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

something like

assignment_kills_loan(
    assigned_place: &Place,
    borrowed_place: &Place,
) -> bool {
  ..
}
davidtwco (Dec 12 2018 at 21:17, on Zulip):

Where would I get the borrowed_place from?

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

well e.g. in the polonius code you can find it in the loop, I'm trying to find the other code

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

(incidentally, I think that Simon's example from the issue is not going to be fixed by this issue, but rather requires polonius)

davidtwco (Dec 12 2018 at 21:18, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/dataflow/impls/borrows.rs#L238-L242

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

right, just found it

davidtwco (Dec 12 2018 at 21:19, on Zulip):

(I had a working fix for that in the PR as it is by just extending the existing code to accept *_1 as a kill for _1 as well as _1 - not sure if that is correct though, but only that test changed)

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

the naive thing of course would be to iterate over all the borrows

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

but that will kill perf

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

I suppose we'd want to index them in some way such that we can walk down the assigned path

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

it feels like a lot of work :) but an ideal thing would be some sort of tree I guess

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

this would all be easier/nicer if @csmoe's refactoring were landed -- I need to get back to them on that :smile:

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

so e.g. if there were borrows of a.b and a.c and a.c.d, you could imagine some sort of tree like:

a
|- b
|- c
   |- d

then when you have an assignment to (say) a.c, you'd walk down to that node and kill all the borrows in that node or its descendants

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

(gotta run)

davidtwco (Dec 17 2018 at 12:31, on Zulip):

I've updated this PR so the approach it takes is now much more reasonable. There's one test that has less errors that isn't ideal, but I'm not really sure how to work around that case - it isn't wrong necessarily, but it's not great.

pnkfelix (Dec 17 2018 at 13:20, on Zulip):

I wrote a few more comments (on the issue), mostly explaining the code to myself as I went

pnkfelix (Dec 17 2018 at 13:21, on Zulip):

The only real issue I have is the one about whether we need to look at propagate_call_return as well. But to be honest, that might be left over (as in, whatever earlier PR started inspecting the Local should also have made changes to propagate_call_return ...

pnkfelix (Dec 17 2018 at 13:22, on Zulip):

I think your PR as it stands is sound. And given the conditional nature of the overwrite caused in propagate_call_return, I think you should not do the similar kill_borrows over there. Update: no, I don't know what to think yet....

pnkfelix (Dec 17 2018 at 13:22, on Zulip):

So I think I just answered my own Q.

pnkfelix (Dec 17 2018 at 13:24, on Zulip):

A bit of explanation here: propagate_call_return is a hack that is working around the fact that our data-flow system is not designed to be as general purpose as we really require.

pnkfelix (Dec 17 2018 at 13:25, on Zulip):

In particular, we (I) designed it so that most of the interesting effects on the dataflow bitsets occur at the statement level, as we progress directly from statement to statement

pnkfelix (Dec 17 2018 at 13:26, on Zulip):

The problem with that model is that a Terminator::Call has a side-effect that is control-flow dependent: On a successful call, it will overwrite the given dest_place. On a call that unwinds (e.g. due to panic), it will not overwrite the given dest_place.

pnkfelix (Dec 17 2018 at 13:26, on Zulip):

So the hack is that in the dataflow system itself, we add in those control-flow edge effects while we are doing the fixed-point iteration.

pnkfelix (Dec 17 2018 at 13:27, on Zulip):

that (if my memory correctly matches the current state of the code) is the purpose of propagate_call_return.

davidtwco (Dec 17 2018 at 13:28, on Zulip):

Let me just catch up with what you've written.

pnkfelix (Dec 17 2018 at 13:30, on Zulip):

In particular, propagate_call_return is only invoked to update the dataflow bitset when the control flow traverses from the call's basic block (every call is a terminator, so there is a mapping from every call to its unique BB) to the basic block that we jump to on successful (non-unwinding) call.

pnkfelix (Dec 17 2018 at 13:31, on Zulip):

And the main effect you want to model in that scenario is that the dest_place will be overwritten, just as it is by an Assign statement.

pnkfelix (Dec 17 2018 at 13:32, on Zulip):

(I'm starting to wonder if we'd just be better served with a default method in the DataflowAnalysis trait that just said "this is called for assignment-like things, namely Assign statements and a successful return from a Call terminator" ...)

pnkfelix (Dec 17 2018 at 13:33, on Zulip):

So this is all a long-winded way of me saying: "I think the same code that you've added to the Assign case might also be good to add to the propagate_call_return method." It should be sound for you to leave it out, but I think it will also be sound for you to put it in, and will lead to us accepting more code.

pnkfelix (Dec 17 2018 at 13:33, on Zulip):

But to confirm this I guess I should make some illustrative example.

pnkfelix (Dec 17 2018 at 13:50, on Zulip):

@davidtwco I know I wrote a lot (both here and on the PR) and maybe you want to finish reading it all before you answer Q's, but here's maybe a simple one: Would you prefer me to r+ this one now, and leave the propagate_call_return generalization for later? Or do you feel like trying to throw that in with a follow-on commit + testcase?

davidtwco (Dec 17 2018 at 13:50, on Zulip):

Pushed a commit with better comments.

davidtwco (Dec 17 2018 at 13:50, on Zulip):

I'm happy to add it in.

davidtwco (Dec 17 2018 at 13:50, on Zulip):

It doesn't sound like a large change.

pnkfelix (Dec 17 2018 at 13:50, on Zulip):

Yeah I think the hardest part might be adding a test

pnkfelix (Dec 17 2018 at 13:50, on Zulip):

since I'm not certain whether you can even generate code that would get into the case

pnkfelix (Dec 17 2018 at 13:51, on Zulip):

I.e. I may be wrong but based on the way we generated MIR today, calls might always write into locals

davidtwco (Dec 17 2018 at 13:51, on Zulip):

In that case, r+ and file a follow-up if you're happy with the new comments?

pnkfelix (Dec 17 2018 at 13:51, on Zulip):

(but then again ... there's not even code to handle locals yet in propagate_call_return, right? That's sort of what I was getting at when I said I wasn't sure whether people in the past had been acknowledging the comment in the first place.)

davidtwco (Dec 17 2018 at 13:52, on Zulip):

It's really easy to glaze over and not really notice those comments if they don't seem immediately relevant.

pnkfelix (Dec 17 2018 at 13:52, on Zulip):

I know.

pnkfelix (Dec 17 2018 at 13:52, on Zulip):

I'm guilty of it too

davidtwco (Dec 17 2018 at 13:55, on Zulip):

Does this demonstrate your case?

#![allow(warnings)]
#![feature(rustc_attrs)]

fn main() {}

fn nll_fail() {
    let mut data = ('a', 'b', 'c');
    let c = &mut data.0;
    capitalize(c);
    data.0 = capitalize(&mut 'e'); // one error here if assign 'e' directly, one error here if call
    data.0 = capitalize(&mut 'f'); // one error here if call
    data.0 = capitalize(&mut 'g'); // one error here if call
    capitalize(c);
}

fn capitalize(_: &mut char) -> char {
    'f'
}

It's similar to the loan_ends_mid_block_pair test - but instead of assigning characters directly into data.0 (which as of this PR, produces one error), it produces three errors when replaced with a call?

pnkfelix (Dec 17 2018 at 13:56, on Zulip):

Hmm. So am I right in my understanding that this change didn't turn any test from compile-fail into run-pass, and the only place where you saw anything like that was in src/test/ui/nll/loan_ends_mid_block_pair.rs (where you removed two Mir cases in the borrowck=compare output) ?

davidtwco (Dec 17 2018 at 13:56, on Zulip):

Yeah.

pnkfelix (Dec 17 2018 at 13:57, on Zulip):

It's similar to the loan_ends_mid_block_pair test - but instead of assigning characters directly into data.0 (which as of this PR, produces one error), it produces three errors when replaced with a call?

Yes I think that (putting a call on the RHS rather than a more-trivial expression) is exactly what I expected to illustrate the difference.

davidtwco (Dec 17 2018 at 13:57, on Zulip):

I wasn't sure that was better, but couldn't see a way around it.

pnkfelix (Dec 17 2018 at 13:57, on Zulip):

You should add the example (perhaps adapted in some way) from #46589 as an explicit run-pass test, IMO.

pnkfelix (Dec 17 2018 at 13:58, on Zulip):

i.e. solely relying on the change from load_ends_mid_block_pair to illustrate/test the increased expressiveness here is too subtle, in my opinion.

davidtwco (Dec 17 2018 at 13:58, on Zulip):

Yeah, the test as was there had another error that remained after this.

davidtwco (Dec 17 2018 at 13:58, on Zulip):

There is a test from that issue that I added.

davidtwco (Dec 17 2018 at 13:58, on Zulip):

Just not as run-pass.

pnkfelix (Dec 17 2018 at 13:58, on Zulip):

right, the one you added was a compile-fail to double-check that we don't generalize too much, right?

davidtwco (Dec 17 2018 at 13:59, on Zulip):

No, it's exactly what was on the issue, it removed the error that it was supposed to with assigning into a deref projection but there was another unrelated error that test case would cause.

pnkfelix (Dec 17 2018 at 13:59, on Zulip):

what about the example that SimonSapin had posted to that issue? Does that pass under your branch, or does it still error then too?

davidtwco (Dec 17 2018 at 13:59, on Zulip):

(as far as I understood it)

davidtwco (Dec 17 2018 at 13:59, on Zulip):

Forgot to try that one.

pnkfelix (Dec 17 2018 at 14:00, on Zulip):

In some ways thats the most important one to get working, since that represents a real "customer request" :smile:

davidtwco (Dec 17 2018 at 14:00, on Zulip):

It looks like it gets the same error.

pnkfelix (Dec 17 2018 at 14:01, on Zulip):

ah well that's too bad.

davidtwco (Dec 17 2018 at 14:02, on Zulip):

I'll look into it and see if I can't add something to propagate_call_return for that too.

pnkfelix (Dec 17 2018 at 14:02, on Zulip):

let me look again at the test you added and see if I can tease out a run-pass test that won't hit the error then

pnkfelix (Dec 17 2018 at 14:02, on Zulip):

oh also, if you can

pnkfelix (Dec 17 2018 at 14:02, on Zulip):

try to make a different title for your PR than the text of the issue it fixes

davidtwco (Dec 17 2018 at 14:03, on Zulip):

Sure, that's all I've done for the last 50 PRs or something because it meant I didn't need to get creative.

pnkfelix (Dec 17 2018 at 14:03, on Zulip):

I think bors includes the title in the commit message, but more importantly, different text helps people like me who have the issue and the PR in two different tabs in one browser window.

davidtwco (Dec 17 2018 at 14:04, on Zulip):

Fair enough, I'll do that in future.

davidtwco (Dec 17 2018 at 14:07, on Zulip):

It's annoying that propagate_call_return takes a BitSet rather than the BlockSets of statement_effect.

davidtwco (Dec 17 2018 at 14:07, on Zulip):

Makes this a tad more work.

pnkfelix (Dec 17 2018 at 14:07, on Zulip):

Well

pnkfelix (Dec 17 2018 at 14:07, on Zulip):

I think the intent was that the effect of a call could be non-local

davidtwco (Dec 17 2018 at 14:07, on Zulip):

Yeah, it makes sense, just adds a little friction to this change.

pnkfelix (Dec 17 2018 at 14:08, on Zulip):

but maybe it should have done the look up for the blockset for the call block and the dest block

pnkfelix (Dec 17 2018 at 14:08, on Zulip):

and passed those two blocks in

davidtwco (Dec 17 2018 at 14:18, on Zulip):

Ran into an error that might need improved:

error[E0521]: borrowed data escapes outside of function
   --> src/librustc_mir/dataflow/impls/borrows.rs:391:9
    |
385 |         &self,
    |         ----- `self` is declared here, outside of the function body
...
389 |         dest_place: &mir::Place,
    |         ---------- `dest_place` is a reference that is only valid in the function body
390 |     ) {
391 |         self.kill_borrows_on_call_return(in_out, dest_place);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `dest_place` escapes the function body here
davidtwco (Dec 17 2018 at 14:18, on Zulip):

Doesn't make sense to me that parameters should simultaneously be in and outside the function body.

pnkfelix (Dec 17 2018 at 14:18, on Zulip):

You know its possible fixing the propagate_call_return could help with the test you added, since that is a method call happening in None => (*other).new_self()... not sure yet.

davidtwco (Dec 17 2018 at 14:41, on Zulip):

I'm finding the borrow checker surprisingly impenetrable being able to call a function from this propagate_call_return function.

pnkfelix (Dec 17 2018 at 14:43, on Zulip):

okay. Maybe just file a follow-up issue and don't worry about this now.

davidtwco (Dec 17 2018 at 14:53, on Zulip):

It should be easy enough to do, but I just can't get the lifetimes to work out in order to use self from propagate_call_return.

pnkfelix (Dec 17 2018 at 14:56, on Zulip):

I assume you've already skimmed over other impls of fn propagate_call_return for inspiration?

davidtwco (Dec 17 2018 at 14:57, on Zulip):

Yeah, they either don't use dest_place or call some function on it to get owned data (such as MoveData) and then use that in combination with something from self and then that seems to work.

davidtwco (Dec 17 2018 at 15:03, on Zulip):

I think it ends up being this that is the issue:

error: unsatisfied lifetime constraints
   --> src/librustc_mir/dataflow/impls/borrows.rs:398:55
    |
286 | impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> {
    |      -- lifetime `'a` defined here
...
396 |         dest_place: &mir::Place,
    |         ---------- has type `&rustc::mir::Place<'1>`
397 |     ) {
398 |         kill_borrows_on_call_return(&self.borrow_set, self.tcx, self.mir, in_out, dest_place);
    |                                                       ^^^^^^^^ copying this value requires that `'1` must outlive `'a`
davidtwco (Dec 17 2018 at 15:03, on Zulip):

(after moving things around to get rid of some errors)

davidtwco (Dec 17 2018 at 15:06, on Zulip):

If I manually specify Place<'tcx> then I can never get it to match what the trait expects, no matter what I do.

davidtwco (Dec 17 2018 at 15:07, on Zulip):
error[E0308]: method not compatible with trait
   --> src/librustc_mir/dataflow/impls/borrows.rs:343:5
    |
343 | /     fn propagate_call_return(
344 | |         &self,
345 | |         set: &mut BitSet<BorrowIndex>,
346 | |         _call_bb: mir::BasicBlock,
...   |
386 | |         }
387 | |     }
    | |_____^ lifetime mismatch
    |
    = note: expected type `fn(&dataflow::impls::borrows::Borrows<'a, 'gcx, 'tcx>, &mut rustc_data_structures::bit_set::BitSet<dataflow::move_paths::indexes::BorrowIndex>, rustc::mir::BasicBlock, rustc::mir::BasicBlock, &rustc::mir::Place<'tcx>)`
               found type `fn(&dataflow::impls::borrows::Borrows<'a, 'gcx, 'tcx>, &mut rustc_data_structures::bit_set::BitSet<dataflow::move_paths::indexes::BorrowIndex>, rustc::mir::BasicBlock, rustc::mir::BasicBlock, &rustc::mir::Place<'tcx>)`
note: the lifetime 'tcx as defined on the method body at 343:5...
   --> src/librustc_mir/dataflow/impls/borrows.rs:343:5
    |
343 | /     fn propagate_call_return(
344 | |         &self,
345 | |         set: &mut BitSet<BorrowIndex>,
346 | |         _call_bb: mir::BasicBlock,
...   |
386 | |         }
387 | |     }
    | |_____^
note: ...does not necessarily outlive the lifetime 'tcx as defined on the impl at 238:16
   --> src/librustc_mir/dataflow/impls/borrows.rs:238:16
    |
238 | impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> {
pnkfelix (Dec 17 2018 at 15:12, on Zulip):

hmm. something seems sketchy there

davidtwco (Dec 17 2018 at 15:12, on Zulip):

Yeah, my ability to decode lifetime errors degrades quickly in cases like this.

pnkfelix (Dec 17 2018 at 15:14, on Zulip):

hmm

pnkfelix (Dec 17 2018 at 15:14, on Zulip):

I wonder if the problem is that the trait definiton

pnkfelix (Dec 17 2018 at 15:14, on Zulip):

should be using

pnkfelix (Dec 17 2018 at 15:14, on Zulip):

Place<'tcx>

pnkfelix (Dec 17 2018 at 15:15, on Zulip):

on the header for propagate_call_return ?

davidtwco (Dec 17 2018 at 15:15, on Zulip):

That second error is what I get when I change it to that.

davidtwco (Dec 17 2018 at 15:15, on Zulip):

(and then update my impl of it to match)

pnkfelix (Dec 17 2018 at 15:15, on Zulip):

don't you have to update all the other impls too?

davidtwco (Dec 17 2018 at 15:16, on Zulip):

The first error is when I'm (without having changed the signature) just trying to pass in the relevant information to a function.

davidtwco (Dec 17 2018 at 15:16, on Zulip):

I'd expect so, yeah. But I'd expect errors saying they don't match, instead of my impl not matching?

pnkfelix (Dec 17 2018 at 15:16, on Zulip):

true, that is what I'd expect too

davidtwco (Dec 17 2018 at 15:16, on Zulip):

Maybe if I do that it'll resolve itself, might be a worth a shot.

pnkfelix (Dec 17 2018 at 15:18, on Zulip):

Do you want me to r+ you current PR as is, in any case?

pnkfelix (Dec 17 2018 at 15:18, on Zulip):

(and you'll do this stuff in some follow-up?)

pnkfelix (Dec 17 2018 at 15:18, on Zulip):

Or shall we wait a bit to see if you work this lifetime issue out?

davidtwco (Dec 17 2018 at 15:18, on Zulip):

Before you do that, I'm looking at SimonSapin's test case and not sure if it is a instance of this issue.

pnkfelix (Dec 17 2018 at 15:18, on Zulip):

ah okay

davidtwco (Dec 17 2018 at 15:22, on Zulip):

If I update all of the impls, then they all complain that they don't match.

pnkfelix (Dec 17 2018 at 15:22, on Zulip):

interesting

pnkfelix (Dec 17 2018 at 15:23, on Zulip):

wait a minute... does this trait definition even have a 'tcx on it?

pnkfelix (Dec 17 2018 at 15:23, on Zulip):

/me looks some more

davidtwco (Dec 17 2018 at 15:23, on Zulip):

Don't think so.

pnkfelix (Dec 17 2018 at 15:23, on Zulip):

well then what does 'tcx even mean in that context...?

pnkfelix (Dec 17 2018 at 15:23, on Zulip):

that might be the source of the problem here

pnkfelix (Dec 17 2018 at 15:24, on Zulip):

i.e. this may be a combination of that new lifetime header feature

pnkfelix (Dec 17 2018 at 15:24, on Zulip):

where 'tcx gets implicitly bound at teh method level (I think)

pnkfelix (Dec 17 2018 at 15:24, on Zulip):

if you don't explicitly declare it otherwise?

pnkfelix (Dec 17 2018 at 15:24, on Zulip):

in other words, you might need to change trait BitDenotation to trait BitDenotation<'tcx>

davidtwco (Dec 17 2018 at 15:24, on Zulip):

I'll go through and try that then.

pnkfelix (Dec 17 2018 at 15:25, on Zulip):

(and lordy that means a lot more churn)

pnkfelix (Dec 17 2018 at 15:25, on Zulip):

seems like all of this is going to be churn that may not buy us anything

pnkfelix (Dec 17 2018 at 15:25, on Zulip):

but we'll see

pnkfelix (Dec 17 2018 at 15:25, on Zulip):

in any case, the exercise may be good

pnkfelix (Dec 17 2018 at 15:25, on Zulip):

since the frustration you had with the error diagnostic above

pnkfelix (Dec 17 2018 at 15:26, on Zulip):

indicates that we need to improve our diagnostic reporting here a lot. This is a new feature which means no one may be prepared for this kind of error when we hit it.

davidtwco (Dec 17 2018 at 15:27, on Zulip):

Yeah, there's definitely work to do with errors like that.

davidtwco (Dec 17 2018 at 15:48, on Zulip):

So, did that and down to two lifetime errors relating to closures in the graphviz code - which anecdotally whenever I make a change like this, it always feels like it ends up in the graphviz code.

pnkfelix (Dec 17 2018 at 15:57, on Zulip):

I wish we had telemetry on how often that code is used

pnkfelix (Dec 17 2018 at 15:57, on Zulip):

I was probably its biggest client at one point but now ... who knows who uses it, if anyone.

davidtwco (Dec 17 2018 at 15:58, on Zulip):

Managed to get around it.

davidtwco (Dec 17 2018 at 15:59, on Zulip):

Got rustc_mir to compile with Place<'tcx>, so I suspect the lifetime situation is all resolved.

davidtwco (Dec 17 2018 at 16:13, on Zulip):

@pnkfelix the MIR of the test case for the calls instead of assignments ends up assigning into a local that the next block assigns into the field - so it doesn't end up using the code path for the propagate_call_return.

davidtwco (Dec 17 2018 at 16:14, on Zulip):

But, it still has three errors instead of the one I'd expect because the borrow indices set that we iterate over end up being empty - which is odd.

davidtwco (Dec 17 2018 at 16:14, on Zulip):

Looks like those sets only have the borrows from that block, which is why.

pnkfelix (Dec 17 2018 at 16:18, on Zulip):

Oh, well... propagate_call_return is called during data flow solving (the iteration to a fixed point)

pnkfelix (Dec 17 2018 at 16:19, on Zulip):

So I’d expect it to be called many times? And potentially the sets change between the calls...?

davidtwco (Dec 17 2018 at 16:19, on Zulip):

I'd like to keep the lifetime changes in because I think they're more correct and someone else will need to do it if they end up doing something similar in that function in future.

davidtwco (Dec 17 2018 at 16:20, on Zulip):

(that is, the BitDenotation<'tcx> change)

davidtwco (Dec 17 2018 at 16:25, on Zulip):

I'm fairly sure (would appreciate if you could take a look at confirm) that SimonSapin's test case isn't related. I could be wrong, but I don't think it is assigning into a projection of anything and then relying on that to kill a borrow.

davidtwco (Dec 17 2018 at 16:27, on Zulip):

Other than that, happy for you to r+ - just pushed a change with the lifetime fixes.

davidtwco (Dec 17 2018 at 16:34, on Zulip):

I'm fairly sure (would appreciate if you could take a look at confirm) that SimonSapin's test case isn't related. I could be wrong, but I don't think it is assigning into a projection of anything and then relying on that to kill a borrow.

If it is related then I think it is worth trying to fix that before landing?

nikomatsakis (Dec 17 2018 at 20:32, on Zulip):

what about the example that SimonSapin had posted to that issue? Does that pass under your branch, or does it still error then too?

I think that @simonsapin's example requires polonius

nikomatsakis (Dec 17 2018 at 20:34, on Zulip):

at least that was my belief when I last looked

Last update: Nov 21 2019 at 13:20UTC