Stream: t-compiler/wg-nll

Topic: rustc internals notes


pnkfelix (Jun 05 2018 at 15:02, on Zulip):

@nikomatsakis You said it should be safe to thread down the hir::HirId, right? As in attaching it to the mir::LocalDecl should not cause dep-graph to break?

nikomatsakis (Jun 05 2018 at 15:05, on Zulip):

yes

nikomatsakis (Jun 05 2018 at 15:06, on Zulip):

HirId is "incremental safe"

pnkfelix (Jun 05 2018 at 19:13, on Zulip):

sorry to be such a pill about this, but I really would like to understand better. You are saying that you would not expect a diff like the one on this gist to cause the compile-fail errors also on that gist: https://gist.github.com/pnkfelix/e3e27cdfa0f536e6334ffa5e1edd199b#file-thread-hir_id-diff

pnkfelix (Jun 05 2018 at 19:14, on Zulip):

To be clear, there are some other changes that were part of that range of commits; but these are the only ones that modified internal compiler data structures, and thus the only ones relevant to dep-graph...?

pnkfelix (Jun 05 2018 at 19:14, on Zulip):

Is there some other step I need to also do in order to satisfy dep-graph's needs?

pnkfelix (Jun 05 2018 at 19:17, on Zulip):

(To provide a bit more context: the new enum BindingForm and struct VarBindingForm are the things attached to the LocalDecl.is_user_variable, and that is the only place they arise. You can sort of see how they are used in the diff if you look at the if let in src/librustc_mir/borrow_check/mod.rs)

nikomatsakis (Jun 05 2018 at 19:22, on Zulip):

@pnkfelix I did not expect that

nikomatsakis (Jun 05 2018 at 19:23, on Zulip):

I could imagine some changes to incremental tests

pnkfelix (Jun 05 2018 at 19:23, on Zulip):

maybe I'll just put aside the threading-down the HirId approach for now

nikomatsakis (Jun 05 2018 at 19:23, on Zulip):

but the ICEs surprise me

pnkfelix (Jun 05 2018 at 19:23, on Zulip):

there's only so many large swaths of infrastructure that I want to revise at once

nikomatsakis (Jun 05 2018 at 19:23, on Zulip):

fair :)

pnkfelix (Jun 05 2018 at 19:23, on Zulip):

/me is no pcwalton

pnkfelix (Jun 05 2018 at 19:24, on Zulip):

^ above is stated with utmost admiration, of course

pnkfelix (Jun 05 2018 at 19:24, on Zulip):

(deleted)

pnkfelix (Jun 05 2018 at 20:39, on Zulip):

Is there some other step I need to also do in order to satisfy dep-graph's needs?

do I perhaps need to encapsulate the fields that use HirId inside of ClearCrossCrate instances in some manner?

pnkfelix (Jun 05 2018 at 20:40, on Zulip):

(I just happened to notice a NodeId under mir::VisibilityScopeInfo but I am inferring that ClearCrossCrate<IndexVec<VisibilityScopeIndex, VisibilityScopeInfo>> is what is making that not break...)

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

hmm. That's .. plausible actually.

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

but I don't think you should have to

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

do you have a RUST_BACKTRACE of what is ICEing?

pnkfelix (Jun 05 2018 at 21:42, on Zulip):

(sorry had to go afk to deal with Logan; will try to reproduce now)

pnkfelix (Jun 05 2018 at 21:44, on Zulip):

but the ICE's are of index out of bound errors

pnkfelix (Jun 05 2018 at 21:44, on Zulip):

I know that is not helpful. :)

pnkfelix (Jun 05 2018 at 21:44, on Zulip):

(its more that I had at first assumed that a backtrace would not be useful. But now I am rethinking that assumption.)

pnkfelix (Jun 05 2018 at 22:02, on Zulip):

I added a backtrace to the same gist. https://gist.github.com/pnkfelix/e3e27cdfa0f536e6334ffa5e1edd199b

pnkfelix (Jun 05 2018 at 22:03, on Zulip):

i guess this might be relevant https://gist.github.com/pnkfelix/e3e27cdfa0f536e6334ffa5e1edd199b#file-c-backtrace-txt-L27

pnkfelix (Jun 05 2018 at 22:07, on Zulip):

but I need to hit the sack now.

nikomatsakis (Jun 05 2018 at 23:28, on Zulip):

your hypothesis about the "cross-crate wrapper" type (I forgot the name) might be correct, I think

pnkfelix (Jun 06 2018 at 09:23, on Zulip):

@nikomatsakis can you think of any reason not to #[derive(Copy)] on mir::ClearCrossCrate<T>` ?

nikomatsakis (Jun 06 2018 at 09:30, on Zulip):

no

pnkfelix (Jun 06 2018 at 09:43, on Zulip):

Okay. It looks like adding mir::ClearCrossCrate around the state I'm adding to mir::LocalDecl.is_user_variable fixes the dep-graph problems I was having. Yay!

nikomatsakis (Jun 06 2018 at 09:49, on Zulip):

interesting

nikomatsakis (Jun 06 2018 at 09:49, on Zulip):

makes sense though, from the backtraces

pnkfelix (Jun 06 2018 at 09:50, on Zulip):

I don't understand what's wrong with HirId though

pnkfelix (Jun 06 2018 at 09:51, on Zulip):

based on my cursory understanding of the overall picture

pnkfelix (Jun 06 2018 at 09:51, on Zulip):

DefId itself is stable, right?

pnkfelix (Jun 06 2018 at 09:51, on Zulip):

("stable" in the sense that it can be safely embedded in MIR data structures)

nikomatsakis (Jun 06 2018 at 12:29, on Zulip):

DefId itself is stable, right?

yes

nikomatsakis (Jun 06 2018 at 12:30, on Zulip):

I don't understand what's wrong with HirId though

if I had to guess, there is some code that is assuming (without asserting) that the HirId is local to the current crate? I agree that it seems sorta like a bug to me

nikomatsakis (Jun 06 2018 at 12:30, on Zulip):

or perhaps it's that you are indexing into things that are not in the metadata otherwise...?

nikomatsakis (Jun 06 2018 at 12:30, on Zulip):

I'm not entirely sure

Last update: Nov 21 2019 at 13:10UTC