Stream: t-compiler/wg-nll

Topic: weekly meeting November 28


pnkfelix (Nov 28 2018 at 20:28, on Zulip):

hi @WG-compiler-nll ; the weekly meeting will be starting in ... gee about two minutes; sorry for not providing more of a heads up

pnkfelix (Nov 28 2018 at 20:31, on Zulip):

(some quick triage; I just categorized #56254 as NLL-sound ...)

pnkfelix (Nov 28 2018 at 20:32, on Zulip):

I don't know what we're doing right now about milestone assignments so I'll leave that item on the standing agenda alone

pnkfelix (Nov 28 2018 at 20:32, on Zulip):

and the unassigned link doesn't have anyone listed on it, but that might be a sign that we need to fix some milestones. Or maybe all is well.

nikomatsakis (Nov 28 2018 at 20:33, on Zulip):

:wave:

pnkfelix (Nov 28 2018 at 20:33, on Zulip):

(maybe we've removed everything from the one remaining milestone since its all linked to the edition, and so we need to use a different methodology for triage now....)

pnkfelix (Nov 28 2018 at 20:34, on Zulip):

So what should we talk about today?

nikomatsakis (Nov 28 2018 at 20:34, on Zulip):

I've been kind of out of it last few days owing to illness but I have a few things on my mind :)

pnkfelix (Nov 28 2018 at 20:34, on Zulip):

There are 7 open issues tagged as NLL-sound

pnkfelix (Nov 28 2018 at 20:34, on Zulip):

5 open issues tagged as NLL-complete

pnkfelix (Nov 28 2018 at 20:35, on Zulip):

(I'm not trying to do anything clever with filtering out NLL-deferred etc though right now)

pnkfelix (Nov 28 2018 at 20:35, on Zulip):

41 open issues tagged as NLL-deferred

nikomatsakis (Nov 28 2018 at 20:35, on Zulip):

definitely one of the big questions has been how to prioritize

pnkfelix (Nov 28 2018 at 20:36, on Zulip):

I tagged #54943 as I-needs-decision back in mid-october

nikomatsakis (Nov 28 2018 at 20:36, on Zulip):

I think in a previous meeting we kind of broke down into a few buckets of "things to do going forward"...

was sort of my breakdown

nikomatsakis (Nov 28 2018 at 20:37, on Zulip):

I feel like item 2 is probably worth deferring a bit. I think fixing bugs is good :)

pnkfelix (Nov 28 2018 at 20:37, on Zulip):

@nikomatsakis you removed #54943 from the release milestone. But the comment you left leads me to think that its still I-needs-decision ... its just not a high priority decision?

davidtwco (Nov 28 2018 at 20:37, on Zulip):

I tagged #54943 as I-needs-decision back in mid-october

That's 95% done now, see the topic here for latest updates on it.

pnkfelix (Nov 28 2018 at 20:37, on Zulip):

okay

nikomatsakis (Nov 28 2018 at 20:37, on Zulip):

yeah I think we more-or-less decided "might as well fix"

pnkfelix (Nov 28 2018 at 20:37, on Zulip):

"Port code over to using NLL borrow check that doesn't use it today" What is this about?

Matthew Jasper (Nov 28 2018 at 20:37, on Zulip):

We should recategorize NLL-deferred issues in those groups.

pnkfelix (Nov 28 2018 at 20:38, on Zulip):

identifying code in the rustc or stdlib that falls into that category?

Matthew Jasper (Nov 28 2018 at 20:38, on Zulip):

In rustc

davidtwco (Nov 28 2018 at 20:38, on Zulip):

I thought the vast majority of rustc crates were using NLL already?

nikomatsakis (Nov 28 2018 at 20:38, on Zulip):

"Port code over to using NLL borrow check that doesn't use it today" What is this about?

there are lots of bits of code in rustc that still use the lexical region checker

pnkfelix (Nov 28 2018 at 20:38, on Zulip):

I would think the only cases where we would prioritize such rewrites is where the existing code is slower because it is working around AST-borrowck limitations

nikomatsakis (Nov 28 2018 at 20:39, on Zulip):

I want to remove the lexical solver

pnkfelix (Nov 28 2018 at 20:39, on Zulip):

Oh oh I see now

nikomatsakis (Nov 28 2018 at 20:39, on Zulip):

therefore, I want to convert that code

pnkfelix (Nov 28 2018 at 20:39, on Zulip):

Sorry I misintpreted the bullet

nikomatsakis (Nov 28 2018 at 20:39, on Zulip):

however, I don't want to prioritize that

davidtwco (Nov 28 2018 at 20:39, on Zulip):

Oh, I see what you meant by that. :face_palm:

nikomatsakis (Nov 28 2018 at 20:39, on Zulip):

because I want to do more work on NLL checker first

pnkfelix (Nov 28 2018 at 20:39, on Zulip):

as meaning make use of NLL as a language feature

nikomatsakis (Nov 28 2018 at 20:39, on Zulip):

yeah, it was sort of ambiguous :)

nikomatsakis (Nov 28 2018 at 20:39, on Zulip):

(and because I'd like us to have time to shake out bugs etc)

nikomatsakis (Nov 28 2018 at 20:40, on Zulip):

anyway I agree with @Matthew Jasper that revisited "deferred" issues is a good idea

Matthew Jasper (Nov 28 2018 at 20:40, on Zulip):

Is it "just" item checking that still needs porting, or is there something more significant?

pnkfelix (Nov 28 2018 at 20:40, on Zulip):

@nikomatsakis so you're pretty confident that most (or all) of the NLL-deferred things fall into one of those three buckets, or are very low priority?

nikomatsakis (Nov 28 2018 at 20:40, on Zulip):

no :) I'm not sure what's in there

nikomatsakis (Nov 28 2018 at 20:40, on Zulip):

I think it makes sense to remove NLL-deferred

pnkfelix (Nov 28 2018 at 20:40, on Zulip):

k

nikomatsakis (Nov 28 2018 at 20:40, on Zulip):

as a thing

nikomatsakis (Nov 28 2018 at 20:40, on Zulip):

and move instead to P-medium vs P-low I guess

pnkfelix (Nov 28 2018 at 20:41, on Zulip):

or untag en masse and then reuse it for future postponements. :)

