Stream: t-compiler

Topic: LocalDefId instead of DefId where possible


marmeladema (Apr 07 2020 at 23:57, on Zulip):

I gave a first try to https://github.com/rust-lang/rust/issues/70853 with https://github.com/rust-lang/rust/pull/70909
I know its really a tiny change compared to whats needed but i wanted to start with an easy-to-discuss change so that i can be confident i am not heading in the wrong direction.

eddyb (Apr 07 2020 at 23:57, on Zulip):

oh it's fine to do it in steps

marmeladema (Apr 08 2020 at 00:00, on Zulip):

Yep, the issue implies really a lot of tiny changes all over the place. Doing that in one go is pretty much unfeasible imo.

marmeladema (Apr 08 2020 at 00:01, on Zulip):

Thanks for the quick review!

eddyb (Apr 08 2020 at 00:02, on Zulip):

by the nit I mean https://github.com/rust-lang/rust/pull/70909#discussion_r405181950 btw

eddyb (Apr 08 2020 at 00:02, on Zulip):

the maps thing can be done later

marmeladema (Apr 08 2020 at 00:03, on Zulip):

Ok, i'll keep that in mind.

marmeladema (Apr 08 2020 at 13:25, on Zulip):

About https://github.com/rust-lang/rust/pull/70909, eddyb you approved but what are the next steps? Shall I push more commits to it? Or wait for that to be merged first and do another PR?

eddyb (Apr 08 2020 at 13:26, on Zulip):

wait for it to be merged, unless you have more commits ready and it's not in a rollup or something

eddyb (Apr 08 2020 at 14:19, on Zulip):

@marmeladema yeah so I guess feel free to add commits to it until https://buildbot2.rust-lang.org/homu/queue/rust shows your PR is getting close to being tested - right now "the tree is closed" probably because of some GitHub Pages breakage, so nothing is landed anyway

marmeladema (Apr 08 2020 at 14:30, on Zulip):

Thanks!

marmeladema (Apr 08 2020 at 14:31, on Zulip):

I am continuing to work locally for now but I have a question. Should as_local_hir_id in librustc_hir and librustc_middle take a LocalDefId as argument instead of a bare DefId?

marmeladema (Apr 08 2020 at 14:34, on Zulip):

According to the issue description:

you are more likely to see older idioms (e.g., tcx.as_local_hir_id(def_id).unwrap()). Code like this should be refactored to take a LocalDefId instead of a DefId and their caller made responsible for asserting that a given DefId is local.

eddyb (Apr 08 2020 at 14:35, on Zulip):

yeah

marmeladema (Apr 08 2020 at 14:38, on Zulip):

Cool because all those calls to .as_local_hir_id(def_id.to_def_id()) are making me crazy

marmeladema (Apr 08 2020 at 14:39, on Zulip):

I mean, i need to add the .to_def_id temporarily so that i can progress step by step, but i want to clean this up

eddyb (Apr 08 2020 at 14:48, on Zulip):

btw, if def_id: DefId, then .as_local_hir_id(def_id).unwrap()) should be .as_local_hir_id(def_id.expect_local()) - it shouldn't need to return Option itself if the input is LocalDefId

marmeladema (Apr 08 2020 at 15:36, on Zulip):

Noted.

marmeladema (Apr 08 2020 at 15:51, on Zulip):

I tried to change local_def_id(...) to return a LocalDefId, but its breaking literally everywhere. Especially in calls to tcx.generics_of() tcx.typeof(), tcx.variances_of(), etc... Should those methods changed to only accept a LocalDefId? If not it implies adding .to_def_id() everywhere?

bjorn3 (Apr 08 2020 at 15:53, on Zulip):

Those queries work fine for cross-crate DefId's. When queries are called on cross-crate DefId's they often just get dispatched to a different method.

mark-i-m (Apr 08 2020 at 15:54, on Zulip):

I wish I had seen this PR earlier. I think this could simplify my PR to add more DefKinds

marmeladema (Apr 08 2020 at 15:57, on Zulip):

So, its my first contribution so i slowly understanding how to compiler works. But for example, i tried to look for the implementation of variances_of and i found something in src/librustc_typeck/variance/mod.rs. And the first line of variances_of istcx.hir().as_local_hir_id(item_def_id).expect("expected local def-id")

marmeladema (Apr 08 2020 at 15:58, on Zulip):

It sounds that this one should accept a LocalDefId? Or is there another implementation for cross-crate queries somewhere that i missed?

bjorn3 (Apr 08 2020 at 15:58, on Zulip):

For example generics_of for local DefId's is at https://github.com/rust-lang/rust/blob/42abbd8878d3b67238f3611b0587c704ba94f39c/src/librustc_typeck/collect.rs#L66, while generics_of for cross-crate DefId's is at https://github.com/rust-lang/rust/blob/42abbd8878d3b67238f3611b0587c704ba94f39c/src/librustc_metadata/rmeta/decoder/cstore_impl.rs#L92

marmeladema (Apr 08 2020 at 15:58, on Zulip):

Oh cool i'll take a look to those

marmeladema (Apr 08 2020 at 15:59, on Zulip):

So basically, i am doomed and if I have to add calls to .to_def_id() all over the place :'(

ecstatic-morse (Apr 08 2020 at 17:17, on Zulip):

@bjorn3 @eddyb Why don't we make the helper methods on TyCtxt that look up query results take an impl Into<DefId> instead of a DefId?

ecstatic-morse (Apr 08 2020 at 17:17, on Zulip):

Also @marmeladema nice job so far! Thanks for taking this on.

eddyb (Apr 08 2020 at 17:17, on Zulip):

oh right I noted that down somewhere

eddyb (Apr 08 2020 at 17:17, on Zulip):

the Into thing

eddyb (Apr 08 2020 at 17:17, on Zulip):

/me then proceeded to forget it

eddyb (Apr 08 2020 at 17:18, on Zulip):

you'd likely want a different trait just so .into() doesn't become prevalent idk

eddyb (Apr 08 2020 at 17:18, on Zulip):

but generally you could make all the queries have an impl Into wrapper if we wanted to

eddyb (Apr 08 2020 at 17:18, on Zulip):

maybe a separate trait

eddyb (Apr 08 2020 at 17:19, on Zulip):

