-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Store #[deprecated] attribute's since
value in parsed form
#117377
Conversation
r? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
}; | ||
|
||
// Assume deprecation is in effect if "since" field is missing | ||
// or if we can't determine the current Rust version. |
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.
The "or if we can't determine the current Rust version" part of this comment should have been removed by #117256.
compiler/rustc_attr/src/builtin.rs
Outdated
Some(DeprecatedSince::RustcVersion(version)) | ||
} else { | ||
sess.emit_err(session_diagnostics::InvalidSince { span: attr.span }); | ||
Some(DeprecatedSince::Symbol(since)) |
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.
I'm not convinced that reusing this variant for both purposes is useful. What about an Err variant?
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.
👍 I have added DeprecatedSince::Err
for this case
compiler/rustc_attr/src/builtin.rs
Outdated
} | ||
} else if is_rustc { | ||
sess.emit_err(session_diagnostics::MissingSince { span: attr.span }); | ||
continue; |
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.
Should we stop discarding the attribute on missing since?
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.
Yes, good catch. Fixed.
I chose to use DeprecatedSince::Err
here instead of DeprecatedSince::Unspecified
. I used Unspecified
for the non-feature(staged_api)
case where deprecation versions are optional, and DeprecatedSince::Err
for the feature(staged_api)
case where deprecation versions are mandatory and must be parsed as a valid version; whether missing or invalid, you get DeprecatedSince::Err
, and it tells downstream code that it can rely on an error already having been emitted. Let me know what you think.
@@ -126,36 +128,14 @@ pub fn report_unstable( | |||
/// Checks whether an item marked with `deprecated(since="X")` is currently | |||
/// deprecated (i.e., whether X is not greater than the current rustc version). | |||
pub fn deprecation_in_effect(depr: &Deprecation) -> bool { |
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 this be made an inherent method on Deprecation or DeprecatedSince?
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.
Done; added pub fn is_in_effect(&self) -> bool
on Deprecation
.
compiler/rustc_attr/src/builtin.rs
Outdated
/// already been emitted. In the former case, deprecation versions outside | ||
/// the standard library are allowed to be arbitrary strings, for better or | ||
/// worse. | ||
Symbol(Symbol), |
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 variant name is not very descriptive? Custom ? UserDefined ?
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.
Do you like NonStandard
? It indicates both that this is used outside the standard library and also that it can contain an arbitrary string.
@@ -381,7 +359,7 @@ impl<'tcx> TyCtxt<'tcx> { | |||
// With #![staged_api], we want to emit down the whole | |||
// hierarchy. | |||
let depr_attr = &depr_entry.attr; | |||
if !skip || depr_attr.is_since_rustc_version { | |||
if !skip || matches!(depr_attr.since, Some(DeprecatedSince::RustcVersion(_))) { |
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.
Do we need a is_since_rustc_version method?
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.
It's okay either way, I think. Here is what having a method looks like: 8b8906b.
It's not that widely useful because most places that look for DeprecatedSince::RustcVersion(version)
also want to do something with the version
. I found 2 places that didn't (the ones in that commit link). We could do fn get_since_rustc_version(&self) -> Option<RustcVersion>
which is more broadly applicable, and then instead of .is_since_rustc_version()
do .get_since_rustc_version().is_some()
, but at that point it's no longer better than pattern matching the usual ways.
compiler/rustc_attr/src/builtin.rs
Outdated
@@ -720,17 +721,37 @@ pub fn eval_condition( | |||
|
|||
#[derive(Copy, Debug, Encodable, Decodable, Clone, HashStable_Generic)] | |||
pub struct Deprecation { | |||
pub since: Option<Symbol>, | |||
pub since: Option<DeprecatedSince>, |
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.
Should we add a Missing variant and remove the Option?
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.
Missing
would make some things more complicated because then it no longer makes sense for DeprecatedSince
to implement Display
, which rustdoc currently relies on.
Rustdoc may already need a different approach upon introducing an Err
variant so I'll take a look.
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.
After introducing Err
, eliminating the Option
indeed seems like the right call. 👍
Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in src/librustdoc/clean/types.rs cc @camelid |
Thanks! |
☀️ Test successful - checks-actions |
Finished benchmarking commit (22b2712): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 633.93s -> 636.003s (0.33%) |
78: Automated pull from upstream `master` r=tshepang a=github-actions[bot] This PR pulls the following changes from the upstream repository: * rust-lang/rust#113970 * rust-lang/rust#117459 * rust-lang/rust#117451 * rust-lang/rust#117439 * rust-lang/rust#117417 * rust-lang/rust#117388 * rust-lang/rust#113241 * rust-lang/rust#117462 * rust-lang/rust#117450 * rust-lang/rust#117407 * rust-lang/rust#117444 * rust-lang/rust#117438 * rust-lang/rust#117421 * rust-lang/rust#117416 * rust-lang/rust#116712 * rust-lang/rust#116267 * rust-lang/rust#117377 * rust-lang/rust#117419 Co-authored-by: Alexis (Poliorcetics) Bourget <ab_github@poliorcetiq.eu> Co-authored-by: Esteban Küber <esteban@kuber.com.ar> Co-authored-by: David Tolnay <dtolnay@gmail.com> Co-authored-by: Celina G. Val <celinval@amazon.com> Co-authored-by: Michael Goulet <michael@errs.io> Co-authored-by: bors <bors@rust-lang.org> Co-authored-by: Camille GILLOT <gillot.camille@gmail.com> Co-authored-by: lcnr <rust@lcnr.de> Co-authored-by: Zalathar <Zalathar@users.noreply.github.com> Co-authored-by: Oli Scherer <git-spam-no-reply9815368754983@oli-obk.de>
This PR implements the first followup bullet listed in #117148 (comment).
We centralize error handling to the attribute parsing code in
compiler/rustc_attr/src/builtin.rs
, and thereby remove some awkward error codepaths from later phases of compilation that had to make sense of these #[deprecated] attributes, namelycompiler/rustc_passes/src/stability.rs
andcompiler/rustc_middle/src/middle/stability.rs
.