Remove visibility information from HIR by cjgillot · Pull Request #93970 · rust-lang/rust

cjgillot · 2022-02-13T18:53:32Z

The resolver exports all the necessary visibility information through the tcx.visibility query.
This PR stops having a dedicated visibility field in HIR, in order to use this query.
We keep a vis_span field for diagnostic purposes.

rust-highfive · 2022-02-13T18:53:35Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2022-02-13T18:53:36Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

lcnr · 2022-02-14T06:44:12Z

r? @petrochenkov

src/tools/clippy/clippy_lints/src/utils/inspector.rs

compiler/rustc_lint/src/builtin.rs

compiler/rustc_passes/src/dead.rs

compiler/rustc_privacy/src/lib.rs

petrochenkov · 2022-02-14T10:49:49Z

failures:
    [ui] ui/error-codes/E0431.rs
    [ui] ui/save-analysis/issue-73020.rs
    [ui] ui/use/use-mod.rs

Apparently some imports still miss their visibility entries in the table.

petrochenkov · 2022-02-15T05:51:27Z

compiler/rustc_lint/src/builtin.rs

Looks like this whole arm can be removed now (including this comment).

Wouldn't removing this arm will change behaviour?

Maybe, but it still looks like a reasonable change.
It can be done in a separate PR.

compiler/rustc_passes/src/dead.rs

petrochenkov · 2022-02-15T05:55:14Z

r=me after addressing the remaining comments and squashing commits.

cjgillot · 2022-02-17T17:06:08Z

This PR introduces untracked information in metadata. As visibility is not in the HIR any more, it's not in the crate_hash, and will create incr comp ICEs in downstream crates. I'll add an extra commit to adress this.

cjgillot · 2022-02-18T21:44:10Z

@bors try @rust-timer queue

rust-timer · 2022-02-18T21:44:12Z

Awaiting bors try build completion.

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

bors · 2022-02-18T21:44:19Z

⌛ Trying commit 1d2707f49db710409cb70bf7cedda3bfc7e28fa7 with merge 5a9e9f6c9170cf730f258de9ecbaac918bf4a913...

bors · 2022-02-18T23:18:57Z

☀️ Try build successful - checks-actions
Build commit: 5a9e9f6c9170cf730f258de9ecbaac918bf4a913 (5a9e9f6c9170cf730f258de9ecbaac918bf4a913)

cjgillot · 2022-04-23T14:07:02Z

This version hashes visibility information into the crate hash, so should be sound in terms of metadata, unblocking.
The slowdown is mostly in "unused-warnings", which is a stress benchmark, in crate_hash query, which is expected.
Commit b272468 fixes the failure of image-0.24.1 benchmark.

petrochenkov · 2022-04-23T16:26:13Z

compiler/rustc_middle/src/ty/mod.rs

Shouldn't it be included into the crate hash too?

It is only used to emit a diagnostic, which won't appear in metadata. Therefore, it does not need to appear in crate_hash. I can include it as a safety precaution.

petrochenkov · 2022-04-23T16:33:15Z

r=me after addressing #93970 (comment) and squashing commits.

cjgillot · 2022-04-23T22:07:53Z

@bors r=petrochenkov

bors · 2022-04-23T22:07:55Z

📌 Commit 0a6e135 has been approved by petrochenkov

bors · 2022-04-23T22:25:54Z

⌛ Testing commit 0a6e135 with merge 143eaa8...

bors · 2022-04-24T01:09:06Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 143eaa8 to master...

rust-timer · 2022-04-24T02:24:32Z

Finished benchmarking commit (143eaa8): comparison url.

Summary:

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 😿 relevant regressions found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	16	2	2	2
mean²	N/A	1.3%	-0.2%	-0.3%	-0.2%
max	N/A	2.2%	-0.2%	-0.3%	-0.2%

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

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

cjgillot · 2022-04-24T07:21:23Z

This PR removes redundant information from the HIR to directly use the resolver outputs. The regression only appears on usused-warnings, which is a stress test. The regression is concentrated on the crate_hash query, which now hashes visibility. This increased work in crate_hash is arguably a bugfix, since visibilities were not being hashed.
@rustbot label: +perf-regression-triaged

…:FnKind` are gone <rust-lang/rust#93970>

rust-highfive assigned lcnr Feb 13, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2022

cjgillot force-pushed the novis branch from ae552b7 to 9c67215 Compare February 13, 2022 18:57

This comment has been minimized.

Sign in to view

petrochenkov self-assigned this Feb 14, 2022

lcnr removed their assignment Feb 14, 2022

cjgillot force-pushed the novis branch from 9c67215 to d950d1a Compare February 14, 2022 07:28

petrochenkov reviewed Feb 14, 2022

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 Feb 14, 2022

This comment has been minimized.

Sign in to view

cjgillot 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 Feb 14, 2022

petrochenkov reviewed Feb 15, 2022

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 Feb 15, 2022

cjgillot force-pushed the novis branch from 0411db8 to 1d2707f Compare February 18, 2022 21:42

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 18, 2022

cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 23, 2022

petrochenkov reviewed Apr 23, 2022

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 Apr 23, 2022

Fix lints.

423a712

cjgillot force-pushed the novis branch from b272468 to 5751752 Compare April 23, 2022 21:03

cjgillot added 4 commits April 23, 2022 23:03

Make clippy inspector more precise.

88de3e5

Compute has_pub_restricted in the resolver.

9861bc8

Hash visibilities in crate_hash.

c3276c6

Bless pretty-print output.

0a6e135

cjgillot force-pushed the novis branch from 5751752 to 0a6e135 Compare April 23, 2022 21:03

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 Apr 23, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 24, 2022

bors merged commit 143eaa8 into rust-lang:master Apr 24, 2022

rustbot added this to the 1.62.0 milestone Apr 24, 2022

bors mentioned this pull request Apr 24, 2022

Warn about dead tuple struct fields #95977

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 24, 2022

cjgillot deleted the novis branch April 24, 2022 07:21

matthiaskrgr mentioned this pull request Apr 24, 2022

ice: broken mir: end of phase transition to Optimization compiler/rustc_const_eval/src/transform/validate.rs:119:36 #92495

Open

yvt added a commit to yvt/servo that referenced this pull request Oct 16, 2022

fix(script_plugins): the visibility fields of `rustc_hir::intravisit:…

b4c1c97

…:FnKind` are gone <rust-lang/rust#93970>

Uh oh!

Conversation

cjgillot commented Feb 13, 2022

Uh oh!

rust-highfive commented Feb 13, 2022

Uh oh!

rust-highfive commented Feb 13, 2022

Uh oh!

This comment has been minimized.

lcnr commented Feb 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Feb 14, 2022

Uh oh!

This comment has been minimized.

petrochenkov Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

cjgillot Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

petrochenkov Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrochenkov commented Feb 15, 2022

Uh oh!

cjgillot commented Feb 17, 2022

Uh oh!

cjgillot commented Feb 18, 2022

Uh oh!

rust-timer commented Feb 18, 2022

Uh oh!

bors commented Feb 18, 2022

Uh oh!

bors commented Feb 18, 2022

Uh oh!

cjgillot commented Apr 23, 2022

Uh oh!

petrochenkov Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

cjgillot Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Apr 23, 2022

Uh oh!

cjgillot commented Apr 23, 2022

Uh oh!

bors commented Apr 23, 2022

Uh oh!

bors commented Apr 23, 2022

Uh oh!

bors commented Apr 24, 2022

Uh oh!

rust-timer commented Apr 24, 2022

Footnotes

Uh oh!

cjgillot commented Apr 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants