Move the definition of `DefId` to librustc by nikomatsakis · Pull Request #27856 · rust-lang/rust

nikomatsakis · 2015-08-16T11:41:33Z

It doesn't really make sense for DefId to be in libsyntax -- it is concerned with a single crate only. It is the compiler that understands the idea of many crates. (At some point, there might be a useful intermediate point here.) This is a refactoring in support of incr. compilation, which will be adjusting the notion of a DefId to make it more durable across compilations.

This will probably be a [breaking-change] for every plugin ever. You need to adjust things as follows:

use rustc::middle::def_id::{DefId, LOCAL_CRATE}; // two most common definitions
ast_util::is_local(def_id) => def_id.is_local()
ast_util::local_def(node_id) => DefId::local(node_id)

rust-highfive · 2015-08-16T11:41:47Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

arielb1 · 2015-08-16T12:04:08Z

src/librustc/middle/def_id.rs

Could you have an as_local(&self) -> Option<NodeId> method instead of using .krate == LOCAL_CRATE everywhere?

@arielb1

Could you have an as_local(&self) -> Option method instead of using .krate == LOCAL_CRATE everywhere?

We have is_local() as well, I could convert some code to use that.

But you just reminded me that I meant to purge the DEF_ID_DEBUG thing...

(But as_local would be nice too to be more type-safe)

Hmm, I guess DEF_ID_DEBUG may still have value, I'm not sure if the tcx is ALWAYS available when DefIds are being debugged, and there doesn't seem to be a method for "conditionally" reading the current tcx.

I was going to mention it but I see you've already thought about doing it.
The only method that needs to be exposed is is_set.

nikomatsakis · 2015-08-16T13:03:54Z

@eddyb what are your thoughts about DEF_ID_DEBUG. I wanted to purge it, but it seems like it may still play some role to me, as the tcx may not yet have been entered. I could add a method "tls::opt_with" or something, but I'm curious what you think is cleanest thing.

eddyb · 2015-08-16T14:20:32Z

LGTM after the ty::tls::with_opt addition and the new Debug implementation for DefId.

nikomatsakis · 2015-08-16T14:22:04Z

@arielb1 I investigated as_local_id(), but honestly the code felt kind of messier afterwards; frequently, the branches aren't using the node field, and other times, it just adds another identifier to the mix (versus def_id.node). So I'm inclined to leave that sort of refactoring for later.

nikomatsakis · 2015-08-16T14:22:20Z

r? @eddyb

eddyb · 2015-08-16T14:27:44Z

src/librustc/middle/def_id.rs

I'm not sure there's even a point to restricting this to only certain nodes anymore, if there ever was.

@eddyb

I'm not sure there's even a point to restricting this to only certain nodes anymore, if there ever was.

perhaps not; looking over the code it seems like it will always gin up some kind of path, even if it's an empty or useless one.

eddyb · 2015-08-16T14:31:38Z

r=me after optionally removing the node kind restriction (does something break if you do?) and maybe even printing DefId paths for non-local IDs.

nikomatsakis · 2015-08-16T16:10:39Z

@bors r=eddyb

bors · 2015-08-16T16:10:40Z

📌 Commit 3521bf3 has been approved by eddyb

bors · 2015-08-16T16:26:28Z

⌛ Testing commit 3521bf3 with merge 7d400d6...

bors · 2015-08-16T17:17:42Z

💔 Test failed - auto-mac-32-opt

bors · 2015-08-17T23:46:18Z

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

nikomatsakis · 2015-08-18T22:48:38Z

Test failure looks like #27583. Hopefully #27892 will fix it.

bors · 2015-08-22T10:03:30Z

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

think it can no longer panic

nikomatsakis · 2015-08-24T10:03:46Z

@bors r=eddyb

bors · 2015-08-24T10:03:46Z

📌 Commit 1994875 has been approved by eddyb

bors · 2015-08-24T10:03:49Z

⌛ Testing commit 1994875 with merge db67cbe...

It doesn't really make sense for DefId to be in libsyntax -- it is concerned with a single crate only. It is the compiler that understands the idea of many crates. (At some point, there might be a useful intermediate point here.) This is a refactoring in support of incr. compilation, which will be adjusting the notion of a DefId to make it more durable across compilations. This will probably be a [breaking-change] for every plugin ever. You need to adjust things as follows: use rustc::middle::def_id::{DefId, LOCAL_CRATE}; // two most common definitions ast_util::is_local(def_id) => def_id.is_local() ast_util::local_def(node_id) => DefId::local(node_id)

bors · 2015-08-24T12:47:56Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned pnkfelix Aug 16, 2015

nikomatsakis force-pushed the move-def-id-to-rustc branch from e7fc786 to 0b21080 Compare August 16, 2015 11:45

arielb1 reviewed Aug 16, 2015
View reviewed changes

rust-highfive assigned eddyb and unassigned pnkfelix Aug 16, 2015

eddyb reviewed Aug 16, 2015
View reviewed changes

nikomatsakis force-pushed the move-def-id-to-rustc branch 3 times, most recently from 755fbf7 to 3521bf3 Compare August 16, 2015 16:09

nikomatsakis force-pushed the move-def-id-to-rustc branch from 3521bf3 to c71466f Compare August 18, 2015 22:48

nikomatsakis force-pushed the move-def-id-to-rustc branch from c71466f to d735c65 Compare August 19, 2015 08:37

alexcrichton mentioned this pull request Aug 19, 2015

Fix #23808: dead_code lint: visit types in paths #27565

Merged

nikomatsakis added 4 commits August 24, 2015 05:34

move def-id to rustc crate

afba694

fallout from moving def-id

e91bef2

convert to use is_local instead of == LOCAL_CRATE

c0de23d

purge DEF_ID_DEBUG TLS variable, and just always print a path, since I

1994875

think it can no longer panic

nikomatsakis force-pushed the move-def-id-to-rustc branch from d735c65 to 1994875 Compare August 24, 2015 10:00

bors merged commit 1994875 into rust-lang:master Aug 24, 2015

This was referenced Aug 24, 2015

Speed up operations on scopes #27943

Merged

Allow where clauses involving types which don't include a type parameter. #27972

Closed

thepowersgang added a commit to thepowersgang/tag_safe that referenced this pull request Aug 27, 2015

Fix after rust-lang/rust#27856

0485223

nikomatsakis deleted the move-def-id-to-rustc branch March 30, 2016 16:16

Uh oh!

Conversation

nikomatsakis commented Aug 16, 2015

Uh oh!

rust-highfive commented Aug 16, 2015

Uh oh!

arielb1 Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

eddyb Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Aug 16, 2015

Uh oh!

eddyb commented Aug 16, 2015

Uh oh!

nikomatsakis commented Aug 16, 2015

Uh oh!

nikomatsakis commented Aug 16, 2015

Uh oh!

eddyb Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

nikomatsakis Aug 16, 2015

Choose a reason for hiding this comment

Uh oh!

eddyb commented Aug 16, 2015

Uh oh!

nikomatsakis commented Aug 16, 2015

Uh oh!

bors commented Aug 16, 2015

Uh oh!

bors commented Aug 16, 2015

Uh oh!

bors commented Aug 16, 2015

Uh oh!

bors commented Aug 17, 2015

Uh oh!

nikomatsakis commented Aug 18, 2015

Uh oh!

bors commented Aug 22, 2015

Uh oh!

nikomatsakis commented Aug 24, 2015

Uh oh!

bors commented Aug 24, 2015

Uh oh!

bors commented Aug 24, 2015

Uh oh!

bors commented Aug 24, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants