rustc: use LocalDefId instead of DefIndex where possible. #66131

eddyb · 2019-11-05T19:09:45Z

That is, wherever DefIndex always referred to a "def" in the local crate, I replaced it with LocalDefId.
While LocalDefId already existed, it wasn't used a lot, but I hope I'm on the right track.

Unresolved questions:

~~should LocalDefId implement rustc_index::Idx?~~
- ~~this would get rid of a couple more DefIndex uses~~
~~should LocalDefId be encoded/decoded as just a DefIndex?~~
- ~~right now it's a bit messy, LocalDefId encodes/decodes like DefId~~
~~should DefId::assert_local be named something else, like expect_local?~~

A future PR should change tcx.hir().local_def_id(...) to return LocalDefId instead of DefId, as changing it in this PR would be too noisy.

r? @michaelwoerister cc @nikomatsakis @petrochenkov @Zoxc

src/librustc/hir/def_id.rs

petrochenkov · 2019-11-06T19:18:58Z

Is enforcing the invariant through type system the end goal here, or this is a part of some larger plan?
(Seems like a good idea regardless.)

petrochenkov · 2019-11-06T19:19:46Z

should DefId::assert_local be named something else, like expect_local?

expect_local looks better to me.

eddyb · 2019-11-06T19:46:51Z

Is enforcing the invariant through type system the end goal here, or this is a part of some larger plan?

I don't remember very well why I started this precisely, but I believe HirId's owner field, and related logic, was a significant part of it.

Probably the immediate next step would be to make {Impl,Trait}ItemId and ItemId use LocalDefId instead of HirId, and maybe even replace them altogether.
I'm experimenting with expanding the set of "HIR ID owners" from "just item-likes" towards "anything with a DefId", but the relevant code has been fighting me, hence switching to making PRs like this one.

I should mention that other than the invariant, LocalDefId could also be a performance boost (including potential uses for optimizing certain maps into flat arrays etc.).

Speaking of which:

@bors try @rust-timer queue

rust-timer · 2019-11-06T19:46:53Z

Awaiting bors try build completion

bors · 2019-11-06T19:47:02Z

⌛ Trying commit f1a113e with merge 5e1cb0a...

@michaelwoerister

rustc: use LocalDefId instead of DefIndex where possible. That is, wherever `DefIndex` always referred to a "def" in the local crate, I replaced it with `LocalDefId`. While `LocalDefId` already existed, it wasn't used a lot, but I hope I'm on the right track. Unresolved questions: * should `LocalDefId` implement `rustc_index::Idx`? * this would get rid of a couple more `DefIndex` uses * should `LocalDefId` be encoded/decoded as just a `DefIndex`? * right now it's a bit messy, `LocalDefId` encodes/decodes like `DefId` * should `DefId::assert_local` be named something else, like `expect_local`? * what do we do with `tcx.hir().local_def_id(...)`? * right now it returns `DefId`, but changing it in this PR would be noisy * ideally it would return `LocalDefId` to spare the caller of `.assert_local()` r? @michaelwoerister cc @nikomatsakis @petrochenkov @Zoxc

bors · 2019-11-06T22:14:41Z

☀️ Try build successful - checks-azure
Build commit: 5e1cb0a (5e1cb0a8eaabbb729bdab2fd2e8af0c616f9d34b)

eddyb · 2019-11-07T07:37:01Z

@rust-timer build 5e1cb0a

rust-timer · 2019-11-07T07:37:03Z

Queued 5e1cb0a with parent 3f0e164, future comparison URL.

rust-timer · 2019-11-07T13:32:16Z

Finished benchmarking try commit 5e1cb0a, comparison URL.

bors · 2019-11-08T19:12:46Z

