Fixes wrong unreachable_pub lints on nested and glob public reexport by lambinoo · Pull Request #87487 · rust-lang/rust

lambinoo · 2021-07-26T17:40:54Z

Linked issues: #64762 & #82064

rust-highfive · 2021-07-26T17:40:57Z

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

lambinoo · 2021-07-26T17:41:20Z

lambinoo · 2021-07-26T17:42:50Z

I've had one test weirdly failing locally in the debuginfo-gdb pass but I'm not too sure how it can be related to my change

petrochenkov · 2021-07-27T22:05:00Z

What I wanted to achieve with #82064 is assigning all Public and Exported levels in resolve, to all items, not only exports.
The EmbargoVisitor::access_levels table will be pre-initialized from the resolver table.
(After that we'll be able to add an assert to EmbargoVisitor::update making sure that only Reachable and ReachableFromImplTrait levels are inserted at that point.)

lambinoo · 2021-07-31T20:46:13Z

@petrochenkov Hey, good evening! I've been able to look at the necessary work during the week. Would that basically involve moving a big chunk of the code from the EmbargoVisitor in rustc_privacy to a new PrivacyVisitor in rustc_resolve?

I already have some local changes towards that goal but I prefer to ask before sinking too much time in the wrong direction

petrochenkov · 2021-07-31T21:07:47Z

Most of EmbargoVisitor is about Reachable and ReachableFromImplTrait so there's not much to move if you want to support Public and Exported only.
Only modules (in resolve sense, including enums and traits) need to be treated specially, all other items don't need to be visited in detail at all.

lambinoo · 2021-08-02T08:26:13Z

@petrochenkov Mmmh, I've noticed what I think is a major blocker.

As far as I know, I cannot correctly resolve the access level of Impl contained inside a Block from the Resolver. I need access to a TyCtxt for that.

rust/compiler/rustc_privacy/src/lib.rs

Lines 639 to 641 in 8759f00

    
           hir::ItemKind::Impl { .. } => { 
        
               Option::<AccessLevel>::of_impl(item.hir_id(), self.tcx, &self.access_levels) 
        
           }

https://github.com/rust-lang/rust/blob/86bd8913c953a34c8bd4fd59dbdd46620c5419a5/compiler/rustc_privacy/src/lib.rs#L357-L364

Current workarounds I found are:

being overly optimistic (and mark every pub items as Public) and rely on the types being private to throw errors
keeping a Public/Exported access level pass in rustc_privacy explicitely for Impl which is what I did
https://github.com/rust-lang/rust/blob/86bd8913c953a34c8bd4fd59dbdd46620c5419a5/compiler/rustc_privacy/src/lib.rs#L589-L598

My last push contains those changes (I still need to see how to handle macros so it's not finished yet)

petrochenkov · 2021-08-02T09:54:09Z

Ah, right, impls can also be public or exported.
You are right, you cannot correctly process impls early in resolve, the logic for impls will need to stay in rustc_privacy.

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)

petrochenkov · 2021-12-26T02:07:28Z

Perf is fine.
r=me after squashing commits.

lambinoo · 2021-12-26T06:40:42Z

I started to rework that weird way I get NameBindings but I think it's better if we get this through first before I need to rebase with master and fix new problems that could arise. I will make a new PR afterwards

@rustbot label -S-waiting-on-author +S-waiting-on-review

petrochenkov · 2021-12-26T13:26:28Z

-S-waiting-on-author +S-waiting-on-review

Nothing changed since the previous review?
This PR still needs green CI and commit squashing.

lambinoo · 2021-12-30T19:49:00Z

@petrochenkov Oh... I thought I squashed all those commits down.. my bad. Squashed, pushed!

Thanks a lot for your time as always!

calebcartwright · 2021-12-31T02:42:06Z

By the way you should be able to rebase now to pull in the rustfmt updates

petrochenkov · 2021-12-31T09:24:06Z

I cannot r+ this until the failing test on CI is fixed.
The test is a clippy test - src\tools\clippy\tests\ui\implicit_hasher.rs, implementation of the corresponding lint is in src\tools\clippy\clippy_lints\src\implicit_hasher.rs (looks like it indeed uses access_levels).

petrochenkov · 2021-12-31T09:24:48Z

Clippy tests can be run with ./x.py test --bless --stage 2 src/tools/clippy.

Mak DefId to AccessLevel map in resolve for export hir_id to accesslevel in resolve and applied in privacy using local def id removing tracing probes making function not recursive and adding comments Move most of Exported/Public res to rustc_resolve moving public/export res to resolve fix missing stability attributes in core, std and alloc move code to access_levels.rs return for some kinds instead of going through them Export correctness, macro changes, comments add comment for import binding add comment for import binding renmae to access level visitor, remove comments, move fn as closure, remove new_key fmt fix rebase fix rebase fmt fmt fix: move macro def to rustc_resolve fix: reachable AccessLevel for enum variants fmt fix: missing stability attributes for other architectures allow unreachable pub in rustfmt fix: missing impl access level + renaming export to reexport Missing impl access level was found thanks to a test in clippy

petrochenkov · 2022-01-10T00:34:46Z

@lambinoo
Has something changed?
I see that the clippy errors on CI are gone, but I don't see any changes in clippy.

lambinoo · 2022-01-10T05:32:34Z

I noticed a small mistake in the rustc_pricacy code. A line disappeared when I shuffled some code around.

I forgot to update the access level of the impl in visit_item (L650).

I found this in the most overkill way by having the compiler dump the access level structure and doing a diff between this branch and master... And a lot of impl that should have been public were missing.

petrochenkov · 2022-01-10T06:39:01Z

Great, hopefully it will pass now.
@bors r+

bors · 2022-01-10T06:39:02Z

📌 Commit 3a77bb8 has been approved by petrochenkov

bors · 2022-01-10T08:55:55Z

⌛ Testing commit 3a77bb8 with merge df035a3...

bors · 2022-01-10T11:53:09Z

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

rust-timer · 2022-01-10T13:24:29Z

Finished benchmarking commit (df035a3): comparison url.

Summary: This change led to moderate relevant improvements 🎉 in compiler performance.

Moderate improvement in instruction counts (up to -1.1% on incr-unchanged builds of unused-warnings)

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

@rustbot label: -perf-regression

lambinoo · 2022-01-10T13:27:24Z

Success! Gosh this one has been cooking for a while. I'll be working on improving the whole resolution thing now but I think the two linked issues can be closed now!
I wanted to thank you @petrochenkov (again x3) for the time you gave me for this PR and for your patience!

rust-highfive assigned nagisa Jul 26, 2021

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

rust-highfive assigned petrochenkov and unassigned nagisa Jul 26, 2021

This comment has been minimized.

Sign in to view

lambinoo changed the title ~~Unreachable pub on nested uses~~ Fixes false positive unreachable_pub lints on nested and glob public reexport Jul 26, 2021

lambinoo changed the title ~~Fixes false positive unreachable_pub lints on nested and glob public reexport~~ Fixes wrong unreachable_pub lints on nested and glob public reexport Jul 26, 2021

lambinoo marked this pull request as ready for review July 27, 2021 13:57

lambinoo force-pushed the I-64762_unreachable_pub_lint branch from 32d0081 to 3c5d0e8 Compare July 27, 2021 14:17

petrochenkov mentioned this pull request Jul 28, 2021

rustc: Replace HirIds with LocalDefIds in AccessLevels tables #87568

Merged

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 29, 2021

lambinoo force-pushed the I-64762_unreachable_pub_lint branch from 3c5d0e8 to 32eaff5 Compare August 2, 2021 08:26

lambinoo force-pushed the I-64762_unreachable_pub_lint branch from 32eaff5 to 86bd891 Compare August 2, 2021 08:27

This comment has been minimized.

Sign in to view

lambinoo force-pushed the I-64762_unreachable_pub_lint branch from 86bd891 to b888dc9 Compare August 4, 2021 15:49

This comment has been minimized.

Sign in to view

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 Dec 25, 2021

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 Dec 26, 2021

lambinoo force-pushed the I-64762_unreachable_pub_lint branch from 9fd1731 to d4a21fe Compare December 26, 2021 06:38

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 26, 2021

This comment has been minimized.

Sign in to view

This was referenced Jan 10, 2022

Move collection of items accessible directly or through reexports from rustc_privacy to rustc_resolve #82064

Closed

Lint unreachable_pub got easily confused by multiple items in a single pub use ... statement #64762

Closed

ehuss mentioned this pull request Jan 11, 2022

no MIR available for DefId #92755

Closed

petrochenkov mentioned this pull request Jan 12, 2022

Set struct/union/enum fields/variants as reachable when item is #92781

Merged

Bryanskiy mentioned this pull request Aug 4, 2022

optimization of access level table construction #100147

Merged

Uh oh!

Conversation

lambinoo commented Jul 26, 2021

Uh oh!

rust-highfive commented Jul 26, 2021

Uh oh!

lambinoo commented Jul 26, 2021

Uh oh!

lambinoo commented Jul 26, 2021

Uh oh!

This comment has been minimized.

petrochenkov commented Jul 27, 2021

Uh oh!

lambinoo commented Jul 31, 2021

Uh oh!

petrochenkov commented Jul 31, 2021

Uh oh!

lambinoo commented Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

petrochenkov commented Aug 2, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Dec 26, 2021

Uh oh!

lambinoo commented Dec 26, 2021

Uh oh!

This comment has been minimized.

petrochenkov commented Dec 26, 2021

Uh oh!

lambinoo commented Dec 30, 2021

Uh oh!

This comment has been minimized.

calebcartwright commented Dec 31, 2021

Uh oh!

petrochenkov commented Dec 31, 2021

Uh oh!

petrochenkov commented Dec 31, 2021

Uh oh!

This comment has been minimized.

petrochenkov commented Jan 10, 2022

Uh oh!

lambinoo commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Jan 10, 2022

Uh oh!

bors commented Jan 10, 2022

Uh oh!

bors commented Jan 10, 2022

Uh oh!

bors commented Jan 10, 2022

Uh oh!

rust-timer commented Jan 10, 2022

Uh oh!

lambinoo commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

lambinoo commented Aug 2, 2021 •

edited

Loading

lambinoo commented Jan 10, 2022 •

edited

Loading

lambinoo commented Jan 10, 2022 •

edited

Loading