Skip to content

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