☔ The latest upstream changes (presumably #66225) made this pull request unmergeable. Please resolve the merge conflicts.

michaelwoerister

Very nice! Thanks, @eddyb.

I left a few comments.

Regarding your questions:

should LocalDefId implement rustc_index::Idx?

Yes, I think that would be a good idea.

should LocalDefId be encoded/decoded as just a DefIndex?

In the incr. comp. cache we need to encode in the form of a DefPathHash for remapping. I'm not sure about crate metadata. Encoding as a DefId might make the code more consistent? How many cases are there where we encode a LocalDefId in crate metadata?

should DefId::assert_local be named something else, like expect_local?

As mentioned below, I also like expect_local more than assert_local.

what do we do with tcx.hir().local_def_id(...)?

Yeah, I'd also like this to return a LocalDefId. If it's too much for this PR, I'm fine with doing it later (maybe leave a FIXME, just in case).

src/librustc/hir/def_id.rs

michaelwoerister · 2019-11-11T15:36:53Z

src/librustc/hir/lowering/item.rs

@@ -1272,7 +1272,7 @@ impl LoweringContext<'_> {
            AnonymousLifetimeMode::PassThrough,
            |this, idty| this.lower_fn_decl(
                &sig.decl,
-                Some((fn_def_id, idty)),
+                Some((fn_def_id.to_def_id(), idty)),


I'm starting to wonder if this method should be called to_global_def_id...

src/librustc/hir/map/definitions.rs

src/librustc/hir/mod.rs

src/librustc/ty/query/keys.rs

src/librustc/ty/query/on_disk_cache.rs

eddyb · 2020-03-19T09:16:34Z

@bors r=petrochenkov

bors · 2020-03-19T09:16:35Z

📌 Commit 16e25f0 has been approved by petrochenkov

bors · 2020-03-19T09:18:55Z

⌛ Testing commit 16e25f0 with merge 3c6f982...

bors · 2020-03-19T12:21:49Z

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 3c6f982 to master...

Rustup to rust-lang/rust#66131 changelog: none

@ghost

Rollup of 4 pull requests Successful merges: - #5326 (rustup rust-lang/rust#69838) - #5333 (rustup rust-lang/rust#69189) - #5336 (rustup rust-lang/rust#69920) - #5341 (Rustup to rust-lang/rust#66131) Failed merges: r? @ghost

@ghost

Rollup of 4 pull requests Successful merges: - #5326 (rustup rust-lang/rust#69838) - #5333 (rustup rust-lang/rust#69189) - #5336 (rustup rust-lang/rust#69920) - #5341 (Rustup to rust-lang/rust#66131) Failed merges: r? @ghost changelog: none

@ghost

Rollup of 4 pull requests Successful merges: - #5326 (rustup rust-lang/rust#69838) - #5333 (rustup rust-lang/rust#69189) - #5336 (rustup rust-lang/rust#69920) - #5341 (Rustup to rust-lang/rust#66131) Failed merges: r? @ghost changelog: none

@ghost

Rollup of 4 pull requests Successful merges: - #5326 (rustup rust-lang/rust#69838) - #5333 (rustup rust-lang/rust#69189) - #5336 (rustup rust-lang/rust#69920) - #5341 (Rustup to rust-lang/rust#66131) Failed merges: r? @ghost changelog: none

@ghost

Rollup of 4 pull requests Successful merges: - #5326 (rustup rust-lang/rust#69838) - #5333 (rustup rust-lang/rust#69189) - #5336 (rustup rust-lang/rust#69920) - #5341 (Rustup to rust-lang/rust#66131) Failed merges: r? @ghost changelog: none

Fix oudated comment for NamedRegionMap `ResolveLifetimes` uses a `LocalDefId` since rust-lang#66131.

rust-highfive assigned michaelwoerister Nov 5, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2019

eddyb mentioned this pull request Nov 5, 2019

[WIP] rustc: push LocalDefId and/or assert_local calls farther back. #66132

Closed

Centril reviewed Nov 5, 2019

View reviewed changes

src/librustc/hir/def_id.rs Outdated Show resolved Hide resolved

petrochenkov self-assigned this Nov 5, 2019

This comment has been minimized.

Sign in to view

eddyb force-pushed the local-def-id branch from 864791f to f1a113e Compare November 6, 2019 18:04

petrochenkov removed their assignment Nov 6, 2019

eddyb force-pushed the local-def-id branch from f1a113e to 9c1f88e Compare November 8, 2019 17:36

michaelwoerister reviewed Nov 11, 2019

View reviewed changes

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2019

This comment has been minimized.

Sign in to view

eddyb force-pushed the local-def-id branch from 9c1f88e to a6666ed Compare November 28, 2019 20:02

This comment has been minimized.

Sign in to view

joelpalmer added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2020

This comment has been minimized.

Sign in to view

eddyb added 3 commits March 19, 2020 11:16

rustc: use LocalDefId instead of DefIndex in HirId.

f3ec069

rustc: use LocalDefId instead of DefIndex in ich.

2b0a21e

rustc: use LocalDefId instead of DefIndex in hir::map::definitions.

16e25f0

eddyb force-pushed the local-def-id branch from 8efa258 to 16e25f0 Compare March 19, 2020 09:16

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 19, 2020

bors merged commit 3c6f982 into rust-lang:master Mar 19, 2020

eddyb deleted the local-def-id branch March 19, 2020 12:25

bors mentioned this pull request Mar 19, 2020

Add queries for LocalDefId <-> HirId conversion #70039

Closed

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Mar 19, 2020

Rustup to rust-lang/rust#66131

21aeb21

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Mar 19, 2020

Rollup merge of rust-lang#5341 - flip1995:rustup, r=flip1995

606e328

Rustup to rust-lang/rust#66131 changelog: none

flip1995 mentioned this pull request Mar 19, 2020

Rollup of 4 pull requests rust-lang/rust-clippy#5342

Merged

bjorn3 mentioned this pull request Mar 20, 2020

Fix oudated comment for NamedRegionMap #70177

Merged

jdm mentioned this pull request Mar 20, 2020

Importing modules from inside include! no longer works #70185

Closed

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 20, 2020

Rollup merge of rust-lang#70177 - bjorn3:patch-2, r=Dylan-DPC

7dcf300

Fix oudated comment for NamedRegionMap `ResolveLifetimes` uses a `LocalDefId` since rust-lang#66131.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 21, 2020

Rollup merge of rust-lang#70177 - bjorn3:patch-2, r=Dylan-DPC

8ce4c1c

Fix oudated comment for NamedRegionMap `ResolveLifetimes` uses a `LocalDefId` since rust-lang#66131.

Centril added a commit to Centril/rust that referenced this pull request Mar 21, 2020

Rollup merge of rust-lang#70177 - bjorn3:patch-2, r=Dylan-DPC

632fa8e

Fix oudated comment for NamedRegionMap `ResolveLifetimes` uses a `LocalDefId` since rust-lang#66131.

Centril added a commit to Centril/rust that referenced this pull request Mar 21, 2020

Rollup merge of rust-lang#70177 - bjorn3:patch-2, r=Dylan-DPC

a3771e4

Fix oudated comment for NamedRegionMap `ResolveLifetimes` uses a `LocalDefId` since rust-lang#66131.

hellow554 mentioned this pull request Feb 24, 2021

Compiler panic with incorrect JoinHandle in thread_local var #82455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: use LocalDefId instead of DefIndex where possible. #66131

rustc: use LocalDefId instead of DefIndex where possible. #66131

eddyb commented Nov 5, 2019 •

edited

Loading

This comment has been minimized.

petrochenkov commented Nov 6, 2019

petrochenkov commented Nov 6, 2019

eddyb commented Nov 6, 2019

rust-timer commented Nov 6, 2019

bors commented Nov 6, 2019

bors commented Nov 6, 2019

eddyb commented Nov 7, 2019

rust-timer commented Nov 7, 2019

rust-timer commented Nov 7, 2019

bors commented Nov 8, 2019

michaelwoerister left a comment

michaelwoerister Nov 11, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Mar 19, 2020

bors commented Mar 19, 2020

bors commented Mar 19, 2020

bors commented Mar 19, 2020

rustc: use LocalDefId instead of DefIndex where possible. #66131

rustc: use LocalDefId instead of DefIndex where possible. #66131

Conversation

eddyb commented Nov 5, 2019 • edited Loading

This comment has been minimized.

petrochenkov commented Nov 6, 2019

petrochenkov commented Nov 6, 2019

eddyb commented Nov 6, 2019

rust-timer commented Nov 6, 2019

bors commented Nov 6, 2019

bors commented Nov 6, 2019

eddyb commented Nov 7, 2019

rust-timer commented Nov 7, 2019

rust-timer commented Nov 7, 2019

bors commented Nov 8, 2019

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Nov 11, 2019

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Mar 19, 2020

bors commented Mar 19, 2020

bors commented Mar 19, 2020

bors commented Mar 19, 2020

eddyb commented Nov 5, 2019 •

edited

Loading