rustc: Replace `HirId`s with `LocalDefId`s in `AccessLevels` tables #87568

petrochenkov · 2021-07-28T21:07:52Z

and passes using those tables - primarily privacy checking, stability checking and dead code checking.

All these passes work with definitions rather than with arbitrary HIR nodes.
r? @cjgillot
cc @lambinoo (#87487)

rust-highfive · 2021-07-28T21:07:54Z

Some changes occurred in intra-doc-links.

petrochenkov · 2021-07-28T21:12:50Z

compiler/rustc_privacy/src/lib.rs

                break;
            }
-            module_id = self.tcx.hir().get_parent_node(module_id);
+            module_def_id =
+                ty::DefIdTree::parent(self.tcx, module_def_id.to_def_id()).unwrap().expect_local();


The logic in this function was slightly changed to walk DefId parents instead of HirId parents, but the end result should be the same, I think.

IIUC, this loop is only accessed when macro_module_def_id point to a module that can be named from the outside. As a consequence, all its parents are modules. This is relied upon by update_macro_reachable. All modules are HIR owners, so walking parents by def_key or through HIR is equivalent.
Do you agree with the explanation? Should there be a comment?
Can the loop be simplified using tcx.hir().parent_owner_iter()?

Added a comment.
parent_owner_iter doesn't seem to make things simpler because it requires converting to HirIds and back.

petrochenkov · 2021-07-28T21:13:26Z

@bors try @rust-timer queue

rust-timer · 2021-07-28T21:13:27Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-07-28T21:13:36Z

⌛ Trying commit 901e8567836f70407d55e924ecae35c822743b76 with merge 258b818d64e93a02410b9a5a79c00e11788dfdf6...

petrochenkov · 2021-07-28T22:33:29Z

@bors try @rust-timer queue

rust-timer · 2021-07-28T22:33:30Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-07-28T22:33:38Z

⌛ Trying commit b639c36b9dad866d247c53f0053cb2105c84f27c with merge 0b23956c3baae739f7b14e136e301972fff31032...

bors · 2021-07-28T23:20:14Z

☀️ Try build successful - checks-actions
Build commit: 0b23956c3baae739f7b14e136e301972fff31032 (0b23956c3baae739f7b14e136e301972fff31032)

rust-timer · 2021-07-28T23:20:16Z

Queued 0b23956c3baae739f7b14e136e301972fff31032 with parent b708886, future comparison URL.

rust-timer · 2021-07-29T01:27:25Z

Finished benchmarking try commit (0b23956c3baae739f7b14e136e301972fff31032): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

petrochenkov · 2021-07-29T08:27:04Z

@bors rollup=maybe

cjgillot

LGTM. I left a few questions.

compiler/rustc_passes/src/dead.rs

compiler/rustc_privacy/src/lib.rs

cjgillot · 2021-07-30T15:54:03Z

compiler/rustc_privacy/src/lib.rs

                break;
            }
-            module_id = self.tcx.hir().get_parent_node(module_id);
+            module_def_id =
+                ty::DefIdTree::parent(self.tcx, module_def_id.to_def_id()).unwrap().expect_local();


IIUC, this loop is only accessed when macro_module_def_id point to a module that can be named from the outside. As a consequence, all its parents are modules. This is relied upon by update_macro_reachable. All modules are HIR owners, so walking parents by def_key or through HIR is equivalent.
Do you agree with the explanation? Should there be a comment?
Can the loop be simplified using tcx.hir().parent_owner_iter()?

cjgillot · 2021-07-30T15:58:42Z

compiler/rustc_middle/src/middle/privacy.rs

-#[derive(Clone)]
-pub struct AccessLevels<Id = HirId> {
+#[derive(Debug)]
+pub struct AccessLevels<Id = LocalDefId> {


Is this used somewhere with another Id?

Yes, rustdoc uses AccessLevels<DefId> and actually adds foreign ids to the table as public or exported, I didn't investigate why exactly.

IIRC it's because rustdoc needs to do its own privacy pass because rustc's is incorrect (#64762). It may be possible to remove when that's fixed, but don't quote me on that.

and passes using them - primarily privacy checking, stability checking and dead code checking. WIP

petrochenkov · 2021-07-31T17:43:25Z

Updated.

cjgillot · 2021-08-01T21:06:16Z

@bors r+

bors · 2021-08-01T21:06:17Z

📌 Commit f921410 has been approved by cjgillot

bors · 2021-08-02T10:44:55Z

⌛ Testing commit f921410 with merge 8f0efc93fd987665366d79b755a92641832ed0d1...

rust-log-analyzer · 2021-08-02T10:49:24Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2021-08-02T10:50:11Z

💔 Test failed - checks-actions

jyn514 · 2021-08-02T13:03:11Z

fatal: unable to access 'https://github.com/rust-lang-ci/rust/': Failed to connect to github.com port 443: Connection timed out

@bors retry

bors · 2021-08-04T02:04:07Z

⌛ Testing commit f921410 with merge bb744e1...

bors · 2021-08-04T04:27:28Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing bb744e1 to master...

rustc: Replace `HirId`s with `LocalDefId`s in `AccessLevels` tables and passes using those tables - primarily privacy checking, stability checking and dead code checking. All these passes work with definitions rather than with arbitrary HIR nodes. r? `@cjgillot` cc `@lambinoo` (rust-lang#87487)

rustc_privacy: Replace `HirId`s and `DefId`s with `LocalDefId`s where possible Follow up to rust-lang#87568

rust-highfive assigned cjgillot Jul 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 28, 2021

petrochenkov commented Jul 28, 2021

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 28, 2021

This comment has been minimized.

Sign in to view

bors 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 Jul 28, 2021

jyn514 approved these changes Jul 28, 2021

View reviewed changes

petrochenkov force-pushed the localevel branch from 901e856 to b639c36 Compare July 28, 2021 22:32

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 29, 2021

petrochenkov removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 29, 2021

cjgillot approved these changes Jul 30, 2021

View reviewed changes

petrochenkov 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 Jul 30, 2021

petrochenkov added 2 commits July 31, 2021 19:31

rustc: Replace HirIds with LocalDefIds in AccessLevels tables

b08576b

and passes using them - primarily privacy checking, stability checking and dead code checking. WIP

Fix clippy

f921410

petrochenkov force-pushed the localevel branch from b639c36 to f921410 Compare July 31, 2021 17:42

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

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 2, 2021

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 2, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 4, 2021

bors merged commit bb744e1 into rust-lang:master Aug 4, 2021

rustbot added this to the 1.56.0 milestone Aug 4, 2021

bors mentioned this pull request Aug 4, 2021

Remove Session.used_attrs and move logic to CheckAttrVisitor #87739

Merged

petrochenkov mentioned this pull request Aug 14, 2021

rustc_privacy: Replace HirIds and DefIds with LocalDefIds where possible #88034

Merged

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 19, 2021

Rollup merge of rust-lang#88034 - petrochenkov:localevel2, r=wesleywiser

8b9a4c3

rustc_privacy: Replace `HirId`s and `DefId`s with `LocalDefId`s where possible Follow up to rust-lang#87568

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Replace `HirId`s with `LocalDefId`s in `AccessLevels` tables #87568

rustc: Replace `HirId`s with `LocalDefId`s in `AccessLevels` tables #87568

petrochenkov commented Jul 28, 2021 •

edited

Loading

rust-highfive commented Jul 28, 2021

petrochenkov Jul 28, 2021

cjgillot Jul 30, 2021

petrochenkov Jul 31, 2021

petrochenkov commented Jul 28, 2021

rust-timer commented Jul 28, 2021

bors commented Jul 28, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Jul 28, 2021

rust-timer commented Jul 28, 2021

bors commented Jul 28, 2021

bors commented Jul 28, 2021

rust-timer commented Jul 28, 2021

rust-timer commented Jul 29, 2021

petrochenkov commented Jul 29, 2021

cjgillot left a comment

cjgillot Jul 30, 2021

cjgillot Jul 30, 2021

petrochenkov Jul 31, 2021

jyn514 Jul 31, 2021 •

edited

Loading

petrochenkov commented Jul 31, 2021

cjgillot commented Aug 1, 2021

bors commented Aug 1, 2021

bors commented Aug 2, 2021

rust-log-analyzer commented Aug 2, 2021

bors commented Aug 2, 2021

jyn514 commented Aug 2, 2021

bors commented Aug 4, 2021

bors commented Aug 4, 2021

rustc: Replace HirIds with LocalDefIds in AccessLevels tables #87568

rustc: Replace HirIds with LocalDefIds in AccessLevels tables #87568

Conversation

petrochenkov commented Jul 28, 2021 • edited Loading

rust-highfive commented Jul 28, 2021

petrochenkov Jul 28, 2021

Choose a reason for hiding this comment

cjgillot Jul 30, 2021

Choose a reason for hiding this comment

petrochenkov Jul 31, 2021

Choose a reason for hiding this comment

petrochenkov commented Jul 28, 2021

rust-timer commented Jul 28, 2021

bors commented Jul 28, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Jul 28, 2021

rust-timer commented Jul 28, 2021

bors commented Jul 28, 2021

bors commented Jul 28, 2021

rust-timer commented Jul 28, 2021

rust-timer commented Jul 29, 2021

petrochenkov commented Jul 29, 2021

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot Jul 30, 2021

Choose a reason for hiding this comment

cjgillot Jul 30, 2021

Choose a reason for hiding this comment

petrochenkov Jul 31, 2021

Choose a reason for hiding this comment

jyn514 Jul 31, 2021 • edited Loading

Choose a reason for hiding this comment

petrochenkov commented Jul 31, 2021

cjgillot commented Aug 1, 2021

bors commented Aug 1, 2021

bors commented Aug 2, 2021

rust-log-analyzer commented Aug 2, 2021

bors commented Aug 2, 2021

jyn514 commented Aug 2, 2021

bors commented Aug 4, 2021

bors commented Aug 4, 2021

rustc: Replace `HirId`s with `LocalDefId`s in `AccessLevels` tables #87568

rustc: Replace `HirId`s with `LocalDefId`s in `AccessLevels` tables #87568

petrochenkov commented Jul 28, 2021 •

edited

Loading

jyn514 Jul 31, 2021 •

edited

Loading