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

Improve errors about #[deprecated] attribute #78626

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions compiler/rustc_attr/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -637,19 +637,15 @@ pub struct Deprecation {
}

/// Finds the deprecation attribute. `None` if none exists.
pub fn find_deprecation(sess: &Session, attrs: &[Attribute], item_sp: Span) -> Option<Deprecation> {
find_deprecation_generic(sess, attrs.iter(), item_sp)
pub fn find_deprecation(sess: &Session, attrs: &[Attribute]) -> Option<(Deprecation, Span)> {
find_deprecation_generic(sess, attrs.iter())
}

fn find_deprecation_generic<'a, I>(
sess: &Session,
attrs_iter: I,
item_sp: Span,
) -> Option<Deprecation>
fn find_deprecation_generic<'a, I>(sess: &Session, attrs_iter: I) -> Option<(Deprecation, Span)>
where
I: Iterator<Item = &'a Attribute>,
{
let mut depr: Option<Deprecation> = None;
let mut depr: Option<(Deprecation, Span)> = None;
let diagnostic = &sess.parse_sess.span_diagnostic;

'outer: for attr in attrs_iter {
Expand All @@ -658,8 +654,10 @@ where
continue;
}

if depr.is_some() {
struct_span_err!(diagnostic, item_sp, E0550, "multiple deprecated attributes").emit();
if let Some((_, span)) = &depr {
struct_span_err!(diagnostic, attr.span, E0550, "multiple deprecated attributes")
.span_note(*span, "first deprecation attribute here")
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 happen multiple times (if you have three deprecation attributes)? If so, reword the label to not make it technically incorrect.

Also, use prefer to use span_label when possible.

Suggested change
struct_span_err!(diagnostic, attr.span, E0550, "multiple deprecated attributes")
.span_note(*span, "first deprecation attribute here")
struct_span_err!(diagnostic, attr.span, E0550, "multiple deprecated attributes")
.span_label(attr.span, "repeated deprecated attribute")
.span_label(*span, "prior deprecation attribute")

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 stops right after the first duplicate, so having three deprecation attributes wouldn't output any more diagnostics than two.

Is there documentation somewhere about all the different options for diagnostics, like span_label, span_suggestion_short, etc.?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/rust-lang/rust/blob/master/compiler/rustc_errors/src/diagnostic.rs has all the methods that can be used and they have good docstrings for the most part.

.emit();
break;
}

Expand Down Expand Up @@ -780,7 +778,7 @@ where
sess.mark_attr_used(&attr);

let is_since_rustc_version = sess.check_name(attr, sym::rustc_deprecated);
depr = Some(Deprecation { since, note, suggestion, is_since_rustc_version });
depr = Some((Deprecation { since, note, suggestion, is_since_rustc_version }, attr.span));
}

depr
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_expand/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ impl SyntaxExtension {
allow_internal_unsafe: sess.contains_name(attrs, sym::allow_internal_unsafe),
local_inner_macros,
stability,
deprecation: attr::find_deprecation(&sess, attrs, span),
deprecation: attr::find_deprecation(&sess, attrs).map(|(d, _)| d),
helper_attrs,
edition,
is_builtin,
Expand Down
27 changes: 27 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2705,6 +2705,32 @@ declare_lint! {
};
}

declare_lint! {
/// The `useless_deprecated` lint detects deprecation attributes with no effect.
///
/// ### Example
///
/// ```rust,compile_fail
/// struct X;
///
/// #[deprecated = "message"]
/// impl Default for X {
/// fn default() -> Self {
/// X
/// }
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Deprecation attributes have no effect on trait implementations.
pub USELESS_DEPRECATED,
Deny,
"detects deprecation attributes with no effect",
}

declare_tool_lint! {
pub rustc::INEFFECTIVE_UNSTABLE_TRAIT_IMPL,
Deny,
Expand Down Expand Up @@ -2792,6 +2818,7 @@ declare_lint_pass! {
INEFFECTIVE_UNSTABLE_TRAIT_IMPL,
UNINHABITED_STATIC,
FUNCTION_ITEM_REFERENCES,
USELESS_DEPRECATED,
]
}

