Skip to content

Replace DefId with LocalDefId where possible #70853

@ecstatic-morse

Description

@ecstatic-morse
Contributor

In the compiler, a DefId is used to lookup a single "definition" (type, method, const, etc.) somewhere in the code. It can refer to definitions in the crate currently being compiled or to definitions in other crates. There are quite a few places in the compiler which will only work if passed a local DefId--maybe they need to access the HIR for that definition, which is only available in the current crate--but accept DefId as a parameter. These places should use LocalDefId instead.

To resolve this issue, you need to find functions or methods that will panic if a DefId is not local. Such places should be calling DefId::expect_local and then working with the returned LocalDefId, but 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. The end goal is to move the call to expect_local as high up in the call graph as we can. If possible, it should be the first thing we do when executing a query like so,

is_const_impl_raw: |tcx, def_id| is_const_impl_raw(tcx, def_id.expect_local()),

Ideally this would be done module-by-module so it can be reviewed more easily (not in a single, giant PR). See the last commit in #66132 for prior art.

This issue has been assigned to @marmeladema via this comment.

Activity

added
E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
C-cleanupCategory: PRs that clean code up or issues documenting cleanup.
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
E-help-wantedCall for participation: Help is requested to fix this issue.
on Apr 6, 2020
lcnr

lcnr commented on Apr 6, 2020

@lcnr
Contributor

Is there a reason why the queries should keep using DefId instead of LocalDefId?

ecstatic-morse

ecstatic-morse commented on Apr 6, 2020

@ecstatic-morse
ContributorAuthor

@lcnr For many queries, the result is encoded into a crate's metadata so it can be looked up while compiling other crates. For these queries, only the initial computation of the result of needs to be done in the local crate. Cross-crate invocations will look up the cached result via metadata.

However, queries that are not encoded into a crate's metadata are only accessible in the local crate. Such queries should indeed take a LocalDefId instead of a DefId.

marmeladema

marmeladema commented on Apr 7, 2020

@marmeladema
Contributor

I can try to take a look, its sounds easy enough and well constrained.

Dylan-DPC-zz

Dylan-DPC-zz commented on Apr 7, 2020

@Dylan-DPC-zz
self-assigned this
on Apr 7, 2020
added a commit that references this issue on Apr 9, 2020

8 remaining items

added a commit that references this issue on Apr 27, 2020
added a commit that references this issue on Apr 28, 2020
marmeladema

marmeladema commented on Apr 28, 2020

@marmeladema
Contributor

@eddyb @ecstatic-morse the last PR i had has landed. Do you see any other places where LocalDefId should be used?

eddyb

eddyb commented on Apr 28, 2020

@eddyb
Member

There are some places that use HirId but are always owners, I think, which would allow using LocalDefId instead, but that doesn't (directly) count.

There might still be DefId which could be LocalDefId in smaller (or shorter-lived, I guess) context types, but I wouldn't know where to start looking for them.

added a commit that references this issue on Jun 27, 2020
added 2 commits that reference this issue on Jun 28, 2020
torhovland

torhovland commented on Nov 11, 2021

@torhovland
Contributor

Looks like this issue can be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

C-cleanupCategory: PRs that clean code up or issues documenting cleanup.E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.E-help-wantedCall for participation: Help is requested to fix this issue.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @torhovland@eddyb@marmeladema@Dylan-DPC-zz@ecstatic-morse

      Issue actions

        Replace `DefId` with `LocalDefId` where possible · Issue #70853 · rust-lang/rust