Stream: t-compiler/wg-nll

Topic: issue-46908-borrowck=migrate


nikomatsakis (Jul 03 2018 at 21:06, on Zulip):

@Santiago Pastorino just opening a thread to chat about that issue =)

Santiago Pastorino (Jul 04 2018 at 14:40, on Zulip):

In particular, there is some code that is accepted today under AST borrowck that NLL rejects. So we need a migration path.

Santiago Pastorino (Jul 04 2018 at 14:40, on Zulip):

@nikomatsakis remember me what that code is

Santiago Pastorino (Jul 04 2018 at 14:40, on Zulip):

:)

nikomatsakis (Jul 04 2018 at 15:09, on Zulip):

just look for NLL-fixed-by-NLL tag

nikomatsakis (Jul 04 2018 at 15:09, on Zulip):

plenty of bugs fixed by NLL: https://github.com/rust-lang/rust/labels/NLL-fixed-by-NLL

nikomatsakis (Jul 04 2018 at 15:10, on Zulip):

all of that is code that is accepted under the AST borrow check

nikomatsakis (Jul 04 2018 at 15:10, on Zulip):

but should not have been

Santiago Pastorino (Jul 04 2018 at 16:17, on Zulip):

@nikomatsakis back, I always thought that that was not going to happen

Santiago Pastorino (Jul 04 2018 at 16:18, on Zulip):

and also having open issues about that confuses me

Santiago Pastorino (Jul 04 2018 at 16:18, on Zulip):

you mean that the code that is accepted in AST borrowck but NLL rejects are issues to be fixed? or is there really code that is accepted under AST borrowck and not by NLL?

Santiago Pastorino (Jul 04 2018 at 16:19, on Zulip):

haven't gone through the issues, checking ...

Santiago Pastorino (Jul 04 2018 at 16:20, on Zulip):

ahhh I see you meant that is code wrongly accepted by AST borrowck and should not

Santiago Pastorino (Jul 04 2018 at 16:21, on Zulip):

if it's that I'm fine :)

Santiago Pastorino (Jul 04 2018 at 16:21, on Zulip):

reading @pnkfelix phrase confused me, I thought there was code that worked on AST and not on NLL

nikomatsakis (Jul 04 2018 at 17:02, on Zulip):

if it's that I'm fine :)

correct

Santiago Pastorino (Jul 04 2018 at 17:52, on Zulip):

@nikomatsakis I think the link at the end of https://github.com/rust-lang/rust/issues/46908 is now incorrect

Santiago Pastorino (Jul 04 2018 at 17:53, on Zulip):

ahh no, seems fine

Santiago Pastorino (Jul 04 2018 at 17:58, on Zulip):

so checking just in case, if mir doesn't report any error the code should be allowed, right?

nikomatsakis (Jul 04 2018 at 17:59, on Zulip):

yes

nikomatsakis (Jul 04 2018 at 17:59, on Zulip):

the presumption is that MIR borrowck is strictly more correct :)

Santiago Pastorino (Jul 04 2018 at 17:59, on Zulip):

if MIR has errors and AST too -> report MIR errors

nikomatsakis (Jul 04 2018 at 17:59, on Zulip):

yes

Santiago Pastorino (Jul 04 2018 at 18:00, on Zulip):

and if MIR has errors but AST don't -> allow code and report a warning

Santiago Pastorino (Jul 04 2018 at 18:00, on Zulip):

ok, checking just in case :)

nikomatsakis (Jul 04 2018 at 18:02, on Zulip):

yes

Santiago Pastorino (Jul 04 2018 at 19:57, on Zulip):

@nikomatsakis back to this thing :)

Santiago Pastorino (Jul 04 2018 at 19:57, on Zulip):

Presently, we are actually always executing AST borrowck, as well, which will need to be tweaked:

Santiago Pastorino (Jul 04 2018 at 19:57, on Zulip):

you want to execute mir borrowck and if no errors avoid ast borrowck?

Santiago Pastorino (Jul 04 2018 at 19:58, on Zulip):

from that phrase seems that

Santiago Pastorino (Jul 04 2018 at 19:58, on Zulip):

and I guess it makes sense

Santiago Pastorino (Jul 04 2018 at 19:59, on Zulip):

the issue doesn't state clearly that, so checking just in case

Santiago Pastorino (Jul 04 2018 at 19:59, on Zulip):

but I guess it makes sense

nikomatsakis (Jul 04 2018 at 20:06, on Zulip):

yes

nikomatsakis (Jul 04 2018 at 20:07, on Zulip):

I'd prefer not to run AST borrowck if we don't have to :)

Santiago Pastorino (Jul 04 2018 at 20:07, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 20:07, on Zulip):

yes make sense

nikomatsakis (Jul 04 2018 at 20:07, on Zulip):

that might actualyl be a plausible first PR

nikomatsakis (Jul 04 2018 at 20:07, on Zulip):

just skip AST borowck if borrowck=mir

Santiago Pastorino (Jul 04 2018 at 20:07, on Zulip):

yep

nikomatsakis (Jul 04 2018 at 20:07, on Zulip):

I wouldn't be surprised if this proves to be more complex than expected

nikomatsakis (Jul 04 2018 at 20:07, on Zulip):

e.g., maybe add an assertion into borrowck that it does not run if borrowck=mir

nikomatsakis (Jul 04 2018 at 20:07, on Zulip):

just to be sure :)

Santiago Pastorino (Jul 04 2018 at 20:07, on Zulip):

you want to do it if borrowck=mir ?

Santiago Pastorino (Jul 04 2018 at 20:07, on Zulip):

or just for migrate mode?

Santiago Pastorino (Jul 04 2018 at 20:08, on Zulip):

I guess in all those modes?

nikomatsakis (Jul 04 2018 at 20:08, on Zulip):

both really

nikomatsakis (Jul 04 2018 at 20:08, on Zulip):

but I'm saying, before we even add migrate mode

nikomatsakis (Jul 04 2018 at 20:08, on Zulip):

we could do the hard work for -Zborrowck=mir

Santiago Pastorino (Jul 04 2018 at 20:08, on Zulip):

yes

Santiago Pastorino (Jul 04 2018 at 20:08, on Zulip):

gonna do that

Santiago Pastorino (Jul 04 2018 at 20:08, on Zulip):

was investigating about ast borrowck errors

nikomatsakis (Jul 04 2018 at 20:08, on Zulip):

right now it does this weird thing where it "silences" them

nikomatsakis (Jul 04 2018 at 20:08, on Zulip):

I think that was because of the "unused mutability" lint

Santiago Pastorino (Jul 04 2018 at 20:08, on Zulip):

yeah trying to find all that :)

nikomatsakis (Jul 04 2018 at 20:09, on Zulip):

which we've now reimplemented in MIR borrowck

Santiago Pastorino (Jul 04 2018 at 20:09, on Zulip):

I guess it's related to that Result struct

Santiago Pastorino (Jul 04 2018 at 20:09, on Zulip):

BorrowCheckResult

Santiago Pastorino (Jul 04 2018 at 20:11, on Zulip):

unused::check :P

Santiago Pastorino (Jul 04 2018 at 20:14, on Zulip):

to be honest there's nothing inside fn borrowck that looks like silencing errors (to me)

Santiago Pastorino (Jul 04 2018 at 20:15, on Zulip):

ahh I guess it happens here build_borrowck_dataflow_data

nikomatsakis (Jul 04 2018 at 20:19, on Zulip):

it probably calls cancel() or something

Santiago Pastorino (Jul 04 2018 at 20:20, on Zulip):

not seeing that (yet)

nikomatsakis (Jul 04 2018 at 20:20, on Zulip):

look for

    fn cancel_if_wrong_origin(self,
                              mut diag: DiagnosticBuilder<'a>,
                              o: Origin)
                              -> DiagnosticBuilder<'a>
    {
        if !o.should_emit_errors(self.tcx.borrowck_mode()) {
            self.tcx.sess.diagnostic().cancel(&mut diag);
        }
        diag
    }
nikomatsakis (Jul 04 2018 at 20:20, on Zulip):

that said, I would put an asseriton right in the beginning of AST borrowck

Santiago Pastorino (Jul 04 2018 at 20:20, on Zulip):

yes

Santiago Pastorino (Jul 04 2018 at 20:21, on Zulip):

isn't it cancelling on check_loans

nikomatsakis (Jul 04 2018 at 20:21, on Zulip):

that is some other weird code

nikomatsakis (Jul 04 2018 at 20:22, on Zulip):

I think maybe that is suppressing duplicates?

Santiago Pastorino (Jul 04 2018 at 20:22, on Zulip):

I saw that cancel_if_wrong_origin

nikomatsakis (Jul 04 2018 at 20:22, on Zulip):

well in any case that use of cancel is distinct

nikomatsakis (Jul 04 2018 at 20:22, on Zulip):

it seems to be that there are two operations A and B

nikomatsakis (Jul 04 2018 at 20:22, on Zulip):

that must be mutually compatible

nikomatsakis (Jul 04 2018 at 20:22, on Zulip):

(in check_loans I mean)

Santiago Pastorino (Jul 04 2018 at 20:22, on Zulip):

ok

nikomatsakis (Jul 04 2018 at 20:22, on Zulip):

so we check whether A prevents B and whether B prevents A

nikomatsakis (Jul 04 2018 at 20:23, on Zulip):

if both are errors, we pick one arbitrarily

nikomatsakis (Jul 04 2018 at 20:23, on Zulip):

otherwise, we report the one that failed

Santiago Pastorino (Jul 04 2018 at 20:23, on Zulip):

unsure what cancel_if_wrong_origin means :P

nikomatsakis (Jul 04 2018 at 20:23, on Zulip):

it means "cancel if this came from the wrong borrowck"

Santiago Pastorino (Jul 04 2018 at 20:23, on Zulip):

ahh ok

Santiago Pastorino (Jul 04 2018 at 20:24, on Zulip):

so we don't want that anymore

Santiago Pastorino (Jul 04 2018 at 20:25, on Zulip):

I'm talking about PR #2 the first thing as you've said is to make mir mode only run mir borrowck

nikomatsakis (Jul 04 2018 at 20:26, on Zulip):

hmm well

nikomatsakis (Jul 04 2018 at 20:26, on Zulip):

we might still need similar logic in the end

nikomatsakis (Jul 04 2018 at 20:26, on Zulip):

because if we are in migrate mode

nikomatsakis (Jul 04 2018 at 20:26, on Zulip):

we want to know if there were errors

nikomatsakis (Jul 04 2018 at 20:26, on Zulip):

but we don't want to report them

Santiago Pastorino (Jul 04 2018 at 20:26, on Zulip):

ya, right

Santiago Pastorino (Jul 04 2018 at 20:26, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 20:29, on Zulip):

@nikomatsakis another question

Santiago Pastorino (Jul 04 2018 at 20:29, on Zulip):

when we run compare mode we are running ast borrowck twice, right?

Santiago Pastorino (Jul 04 2018 at 20:29, on Zulip):

one in ast mode and another one in mir mode, because there we run both modes

nikomatsakis (Jul 04 2018 at 20:30, on Zulip):

I don't think so

nikomatsakis (Jul 04 2018 at 20:30, on Zulip):

compare-mode is just another mode

nikomatsakis (Jul 04 2018 at 20:30, on Zulip):

ast-borrowck is a query

nikomatsakis (Jul 04 2018 at 20:30, on Zulip):

so it is memoized

nikomatsakis (Jul 04 2018 at 20:30, on Zulip):

no matter how many times you request it, it will only run once

nikomatsakis (Jul 04 2018 at 20:31, on Zulip):

when it is in compare mode, it adds (Ast) to its errors and otherwise emits them as normal

Santiago Pastorino (Jul 04 2018 at 20:31, on Zulip):

ahh ok, it doesn't run because it's memoized

nikomatsakis (Jul 04 2018 at 20:31, on Zulip):

when we are in AST mode, it just emits its errors

Santiago Pastorino (Jul 04 2018 at 20:31, on Zulip):

that fine

nikomatsakis (Jul 04 2018 at 20:31, on Zulip):

when we are in MIR mode, it emits nothing

Santiago Pastorino (Jul 04 2018 at 20:31, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 20:35, on Zulip):

what about this ...

Santiago Pastorino (Jul 04 2018 at 20:35, on Zulip):
    // Return early if we are not supposed to use MIR borrow checker for this function.
    return_early = !tcx.has_attr(def_id, "rustc_mir_borrowck") && !tcx.use_mir_borrowck();
Santiago Pastorino (Jul 04 2018 at 20:35, on Zulip):

the idea is that won't happen anymore?

nikomatsakis (Jul 04 2018 at 20:36, on Zulip):

we should probably just remove that #[rustc_mir_borrowck] attribute to be honest

nikomatsakis (Jul 04 2018 at 20:36, on Zulip):

I'd say we are passed that point

nikomatsakis (Jul 04 2018 at 20:36, on Zulip):

that said

nikomatsakis (Jul 04 2018 at 20:36, on Zulip):

I'm not sure what you mean by "that won't happen anymore"

nikomatsakis (Jul 04 2018 at 20:36, on Zulip):

it could still happen if we are in AST mode?

nikomatsakis (Jul 04 2018 at 20:36, on Zulip):

that is, returning early could still happen

Santiago Pastorino (Jul 04 2018 at 20:36, on Zulip):

but why in AST mode do we call MIR borrowck stuff?

nikomatsakis (Jul 04 2018 at 20:37, on Zulip):

well, I guess you could fix that, true

nikomatsakis (Jul 04 2018 at 20:37, on Zulip):

right now we call the mir_borrowck unconditionally I think

nikomatsakis (Jul 04 2018 at 20:37, on Zulip):

we could consult the mode and decide not to do so

Santiago Pastorino (Jul 04 2018 at 20:37, on Zulip):

I see

Santiago Pastorino (Jul 04 2018 at 20:59, on Zulip):

@nikomatsakis what does #[rustc_mir_borrowck] means?

Santiago Pastorino (Jul 04 2018 at 20:59, on Zulip):

you told me that I should remove that

Santiago Pastorino (Jul 04 2018 at 20:59, on Zulip):

but change to what?

Santiago Pastorino (Jul 04 2018 at 21:00, on Zulip):

#![feature(nll)]?

nikomatsakis (Jul 04 2018 at 21:02, on Zulip):

it enables MIR borrowck for just one function

nikomatsakis (Jul 04 2018 at 21:02, on Zulip):

if it is used in tests, then I think yes just replacing with #![feature(nll)] would be fine

Santiago Pastorino (Jul 04 2018 at 21:04, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 21:04, on Zulip):

and last thing, how do I check for errors after calling borrowck?

Santiago Pastorino (Jul 04 2018 at 21:04, on Zulip):

or mir_borrowck?

Santiago Pastorino (Jul 04 2018 at 21:05, on Zulip):

I've this BorrowCheckResult thing but don't see how to check for errors

nikomatsakis (Jul 04 2018 at 21:38, on Zulip):

yes, I guess we have to add a field or something

nikomatsakis (Jul 04 2018 at 21:38, on Zulip):

that indicates whether any errors occurred

Santiago Pastorino (Jul 04 2018 at 21:40, on Zulip):

but I don't know when exactly the code figures out that there's an error

nikomatsakis (Jul 04 2018 at 21:41, on Zulip):

well, it's basically exactly that function

nikomatsakis (Jul 04 2018 at 21:41, on Zulip):

the one that checks whether to "cancel" the error

nikomatsakis (Jul 04 2018 at 21:41, on Zulip):

I would think that we would have it flip some flag to true

Santiago Pastorino (Jul 04 2018 at 21:42, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 21:42, on Zulip):

will check that

Santiago Pastorino (Jul 04 2018 at 21:42, on Zulip):

another thing

Santiago Pastorino (Jul 04 2018 at 21:42, on Zulip):

was looking to remove rustc_mir_borrowck

Santiago Pastorino (Jul 04 2018 at 21:43, on Zulip):
[santiago@archlinux rust1 (z-borrowck-migrate)]$ rg rustc_mir_borrowck
src/librustc_mir/borrow_check/mod.rs
83:    return_early = !tcx.has_attr(def_id, "rustc_mir_borrowck") && !tcx.use_mir_borrowck();

src/librustc_mir/transform/rustc_peek.rs
39:        if !tcx.has_attr(def_id, "rustc_mir_borrowck") {
Santiago Pastorino (Jul 04 2018 at 21:43, on Zulip):

those are all the occurrences of it

Santiago Pastorino (Jul 04 2018 at 21:43, on Zulip):

but never see where or how it's defined

Santiago Pastorino (Jul 04 2018 at 21:44, on Zulip):

that just shows up if you use an attribute on top of a function?

Santiago Pastorino (Jul 04 2018 at 21:44, on Zulip):

as the test is doing?

Santiago Pastorino (Jul 04 2018 at 21:45, on Zulip):

I was expecting to see a white list of attrs or something

nikomatsakis (Jul 04 2018 at 21:46, on Zulip):

there is a white list; I think we might have some special rule for rustc_*

Santiago Pastorino (Jul 04 2018 at 21:46, on Zulip):

ahh ok

Santiago Pastorino (Jul 04 2018 at 21:46, on Zulip):

that's why I don't find the thing :)

Santiago Pastorino (Jul 04 2018 at 21:46, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 21:46, on Zulip):

also

nikomatsakis (Jul 04 2018 at 21:46, on Zulip):

however, if you look for other such attributes (e.g., rustc_if_this_changed), they have entries in the whitelist

nikomatsakis (Jul 04 2018 at 21:46, on Zulip):

it's possible that rustc_mir_borrowck was just incompletely removed

Santiago Pastorino (Jul 04 2018 at 21:47, on Zulip):

also

Santiago Pastorino (Jul 04 2018 at 21:47, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/rustc_peek.rs#L39

Santiago Pastorino (Jul 04 2018 at 21:47, on Zulip):

unsure what's the code about

Santiago Pastorino (Jul 04 2018 at 21:47, on Zulip):

but this is ... always? ... skipped

Santiago Pastorino (Jul 04 2018 at 21:48, on Zulip):

I mean if you don't use the attr

Santiago Pastorino (Jul 04 2018 at 21:48, on Zulip):

unless the attr is cascade from another attr

Santiago Pastorino (Jul 04 2018 at 21:48, on Zulip):

need to search for it without rustc_ to see how it's used

nikomatsakis (Jul 04 2018 at 21:50, on Zulip):

oh, that's some stuff that pnkfelix added

nikomatsakis (Jul 04 2018 at 21:50, on Zulip):

for asserting things about the dataflow state

nikomatsakis (Jul 04 2018 at 21:50, on Zulip):

alternatively, we could keep the attribute, but just error if you use it without #![feature(nll)]

nikomatsakis (Jul 04 2018 at 21:50, on Zulip):

though I think it'd probably be fine to just rip out all that code

nikomatsakis (Jul 04 2018 at 21:50, on Zulip):

but it is a way of doing some unit testing

nikomatsakis (Jul 04 2018 at 21:51, on Zulip):

in fact, people using that code appear to be the only ones using #![rustc_mir_borrowck]

Santiago Pastorino (Jul 04 2018 at 21:51, on Zulip):

I'm not understanding what's going on

Santiago Pastorino (Jul 04 2018 at 21:51, on Zulip):

don't find anything about mir_borrowck

Santiago Pastorino (Jul 04 2018 at 21:51, on Zulip):

what I find seems unrelated

nikomatsakis (Jul 04 2018 at 21:51, on Zulip):

it's really a red herring

nikomatsakis (Jul 04 2018 at 21:51, on Zulip):

this attribute I mean

Santiago Pastorino (Jul 04 2018 at 21:51, on Zulip):

in fact, people using that code appear to be the only ones using #![rustc_mir_borrowck]

exactly that's what I mean

nikomatsakis (Jul 04 2018 at 21:51, on Zulip):

but basically: it enables MIR borrowck on an individual function

nikomatsakis (Jul 04 2018 at 21:52, on Zulip):

and it also (apparently) enables this other kind of wacky unit testing stuff

nikomatsakis (Jul 04 2018 at 21:52, on Zulip):

where we recognize special fuinctions

nikomatsakis (Jul 04 2018 at 21:52, on Zulip):

and trigger extra assertions

nikomatsakis (Jul 04 2018 at 21:52, on Zulip):

e.g., to test that a variable is considered uninitialized at a particular point

nikomatsakis (Jul 04 2018 at 21:52, on Zulip):

now that we have the full MIR borrowck working, those tests are less imporatnt

nikomatsakis (Jul 04 2018 at 21:52, on Zulip):

since we can just test for ordinary errors

Santiago Pastorino (Jul 04 2018 at 21:52, on Zulip):

should we keep or not?

nikomatsakis (Jul 04 2018 at 21:52, on Zulip):

I'm not sure :)

Santiago Pastorino (Jul 04 2018 at 21:53, on Zulip):

hehe

nikomatsakis (Jul 04 2018 at 21:53, on Zulip):

I think I'd just leave it, it's not important

nikomatsakis (Jul 04 2018 at 21:53, on Zulip):

but we can simplify the logic around it

Santiago Pastorino (Jul 04 2018 at 21:53, on Zulip):

also

nikomatsakis (Jul 04 2018 at 21:53, on Zulip):

maybe @pnkfelix wants to keep it, not sure

Santiago Pastorino (Jul 04 2018 at 21:53, on Zulip):

can you point me to where is this attribute defined?

Santiago Pastorino (Jul 04 2018 at 21:53, on Zulip):

I can't find it even removing rustc_

Santiago Pastorino (Jul 04 2018 at 21:53, on Zulip):

the white list you mentioned

lqd (Jul 04 2018 at 21:56, on Zulip):

maybe https://github.com/rust-lang/rust/blob/master/src/libsyntax/feature_gate.rs#L697

Santiago Pastorino (Jul 04 2018 at 21:59, on Zulip):

@lqd yes but interestingly, rustc_mir_borrowck is not listed there

Santiago Pastorino (Jul 04 2018 at 21:59, on Zulip):

maybe rustc_mir acts like rustc_mir_* or something?

nikomatsakis (Jul 04 2018 at 22:01, on Zulip):

we don't really have a notion of "defining" attributes

nikomatsakis (Jul 04 2018 at 22:01, on Zulip):

they are just strings

nikomatsakis (Jul 04 2018 at 22:01, on Zulip):

but we do have this whitelist that prevents you from using random stuff

nikomatsakis (Jul 04 2018 at 22:01, on Zulip):

(the one that @lqd cited, yes)

nikomatsakis (Jul 04 2018 at 22:02, on Zulip):

you'd have to check, maybe it has some rules around rustc_, I can't recall

nikomatsakis (Jul 04 2018 at 22:02, on Zulip):

I don't see any obvious code of that kind

nikomatsakis (Jul 04 2018 at 22:02, on Zulip):

try rustc_foo_bar and see if you get an error :)

nikomatsakis (Jul 04 2018 at 22:03, on Zulip):

somewhere there must be such code:

https://play.rust-lang.org/?gist=dbe1c01e25907f7b9ded86396cbce0a6&version=nightly&mode=debug&edition=2015

nikomatsakis (Jul 04 2018 at 22:03, on Zulip):

ah, I see it

nikomatsakis (Jul 04 2018 at 22:03, on Zulip):

right here

Santiago Pastorino (Jul 04 2018 at 22:04, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 22:04, on Zulip):

anyway ...

Santiago Pastorino (Jul 04 2018 at 22:04, on Zulip):

@nikomatsakis what do you think I should do?

nikomatsakis (Jul 04 2018 at 22:04, on Zulip):

I think you should just leave it :) I apologize for sending you on a wild goose chase

Santiago Pastorino (Jul 04 2018 at 22:05, on Zulip):

no worries

Santiago Pastorino (Jul 04 2018 at 22:05, on Zulip):

but I mean as high level

nikomatsakis (Jul 04 2018 at 22:05, on Zulip):

but I think that we can feel free to tweak the logic around it

nikomatsakis (Jul 04 2018 at 22:05, on Zulip):

in particular, I would modify the tests that use it (there are like 4)

nikomatsakis (Jul 04 2018 at 22:05, on Zulip):

to have #![feature(nll)]

nikomatsakis (Jul 04 2018 at 22:05, on Zulip):

and say that it does not affect whether MIR borowck gets used or not

nikomatsakis (Jul 04 2018 at 22:05, on Zulip):

it's only there for this rustc_peek stuff

nikomatsakis (Jul 04 2018 at 22:05, on Zulip):

actually

nikomatsakis (Jul 04 2018 at 22:06, on Zulip):

all of that code also uses #[rustc_mir(...)]

nikomatsakis (Jul 04 2018 at 22:06, on Zulip):

so that could be the attribute we use to gate that stuff

nikomatsakis (Jul 04 2018 at 22:06, on Zulip):

see e.g. src/test/compile-fail/mir-dataflow/uninits-1.rs

Santiago Pastorino (Jul 04 2018 at 22:06, on Zulip):

yes

nikomatsakis (Jul 04 2018 at 22:06, on Zulip):

in which case I think we shoudl remove it again :)

Santiago Pastorino (Jul 04 2018 at 22:06, on Zulip):

you want to change all that and use just feature(nll)?

nikomatsakis (Jul 04 2018 at 22:06, on Zulip):

I mostly didn't want to remove the unit testing stuff

nikomatsakis (Jul 04 2018 at 22:07, on Zulip):

just as a simplification

nikomatsakis (Jul 04 2018 at 22:07, on Zulip):

this logic is complex enough

nikomatsakis (Jul 04 2018 at 22:07, on Zulip):

without adding add'l attributes in there

Santiago Pastorino (Jul 04 2018 at 22:07, on Zulip):

but if we change to feature(nll) we won't use the test anymore

Santiago Pastorino (Jul 04 2018 at 22:07, on Zulip):

the unit test

nikomatsakis (Jul 04 2018 at 22:07, on Zulip):

why?

Santiago Pastorino (Jul 04 2018 at 22:07, on Zulip):

the peek thing

nikomatsakis (Jul 04 2018 at 22:07, on Zulip):

I mean you also have to modify that peek code

nikomatsakis (Jul 04 2018 at 22:07, on Zulip):

not to look for #![rustc_mir_borrowck] but instead to look for #![rustc_mir]

Santiago Pastorino (Jul 04 2018 at 22:07, on Zulip):

I see

Santiago Pastorino (Jul 04 2018 at 22:07, on Zulip):

ok

Santiago Pastorino (Jul 04 2018 at 22:08, on Zulip):

that's the first thing to do

Santiago Pastorino (Jul 04 2018 at 22:08, on Zulip):

then I can make borrowck ast not run when borrowck=mir

nikomatsakis (Jul 04 2018 at 22:08, on Zulip):

just change if !tcx.has_attr(def_id, "rustc_mir_borrowck") to if !tcx.has_attr(def_id, "rustc_mir")

nikomatsakis (Jul 04 2018 at 22:08, on Zulip):

cool :)

Santiago Pastorino (Jul 04 2018 at 22:08, on Zulip):

and then tackle the issue I guess :)

nikomatsakis (Jul 04 2018 at 22:08, on Zulip):

heh :)

Santiago Pastorino (Jul 05 2018 at 03:29, on Zulip):

@nikomatsakis can't make cancel_if_wrong_origin modify BorrowckCtxt

Santiago Pastorino (Jul 05 2018 at 03:30, on Zulip):

to make that happen I'd need to implement BorrowckErrors for &mut BorrowckCtxt

Santiago Pastorino (Jul 05 2018 at 03:30, on Zulip):

and that's not possible because &mut BorrowckCtxt doesn't satisfy Copy

Santiago Pastorino (Jul 05 2018 at 03:30, on Zulip):
error[E0277]: the trait bound `&'a mut borrowck::BorrowckCtxt<'b, 'tcx>: std::marker::Copy` is not satisfied
   --> librustc_borrowck/borrowck/mod.rs:282:24
    |
282 | impl<'a, 'b, 'tcx: 'b> BorrowckErrors<'a> for &'a mut BorrowckCtxt<'b, 'tcx> {
    |                        ^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `&'a mut borrowck::BorrowckCtxt<'b, 'tcx>`
Santiago Pastorino (Jul 05 2018 at 03:51, on Zulip):

what about something like ...

Santiago Pastorino (Jul 05 2018 at 03:51, on Zulip):
    // (Mir-)Borrowck uses `mir_validated`, so we have to force it to
    // execute before we can steal.
    let _ = tcx.mir_borrowck(def_id);

    if let Some(_) = tcx.sess.diagnostic().get_code() {
        let _ = tcx.borrowck(def_id);
    }
Santiago Pastorino (Jul 05 2018 at 03:51, on Zulip):

will check this out tomorrow

Santiago Pastorino (Jul 05 2018 at 03:53, on Zulip):

unsure if all errors have codes...

Santiago Pastorino (Jul 05 2018 at 03:57, on Zulip):

well I guess I'd need to push the flag to diagnostic and done

Santiago Pastorino (Jul 05 2018 at 03:57, on Zulip):

@nikomatsakis will check tomorrow if you have any suggestion

Santiago Pastorino (Jul 05 2018 at 04:10, on Zulip):

last thing, now I wonder if if self.tcx.sess.has_errors() { doesn't do the trick

nikomatsakis (Jul 05 2018 at 10:20, on Zulip):

last thing, now I wonder if if self.tcx.sess.has_errors() { doesn't do the trick

well, that will be a bit "crude" — i.e., the errors could've come from anywhere

nikomatsakis (Jul 05 2018 at 10:20, on Zulip):

let me look at the code a bit more closely :)

nikomatsakis (Jul 05 2018 at 10:21, on Zulip):

@Santiago Pastorino you could use a Cell

nikomatsakis (Jul 05 2018 at 10:21, on Zulip):

however, I wonder why it has that Copy bound

nikomatsakis (Jul 05 2018 at 10:21, on Zulip):

oh, I sort of remember this

nikomatsakis (Jul 05 2018 at 10:22, on Zulip):

@Santiago Pastorino I would add a Cell; that type already has a RefCell in it

nikomatsakis (Jul 05 2018 at 10:23, on Zulip):

e.g.,

pub struct BorrowckCtxt<'a, 'tcx: 'a> {
    tcx: TyCtxt<'a, 'tcx, 'tcx>,

    // tables for the current thing we are checking; set to
    // Some in `borrowck_fn` and cleared later
    tables: &'a ty::TypeckTables<'tcx>,

    region_scope_tree: Lrc<region::ScopeTree>,

    owner_def_id: DefId,

    body: &'tcx hir::Body,

    errors_reported: Cell<bool>, // <-- added this!

    used_mut_nodes: RefCell<FxHashSet<HirId>>,
}
Santiago Pastorino (Jul 05 2018 at 16:16, on Zulip):

so ...

Santiago Pastorino (Jul 05 2018 at 16:16, on Zulip):

cancel_if_wrong_origin is defined on both TyCtxt and BorrowckCtxt

Santiago Pastorino (Jul 05 2018 at 16:17, on Zulip):

it seems that AST borrow checker uses the one in BorrowckCtxt and MIR borrow checker uses the one in TyCtxt

Santiago Pastorino (Jul 05 2018 at 16:17, on Zulip):

for the first thing I need to do is to skip AST borrow check if MIR borrow check accepts the code

Santiago Pastorino (Jul 05 2018 at 16:18, on Zulip):

should I add the flag to TyCtxt?

Santiago Pastorino (Jul 05 2018 at 16:18, on Zulip):

it's seems to me like polluting things a lot with stuff

Santiago Pastorino (Jul 05 2018 at 16:18, on Zulip):

@nikomatsakis unsure what you think about it

Santiago Pastorino (Jul 05 2018 at 16:18, on Zulip):

and unsure why we don't have a borrow check context for MIR

nikomatsakis (Jul 05 2018 at 16:32, on Zulip):

should I add the flag to TyCtxt?

definitely not

nikomatsakis (Jul 05 2018 at 16:32, on Zulip):

hmm I think what we should do with the MIR errors

nikomatsakis (Jul 05 2018 at 16:32, on Zulip):

is buffer them somehow

nikomatsakis (Jul 05 2018 at 16:32, on Zulip):

MIR errors should never "cancel"

nikomatsakis (Jul 05 2018 at 16:32, on Zulip):

but they may need to be converted to lints

nikomatsakis (Jul 05 2018 at 16:33, on Zulip):

did you make progress on not running AST when in borrowck=mir mode?

Santiago Pastorino (Jul 05 2018 at 16:40, on Zulip):

ok

Santiago Pastorino (Jul 05 2018 at 16:41, on Zulip):

I guess I can make just that change and push

Santiago Pastorino (Jul 05 2018 at 16:46, on Zulip):
-    let _ = tcx.borrowck(def_id);
+
+    if tcx.borrowck_mode() != BorrowckMode::Mir {
+        let _ = tcx.borrowck(def_id);
+    }
Santiago Pastorino (Jul 05 2018 at 16:46, on Zulip):

naively did this

Santiago Pastorino (Jul 05 2018 at 16:46, on Zulip):

@nikomatsakis is there something else to consider?

Santiago Pastorino (Jul 05 2018 at 16:47, on Zulip):

I'm compiling and going to run tests

Santiago Pastorino (Jul 05 2018 at 16:47, on Zulip):

well I can make that less hacky

Santiago Pastorino (Jul 05 2018 at 16:47, on Zulip):

I guess I should change use_ast() fn to check for that

nikomatsakis (Jul 05 2018 at 17:02, on Zulip):

@Santiago Pastorino I would add an assertion in the AST borrowck the borrowck mode is not Mir. Then we'll find out if there is something else to consider. :)

Santiago Pastorino (Jul 05 2018 at 17:05, on Zulip):

done!

nikomatsakis (Jul 05 2018 at 17:06, on Zulip):

nice

Santiago Pastorino (Jul 05 2018 at 17:15, on Zulip):

and everything fails

Santiago Pastorino (Jul 05 2018 at 17:15, on Zulip):

hehe

Santiago Pastorino (Jul 05 2018 at 17:34, on Zulip):

@nikomatsakis rustc::ty::query::__query_compute::borrowck calls the old borrowck

Santiago Pastorino (Jul 05 2018 at 17:34, on Zulip):

investigating

nikomatsakis (Jul 05 2018 at 17:35, on Zulip):

@Santiago Pastorino that is just the query wrapper -- who calls that ?

Santiago Pastorino (Jul 05 2018 at 17:36, on Zulip):

9: 0x7fbbeb5df69c - rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::borrowck<'tcx>>::compute::h79b1dcd73cb26cfd

Santiago Pastorino (Jul 05 2018 at 17:38, on Zulip):

I see

Santiago Pastorino (Jul 05 2018 at 17:38, on Zulip):

check_crate is the thing

Santiago Pastorino (Jul 05 2018 at 17:38, on Zulip):
pub fn check_crate<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>) {
    tcx.par_body_owners(|body_owner_def_id| {
        tcx.borrowck(body_owner_def_id);
    });
}
nikomatsakis (Jul 05 2018 at 17:38, on Zulip):

makes sense

nikomatsakis (Jul 05 2018 at 17:38, on Zulip):

we should also not call that :)

Santiago Pastorino (Jul 05 2018 at 17:39, on Zulip):

explain :)

Santiago Pastorino (Jul 05 2018 at 17:39, on Zulip):

what does this do?

nikomatsakis (Jul 05 2018 at 17:40, on Zulip):

it invokes borrowck on every function

nikomatsakis (Jul 05 2018 at 17:40, on Zulip):

that basically makes sure that borrowck is run

nikomatsakis (Jul 05 2018 at 17:40, on Zulip):

before compilation completes

Santiago Pastorino (Jul 05 2018 at 17:40, on Zulip):

why is called check_crate if it does that?

Santiago Pastorino (Jul 05 2018 at 17:41, on Zulip):

and why that's not relevant on mir mode?

nikomatsakis (Jul 05 2018 at 17:43, on Zulip):

why is called check_crate if it does that?

because that is what "check crate" means?

nikomatsakis (Jul 05 2018 at 17:43, on Zulip):

that is, it checks the whole crate

nikomatsakis (Jul 05 2018 at 17:43, on Zulip):

(which is done by checking all the functions in the crate)

Santiago Pastorino (Jul 05 2018 at 17:43, on Zulip):

:+1:

nikomatsakis (Jul 05 2018 at 17:43, on Zulip):

and why that's not relevant on mir mode?

because it is running the AST borrowck

nikomatsakis (Jul 05 2018 at 17:43, on Zulip):

we have a similar function for MIR borrowck already I think

Santiago Pastorino (Jul 05 2018 at 17:43, on Zulip):

ahh ok ok

nikomatsakis (Jul 05 2018 at 17:44, on Zulip):

ok, we don't have a function but the code is inlined into the driver

            time(sess,
                 "MIR borrow checking",
                 || tcx.par_body_owners(|def_id| { tcx.mir_borrowck(def_id); }));
Santiago Pastorino (Jul 05 2018 at 17:47, on Zulip):

:+1:

Santiago Pastorino (Jul 05 2018 at 18:02, on Zulip):

@nikomatsakis btw, shouldn't we do the same on ast mode?

Santiago Pastorino (Jul 05 2018 at 18:02, on Zulip):

I mean, avoid executing mir borrowck when mode=ast

Santiago Pastorino (Jul 05 2018 at 18:03, on Zulip):

mode=ast only ast borrowck
mode=mir only mir borrowck
mode=compare both
mode=migrate both

nikomatsakis (Jul 05 2018 at 18:03, on Zulip):

probably. we already just "skip over" MIR borrowck in that case though

Santiago Pastorino (Jul 05 2018 at 18:03, on Zulip):

I know is not that important but if someone for perf reasons meanwhile this thing is shaped or something want to avoid nll I guess it's possible to do that

nikomatsakis (Jul 05 2018 at 18:03, on Zulip):

but it'd probably be cleaner to never invoke the query to begin with, if it's easy to do

Santiago Pastorino (Jul 05 2018 at 18:04, on Zulip):

but it'd probably be cleaner to never invoke the query to begin with, if it's easy to do

explain the meaning of this, please :)

Santiago Pastorino (Jul 05 2018 at 18:04, on Zulip):

querys? how does the thing work more or less?

Santiago Pastorino (Jul 05 2018 at 18:05, on Zulip):

I guess what you mean but want more info

Santiago Pastorino (Jul 05 2018 at 18:05, on Zulip):

you probably meant that this is called several times and structures collected are memoized

Santiago Pastorino (Jul 05 2018 at 18:05, on Zulip):

so on different calls you will get memoized results

Santiago Pastorino (Jul 05 2018 at 18:06, on Zulip):

I guess what you're talking about is something like that (?)

nikomatsakis (Jul 05 2018 at 18:07, on Zulip):

I'm not sure what you want me to explain :)

nikomatsakis (Jul 05 2018 at 18:07, on Zulip):

there is a mir_borrowck query

nikomatsakis (Jul 05 2018 at 18:07, on Zulip):

it is invoked from a few places

nikomatsakis (Jul 05 2018 at 18:07, on Zulip):

right now, we always invoke the query, but sometimes it does nothing

nikomatsakis (Jul 05 2018 at 18:07, on Zulip):

we've been modifying the ast-borrowck so that the query is only invoked if it is needed

nikomatsakis (Jul 05 2018 at 18:08, on Zulip):

I don't think we have to do the same for MIR

nikomatsakis (Jul 05 2018 at 18:08, on Zulip):

but we could

nikomatsakis (Jul 05 2018 at 18:08, on Zulip):

the reason it was important for AST is because -- in the migration mode --

nikomatsakis (Jul 05 2018 at 18:08, on Zulip):

we want to be able to run the MIR and only run the AST borrowck for those functions that need it

nikomatsakis (Jul 05 2018 at 18:08, on Zulip):

so if we are always invoking it on all functions (as we do now)

nikomatsakis (Jul 05 2018 at 18:08, on Zulip):

that won't work

Santiago Pastorino (Jul 05 2018 at 18:09, on Zulip):

yeah, was just surprised that you called that a query :)

Santiago Pastorino (Jul 05 2018 at 18:09, on Zulip):

I guess there are reasons for that, that I don't see

nikomatsakis (Jul 05 2018 at 18:15, on Zulip):

@Santiago Pastorino https://rust-lang-nursery.github.io/rustc-guide/query.html

nikomatsakis (Jul 05 2018 at 18:15, on Zulip):

I mean a very specific thing by query

Santiago Pastorino (Jul 05 2018 at 18:17, on Zulip):

@nikomatsakis I don't know why I never read the rustc-guide :(

Santiago Pastorino (Jul 05 2018 at 18:17, on Zulip):

it's in my todo list

Santiago Pastorino (Jul 05 2018 at 18:18, on Zulip):

it takes time :/

Santiago Pastorino (Jul 05 2018 at 18:18, on Zulip):

should I open a PR with just this?

Santiago Pastorino (Jul 05 2018 at 18:18, on Zulip):

or land all at once?

nikomatsakis (Jul 05 2018 at 18:18, on Zulip):

I would say yes

nikomatsakis (Jul 05 2018 at 18:18, on Zulip):

it may be a perf win :)

nikomatsakis (Jul 05 2018 at 18:18, on Zulip):

probably a small one

Santiago Pastorino (Jul 05 2018 at 18:18, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/pull/52083

Santiago Pastorino (Jul 05 2018 at 18:18, on Zulip):

would this be small?

Santiago Pastorino (Jul 05 2018 at 18:19, on Zulip):

I thought it was going to be huge

Santiago Pastorino (Jul 05 2018 at 18:19, on Zulip):

I mean, skipping the whole AST borrowck ...

nikomatsakis (Jul 05 2018 at 18:21, on Zulip):

the AST borrowck is pretty fast :)

Santiago Pastorino (Jul 05 2018 at 18:21, on Zulip):

ok :)

Santiago Pastorino (Jul 05 2018 at 18:22, on Zulip):

btw, 2 stderr files changed in here https://github.com/rust-lang/rust/pull/52083

Santiago Pastorino (Jul 05 2018 at 18:22, on Zulip):

unsure why

Santiago Pastorino (Jul 05 2018 at 18:22, on Zulip):

changes doesn't have any semantic

Santiago Pastorino (Jul 05 2018 at 18:22, on Zulip):

just moved a couple of lines around

Santiago Pastorino (Jul 05 2018 at 18:22, on Zulip):

also seems better now :P

Santiago Pastorino (Jul 05 2018 at 18:22, on Zulip):

but unsure what made that happen

nikomatsakis (Jul 05 2018 at 18:23, on Zulip):

yeah, must be some change in evaluation order, seems ok

nikomatsakis (Jul 05 2018 at 18:23, on Zulip):

I left one nit

Santiago Pastorino (Jul 05 2018 at 18:25, on Zulip):

@nikomatsakis actually what I did is wrong

Santiago Pastorino (Jul 05 2018 at 18:26, on Zulip):

that should be assert!(tcx.use_ast_borrowck())

Santiago Pastorino (Jul 05 2018 at 18:26, on Zulip):

use_ast_borrowck() consider the match already

nikomatsakis (Jul 05 2018 at 18:26, on Zulip):

good point

Santiago Pastorino (Jul 05 2018 at 18:26, on Zulip):

and does the right thing

nikomatsakis (Jul 05 2018 at 18:26, on Zulip):

yes, seems better

Santiago Pastorino (Jul 05 2018 at 18:27, on Zulip):

so .. now

Santiago Pastorino (Jul 05 2018 at 18:27, on Zulip):

how to tackle the migrate thing?

nikomatsakis (Jul 05 2018 at 18:28, on Zulip):

hmm ok so

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

one first step might be

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

to modify the MIR borrowck

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

so that instead of emitting the errors right away

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

it buffers them up (per function)

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

and then emits them at the end

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

though I suppose that isn't .. strictly needed

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

I could see two ways of going about things

nikomatsakis (Jul 05 2018 at 18:29, on Zulip):

in one version, we buffer up the errors

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

and then if that buffer is non-empty, we run the AST borrowck

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

and if it comes back with errors

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

(which we suppress)

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

then we emit the errors from the buffer

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

else we go back over them

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

and convert them to warnings

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

and emit them

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

another way to do it though

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

would be that when we are about to emit an error

nikomatsakis (Jul 05 2018 at 18:30, on Zulip):

we run the AST borrowck just as before

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

and convert that pending error to a warning if the AST borrowck gets no errors

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

point is, we don't buffer, we just invoke the AST borrowck for each error as we are emitting it

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

since it is memoized

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

that won't really cost anything

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

I guess either way the very first step is to add a Migrate mode

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

and to modify the AST borrowck

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

er, wait

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

well, yes

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

and modify the AST borrowck so that it includes a boolean flag

nikomatsakis (Jul 05 2018 at 18:31, on Zulip):

indicating whether any errors occurred

nikomatsakis (Jul 05 2018 at 18:32, on Zulip):

actually maybe just add that flag :)

nikomatsakis (Jul 05 2018 at 18:32, on Zulip):

that's at least an initial commit

nikomatsakis (Jul 05 2018 at 18:32, on Zulip):

make sense?

Santiago Pastorino (Jul 05 2018 at 18:36, on Zulip):

brewing coffee :smiley:, as soon as I have my :coffee:️ will check this

nikomatsakis (Jul 05 2018 at 18:40, on Zulip):

I would never come between a person and their :coffee:

Santiago Pastorino (Jul 05 2018 at 19:09, on Zulip):

and it's also cold here, so I needed something hot to warm myself a bit :)

Santiago Pastorino (Jul 05 2018 at 19:27, on Zulip):

@nikomatsakis back, so read it and the reasoning makes sense

Santiago Pastorino (Jul 05 2018 at 19:27, on Zulip):

didn't understand 100% the steps

Santiago Pastorino (Jul 05 2018 at 19:27, on Zulip):

but first of all

nikomatsakis (Jul 05 2018 at 19:27, on Zulip):

I think I would go for the non-buffering version

nikomatsakis (Jul 05 2018 at 19:27, on Zulip):

seems easier

Santiago Pastorino (Jul 05 2018 at 19:27, on Zulip):

ahh ok, I was going to ask that

Santiago Pastorino (Jul 05 2018 at 19:27, on Zulip):

I was going to go with the buffered version :P

Santiago Pastorino (Jul 05 2018 at 19:27, on Zulip):

yeah seems easier, but doesn't seem better

Santiago Pastorino (Jul 05 2018 at 19:28, on Zulip):

to me the first approach seems cleaner

nikomatsakis (Jul 05 2018 at 19:28, on Zulip):

I don't have a strong opinion =) I'd be ok w/ buffering

Santiago Pastorino (Jul 05 2018 at 19:28, on Zulip):

anyway I can go with the non-buffering and we could revisit at some point

nikomatsakis (Jul 05 2018 at 19:28, on Zulip):

I see some advantages to that too

nikomatsakis (Jul 05 2018 at 19:28, on Zulip):

in particular, we might be able to combine errors later on

nikomatsakis (Jul 05 2018 at 19:28, on Zulip):

so maybe it's a useful refactoring

Santiago Pastorino (Jul 05 2018 at 19:28, on Zulip):

yeah

nikomatsakis (Jul 05 2018 at 19:28, on Zulip):

down the road

nikomatsakis (Jul 05 2018 at 19:29, on Zulip):

might also be helpful for supressing duplicates etc

Santiago Pastorino (Jul 05 2018 at 19:29, on Zulip):

unsure how much do we need to change

Santiago Pastorino (Jul 05 2018 at 19:29, on Zulip):

:P

nikomatsakis (Jul 05 2018 at 19:29, on Zulip):

probably not very much

Santiago Pastorino (Jul 05 2018 at 19:29, on Zulip):

ok, let's do it then :)

nikomatsakis (Jul 05 2018 at 19:29, on Zulip):

but I have to look

Santiago Pastorino (Jul 05 2018 at 19:29, on Zulip):

let me ask you some questions because I have no idea of things, hehe :P

nikomatsakis (Jul 05 2018 at 19:30, on Zulip):

we already have these DiagnosticBuilder<'_> things that represent messages

Santiago Pastorino (Jul 05 2018 at 19:30, on Zulip):

I mean, what needs to be done is completely clear

Santiago Pastorino (Jul 05 2018 at 19:30, on Zulip):

the thing is where are the things

Santiago Pastorino (Jul 05 2018 at 19:30, on Zulip):

where stuff is happening

Santiago Pastorino (Jul 05 2018 at 19:30, on Zulip):

etc

Santiago Pastorino (Jul 05 2018 at 19:30, on Zulip):

so ...

nikomatsakis (Jul 05 2018 at 19:30, on Zulip):

probably the very first step

nikomatsakis (Jul 05 2018 at 19:30, on Zulip):

is modifying the AST borrowck

nikomatsakis (Jul 05 2018 at 19:30, on Zulip):

to record whether it had any errors

nikomatsakis (Jul 05 2018 at 19:30, on Zulip):

which is I think just a few lines :)

nikomatsakis (Jul 05 2018 at 19:31, on Zulip):

basically adding a error_count: Cell<usize> and having that method do error_count.set(error_count.get() + 1)

Santiago Pastorino (Jul 05 2018 at 19:31, on Zulip):

yep

Santiago Pastorino (Jul 05 2018 at 19:31, on Zulip):

I've code like that already stashed

nikomatsakis (Jul 05 2018 at 19:31, on Zulip):

ok

nikomatsakis (Jul 05 2018 at 19:31, on Zulip):

in that case the next thing I would imagine is to setup the buffering

nikomatsakis (Jul 05 2018 at 19:31, on Zulip):

without changing existing behavior

nikomatsakis (Jul 05 2018 at 19:32, on Zulip):

in the MIR borrow check I mean

Santiago Pastorino (Jul 05 2018 at 19:32, on Zulip):

yeah yeah

Santiago Pastorino (Jul 05 2018 at 19:32, on Zulip):

the buffering happens there, yeah

Santiago Pastorino (Jul 05 2018 at 19:32, on Zulip):

the thing is ... where are currently stored errors?

Santiago Pastorino (Jul 05 2018 at 19:32, on Zulip):

through diagnostic(s)?

nikomatsakis (Jul 05 2018 at 19:46, on Zulip):

we don't store errors today

nikomatsakis (Jul 05 2018 at 19:46, on Zulip):

we emit them and forget about them

Santiago Pastorino (Jul 05 2018 at 19:47, on Zulip):

ok, so I should make emit not emit and store them?

Santiago Pastorino (Jul 05 2018 at 19:47, on Zulip):

and change emit name to store :P

Santiago Pastorino (Jul 05 2018 at 19:47, on Zulip):

or something

Santiago Pastorino (Jul 05 2018 at 19:47, on Zulip):

and then search for a place very late in the process to call a real emit thing

nikomatsakis (Jul 05 2018 at 19:51, on Zulip):

if you want to buffer them, they need to be put into a vector

nikomatsakis (Jul 05 2018 at 19:51, on Zulip):

and then at the end we would iterate over and invoke emit()

nikomatsakis (Jul 05 2018 at 19:54, on Zulip):

@Santiago Pastorino here is an example of the code

nikomatsakis (Jul 05 2018 at 19:54, on Zulip):

we create the error here

nikomatsakis (Jul 05 2018 at 19:55, on Zulip):

and emit here

nikomatsakis (Jul 05 2018 at 19:55, on Zulip):

so we could hold off on that final call to emit()

nikomatsakis (Jul 05 2018 at 19:56, on Zulip):

they ought to have the type DiagnosticBuilder<'cx>

Santiago Pastorino (Jul 05 2018 at 19:59, on Zulip):

ok will check all this in a bit

nikomatsakis (Jul 05 2018 at 23:02, on Zulip):

@Santiago Pastorino looks like a decent perf win from your PR :)

Santiago Pastorino (Jul 06 2018 at 20:03, on Zulip):

@nikomatsakis tackling this now

Santiago Pastorino (Jul 06 2018 at 20:03, on Zulip):

so ...

Santiago Pastorino (Jul 06 2018 at 20:03, on Zulip):

I understood your comment about this code https://github.com/rust-lang/rust/blob/94eb1760551096363ec04e42367b6b195592dbd8/src/librustc_mir/borrow_check/error_reporting.rs#L179-L199

Santiago Pastorino (Jul 06 2018 at 20:03, on Zulip):

basically, I should put the err on a buffer

Santiago Pastorino (Jul 06 2018 at 20:03, on Zulip):

and remove the emit

Santiago Pastorino (Jul 06 2018 at 20:04, on Zulip):

my question is ... when is that last moment in time where I should emit?

nikomatsakis (Jul 06 2018 at 20:04, on Zulip):

like, when do you iterate over that buffer?

Santiago Pastorino (Jul 06 2018 at 20:04, on Zulip):

yes

Santiago Pastorino (Jul 06 2018 at 20:04, on Zulip):

at some point I should iterate and call emit over all the errs

Santiago Pastorino (Jul 06 2018 at 20:05, on Zulip):

but when is a good moment to do that

nikomatsakis (Jul 06 2018 at 20:05, on Zulip):

somewhere around the end of do_mir_borrowck, I guess

Santiago Pastorino (Jul 06 2018 at 20:05, on Zulip):

ok

Santiago Pastorino (Jul 06 2018 at 20:05, on Zulip):

also ... you wanted me to split this in some way? or just do all at once?

nikomatsakis (Jul 06 2018 at 20:05, on Zulip):

split what?

nikomatsakis (Jul 06 2018 at 20:05, on Zulip):

oh

Santiago Pastorino (Jul 06 2018 at 20:05, on Zulip):

I meant, I remember you talking about the option on one side

nikomatsakis (Jul 06 2018 at 20:06, on Zulip):

I'd probably do a "buffer" step that doesn't change anything else

Santiago Pastorino (Jul 06 2018 at 20:06, on Zulip):

and this thing on the other side

Santiago Pastorino (Jul 06 2018 at 20:06, on Zulip):

ok

nikomatsakis (Jul 06 2018 at 20:06, on Zulip):

well we were discussing whether to buffer or not

nikomatsakis (Jul 06 2018 at 20:06, on Zulip):

I think the "commits" (maybe PRs?) are:

Santiago Pastorino (Jul 06 2018 at 20:07, on Zulip):

ok

Santiago Pastorino (Jul 06 2018 at 21:01, on Zulip):

@nikomatsakis all the errors of mir borrowck are the ones reported in src/librustc_mir

Santiago Pastorino (Jul 06 2018 at 21:01, on Zulip):

I was looking for rg "emit\(" src/librustc_mir to start

Santiago Pastorino (Jul 06 2018 at 21:02, on Zulip):

but unsure if all the stuff there is what's needed, investigating ...

Santiago Pastorino (Jul 06 2018 at 21:03, on Zulip):

for instance ...

Santiago Pastorino (Jul 06 2018 at 21:03, on Zulip):

what about https://github.com/rust-lang/rust/blob/94eb1760551096363ec04e42367b6b195592dbd8/src/librustc_mir/borrow_check/move_errors.rs#L278 ?

nikomatsakis (Jul 06 2018 at 21:09, on Zulip):

all the errors of mir borrowck are the ones reported in src/librustc_mir

I believe so, yes

nikomatsakis (Jul 06 2018 at 21:09, on Zulip):

what about https://github.com/rust-lang/rust/blob/94eb1760551096363ec04e42367b6b195592dbd8/src/librustc_mir/borrow_check/move_errors.rs#L278 ?

that is also a borrowck error, yes

Santiago Pastorino (Jul 06 2018 at 21:15, on Zulip):

there's a lot of stuff

Santiago Pastorino (Jul 06 2018 at 21:16, on Zulip):

there's no borrowckctxt there, though

Santiago Pastorino (Jul 06 2018 at 21:17, on Zulip):

I have the MoveErrorCtxt

nikomatsakis (Jul 06 2018 at 21:31, on Zulip):

it's called from report_move_errors — maybe pass in an &mut Vec there or something

Santiago Pastorino (Jul 06 2018 at 22:13, on Zulip):

yes, I can pass stuff around but when this kind of stuff happens it starts to feel hacky to me

Santiago Pastorino (Jul 06 2018 at 22:13, on Zulip):

but if you say it's good it's good :)

Santiago Pastorino (Jul 06 2018 at 22:14, on Zulip):

shouldn't most of these functions have also a more general purpose context?

Santiago Pastorino (Jul 06 2018 at 22:14, on Zulip):

I mean, besides from this particular context

Santiago Pastorino (Jul 06 2018 at 22:16, on Zulip):

I still don't know all the contexts that are around and their connections but in lot of cases happened something like this and we kept adding parameters to functions

Santiago Pastorino (Jul 06 2018 at 22:16, on Zulip):

maybe if all the functions receive some kind of compilation context or something kind of general

Santiago Pastorino (Jul 06 2018 at 22:17, on Zulip):

unsure ...

nikomatsakis (Jul 06 2018 at 22:34, on Zulip):

I don't know, both have their appeal. A "master context" means everything is less independent

nikomatsakis (Jul 06 2018 at 22:34, on Zulip):

but in any case there is no such context at present

nikomatsakis (Jul 06 2018 at 22:34, on Zulip):

you could also make the MoveData accumulate errors into an internal vector

nikomatsakis (Jul 06 2018 at 22:34, on Zulip):

and then just return that vector

Matthew Jasper (Jul 07 2018 at 08:54, on Zulip):

@Santiago Pastorino I'm planning to change to a mir borrowck context for move errors.

Santiago Pastorino (Jul 07 2018 at 15:39, on Zulip):

cool, I guess in this issue I'm going to pass stuff around

Santiago Pastorino (Jul 07 2018 at 15:40, on Zulip):

then I guess we can move that inside the context when you add that context

pnkfelix (Jul 12 2018 at 18:04, on Zulip):

hi @Santiago Pastorino

Santiago Pastorino (Jul 12 2018 at 18:05, on Zulip):

hi

pnkfelix (Jul 12 2018 at 18:05, on Zulip):

I assume a text-based chat in here is good

Santiago Pastorino (Jul 12 2018 at 18:05, on Zulip):

:+1:

pnkfelix (Jul 12 2018 at 18:05, on Zulip):

so, I haven't attempted to look at your branch

Santiago Pastorino (Jul 12 2018 at 18:05, on Zulip):

so ... I was lacking a bit of time so I didn't make a lot of progress

pnkfelix (Jul 12 2018 at 18:05, on Zulip):

(assuming its still the same branch as the one you shared)

Santiago Pastorino (Jul 12 2018 at 18:05, on Zulip):

yes

pnkfelix (Jul 12 2018 at 18:06, on Zulip):

okay. Lets just talk about where you are and where you want to go

Santiago Pastorino (Jul 12 2018 at 18:06, on Zulip):

current status is ...

Santiago Pastorino (Jul 12 2018 at 18:06, on Zulip):

well basically the idea we discussed with Niko was ...

Santiago Pastorino (Jul 12 2018 at 18:06, on Zulip):
- modify MIR borrowck to buffer up errors
- add a flag to AST borowck so that it indicates whether it got errors or not in the result of its query (without necessarily emitting them)
- combine those two?
Santiago Pastorino (Jul 12 2018 at 18:06, on Zulip):

I was modifying the MIR borrowck to buffer up errors

Santiago Pastorino (Jul 12 2018 at 18:06, on Zulip):

have made that in a couple of places

Santiago Pastorino (Jul 12 2018 at 18:06, on Zulip):

there's still some cases missing

Santiago Pastorino (Jul 12 2018 at 18:07, on Zulip):

which I haven't looked at the code to see how hard it could be

pnkfelix (Jul 12 2018 at 18:07, on Zulip):

okay

Santiago Pastorino (Jul 12 2018 at 18:07, on Zulip):

mainly may lack the context in some specific areas

Santiago Pastorino (Jul 12 2018 at 18:07, on Zulip):

I think after this, the rest should be easy

Santiago Pastorino (Jul 12 2018 at 18:07, on Zulip):

and this may also be easy :P

Santiago Pastorino (Jul 12 2018 at 18:07, on Zulip):

haven't checked properly yet

Santiago Pastorino (Jul 12 2018 at 18:08, on Zulip):
[santiago@archlinux rust1 (z-borrowck-migrate)]$ rg "\.emit\(\)" src/librustc_mir
src/librustc_mir/monomorphize/collector.rs
514:        diag.emit();

src/librustc_mir/transform/qualify_consts.rs
178:            err.emit();
198:            err.emit();
485:                    err.emit()
516:                                    err.emit();
666:                            err.emit();
763:                            err.emit();
786:                        .emit();
805:                    err.emit();
936:                            err.emit();
963:                    err.emit();
1005:                            .emit();
1058:                    err.emit();

src/librustc_mir/transform/check_unsafety.rs
432:    db.emit();
475:                    .emit();

src/librustc_mir/borrow_check/mod.rs
294:        err.emit();
327:                .emit();
1365:                .emit();
1883:        err.emit();

src/librustc_mir/borrow_check/nll/mod.rs
342:        err.emit();
349:        err.emit();

src/librustc_mir/hair/pattern/mod.rs
391:                                err.emit();

src/librustc_mir/hair/pattern/check_match.rs
146:                    ).emit();
152:                    ).emit();
241:                    err.emit();
304:            diag.emit();
333:                        err.emit();
390:                                        .emit();
421:                                            .emit();
441:                            err.emit();
503:                .emit();
537:                .emit();
542:                .emit();
548:                    .emit();
606:                    .emit();
617:                    .emit();
648:                        .emit();

src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs
257:        diag.emit();
pnkfelix (Jul 12 2018 at 18:08, on Zulip):

is your latest status pushed to your repo up on github?

Santiago Pastorino (Jul 12 2018 at 18:08, on Zulip):

this ^^^ may show more or less the places where I need to change emit calls and buffer the stuff

Santiago Pastorino (Jul 12 2018 at 18:08, on Zulip):

is your latest status pushed to your repo up on github?

yes

Santiago Pastorino (Jul 12 2018 at 18:09, on Zulip):

I'm going to continue now or in a few minutes with the task

Santiago Pastorino (Jul 12 2018 at 18:09, on Zulip):

I'm not sure if you have any particular suggestion

pnkfelix (Jul 12 2018 at 18:09, on Zulip):

well let me quickly look

pnkfelix (Jul 12 2018 at 18:09, on Zulip):

the main thing I wanted to immediate check in about

pnkfelix (Jul 12 2018 at 18:09, on Zulip):

was about that idea we had discussed

pnkfelix (Jul 12 2018 at 18:10, on Zulip):

about trying to make the replacement for the emit() calls look ... well, look something much like "just another method call" (but on a different receiving object)

Santiago Pastorino (Jul 12 2018 at 18:11, on Zulip):

yes, you talked about changing emit to signal

pnkfelix (Jul 12 2018 at 18:11, on Zulip):

The name didn't matter

Santiago Pastorino (Jul 12 2018 at 18:11, on Zulip):

which may emit or may buffer depending on some setting

pnkfelix (Jul 12 2018 at 18:11, on Zulip):

I'd be fine with receiver.eventually_emit(..)

Santiago Pastorino (Jul 12 2018 at 18:11, on Zulip):

:+1:

Santiago Pastorino (Jul 12 2018 at 18:11, on Zulip):

I liked signal :P

Santiago Pastorino (Jul 12 2018 at 18:11, on Zulip):

well unsure if it's the best name ...

Santiago Pastorino (Jul 12 2018 at 18:11, on Zulip):

whatever you prefer

pnkfelix (Jul 12 2018 at 18:11, on Zulip):

signal is also fine; it just may not be a meaningful difference to some people

pnkfelix (Jul 12 2018 at 18:12, on Zulip):

I see "eventually emit" as trying to express the idea that this might be delayed

Santiago Pastorino (Jul 12 2018 at 18:12, on Zulip):

can't discuss names, english is not my best ability :')

Santiago Pastorino (Jul 12 2018 at 18:12, on Zulip):

ok, eventually_emit then :)

Santiago Pastorino (Jul 12 2018 at 18:12, on Zulip):

you convinced me quickly

pnkfelix (Jul 12 2018 at 18:12, on Zulip):

okay I see, you really are just doing self.errors_buffer.push(err). Well, if that works then it should be easy to change it later

pnkfelix (Jul 12 2018 at 18:12, on Zulip):

its up to you.

Santiago Pastorino (Jul 12 2018 at 18:13, on Zulip):

so ... what if I just go ahead ... was going to say exactly that

Santiago Pastorino (Jul 12 2018 at 18:13, on Zulip):

I think it would be easier to continue and leave that to another PR or same PR another commit

pnkfelix (Jul 12 2018 at 18:13, on Zulip):

(deleted)

pnkfelix (Jul 12 2018 at 18:14, on Zulip):

/me is still getting used to Zulip idioms

pnkfelix (Jul 12 2018 at 18:14, on Zulip):

Okay from what I see in your branch, this looks good to me

Santiago Pastorino (Jul 12 2018 at 18:14, on Zulip):

hehe, I probably didn't get used yet :P

Santiago Pastorino (Jul 12 2018 at 18:15, on Zulip):

ok, so ... I can just start to look around and in any case ask questions

pnkfelix (Jul 12 2018 at 18:15, on Zulip):

Ah

pnkfelix (Jul 12 2018 at 18:15, on Zulip):

I remember why you might want to do it as eventually_emit (or signal etc), now

pnkfelix (Jul 12 2018 at 18:16, on Zulip):

if you do it as recv.eventually_emit(..) now, then you can truly retain the old "immediately emit" behavior by default and put the new behavior under a debugflag

pnkfelix (Jul 12 2018 at 18:16, on Zulip):

that in turn will mean that you should be able to run the compiler test suite out of the box

pnkfelix (Jul 12 2018 at 18:17, on Zulip):

Putting in buffering and buffering alone, via the ctxt.errors_buffer.push(..) style, will force you to actually figure out how to get those errors emitted

Santiago Pastorino (Jul 12 2018 at 18:17, on Zulip):

I see :)

pnkfelix (Jul 12 2018 at 18:17, on Zulip):

and either you'll figure out how to emit them in exactly the same way that they get emitted today

pnkfelix (Jul 12 2018 at 18:17, on Zulip):

(which means all the tests will immediately pass)

Santiago Pastorino (Jul 12 2018 at 18:17, on Zulip):

anyway, right now the changes are just buffer to call emit at the end

pnkfelix (Jul 12 2018 at 18:18, on Zulip):

or you'll have some changes in your output (even if its just in the order that errors are emitted)

Santiago Pastorino (Jul 12 2018 at 18:18, on Zulip):

there's nothing changed

pnkfelix (Jul 12 2018 at 18:18, on Zulip):

and you'll then have to verify that those changes are correct

pnkfelix (Jul 12 2018 at 18:18, on Zulip):

You sound quite confident that there won't be any visible effect

Santiago Pastorino (Jul 12 2018 at 18:18, on Zulip):

or you'll have some changes in your output (even if its just in the order that errors are emitted)

the order should be the same, I'm doing FIFO

pnkfelix (Jul 12 2018 at 18:19, on Zulip):

I was under the impression that you didn't have anything building and thus nothing running yet

Santiago Pastorino (Jul 12 2018 at 18:19, on Zulip):

You sound quite confident that there won't be any visible effect

hehe, I'm never that confident :P

pnkfelix (Jul 12 2018 at 18:19, on Zulip):

(and I've learned to not be quite so confident in such scenarios)

Santiago Pastorino (Jul 12 2018 at 18:19, on Zulip):

yeah

Santiago Pastorino (Jul 12 2018 at 18:19, on Zulip):

agree on that

pnkfelix (Jul 12 2018 at 18:19, on Zulip):

but okay, yes I agree that FIFO order means that you shoudn't observer any change

Santiago Pastorino (Jul 12 2018 at 18:19, on Zulip):

anyway, let's see

Santiago Pastorino (Jul 12 2018 at 18:20, on Zulip):

I will impact your changes for sure

Santiago Pastorino (Jul 12 2018 at 18:20, on Zulip):

the main thing is that I don't want to delay this stuff

pnkfelix (Jul 12 2018 at 18:20, on Zulip):

which of my changes do you mean?

pnkfelix (Jul 12 2018 at 18:20, on Zulip):

like changes to diagnostics?

Santiago Pastorino (Jul 12 2018 at 18:20, on Zulip):

the eventually_emit idea

pnkfelix (Jul 12 2018 at 18:20, on Zulip):

oh oh h

Santiago Pastorino (Jul 12 2018 at 18:20, on Zulip):

sooner or later I will do

Santiago Pastorino (Jul 12 2018 at 18:20, on Zulip):

let me see what actually happens in code

pnkfelix (Jul 12 2018 at 18:21, on Zulip):

one might even say that you will "eventually_incorporate" them...

Santiago Pastorino (Jul 12 2018 at 18:21, on Zulip):

hehe :D

pnkfelix (Jul 12 2018 at 18:21, on Zulip):

okay great. the only other thing I would potentially want to discuss is what to do after this task is done

pnkfelix (Jul 12 2018 at 18:21, on Zulip):

but I think the right strategy

pnkfelix (Jul 12 2018 at 18:21, on Zulip):

will probably be to finish this task

Santiago Pastorino (Jul 12 2018 at 18:21, on Zulip):

yes

pnkfelix (Jul 12 2018 at 18:21, on Zulip):

and get it landed in the github repo

Santiago Pastorino (Jul 12 2018 at 18:22, on Zulip):

agreed

pnkfelix (Jul 12 2018 at 18:22, on Zulip):

the remaining work I expect will be much less effort

Santiago Pastorino (Jul 12 2018 at 18:22, on Zulip):

I was thinking about jumping into perf stuff

Santiago Pastorino (Jul 12 2018 at 18:22, on Zulip):

but you have more idea than I about what's more important

pnkfelix (Jul 12 2018 at 18:22, on Zulip):

great then I don't think there's much else we need to talk about. You can focus on finishing this up and I can go back to looking at the unrelated NLL diagnostic thing I was playing with today.

pnkfelix (Jul 12 2018 at 18:22, on Zulip):

Oh, the perf stuff is very important

pnkfelix (Jul 12 2018 at 18:23, on Zulip):

I did say that this -Z borrowck=migrate is a blocker

pnkfelix (Jul 12 2018 at 18:23, on Zulip):

but I think once you get a PR up for this thing you have here

Santiago Pastorino (Jul 12 2018 at 18:23, on Zulip):

so after finishing this task will ping you to see what could be next

pnkfelix (Jul 12 2018 at 18:23, on Zulip):

then that will be really close to the finish line

pnkfelix (Jul 12 2018 at 18:23, on Zulip):

sounds great

Santiago Pastorino (Jul 12 2018 at 18:23, on Zulip):

:+1:

Santiago Pastorino (Jul 12 2018 at 18:23, on Zulip):

thanks for sharing thoughts and ideas

Santiago Pastorino (Jul 12 2018 at 18:23, on Zulip):

talk to you later

pnkfelix (Jul 12 2018 at 18:25, on Zulip):

bye!

Santiago Pastorino (Jul 17 2018 at 16:52, on Zulip):

@pnkfelix @nikomatsakis giving you both an update about this because I saw you mentioned this issue, I had zero time until now, I'm right now jumping again in this issue

Santiago Pastorino (Jul 17 2018 at 18:26, on Zulip):

@nikomatsakis I'm wondering how is the best way to pass the buffer in some places

Santiago Pastorino (Jul 17 2018 at 18:26, on Zulip):

for instance in qualify_consts

nikomatsakis (Jul 17 2018 at 18:27, on Zulip):

wait what

nikomatsakis (Jul 17 2018 at 18:27, on Zulip):

why are you modifying that?

Santiago Pastorino (Jul 17 2018 at 18:28, on Zulip):

there's a call to emit there

Santiago Pastorino (Jul 17 2018 at 18:28, on Zulip):

and it's in librustc_mir

Santiago Pastorino (Jul 17 2018 at 18:29, on Zulip):

hmm I see what you mean :)

Santiago Pastorino (Jul 17 2018 at 18:29, on Zulip):

what I need to modify is all the calls to emit inside src/librustc_mir/borrow_check/ right?

nikomatsakis (Jul 17 2018 at 18:30, on Zulip):

yep

Santiago Pastorino (Jul 17 2018 at 18:30, on Zulip):

that one I was talking about is not part of the borrow_check

Santiago Pastorino (Jul 17 2018 at 18:30, on Zulip):

ok

Santiago Pastorino (Jul 17 2018 at 18:30, on Zulip):

was naively searching for stuff without noticing that

nikomatsakis (Jul 17 2018 at 18:33, on Zulip):

good thing you asked :)

Santiago Pastorino (Jul 17 2018 at 18:39, on Zulip):

:)

Santiago Pastorino (Jul 17 2018 at 18:39, on Zulip):

just for the sake of checking

Santiago Pastorino (Jul 17 2018 at 18:39, on Zulip):

let me push the stuff I have

Santiago Pastorino (Jul 17 2018 at 18:40, on Zulip):

I think this is close and should be ready soon

Santiago Pastorino (Jul 17 2018 at 18:40, on Zulip):

thought it was more complicated than what it seems to be

nikomatsakis (Jul 17 2018 at 18:42, on Zulip):

cool

Santiago Pastorino (Jul 17 2018 at 18:42, on Zulip):

@nikomatsakis here is the relevant commit https://github.com/spastorino/rust/commit/fa80c651ac27151bced89490a109ff1a99b19ce2

Santiago Pastorino (Jul 17 2018 at 18:42, on Zulip):

here https://github.com/spastorino/rust/blob/fa80c651ac27151bced89490a109ff1a99b19ce2/src/librustc_mir/borrow_check/mod.rs#L294-L296 you see me iterating over the buffer and emitting

Santiago Pastorino (Jul 17 2018 at 18:43, on Zulip):

I was wondering about the rest of the emit calls in mod.rs

Santiago Pastorino (Jul 17 2018 at 18:43, on Zulip):

for instance ...

Santiago Pastorino (Jul 17 2018 at 18:45, on Zulip):

https://github.com/spastorino/rust/blob/fa80c651ac27151bced89490a109ff1a99b19ce2/src/librustc_mir/borrow_check/mod.rs#L328

Santiago Pastorino (Jul 17 2018 at 18:45, on Zulip):

that one doesn't seem to be important because it will happen after the buffer already emitted all what's in the buffer

Santiago Pastorino (Jul 17 2018 at 18:45, on Zulip):

I could make that push anyway and have a method which is called at the very end and just emit

Santiago Pastorino (Jul 17 2018 at 18:45, on Zulip):

maybe that's more clear

nikomatsakis (Jul 17 2018 at 18:46, on Zulip):

why not move the "emit site" down?

nikomatsakis (Jul 17 2018 at 18:46, on Zulip):

I feel like if we're gonna buffer, let's buffer everything...

Santiago Pastorino (Jul 17 2018 at 18:46, on Zulip):

yes

Santiago Pastorino (Jul 17 2018 at 18:46, on Zulip):

that's what I meant

Santiago Pastorino (Jul 17 2018 at 18:46, on Zulip):

ok

nikomatsakis (Jul 17 2018 at 18:46, on Zulip):

although one other thing is that I do find it annoying to get stuff like "unnecessary mut" when there are other, real errors

nikomatsakis (Jul 17 2018 at 18:46, on Zulip):

but that's a secondary issue

nikomatsakis (Jul 17 2018 at 18:46, on Zulip):

we can talk about sorting later

nikomatsakis (Jul 17 2018 at 18:47, on Zulip):

(often, the reason that the mut is not used is because of some other error, I find)

Santiago Pastorino (Jul 17 2018 at 18:47, on Zulip):

there's also https://github.com/spastorino/rust/blob/fa80c651ac27151bced89490a109ff1a99b19ce2/src/librustc_mir/borrow_check/mod.rs#L1366

Santiago Pastorino (Jul 17 2018 at 18:47, on Zulip):

and https://github.com/spastorino/rust/blob/fa80c651ac27151bced89490a109ff1a99b19ce2/src/librustc_mir/borrow_check/mod.rs#L1908

Santiago Pastorino (Jul 17 2018 at 18:47, on Zulip):

need to buffer those too

Santiago Pastorino (Jul 17 2018 at 18:47, on Zulip):

ok, will do with all the stuff

pnkfelix (Jul 17 2018 at 18:47, on Zulip):

so something I've been thinking about

pnkfelix (Jul 17 2018 at 18:47, on Zulip):

it seems like you are using grep to find the instances of emit()

Santiago Pastorino (Jul 17 2018 at 18:48, on Zulip):

yep :)

pnkfelix (Jul 17 2018 at 18:48, on Zulip):

There is another option.

pnkfelix (Jul 17 2018 at 18:48, on Zulip):

In particular, you could 1. Add a trait that defines emit, 2. Rename the existing emit (to something like emit_ or do_emit) and make it private, then implement the aforementioned trait, and 3. use the trait in each module that you want to alllow emit to be called.

pnkfelix (Jul 17 2018 at 18:49, on Zulip):

This is not something that you would have to make part of your PR. It could just be a separate commit that you use as a tool to assure yourself that you've identified all the sites where emit() is called.

Santiago Pastorino (Jul 17 2018 at 18:50, on Zulip):

:thumbs_up:

pnkfelix (Jul 17 2018 at 18:50, on Zulip):

Its up to you, of course. I just figure it seems like you're doing a lot of work to identify the emit's.

Santiago Pastorino (Jul 17 2018 at 18:50, on Zulip):

it's not actually that much

Santiago Pastorino (Jul 17 2018 at 18:51, on Zulip):

the thing is ... actually what you're saying is not going to work

Santiago Pastorino (Jul 17 2018 at 18:51, on Zulip):

well, unless I'm not seeing something

Santiago Pastorino (Jul 17 2018 at 18:51, on Zulip):

so ... I need to avoid calls to emit inside borrow_check mod

pnkfelix (Jul 17 2018 at 18:51, on Zulip):

... maybe I'm not explaining it correctly. Its what I think of as the standard pattern for tying methods to modules

Santiago Pastorino (Jul 17 2018 at 18:51, on Zulip):

but leave as is for the rest of the places where the calls happen

pnkfelix (Jul 17 2018 at 18:52, on Zulip):

right. So you would use this new trait in all the modules except rustc_mir::borrow_check (and everything underneath it)

Santiago Pastorino (Jul 17 2018 at 18:52, on Zulip):

ok

Santiago Pastorino (Jul 17 2018 at 18:52, on Zulip):

yes

Santiago Pastorino (Jul 17 2018 at 18:52, on Zulip):

something like that would work

Santiago Pastorino (Jul 17 2018 at 18:53, on Zulip):

yeah I see what you mean

Santiago Pastorino (Jul 17 2018 at 18:53, on Zulip):

I can also keep doing grep and change stuff and then move to a trait to see if something is failing in borrow_check

Santiago Pastorino (Jul 17 2018 at 18:53, on Zulip):

if nothing fails inside there I can just git checkout the trait and that stuff

Santiago Pastorino (Jul 17 2018 at 19:21, on Zulip):

@nikomatsakis I remember why I didn't push to errors_buffer on that loop

Santiago Pastorino (Jul 17 2018 at 19:21, on Zulip):
error[E0502]: cannot borrow `mbcx.errors_buffer` as mutable because `mbcx` is also borrowed as immutable
   --> librustc_mir/borrow_check/mod.rs:326:13
    |
297 |         .filter(|local| !mbcx.used_mut.contains(local))
    |                 -------  ----                         - immutable borrow ends here
    |                 |        |
    |                 |        previous borrow occurs due to use of `mbcx` in closure
    |                 immutable borrow occurs here
...
326 |             mbcx.errors_buffer.push(err);
    |             ^^^^^^^^^^^^^^^^^^ mutable borrow occurs here

error: aborting due to previous error
Santiago Pastorino (Jul 17 2018 at 19:21, on Zulip):

thinking how to avoid this ...

simulacrum (Jul 17 2018 at 19:22, on Zulip):

I think you might be able to do something like let used_mut = &mbcx.used_mut; above the filter and then use that inside

simulacrum (Jul 17 2018 at 19:23, on Zulip):

field-level disparity is tracked by AST borrowck I believe

nikomatsakis (Jul 17 2018 at 19:23, on Zulip):

yes, that is the topic of this blog post: http://smallcultfollowing.com/babysteps/blog/2018/04/24/rust-pattern-precise-closure-capture-clauses/

nikomatsakis (Jul 17 2018 at 19:23, on Zulip):

also there is a pending RFC to improve it that I have been neglecting :(

Santiago Pastorino (Jul 17 2018 at 19:24, on Zulip):

ahh, makes sense

Santiago Pastorino (Jul 17 2018 at 19:24, on Zulip):

but

Santiago Pastorino (Jul 17 2018 at 19:24, on Zulip):

field-level disparity is tracked by AST borrowck I believe

what does this ^^^ means?

Santiago Pastorino (Jul 17 2018 at 19:25, on Zulip):

what do you mean by field-level disparity

simulacrum (Jul 17 2018 at 19:26, on Zulip):

i.e. you can do

struct Bar { foo: u32, baz: u32 };
let mut bar = Bar { foo: 10, baz: 10 };
let field_foo = &bar.foo;
let field_baz = &mut bar.baz;
simulacrum (Jul 17 2018 at 19:27, on Zulip):

but maybe I'm remembering wrong

Jake Goulding (Jul 17 2018 at 19:27, on Zulip):

I think the term I normally hear is "disjoint".

nikomatsakis (Jul 17 2018 at 19:27, on Zulip):

yes of course

nikomatsakis (Jul 17 2018 at 19:27, on Zulip):

without that nothing at all would work :)

Jake Goulding (Jul 17 2018 at 19:27, on Zulip):

The borrow checker knows that fields of a structure are disjoint and do not overlap

nikomatsakis (Jul 17 2018 at 19:27, on Zulip):

but it must be done within one fn, and a closure is a separate fn. that's the problem.

Santiago Pastorino (Jul 17 2018 at 19:28, on Zulip):

ya makes, sense

Jake Goulding (Jul 17 2018 at 19:28, on Zulip):

yep, your closure has borrowed all of mbcx

Santiago Pastorino (Jul 17 2018 at 19:28, on Zulip):

it was that I had no idea about the term field level disparity :)

Jake Goulding (Jul 17 2018 at 19:29, on Zulip):

@nikomatsakis I'll admit that that particular style espoused in your blog was new to me. I haven't needed it too much since reading then, but I've been trying it when I do.

nikomatsakis (Jul 17 2018 at 19:29, on Zulip):

do you mean the

{
    let x = ...;
    move || x ...
}

?

nikomatsakis (Jul 17 2018 at 19:30, on Zulip):

I'm probably the only one who does it :) but I like packaging up the "closure borrows" with the closure itself...

simulacrum (Jul 17 2018 at 19:30, on Zulip):

right, I mean pulling the field access out of the closure

simulacrum (Jul 17 2018 at 19:30, on Zulip):

not pretty but it should work

Jake Goulding (Jul 17 2018 at 19:32, on Zulip):

@nikomatsakis indeed. I can see the benefit of it, but it's still a little odd to actually use "bare" { } like that

nikomatsakis (Jul 17 2018 at 19:33, on Zulip):

oh, I do that a lot too, if I want to emphasize "these are temporary values used to construct this other thing"

pnkfelix (Jul 18 2018 at 11:13, on Zulip):

@Santiago Pastorino hey, so as an experiment I grabbed your branch and tried doing the trait experiment I described, mostly to get an idea how painful it was

pnkfelix (Jul 18 2018 at 11:13, on Zulip):

and one thing I noticed while working on it: There are a few places where errors are being generated that are not going through the structured DiagnosticBuilder interface.

pnkfelix (Jul 18 2018 at 11:15, on Zulip):

Its probably easiest to just fix all those cases as part of this work, since otherwise you'll end up with deltas to the test results (namely in the ordering of diagnostic output) that you'd have to account for; such accounting is harder than just fixing the code to use DiagnosticBuilder

pnkfelix (Jul 18 2018 at 12:29, on Zulip):

Also, something I only just thought of last night that hasn't really been specified yet: What will the behavior of #![feature(nll)] be under -Z borrowck=migrate, aka under edition=2018 ?

pnkfelix (Jul 18 2018 at 12:30, on Zulip):

My thought is that if the user opts into #![feature(nll)] then we should just run with NLL and not do AST at all; they've opted into it and they want the errors.

pnkfelix (Jul 18 2018 at 12:32, on Zulip):

But then that raises a question: Should we add separate feature for users to opt into the -Z borrowck=migrate mode, so that crates can turn it on? Or is this something that we'll just toggle via the edition opt-in, and via the -Z flag in our unit tests?

pnkfelix (Jul 18 2018 at 12:32, on Zulip):

@nikomatsakis you may have thoughts on this ^

nikomatsakis (Jul 18 2018 at 12:32, on Zulip):

my main concern is the logic getting too complex

nikomatsakis (Jul 18 2018 at 12:32, on Zulip):

so..many...modes....

pnkfelix (Jul 18 2018 at 12:33, on Zulip):

I'm personally leaning towards "we'll just toggle via the edition opt-in, and via the -Z flag in our unit tests"

nikomatsakis (Jul 18 2018 at 13:59, on Zulip):

so what are the full set of modes? there are at least two things, right?

Right now #![feature(nll)] means mir + 2-phase

I guess you are saying we could add a new #![feature(nll_migrate)] that means migrate + 2-phase

And have --edition 2018 enable that feature

nikomatsakis (Jul 18 2018 at 13:59, on Zulip):

that seems ok

nikomatsakis (Jul 18 2018 at 13:59, on Zulip):

otoh I think maybe it's easier to just have #![feature(nll)] be the one user-exposed thing

nikomatsakis (Jul 18 2018 at 14:00, on Zulip):

well, we don't technically need a feature(nll_migrate)

nikomatsakis (Jul 18 2018 at 14:00, on Zulip):

basically the "default" borrowck mode would depend on the edition (2015 = AST, 2018 = Migrate)

nikomatsakis (Jul 18 2018 at 14:00, on Zulip):

right?

nikomatsakis (Jul 18 2018 at 14:00, on Zulip):

I don't really have a strong opinion is the truth

Jake Goulding (Jul 18 2018 at 14:04, on Zulip):

would polonius become a feature flag ever (after nll is enabled, presumably)

nikomatsakis (Jul 18 2018 at 14:08, on Zulip):

presumably at some point

nikomatsakis (Jul 18 2018 at 14:08, on Zulip):

but yes I imagine by then nll will just be the deafult

Jake Goulding (Jul 18 2018 at 14:10, on Zulip):

so it wont add to the matrix

pnkfelix (Jul 18 2018 at 14:15, on Zulip):

We don't need feature(nll_migrate) unless we think people will want to opt into that mode separately from opting into the edition. (Right?)

pnkfelix (Jul 18 2018 at 14:15, on Zulip):

That is, I'm attempting to say I agree with @nikomatsakis 's statement above, or rather, I am attempting to refine it.

nikomatsakis (Jul 18 2018 at 14:16, on Zulip):

yes, true

nikomatsakis (Jul 18 2018 at 14:16, on Zulip):

I initially had this idea that the edition should be the union of various feature gates, but that's not necessarily true

pnkfelix (Jul 18 2018 at 14:16, on Zulip):

And I think I agree with the statement. Someone who wants to migrate (but cannot or will not jump into edition=2018) and is on nightly should probably jump whole hog into feature(nll).

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

or rather, we should not be making further pains to support someone who is not willing to fix their code. (Right?)

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

Because I see the whole backwards-compatibility thing in this case as being about supporting people who are using either old/abandoned third-party libraries

pnkfelix (Jul 18 2018 at 14:20, on Zulip):

I initially had this idea that the edition should be the union of various feature gates, but that's not necessarily true

Okay I'll admit this is an alternative, somewhat more compelling argument for feature(nll_migrate).

pnkfelix (Jul 18 2018 at 14:21, on Zulip):

@nikomatsakis while I have your attention in this topic area: Did (or do) you have any preference about how the overall control-flow should look for how we fallback on the AST-borrowck?

pnkfelix (Jul 18 2018 at 14:23, on Zulip):

In particular, @Santiago Pastorino 's current branch is building the error buffers as a temporary Vec inside rustc_mir::borrow_check::do_mir_borrowck. If that vec doesn't leak outside of that method, then the fallback run of AST-borrowck has to be invoked from within do_mir_borrowck. (Which is fine with me if it is doable.)

nikomatsakis (Jul 18 2018 at 14:23, on Zulip):

not obvious to me why that is a problem

nikomatsakis (Jul 18 2018 at 14:23, on Zulip):

one other thing: one could imagine that in 2018 mode, we just always give hard NLL errors

nikomatsakis (Jul 18 2018 at 14:23, on Zulip):

but in 2015 mode, we do fallback

nikomatsakis (Jul 18 2018 at 14:23, on Zulip):

and there is only one feature gate (nll)

pnkfelix (Jul 18 2018 at 14:23, on Zulip):

The alternative control-flow I could envisage would involve do_mir_borrowck passing the errors_buffer back up to its caller and letting it process them. Or make them the result of the query, etc.

nikomatsakis (Jul 18 2018 at 14:24, on Zulip):

I think they cannot be the result of the query

nikomatsakis (Jul 18 2018 at 14:24, on Zulip):

because they contain references they ought not to

pnkfelix (Jul 18 2018 at 14:24, on Zulip):

Oh, because lifetimes

pnkfelix (Jul 18 2018 at 14:24, on Zulip):

right

pnkfelix (Jul 18 2018 at 14:25, on Zulip):

I've mused about that too. There's one obvious hack: Don't store DiagnoticBuilder<'a> in the vector; instead, convert each one to its string form eagerly.

pnkfelix (Jul 18 2018 at 14:26, on Zulip):

but that seems unnecessary if you are okay with us just invoking the AST-borrowck query from within MIR-borrowck.

Jake Goulding (Jul 18 2018 at 14:26, on Zulip):

Oh, because lifetimes

The story of programming in Rust.

pnkfelix (Jul 18 2018 at 14:26, on Zulip):

Oh, because lifetimes

The story of programming in Rust.

Its probably the story of programming in any non-GC'ed language. We just forced people to start talking about it. (What a "woke" language!)

pnkfelix (Jul 18 2018 at 15:13, on Zulip):

and one thing I noticed while working on it: There are a few places where errors are being generated that are not going through the structured DiagnosticBuilder interface.

I looked into this further; I think these are the main instances of this: nll/region_infer report_generic_bound_failure, nll/region_infer span_err, nll/type_check span_err.

pnkfelix (Jul 18 2018 at 15:14, on Zulip):

(However, I only just now noticed the instance of report_generic_bound_failure. There might be other instances of that.)

Santiago Pastorino (Jul 18 2018 at 16:04, on Zulip):

and one thing I noticed while working on it: There are a few places where errors are being generated that are not going through the structured DiagnosticBuilder interface.

@pnkfelix can you share the stuff you've already figured out? like the traits code if you think it's useful and the places where errors are being generated without using DiagnosticBuilder in case you have them identified

Santiago Pastorino (Jul 18 2018 at 16:04, on Zulip):

Its probably easiest to just fix all those cases as part of this work, since otherwise you'll end up with deltas to the test results (namely in the ordering of diagnostic output) that you'd have to account for; such accounting is harder than just fixing the code to use DiagnosticBuilder

:+1:

Santiago Pastorino (Jul 18 2018 at 16:11, on Zulip):

and one thing I noticed while working on it: There are a few places where errors are being generated that are not going through the structured DiagnosticBuilder interface.

I looked into this further; I think these are the main instances of this: nll/region_infer report_generic_bound_failure, nll/region_infer span_err, nll/type_check span_err.

just saw this message :) so part of my question is already answered :)

pnkfelix (Jul 18 2018 at 18:05, on Zulip):

I’ll try to share what i have from today after I manage to coax my son into sleeping... which may take two hours but I hope not

pnkfelix (Jul 18 2018 at 19:41, on Zulip):

can you share the stuff you've already figured out?

Here you go: https://github.com/pnkfelix/rust/commits/z-borrowck-migrate

pnkfelix (Jul 18 2018 at 19:42, on Zulip):

that builds and gets through the non-nll ui/ tests. But it gets hung up on three of the ui (nll) tests. I am hoping it is due to the aforementioned cases I identified (which are tagged with FIXME in an isolated commit on that branch.

pnkfelix (Jul 18 2018 at 19:43, on Zulip):

BTW lets make sure we keep that trait Emit code factored out; I don' think we'll want to land that.

Santiago Pastorino (Jul 18 2018 at 20:18, on Zulip):

:+1:

Santiago Pastorino (Jul 18 2018 at 20:18, on Zulip):

actually I have not been paying attention to this because I have been fighting with the borrow checker for a while :)

Santiago Pastorino (Jul 18 2018 at 20:19, on Zulip):

last thing I need to fix is ...

Santiago Pastorino (Jul 18 2018 at 20:19, on Zulip):
error[E0597]: `mdpe` does not live long enough
   --> librustc_mir/borrow_check/mod.rs:177:48
    |
177 |         MaybeInitializedPlaces::new(tcx, mir, &mdpe),
    |                                                ^^^^ borrowed value does not live long enough
...
345 | }
    | - `mdpe` dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

error[E0597]: `mdpe` does not live long enough
   --> librustc_mir/borrow_check/mod.rs:186:50
    |
186 |         MaybeUninitializedPlaces::new(tcx, mir, &mdpe),
    |                                                  ^^^^ borrowed value does not live long enough
...
345 | }
    | - `mdpe` dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

error[E0597]: `mdpe` does not live long enough
   --> librustc_mir/borrow_check/mod.rs:195:45
    |
195 |         MovingOutStatements::new(tcx, mir, &mdpe),
    |                                             ^^^^ borrowed value does not live long enough
...
345 | }
    | - `mdpe` dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

error[E0597]: `mdpe` does not live long enough
   --> librustc_mir/borrow_check/mod.rs:204:47
    |
204 |         EverInitializedPlaces::new(tcx, mir, &mdpe),
    |                                               ^^^^ borrowed value does not live long enough