Expand Down
33 changes: 22 additions & 11 deletions compiler/rustc_passes/src/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use rustc_middle::middle::privacy::AccessLevels;
use rustc_middle::middle::stability::{DeprecationEntry, Index};
use rustc_middle::ty::{self, query::Providers, TyCtxt};
use rustc_session::lint;
use rustc_session::lint::builtin::INEFFECTIVE_UNSTABLE_TRAIT_IMPL;
use rustc_session::lint::builtin::{INEFFECTIVE_UNSTABLE_TRAIT_IMPL, USELESS_DEPRECATED};
use rustc_session::parse::feature_err;
use rustc_session::Session;
use rustc_span::symbol::{sym, Symbol};
Expand All @@ -31,6 +31,8 @@ enum AnnotationKind {
Required,
// Annotation is useless, reject it
Prohibited,
// Deprecation annotation is useless, reject it. (Stability attribute is still required.)
DeprecationProhibited,
// Annotation itself is useless, but it can be propagated to children
Container,
}
Expand Down Expand Up @@ -83,14 +85,22 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
did_error = self.forbid_staged_api_attrs(hir_id, attrs, inherit_deprecation.clone());
}

let depr =
if did_error { None } else { attr::find_deprecation(&self.tcx.sess, attrs, item_sp) };
let depr = if did_error { None } else { attr::find_deprecation(&self.tcx.sess, attrs) };
let mut is_deprecated = false;
if let Some(depr) = &depr {
if let Some((depr, span)) = &depr {
is_deprecated = true;

if kind == AnnotationKind::Prohibited {
self.tcx.sess.span_err(item_sp, "This deprecation annotation is useless");
if kind == AnnotationKind::Prohibited || kind == AnnotationKind::DeprecationProhibited {
self.tcx.struct_span_lint_hir(USELESS_DEPRECATED, hir_id, *span, |lint| {
lint.build("this `#[deprecated]' annotation has no effect")
m-ou-se marked this conversation as resolved.
Show resolved Hide resolved
.span_suggestion(
m-ou-se marked this conversation as resolved.
Show resolved Hide resolved
*span,
"try removing the deprecation attribute",
m-ou-se marked this conversation as resolved.
Show resolved Hide resolved
String::new(),
rustc_errors::Applicability::MachineApplicable,
)
.emit()
});
}

// `Deprecation` is just two pointers, no need to intern it
Expand All @@ -114,7 +124,7 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
}
} else {
self.recurse_with_stability_attrs(
depr.map(|d| DeprecationEntry::local(d, hir_id)),
depr.map(|(d, _)| DeprecationEntry::local(d, hir_id)),
None,
None,
visit_children,
Expand All @@ -139,11 +149,11 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
}
}

if depr.as_ref().map_or(false, |d| d.is_since_rustc_version) {
if let Some((rustc_attr::Deprecation { is_since_rustc_version: true, .. }, span)) = &depr {
if stab.is_none() {
struct_span_err!(
self.tcx.sess,
item_sp,
*span,
E0549,
"rustc_deprecated attribute must be paired with \
either stable or unstable attribute"
Expand All @@ -166,7 +176,7 @@ 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)
(&depr.as_ref().and_then(|(d, _)| d.since), &stab.level)
{
// Explicit version of iter::order::lt to handle parse errors properly
for (dep_v, stab_v) in
Expand Down Expand Up @@ -212,7 +222,7 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
}

self.recurse_with_stability_attrs(
depr.map(|d| DeprecationEntry::local(d, hir_id)),
depr.map(|(d, _)| DeprecationEntry::local(d, hir_id)),
stab,
const_stab,
visit_children,
Expand Down Expand Up @@ -322,6 +332,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Annotator<'a, 'tcx> {
}
hir::ItemKind::Impl { of_trait: Some(_), .. } => {
self.in_trait_impl = true;
kind = AnnotationKind::DeprecationProhibited;
}
hir::ItemKind::Struct(ref sd, _) => {
if let Some(ctor_hir_id) = sd.ctor_hir_id() {
Expand Down
13 changes: 11 additions & 2 deletions src/test/ui/deprecation/deprecation-sanity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,19 @@ mod bogus_attribute_types_1 {
}

#[deprecated(since = "a", note = "b")]
#[deprecated(since = "a", note = "b")]
fn multiple1() { } //~ ERROR multiple deprecated attributes
#[deprecated(since = "a", note = "b")] //~ ERROR multiple deprecated attributes
fn multiple1() { }

#[deprecated(since = "a", since = "b", note = "c")] //~ ERROR multiple 'since' items
fn f1() { }

struct X;

#[deprecated = "hello"] //~ ERROR this `#[deprecated]' annotation has no effect
impl Default for X {
fn default() -> Self {
X
}
}

fn main() { }
22 changes: 18 additions & 4 deletions src/test/ui/deprecation/deprecation-sanity.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,32 @@ LL | #[deprecated("test")]
| ^^^^^^

error[E0550]: multiple deprecated attributes
--> $DIR/deprecation-sanity.rs:28:1
--> $DIR/deprecation-sanity.rs:27:1
|
LL | fn multiple1() { }
| ^^^^^^^^^^^^^^^^^^
LL | #[deprecated(since = "a", note = "b")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: first deprecation attribute here
--> $DIR/deprecation-sanity.rs:26:1
|
LL | #[deprecated(since = "a", note = "b")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0538]: multiple 'since' items
--> $DIR/deprecation-sanity.rs:30:27
|
LL | #[deprecated(since = "a", since = "b", note = "c")]
| ^^^^^^^^^^^

error: aborting due to 9 previous errors
error: this `#[deprecated]' annotation has no effect
--> $DIR/deprecation-sanity.rs:35:1
|
LL | #[deprecated = "hello"]
| ^^^^^^^^^^^^^^^^^^^^^^^ help: try removing the deprecation attribute
|
= note: `#[deny(useless_deprecated)]` on by default

error: aborting due to 10 previous errors

Some errors have detailed explanations: E0538, E0541, E0550, E0551, E0565.
For more information about an error, try `rustc --explain E0538`.
6 changes: 3 additions & 3 deletions src/test/ui/stability-attribute/stability-attribute-sanity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ fn multiple3() { }

#[stable(feature = "a", since = "b")]
#[rustc_deprecated(since = "b", reason = "text")]
#[rustc_deprecated(since = "b", reason = "text")]
#[rustc_deprecated(since = "b", reason = "text")] //~ ERROR multiple deprecated attributes
#[rustc_const_unstable(feature = "c", issue = "none")]
#[rustc_const_unstable(feature = "d", issue = "none")] //~ ERROR multiple stability levels
pub const fn multiple4() { } //~ ERROR multiple deprecated attributes
pub const fn multiple4() { }
//~^ ERROR Invalid stability or deprecation version found

#[rustc_deprecated(since = "a", reason = "text")]
fn deprecated_without_unstable_or_stable() { }
//~^ ERROR rustc_deprecated attribute must be paired with either stable or unstable attribute
//~^^ ERROR rustc_deprecated attribute must be paired with either stable or unstable attribute

fn main() { }
18 changes: 12 additions & 6 deletions src/test/ui/stability-attribute/stability-attribute-sanity.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,16 @@ LL | #[stable(feature = "a", since = "b")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0550]: multiple deprecated attributes
--> $DIR/stability-attribute-sanity.rs:65:1
--> $DIR/stability-attribute-sanity.rs:62:1
|
LL | pub const fn multiple4() { }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | #[rustc_deprecated(since = "b", reason = "text")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: first deprecation attribute here
--> $DIR/stability-attribute-sanity.rs:61:1
|
LL | #[rustc_deprecated(since = "b", reason = "text")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0544]: multiple stability levels
--> $DIR/stability-attribute-sanity.rs:64:1
Expand All @@ -101,10 +107,10 @@ LL | pub const fn multiple4() { }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0549]: rustc_deprecated attribute must be paired with either stable or unstable attribute
--> $DIR/stability-attribute-sanity.rs:69:1
--> $DIR/stability-attribute-sanity.rs:68:1
|
LL | fn deprecated_without_unstable_or_stable() { }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | #[rustc_deprecated(since = "a", reason = "text")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 18 previous errors

Expand Down