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

Suggest removing #![feature] for library features that have been stabilized #89012

Merged
merged 1 commit into from
Sep 17, 2021

Conversation

vishadGoyal
Copy link
Contributor

@vishadGoyal vishadGoyal commented Sep 16, 2021

Issue: #88802
Delayed the check if #![feature] has been used to enable lib features in a non-nightly build to occur after TyCtxt has been constructed.

@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 @matthewjasper (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2021
@jyn514 jyn514 changed the title Issue 88802 fix Suggest removing #![feature] for library features that have been stabilized Sep 16, 2021
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Isn't the point to suggest removing the feature? This looks like you've just duplicated the check later into the compiler.

use rustc_errors::Applicability;

if !sess.opts.unstable_features.is_nightly_build() {
let lang_features = &sess.features_untracked().declared_lang_features;
if lang_features.len() == 0 {
return;
Copy link
Member

Choose a reason for hiding this comment

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

What is this check doing? It should never be hit since there will always be some features built-in, but it also doesn't seem to have anything to do with the rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lang_features vector contains the declared_lang_features in the session. So only the ones which have been enabled show up here.
I've added this check to skip emitting an error if only lib_features have been enabled and no lang_features

Copy link
Member

Choose a reason for hiding this comment

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

Ok. That still seems like the wrong check, though - if there's one language and one library feature, it should still suggest removing the stable library feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How will that be possible since the suggestion can be generated only after TyCtxt is generated? And we will be throwing an error at the parsing stage if a lang feature has been enabled, the compiler will abort and won't go to the next stage

Copy link
Member

Choose a reason for hiding this comment

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

And we will be throwing an error at the parsing stage if a lang feature has been enabled, the compiler will abort and won't go to the next stage

Oh hmm, good point. Ok, this seems fine then - if the language feature wasn't stable removing the library feature wouldn't have helped anyway.

compiler/rustc_passes/src/stability.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 assigned jyn514 and unassigned matthewjasper Sep 16, 2021
@jyn514 jyn514 added A-stability Area: `#[stable]`, `#[unstable]` etc. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 16, 2021
@rust-log-analyzer

This comment has been minimized.

@vishadGoyal
Copy link
Contributor Author

Isn't the point to suggest removing the feature? This looks like you've just duplicated the check later into the compiler.

The warnings for unnecessary use of #![feature] are already emitted in this check. After my changes, it also emits an error if appropriate.
Behaviour:
Screenshot from 2021-09-16 19-56-33

@jyn514
Copy link
Member

jyn514 commented Sep 16, 2021

Ah! That's perfect then :) can you add that as a test case? It would go somewhere in src/test/ui - anywhere other than issues/ is probably fine.

@vishadGoyal
Copy link
Contributor Author

Ah! That's perfect then :) can you add that as a test case? It would go somewhere in src/test/ui - anywhere other than issues/ is probably fine.

@jyn514 Is there a way to specify which channel the test should be run on? I'm adding a sample code to reproduce this situation and adding the expected stderr output. However, this will be different for different channels (beta, nightly, etc.)

@jyn514
Copy link
Member

jyn514 commented Sep 16, 2021

@jyn514 Is there a way to specify which channel the test should be run on? I'm adding a sample code to reproduce this situation and adding the expected stderr output. However, this will be different for different channels (beta, nightly, etc.)

No, unfortunately, I forgot about that. If you do a manual test and post the output that's good enough.

@vishadGoyal
Copy link
Contributor Author

Manual test:
Screenshot from 2021-09-16 21-56-02

Is this good enough? @jyn514

@jyn514
Copy link
Member

jyn514 commented Sep 16, 2021

@vishadGoyal how does it behave when there's both a library feature and at least one language feature (e.g. #![feature(never_type_fallback)])?

@vishadGoyal
Copy link
Contributor Author

@vishadGoyal how does it behave when there's both a library feature and at least one language feature (e.g. #![feature(never_type_fallback)])?
Screenshot from 2021-09-16 22-20-42

I guess that's functional but not exactly what I had in mind.

@jyn514 jyn514 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 Sep 16, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 16, 2021

@bors r+ squash

Thanks!

@bors
Copy link
Contributor

bors commented Sep 16, 2021

📌 Commit b53c0eb242065377b989d77dba6a113926eb9ef5 has been approved by jyn514

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 16, 2021
If #![feature] is used outside the nightly channel for only lib
features, the check will be delayed to the stability pass after
parsing. This is done so that appropriate help messages can be shown if
the #![feature] has been used needlessly
@Mark-Simulacrum
Copy link
Member

@bors r=jyn514 squash-

Squashed the commits locally -- I don't think we've fixed that to behave well in bors just yet.

@bors
Copy link
Contributor

bors commented Sep 16, 2021

📌 Commit 9f7e281 has been approved by jyn514

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2021
…laumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#86422 (Emit clearer diagnostics for parens around `for` loop heads)
 - rust-lang#87460 (Point to closure when emitting 'cannot move out' for captured variable)
 - rust-lang#87566 (Recover invalid assoc type bounds using `==`)
 - rust-lang#88666 (Improve build command for compiler docs)
 - rust-lang#88899 (Do not issue E0071 if a type error has already been reported)
 - rust-lang#88949 (Fix handling of `hir::GenericArg::Infer` in `wrong_number_of_generic_args.rs`)
 - rust-lang#88953 (Add chown functions to std::os::unix::fs to change the owner and group of files)
 - rust-lang#88954 (Allow `panic!("{}", computed_str)` in const fn.)
 - rust-lang#88964 (Add rustdoc version into the help popup)
 - rust-lang#89012 (Suggest removing `#![feature]` for library features that have been stabilized)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 101a88f into rust-lang:master Sep 17, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. 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
Development

Successfully merging this pull request may close these issues.

8 participants