...
345 | }
    | - `mdpe` dropped here while still borrowed
    |
    = note: values in a scope are dropped in the opposite order they are created

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0597`.
error: Could not compile `rustc_mir`.
Santiago Pastorino (Jul 18 2018 at 20:20, on Zulip):

I don't see why is mdpe still borrowed

pnkfelix (Jul 18 2018 at 20:25, on Zulip):

you can see how I fixed that on my branch

pnkfelix (Jul 18 2018 at 20:26, on Zulip):

namely in this commit

pnkfelix (Jul 18 2018 at 20:26, on Zulip):

the mdpe needs to outlive the errors_buffer

Santiago Pastorino (Jul 18 2018 at 20:27, on Zulip):

I've just fixed it

Santiago Pastorino (Jul 18 2018 at 20:27, on Zulip):

anyway gonna check your commit

pnkfelix (Jul 18 2018 at 20:28, on Zulip):

The way the code was before, with the let mdpe at the point where mdpe is initialized, makes it get dropped too soon. Which ... I don't think actually matters ... but I think it was causing the region inference to get confused.

Santiago Pastorino (Jul 18 2018 at 20:28, on Zulip):

your fix is simpler and better :)

pnkfelix (Jul 18 2018 at 20:28, on Zulip):

I had to take a walk in between when I saw that error and when I actually fixed it. :)

pnkfelix (Jul 18 2018 at 20:29, on Zulip):

there was some useless head-banging against the keyboard when I first hit it.

Santiago Pastorino (Jul 18 2018 at 20:29, on Zulip):

:)

Santiago Pastorino (Jul 18 2018 at 20:29, on Zulip):

the code I have is now compiling

Santiago Pastorino (Jul 18 2018 at 20:30, on Zulip):

after all the borrowck headaches

pnkfelix (Jul 18 2018 at 20:30, on Zulip):

now my question is

pnkfelix (Jul 18 2018 at 20:30, on Zulip):

how many of these headaches will NLL fix...

Santiago Pastorino (Jul 18 2018 at 20:30, on Zulip):

:P

Santiago Pastorino (Jul 18 2018 at 20:30, on Zulip):

I have no idea

Santiago Pastorino (Jul 18 2018 at 20:30, on Zulip):

hehehe

Santiago Pastorino (Jul 18 2018 at 20:31, on Zulip):

I don't think the issues I was hitting are related

Santiago Pastorino (Jul 18 2018 at 20:31, on Zulip):

just needed to properly adjust a lot of structs lifetimes

Santiago Pastorino (Jul 18 2018 at 20:31, on Zulip):

and properly mark outlives relations

Santiago Pastorino (Jul 18 2018 at 20:31, on Zulip):

I think is the first time I do this in Rust in a such intensive way

pnkfelix (Jul 18 2018 at 20:34, on Zulip):

Sometimes that kind of change ends up not being necessary

pnkfelix (Jul 18 2018 at 20:34, on Zulip):

if the types end up all being covariant

pnkfelix (Jul 18 2018 at 20:35, on Zulip):

but as soon as something has a &mut or other kinds of mutability like Cell, you end up having a lot of pain with explicit lifetime maintenance

pnkfelix (Jul 18 2018 at 20:35, on Zulip):

(Thus, passing down a &mut errors_buffer like this, and trying to add it to an existing struct, can lead to a lot of headaches.)

pnkfelix (Jul 18 2018 at 20:36, on Zulip):

its an area where the language will need to be improved, long term.

Santiago Pastorino (Jul 18 2018 at 20:38, on Zulip):

I see what you described happening in practice

Santiago Pastorino (Jul 18 2018 at 20:39, on Zulip):

I don't know why is all this related with the thing being &mut

Santiago Pastorino (Jul 18 2018 at 20:39, on Zulip):

Niko told me the same thing earlier

pnkfelix (Jul 18 2018 at 20:39, on Zulip):

I have a presentation you can watch if you want to learn

Santiago Pastorino (Jul 18 2018 at 20:39, on Zulip):

:+1:

Santiago Pastorino (Jul 18 2018 at 20:39, on Zulip):

I told Niko, would be nice to have a rustc conference :P

pnkfelix (Jul 18 2018 at 20:39, on Zulip):

this one: https://www.youtube.com/watch?v=fI4RG_uq-WU

pnkfelix (Jul 18 2018 at 20:40, on Zulip):

You may need to connect a few more dots after watching that to connect it to what we're discussing, I cannot remember now.

Santiago Pastorino (Jul 18 2018 at 20:40, on Zulip):

I feel that there's a lot of knowledge you have and if properly shared I and pretty sure other contributors will be more productive :)

Jake Goulding (Jul 18 2018 at 21:07, on Zulip):

Variance is both an intuitive and unintuitive concept. I refer back to https://doc.rust-lang.org/nomicon/subtyping.html when thinking about it.

pnkfelix (Jul 18 2018 at 21:23, on Zulip):

@Santiago Pastorino FYI I just pushed some more commits to my repo's z-borrowck-migrate branch

pnkfelix (Jul 18 2018 at 21:24, on Zulip):

@Santiago Pastorino i haven't identified all the problematic points yet;I believe I've fixed the three that I identified earlier, but the test suite still has two test cases where the output ordering differs.

pnkfelix (Jul 18 2018 at 21:30, on Zulip):

(note that I would not necessarily try to land this code I've written as is. I'm just trying to see how far I can get.)

davidtwco (Jul 18 2018 at 21:39, on Zulip):

(@pnkfelix that talk was really great, thanks for linking it!)

pnkfelix (Jul 18 2018 at 21:39, on Zulip):

every time I skim over the slides I marvel at how long I spent preparing it

Santiago Pastorino (Jul 18 2018 at 21:50, on Zulip):

@Santiago Pastorino i haven't identified all the problematic points yet;I believe I've fixed the three that I identified earlier, but the test suite still has two test cases where the output ordering differs.

@pnkfelix cool, I've this thing compiling today is a national holiday here, so will probably stop now

Santiago Pastorino (Jul 18 2018 at 21:50, on Zulip):

tomorrow I guess I should finish this

Santiago Pastorino (Jul 18 2018 at 21:50, on Zulip):

haven't checked your branch yet

pnkfelix (Jul 18 2018 at 21:50, on Zulip):

i too must stop because i fear my toddler may awake at any point during the night

Santiago Pastorino (Jul 18 2018 at 21:50, on Zulip):

(note that I would not necessarily try to land this code I've written as is. I'm just trying to see how far I can get.)

and no worries about this :)

Santiago Pastorino (Jul 18 2018 at 21:51, on Zulip):

we should make this happen together :)

pnkfelix (Jul 18 2018 at 21:51, on Zulip):

@Santiago Pastorino have you pushed your branch? Please do so just so I can look at it

pnkfelix (Jul 18 2018 at 21:51, on Zulip):

And yes, I think it would be good for us to try to collaborate to get this to happen. I was musing earlier that it might be good if we tried to literally hand work off to each other

Santiago Pastorino (Jul 18 2018 at 21:51, on Zulip):

@pnkfelix yes, I've pushed

Santiago Pastorino (Jul 18 2018 at 21:51, on Zulip):

it's here https://github.com/spastorino/rust/compare/z-borrowck-migrate

pnkfelix (Jul 18 2018 at 21:52, on Zulip):

Great. I may look at it tomorrow.

Santiago Pastorino (Jul 18 2018 at 21:52, on Zulip):

haven't checked properly yet

Santiago Pastorino (Jul 18 2018 at 21:52, on Zulip):

may be some weird lifetimes dance that I still need to remove

Santiago Pastorino (Jul 18 2018 at 21:53, on Zulip):

@pnkfelix would be good to make a list of things that we want to accomplish

Santiago Pastorino (Jul 18 2018 at 21:53, on Zulip):

we talked about a lot of things and may be a good idea to build a checklist to not forget about stuff

pnkfelix (Jul 18 2018 at 21:54, on Zulip):

well if we can get the buffering to the point where the errors it emits are in the same order, then that would be a fine point to make a PR

pnkfelix (Jul 18 2018 at 21:55, on Zulip):

But yeah, I'll put a checklist up on issue #46908 itself, so that we know the task list for the issue itself.

Santiago Pastorino (Jul 18 2018 at 22:00, on Zulip):

exactly

Santiago Pastorino (Jul 18 2018 at 22:00, on Zulip):

yeah

Santiago Pastorino (Jul 18 2018 at 22:00, on Zulip):

makes sense

Santiago Pastorino (Jul 18 2018 at 22:45, on Zulip):

every time I skim over the slides I marvel at how long I spent preparing it

@pnkfelix out of curiosity, how long is that? :)

pnkfelix (Jul 19 2018 at 02:17, on Zulip):

Well one can identify some rough bounds on an estimate by looking at the commit history here https://github.com/pnkfelix/presentations/commits/rustfest-berlin-2016

Santiago Pastorino (Jul 19 2018 at 04:54, on Zulip):

3 years :)

pnkfelix (Jul 19 2018 at 10:44, on Zulip):

well that a pretty conservative bound

pnkfelix (Jul 19 2018 at 10:49, on Zulip):

(a more thorough answer: I wrote the talk proposal in late June, and made final mods to the slides themselves in early september. The commit history is misleading, in that it shows that a lot of the text was inserted over the course of a few days, but I had worked out the ideas on paper & index cards long before then.)

pnkfelix (Jul 19 2018 at 11:24, on Zulip):

@Santiago Pastorino okay I think I identified the other emit calls that I had missed. Or at least, I am now able to get through the ui/ tests.

Santiago Pastorino (Jul 19 2018 at 13:01, on Zulip):

@pnkfelix have you based your work on top of my updated branch or something?

pnkfelix (Jul 19 2018 at 13:02, on Zulip):

what do you mean?

Santiago Pastorino (Jul 19 2018 at 13:02, on Zulip):

maybe I can just add you as a collaborator to my repo and you can push your stuff on top of mine

Santiago Pastorino (Jul 19 2018 at 13:02, on Zulip):

so we can open a PR

Santiago Pastorino (Jul 19 2018 at 13:02, on Zulip):

or you can point me to what you have done and I guess I can grab your commits and open a PR

pnkfelix (Jul 19 2018 at 13:03, on Zulip):

I pushed the working system to the same branch on my repo (same name as the one on your rust repo)

pnkfelix (Jul 19 2018 at 13:03, on Zulip):

and now I've been rebasing and cleaning it up a little

pnkfelix (Jul 19 2018 at 13:03, on Zulip):

If you like I can handle opening the PR at this point

Santiago Pastorino (Jul 19 2018 at 13:03, on Zulip):

:+1:

pnkfelix (Jul 19 2018 at 13:03, on Zulip):

its up to you; i'll leave the commits that you did attributed to you

Santiago Pastorino (Jul 19 2018 at 13:04, on Zulip):

no worries about that, you can merge stuff if you prefer

Santiago Pastorino (Jul 19 2018 at 13:04, on Zulip):

I don't mind

pnkfelix (Jul 19 2018 at 13:04, on Zulip):

at least, I got the impression that you were interested in going back to working on performance investigation?

Santiago Pastorino (Jul 19 2018 at 13:04, on Zulip):

I'm fine to do whatever is needed

Santiago Pastorino (Jul 19 2018 at 13:05, on Zulip):

but I could continue with the -Z migrate thing I guess

Santiago Pastorino (Jul 19 2018 at 13:05, on Zulip):

whatever you prefer

Santiago Pastorino (Jul 19 2018 at 13:05, on Zulip):

check my branch again if you didn't because yesterday late I made some changes

pnkfelix (Jul 19 2018 at 13:05, on Zulip):

okay I'll look at that

Santiago Pastorino (Jul 19 2018 at 13:06, on Zulip):

my branch was compiling, it was probably lacking those emit order stuff you were investigating :)

pnkfelix (Jul 19 2018 at 13:06, on Zulip):

my branch passes the {ui,compile-fail,run-pass,mir-opt} test suites

pnkfelix (Jul 19 2018 at 13:06, on Zulip):

which is a good sign

Santiago Pastorino (Jul 19 2018 at 13:08, on Zulip):

:+1:

Santiago Pastorino (Jul 19 2018 at 13:58, on Zulip):

@pnkfelix let me know when you finish or if I can help in any way so I can continue with the rest

Santiago Pastorino (Jul 19 2018 at 13:58, on Zulip):

or well ... I can assume it's done and continue in a new branch

Santiago Pastorino (Jul 19 2018 at 13:59, on Zulip):

I have plenty of time today

Santiago Pastorino (Jul 19 2018 at 16:21, on Zulip):

@pnkfelix ?

Santiago Pastorino (Jul 19 2018 at 16:22, on Zulip):

I can continue with everything from the point you have it now I guess

Santiago Pastorino (Jul 19 2018 at 16:22, on Zulip):

I guess I can grab your branch

Santiago Pastorino (Jul 19 2018 at 16:22, on Zulip):

I have plenty of time :)

pnkfelix (Jul 19 2018 at 16:23, on Zulip):

I’m busy with child card right now, yeah

Santiago Pastorino (Jul 19 2018 at 16:23, on Zulip):

no worries

Santiago Pastorino (Jul 19 2018 at 16:23, on Zulip):

so I can continue

Santiago Pastorino (Jul 19 2018 at 16:23, on Zulip):

should I grab your branch?

Santiago Pastorino (Jul 19 2018 at 16:23, on Zulip):

is it up to date?

Santiago Pastorino (Jul 19 2018 at 16:23, on Zulip):

gonna cherry pick the commits that make sense and continue on my branch I guess

Santiago Pastorino (Jul 20 2018 at 04:05, on Zulip):

@pnkfelix all my commits + yours are here https://github.com/rust-lang/rust/compare/master...spastorino:z-borrowck-migrate

Santiago Pastorino (Jul 20 2018 at 04:07, on Zulip):

it's giving some lifetimes errors after having it rebased with master

Santiago Pastorino (Jul 20 2018 at 04:07, on Zulip):

tomorrow will look into it

Santiago Pastorino (Jul 20 2018 at 04:09, on Zulip):

there were a lot of things in your repo that I already have done in mine also

Santiago Pastorino (Jul 20 2018 at 04:09, on Zulip):

so the merge ended not being so straightforward

pnkfelix (Jul 20 2018 at 11:17, on Zulip):

Yeah I should have warned you about that, I had seen lifetime errors post rebase and while I have a fixed, I had not pushed my own rebased version

Santiago Pastorino (Jul 20 2018 at 12:47, on Zulip):

@pnkfelix can you point me to the code that fixes that?

Santiago Pastorino (Jul 20 2018 at 12:48, on Zulip):

I'm going right now to start fighting with this issues

pnkfelix (Jul 20 2018 at 12:55, on Zulip):

So

pnkfelix (Jul 20 2018 at 12:56, on Zulip):

I just finished rebasing my work atop your branch

pnkfelix (Jul 20 2018 at 12:56, on Zulip):

and after looking over your changes, I'm not sure we're winning trying to contintually rebase this way

pnkfelix (Jul 20 2018 at 12:56, on Zulip):

in terms of the amount of time that it seems like we might be hitting conflicts in our changes?

Santiago Pastorino (Jul 20 2018 at 12:58, on Zulip):

yeah, maybe, unsure

Santiago Pastorino (Jul 20 2018 at 12:58, on Zulip):

what you suggest?

pnkfelix (Jul 20 2018 at 12:58, on Zulip):

Not sure yet

Santiago Pastorino (Jul 20 2018 at 12:58, on Zulip):

I'm curious about the lifetimes issue fix :)

pnkfelix (Jul 20 2018 at 12:58, on Zulip):

sure, i believe it is this bit: https://github.com/pnkfelix/rust/commit/389c4cf3e527df07212646844efcf95c5ddcd382#diff-681da89983b0186166523349cf3c2a8cR155

Santiago Pastorino (Jul 20 2018 at 12:58, on Zulip):

but this first part should be ready, right?

pnkfelix (Jul 20 2018 at 12:59, on Zulip):

that is, I added a new 'errs lifetime

Santiago Pastorino (Jul 20 2018 at 12:59, on Zulip):

I was thinking exactly the same thing

pnkfelix (Jul 20 2018 at 12:59, on Zulip):

where 'a can be shorter than 'errs but 'errs can be shorter than 'gcx

Santiago Pastorino (Jul 20 2018 at 12:59, on Zulip):

makes sense

pnkfelix (Jul 20 2018 at 13:00, on Zulip):

This first part is indeed ready, or close to it.

pnkfelix (Jul 20 2018 at 13:00, on Zulip):

I did want to ask you

pnkfelix (Jul 20 2018 at 13:00, on Zulip):

why did you rename 'a to 'cx ?

Santiago Pastorino (Jul 20 2018 at 13:00, on Zulip):

do you want me to continue?

pnkfelix (Jul 20 2018 at 13:00, on Zulip):

At least, I personally don't find 'cx to be more informative. they're both equally generic.

Santiago Pastorino (Jul 20 2018 at 13:00, on Zulip):

yeah

Santiago Pastorino (Jul 20 2018 at 13:01, on Zulip):

I thought about that

Santiago Pastorino (Jul 20 2018 at 13:01, on Zulip):

can revert that stuff

pnkfelix (Jul 20 2018 at 13:01, on Zulip):

well

pnkfelix (Jul 20 2018 at 13:01, on Zulip):

I'll do that

pnkfelix (Jul 20 2018 at 13:01, on Zulip):

I think at this point

pnkfelix (Jul 20 2018 at 13:01, on Zulip):

the most efficient thing

pnkfelix (Jul 20 2018 at 13:01, on Zulip):

well it depends

pnkfelix (Jul 20 2018 at 13:02, on Zulip):

Were you planning to spend time on the other followup tasks for this issue over the weekend?

pnkfelix (Jul 20 2018 at 13:03, on Zulip):

I usually don't have free time to code (or assist on the chat room/email) during the weekend

Santiago Pastorino (Jul 20 2018 at 13:05, on Zulip):

yeah I plan to work on this

pnkfelix (Jul 20 2018 at 13:05, on Zulip):

in that case

Santiago Pastorino (Jul 20 2018 at 13:05, on Zulip):

and I have time today

pnkfelix (Jul 20 2018 at 13:05, on Zulip):

since I won't have time, and I'll be finishing up my day soon

Santiago Pastorino (Jul 20 2018 at 13:05, on Zulip):

but I can jump to other task if you prefer

pnkfelix (Jul 20 2018 at 13:06, on Zulip):

no I think it would be good for you to keep going

Santiago Pastorino (Jul 20 2018 at 13:06, on Zulip):

cool

pnkfelix (Jul 20 2018 at 13:06, on Zulip):

since you have time and were already planning on going after it

pnkfelix (Jul 20 2018 at 13:06, on Zulip):

but I'd like to make sure you start off ahead of the curve

pnkfelix (Jul 20 2018 at 13:06, on Zulip):

as in: My branch, I believe, has everything in yours, and has been rebased atop master, and passes the test suite

pnkfelix (Jul 20 2018 at 13:07, on Zulip):

so I should hand it off to you

pnkfelix (Jul 20 2018 at 13:07, on Zulip):

and let you run with it

pnkfelix (Jul 20 2018 at 13:07, on Zulip):

let me just

pnkfelix (Jul 20 2018 at 13:07, on Zulip):

finish off the last task of getting rid of that 'a to 'cx rename commit

Santiago Pastorino (Jul 20 2018 at 13:07, on Zulip):

:+1:

pnkfelix (Jul 20 2018 at 13:08, on Zulip):

and then I think I'll put up a PR with that

pnkfelix (Jul 20 2018 at 13:08, on Zulip):

and then you can work on followup tasks

pnkfelix (Jul 20 2018 at 13:08, on Zulip):

based on that PR

pnkfelix (Jul 20 2018 at 13:08, on Zulip):

does that make sense?

pnkfelix (Jul 20 2018 at 13:08, on Zulip):

as in, we can get niko to review the PR of the error buffering work

pnkfelix (Jul 20 2018 at 13:08, on Zulip):

while you work on the other tasks for this issue

Santiago Pastorino (Jul 20 2018 at 13:08, on Zulip):

one of the reasoning about 'a -> 'cx is that it is a bit confusing that in some places is called 'a and in other 'cx

Santiago Pastorino (Jul 20 2018 at 13:08, on Zulip):

but unsure

pnkfelix (Jul 20 2018 at 13:08, on Zulip):

that I can understand

pnkfelix (Jul 20 2018 at 13:09, on Zulip):

but right now it just seemed to me to inject noise into the PR

Santiago Pastorino (Jul 20 2018 at 13:09, on Zulip):

yeah avoid that commit from PR

pnkfelix (Jul 20 2018 at 13:09, on Zulip):

okay

Santiago Pastorino (Jul 20 2018 at 13:09, on Zulip):

anyway if we were making a change like that would need to be consistent over all the uses, not just a few :)

Santiago Pastorino (Jul 20 2018 at 13:10, on Zulip):

does that make sense?

:+1:, makes sense

Santiago Pastorino (Jul 20 2018 at 13:10, on Zulip):

I was going to say that

Santiago Pastorino (Jul 20 2018 at 13:10, on Zulip):

open a PR

Santiago Pastorino (Jul 20 2018 at 13:10, on Zulip):

:)

pnkfelix (Jul 20 2018 at 13:29, on Zulip):

was that a command or a clarification?

pnkfelix (Jul 20 2018 at 13:29, on Zulip):

"sudo open a PR"

pnkfelix (Jul 20 2018 at 13:34, on Zulip):

In any case, here is the PR: https://github.com/rust-lang/rust/pull/52566

pnkfelix (Jul 20 2018 at 13:35, on Zulip):

@Santiago Pastorino do you actually know which of the -Z borrowck=migrate tasks you are likely to take on in the immediate future?

Santiago Pastorino (Jul 20 2018 at 13:35, on Zulip):

hahaha, I clarification :P

Santiago Pastorino (Jul 20 2018 at 13:36, on Zulip):

I was going to say, if it's ready and all that should I grab your branch and open a PR or are you doing that?

Santiago Pastorino (Jul 20 2018 at 13:37, on Zulip):

@Santiago Pastorino do you actually know which of the -Z borrowck=migrate tasks you are likely to take on in the immediate future?

no, whatever shows up first I guess

pnkfelix (Jul 20 2018 at 13:37, on Zulip):

my thinking is that I'll keep my on-going work on the z-borrowck-migrate branch. The PR I put up is based on a newly named branch that is just a point on that branch.

Santiago Pastorino (Jul 20 2018 at 13:37, on Zulip):

are you going to do something?

Santiago Pastorino (Jul 20 2018 at 13:37, on Zulip):

in that case, we can split some parts

pnkfelix (Jul 20 2018 at 13:37, on Zulip):

The reason I asked which thing you might work on first is that I see two interesting tasks left

pnkfelix (Jul 20 2018 at 13:38, on Zulip):

1. Add a mode to AST-borrowck so that you can invoke it without it emitting any errors itself (and instead it just returns "saw an error" or "saw no errors")

pnkfelix (Jul 20 2018 at 13:38, on Zulip):

2. Add the code to MIR-borrowck that calls out to AST-borrowck if MIR -borrowck saw any errors.

pnkfelix (Jul 20 2018 at 13:38, on Zulip):

One could start on 2. without having yet done 1.

pnkfelix (Jul 20 2018 at 13:39, on Zulip):

So yeah, I figured its potentially splittable.

Santiago Pastorino (Jul 20 2018 at 13:39, on Zulip):

cool

pnkfelix (Jul 20 2018 at 13:39, on Zulip):

I'm going to be working for about another two hours or so

pnkfelix (Jul 20 2018 at 13:39, on Zulip):

Given the description on #46908, can you think of any other work items?

Santiago Pastorino (Jul 20 2018 at 13:39, on Zulip):

the other thing we can do ... is ... are you already working on 1 or what are you currently doing?

pnkfelix (Jul 20 2018 at 13:40, on Zulip):

(there's also the bit about making sure the diagnostics and docs properly indicate the severity of the supposed "warnings" you'll get)

Santiago Pastorino (Jul 20 2018 at 13:40, on Zulip):

because I can just let you do what you're doing and then you can hand over where you left the stuff to me

pnkfelix (Jul 20 2018 at 13:40, on Zulip):

I haven't actually started on 1. or 2. yet

Santiago Pastorino (Jul 20 2018 at 13:41, on Zulip):

maybe you can start with 1

Santiago Pastorino (Jul 20 2018 at 13:41, on Zulip):

and if I can do something before you leave I will start 2

pnkfelix (Jul 20 2018 at 13:41, on Zulip):

Great, sounds good.

Santiago Pastorino (Jul 20 2018 at 13:41, on Zulip):

and before leaving ping me so I know where you left and I merge everything over and try to finish my stuff and yours if there's something missing

pnkfelix (Jul 20 2018 at 13:42, on Zulip):

Okay. If I forget to ping you, the on-going work, if any, will be pushed to my z-borrowck-migrate branch. (That's why I made a fresh name for the PR)

Santiago Pastorino (Jul 20 2018 at 13:45, on Zulip):

:+1:

pnkfelix (Jul 20 2018 at 15:35, on Zulip):

So I might have managed to do both 1. and 2.

pnkfelix (Jul 20 2018 at 15:36, on Zulip):

I'm not 100% sure yet because my first attempt to actually test -Z borrowck=migrate, I discovered I missed one spot where we validate the -Z input text and thus I couldn't actually test the flag.

pnkfelix (Jul 20 2018 at 15:36, on Zulip):

so I'm rebuilding the compiler now with that fixed.

pnkfelix (Jul 20 2018 at 15:36, on Zulip):

but it does seem like my current changes don't break the existing code at least.

Santiago Pastorino (Jul 20 2018 at 15:37, on Zulip):

cool

pnkfelix (Jul 20 2018 at 15:37, on Zulip):

But I'm also going to have to leave in like four minutes so it may not build before I get a chance to actually do any real testing of -Z borrowck=migrate itself.

Santiago Pastorino (Jul 20 2018 at 15:37, on Zulip):

let me know what I need to do then or if it's just done :)

pnkfelix (Jul 20 2018 at 15:37, on Zulip):

I've pushed the work to my z-borrowck-mode branch

Santiago Pastorino (Jul 20 2018 at 15:37, on Zulip):

ok

pnkfelix (Jul 20 2018 at 15:37, on Zulip):

it still needs unit tests to be written

Santiago Pastorino (Jul 20 2018 at 15:37, on Zulip):

what should I do?

Santiago Pastorino (Jul 20 2018 at 15:37, on Zulip):

I could also leave it up to you

pnkfelix (Jul 20 2018 at 15:38, on Zulip):

one way to do a unit test would be to identify a piece of code that passes AST borrowck but is rejected by MIR-borrowck

Santiago Pastorino (Jul 20 2018 at 15:38, on Zulip):

for when you have a chance and look for other tasks

pnkfelix (Jul 20 2018 at 15:38, on Zulip):

and then make sure that such code emits a warning under -Z borrowck=migrate

pnkfelix (Jul 20 2018 at 15:38, on Zulip):

the most obvious example of such a piece of code are the examples from #27282

pnkfelix (Jul 20 2018 at 15:39, on Zulip):

And of course we also would want to make sure that every other piece of code behaves the way we expect, in terms of acceptance or rejection by compiler, under -Z borrowck=migrate.

pnkfelix (Jul 20 2018 at 15:40, on Zulip):

I actually don't know the easiest way to accomplish that. Add a new compare-more to compiletest?

pnkfelix (Jul 20 2018 at 15:40, on Zulip):

or repurpose the existing compare-mode ?

pnkfelix (Jul 20 2018 at 15:40, on Zulip):

or just use the revision infrastructure?

pnkfelix (Jul 20 2018 at 15:41, on Zulip):

Anyway, I have to leave now. I leave it up to you whether you want to try to follow up on this

pnkfelix (Jul 20 2018 at 15:41, on Zulip):

I.e. if you feel like doing something totally different with your time, that's fine

pnkfelix (Jul 20 2018 at 15:41, on Zulip):

but if you're cool with checking out the branch and trying to find flaws in it and maybe writing a couple tests, that would be helpful

lqd (Jul 20 2018 at 15:58, on Zulip):

(there's also a bunch of minimized repros that passed AST borrowck and MIR borrowck rejected, in the crater run if you need some)

pnkfelix (Jul 20 2018 at 16:07, on Zulip):

@lqd could you actually put url linking to those in the issue??

lqd (Jul 20 2018 at 16:08, on Zulip):

sure

lqd (Jul 20 2018 at 16:16, on Zulip):

(though tbf some of those are MIR borrowck bugs, but I guess those can be useful as well)

lqd (Jul 20 2018 at 16:25, on Zulip):

https://github.com/rust-lang/rust/issues/46908#issuecomment-406652255

Santiago Pastorino (Jul 20 2018 at 21:09, on Zulip):

@pnkfelix shouldn't we make this https://github.com/rust-lang/rust/blob/64aa4c5369611ba9867b6956fe9a57e0fc11ca49/src/librustc/infer/error_reporting/mod.rs#L306 depend on the mode or something at some point?

pnkfelix (Jul 20 2018 at 22:22, on Zulip):

Feel free to change it; I’m not familiar with the logic in that part of the code, and like I said, I didn’t get a chance to test my -Z borrowck=migrate before I left

pnkfelix (Jul 20 2018 at 22:22, on Zulip):

(Have you built it yourself yet? Does it seem like it’s plausibly working?)

Santiago Pastorino (Jul 20 2018 at 22:24, on Zulip):

I didn't do that much given that most of the work was more or less done

Santiago Pastorino (Jul 20 2018 at 22:25, on Zulip):

have built the thing and left tests running

Santiago Pastorino (Jul 20 2018 at 22:25, on Zulip):

will try to check a bit today or during the weekend

Santiago Pastorino (Jul 20 2018 at 22:43, on Zulip):

also, shouldn't we skip this https://github.com/rust-lang/rust/blob/64aa4c5369611ba9867b6956fe9a57e0fc11ca49/src/librustc_mir/transform/mod.rs#L229-L231 if mir_borrowck didn't fail?

pnkfelix (Jul 21 2018 at 05:38, on Zulip):

It probably doesn’t matter, since the result of the query is cached, right?

nikomatsakis (Jul 21 2018 at 11:57, on Zulip):

so @pnkfelix / @Santiago Pastorino are either of you around? I'm wondering about a possible way to simplify #52566

Santiago Pastorino (Jul 21 2018 at 12:27, on Zulip):

I'm here

nikomatsakis (Jul 21 2018 at 12:30, on Zulip):

I left a comment on the PR

nikomatsakis (Jul 21 2018 at 12:30, on Zulip):

https://github.com/rust-lang/rust/pull/52566#pullrequestreview-139256779

nikomatsakis (Jul 21 2018 at 12:31, on Zulip):

TL;DR I think we could simplify some of the lifetime hell you went through :)

Santiago Pastorino (Jul 21 2018 at 12:32, on Zulip):

:)

Santiago Pastorino (Jul 21 2018 at 12:32, on Zulip):

ok

Santiago Pastorino (Jul 21 2018 at 12:33, on Zulip):

will see if I can invest some time to this during the weekend

pnkfelix (Jul 23 2018 at 10:24, on Zulip):

i'm looking at it now as well, since I didn't see any notes from @Santiago Pastorino . The main interesting complication so far is that DiagnosticBuilderimplements Drop, so you have to use ptr::read to pull out its self.diagnostic (and then mem::forget(self) afterward).

nikomatsakis (Jul 23 2018 at 13:51, on Zulip):

@pnkfelix I would probably just clone the diagnostic

pnkfelix (Jul 23 2018 at 13:51, on Zulip):

well that's no fun

nikomatsakis (Jul 23 2018 at 13:51, on Zulip):

and/or make it an Option<Diagnostic> (I think there is already some kind of flag, right? That's how the emit prevents it from erroring out?)

nikomatsakis (Jul 23 2018 at 13:51, on Zulip):

not sure how that works actually

pnkfelix (Jul 23 2018 at 13:51, on Zulip):

there's a Level::Cancelled

pnkfelix (Jul 23 2018 at 13:52, on Zulip):

but wait is that how that works

nikomatsakis (Jul 23 2018 at 13:52, on Zulip):

maybe emit uses mem::forget to cancel the dtor

pnkfelix (Jul 23 2018 at 13:52, on Zulip):

no it just calls self.cancel().

pnkfelix (Jul 23 2018 at 13:52, on Zulip):

(and that then sets the level to Level::Cancelled)

nikomatsakis (Jul 23 2018 at 13:53, on Zulip):

ok. well, I still think clone is perfectly fine here :)

nikomatsakis (Jul 23 2018 at 13:53, on Zulip):

this is not a hot path

pnkfelix (Jul 23 2018 at 13:53, on Zulip):

true

Santiago Pastorino (Jul 23 2018 at 18:00, on Zulip):

@pnkfelix @nikomatsakis sorry but I couldn't do anything during the weekend

Santiago Pastorino (Jul 23 2018 at 18:00, on Zulip):

I'm back and can start tackling something

Santiago Pastorino (Jul 23 2018 at 18:00, on Zulip):

unsure what's the current status of this

Santiago Pastorino (Jul 23 2018 at 18:00, on Zulip):

let me know and I can help or finish whatever is needed

Santiago Pastorino (Jul 23 2018 at 18:01, on Zulip):

no idea how much progress did Felix here

nikomatsakis (Jul 23 2018 at 18:03, on Zulip):

well I r+'d some PR :)

Santiago Pastorino (Jul 23 2018 at 18:10, on Zulip):

ok

Santiago Pastorino (Jul 23 2018 at 18:10, on Zulip):

do you have an idea about what Felix has done?

Santiago Pastorino (Jul 23 2018 at 18:10, on Zulip):

he has a branch, I can check that

Santiago Pastorino (Jul 23 2018 at 18:10, on Zulip):

know there were some missing tests

Santiago Pastorino (Jul 23 2018 at 18:10, on Zulip):

and I guess I can make the changes you suggested on top of this r+'d PR :)

Santiago Pastorino (Jul 23 2018 at 18:10, on Zulip):

sounds good?

Santiago Pastorino (Jul 23 2018 at 18:10, on Zulip):

or do you want this to stay as is?

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

well pnkfelix had https://github.com/rust-lang/rust/pull/52566

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

which is the PR I r+'d

nikomatsakis (Jul 23 2018 at 18:16, on Zulip):

it adds buffering, I think that's all it does

nikomatsakis (Jul 23 2018 at 18:16, on Zulip):

so I guess the next step would be to build from there, presumably to start adding the =migrate logic?

Santiago Pastorino (Jul 23 2018 at 18:22, on Zulip):

yes, that's the old thing we did

Santiago Pastorino (Jul 23 2018 at 18:22, on Zulip):

he also had the =migrate logic

Santiago Pastorino (Jul 23 2018 at 18:22, on Zulip):

that's why I kind of stopped

nikomatsakis (Jul 23 2018 at 18:22, on Zulip):

"old thing"?

nikomatsakis (Jul 23 2018 at 18:22, on Zulip):

ok, I'm not really clear on that

Santiago Pastorino (Jul 23 2018 at 18:22, on Zulip):

I mean, it's the mergeable thing

nikomatsakis (Jul 23 2018 at 18:22, on Zulip):

I see

Santiago Pastorino (Jul 23 2018 at 18:22, on Zulip):

the current thing

Santiago Pastorino (Jul 23 2018 at 18:23, on Zulip):

but I was wondering if he had something new

nikomatsakis (Jul 23 2018 at 18:23, on Zulip):

I see

nikomatsakis (Jul 23 2018 at 18:23, on Zulip):

I don't really know :(

Santiago Pastorino (Jul 23 2018 at 18:23, on Zulip):

or if he implemented your proposed changes

nikomatsakis (Jul 23 2018 at 18:23, on Zulip):

we could try to find another task

nikomatsakis (Jul 23 2018 at 18:23, on Zulip):

should be possible ;)

nikomatsakis (Jul 23 2018 at 18:23, on Zulip):

he did implement my proposed changes

Santiago Pastorino (Jul 23 2018 at 18:23, on Zulip):

ahh where?

Santiago Pastorino (Jul 23 2018 at 18:23, on Zulip):

in that PR?

Santiago Pastorino (Jul 23 2018 at 18:24, on Zulip):

ahh I see, he fixed some commits

Santiago Pastorino (Jul 23 2018 at 18:24, on Zulip):

it would worth for me checking that

Santiago Pastorino (Jul 23 2018 at 18:25, on Zulip):

ok, I'd say I'd leave the rest of this work to @pnkfelix, most of the stuff is already done

Santiago Pastorino (Jul 23 2018 at 18:25, on Zulip):

I think we are spending a lot of time on checking with each other

Santiago Pastorino (Jul 23 2018 at 18:26, on Zulip):

the same happened to Felix when he needed to check my stuff :)

nikomatsakis (Jul 23 2018 at 18:26, on Zulip):

heh :)

nikomatsakis (Jul 23 2018 at 18:27, on Zulip):

I guess the question then is what maybe to look at next

nikomatsakis (Jul 23 2018 at 18:27, on Zulip):

not sure if you had anything else enqueued?

Santiago Pastorino (Jul 23 2018 at 18:32, on Zulip):

let me chat with you in private to avoid adding uninteresting stuff here :)

pnkfelix (Jul 24 2018 at 06:22, on Zulip):

There’s a branch with follow on work

pnkfelix (Jul 24 2018 at 06:22, on Zulip):

z-borrowck-migrate

pnkfelix (Jul 24 2018 at 06:23, on Zulip):

I think it’s about done; but it needs tests. I’ll do that today

pnkfelix (Jul 24 2018 at 20:47, on Zulip):

@nikomatsakis ah the situation I described earlier isn't nearly as dire as I had feared

nikomatsakis (Jul 24 2018 at 20:47, on Zulip):

that's good

nikomatsakis (Jul 24 2018 at 20:47, on Zulip):

I think i've about got that PR ready for your review

pnkfelix (Jul 24 2018 at 20:47, on Zulip):

yes, elaborate-drops does call out to move_data::gather_moves, and yes, that does return an Err in this case

nikomatsakis (Jul 24 2018 at 20:47, on Zulip):

I'm doing some rebasing and cleanup

pnkfelix (Jul 24 2018 at 20:47, on Zulip):

but the Err it returns includes the constructed MoveData

pnkfelix (Jul 24 2018 at 20:47, on Zulip):

along with a Vec of MoveErrors

pnkfelix (Jul 24 2018 at 20:47, on Zulip):

(which we, presumably, have already previously reported as warnings)

pnkfelix (Jul 24 2018 at 20:48, on Zulip):

so it might be enough to just ... let elaborate-drops muddle along with the returned MoveData

pnkfelix (Jul 24 2018 at 20:48, on Zulip):

it is still semi-risky, but not really much more than we are already being risky by allowing unsound code to compile at all in the name of backwards compat

nikomatsakis (Jul 24 2018 at 20:48, on Zulip):

hmm

nikomatsakis (Jul 24 2018 at 20:48, on Zulip):

well it can lead to ICEs

pnkfelix (Jul 24 2018 at 20:49, on Zulip):

sure

nikomatsakis (Jul 24 2018 at 20:49, on Zulip):

or just not work very well

nikomatsakis (Jul 24 2018 at 20:49, on Zulip):

that is, the code might do different stuff, not what it did before

nikomatsakis (Jul 24 2018 at 20:49, on Zulip):

still, maybe worth a try

nikomatsakis (Jul 24 2018 at 20:49, on Zulip):

I don't really know what the effect will be

nikomatsakis (Jul 24 2018 at 20:49, on Zulip):

I also do think it's ok to make this a kind of "hard error"

Jake Goulding (Jul 24 2018 at 20:53, on Zulip):

that is, the code might do different stuff, not what it did before

Ah, the old compiler-writer's "it's undefined behavior" :wink:

nikomatsakis (Jul 25 2018 at 14:23, on Zulip):

@pnkfelix ok I just reviewed https://github.com/rust-lang/rust/pull/52681/ -- seems great, one thing I was wondering though

nikomatsakis (Jul 25 2018 at 14:23, on Zulip):

first off, if you use #![feature(nll)] in this, still get the current mode, right?

nikomatsakis (Jul 25 2018 at 14:23, on Zulip):

I was wondering if we wanted to try and make this a future-compat lint, or if that is too much of a pain in the neck

pnkfelix (Jul 25 2018 at 14:23, on Zulip):

"current mode" as in, -Z borrowck overrides the feature?

nikomatsakis (Jul 25 2018 at 14:23, on Zulip):

no I mean what does #![feature(nll)] do -- hard errors, or migration mode?

nikomatsakis (Jul 25 2018 at 14:23, on Zulip):

did I miss a change there?

pnkfelix (Jul 25 2018 at 14:23, on Zulip):

right that's a good question

nikomatsakis (Jul 25 2018 at 14:24, on Zulip):

I do imagine some people will want to go with "hard error"

pnkfelix (Jul 25 2018 at 14:24, on Zulip):

I think I tried this out but forgot to make a test encoding the behavior

nikomatsakis (Jul 25 2018 at 14:24, on Zulip):

I was just thinking about the test best way to give them that :)

pnkfelix (Jul 25 2018 at 14:24, on Zulip):

I actually wanted to ask you

pnkfelix (Jul 25 2018 at 14:24, on Zulip):

is our short term plan

pnkfelix (Jul 25 2018 at 14:24, on Zulip):

to turn -Z borrowck=migrate on by default at the same time for both 2015 edition and 2018 edition

pnkfelix (Jul 25 2018 at 14:25, on Zulip):

or is our short term plan to have the 2018 edition, as part of EP2, enable -Z borrowck=migrate

pnkfelix (Jul 25 2018 at 14:25, on Zulip):

(and thus people who want to sample living in the future will have to opt into it)

pnkfelix (Jul 25 2018 at 14:25, on Zulip):

I've been assuming our plan is the latter.

pnkfelix (Jul 25 2018 at 14:26, on Zulip):

In addition, I've been personally planning that #![feature(nll)] should override the migration injected via 2018-edition opt-in

pnkfelix (Jul 25 2018 at 14:26, on Zulip):

Which probably means that it should also override -Z borrowck=migrate.

pnkfelix (Jul 25 2018 at 14:26, on Zulip):

(just to keep ourselves sane...)

pnkfelix (Jul 25 2018 at 14:26, on Zulip):

Does that all make sense to you? Again, I did not put in a test encoding that behavior

pnkfelix (Jul 25 2018 at 14:26, on Zulip):

but I want to do so

nikomatsakis (Jul 25 2018 at 14:26, on Zulip):

so it's kind of like: we take the "max" of various signals

nikomatsakis (Jul 25 2018 at 14:26, on Zulip):

#![feature(nll)]signals "hard error"

nikomatsakis (Jul 25 2018 at 14:27, on Zulip):

-Zborrowck=mir the same

nikomatsakis (Jul 25 2018 at 14:27, on Zulip):

--edition 2018 signals "migration"

nikomatsakis (Jul 25 2018 at 14:27, on Zulip):

-Zborrowck=migrate the same

nikomatsakis (Jul 25 2018 at 14:27, on Zulip):

?

pnkfelix (Jul 25 2018 at 14:27, on Zulip):

Yes

nikomatsakis (Jul 25 2018 at 14:27, on Zulip):

er, default is "ast" still

pnkfelix (Jul 25 2018 at 14:27, on Zulip):

Yes

nikomatsakis (Jul 25 2018 at 14:27, on Zulip):

I did not plan to enable it for everyone yet ,yeah, just the 2018 folks

nikomatsakis (Jul 25 2018 at 14:27, on Zulip):

that plan sounds fine to me

pnkfelix (Jul 25 2018 at 14:27, on Zulip):

Okay great

nikomatsakis (Jul 25 2018 at 14:28, on Zulip):

I guess this is all still nightly territory so anyone using it can as well use #![feature(nll)] if they want to force hard errors

nikomatsakis (Jul 25 2018 at 14:28, on Zulip):

so that seems good

pnkfelix (Jul 25 2018 at 14:28, on Zulip):

I wouldn't be surprised if the current PR does not currently encode that desired semantics of the relationship between #![feature(nll)] and -Z borrowck=migrate.

pnkfelix (Jul 25 2018 at 14:28, on Zulip):

But I want that to happen

pnkfelix (Jul 25 2018 at 14:28, on Zulip):

So I will right now 1. write the test, and then 2. ensure that it passes, hacking the code if necessary.

nikomatsakis (Jul 25 2018 at 14:29, on Zulip):

ok

nikomatsakis (Jul 25 2018 at 14:29, on Zulip):

I don't have a strong opinion about that particular thing

nikomatsakis (Jul 25 2018 at 14:29, on Zulip):

but I agree that the "max" interpretation is consistent

pnkfelix (Jul 25 2018 at 14:32, on Zulip):

oh I remember what I did test

pnkfelix (Jul 25 2018 at 14:33, on Zulip):

I wanted to see what -Z borrowck=mir -Z borrowck=migrate would do

pnkfelix (Jul 25 2018 at 14:33, on Zulip):

because that is what ends up happening for this test with compare-mode=nll.

nikomatsakis (Jul 25 2018 at 14:33, on Zulip):

:)

pnkfelix (Jul 25 2018 at 14:33, on Zulip):

(and the answer appears to be that the last flag wins. )

nikomatsakis (Jul 25 2018 at 14:33, on Zulip):

seems...yeah whatever :)

nikomatsakis (Jul 25 2018 at 14:33, on Zulip):

though I did wonder in passing

pnkfelix (Jul 25 2018 at 14:34, on Zulip):

yeah, that I don't care about

pnkfelix (Jul 25 2018 at 14:38, on Zulip):

Okay yeah I'm going to need to make another commit to the PR to get the "max" interpretation. Will have that up soon.

nikomatsakis (Jul 25 2018 at 14:39, on Zulip):

@pnkfelix can you also link to the edition?

nikomatsakis (Jul 25 2018 at 14:39, on Zulip):

I think we might as well at this point, don't you?

pnkfelix (Jul 25 2018 at 14:39, on Zulip):

Well can we try to land this first?

nikomatsakis (Jul 25 2018 at 14:39, on Zulip):

sure, we can do that later

nikomatsakis (Jul 25 2018 at 14:39, on Zulip):

I was just trying to save a bors cycle :)

pnkfelix (Jul 25 2018 at 14:40, on Zulip):

Its going to take me a little bit of effort to figure out the right way to hook in the edition

pnkfelix (Jul 25 2018 at 14:40, on Zulip):

if I can do it faster than bors then fine. But I dont think i will given the other things I want to address today.

lqd (Jul 25 2018 at 14:41, on Zulip):

(travis is not exactly cooperating rn anyway :)

nikomatsakis (Jul 25 2018 at 14:42, on Zulip):

ok

nikomatsakis (Jul 25 2018 at 14:42, on Zulip):

(travis is not exactly cooperating rn anyway :)

oh, is that why I've noticed an absence of green checkmarks?

lqd (Jul 25 2018 at 14:43, on Zulip):

very likely, there seems to be hook problems or something, in addition to the shutting down of builders and/or mac CI, it's a party

simulacrum (Jul 25 2018 at 14:44, on Zulip):

Yes we're working on it

simulacrum (Jul 25 2018 at 14:44, on Zulip):

Travis is currently ignoring rust-lang/rust for some reason

DPC (Jul 25 2018 at 14:49, on Zulip):

is it because of mac beta issue?

simulacrum (Jul 25 2018 at 14:51, on Zulip):

well possibly, we don't really knwo

DPC (Jul 25 2018 at 14:53, on Zulip):

ah there is https://github.com/rust-lang/rust/issues/52701

nikomatsakis (Jul 25 2018 at 14:59, on Zulip):

@simulacrum what is the "perf command" again...? is it documented somewhere?

nikomatsakis (Jul 25 2018 at 14:59, on Zulip):

I usually search zulip...

simulacrum (Jul 25 2018 at 14:59, on Zulip):

@rust-timer build <full commit hash>

simulacrum (Jul 25 2018 at 15:00, on Zulip):

Not documented

nikomatsakis (Jul 25 2018 at 15:00, on Zulip):

although given travis maybe @bors try will not work

simulacrum (Jul 25 2018 at 15:02, on Zulip):

Nope no builds for now

nikomatsakis (Jul 25 2018 at 15:07, on Zulip):

:'(

pnkfelix (Jul 26 2018 at 10:42, on Zulip):

@nikomatsakis wrote:

@pnkfelix can you also link to the edition?
I think we might as well at this point, don't you?

But: I'm now wondering if that step should wait until we've fixed the issues that block us bootstrapping rustc, at least?

nikomatsakis (Jul 26 2018 at 11:43, on Zulip):

why?

nikomatsakis (Jul 26 2018 at 11:44, on Zulip):

well I can see why

nikomatsakis (Jul 26 2018 at 11:44, on Zulip):

but that's a bit tricky

nikomatsakis (Jul 26 2018 at 11:44, on Zulip):

e.g., bootstrapping requires that things be in beta, etc

nikomatsakis (Jul 26 2018 at 11:44, on Zulip):

I definitely think NLL should be in EP2

pnkfelix (Jul 26 2018 at 11:52, on Zulip):

I guess I just anticipate someone, as a test, trying to make sure we can still bootstrap rustc under --edition 2018.

pnkfelix (Jul 26 2018 at 11:52, on Zulip):

so I figure why cause them pain?

pnkfelix (Jul 26 2018 at 11:53, on Zulip):

Anyway the PR that fixes bootstrapping rustc should be landing soon enough anyway, so sure, I'll probably put the edition linking into the same PR

Last update: Nov 21 2019 at 15:15UTC