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

Fix cross-crate visibility of fictive variant constructors #59936

Merged
merged 2 commits into from
Apr 14, 2019

Conversation

petrochenkov
Copy link
Contributor

After merging #59376 I realized that the code in the decoder wasn't entirely correct - we "decoded" fictive variant constructors with their variant's visibility, which could be public, rather than demoted to pub(crate).

Fictive constructors are not directly usable in expression/patterns, but the effect still can be observed with imports.

r? @davidtwco

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2019
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Apr 13, 2019

The alternative is to not change visibilities of the fictive variant constructors from pub to pub(crate) (consistently cross-crate and locally).
This way they will generate more errors, and that may be what we actually want, since the intent is to reserve those constructors and with more errors we are reserving them harder.

(Of course, what I'd personally like is to un-reserve them already and remove braced variants from value namespace, they've been reserved for years without any concrete plans of use.)

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This looks great, just one minor comment.

src/librustc_metadata/decoder.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor Author

Updated.

@davidtwco
Copy link
Member

Great, thanks for making those changes! r=me when Travis finishes.

@Centril
Copy link
Contributor

Centril commented Apr 14, 2019

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Apr 14, 2019

📌 Commit dbc7042 has been approved by davidtwco

@bors 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 Apr 14, 2019
Centril added a commit to Centril/rust that referenced this pull request Apr 14, 2019
Fix cross-crate visibility of fictive variant constructors

After merging rust-lang#59376 I realized that the code in the decoder wasn't entirely correct - we "decoded" fictive variant constructors with their variant's visibility, which could be public, rather than demoted to `pub(crate)`.

Fictive constructors are not directly usable in expression/patterns, but the effect still can be observed with imports.

r? @davidtwco
Centril added a commit to Centril/rust that referenced this pull request Apr 14, 2019
Fix cross-crate visibility of fictive variant constructors

After merging rust-lang#59376 I realized that the code in the decoder wasn't entirely correct - we "decoded" fictive variant constructors with their variant's visibility, which could be public, rather than demoted to `pub(crate)`.

Fictive constructors are not directly usable in expression/patterns, but the effect still can be observed with imports.

r? @davidtwco
bors added a commit that referenced this pull request Apr 14, 2019
Rollup of 7 pull requests

Successful merges:

 - #59856 (update polonius-engine)
 - #59877 (HirIdify hir::Def)
 - #59896 (Remove duplicated redundant spans)
 - #59900 (Remove [mut] syntax in pin docs)
 - #59906 (Make BufWriter use get_mut instead of manipulating inner in Write implementation)
 - #59936 (Fix cross-crate visibility of fictive variant constructors)
 - #59957 (Add missing backtick to Symbol documentation.)

Failed merges:

r? @ghost
@bors bors merged commit dbc7042 into rust-lang:master Apr 14, 2019
@bors
Copy link
Contributor

bors commented Apr 14, 2019

⌛ Testing commit dbc7042 with merge 9cd61f0...

@tesuji
Copy link
Contributor

tesuji commented Apr 14, 2019

Why is bor still testing after the PR is merged?
Edit: Same in #59856 (comment)

@Centril
Copy link
Contributor

Centril commented Apr 14, 2019

@lzutao bors isn't really testing, the message is a bug.

@petrochenkov petrochenkov deleted the confict branch June 5, 2019 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants