Stream: t-compiler/wg-nll

Topic: #47349-suggestion-to-pull-out-subcomputation


Keith Yeung (Aug 06 2018 at 22:36, on Zulip):

okay, so i'm currently looking at this issue, and the first thing i'd like to know now is which function MIR borrowck calls in order to generate that error message

Keith Yeung (Aug 06 2018 at 22:37, on Zulip):

i currently have a few ideas: check_access_conflict or check_access_permissions

nikomatsakis (Aug 06 2018 at 23:08, on Zulip):

check_access_conflict, I believe

nikomatsakis (Aug 06 2018 at 23:09, on Zulip):

check_access_permissions is different -- it checks just one borrow to see if you have sufficient permissions to do that (e.g., an &mut borrow of something that is not mutable is just a plain error)

Keith Yeung (Aug 11 2018 at 18:53, on Zulip):

i'm forgetting things, but I recall that there's a BasicBlock data structure that i can access somewhere

Keith Yeung (Aug 11 2018 at 18:54, on Zulip):

bingo, it's MIR

Keith Yeung (Aug 11 2018 at 18:55, on Zulip):

i'm planning to use this to look for the next MIR statements that have a function call argument containing the "starting borrow's" temporary

Keith Yeung (Aug 12 2018 at 00:55, on Zulip):

actually, that is not necessary, because function calls are only done in terminators

Keith Yeung (Aug 12 2018 at 01:00, on Zulip):

