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.

Sign up for GitHub

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

Jump to bottom

Allow unstable items to be re-exported unstably without requiring the feature be enabled #97301

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Allow unstable items to be re-exported unstably without requiring the feature be enabled #97301

merged 1 commit into from
Jun 7, 2022

Conversation

semicoleon
Copy link
Contributor

Closes #94972

The diagnostic may need some work still, and I haven't added a test yet

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 23, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2022
@lcnr
Copy link
Contributor

lcnr commented May 23, 2022 •
edited
Loading

don't trust myself to review anything loosening our stability checks

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned lcnr May 23, 2022
petrochenkov
petrochenkov reviewed May 23, 2022
View reviewed changes
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

This also needs a test.

@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 May 23, 2022
@semicoleon
Copy link
Contributor Author

semicoleon commented May 29, 2022 •
edited
Loading

When I was playing around with the test, I noticed that I could get master to pass a version of the test that looks like it should probably fail. I pushed that version of the test here master...semicoleon:reexport-feature-ignored

I think we're just not checking the stability attributes on re-exports at all. So if you enable the feature the actual resolved item requires, any #[unstable(...)] attributes on re-exports between the resolved item and the call site don't produce an error the way I'd expect it to.

I'm not sure that's particularly relevant to this PR, or whether it's really worth worrying about, but I figured I'd mention it

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@semicoleon
Copy link
Contributor Author

I went ahead and implemented a check for "public" visibility since the test failures were all on non-public uses. I also moved the check for not emitting unstable errors inside eval_stability so the deprecation warnings would still run in all cases.

@semicoleon
Copy link
Contributor Author

That last test failure is a re-export, so we might need to change that test if this version of the implementation is what we want to go with

This is the test that's failing

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov petrochenkov 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 May 30, 2022
@petrochenkov
Copy link
Contributor

so the deprecation warnings would still run in all cases

Yeah, that's probably better, at least for a start.
If people are not asking to skip deprecation messages in cases like #[unstable] pub use something_deprecated;, then let's keep them.
Ping @m-ou-se just in case.

@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 Jun 2, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@semicoleon
Copy link
Contributor Author

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

@rustbot rustbot 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 Jun 3, 2022
petrochenkov
petrochenkov reviewed Jun 4, 2022
View reviewed changes
compiler/rustc_middle/src/middle/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
compiler/rustc_passes/src/stability.rs Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

LGTM, could you also squash commits after addressing the remaining comments?
@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2022
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 4, 2022
@semicoleon
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Jun 4, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 5, 2022

📌 Commit f3d93b6 has been approved by petrochenkov

@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 Jun 5, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 7, 2022
@Dylan-DPC
Rollup merge of rust-lang#97301 - semicoleon:unstable-reexport, r=pet…
4d14dc3 
…rochenkov

Allow unstable items to be re-exported unstably without requiring the feature be enabled

Closes rust-lang#94972

The diagnostic may need some work still, and I haven't added a test yet
This was referenced Jun 7, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2022
@bors
Auto merge of rust-lang#97825 - Dylan-DPC:rollup-ya51k1k, r=Dylan-DPC
7fe2c4b 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97058 (Various refactors to the incr comp workproduct handling)
 - rust-lang#97301 (Allow unstable items to be re-exported unstably without requiring the feature be enabled)
 - rust-lang#97738 (Fix ICEs from zsts within unsized types with non-zero offsets)
 - rust-lang#97771 (Remove SIGIO reference on Haiku)
 - rust-lang#97808 (Add some unstable target features for the wasm target codegen)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2035b50 into rust-lang:master Jun 7, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 7, 2022
JohnTitor pushed a commit to JohnTitor/rust that referenced this pull request Jul 26, 2022
@bors
Auto merge of rust-lang#97825 - Dylan-DPC:rollup-ya51k1k, r=Dylan-DPC
b867d41 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97058 (Various refactors to the incr comp workproduct handling)
 - rust-lang#97301 (Allow unstable items to be re-exported unstably without requiring the feature be enabled)
 - rust-lang#97738 (Fix ICEs from zsts within unsized types with non-zero offsets)
 - rust-lang#97771 (Remove SIGIO reference on Haiku)
 - rust-lang#97808 (Add some unstable target features for the wasm target codegen)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@RalfJung
Copy link
Member

This seems to be the likely cause of a subtle bug, but more investigation is definitely needed: #114838.

@RalfJung
Copy link
Member

Ah: this patch also kicks in when -Zforce-unstable-if-unmarked is enabled. That's not intentional, is it?

@semicoleon
Copy link
Contributor Author

I don't believe so, no

@RalfJung
Copy link
Member

The problem is that it kicks in for every unstable reexport -- whether it has an explicit #[unstable] or whether it is implicitly unstable due to the -Z flag.

Maybe it should be gated on "the item is unstable and the crate explicitly enabled staged_api?

@petrochenkov
Copy link
Contributor

For the purpose of fixing #94972, I think it would be enough if AllowUnstable::Yes was enabled only for reexports with explicit unstable attributes.

@RalfJung
Copy link
Member

Or that, yes. (Then in std it wouldn't kick in when unstable is inherited from the module.)

@RalfJung RalfJung mentioned this pull request Aug 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbstj fbstj fbstj left review comments

@petrochenkov petrochenkov petrochenkov left review comments

@lcnr lcnr lcnr left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

Allow std to re-export unstable things from core without having to enable the feature itself
9 participants
@semicoleon @rust-highfive @lcnr @petrochenkov @rust-log-analyzer @bors @RalfJung @fbstj @rustbot