-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
WIP: Lint unused features #152164
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
base: main
Are you sure you want to change the base?
WIP: Lint unused features #152164
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is neat! in the future, it would be easier to review if you split the rename into a separate commit from the change in behavior.
could you add some tests for this please?
cb9b84c to
cd83e12
Compare
|
Initial implementation completed. I plan to add more tests and check whether features in rustc are reported as unused correctly or not. |
This comment has been minimized.
This comment has been minimized.
cd83e12 to
2945856
Compare
This comment has been minimized.
This comment has been minimized.
2945856 to
36c0a20
Compare
This comment has been minimized.
This comment has been minimized.
36c0a20 to
2ddaf82
Compare
This comment has been minimized.
This comment has been minimized.
2ddaf82 to
fb08c95
Compare
This comment has been minimized.
This comment has been minimized.
fb08c95 to
38aefe4
Compare
This comment has been minimized.
This comment has been minimized.
38aefe4 to
0cda8d4
Compare
This comment has been minimized.
This comment has been minimized.
0cda8d4 to
2d2577f
Compare
This comment has been minimized.
This comment has been minimized.
2d2577f to
b67632e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a940d65 to
435090a
Compare
402258f to
2e19ff0
Compare
This comment has been minimized.
This comment has been minimized.
2e19ff0 to
dbc424d
Compare
This comment has been minimized.
This comment has been minimized.
| // `restricted_std` is declared to mark std as unstable for certain targets, | ||
| // but it has no direct call sites to record usage, so it should never lint. | ||
| && f.as_str() != "restricted_std" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is confusing to me — do you mean it's only checked downstream by a later invocation of the compiler? like, when std is no longer the local crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library feature is declared in
Lines 750 to 754 in f21b4c0
| // This is required to avoid an unstable error when `restricted-std` is not | |
| // enabled. The use of #![feature(restricted_std)] in rustc-std-workspace-std | |
| // is unconditional, so the unstable feature needs to be defined somewhere. | |
| #[unstable(feature = "restricted_std", issue = "none")] | |
| mod __restricted_std_workaround {} |
It is only on the private mod, so that it cannot be recorded as used in downstream crates, and will always be an unused feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'm not sure it's the best way to solve this. Because we can define such features arbitrarily. And as we know they will be always unused, maybe users who use such features should allow unused_features manually or export a pub thing like pub mod workaround {} to make them used? 🤔
dbc424d to
b7f5529
Compare
This comment has been minimized.
This comment has been minimized.
b7f5529 to
5800ec9
Compare
This comment has been minimized.
This comment has been minimized.
5800ec9 to
ca05c07
Compare
|
Nice, CI has passes (finally :)), although I allowed |
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| static USED_FEATURES: LazyLock<Mutex<FxHashSet<Symbol>>> = | ||
| LazyLock::new(|| Mutex::new(FxHashSet::default())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this in the Session instead to correctly handle multiple compiler sessions in the same process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how are you ensuring that it works correctly with incr comp? Something needs to either record the side effects of marking a feature as used or force the query that marks a feature as used to be rerun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this in the Session instead to correctly handle multiple compiler sessions in the same process?
Done. Thanks for the advice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how are you ensuring that it works correctly with incr comp?
The query tracked_features_query is marked as eval_always and no_hash (as you suggested in the deleted comment, I thought wrongly that just the eval_always could make the node always red), so that any other queries depend on it (tcx.features() in fact) will re-compute. And then, checking the features is enabled or not will mark it as used in this table.
And TrackedFeatures does not implement HashStable, queries that depend on it can only check the feature internally but cannot pass or return TrackedFeatures to cause a unexpected usage outside query system. Therefore, as long as no other parts of the code directly use TrackedFeatures, it should work correctly with incremental compilation.
84b5565 to
c189c97
Compare
This comment has been minimized.
This comment has been minimized.
| self.enabled_lang_features().hash_stable(hcx, hasher); | ||
| self.enabled_lib_features().hash_stable(hcx, hasher); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary with the no_hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been removed in the third commit. Let me squash these two commits.
c189c97 to
4ba23c6
Compare
Put USED_FEATURES in the Session and mark tracked_features_query no_hash
4ba23c6 to
acb3115
Compare
Fixes #44232
Fixes #151752