Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make tcx.visibility() work for items in the local crate #77889

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 13, 2020

This also changes rustc_metadata to use tcx.visibility() for record!ing metadata.

See #77820 (comment) for the history.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2020
@jyn514
Copy link
Member Author

jyn514 commented Oct 13, 2020

The important commit is 3a88f79, the first one just switches lots of places from ty::Visibility::from_hir(def_id) to tcx.visibility(def_id). I recommend reading it with whitespace changes hidden. The current compile error is

error[E0004]: non-exhaustive patterns: `Ty(_)`, `TraitRef(_)`, `Binding(_)` and 8 more not covered
   --> compiler/rustc_privacy/src/lib.rs:254:25
    |
254 |     let hir_vis = match tcx.hir().get(hir_id) {
    |                         ^^^^^^^^^^^^^^^^^^^^^ patterns `Ty(_)`, `TraitRef(_)`, `Binding(_)` and 8 more not covered

I don't know how to handle these.

@jyn514 jyn514 added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-visibility Area: Visibility / privacy. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2020
@petrochenkov
Copy link
Contributor

@jyn514
Every record!(self.tables.visibility[...] <- RIGHT_HAND); in encoder.rs should use tcx.visibility(def_id) on the right hand side.
The body of fn visibility should be equivalent to those right hand sides combined.
That's what I previously mentioned in #77820 (comment) and #77820 (comment).

Touching rustc_privacy::def_id_visibility is not a goal right now, it may do different things from tcx.visibility(def_id) and I don't remember the exact details, I'll audit it later.
(If some pieces of rustc_privacy::def_id_visibility are obviously equivalent to those from tcx.visibility(def_id) based on encoding.rs, then tcx.visibility(def_id) can be used in implementation of rustc_privacy::def_id_visibility, but it's not a goal to replace the whole rustc_privacy::def_id_visibility with tcx.visibility(def_id).)

@petrochenkov 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 Oct 13, 2020
@jyn514 jyn514 changed the title [WIP] make tcx.visibility() work for items in the local crate Make tcx.visibility() work for items in the local crate Oct 15, 2020
@jyn514 jyn514 marked this pull request as ready for review October 15, 2020 03:16
@jyn514
Copy link
Member Author

jyn514 commented Oct 15, 2020

I think I did it right this time :) It passes src/test/ui at least.

This also changes rustc_metadata to use `tcx.visibility()` for `record!`ing metadata.
@jyn514 jyn514 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 Oct 15, 2020
@jyn514
Copy link
Member Author

jyn514 commented Oct 15, 2020

Let me know if I should switch more places to using tcx.visibility() instead of calculating it themselves, I've only done rustc_metadata so far.

@petrochenkov
Copy link
Contributor

So I ended up fixing this properly and deduplicating visibility calculations in all three places in which they are performed - resolve (which can't use queries), metadata encoding, and privacy.

I'm still debugging, but if I finish that work today or at least tomorrow, I'll close this PR in favor of it.

@petrochenkov
Copy link
Contributor

Closing in favor of #78077.

@jyn514 jyn514 deleted the local-visibility branch October 18, 2020 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-visibility Area: Visibility / privacy. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants