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

Store #[deprecated] attribute's since value in parsed form #117377

Merged
merged 10 commits into from
Oct 31, 2023
Merged
73 changes: 51 additions & 22 deletions compiler/rustc_attr/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use rustc_session::parse::{feature_err, ParseSess};
use rustc_session::{RustcVersion, Session};
use rustc_span::hygiene::Transparency;
use rustc_span::{symbol::sym, symbol::Symbol, Span};
use std::fmt::{self, Display};
use std::num::NonZeroU32;

use crate::session_diagnostics::{self, IncorrectReprFormatGenericCause};
Expand Down Expand Up @@ -139,7 +140,7 @@ pub enum StabilityLevel {
/// `#[stable]`
Stable {
/// Rust release which stabilized this feature.
since: Since,
since: StableSince,
/// Is this item allowed to be referred to on stable, despite being contained in unstable
/// modules?
allowed_through_unstable_modules: bool,
Expand All @@ -149,7 +150,7 @@ pub enum StabilityLevel {
/// Rust release in which a feature is stabilized.
#[derive(Encodable, Decodable, PartialEq, Copy, Clone, Debug, Eq, Hash)]
#[derive(HashStable_Generic)]
pub enum Since {
pub enum StableSince {
Version(RustcVersion),
/// Stabilized in the upcoming version, whatever number that is.
Current,
Expand Down Expand Up @@ -378,16 +379,16 @@ fn parse_stability(sess: &Session, attr: &Attribute) -> Option<(Symbol, Stabilit

let since = if let Some(since) = since {
if since.as_str() == VERSION_PLACEHOLDER {
Since::Current
StableSince::Current
} else if let Some(version) = parse_version(since) {
Since::Version(version)
StableSince::Version(version)
} else {
sess.emit_err(session_diagnostics::InvalidSince { span: attr.span });
Since::Err
StableSince::Err
}
} else {
sess.emit_err(session_diagnostics::MissingSince { span: attr.span });
Since::Err
StableSince::Err
};

match feature {
Expand Down Expand Up @@ -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>,
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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. 👍

/// The note to issue a reason.
pub note: Option<Symbol>,
/// A text snippet used to completely replace any use of the deprecated item in an expression.
///
/// This is currently unstable.
pub suggestion: Option<Symbol>,
}

/// Release in which an API is deprecated.
#[derive(Copy, Debug, Encodable, Decodable, Clone, HashStable_Generic)]
pub enum DeprecatedSince {
RustcVersion(RustcVersion),
/// Deprecated in the future ("to be determined").
Future,
/// `feature(staged_api)` is off, or it's on but the deprecation version
/// cannot be parsed as a RustcVersion. In the latter case, an error has
/// 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),
Copy link
Contributor

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 ?

Copy link
Member Author

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.

}

/// Whether to treat the since attribute as being a Rust version identifier
/// (rather than an opaque string).
pub is_since_rustc_version: bool,
impl Display for DeprecatedSince {
fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
DeprecatedSince::RustcVersion(since) => Display::fmt(since, formatter),
DeprecatedSince::Future => formatter.write_str("TBD"),
DeprecatedSince::Symbol(since) => Display::fmt(since, formatter),
}
}
}

/// Finds the deprecation attribute. `None` if none exists.
Expand Down Expand Up @@ -839,22 +860,30 @@ pub fn find_deprecation(
}
}

if is_rustc {
if since.is_none() {
sess.emit_err(session_diagnostics::MissingSince { span: attr.span });
continue;
let since = if let Some(since) = since {
if since.as_str() == "TBD" {
Some(DeprecatedSince::Future)
} else if !is_rustc {
Some(DeprecatedSince::Symbol(since))
} else if let Some(version) = parse_version(since) {
Some(DeprecatedSince::RustcVersion(version))
} else {
sess.emit_err(session_diagnostics::InvalidSince { span: attr.span });
Some(DeprecatedSince::Symbol(since))
Copy link
Contributor

@cjgillot cjgillot Oct 30, 2023

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?

Copy link
Member Author

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

}
} else if is_rustc {
sess.emit_err(session_diagnostics::MissingSince { span: attr.span });
continue;
Copy link
Contributor

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?

Copy link
Member Author

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.

} else {
None
};

if note.is_none() {
sess.emit_err(session_diagnostics::MissingNote { span: attr.span });
continue;
}
if is_rustc && note.is_none() {
sess.emit_err(session_diagnostics::MissingNote { span: attr.span });
continue;
}

depr = Some((
Deprecation { since, note, suggestion, is_since_rustc_version: is_rustc },
attr.span,
));
depr = Some((Deprecation { since, note, suggestion }, attr.span));
}

depr
Expand Down
46 changes: 12 additions & 34 deletions compiler/rustc_middle/src/middle/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ pub use self::StabilityLevel::*;

use crate::ty::{self, TyCtxt};
use rustc_ast::NodeId;
use rustc_attr::{self as attr, ConstStability, DefaultBodyStability, Deprecation, Stability};
use rustc_attr::{
self as attr, ConstStability, DefaultBodyStability, DeprecatedSince, Deprecation, Stability,
};
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::{Applicability, Diagnostic};
use rustc_feature::GateIssue;
Expand Down Expand Up @@ -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 {
Copy link
Contributor

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?

Copy link
Member Author

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.

let is_since_rustc_version = depr.is_since_rustc_version;
let since = depr.since.as_ref().map(Symbol::as_str);

if !is_since_rustc_version {
match depr.since {
Some(DeprecatedSince::RustcVersion(since)) => since <= RustcVersion::CURRENT,
Some(DeprecatedSince::Future) => false,
// The `since` field doesn't have semantic purpose without `#![staged_api]`.
return true;
Some(DeprecatedSince::Symbol(_)) => true,
// Assume deprecation is in effect if "since" field is missing.
None => true,
}

if let Some(since) = since {
if since == "TBD" {
return false;
}

// We ignore non-integer components of the version (e.g., "nightly").
let since: Vec<u16> =
since.split(|c| c == '.' || c == '-').flat_map(|s| s.parse()).collect();

// We simply treat invalid `since` attributes as relating to a previous
// Rust version, thus always displaying the warning.
if since.len() != 3 {
return true;
}

let rustc = RustcVersion::CURRENT;
return since.as_slice() <= &[rustc.major, rustc.minor, rustc.patch];
};

// Assume deprecation is in effect if "since" field is missing
// or if we can't determine the current Rust version.
Copy link
Member Author

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.

true
}

pub fn deprecation_suggestion(
Expand All @@ -180,17 +160,15 @@ fn deprecation_lint(is_in_effect: bool) -> &'static Lint {

fn deprecation_message(
is_in_effect: bool,
since: Option<Symbol>,
since: Option<DeprecatedSince>,
note: Option<Symbol>,
kind: &str,
path: &str,
) -> String {
let message = if is_in_effect {
format!("use of deprecated {kind} `{path}`")
} else {
let since = since.as_ref().map(Symbol::as_str);

if since == Some("TBD") {
if let Some(DeprecatedSince::Future) = since {
format!("use of {kind} `{path}` that will be deprecated in a future Rust version")
} else {
format!(
Expand Down Expand Up @@ -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(_))) {
Copy link
Contributor

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?

Copy link
Member Author

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.

// Calculating message for lint involves calling `self.def_path_str`.
// Which by default to calculate visible path will invoke expensive `visible_parent_map` query.
// So we skip message calculation altogether, if lint is allowed.
Expand Down
5 changes: 0 additions & 5 deletions compiler/rustc_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -396,11 +396,6 @@ passes_invalid_attr_at_crate_level =
passes_invalid_attr_at_crate_level_item =
the inner attribute doesn't annotate this {$kind}

passes_invalid_deprecation_version =
invalid deprecation version found
.label = invalid deprecation version
.item = the stability attribute annotates this item

passes_invalid_macro_export_arguments = `{$name}` isn't a valid `#[macro_export]` argument

passes_invalid_macro_export_arguments_too_many_items = `#[macro_export]` can only take 1 or 0 arguments
Expand Down
10 changes: 0 additions & 10 deletions compiler/rustc_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1524,16 +1524,6 @@ pub struct CannotStabilizeDeprecated {
pub item_sp: Span,
}

#[derive(Diagnostic)]
#[diag(passes_invalid_deprecation_version)]
pub struct InvalidDeprecationVersion {
#[primary_span]
#[label]
pub span: Span,
#[label(passes_item)]
pub item_sp: Span,
}

#[derive(Diagnostic)]
#[diag(passes_missing_stability_attr)]
pub struct MissingStabilityAttr<'a> {
Expand Down
55 changes: 18 additions & 37 deletions compiler/rustc_passes/src/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

use crate::errors;
use rustc_attr::{
self as attr, ConstStability, Since, Stability, StabilityLevel, Unstable, UnstableReason,
VERSION_PLACEHOLDER,
self as attr, ConstStability, DeprecatedSince, Stability, StabilityLevel, StableSince,
Unstable, UnstableReason, VERSION_PLACEHOLDER,
};
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap};
use rustc_hir as hir;
Expand All @@ -24,8 +24,6 @@ use rustc_span::symbol::{sym, Symbol};
use rustc_span::Span;
use rustc_target::spec::abi::Abi;

use std::cmp::Ordering;
use std::iter;
use std::mem::replace;
use std::num::NonZeroU32;

Expand Down Expand Up @@ -198,7 +196,11 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
}
}

if let Some((rustc_attr::Deprecation { is_since_rustc_version: true, .. }, span)) = &depr {
if let Some((
rustc_attr::Deprecation { since: Some(DeprecatedSince::RustcVersion(_)), .. },
span,
)) = &depr
{
if stab.is_none() {
self.tcx.sess.emit_err(errors::DeprecatedAttribute { span: *span });
}
Expand All @@ -223,44 +225,23 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {

// Check if deprecated_since < stable_since. If it is,
// this is *almost surely* an accident.
if let (&Some(dep_since), &attr::Stable { since: stab_since, .. }) =
(&depr.as_ref().and_then(|(d, _)| d.since), &stab.level)
if let (
&Some(DeprecatedSince::RustcVersion(dep_since)),
&attr::Stable { since: stab_since, .. },
) = (&depr.as_ref().and_then(|(d, _)| d.since), &stab.level)
{
match stab_since {
Since::Current => {
StableSince::Current => {
self.tcx.sess.emit_err(errors::CannotStabilizeDeprecated { span, item_sp });
}
Since::Version(stab_since) => {
// Explicit version of iter::order::lt to handle parse errors properly
for (dep_v, stab_v) in iter::zip(
dep_since.as_str().split('.'),
[stab_since.major, stab_since.minor, stab_since.patch],
) {
match dep_v.parse::<u64>() {
Ok(dep_vp) => match dep_vp.cmp(&u64::from(stab_v)) {
Ordering::Less => {
self.tcx.sess.emit_err(errors::CannotStabilizeDeprecated {
span,
item_sp,
});
break;
}
Ordering::Equal => continue,
Ordering::Greater => break,
},
Err(_) => {
if dep_v != "TBD" {
self.tcx.sess.emit_err(errors::InvalidDeprecationVersion {
span,
item_sp,
});
}
break;
}
}
StableSince::Version(stab_since) => {
if dep_since < stab_since {
self.tcx
.sess
.emit_err(errors::CannotStabilizeDeprecated { span, item_sp });
}
}
Since::Err => {
StableSince::Err => {
// An error already reported. Assume the unparseable stabilization
// version is older than the deprecation version.
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustdoc/clean/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use thin_vec::ThinVec;

use rustc_ast as ast;
use rustc_ast_pretty::pprust;
use rustc_attr::{ConstStability, Deprecation, Since, Stability, StabilityLevel};
use rustc_attr::{ConstStability, Deprecation, Stability, StabilityLevel, StableSince};
use rustc_const_eval::const_eval::is_unstable_const_fn;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir as hir;
Expand Down Expand Up @@ -585,14 +585,14 @@ impl Item {
})
}

pub(crate) fn stable_since(&self, tcx: TyCtxt<'_>) -> Option<Since> {
pub(crate) fn stable_since(&self, tcx: TyCtxt<'_>) -> Option<StableSince> {
match self.stability(tcx)?.level {
StabilityLevel::Stable { since, .. } => Some(since),
StabilityLevel::Unstable { .. } => None,
}
}

pub(crate) fn const_stable_since(&self, tcx: TyCtxt<'_>) -> Option<Since> {
pub(crate) fn const_stable_since(&self, tcx: TyCtxt<'_>) -> Option<StableSince> {
match self.const_stability(tcx)?.level {
StabilityLevel::Stable { since, .. } => Some(since),
StabilityLevel::Unstable { .. } => None,
Expand Down
Loading
Loading