@ecstatic-morse this is easier due to the single-argument nature of queries (which is mostly because Rust has no VG, although the macros could let us allow more arities, hmm :P

ecstatic-morse (Apr 08 2020 at 17:31, on Zulip):

I don't have a problem with Into, but we can take a poll re: Into vs a custom trait at some point.

marmeladema (Apr 08 2020 at 18:15, on Zulip):

So about the trait. In the link for cross-crate generics_of defined here: https://github.com/rust-lang/rust/blob/42abbd8878d3b67238f3611b0587c704ba94f39c/src/librustc_metadata/rmeta/decoder/cstore_impl.rs#L72
there is the IntoArgs trait implemented for DefId. But i don't think the same mechanism is used for local crate?

marmeladema (Apr 08 2020 at 18:17, on Zulip):

Because i thought i could maybe implement it for LocalDefId

eddyb (Apr 08 2020 at 18:22, on Zulip):

that's unrelated

marmeladema (Apr 08 2020 at 18:23, on Zulip):

Ok, sorry i am a bit lost :/

eddyb (Apr 08 2020 at 18:23, on Zulip):

that IntoArgs is just a local hack

eddyb (Apr 08 2020 at 18:23, on Zulip):

nobody using the query system would see it

eddyb (Apr 08 2020 at 18:23, on Zulip):

that trait is private to that module

eddyb (Apr 08 2020 at 18:24, on Zulip):

and it just does a simple job

eddyb (Apr 08 2020 at 18:26, on Zulip):

@marmeladema this is the TyCtxt::generics_of function https://github.com/rust-lang/rust/blob/master/src/librustc_middle/ty/query/plumbing.rs#L424

eddyb (Apr 08 2020 at 18:26, on Zulip):

and every ohter query entry-point

eddyb (Apr 08 2020 at 18:26, on Zulip):

see also https://github.com/rust-lang/rust/blob/master/src/librustc_middle/ty/query/plumbing.rs#L461 just below

eddyb (Apr 08 2020 at 18:27, on Zulip):

so yeah you could replace key: $K by key: impl Into<$K>

eddyb (Apr 08 2020 at 18:27, on Zulip):

and then key.into() in the body

eddyb (Apr 08 2020 at 18:27, on Zulip):

actually the first one calls the second one so maybe don't use .into() in the first one

marmeladema (Apr 08 2020 at 18:35, on Zulip):

Why is it better to call .into() in the second one rather than in the first one?

marmeladema (Apr 08 2020 at 18:35, on Zulip):

(I don't mind either, its just not clear to me why one is better than the other)

eddyb (Apr 08 2020 at 18:36, on Zulip):

@marmeladema it's either both or just in the second one. but if both take impl Into, and the first calls the second, the first doesn't have to call .into() itself

eddyb (Apr 08 2020 at 18:36, on Zulip):

it can just pass whatever it got, along

eddyb (Apr 08 2020 at 19:06, on Zulip):

@marmeladema if you want to add more commits to this PR, lmk to r- it https://github.com/rust-lang/rust/pull/70909#issuecomment-611072623

marmeladema (Apr 08 2020 at 19:19, on Zulip):

Nah its fine, i'll do another PR.

marmeladema (Apr 09 2020 at 08:42, on Zulip):

So i have other commits ready, and I originally thought the first PR would be merged by that time but here we are. I fear that if i add more commits, the PR will be so big that i will endup rebasing and fixing big conflicts all the time to keep up with upstream changes. I am not sure how should I proceed.

eddyb (Apr 09 2020 at 08:44, on Zulip):

oh yeah it takes a while :P

eddyb (Apr 09 2020 at 08:46, on Zulip):

@marmeladema it's prioritized so all we have to do is review it quickly

eddyb (Apr 09 2020 at 08:46, on Zulip):

(if you push to the current PR)

eddyb (Apr 09 2020 at 08:46, on Zulip):

@marmeladema wait, do any of those commits change queries to take impl Into?

eddyb (Apr 09 2020 at 08:47, on Zulip):

because that should be a separate PR of its own

marmeladema (Apr 09 2020 at 08:47, on Zulip):

Yes ...

eddyb (Apr 09 2020 at 08:47, on Zulip):

it has too many implications outside of the scope of this change

marmeladema (Apr 09 2020 at 08:47, on Zulip):

I can have a separate PR for the impl Into

eddyb (Apr 09 2020 at 08:48, on Zulip):

small things can go in the current PR, as long as it's not getting close to the top of https://buildbot2.rust-lang.org/homu/queue/rust

eddyb (Apr 09 2020 at 08:48, on Zulip):

just don't forget to ping me here so I go review the PR again

mark-i-m (Apr 09 2020 at 13:40, on Zulip):

@marmeladema Unfortunately, I don't there is any good way around the who rebasing/conflicts problem. Almost all of my refactoring PRs have had to be rebased multiple times. It's a struggle.

marmeladema (Apr 09 2020 at 13:57, on Zulip):

Ok, then, everyone is in the same boat i guess :) I'll do the PR for the Into DefId first anyway as its kind of needed before further changes

marmeladema (Apr 09 2020 at 14:34, on Zulip):

Here we go https://github.com/rust-lang/rust/pull/70956

marmeladema (Apr 10 2020 at 11:34, on Zulip):

I have split my next PR in two to make progress while the discussion for `impl Into<$K> is going on: https://github.com/rust-lang/rust/pull/70986

eddyb (Apr 10 2020 at 11:38, on Zulip):

@marmeladema do you want this to land first, or do you want to simplify it if we do the impl Into stuff?

marmeladema (Apr 10 2020 at 11:39, on Zulip):

this does not rely on the impl Into stuff so i can be merged right now

marmeladema (Apr 10 2020 at 11:42, on Zulip):

The next commit, which is not part of this PR, is to change the return type of local_def_id() function but it implies adding .to_def_id() all over the place. Its "only" for this commit that i wanted to add the impl Into stuff. To keep the code base cleaner and to save me the burden of fixing hundreds of errors manually.

eddyb (Apr 10 2020 at 11:46, on Zulip):

aaah I see

marmeladema (Apr 10 2020 at 11:49, on Zulip):

And if the impl Into stuff is too controversial, i can call .to_def_id() everywhere I guess.

marmeladema (Apr 10 2020 at 15:29, on Zulip):

@eddyb you told me a few days ago that once as_local_hir_id takes a LocalDefId then it should not return an Option<> anymore. But how do i get rid of the check for DUMMY_HIR_ID? Right now the code does:
if hir_id != DUMMY_HIR_ID { Some(hir_id) } else { None }

eddyb (Apr 10 2020 at 15:29, on Zulip):

ughhh DUMMY_HIR_ID shouldn't exist

eddyb (Apr 10 2020 at 15:29, on Zulip):

is it really constructed in practice?

eddyb (Apr 10 2020 at 15:30, on Zulip):

@marmeladema try changing the code (without modifying signatures) to assert_ne!(hir_id, DUMMY_HIR_ID) and see if tests still pass

marmeladema (Apr 10 2020 at 15:31, on Zulip):

Ok thanks, i'll try that

marmeladema (Apr 10 2020 at 17:23, on Zulip):

I have 3 failing tests:
[ui] ui/save-analysis/issue-68621.rs
[ui] ui/type-alias-impl-trait/issue-63279.rs
[ui] ui/type-alias-impl-trait/issue-65679-inst-opaque-ty-from-val-twice.rs

eddyb (Apr 10 2020 at 17:25, on Zulip):

huuuh

eddyb (Apr 10 2020 at 17:26, on Zulip):

https://github.com/rust-lang/rust/pull/68744/files#diff-1d1b0d29a2e8da97c6bfb6e364d920c7L840-R840

eddyb (Apr 10 2020 at 17:27, on Zulip):

this looks like the wrong fix if I had to guess

eddyb (Apr 10 2020 at 17:28, on Zulip):

yeah it's impossible https://github.com/rust-lang/rust/blob/8926bb497d9b127eb318aea5aed0e745d8381591/src/librustc_passes/hir_id_validator.rs#L146-L154

eddyb (Apr 10 2020 at 17:29, on Zulip):

@marmeladema so the bug is that DUMMY_HIR_ID exists and things pass it around (but it doesn't come from the HIR)

eddyb (Apr 10 2020 at 17:30, on Zulip):

ah no I found it

eddyb (Apr 10 2020 at 17:31, on Zulip):

https://github.com/rust-lang/rust/blob/0c835b0cca83fe21090562603e4bda77c183ace3/src/librustc_ast_lowering/lib.rs#L569-L595

eddyb (Apr 10 2020 at 17:32, on Zulip):

@marmeladema so you could add an assert_ne!(ast_node_id, DUMMY_NODE_ID) in that function I just linked

eddyb (Apr 10 2020 at 17:32, on Zulip):

to see when the AST is malformed

eddyb (Apr 10 2020 at 17:32, on Zulip):

we really shouldn't be allowing this anymore...

marmeladema (Apr 10 2020 at 17:34, on Zulip):

Ok i added the new assert and i am recompiling/rerunning the tests

marmeladema (Apr 10 2020 at 18:18, on Zulip):

https://paste.ee/p/u8Vb4
does that help?

eddyb (Apr 10 2020 at 18:25, on Zulip):

@marmeladema that's not in rustc_ast_lowering

marmeladema (Apr 10 2020 at 18:28, on Zulip):

hum, i added the assert as the first line in the function oO

eddyb (Apr 10 2020 at 18:32, on Zulip):

@marmeladema just means it doesn't get triggered

eddyb (Apr 10 2020 at 18:32, on Zulip):

which means we can remove DUMMY_HIR_ID, maybe

marmeladema (Apr 10 2020 at 18:47, on Zulip):

Should it be a separate issue?

marmeladema (Apr 10 2020 at 18:55, on Zulip):

Also, should Res::Def(DefKind, LocalDefId)? Instead of DefId

eddyb (Apr 10 2020 at 18:57, on Zulip):

no, because it contains cross-crate things too

eddyb (Apr 10 2020 at 18:57, on Zulip):

like when you write String, that's Res::Def(DefKind::Struct, DefId(alloc::string::String))

marmeladema (Apr 10 2020 at 19:01, on Zulip):

ok

eddyb (Apr 10 2020 at 19:01, on Zulip):

marmeladema said:

Should it be a separate issue?

probably, yeah

marmeladema (Apr 11 2020 at 10:48, on Zulip):

I am trying to go forward with accepting direct LocalDefId as argument to as_local_hir_id() but it forces to add call to .expect_local() everywhere because there not a lot of places that already uses LocalDefId

marmeladema (Apr 11 2020 at 10:48, on Zulip):

I guess some types that hold a DefId could hold a LocalDefId but i am don't know how to recognize those cases

marmeladema (Apr 11 2020 at 10:56, on Zulip):

For example, in librustc_middle, should FnDef or Closure hold a LocalDefId instead of a DefId :thinking:

eddyb (Apr 11 2020 at 11:22, on Zulip):

@marmeladema yeah it's tricky but for example types can't use LocalDefId

eddyb (Apr 11 2020 at 11:22, on Zulip):

because they work cross-crate

eddyb (Apr 11 2020 at 11:22, on Zulip):

it's mostly things that need the HIR

eddyb (Apr 11 2020 at 11:23, on Zulip):

@marmeladema basically you can bubble up an expect_local through functions and then see if the data type always results in expect_local getting called

marmeladema (Apr 11 2020 at 21:54, on Zulip):

I am getting a panic during ./x.py test but the backtraces don't contain file and line number, is there a way to enable those debug info?

eddyb (Apr 11 2020 at 22:03, on Zulip):

debuginfo-level = 1 in config.toml

marmeladema (Apr 11 2020 at 22:05, on Zulip):

great thanks

marmeladema (Apr 12 2020 at 12:48, on Zulip):

So I have a branch ready where as_local_hir_id accepts LocalDefId instead of a DefId but it still rely on the impl Into<$K> for queries.

marmeladema (Apr 12 2020 at 12:49, on Zulip):

And as_local_hir_id still returns an Option<> because I haven't get rid of DUMMY_HIR_ID yet.

eddyb (Apr 12 2020 at 12:51, on Zulip):

@marmeladema but the assert you added rustc_ast_lowering never failed right?

eddyb (Apr 12 2020 at 12:52, on Zulip):

so the problem is things passing DUMMY_HIR_ID where a HirId is expected

marmeladema (Apr 12 2020 at 12:52, on Zulip):

And since https://github.com/rust-lang/rust/pull/70961 seems to slow. So i might add calls to to_def_id() everywhere. Should not take too long now that i have properly configured my code editor :P

marmeladema (Apr 12 2020 at 12:53, on Zulip):

So the 3 failings tests all fail in has_typeck_tables from librustc_typeck/check/mod.rs

marmeladema (Apr 12 2020 at 12:53, on Zulip):

And yes, the assert in ast lowering never triggered, only the one in as_local_hir_id

eddyb (Apr 12 2020 at 12:54, on Zulip):

did GH search just break? https://github.com/rust-lang/rust/search?q=DUMMY_HIR_ID

marmeladema (Apr 12 2020 at 12:54, on Zulip):

it works for me?

eddyb (Apr 12 2020 at 12:55, on Zulip):

it says " We couldn’t find any code matching 'DUMMY_HIR_ID' in rust-lang/rust "

eddyb (Apr 12 2020 at 12:55, on Zulip):

this works though https://github.com/search?q=DUMMY_HIR_ID+repo%3Arust-lang%2Frust+extension%3Ars&type=Code&ref=advsearch&l=&l=

eddyb (Apr 12 2020 at 12:55, on Zulip):

@marmeladema anyway I'll go through them and let you know which can be easily fixed

eddyb (Apr 12 2020 at 12:56, on Zulip):

if we can replace all of them with valid IDs or Option<HirId>, then we can get rid of DUMMY_HIR_ID

eddyb (Apr 12 2020 at 12:57, on Zulip):

or maybe I should do that separately

marmeladema (Apr 12 2020 at 12:57, on Zulip):

Here is the "more detailed" backtrace but line numbers are from my local branch: https://paste.ee/p/0XUZy

marmeladema (Apr 12 2020 at 12:58, on Zulip):

And about impl Into<$K>, shall i ignore for now and add calls to to_def_id?

eddyb (Apr 12 2020 at 13:17, on Zulip):

good news :) https://github.com/rust-lang/rust/pull/70961#issuecomment-612612269

eddyb (Apr 12 2020 at 13:18, on Zulip):

@marmeladema so you can base your work on that PR :D

marmeladema (Apr 12 2020 at 14:16, on Zulip):

Oh great!

marmeladema (Apr 12 2020 at 15:33, on Zulip):

Hu :/ rustdoc tests are failing. Probably because at some point as_local_hir_id returns None because of some DUMMY_HIR_ID somewhere. I think we should remove that dummy id before going further. Its too confusing otherwise.

marmeladema (Apr 13 2020 at 12:25, on Zulip):

By the way, are we merging https://github.com/rust-lang/rust/pull/70961?

eddyb (Apr 13 2020 at 12:26, on Zulip):

I kind of wanted some feedback but we don't have to I don't think

marmeladema (Apr 15 2020 at 17:42, on Zulip):

So i rebased my changes over master but now the compiler is is crashing here: https://paste.ee/p/7mwBb
Failure seems to b restricted to this specific commit: https://github.com/marmeladema/rust/commit/3d00f8dfee5e3fb92e7b75a5d7ce58d02dd10a9a
The one before builds and passes the tests properly

marmeladema (Apr 15 2020 at 17:44, on Zulip):

It crashes in find_entry on this code let node = owner.nodes[id.local_id].as_ref();:

thread 'rustc' panicked at 'index out of bounds: the len is 513 but the index is 980954', /home/adema/code/rust/src/libcore/slice/mod.rs:2872:10
marmeladema (Apr 15 2020 at 17:46, on Zulip):

@eddyb do you have any advises on how to debug my issue? I don't know what to look for :/

eddyb (Apr 15 2020 at 18:06, on Zulip):

looking now

eddyb (Apr 15 2020 at 18:10, on Zulip):

@marmeladema sounds like a HirId was decomposed

eddyb (Apr 15 2020 at 18:10, on Zulip):

and the LocalDefId and ItemLocalId are for different defs

eddyb (Apr 15 2020 at 18:11, on Zulip):

def_id.local_def_index == hir_id.owner.local_def_index can be just def_id == hir_id.owner but that couldn't cause this

marmeladema (Apr 15 2020 at 18:14, on Zulip):

I'll try that anyway, since i don't have any other idea^^

eddyb (Apr 15 2020 at 18:14, on Zulip):

I mean, it's just a nit

eddyb (Apr 15 2020 at 18:15, on Zulip):

@marmeladema you're sure it's that commit o_O?

marmeladema (Apr 15 2020 at 18:16, on Zulip):

Well, i'll try again the previous commits, but there is only two commits over master in this branch

marmeladema (Apr 15 2020 at 18:16, on Zulip):

so its either this one of the one before

eddyb (Apr 15 2020 at 18:19, on Zulip):

previous one being this? https://github.com/rust-lang/rust/commit/a989b80d44863480c9fe54e818f932fd83a0c979

marmeladema (Apr 15 2020 at 18:20, on Zulip):

yes exactly

marmeladema (Apr 15 2020 at 18:21, on Zulip):

https://github.com/marmeladema/rust/commit/a989b80d44863480c9fe54e818f932fd83a0c979

marmeladema (Apr 15 2020 at 18:27, on Zulip):

I am retrying the commit i thought was working

marmeladema (Apr 15 2020 at 18:27, on Zulip):

lets see

eddyb (Apr 15 2020 at 18:30, on Zulip):

this is stumping me

eddyb (Apr 15 2020 at 18:30, on Zulip):

there's nothing you did that, at a glance, could ever cause this

eddyb (Apr 15 2020 at 18:32, on Zulip):

@marmeladema only thing I can think of would be if you got variable shadowing wrong in a critical part of the compiler

eddyb (Apr 15 2020 at 18:33, on Zulip):

the first commit touches even less code sigh

eddyb (Apr 15 2020 at 18:33, on Zulip):

btw pro tip: https://github.com/marmeladema/rust/commit/3d00f8dfee5e3fb92e7b75a5d7ce58d02dd10a9a?w=1 (specifically the ?w=1 ignores whitespace changes)

marmeladema (Apr 15 2020 at 18:38, on Zulip):

Yeah i was kind of thinking of a shadowed variable somewhere. But i have not found it yet.

eddyb (Apr 15 2020 at 18:41, on Zulip):

wait why is it...

eddyb (Apr 15 2020 at 18:41, on Zulip):

that's a weird HirId you have there

eddyb (Apr 15 2020 at 18:42, on Zulip):

isn't num::from_str_radix a fn?

eddyb (Apr 15 2020 at 18:42, on Zulip):

the HirId should have local_id == ItemLocalId(0)

eddyb (Apr 15 2020 at 18:43, on Zulip):

btw I got that name from https://paste.ee/p/7mwBb#s=0&l=410

eddyb (Apr 15 2020 at 18:45, on Zulip):

this is so weird

marmeladema (Apr 15 2020 at 18:47, on Zulip):

I can confirm that a989b80d44863480c9fe54e818f932fd83a0c979 buids and passes the tests

eddyb (Apr 15 2020 at 18:48, on Zulip):

the only thing I can suggest is to try and figure out if you can debug! the exact accessed HirId and what's happening there

eddyb (Apr 15 2020 at 18:48, on Zulip):

I don't see how this could happen

eddyb (Apr 15 2020 at 18:48, on Zulip):

not to mention that 980954 is huge

marmeladema (Apr 15 2020 at 18:49, on Zulip):

Thanks, i'll add debug print everywhere i can :D

marmeladema (Apr 15 2020 at 20:28, on Zulip):
[attrs] id=HirId { owner: DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0]), local_id: 980954 }
[find_entry] id=HirId { owner: DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0]), local_id: 980954 }
eddyb (Apr 15 2020 at 20:28, on Zulip):

oh, DefId debug-prints well :D

eddyb (Apr 15 2020 at 20:30, on Zulip):

@marmeladema yeah so now you need to figure out how far back that is injected

eddyb (Apr 15 2020 at 20:30, on Zulip):

AFAIK that 980954 is nonsense

marmeladema (Apr 15 2020 at 20:47, on Zulip):
[check_attributes]
    hir_id=HirId { owner: DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0]), local_id: 0 }
    local_def_id(hir_id)=DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0])
[codegen_fn_attrs] id=DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0])
[get_attrs] did=DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0])
[attrs] id=HirId { owner: DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0]), local_id: 980954 }
[find_entry] id=HirId { owner: DefId(0:678 ~ core[d046]::num[0]::from_str_radix[0]), local_id: 980954 }
marmeladema (Apr 15 2020 at 20:51, on Zulip):

@eddyb should as_local_def_id (as_local_hir_d(def_id)) == def_id?

eddyb (Apr 15 2020 at 20:51, on Zulip):

yes

eddyb (Apr 15 2020 at 20:52, on Zulip):

oh right you can assert that very early, whenever the relevant tables are created

marmeladema (Apr 15 2020 at 20:52, on Zulip):

yeah that was one of my idea

eddyb (Apr 15 2020 at 20:57, on Zulip):

anyway yeah check_attributes has the right HirId but the LocalDefId -> HirId table seems broken

marmeladema (Apr 15 2020 at 21:45, on Zulip):

Surprisingly, sometime when i recompile, the same bug in find_entry happens but with another backtrace

marmeladema (Apr 15 2020 at 21:45, on Zulip):
query stack during panic:
#0 [param_env] processing `cmp::Ordering::Less::{{constant}}#0`
#1 [collect_mod_item_types] collecting item types in module `cmp`
#2 [analysis] running analysis passes on this crate
eddyb (Apr 15 2020 at 21:45, on Zulip):

this is somewhat disturbing :P

eddyb (Apr 15 2020 at 21:46, on Zulip):

almost sounds like a miscompilation of rustc

marmeladema (Apr 15 2020 at 21:50, on Zulip):

is find_entry recursive somehow?

eddyb (Apr 15 2020 at 21:50, on Zulip):

no

marmeladema (Apr 15 2020 at 21:57, on Zulip):

oh i was tricked by the closure in the backtrace

marmeladema (Apr 15 2020 at 22:15, on Zulip):

whatever the backtrace, local_id is always equal to 980954

marmeladema (Apr 15 2020 at 22:15, on Zulip):

its like corrupted in an almost predictable way

marmeladema (Apr 16 2020 at 00:27, on Zulip):

I did a x.py clean and rebuilt and the crash are still happening

marmeladema (Apr 16 2020 at 00:28, on Zulip):

is there a function where hirid are created? so that i can print each of them to check who is creating that bogus hirid?

eddyb (Apr 16 2020 at 00:28, on Zulip):

@marmeladema look in src/librustc_ast_lowering

eddyb (Apr 16 2020 at 00:29, on Zulip):

all of HirId creation should be somewhere in there

eddyb (Apr 16 2020 at 00:29, on Zulip):

searching for HirId { might be the easiest way to find all the places

marmeladema (Apr 16 2020 at 00:29, on Zulip):

can someone create an hirid after the call to init_node_id_to_hir_id_mapping?

eddyb (Apr 16 2020 at 00:29, on Zulip):

not sure

eddyb (Apr 16 2020 at 00:30, on Zulip):

it's a bit of a mess right now

marmeladema (Apr 16 2020 at 00:31, on Zulip):

haha HirId { matches on function that returns an HirId of course^^

eddyb (Apr 16 2020 at 00:31, on Zulip):

lol

marmeladema (Apr 16 2020 at 19:43, on Zulip):

@eddyb i created https://github.com/rust-lang/rust/pull/71215 as a draft because i want to see if tests passes in CI, because i am totally lost in space right now. I added another commit to remove all unnecessary unwrap() but i don't think it will help fix my issue

eddyb (Apr 16 2020 at 19:48, on Zulip):

ping me when CI finishes

marmeladema (Apr 16 2020 at 20:10, on Zulip):

LOL tests now pass locally with the new commit

eddyb (Apr 16 2020 at 20:12, on Zulip):

uhhh oh

eddyb (Apr 16 2020 at 20:13, on Zulip):

/me gets nervous about potential miscompilation

eddyb (Apr 16 2020 at 20:13, on Zulip):

or maybe it was accidentally doing incremental at stage1? but that doesn't make much sense

marmeladema (Apr 16 2020 at 20:16, on Zulip):

I do have incremental compilation enabled in my config

eddyb (Apr 16 2020 at 20:16, on Zulip):

yeah but still, it shouldn't trigger in stage1, AFAIK

marmeladema (Apr 16 2020 at 20:23, on Zulip):

@eddyb GHA tests passed :tada:

marmeladema (Apr 16 2020 at 20:39, on Zulip):

At some point i wondered if I should have kept as_loca_hir_id(DefId) -> Option<HirId> but introduce local_def_id_to_hir_id(LocalDefId) -> HirId

marmeladema (Apr 16 2020 at 20:40, on Zulip):

because all those calls to as_local().map() they should be abstracted over a function

marmeladema (Apr 16 2020 at 20:40, on Zulip):

but at the same time, it helps to have explicitly call as_local() to identify more code that could be converted to LocalDefId only

eddyb (Apr 16 2020 at 20:48, on Zulip):

yes :D

marmeladema (Apr 16 2020 at 20:50, on Zulip):

When you say More .as_local().map( in if let RHS.

marmeladema (Apr 16 2020 at 20:50, on Zulip):

what do you mean exactly? I am not sure i understand

eddyb (Apr 16 2020 at 20:50, on Zulip):

I thought I could annotate all the places

eddyb (Apr 16 2020 at 20:50, on Zulip):

@marmeladema see the first comment

eddyb (Apr 16 2020 at 20:50, on Zulip):

I gave up after a few :P

marmeladema (Apr 16 2020 at 20:51, on Zulip):

its about shadowing def_id with if let Some(def_id) = def_id.as_local() and doing to lookup for hir_id later in another variable?

eddyb (Apr 16 2020 at 20:51, on Zulip):

yeah

marmeladema (Apr 16 2020 at 20:52, on Zulip):

Ok! Sorry i was not sure and because there are so many, I did not want to spend hours changing something that i had misunderstood

eddyb (Apr 16 2020 at 20:52, on Zulip):

np

eddyb (Apr 16 2020 at 21:15, on Zulip):

oops I should've used this feature https://github.com/rust-lang/rust/pull/71215/files?w=1

marmeladema (Apr 16 2020 at 22:42, on Zulip):

I think i've fixed all the comments

eddyb (Apr 16 2020 at 22:47, on Zulip):

@marmeladema were you able to search for all instances, not just the ones I commented on?

marmeladema (Apr 16 2020 at 22:47, on Zulip):

I looked for all i could find

eddyb (Apr 16 2020 at 22:48, on Zulip):

okay I'll show you a trick in a sec :P

marmeladema (Apr 16 2020 at 22:48, on Zulip):

You can look at the commit I added, there is a few more fixes than the one you commented :P

eddyb (Apr 16 2020 at 22:48, on Zulip):

I'm using the "Viewed" checkboxes

marmeladema (Apr 16 2020 at 22:49, on Zulip):

ah yeah saw that

eddyb (Apr 16 2020 at 22:50, on Zulip):

so the trick is that you can go to this URL: https://github.com/rust-lang/rust/pull/71215.patch

eddyb (Apr 16 2020 at 22:51, on Zulip):

ah but not even firefox lets me do regexes on that :P

eddyb (Apr 16 2020 at 22:52, on Zulip):

anyway just searching .as_local().map(| I'm seeing a bunch of if lets

eddyb (Apr 16 2020 at 22:52, on Zulip):

you might be able to do git diff master or something to see the same thing locally

marmeladema (Apr 16 2020 at 22:52, on Zulip):

hu still?

eddyb (Apr 16 2020 at 22:52, on Zulip):

wait

eddyb (Apr 16 2020 at 22:53, on Zulip):

is the joke on me

eddyb (Apr 16 2020 at 22:53, on Zulip):

ah yes it is

eddyb (Apr 16 2020 at 22:53, on Zulip):

@marmeladema I used the wrong suffix

eddyb (Apr 16 2020 at 22:53, on Zulip):

if you use .diff instead of .patch it's the entire combined diff

eddyb (Apr 16 2020 at 22:54, on Zulip):

if you use .patch, it's usable for things like git am, it has the exact commit history :P

marmeladema (Apr 16 2020 at 22:55, on Zulip):

yep i use that trick sometime too

marmeladema (Apr 16 2020 at 22:56, on Zulip):

i think i've missed one or two

eddyb (Apr 16 2020 at 22:56, on Zulip):

I posted a comment on two

eddyb (Apr 16 2020 at 22:56, on Zulip):

can't easily find more :P

eddyb (Apr 16 2020 at 22:57, on Zulip):

this finds exactly one of them curl 'https://patch-diff.githubusercontent.com/raw/rust-lang/rust/pull/71215.diff' | rg -C10 '^\+.*if let.*=$'

eddyb (Apr 16 2020 at 22:58, on Zulip):

anyway yeah I think we're now ready for perf :D

eddyb (Apr 16 2020 at 22:59, on Zulip):

(after you fix the last two. just so you don't have to push again while it does its thing)

marmeladema (Apr 16 2020 at 23:01, on Zulip):

i fixed your two comments, i could not find more

marmeladema (Apr 16 2020 at 23:01, on Zulip):

i've pushed

eddyb (Apr 16 2020 at 23:03, on Zulip):

thanks! :D

marmeladema (Apr 16 2020 at 23:16, on Zulip):

Assuming that perf tests are ok, what would the next steps?

marmeladema (Apr 16 2020 at 23:18, on Zulip):

For example, is there query that should only accept a localdefid and never a generic defid? Because we could probably change the key

marmeladema (Apr 16 2020 at 23:22, on Zulip):

@eddyb by the way local_def_id_to_hir_id already exists lol and as_local_hir_id calls it :D

eddyb (Apr 16 2020 at 23:22, on Zulip):

yeah I know :P

eddyb (Apr 16 2020 at 23:22, on Zulip):

I mean, I saw it on your PR

marmeladema (Apr 16 2020 at 23:22, on Zulip):

so we could get rid of as_local_hir_id entirely

eddyb (Apr 16 2020 at 23:23, on Zulip):

it's a bit long, but if you like it...

eddyb (Apr 16 2020 at 23:23, on Zulip):

the other thing is that I want the directions to be symmetrical

eddyb (Apr 16 2020 at 23:23, on Zulip):

i.e. hir_id_to_local_def_id

marmeladema (Apr 16 2020 at 23:23, on Zulip):

well, its just that it exists

eddyb (Apr 16 2020 at 23:23, on Zulip):

if you pick the one you want

eddyb (Apr 16 2020 at 23:23, on Zulip):

marmeladema said:

well, its just that it exists

you're rewriting the compiler, what's a little renaming :P

marmeladema (Apr 16 2020 at 23:23, on Zulip):

and having local in the name makes it pretty clear

eddyb (Apr 16 2020 at 23:24, on Zulip):

yeah that is a bonus

eddyb (Apr 16 2020 at 23:24, on Zulip):

actually, this is good, because eventually we want LocalDefId <-> HirId to be free

eddyb (Apr 16 2020 at 23:24, on Zulip):

i.e. each LocalDefId would be the root of a HIR subtree

eddyb (Apr 16 2020 at 23:26, on Zulip):

so the longer name will incentivize that transition

marmeladema (Apr 16 2020 at 23:27, on Zulip):

and tbh a lot of functions in the compiler are longer that that already^^

eddyb (Apr 16 2020 at 23:27, on Zulip):

lol

eddyb (Apr 16 2020 at 23:27, on Zulip):

@marmeladema as for queries, that is easy, -ish

eddyb (Apr 16 2020 at 23:28, on Zulip):

first off, you exclude https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/rmeta/decoder/cstore_impl.rs#L90-L236

eddyb (Apr 16 2020 at 23:28, on Zulip):

as all of those are supported cross-crate

eddyb (Apr 16 2020 at 23:29, on Zulip):

then, for the remaining queries that take DefIds, find where they're provided and see if they expect_local right away

eddyb (Apr 16 2020 at 23:29, on Zulip):

those two facts combined means they can be changed to take LocalDefId

marmeladema (Apr 16 2020 at 23:29, on Zulip):

ok

marmeladema (Apr 16 2020 at 23:30, on Zulip):

very cool thank you

eddyb (Apr 16 2020 at 23:35, on Zulip):

thank you for taking this on :D

marmeladema (Apr 16 2020 at 23:37, on Zulip):

I can't do anything more complicated with what i know about the compiler right now, so a bit of refactor allows me to learn a bit of things work.

marmeladema (Apr 17 2020 at 08:29, on Zulip):

@eddyb perf test is done :D

eddyb (Apr 17 2020 at 12:05, on Zulip):

aaaaaa love the noiseless :D

marmeladema (Apr 17 2020 at 13:14, on Zulip):

Of the perf results?

marmeladema (Apr 17 2020 at 13:19, on Zulip):

@eddyb could / should https://github.com/rust-lang/rust/blob/master/src/librustc_hir/lang_items.rs#L87 return an Option<LocalDefId> instead?

eddyb (Apr 17 2020 at 13:20, on Zulip):

no, lang items can be from other crates

marmeladema (Apr 17 2020 at 13:20, on Zulip):

this is very sad :(

eddyb (Apr 17 2020 at 13:20, on Zulip):

e.g. when you compile regular user code, all lang items are from core, alloc and std

marmeladema (Apr 17 2020 at 13:20, on Zulip):

ok

marmeladema (Apr 17 2020 at 13:26, on Zulip):

I am changing associated_items query to accept aLocalDefId instead of a DefId but i endup adding expect_local() everywhere

eddyb (Apr 17 2020 at 13:27, on Zulip):

@marmeladema that doesn't sound like a local query

marmeladema (Apr 17 2020 at 13:27, on Zulip):

iirc, its not in a list of external queries

eddyb (Apr 17 2020 at 13:28, on Zulip):

ah, this is https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/rmeta/decoder/cstore_impl.rs#L106-L111

eddyb (Apr 17 2020 at 13:28, on Zulip):

so, for associated_items: https://github.com/rust-lang/rust/search?q=%22fn+associated_items%22&unscoped_q=%22fn+associated_items%22

marmeladema (Apr 17 2020 at 13:29, on Zulip):

Yes this is the one i am modifying

eddyb (Apr 17 2020 at 13:29, on Zulip):

@marmeladema so here's the giveaway: the def_id isn't .expect_local'd, instead it's used to call a global query https://github.com/rust-lang/rust/blob/513a6473d69b3af34e6cdaa4efb288fe5283c3e9/src/librustc_ty/ty.rs#L223-L224

eddyb (Apr 17 2020 at 13:29, on Zulip):

if it had .expect_local then you could change its signature

marmeladema (Apr 17 2020 at 13:30, on Zulip):

but associated_item_def_ids expect a local id

marmeladema (Apr 17 2020 at 13:30, on Zulip):

and its the first called in associated_items

eddyb (Apr 17 2020 at 13:30, on Zulip):

no it doesn't: https://github.com/rust-lang/rust/blob/master/src/librustc_metadata/rmeta/decoder/cstore_impl.rs#L106-L111

eddyb (Apr 17 2020 at 13:30, on Zulip):

oh I see

eddyb (Apr 17 2020 at 13:30, on Zulip):

okay this is kind of confusing at first

eddyb (Apr 17 2020 at 13:31, on Zulip):

@marmeladema the function associated_item_def_ids is the local-only provider for that query

eddyb (Apr 17 2020 at 13:31, on Zulip):

but tcx.associated_item_def_ids can dispatch to the rustc_metadata provider

marmeladema (Apr 17 2020 at 13:31, on Zulip):

hu ooook

marmeladema (Apr 17 2020 at 13:31, on Zulip):

every makes much more sense now

eddyb (Apr 17 2020 at 13:31, on Zulip):

phew

marmeladema (Apr 17 2020 at 13:32, on Zulip):

Thanks

eddyb (Apr 17 2020 at 13:32, on Zulip):

/me was worried they'd make it worse

marmeladema (Apr 17 2020 at 13:33, on Zulip):

so identifying queries than should be converted to a local def is not that simple^^

eddyb (Apr 17 2020 at 13:33, on Zulip):

yeaaaah

eddyb (Apr 17 2020 at 13:33, on Zulip):

but usually the local-only stuff has expect_local quite early

eddyb (Apr 17 2020 at 13:34, on Zulip):

typeck_tables_of, for example, should have expect_local + no rustc_metadata counterpart

marmeladema (Apr 17 2020 at 13:35, on Zulip):

Ok i'll look into this one first then

marmeladema (Apr 17 2020 at 13:45, on Zulip):

Sorry to bother you with all my questions but i noticed that typeck_tables_of has cache_on_disk_if { key.is_local() }

marmeladema (Apr 17 2020 at 13:45, on Zulip):

Why cache that on disk if its only available for the local crate?

eddyb (Apr 17 2020 at 13:46, on Zulip):

haha

eddyb (Apr 17 2020 at 13:46, on Zulip):

probably copy-paste

eddyb (Apr 17 2020 at 13:46, on Zulip):

idk if there's a shorthand for cache_on_disk_if { true }

marmeladema (Apr 17 2020 at 13:47, on Zulip):

but why even bother caching that? Its already in memory because its the local crate right?

eddyb (Apr 17 2020 at 13:48, on Zulip):

"on disk" means between incremental compilations

eddyb (Apr 17 2020 at 13:48, on Zulip):

completely separate system to rmeta

marmeladema (Apr 17 2020 at 13:49, on Zulip):

oh ok

eddyb (Apr 17 2020 at 13:55, on Zulip):

(one day maybe we can combine them and have rmeta just be a partial incremental snapshot or something)

marmeladema (Apr 17 2020 at 14:32, on Zulip):

@eddyb should trait_impls in https://github.com/rust-lang/rust/blob/master/src/librustc_hir/hir.rs#L619 use LocalDefId?

marmeladema (Apr 17 2020 at 14:32, on Zulip):

The comment for the struct states that

/// The top-level data structure that stores the entire contents of
/// the crate currently being compiled.
eddyb (Apr 17 2020 at 14:32, on Zulip):

not for the DefId, I believe that's the trait being implemented

eddyb (Apr 17 2020 at 14:33, on Zulip):

Vec<HirId> OTOH.... could totally be LocalDefId

eddyb (Apr 17 2020 at 14:33, on Zulip):

same for modules and proc macros

eddyb (Apr 17 2020 at 14:33, on Zulip):

stuff like this lol https://github.com/rust-lang/rust/blob/master/src/librustc_hir/hir.rs#L1793

eddyb (Apr 17 2020 at 14:34, on Zulip):

ItemId, ImplItemId and TraitItemId could all hold LocalDefIds

eddyb (Apr 17 2020 at 14:34, on Zulip):

instead of HirIds

marmeladema (Apr 17 2020 at 14:36, on Zulip):

Dunno if that would help for my current "problem". I trying to check if trait_impls_of is purely local or not

eddyb (Apr 17 2020 at 14:36, on Zulip):

sorry

eddyb (Apr 17 2020 at 14:36, on Zulip):

it's not local

marmeladema (Apr 17 2020 at 14:37, on Zulip):

thanks^^

marmeladema (Apr 17 2020 at 14:40, on Zulip):

ah i think def_span should take a local def id because its unwrap() if not local

eddyb (Apr 17 2020 at 14:40, on Zulip):

def_span is in the rustc_metadata list :P

marmeladema (Apr 17 2020 at 14:41, on Zulip):

oh but its a query?

marmeladema (Apr 17 2020 at 14:41, on Zulip):

damn, i missed that

marmeladema (Apr 17 2020 at 18:27, on Zulip):

oh no my PR broke clippy

eddyb (Apr 17 2020 at 18:32, on Zulip):

that's normal, until we switch to git subtree

marmeladema (Apr 17 2020 at 20:41, on Zulip):

I am right to think that mir_borrowck query should take a LocalDefId?

marmeladema (Apr 17 2020 at 20:42, on Zulip):

As well as probably mir_validated

marmeladema (Apr 18 2020 at 11:44, on Zulip):

@eddyb i am trying to convert a DefIdSet to a FxHashSet<LocalDefId>but it triggers an error because it does not implement a train for ArenaAllocatable

marmeladema (Apr 18 2020 at 11:45, on Zulip):
ArenaAllocatable` is not satisfied
  --> src/librustc_mir/transform/mod.rs:97:21
   |
97 |     tcx.arena.alloc(set)
   |                     ^^^
   |                     |
   |                     expected an implementor of trait `rustc_middle::arena::ArenaAllocatable`
   |                     help: consider borrowing here: `&set`
   |
   = note: required because of the requirements on the impl of `rustc_middle::arena::ArenaAllocatable` for `std::collections::HashSet<rustc_span::def_id::LocalDefId, std::hash::BuildHasherDefault<rustc_hash::FxHasher>>`
eddyb (Apr 18 2020 at 11:45, on Zulip):

there's a thing somewhere

eddyb (Apr 18 2020 at 11:45, on Zulip):

lemme find it

marmeladema (Apr 18 2020 at 11:45, on Zulip):

I grepped ArenaAllocatable but what i found wasnt very useful

eddyb (Apr 18 2020 at 11:46, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_middle/arena.rs#L37

eddyb (Apr 18 2020 at 11:47, on Zulip):

we should also remove {DefId,Node}{Map,Set} and any other aliases

eddyb (Apr 18 2020 at 11:48, on Zulip):

they offer no benefits IMO

marmeladema (Apr 18 2020 at 11:48, on Zulip):

Ok, i might do that but in another PR

marmeladema (Apr 18 2020 at 11:56, on Zulip):

great that works fine! Thank you!

marmeladema (Apr 18 2020 at 12:02, on Zulip):

@eddyb about mir_validated and mir_borrowck taking a LocalDefId, does that seem sensible?

eddyb (Apr 18 2020 at 12:02, on Zulip):

probably, yeah

eddyb (Apr 18 2020 at 12:02, on Zulip):

since those only happen locally, and only optimized_mir, I think, is serialized cross-crate

eddyb (Apr 18 2020 at 12:03, on Zulip):

mir_borrowck definitely, mir_validated only if rmeta::encoder doesn't mention it

marmeladema (Apr 18 2020 at 12:03, on Zulip):

ok

eddyb (Apr 18 2020 at 12:03, on Zulip):

(but I suspect it doesn't)

marmeladema (Apr 18 2020 at 14:27, on Zulip):

@eddyb when do you think https://github.com/rust-lang/rust/pull/71215 will land? I am starting to have quite of a long list of commits locally on top of this

eddyb (Apr 18 2020 at 14:27, on Zulip):

oh drat we were too late

varkor (Apr 18 2020 at 14:27, on Zulip):

next week, after the next version of Rust is released

eddyb (Apr 18 2020 at 14:28, on Zulip):

it's fine to base a new PR on top of it, since it was approved

eddyb (Apr 18 2020 at 14:28, on Zulip):

git subtree can't come soon enough Q_Q

marmeladema (Apr 18 2020 at 14:28, on Zulip):

Hum, in this case, should the target branch be master or my already approved branch?

eddyb (Apr 18 2020 at 14:29, on Zulip):

oh that's always master, you'd just note in the PR description that it's based on another PR

eddyb (Apr 18 2020 at 14:29, on Zulip):

and we have the label S-blocked now

marmeladema (Apr 18 2020 at 14:31, on Zulip):

ok

marmeladema (Apr 18 2020 at 14:35, on Zulip):

About entry_fn query, it takes a cratenum as argument, but i _think_ i've only see it called with CRATE_LOCAL

marmeladema (Apr 18 2020 at 14:36, on Zulip):

So should it does not take any argument and return a LocalDefId? Or could i introduce a local_entry_fn query to return such a LocalDefId?

eddyb (Apr 18 2020 at 15:12, on Zulip):

the argument doesn't matter although we might want to change it idk

eddyb (Apr 18 2020 at 15:12, on Zulip):

it should return LocalDefId though

marmeladema (Apr 18 2020 at 15:14, on Zulip):

Hum can it return a LocalDefId if you request the entrypoint of a crate that is not the local one?

eddyb (Apr 18 2020 at 15:15, on Zulip):

you can't do that

eddyb (Apr 18 2020 at 15:15, on Zulip):

there are a bunch of CrateNum arguments that are always LOCAL_CRATE

eddyb (Apr 18 2020 at 15:16, on Zulip):

maybe they should be () or some struct LocalCrate; idk

eddyb (Apr 18 2020 at 15:16, on Zulip):

but if we change them, we should change them all at once otherwise it would be confusing

marmeladema (Apr 18 2020 at 15:16, on Zulip):

Oh ok, it a bit weird to even have an argument then no? If it fails with any other value than the current crate

eddyb (Apr 18 2020 at 15:16, on Zulip):

we didn't support () arguments back when they were added

marmeladema (Apr 18 2020 at 15:17, on Zulip):

I understand, i can add an assert in entry_fn as start that enforce that the argument is local crate

eddyb (Apr 18 2020 at 15:17, on Zulip):

oh yeah they should all have it

marmeladema (Apr 18 2020 at 15:17, on Zulip):

in the mean time

marmeladema (Apr 18 2020 at 15:17, on Zulip):

Ok sounds good

eddyb (Apr 18 2020 at 15:17, on Zulip):

if one of the queries that takes CrateNum (and is only used with LOCAL_CRATE) is missing one of those asserts, that's a mistake

marmeladema (Apr 18 2020 at 15:17, on Zulip):

I am slowly converting queries to local_def_id and removing expect_local where i can

marmeladema (Apr 18 2020 at 15:18, on Zulip):

entry_fn i am not sure exactly, but from what i've seen its only called with LOCAL_CRATE

eddyb (Apr 18 2020 at 15:18, on Zulip):

yeah that makes sense, we only search for fn main within one crate

eddyb (Apr 18 2020 at 15:19, on Zulip):

anyway, making the proc macro that you use to define queries allow argument counts != 1 and tupling them for the internals (but not for the tcx methods) would make this more sensible

eddyb (Apr 18 2020 at 15:19, on Zulip):

because you'd call tcx.entry_fn()

eddyb (Apr 18 2020 at 15:20, on Zulip):

but that's more effort

marmeladema (Apr 18 2020 at 17:01, on Zulip):

So @eddyb here we go: https://github.com/rust-lang/rust/pull/71292
I made one commit per query (sometime I cheated and handled 2 queries in one commit I have to admit :p)

marmeladema (Apr 18 2020 at 17:03, on Zulip):

I am starting to lack of ideas on how to find places that i could convert to use LocalDefId

RalfJ (Apr 19 2020 at 08:35, on Zulip):

marmeladema said:

oh no my PR broke clippy

another (or the same) PR also broke Miri. it's fine, the fix wasnt hard. ;)

marmeladema (Apr 19 2020 at 08:45, on Zulip):

Oh sorry it that was me, but yeah i changed / am changing some methods prototype about hird / defid etc so anyone using those will be broken :/

marmeladema (Apr 19 2020 at 08:46, on Zulip):

This one https://github.com/rust-lang/rust/pull/71292 might also break miri again

marmeladema (Apr 19 2020 at 11:20, on Zulip):

@eddyb i rebased over master in both PR. The perf results look good by the way: https://perf.rust-lang.org/compare.html?start=339a938fa6582d5c6f84d811680a1031c684c1c6&end=a70b0e7f3482873f39970306a89e352ead61b4f5

RalfJ (Apr 19 2020 at 11:46, on Zulip):

marmeladema said:

Oh sorry it that was me, but yeah i changed / am changing some methods prototype about hird / defid etc so anyone using those will be broken :/

that's okay, this happens with refactorings :)

RalfJ (Apr 19 2020 at 11:47, on Zulip):

the hardest part of me usually is figuring out from the PR what the pattern is -- typically the actual fix is quite mechanical. For example for https://github.com/rust-lang/miri/pull/1327 I had to learn about to_def_id.

RalfJ (Apr 19 2020 at 11:48, on Zulip):

so if the PR description says something like "to fix tools, you likely just need to insert to_def_id() in places where typechecking fails", that helps a lot

RalfJ (Apr 19 2020 at 11:49, on Zulip):

Also Cc'ing tool maintainers after the PR landed can help, so we can more quickly identify the PR. (please not before that, the actual PR review discussion would just be noise in my inbox.^^)

marmeladema (Apr 19 2020 at 11:49, on Zulip):

Oh yeah i should have though about that! I'll keep that in mind!

marmeladema (Apr 19 2020 at 11:53, on Zulip):

So @RalfJ i assume you maintain miri. Who else should cc for other tools?

RalfJ (Apr 19 2020 at 12:04, on Zulip):

looks like @matthiaskrgr usually does the rustup for clippy

RalfJ (Apr 19 2020 at 12:04, on Zulip):

(my GH handle is @RalfJung)

RalfJ (Apr 19 2020 at 12:04, on Zulip):

not sure if any other tool would be affected?

marmeladema (Apr 19 2020 at 12:26, on Zulip):

Ok thanks. I'll tag both of you in the PRs when its merged.

RalfJ (Apr 19 2020 at 13:09, on Zulip):

@marmeladema thanks <3

marmeladema (Apr 19 2020 at 20:21, on Zulip):

Oh no conflicts again. When does the beta freeze ends?^^

ecstatic-morse (Apr 19 2020 at 20:53, on Zulip):

@marmeladema https://forge.rust-lang.org/

marmeladema (Apr 19 2020 at 22:11, on Zulip):

Oh thank you, i was not aware of that forge :)

marmeladema (Apr 22 2020 at 07:28, on Zulip):

Is the no breaking week over? According to the forge, it should but i don't think the new release is out yet?

marmeladema (Apr 22 2020 at 07:34, on Zulip):

also @eddyb can i re-approve the PR "myself" but tagging you? I've other seen people do something like "@bors r=eddyb"

ecstatic-morse (Apr 22 2020 at 15:45, on Zulip):

Only org members can order bors around. I've added it back to the queue. Beta branched yesterday BTW. That happens before the previous beta gets promoted to stable

marmeladema (Apr 22 2020 at 16:03, on Zulip):

Ok thank you for the explanations!

LeSeulArtichaut (Apr 22 2020 at 16:05, on Zulip):

I believe that an org member can give someone the permission to approve a particular PR. But I haven't seen that often :eyes:

marmeladema (Apr 23 2020 at 08:21, on Zulip):

@ecstatic-morse @eddyb just rebased https://github.com/rust-lang/rust/pull/71215

ecstatic-morse (Apr 23 2020 at 18:39, on Zulip):

@marmeladema Another merge conflict :oops:. Sorry about the bad luck. Once you rebase you'll be at the top of the queue again.

ecstatic-morse (Apr 23 2020 at 18:41, on Zulip):

Thanks again for working on this! I was very happy to see how fast you resolved most of #70853 between #71215 and #71292

marmeladema (Apr 23 2020 at 18:43, on Zulip):

haha no worries, i am rebasing right now lets hope i won't mess up anything^^

marmeladema (Apr 23 2020 at 19:30, on Zulip):

running the tests locally as we speak :)

marmeladema (Apr 23 2020 at 19:31, on Zulip):

i can't wait for my new laptop to arrive so that i can compile faster

marmeladema (Apr 23 2020 at 19:43, on Zulip):

Rebase is done for https://github.com/rust-lang/rust/pull/71215

marmeladema (Apr 23 2020 at 19:45, on Zulip):

@ecstatic-morse lets hope next rollup won't conflict again

marmeladema (Apr 23 2020 at 19:45, on Zulip):

hu build were cancelled?

marmeladema (Apr 23 2020 at 19:45, on Zulip):

I am doomed lol

ecstatic-morse (Apr 23 2020 at 19:47, on Zulip):

Maybe a CI issue, try git commit --amend --no-edit --date "$(date)" followed by a force push?

marmeladema (Apr 23 2020 at 19:49, on Zulip):

Done! :fingers_crossed:
Thank you help helping me through this

marmeladema (Apr 23 2020 at 20:00, on Zulip):

hum cancelled again. I think GHA is having a hard time

ecstatic-morse (Apr 23 2020 at 20:06, on Zulip):

Oh well. BACK IN THE QUEUE.

marmeladema (Apr 23 2020 at 22:05, on Zulip):

Apparently there are errors while building librustdoc: https://dev.azure.com/rust-lang/rust/_build/results?buildId=27278&view=logs&j=74054d28-c774-5fab-ecc2-352fe71b230e&t=6fac545a-db64-5c3f-de7a-e9f2d46d7eae

marmeladema (Apr 23 2020 at 22:05, on Zulip):

but I don't know how to build it myself locally to fix the errors

marmeladema (Apr 23 2020 at 22:14, on Zulip):

I might just have to rebase again to reproduce the CI errors

marmeladema (Apr 23 2020 at 22:44, on Zulip):

Rebased https://github.com/rust-lang/rust/pull/71215 again. Its starting to be boring to rebase all the time :o

marmeladema (Apr 23 2020 at 22:50, on Zulip):

@ecstatic-morse if you could approve one more time that would be awesome

marmeladema (Apr 24 2020 at 07:38, on Zulip):

@RalfJ the first PR got merged. I cc'ed you on it to warn you that it might break miri.

marmeladema (Apr 25 2020 at 10:29, on Zulip):

@eddyb @ecstatic-morse do you think you will have some time to take a look at https://github.com/rust-lang/rust/pull/71292?

ecstatic-morse (Apr 25 2020 at 14:26, on Zulip):

I read through it and for all the parts of the compiler I'm familiar with it looks correct. You'll want eddy to review the rest though.

eddyb (Apr 27 2020 at 07:22, on Zulip):

sorry, I've been ignoring Rust stuff for the past week, looking at it now!

eddyb (Apr 27 2020 at 07:52, on Zulip):

@marmeladema alright, is this PR the only thing you had waiting on me?

marmeladema (Apr 27 2020 at 07:52, on Zulip):

Yep :) At least for me

marmeladema (Apr 27 2020 at 07:53, on Zulip):

Thanks, I'll fix the formatting nit and rebase over master

marmeladema (Apr 27 2020 at 07:54, on Zulip):

Oh right you already approved! Thanks!

LeSeulArtichaut (Apr 27 2020 at 07:58, on Zulip):

@eddyb If you want more, there's #71205 :innocent:

eddyb (Apr 27 2020 at 08:00, on Zulip):

@LeSeulArtichaut that will have to wait with everything else in my GH notifications

eddyb (Apr 27 2020 at 08:00, on Zulip):

I was asking for something related, just because it's easier to do it first

eddyb (Apr 27 2020 at 08:01, on Zulip):

and also @marmeladema has been waiting for me for the past week

LeSeulArtichaut (Apr 27 2020 at 08:01, on Zulip):

Yeah, I see you have 30+ PRs assigned to you :rolling_eyes:

eddyb (Apr 27 2020 at 08:01, on Zulip):

I do? dear lord

LeSeulArtichaut (Apr 27 2020 at 08:01, on Zulip):

Good luck :D

Bastian Kauschke (Apr 27 2020 at 09:33, on Zulip):

partey :sweat_smile:

marmeladema (Apr 28 2020 at 08:19, on Zulip):

@RalfJ i've cc'ed you in the PR, but I broke miri again

marmeladema (Apr 28 2020 at 08:21, on Zulip):

I've never built miri but i guess i could try to fix it myself

marmeladema (Apr 28 2020 at 16:39, on Zulip):

@eddyb here is a proposal to replace the argument type of entry_fn to (): https://github.com/rust-lang/rust/pull/71648

eddyb (Apr 28 2020 at 16:39, on Zulip):

just that one :P?

marmeladema (Apr 28 2020 at 16:40, on Zulip):

haha i wanted to check first if that was fine :) i'll do some more if the approach is sensible

marmeladema (Apr 28 2020 at 16:41, on Zulip):

Especially, i had to implement the Key trait for ()

marmeladema (Apr 28 2020 at 16:57, on Zulip):

Huhu niko answer about "no local crate" made think for a second that all the work i did to try to convert most things to LocalDefId was useless :'(

eddyb (Apr 28 2020 at 16:57, on Zulip):

haha no

eddyb (Apr 28 2020 at 16:58, on Zulip):

that is far into the future at the pace we're moving at

eddyb (Apr 28 2020 at 16:58, on Zulip):

and it's more useful in the interim than just using DefId everywhere

marmeladema (Apr 28 2020 at 16:58, on Zulip):

:sweat_smile:

LeSeulArtichaut (Apr 30 2020 at 16:19, on Zulip):

Is there a reason why codegen_fn_attrs takes a DefId as its argument? Will it be called for non-local IDs? Or is it because you haven't already reached it?

LeSeulArtichaut (Apr 30 2020 at 16:20, on Zulip):

I mean, for the time being, is it safe to just slap a .expect_local() in here?

bjorn3 (Apr 30 2020 at 16:20, on Zulip):

It will be called for foreign statics to know if they are for example #[thread_local].

LeSeulArtichaut (Apr 30 2020 at 16:20, on Zulip):

I need it for #[target_feature]

LeSeulArtichaut (Apr 30 2020 at 16:21, on Zulip):

I'd expect you can't have #[target_feature] on a foreign static, right?

bjorn3 (Apr 30 2020 at 16:23, on Zulip):

It may be possible that codegen_fn_attrs is the one to perform that validation.

bjorn3 (Apr 30 2020 at 16:23, on Zulip):

#[target_feature] is applied to #[inline] intrinsics in stdsimd, which means that they are often codegened from the foreign crate libcore.

LeSeulArtichaut (Apr 30 2020 at 16:23, on Zulip):

I'm gonna read the code then :eyes:

LeSeulArtichaut (Apr 30 2020 at 16:23, on Zulip):

Thanks for the answers!

bjorn3 (Apr 30 2020 at 16:23, on Zulip):

np

LeSeulArtichaut (Apr 30 2020 at 16:25, on Zulip):

Hmm, I don't think it is in codegen_fn_attrs

LeSeulArtichaut (Apr 30 2020 at 16:25, on Zulip):

I remember seeing that kind of checks

LeSeulArtichaut (Apr 30 2020 at 16:25, on Zulip):

But not there

LeSeulArtichaut (Apr 30 2020 at 16:30, on Zulip):

Oh that's rustc_passes::check_attr::check_target_feature

eddyb (May 01 2020 at 05:06, on Zulip):

isn't codegen_fn_attrs saved cross-crate?

eddyb (May 01 2020 at 05:06, on Zulip):

oh huh no

eddyb (May 01 2020 at 05:07, on Zulip):

I guess all it needs is available cross-crate

eddyb (May 01 2020 at 05:07, on Zulip):

@LeSeulArtichaut anyway, it's used all the time with functions, not just statics, cross-crate

eddyb (May 01 2020 at 05:08, on Zulip):

since it's what, uhh, codegen uses :P

eddyb (May 01 2020 at 05:08, on Zulip):

to check various properties of functions

LeSeulArtichaut (May 01 2020 at 09:42, on Zulip):

So if I need to do checking only, I can do that check if the ID is local? I'd assume that if you pass a cross-crate ID, that crate would already have been checked?

eddyb (May 01 2020 at 09:59, on Zulip):

that makes sense... ideally this would be split :/

eddyb (May 01 2020 at 09:59, on Zulip):

and/or the query cached cross-crate

LeSeulArtichaut (May 01 2020 at 10:15, on Zulip):

Alright, thanks a lot for the clarifications eddyb

marmeladema (May 02 2020 at 18:10, on Zulip):

I might have a "fix" for #71104 !

marmeladema (May 02 2020 at 18:28, on Zulip):

well ... a partial fix^^

marmeladema (May 02 2020 at 21:17, on Zulip):

@eddyb if you have some time. I tried to fix that fixme i added in https://github.com/rust-lang/rust/blob/master/src/librustc_privacy/lib.rs#L947
I replaced the call to opt_local_def_id_to_hir_id by as_local_hir_id to reproduce the crash. It crashes for a rustdoc test: https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/macro-in-closure.rs

I added some debug info to print the node_id<->def_id and node_id<->hir_id tables: https://paste.ee/p/ESN8m

I believe that individually node_id<->def_id tables and node_id<->hir_id tables are valid. Its when going from def_id to hir_id that it crashes because both set of tables are not in sync

marmeladema (May 02 2020 at 21:22, on Zulip):

Because node_id_to_hir_id and hir_id_to_node_id seems to only be set once in init_node_id_to_hir_id_mapping, my best guess is that there is bug around create_def_with_parent which seems to be the only function that manipulate def_id_to_node_id and node_id_to_def_id

marmeladema (May 02 2020 at 22:00, on Zulip):

My investigations have lead to librustc_resolve which add new def_id without adding corresponding hir_id

marmeladema (May 02 2020 at 22:00, on Zulip):

it must be on purpose?

Last update: May 29 2020 at 17:40UTC