now here comes a problem: i have two Places, and i want to know if one of them is assigned from another (or if they're both equal)

Keith Yeung (Aug 12 2018 at 01:04, on Zulip):

oh, looks like i can call base_path to achieve that effect

Keith Yeung (Aug 12 2018 at 01:34, on Zulip):

nope, that's only for dereffing Projections

nikomatsakis (Aug 12 2018 at 08:48, on Zulip):

what does "assigned from another" mean? can you give an example?

nikomatsakis (Aug 12 2018 at 08:48, on Zulip):

you might want places_conflict?

Keith Yeung (Aug 12 2018 at 09:46, on Zulip):

i meant "from the other"

Keith Yeung (Aug 12 2018 at 09:46, on Zulip):

i have two Places, and i want to determine whether there is a relationship between them

Keith Yeung (Aug 13 2018 at 03:00, on Zulip):

it doesn't look like places_conflict does what i need, since it doesn't trace the "assignment path" that a place took

Keith Yeung (Aug 13 2018 at 03:01, on Zulip):

here's an MIR example:

_2 = _1
_3 = _2
_4 = const Foo::foo(move _3)

Keith Yeung (Aug 13 2018 at 03:02, on Zulip):

now, suppose that i have _1 and _3

Keith Yeung (Aug 13 2018 at 03:02, on Zulip):

my objective here is to show that they're the same place

Keith Yeung (Aug 13 2018 at 03:06, on Zulip):

aside from that, I seem to be running into trouble getting the correct span for the function call i want

Keith Yeung (Aug 13 2018 at 03:06, on Zulip):
    buckets.slice_mut()[(key as usize).wrapping_add(22).wrapping_rem(buckets.sweep())] = 22;
Keith Yeung (Aug 13 2018 at 03:07, on Zulip):

for some reason, the basic block terminator's span underlines buckets.slice_mut() and not buckets.sweep()

Keith Yeung (Aug 13 2018 at 03:31, on Zulip):

huh, so i was looking at the wrong bb

Keith Yeung (Aug 13 2018 at 03:46, on Zulip):

also, i'm heavily overthinking this, the relevant data that i need is all in BorrowData

Keith Yeung (Aug 13 2018 at 05:28, on Zulip):

alright, PR out https://github.com/rust-lang/rust/pull/53305

Keith Yeung (Aug 16 2018 at 23:09, on Zulip):

@nikomatsakis I'm thinking that your suggestion as written doesn't quite work the way we want it

Keith Yeung (Aug 16 2018 at 23:10, on Zulip):

in my PR, i ignore the "starting borrow" altogether, and only focused on looking at the location that the conflicting borrow occured

Keith Yeung (Aug 16 2018 at 23:11, on Zulip):

that's obviously not the correct way, but factoring the "starting borrow" into consideration doesn't help much

Keith Yeung (Aug 16 2018 at 23:12, on Zulip):

because the lvalue on that starting borrow immediately gets used as an argument to a function, and so there's nothing in between the "starting borrow" and the error location

Keith Yeung (Aug 16 2018 at 23:14, on Zulip):

i suspect you wanted to say "the error case between the lvalue of the return value of the function and the time it's dropped", instead of "the error case between the starting borrow and the call"?

Keith Yeung (Aug 17 2018 at 00:17, on Zulip):

possibly also with the additional requirement that the return value of that function contains a lifetime associated with the "starting borrow"

nikomatsakis (Aug 21 2018 at 16:24, on Zulip):

hmm @Keith Yeung quite possibly so!

nikomatsakis (Aug 21 2018 at 16:25, on Zulip):

can I check your PR for context?

Keith Yeung (Aug 21 2018 at 16:48, on Zulip):

Yeah, it's https://github.com/rust-lang/rust/pull/53305

Keith Yeung (Aug 23 2018 at 21:34, on Zulip):

@nikomatsakis regarding the comments you left, i think what you have in mind is significantly less in scope than what I have imagined

Keith Yeung (Aug 23 2018 at 21:35, on Zulip):

because it seems that we're only interested in solving the cases where foo(xxx, ..., yyy) causes a problem

Keith Yeung (Aug 23 2018 at 21:36, on Zulip):

whereas in the problematic code where the issue was raised, the line of code is:

Keith Yeung (Aug 23 2018 at 21:36, on Zulip):
buckets.slice_mut()[(key as usize).wrapping_add(22).wrapping_rem(buckets.sweep())] = 22;
Keith Yeung (Aug 23 2018 at 21:36, on Zulip):

and this is a conflict between buckets.slice_mut() and buckets.sweep()

Keith Yeung (Aug 23 2018 at 21:37, on Zulip):

it does not follow the format of foo(xxx, ..., yyy)

nikomatsakis (Aug 23 2018 at 21:38, on Zulip):

well, it does

nikomatsakis (Aug 23 2018 at 21:38, on Zulip):

if you desugar, you get

nikomatsakis (Aug 23 2018 at 21:39, on Zulip):

wrapping_rem(&mut buckets.slice_mut()[(key as usize).wrapping_add(22), buckets.sweep()) or something like that...

nikomatsakis (Aug 23 2018 at 21:39, on Zulip):

I'm not sure how much that matters though

nikomatsakis (Aug 23 2018 at 21:39, on Zulip):

all this stuff disappears in MIR to some extent

Keith Yeung (Aug 23 2018 at 21:39, on Zulip):

that doesn't seem to look like what you described in the MIR

Keith Yeung (Aug 23 2018 at 21:40, on Zulip):

oh, you were talking strictly about HIR?

nikomatsakis (Aug 23 2018 at 21:40, on Zulip):

no, not at all

nikomatsakis (Aug 23 2018 at 21:40, on Zulip):

I mean .. we're analyzing the MIR

nikomatsakis (Aug 23 2018 at 21:40, on Zulip):

I'm not proposing we look at the HIR

nikomatsakis (Aug 23 2018 at 21:40, on Zulip):

but in the MIR there is somewhere a Call(A, B) -- and producing that value A creates a mut borrow, whereas producing B takes a shared borrow of same path

nikomatsakis (Aug 23 2018 at 21:41, on Zulip):

but I'm not sure it's the right way to think about it

Keith Yeung (Aug 23 2018 at 21:41, on Zulip):

huh, i'm not seeing that...

Keith Yeung (Aug 23 2018 at 21:41, on Zulip):

here's the MIR in question https://github.com/rust-lang/rust/pull/53305#issuecomment-412427620

Keith Yeung (Aug 23 2018 at 21:42, on Zulip):

it's the terminator for bb0 and bb4

Keith Yeung (Aug 23 2018 at 21:42, on Zulip):

they both take a reference of _1, bb0 uses it mutably while bb4 uses it immutably

nikomatsakis (Aug 23 2018 at 21:42, on Zulip):

well, the call that brings them togther is

_5 = const core::num::<impl usize>::wrapping_rem(move _6, move _9)
nikomatsakis (Aug 23 2018 at 21:43, on Zulip):

from bb5

Keith Yeung (Aug 23 2018 at 21:43, on Zulip):

i see, but the error is raised at bb4

nikomatsakis (Aug 23 2018 at 21:43, on Zulip):

I didn't mean to imply that the point where the error is raised

nikomatsakis (Aug 23 2018 at 21:43, on Zulip):

is the call that brings them together

nikomatsakis (Aug 23 2018 at 21:43, on Zulip):

in fact, I would not expect that

nikomatsakis (Aug 23 2018 at 21:43, on Zulip):

the point where error is part of creating the argument (probably the 2nd one)

nikomatsakis (Aug 23 2018 at 21:44, on Zulip):

so e.g. this call from bb4 produces _9

nikomatsakis (Aug 23 2018 at 21:44, on Zulip):

which then feeds into the call at bb6 (along with _6)

nikomatsakis (Aug 23 2018 at 21:44, on Zulip):

and here the bug is because there is some mut borrow that is held live because of _6

nikomatsakis (Aug 23 2018 at 21:45, on Zulip):

but I'm not sure how relevant that is

nikomatsakis (Aug 23 2018 at 21:45, on Zulip):

like, we're basically trying to say "maybe you should pull this subcomputation out and do it earlier"

nikomatsakis (Aug 23 2018 at 21:45, on Zulip):

and that could apply in many cases

nikomatsakis (Aug 23 2018 at 21:45, on Zulip):

for example

nikomatsakis (Aug 23 2018 at 21:45, on Zulip):
{
    let x = &mut foo;
    let y = foo + 1;
    *x = y + 1;
}
nikomatsakis (Aug 23 2018 at 21:45, on Zulip):

maybe we can identify that foo + 1 could be extracted earlier

nikomatsakis (Aug 23 2018 at 21:46, on Zulip):

I guess to be really right here we'd want to do a more involved "availability" analysis

nikomatsakis (Aug 23 2018 at 21:46, on Zulip):

(e.g., this is a kind of code motion)

nikomatsakis (Aug 23 2018 at 21:46, on Zulip):

that seems a bit much

Keith Yeung (Aug 23 2018 at 21:47, on Zulip):

yeah, i feel like with the tools we currently have in the compiler, we aren't able to accomplish this analysis yet

nikomatsakis (Aug 23 2018 at 21:47, on Zulip):

we could do something hackier involving spans or something but..ugh

Keith Yeung (Aug 23 2018 at 21:47, on Zulip):

but that does sound like the correct way

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

yeah I mean the code motion thing is the right thing

Keith Yeung (Aug 23 2018 at 21:48, on Zulip):

the only problem is whether we'd want a reduced (and perhaps a bit incorrect) subset of this analysis

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

it's sort of unclear because we might want to make the suggestion

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

even if we can't be sure it wouldn't affect semantics..?

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

anyway

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

the alternative is

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

we don't necessarily suggest anything

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

well

nikomatsakis (Aug 23 2018 at 21:48, on Zulip):

we can make a more generic suggestion like

nikomatsakis (Aug 23 2018 at 21:49, on Zulip):

"is it possible to move the shared borrow so it executes earlier?"

nikomatsakis (Aug 23 2018 at 21:49, on Zulip):

this could apply a lot of times

nikomatsakis (Aug 23 2018 at 21:49, on Zulip):

and/or we somehow explain what is happening better

Keith Yeung (Aug 23 2018 at 21:51, on Zulip):

sorry, i'm still stuck at looking at the MIR for this particular issue

Keith Yeung (Aug 23 2018 at 21:51, on Zulip):

i can't see how _6 matters here

nikomatsakis (Aug 23 2018 at 21:51, on Zulip):

it doesn't in particular

nikomatsakis (Aug 23 2018 at 21:52, on Zulip):

the question is basically "when do we offer a suggestion"

nikomatsakis (Aug 23 2018 at 21:53, on Zulip):

basically from the MIR POV there is no difference between "a[x] += a[y]" and let p = &mut a[x]; let v = a[y]; *p += v

nikomatsakis (Aug 23 2018 at 21:53, on Zulip):

which is exactly the problem

nikomatsakis (Aug 23 2018 at 21:53, on Zulip):

i.e., why we have an error

nikomatsakis (Aug 23 2018 at 21:53, on Zulip):

but so either we just suggest "maybe move the shared borrow earlier" like... all the time

nikomatsakis (Aug 23 2018 at 21:53, on Zulip):

or we compare lines or something

nikomatsakis (Aug 23 2018 at 21:53, on Zulip):

I was thinking "or else we try to see if this arose as part of generating temporaries for a method call"

Keith Yeung (Aug 23 2018 at 21:53, on Zulip):

ahh ok, so all we're really concerned about is whether the problematic borrow is later used as an argument to a funciton

nikomatsakis (Aug 23 2018 at 21:54, on Zulip):

which kind of suggests that it was a foo(&mut a[x], a[y]) situation

nikomatsakis (Aug 23 2018 at 21:54, on Zulip):

but there is no particular reason that this suggestion only applies then

Keith Yeung (Aug 23 2018 at 21:54, on Zulip):

and we don't check what happens with the "starting borrow"

nikomatsakis (Aug 23 2018 at 21:54, on Zulip):

except it's sort of... mildly less likely that there is "interference"

Keith Yeung (Aug 23 2018 at 21:58, on Zulip):

hmm... i would've expected let p = &mut a[x]; let v = a[y]; *p += v to compile

Keith Yeung (Aug 23 2018 at 21:59, on Zulip):

but a[x] += a[y] would not

nikomatsakis (Aug 24 2018 at 00:43, on Zulip):

a[x] += a[y] only sometimes does, I think; only if a is a &mut [u32] or some such thing

Last update: Nov 22 2019 at 00:35UTC