nikomatsakis (Nov 28 2018 at 20:41, on Zulip):

Is it "just" item checking that still needs porting, or is there something more significant?

I can think of a few random things; wf checking, comparing impl methods against obligations, ... that may be it, not sure

pnkfelix (Nov 28 2018 at 20:41, on Zulip):

but yeah P-low sounds better I guess

nikomatsakis (Nov 28 2018 at 20:41, on Zulip):

or untag en masse and then reuse it for future postponements. :)

well untagging en masse

nikomatsakis (Nov 28 2018 at 20:41, on Zulip):

and then triaging

nikomatsakis (Nov 28 2018 at 20:41, on Zulip):

is not the worst idea ever

nikomatsakis (Nov 28 2018 at 20:41, on Zulip):

but maybe it makes sense to look at the non-deferred things first?

nikomatsakis (Nov 28 2018 at 20:42, on Zulip):

e.g., the nll-sound issues

nikomatsakis (Nov 28 2018 at 20:42, on Zulip):

I guess they are all assigned

davidtwco (Nov 28 2018 at 20:42, on Zulip):

Non-deferred and non-fixed-by-NLL issues

nikomatsakis (Nov 28 2018 at 20:42, on Zulip):

but maybe it makes sense to decide if any of them are "P-high" in the ordinary compiler sense?

nikomatsakis (Nov 28 2018 at 20:43, on Zulip):

i.e., something we should be checking in on each week

Santiago Pastorino (Nov 28 2018 at 20:43, on Zulip):

ouch forgot about the meeting and filling the doc

Santiago Pastorino (Nov 28 2018 at 20:43, on Zulip):

ok, hi everyone :), sorry for being late

Santiago Pastorino (Nov 28 2018 at 20:43, on Zulip):

just arrived today from Europe

nikomatsakis (Nov 28 2018 at 20:44, on Zulip):

I guess we have some P-high annotations

pnkfelix (Nov 28 2018 at 20:44, on Zulip):

this #54105 is not assigned

nikomatsakis (Nov 28 2018 at 20:44, on Zulip):

yeah, I'll take that one, I want to compare with the work that @scalexm's been doing

pnkfelix (Nov 28 2018 at 20:44, on Zulip):

