Change DefId to be based on a path, and not a NodeId #28742
Conversation
nikomatsakis
force-pushed
the
def-id-encapsulate
branch
from
7fef91d
to
718a317
Compare
|
OK, so I scheduled a crater run to see whether the "creepy case" that I changed to |
|
OK, so, I've verified that always returning |
|
pushed a fix. |
| } | ||
|
|
||
| pub fn def_to_string(did: DefId) -> String { | ||
| format!("{}:{}", did.krate, did.node) | ||
| format!("{}:{}", did.krate, did.index.as_usize()) |
There was a problem hiding this comment.
I don't think def_to_string should exist, at all, especially if we have def_to_u64.
|
LGTM, modulo the few nits above. |
I didn't measure, but I can. |
nikomatsakis
force-pushed
the
def-id-encapsulate
branch
from
49fa6bc
to
040f007
Compare
|
☔ The latest upstream changes (presumably #28778) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I measured time to build libstd and observed:
|
they are being used as an opaque "position identifier"
were returned, either the trait or the *self type itself*, were not particularly representative of what the Def is (a type parameter). Rewrite paths to handle this case specially, just as they handle the primitive case specifically. This entire `def_id` codepath is kind of a mess.
paths, and construct paths for all definitions. Also, stop rewriting DefIds for closures, and instead just load the closure data from the original def-id, which may be in another crate.
have always been returning None anyway, since it was comparing node-ids across crates incorrectly -- and remove the now unused map `extern_const_variants`
nikomatsakis
force-pushed
the
def-id-encapsulate
branch
from
040f007
to
6bfdf37
Compare
|
@bors r=eddyb |
|
📌 Commit f0dc7bd has been approved by |
As described in rust-lang/rfcs#1298, the idea here is to make DefIds independent of changes to the content of other items. They are also *mostly* independent from ordering, so e.g. reordering two functions will change the defids, but it will not change the paths that they expand into (but this is not the case for some things, such as impls). This is a major refactoring, so I did it in slices. The final commit is in some sense The Big One where most of the work is done. The earlier commits just pave the way by gradually refactoring accesses to the `node` field. This is a [breaking-change] for plugin authors. The things you need to do to migrate your code are as follows: 1. For local def-ids, rather than do `def_id.node`, call `tcx.map.as_local_node_id(def_id)`. 2. To construct a local def-id, call `tcx.map.local_def_id(node_id)`. 3. Note that you cannot make def-ids for any node, but only for "definitions" -- which include all items, as well as a number of other things, but not e.g. arbitrary expressions. 4. You can get the path to a def-id by calling `tcx.def_path(def_id)`. One thing that is NOT part of this PR, but which I plan do in a follow-up, is converting uses of the existing `with_path` API to use `def_path`, which is basically the same. r? @eddyb (or @nrc)
As described in rust-lang/rfcs#1298, the idea here is to make DefIds independent of changes to the content of other items. They are also mostly independent from ordering, so e.g. reordering two functions will change the defids, but it will not change the paths that they expand into (but this is not the case for some things, such as impls).
This is a major refactoring, so I did it in slices. The final commit is in some sense The Big One where most of the work is done. The earlier commits just pave the way by gradually refactoring accesses to the
nodefield.This is a [breaking-change] for plugin authors. The things you need to do to migrate your code are as follows:
def_id.node, calltcx.map.as_local_node_id(def_id).tcx.map.local_def_id(node_id).tcx.def_path(def_id).One thing that is NOT part of this PR, but which I plan do in a follow-up, is converting uses of the existing
with_pathAPI to usedef_path, which is basically the same.r? @eddyb (or @nrc)