(why wasn't it caught in the earlier listing...) ((it was there, nevermind))

nikomatsakis (Nov 28 2018 at 20:45, on Zulip):

it's definitely an edge case

nikomatsakis (Nov 28 2018 at 20:45, on Zulip):

if it's even a real bug

nikomatsakis (Nov 28 2018 at 20:45, on Zulip):

it'd be good to decide on way or the other :)

nikomatsakis (Nov 28 2018 at 20:45, on Zulip):

I was not able to reproduce a problem iirc

nikomatsakis (Nov 28 2018 at 20:45, on Zulip):

maybe we should go down the NLL-sound list?

pnkfelix (Nov 28 2018 at 20:45, on Zulip):

okay

nikomatsakis (Nov 28 2018 at 20:45, on Zulip):

e.g., prohibit "two-phase borrows" with existing borrows? #56254

nikomatsakis (Nov 28 2018 at 20:46, on Zulip):

I'm not sure what's the latest in the thread but this needs to be acted up on relatively promptly I think

nikomatsakis (Nov 28 2018 at 20:46, on Zulip):

if we're going to make changes

pnkfelix (Nov 28 2018 at 20:46, on Zulip):

Given that we were already trying to limit two-phase borrows (to e.g. autoref)

nikomatsakis (Nov 28 2018 at 20:46, on Zulip):

I'm not inclined to backport though, as I wrote, because it just seems too stressful and not worth it

pnkfelix (Nov 28 2018 at 20:46, on Zulip):

it probably makes some sense to impose this limitation as well. Though I don't know anything about the supposed motivation (stacked borrows model, whatever that is)

nikomatsakis (Nov 28 2018 at 20:46, on Zulip):

"how much code will be uploaded to crates.io that exploits this in 6 weeks" (famous last words)

nikomatsakis (Nov 28 2018 at 20:47, on Zulip):

it's @RalfJ's attempt to come up with a definition of UB for unsafe code

nikomatsakis (Nov 28 2018 at 20:47, on Zulip):

I mean it is also true that

nikomatsakis (Nov 28 2018 at 20:47, on Zulip):

we may want to permit this sort of thing, even if it complicates the model

nikomatsakis (Nov 28 2018 at 20:47, on Zulip):

but it's nice to have the freedom to decide that

nikomatsakis (Nov 28 2018 at 20:48, on Zulip):

and this pattern is outside the original goals of 2PB

Matthew Jasper (Nov 28 2018 at 20:48, on Zulip):

Well, the PR that I have up breaks some code in the compiler

nikomatsakis (Nov 28 2018 at 20:48, on Zulip):

I've not looked at your PR

nikomatsakis (Nov 28 2018 at 20:48, on Zulip):

what code is broken?

RalfJ (Nov 28 2018 at 20:49, on Zulip):

we may want to permit this sort of thing, even if it complicates the model

complicates the model and makes optimizations harder / require more information / stronger analysis

pnkfelix (Nov 28 2018 at 20:49, on Zulip):

the aforementioned PR from @Matthew Jasper : PR #56301

Matthew Jasper (Nov 28 2018 at 20:50, on Zulip):

it's some code in add_retag.rs :upside_down:

nikomatsakis (Nov 28 2018 at 20:50, on Zulip):
[00:31:40] error[E0502]: cannot borrow `block_data.statements` as mutable because it is also borrowed as immutable
[00:31:40]    --> src/librustc_mir/transform/add_retag.rs:185:29
[00:31:40]     |
[00:31:40] 167 |                 match block_data.statements[i].kind {
[00:31:40] ...
[00:31:40] ...
[00:31:40] 185 |                             block_data.statements.insert(i, Statement {
[00:31:40] 186 |                                 source_info,
[00:31:40] 186 |                                 source_info,
[00:31:40] 187 |                                 kind: StatementKind::EscapeToRaw(src.clone()),
[00:31:40]     |                                                                  --- immutable borrow used here, in later iteration of loop
[00:31:40]
[00:31:40] error[E0502]: cannot borrow `block_data.statements` as mutable because it is also borrowed as immutable
[00:31:40]    --> src/librustc_mir/transform/add_retag.rs:197:25
[00:31:40]     |
[00:31:40] 167 |                 match block_data.statements[i].kind {
[00:31:40] ...
[00:31:40] ...
[00:31:40] 197 |                         block_data.statements.insert(i+1, Statement {
[00:31:40] 198 |                             source_info,
[00:31:40] 198 |                             source_info,
[00:31:40] 199 |                             kind: StatementKind::Retag { fn_entry: false, place: place.clone() },
[00:31:40]     |                                                                                  ----- immutable borrow used here, in later iteration of loop
[00:31:48] error: aborting due to 2 previous errors
pnkfelix (Nov 28 2018 at 20:51, on Zulip):

(see the travis failure)

nikomatsakis (Nov 28 2018 at 20:51, on Zulip):

that I guess?

Matthew Jasper (Nov 28 2018 at 20:51, on Zulip):

Yes, the clones can just be moved out of the calls to fix this

pnkfelix (Nov 28 2018 at 20:52, on Zulip):

but ... is this code that used to work under AST-borrowck ?

nikomatsakis (Nov 28 2018 at 20:52, on Zulip):

interesting

pnkfelix (Nov 28 2018 at 20:52, on Zulip):

or was it only added after we turned on NLL by default?

RalfJ (Nov 28 2018 at 20:52, on Zulip):

I added it fairly recently

RalfJ (Nov 28 2018 at 20:52, on Zulip):

but I was certainly not aware I was even relying on 2PB^^

nikomatsakis (Nov 28 2018 at 20:52, on Zulip):

added 2018-11-06

pnkfelix (Nov 28 2018 at 20:52, on Zulip):

so you admit to using the feature you want to remove! J'accuse! :wink:

nikomatsakis (Nov 28 2018 at 20:52, on Zulip):

amusing

RalfJ (Nov 28 2018 at 20:53, on Zulip):

well I think this refutes niko's argument that no code relying on this will land on crates.io in 6 weeks :P

nikomatsakis (Nov 28 2018 at 20:53, on Zulip):

heh. =) it may well

davidtwco (Nov 28 2018 at 20:54, on Zulip):

There may already be projects on nightly with code that relies on it.

pnkfelix (Nov 28 2018 at 20:54, on Zulip):

do we want to make an executive decision here on our own as a team, or do we want to let T-lang weigh in tomororw night?

Matthew Jasper (Nov 28 2018 at 20:55, on Zulip):

We could just revert the match guard desugaring and then the fix is simple. The match bug has been around since forever and it's only a warning in migrate mode.

Matthew Jasper (Nov 28 2018 at 20:55, on Zulip):

(Or not, but then it's less simple)

pnkfelix (Nov 28 2018 at 20:56, on Zulip):

hmm my reading was that the regression of add_retag.rs was due to the fundamental restriction being added; you say its actually part of the new match guard desugaring?

pnkfelix (Nov 28 2018 at 20:56, on Zulip):

/me goes to look more carefully at the code in question

Matthew Jasper (Nov 28 2018 at 20:57, on Zulip):

No, I'm talking about the easiest way to make Reservations mutable accesses

nikomatsakis (Nov 28 2018 at 20:57, on Zulip):

it looks fundamental to me

Matthew Jasper (Nov 28 2018 at 20:57, on Zulip):

Which is what the current match checking relies on. (It relies on them only being shared accesses)

pnkfelix (Nov 28 2018 at 20:58, on Zulip):

@Matthew Jasper okay, but hold on. There may be different threads of conversation going on ...

nikomatsakis (Nov 28 2018 at 20:58, on Zulip):

I think we should probably hash this out in the other topic

nikomatsakis (Nov 28 2018 at 20:58, on Zulip):

(outside of this mtg?)

RalfJ (Nov 28 2018 at 20:58, on Zulip):

yeah, add_retag fundamentally relies on the thing I am suggesting to not allow

RalfJ (Nov 28 2018 at 20:58, on Zulip):

rather ironically, I must admit^^

nikomatsakis (Nov 28 2018 at 20:58, on Zulip):

but it does seem like we have to reach a decision asap

pnkfelix (Nov 28 2018 at 20:58, on Zulip):

I though the point raised by the add_retag.rs example was that code in the wild might in practice want to lift the constraint being requested by @RalfJ

RalfJ (Nov 28 2018 at 20:58, on Zulip):

not only is that code I wrote, it is part of the Stacked Borrows implementation which gives rise to my suggestions

pnkfelix (Nov 28 2018 at 20:58, on Zulip):

okay.

davidtwco (Nov 28 2018 at 20:59, on Zulip):

That's the best irony I've seen in so long.

pnkfelix (Nov 28 2018 at 20:59, on Zulip):

@nikomatsakis yeah okay so maybe we got sidetracked, or maybe this is the most important Q to resolve in the meeting, given the time pressure...?

pnkfelix (Nov 28 2018 at 20:59, on Zulip):

(honestly not sure)

nikomatsakis (Nov 28 2018 at 20:59, on Zulip):

heh I too am unsure

nikomatsakis (Nov 28 2018 at 21:00, on Zulip):

I am reluctant to revert the match guard protections etc

RalfJ (Nov 28 2018 at 21:00, on Zulip):

I though the point raised by the add_retag.rs example was that code in the wild might in practice want to lift the constraint being requested by @RalfJ

yeah there's a tradeoff between allowing more code on the one hand, and more optimizations / easier analysis (in the optimizer) on the other hand

Matthew Jasper (Nov 28 2018 at 21:00, on Zulip):

Do we think that it's worth trying at all to get a fix for this in before the release?

pnkfelix (Nov 28 2018 at 21:01, on Zulip):

I am reluctant to revert the match guard protections etc

the risks around match guards may be fundamentally lower

pnkfelix (Nov 28 2018 at 21:01, on Zulip):

because there is feature gate guarding the more full featured expressive forms there

pnkfelix (Nov 28 2018 at 21:01, on Zulip):

/me goes to look for the link

pnkfelix (Nov 28 2018 at 21:01, on Zulip):

this one: https://github.com/rust-lang/rfcs/pull/107

pnkfelix (Nov 28 2018 at 21:02, on Zulip):

aka #15287

pnkfelix (Nov 28 2018 at 21:02, on Zulip):

aka #![feature(bind_by_move_pattern_guards)]

nikomatsakis (Nov 28 2018 at 21:02, on Zulip):

I guess then the options are:

in the first two cases, we probably want to revisit the model a bit and see if we can't find something we're happier with overall?

Matthew Jasper (Nov 28 2018 at 21:02, on Zulip):

You need to use a closure that captures the variable being matched on.

pnkfelix (Nov 28 2018 at 21:02, on Zulip):

in other words, I am claiming that it will be harder for people to run into trouble there ... maybe... because we already limit them so much

pnkfelix (Nov 28 2018 at 21:03, on Zulip):

You need to use a closure that captures the variable being matched on.

right, that's the @Ariel Ben-Yehuda workaround for exposing the old bug

nikomatsakis (Nov 28 2018 at 21:03, on Zulip):

it just feels like a lot of code to change too

nikomatsakis (Nov 28 2018 at 21:03, on Zulip):

what does reverting the match guard protection mean exactly?

nikomatsakis (Nov 28 2018 at 21:03, on Zulip):

we would make ref mut p exposed as a true &mut in the arm guard code?

nikomatsakis (Nov 28 2018 at 21:04, on Zulip):

I think we do have some sort of "boolean" there, or used to, to change how the MIR was generated

nikomatsakis (Nov 28 2018 at 21:04, on Zulip):

so perhaps that's still well tested

RalfJ (Nov 28 2018 at 21:04, on Zulip):

we would make ref mut p exposed as a true &mut in the arm guard code?

that's not even needed, "just" the fake refs/reads are in the way

nikomatsakis (Nov 28 2018 at 21:04, on Zulip):

(I think the "final result" we want may well just be the "transmute" approach anyway?)

Matthew Jasper (Nov 28 2018 at 21:04, on Zulip):
nikomatsakis (Nov 28 2018 at 21:05, on Zulip):

that's not even needed, "just" the fake refs/reads are in the way

which fake refs are you referring to?

pnkfelix (Nov 28 2018 at 21:05, on Zulip):

Set the generate_borrow_of_any_match_input flag to false by default, even in NLL

won't that open a soundness hole?

pnkfelix (Nov 28 2018 at 21:06, on Zulip):

oh only if you do the closure thing

Matthew Jasper (Nov 28 2018 at 21:06, on Zulip):

Yes, but it's a hard one to exploit

nikomatsakis (Nov 28 2018 at 21:06, on Zulip):

iirc, we first insert a bunch of shallow borrows for all the "discriminants" we switch on -- these last until we enter the match arm.

then for guards with &mut variables we do 2PB that are never activated.

It's these 2PB that are a problem, right?

RalfJ (Nov 28 2018 at 21:06, on Zulip):

that's not even needed, "just" the fake refs/reads are in the way

which fake refs are you referring to?

the "fake" stuff in https://github.com/rust-lang/rust/issues/56254#issuecomment-442357546

RalfJ (Nov 28 2018 at 21:08, on Zulip):

with those gone, the borrow can even stay &mut2phase, because there'll be no outstanding shared borrows

RalfJ (Nov 28 2018 at 21:08, on Zulip):

(from what I understand, not sure if I got this right)

nikomatsakis (Nov 28 2018 at 21:08, on Zulip):

right, so from my list, you're talking about the "shallow borrows for discriminants"

pnkfelix (Nov 28 2018 at 21:08, on Zulip):

but the whole point of the FakeReads is to catch mutations from the guards, no?

nikomatsakis (Nov 28 2018 at 21:08, on Zulip):

there were some pretty bad soundness problems in matches iirc

nikomatsakis (Nov 28 2018 at 21:08, on Zulip):

that required less than a closure to exploit :)

nikomatsakis (Nov 28 2018 at 21:09, on Zulip):

I want to be sure we don't re-open those doors

nikomatsakis (Nov 28 2018 at 21:10, on Zulip):

I'm not convinced that just reverting the guard treatment helps here? In particular, if we go back to the old way, then aren't ref mut bindings a strict &mut, which means that they will definitely conflict with the "fake" borrows of the discriminants, right? or am I being silly?

Matthew Jasper (Nov 28 2018 at 21:10, on Zulip):

The particularly bad one was that AST borrowck has no concept of a "discriminant(x)" which accesses memory

nikomatsakis (Nov 28 2018 at 21:10, on Zulip):

@Matthew Jasper is my summary roughly correct of what we are doing now?

Matthew Jasper (Nov 28 2018 at 21:11, on Zulip):

Yes, that's why I'm suggesting turning them off, for now

nikomatsakis (Nov 28 2018 at 21:12, on Zulip):

right but how does that help. Then we do the discriminant borrows and we do real &mut borrows, not 2phase

nikomatsakis (Nov 28 2018 at 21:12, on Zulip):

which definitely conflict

RalfJ (Nov 28 2018 at 21:12, on Zulip):

I think he's proposing to not do discriminant borrows

Matthew Jasper (Nov 28 2018 at 21:12, on Zulip):

No, I mean the discriminant borrows

RalfJ (Nov 28 2018 at 21:12, on Zulip):

that's what's being turned off

nikomatsakis (Nov 28 2018 at 21:12, on Zulip):

ah ok

nikomatsakis (Nov 28 2018 at 21:12, on Zulip):

so not this :)

pnkfelix (Nov 28 2018 at 21:13, on Zulip):

.... no that is what he's describing

nikomatsakis (Nov 28 2018 at 21:13, on Zulip):

ok, I guess I'm confused as to the role of that flag

pnkfelix (Nov 28 2018 at 21:13, on Zulip):

Setting generate_borrow_of_any_match_input flag to false will cause the discriminant borrows to be suppressed

nikomatsakis (Nov 28 2018 at 21:13, on Zulip):

anyway

nikomatsakis (Nov 28 2018 at 21:13, on Zulip):

ok ok thanks sorry :)

nikomatsakis (Nov 28 2018 at 21:13, on Zulip):

in that case we don't have to change the ref mut borrows back to 1phase

nikomatsakis (Nov 28 2018 at 21:14, on Zulip):

we can if we like

nikomatsakis (Nov 28 2018 at 21:14, on Zulip):

(right?)

RalfJ (Nov 28 2018 at 21:14, on Zulip):

I think so (that's what I said above)

pnkfelix (Nov 28 2018 at 21:14, on Zulip):

I don't quite grok the logic of what we're talking about

pnkfelix (Nov 28 2018 at 21:14, on Zulip):

in terms of re-injecting an (admittedly subtle) soundness hole

pnkfelix (Nov 28 2018 at 21:15, on Zulip):

in order to accommodate a hypothetical soundness model...

Matthew Jasper (Nov 28 2018 at 21:15, on Zulip):

It's doesn't really matter either way. It just keeps more options open.

RalfJ (Nov 28 2018 at 21:15, on Zulip):

@pnkfelix it'd make restricting 2PB as suggested trivial (backportable, even, maybe), that's all

RalfJ (Nov 28 2018 at 21:16, on Zulip):

but yeah knowingly introducing unsoundness is... rather drastic

nikomatsakis (Nov 28 2018 at 21:16, on Zulip):

it seems like @Matthew Jasper's Pr is "not so bad"

nikomatsakis (Nov 28 2018 at 21:16, on Zulip):

the only thing that we would wnt to do to clean it up

Matthew Jasper (Nov 28 2018 at 21:16, on Zulip):

If we're not backporting. Then I'll create a PR that does this all properly.

nikomatsakis (Nov 28 2018 at 21:16, on Zulip):

is to remove 2PB and replace with transmute

nikomatsakis (Nov 28 2018 at 21:17, on Zulip):

and sidestep the whole issue

nikomatsakis (Nov 28 2018 at 21:17, on Zulip):

i.e., transmute a &&Foo to a &&mut Foo

nikomatsakis (Nov 28 2018 at 21:17, on Zulip):

which -- as @RalfJ has proven =) -- are equivalent

nikomatsakis (Nov 28 2018 at 21:17, on Zulip):

and then we don't have the special cases anymore

nikomatsakis (Nov 28 2018 at 21:18, on Zulip):

(we could maybe even do that in the PR itself, I suppose)

nikomatsakis (Nov 28 2018 at 21:18, on Zulip):

is that correct?

RalfJ (Nov 28 2018 at 21:18, on Zulip):

I think so

Matthew Jasper (Nov 28 2018 at 21:19, on Zulip):

match x { ref mut z if x == z => ...

If we're happy with this compiling

nikomatsakis (Nov 28 2018 at 21:19, on Zulip):

ok, right, so there is the question of

match x { ref mut z if x == 1

but .. I feel ok about it personally. In partcular,

nikomatsakis (Nov 28 2018 at 21:19, on Zulip):

it would be UB to (e.g.) use unsafe code to modify through that *z etc

nikomatsakis (Nov 28 2018 at 21:19, on Zulip):

so there is already some "specialness" going on

nikomatsakis (Nov 28 2018 at 21:20, on Zulip):

i.e., there is an edge case where the desugaring becomes visible, but it's not clearly a problem

nikomatsakis (Nov 28 2018 at 21:20, on Zulip):

I guess I don't know what the alternative is that rejects that, did you have a concrete proposal @Matthew Jasper?

pnkfelix (Nov 28 2018 at 21:20, on Zulip):

wait was it supposed to be 1 or z in there

pnkfelix (Nov 28 2018 at 21:20, on Zulip):

z, right?

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

no

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

the point is

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

we have a z with type &&mut u32

Matthew Jasper (Nov 28 2018 at 21:21, on Zulip):

Either: it doesn't really matter

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

but it's kind of a lie -- but not really a lie

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

er sorry z has type &mut u32

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

but you can access x directly

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

since really what you have when you reference z is not an &mut u32

nikomatsakis (Nov 28 2018 at 21:21, on Zulip):

but rather a "shared" &mut u32

nikomatsakis (Nov 28 2018 at 21:22, on Zulip):

(this also compiles today, I presume)

nikomatsakis (Nov 28 2018 at 21:22, on Zulip):

I feel like guards are in a weird state and when someone notices this, it's time for them to "level up" and learn about the desugaring a bit more deeply :P

nikomatsakis (Nov 28 2018 at 21:22, on Zulip):

but maybe i'm rationalizing

pnkfelix (Nov 28 2018 at 21:23, on Zulip):

Are we going to be committing to this same code gen strategy forever?

nikomatsakis (Nov 28 2018 at 21:23, on Zulip):

we're committed to that code compiling

pnkfelix (Nov 28 2018 at 21:24, on Zulip):

but the stuff about using unsafe code to observe the underlying pointers

nikomatsakis (Nov 28 2018 at 21:24, on Zulip):

but really what it comes down to is

pnkfelix (Nov 28 2018 at 21:24, on Zulip):

I'm asking if we're committing to that always having the same semantics

nikomatsakis (Nov 28 2018 at 21:24, on Zulip):

when does the "uniqueness" kick in

Matthew Jasper (Nov 28 2018 at 21:24, on Zulip):

IMO, we should take the obvious codegen, and remove unsound cases. Not create new cases that are OK

nikomatsakis (Nov 28 2018 at 21:24, on Zulip):

effectively we're building an upgradable read-write lock

nikomatsakis (Nov 28 2018 at 21:24, on Zulip):

I must have missed it. What is the "obvious codegen"?

nikomatsakis (Nov 28 2018 at 21:25, on Zulip):

effectively we're building an upgradable read-write lock

this is what 2PB is doing today; we're recreating that behavior through a shared lock that transitions to a &mut

Matthew Jasper (Nov 28 2018 at 21:25, on Zulip):

Create a mutable reference immediately, no fake borrows, only one version of the variable.

pnkfelix (Nov 28 2018 at 21:26, on Zulip):

@Matthew Jasper the fake borrows are what prevent aliased mutation (via closure)

nikomatsakis (Nov 28 2018 at 21:26, on Zulip):

right, that is what we started with, but we adopted this to close soundness holes

RalfJ (Nov 28 2018 at 21:26, on Zulip):

Create a mutable reference immediately, no fake borrows, only one version of the variable.

but then how do we reject guards that mutate the discriminant?

pnkfelix (Nov 28 2018 at 21:27, on Zulip):

(the background here, for the peanut gallery, is available in PR #50783 and issue #27282 )

nikomatsakis (Nov 28 2018 at 21:27, on Zulip):

ah thanks I was looking for #27282

nikomatsakis (Nov 28 2018 at 21:28, on Zulip):

oh, I was searching with is:closed

nikomatsakis (Nov 28 2018 at 21:28, on Zulip):

that's why I couldn't find it :)

Matthew Jasper (Nov 28 2018 at 21:28, on Zulip):

Indeed, we need to prevent mutation except the "mutation" from ref mut. I just don't think that we should allow more code in the process.

nikomatsakis (Nov 28 2018 at 21:28, on Zulip):

well, including the mutation from ref mut?

nikomatsakis (Nov 28 2018 at 21:29, on Zulip):

(er, what I mean is, the guard should not mutate through the ref mut binding)

Matthew Jasper (Nov 28 2018 at 21:29, on Zulip):

Yes, it shouldn't, but ref mut should be allowed

nikomatsakis (Nov 28 2018 at 21:29, on Zulip):

I see your point, in any case, I'm just not sure how that is to be done.

Matthew Jasper (Nov 28 2018 at 21:29, on Zulip):

Indeed, there's no way I'm implementing a solution there in 2 days.

nikomatsakis (Nov 28 2018 at 21:30, on Zulip):

I know we've written up various edge cases in the past

nikomatsakis (Nov 28 2018 at 21:30, on Zulip):

that said

nikomatsakis (Nov 28 2018 at 21:30, on Zulip):

I think that if we leave this rather narrow window open

nikomatsakis (Nov 28 2018 at 21:30, on Zulip):

that you can access the value being matched in a match arm

pnkfelix (Nov 28 2018 at 21:30, on Zulip):

(and i think many of the edge cases are in the comment thread of #27282 ...)

nikomatsakis (Nov 28 2018 at 21:30, on Zulip):

there's a good chance we can close it, if we come up with a good fix

nikomatsakis (Nov 28 2018 at 21:30, on Zulip):

and if not, I think we can live with it

nikomatsakis (Nov 28 2018 at 21:31, on Zulip):

since basically the way to think about it is:

nikomatsakis (Nov 28 2018 at 21:31, on Zulip):

I agree it's not ideal, but it seems tolerably simple

nikomatsakis (Nov 28 2018 at 21:31, on Zulip):

that said I think it's not a bad idea to try and revisit (and document, this time!) the whole saga

nikomatsakis (Nov 28 2018 at 21:32, on Zulip):

i.e., schedule a time to go over and rederive all the edge cases

nikomatsakis (Nov 28 2018 at 21:32, on Zulip):

and see if we see another solution

nikomatsakis (Nov 28 2018 at 21:32, on Zulip):

I agree it's not ideal, but it seems tolerably simple

it seems like a more likely way for people to learn about this is when they try to pass the &mut to a function that mutates, like

match x {
    ref mut v if v.insert(22) => { ... }
}
nikomatsakis (Nov 28 2018 at 21:33, on Zulip):

and we all agree that will not be permitted...

nikomatsakis (Nov 28 2018 at 21:33, on Zulip):

speaking of which, I bet we give a terrible error message for that

nikomatsakis (Nov 28 2018 at 21:34, on Zulip):

(or we will once that feature gate is lifted)

nikomatsakis (Nov 28 2018 at 21:35, on Zulip):

so does that mean we want to start by trying to land some variant of #56301 ?

nikomatsakis (Nov 28 2018 at 21:35, on Zulip):

and backport?

Matthew Jasper (Nov 28 2018 at 21:35, on Zulip):
error[E0596]: cannot borrow `*v` as mutable, as it is immutable for the pattern guard
 --> <source>:9:22
  |
9 |         ref mut v if v.insert(22) => { }
  |                      ^ cannot borrow as mutable
  |
  = note: variables bound in patterns are immutable until the end of the pattern guard
error: aborting due to previous error
For more information about this error, try `rustc --explain E0596`.
Compiler returned: 1

Could be worse

nikomatsakis (Nov 28 2018 at 21:35, on Zulip):

and then do a comprehensive review of the situation?

nikomatsakis (Nov 28 2018 at 21:35, on Zulip):

Could be worse

I stand corrected! pretty decent

pnkfelix (Nov 28 2018 at 21:36, on Zulip):

is that from nightly, or from your PR, @Matthew Jasper ?

davidtwco (Nov 28 2018 at 21:36, on Zulip):

I got this with #![feature(nll)] unless I'm doing something wrong? playground

error[E0301]: cannot mutably borrow in a pattern guard
 --> src/main.rs:6:22
  |
6 |         ref mut v if v.insert(22) => { println!("{:?}", v); }
  |                      ^ borrowed mutably in pattern guard
  |
  = help: add #![feature(bind_by_move_pattern_guards)] to the crate attributes to enable
Matthew Jasper (Nov 28 2018 at 21:36, on Zulip):

#![feature(nll, bind_by_move_pattern_guards)]

davidtwco (Nov 28 2018 at 21:36, on Zulip):

Or am I missing that attribute..

davidtwco (Nov 28 2018 at 21:37, on Zulip):

Yeah, as soon as I sent that it clicked.

davidtwco (Nov 28 2018 at 21:37, on Zulip):

:face_palm:

Matthew Jasper (Nov 28 2018 at 21:37, on Zulip):

Hmm, how easy is it to generate a transmute in MIR?

nikomatsakis (Nov 28 2018 at 21:37, on Zulip):

oh we can just make it a Cast

nikomatsakis (Nov 28 2018 at 21:38, on Zulip):

(that was my plan, anyway)

pnkfelix (Nov 28 2018 at 21:38, on Zulip):

will that pass mir-typeck ?

nikomatsakis (Nov 28 2018 at 21:38, on Zulip):

I was expecting to add a new CastKind

nikomatsakis (Nov 28 2018 at 21:38, on Zulip):

for converting && T to &&mut T

pnkfelix (Nov 28 2018 at 21:38, on Zulip):

ah

nikomatsakis (Nov 28 2018 at 21:39, on Zulip):

we'd have to add support to mir typeck

nikomatsakis (Nov 28 2018 at 21:39, on Zulip):

but doesn't seem so hard

nikomatsakis (Nov 28 2018 at 21:39, on Zulip):

basically just equate the lifetimes and the type

nikomatsakis (Nov 28 2018 at 21:39, on Zulip):

I wouldn't want to literally invoke transmute, that'd be a pain

nikomatsakis (Nov 28 2018 at 21:39, on Zulip):

(and would hurt compilation time)

Matthew Jasper (Nov 28 2018 at 21:40, on Zulip):

Ah, OK. Still, that sounds like more work than I have time for.

nikomatsakis (Nov 28 2018 at 21:42, on Zulip):

it seems "not hard" but not easy

pnkfelix (Nov 28 2018 at 21:42, on Zulip):

i.e., schedule a time to go over and rederive all the edge cases

I don't know if this counts, but I did try to transcribe all of them into tests. Look in ui/issues/ and look for filenames that start with issue-27282

nikomatsakis (Nov 28 2018 at 21:42, on Zulip):

can we at least fix the travis error @Matthew Jasper in your PR and see what happens next?

nikomatsakis (Nov 28 2018 at 21:42, on Zulip):

that seems like the most immediate thing that needs to be done

Matthew Jasper (Nov 28 2018 at 21:42, on Zulip):

Sure

nikomatsakis (Nov 28 2018 at 21:42, on Zulip):

also, we are ridiculously over time :)

nikomatsakis (Nov 28 2018 at 21:42, on Zulip):

we really have to reach a final decision on this ASAP, presumably tomorrow morning

RalfJ (Nov 28 2018 at 21:42, on Zulip):

so sorry for crashing your meeting :/

pnkfelix (Nov 28 2018 at 21:43, on Zulip):

It had to be addressed

pnkfelix (Nov 28 2018 at 21:43, on Zulip):

that's why I put it on the agenda

nikomatsakis (Nov 28 2018 at 21:43, on Zulip):

@RalfJ just to be clear, though, another alternative here is that SB knows about 2PB and we make the "activation point" be a "thing", right?

RalfJ (Nov 28 2018 at 21:43, on Zulip):

and also big :heart: for actually seriously considering such a change so late in the process. whatever the outcome, it is much appreciated!

pnkfelix (Nov 28 2018 at 21:43, on Zulip):

(even if we never actually reached that agenda item per se)

nikomatsakis (Nov 28 2018 at 21:43, on Zulip):

i.e., a &mut2 pushes something onto the stack that is more like "shared"

nikomatsakis (Nov 28 2018 at 21:44, on Zulip):

and then the activation pushes the true mut on there

nikomatsakis (Nov 28 2018 at 21:44, on Zulip):

i.e., it's not necessarily a full-on-graph that is required

nikomatsakis (Nov 28 2018 at 21:44, on Zulip):

just that 2PB is more "known" to stacked borrows than you would like

RalfJ (Nov 28 2018 at 21:44, on Zulip):

well I think it might need a tree

RalfJ (Nov 28 2018 at 21:44, on Zulip):

@Ariel Ben-Yehuda has some examples with multiple outstanding 2PB

nikomatsakis (Nov 28 2018 at 21:45, on Zulip):

can you send me a link?

nikomatsakis (Nov 28 2018 at 21:45, on Zulip):

I'm sure you have already done so ...

RalfJ (Nov 28 2018 at 21:45, on Zulip):

I'm afraid @Ariel Ben-Yehuda 's notebook in Rome doesn't have a URL

nikomatsakis (Nov 28 2018 at 21:45, on Zulip):

heh

nikomatsakis (Nov 28 2018 at 21:45, on Zulip):

ok, maybe they can transcribe them

nikomatsakis (Nov 28 2018 at 21:46, on Zulip):

I'd like to see an example at some point

RalfJ (Nov 28 2018 at 21:46, on Zulip):

roughly, it was

let tpb1 = &2phase x;
let tpb2 = &2phase x;
if condition { activate(tbp1) } else { activate (tbp2) }
nikomatsakis (Nov 28 2018 at 21:46, on Zulip):

that can't happen

RalfJ (Nov 28 2018 at 21:46, on Zulip):

well, with the current restrictive desugaring yes

RalfJ (Nov 28 2018 at 21:46, on Zulip):

but it is a rather natural question if you say "2pb isn't initially exclusive"

RalfJ (Nov 28 2018 at 21:47, on Zulip):

and remember that the model has to work in unsafe code

nikomatsakis (Nov 28 2018 at 21:47, on Zulip):

basically I am trying to figure out

nikomatsakis (Nov 28 2018 at 21:47, on Zulip):

even in that case, I guess

nikomatsakis (Nov 28 2018 at 21:47, on Zulip):

it seems like if we made the activations a real thing

nikomatsakis (Nov 28 2018 at 21:47, on Zulip):

and the borrow is then just effectively a shared borrow

RalfJ (Nov 28 2018 at 21:47, on Zulip):

that reminds me of EndRegion. in all the bad ways.

nikomatsakis (Nov 28 2018 at 21:47, on Zulip):

but the activation is what pushes the &mut

Matthew Jasper (Nov 28 2018 at 21:48, on Zulip):
fn foo(_: &mut i32, _: &mut i32, (): ()) {}

fn bar() {
    let mut x = 3;
    let y = &mut x;
    foo(y, y, return);
}
RalfJ (Nov 28 2018 at 21:48, on Zulip):

wtf is that?^^

pnkfelix (Nov 28 2018 at 21:48, on Zulip):

that seems fine to me

pnkfelix (Nov 28 2018 at 21:48, on Zulip):

the aliasing never gets observed, no?

pnkfelix (Nov 28 2018 at 21:49, on Zulip):

or is @Matthew Jasper 's point that this is not intuitive?

nikomatsakis (Nov 28 2018 at 21:50, on Zulip):

that reminds me of EndRegion. in all the bad ways.

it feels pretty different to me. The activation is a "real point" that we have to consider in the borrow checker anyway, and it corresponds to a single use.

Matthew Jasper (Nov 28 2018 at 21:50, on Zulip):

Actually, never mind, it's fine.

pnkfelix (Nov 28 2018 at 21:50, on Zulip):

(and also, I think the current impl of 2PB would still reject it .... )

nikomatsakis (Nov 28 2018 at 21:50, on Zulip):

in any case, i'm just trying to tease out exactly what we are committed to

nikomatsakis (Nov 28 2018 at 21:50, on Zulip):

if we wind up doing nothing

nikomatsakis (Nov 28 2018 at 21:51, on Zulip):

(and also, I think the current impl of 2PB would still reject it .... )

those are not even 2PB I don't think

Matthew Jasper (Nov 28 2018 at 21:51, on Zulip):

It doesn't even need 2PB

nikomatsakis (Nov 28 2018 at 21:51, on Zulip):

but you could imagine such code being accepted for some plausible extension

Matthew Jasper (Nov 28 2018 at 21:51, on Zulip):

The borrows end immediately

nikomatsakis (Nov 28 2018 at 21:52, on Zulip):

ah, heh :)

pnkfelix (Nov 28 2018 at 21:52, on Zulip):

well we can "fix" that

RalfJ (Nov 28 2018 at 21:52, on Zulip):

i'm just trying to tease out exactly what we are committed to
if we wind up doing nothing

well I dont yet have a model that can explain this. there might be a nice one, or at least nicer than trees. but I wont be able to complete this analysis in time.

nikomatsakis (Nov 28 2018 at 21:52, on Zulip):

basically I think the idea would be that &2phase is a shared borrow

nikomatsakis (Nov 28 2018 at 21:52, on Zulip):

and an activation pushes an &mut onto the stack

nikomatsakis (Nov 28 2018 at 21:53, on Zulip):

basically I think the idea would be that &2phase is a shared borrow

or equivalent to

nikomatsakis (Nov 28 2018 at 21:53, on Zulip):

anyway I know that's not a real proposal and I'm sure it has flaws

RalfJ (Nov 28 2018 at 21:53, on Zulip):

I just had an idea based on what you said (make the two-phase borrows be shared borrows that also remember in their tag the mutable morrow they were created one, and do a mutable reborrow on the first write) -- but whether and how that would work out, no idea (I foresee some conflicts with the "redundant reborrow" rule)

nikomatsakis (Nov 28 2018 at 21:53, on Zulip):

but the currently implemented variant of 2PB, which is fairly restictive in that it requires a postdominating use etc, isn't that expressive

pnkfelix (Nov 28 2018 at 21:53, on Zulip):

okay I really have to go like right now because of :baby: :baby_bottle: :alarm_clock:

nikomatsakis (Nov 28 2018 at 21:53, on Zulip):

I gotta go too :)

pnkfelix (Nov 28 2018 at 21:53, on Zulip):

bye all

nikomatsakis (Nov 28 2018 at 21:54, on Zulip):

illuminating y'all

nikomatsakis (Nov 28 2018 at 21:54, on Zulip):

sure would've been nice to be "illuminated" a month ago but oh well :stuck_out_tongue:

RalfJ (Nov 28 2018 at 21:54, on Zulip):

yeah in hindsight it's so easy to assign priorities...^^

Matthew Jasper (Nov 28 2018 at 21:56, on Zulip):

Apart from matches, 2PB is incredibly minimal. The activation is the next and only use (excluding StorageLive/Dead) of the mutable reference created by the 2PB.

Last update: Nov 22 2019 at 00:05UTC