From 630c6a544f91a29f2a1749dd438e7082400295da Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Sat, 29 Sep 2018 17:25:26 -0700 Subject: [PATCH 1/4] introducing lint reason annotations (RFC 2383) This is just for the `reason =` name-value meta-item; the `#[expect(lint)]` attribute also described in the RFC is a problem for another day. The place where we were directly calling `emit()` on a match block (whose arms returned a mutable reference to a diagnostic-builder) was admittedly cute, but no longer plausibly natural after adding the if-let to the end of the `LintSource::Node` arm. This regards #54503. --- src/librustc/lint/levels.rs | 69 +++++++++++++++++------ src/librustc/lint/mod.rs | 9 ++- src/test/ui/lint/reasons-erroneous.rs | 16 ++++++ src/test/ui/lint/reasons-erroneous.stderr | 45 +++++++++++++++ src/test/ui/lint/reasons-forbidden.rs | 19 +++++++ src/test/ui/lint/reasons-forbidden.stderr | 14 +++++ src/test/ui/lint/reasons.rs | 31 ++++++++++ src/test/ui/lint/reasons.stderr | 28 +++++++++ 8 files changed, 211 insertions(+), 20 deletions(-) create mode 100644 src/test/ui/lint/reasons-erroneous.rs create mode 100644 src/test/ui/lint/reasons-erroneous.stderr create mode 100644 src/test/ui/lint/reasons-forbidden.rs create mode 100644 src/test/ui/lint/reasons-forbidden.stderr create mode 100644 src/test/ui/lint/reasons.rs create mode 100644 src/test/ui/lint/reasons.stderr diff --git a/src/librustc/lint/levels.rs b/src/librustc/lint/levels.rs index 6a4f734674563..86d0a5a479006 100644 --- a/src/librustc/lint/levels.rs +++ b/src/librustc/lint/levels.rs @@ -199,8 +199,7 @@ impl<'a> LintLevelsBuilder<'a> { let store = self.sess.lint_store.borrow(); let sess = self.sess; let bad_attr = |span| { - span_err!(sess, span, E0452, - "malformed lint attribute"); + struct_span_err!(sess, span, E0452, "malformed lint attribute") }; for attr in attrs { let level = match Level::from_str(&attr.name().as_str()) { @@ -214,17 +213,45 @@ impl<'a> LintLevelsBuilder<'a> { let metas = if let Some(metas) = meta.meta_item_list() { metas } else { - bad_attr(meta.span); + let mut err = bad_attr(meta.span); + err.emit(); continue }; + // Before processing the lint names, look for a reason (RFC 2383). + let mut reason = None; + for li in metas { + if let Some(item) = li.meta_item() { + match item.node { + ast::MetaItemKind::Word => {} // actual lint names handled later + ast::MetaItemKind::NameValue(ref name_value) => { + let name_ident = item.ident.segments[0].ident; + let name = name_ident.name.as_str(); + if name == "reason" { + if let ast::LitKind::Str(rationale, _) = name_value.node { + reason = Some(rationale); + } else { + let mut err = bad_attr(name_value.span); + err.help("reason must be a string literal"); + err.emit(); + } + } else { + let mut err = bad_attr(item.span); + err.emit(); + } + }, + ast::MetaItemKind::List(_) => { + let mut err = bad_attr(item.span); + err.emit(); + } + } + } + } + for li in metas { let word = match li.word() { Some(word) => word, - None => { - bad_attr(li.span); - continue - } + None => { continue; } }; let tool_name = if let Some(lint_tool) = word.is_scoped() { if !attr::is_known_lint_tool(lint_tool) { @@ -245,7 +272,7 @@ impl<'a> LintLevelsBuilder<'a> { let name = word.name(); match store.check_lint_name(&name.as_str(), tool_name) { CheckLintNameResult::Ok(ids) => { - let src = LintSource::Node(name, li.span); + let src = LintSource::Node(name, li.span, reason); for id in ids { specs.insert(*id, (level, src)); } @@ -255,7 +282,9 @@ impl<'a> LintLevelsBuilder<'a> { match result { Ok(ids) => { let complete_name = &format!("{}::{}", tool_name.unwrap(), name); - let src = LintSource::Node(Symbol::intern(complete_name), li.span); + let src = LintSource::Node( + Symbol::intern(complete_name), li.span, reason + ); for id in ids { specs.insert(*id, (level, src)); } @@ -286,7 +315,9 @@ impl<'a> LintLevelsBuilder<'a> { Applicability::MachineApplicable, ).emit(); - let src = LintSource::Node(Symbol::intern(&new_lint_name), li.span); + let src = LintSource::Node( + Symbol::intern(&new_lint_name), li.span, reason + ); for id in ids { specs.insert(*id, (level, src)); } @@ -368,11 +399,11 @@ impl<'a> LintLevelsBuilder<'a> { }; let forbidden_lint_name = match forbid_src { LintSource::Default => id.to_string(), - LintSource::Node(name, _) => name.to_string(), + LintSource::Node(name, _, _) => name.to_string(), LintSource::CommandLine(name) => name.to_string(), }; let (lint_attr_name, lint_attr_span) = match *src { - LintSource::Node(name, span) => (name, span), + LintSource::Node(name, span, _) => (name, span), _ => continue, }; let mut diag_builder = struct_span_err!(self.sess, @@ -384,15 +415,19 @@ impl<'a> LintLevelsBuilder<'a> { forbidden_lint_name); diag_builder.span_label(lint_attr_span, "overruled by previous forbid"); match forbid_src { - LintSource::Default => &mut diag_builder, - LintSource::Node(_, forbid_source_span) => { + LintSource::Default => {}, + LintSource::Node(_, forbid_source_span, reason) => { diag_builder.span_label(forbid_source_span, - "`forbid` level set here") + "`forbid` level set here"); + if let Some(rationale) = reason { + diag_builder.note(&rationale.as_str()); + } }, LintSource::CommandLine(_) => { - diag_builder.note("`forbid` lint level was set on command line") + diag_builder.note("`forbid` lint level was set on command line"); } - }.emit(); + } + diag_builder.emit(); // don't set a separate error for every lint in the group break } diff --git a/src/librustc/lint/mod.rs b/src/librustc/lint/mod.rs index 5ac0c0d32dcdc..afd7800810982 100644 --- a/src/librustc/lint/mod.rs +++ b/src/librustc/lint/mod.rs @@ -470,7 +470,7 @@ pub enum LintSource { Default, /// Lint level was set by an attribute. - Node(ast::Name, Span), + Node(ast::Name, Span, Option /* RFC 2383 reason */), /// Lint level was set by a command-line flag. CommandLine(Symbol), @@ -478,7 +478,7 @@ pub enum LintSource { impl_stable_hash_for!(enum self::LintSource { Default, - Node(name, span), + Node(name, span, reason), CommandLine(text) }); @@ -578,7 +578,10 @@ pub fn struct_lint_level<'a>(sess: &'a Session, hyphen_case_flag_val)); } } - LintSource::Node(lint_attr_name, src) => { + LintSource::Node(lint_attr_name, src, reason) => { + if let Some(rationale) = reason { + err.note(&rationale.as_str()); + } sess.diag_span_note_once(&mut err, DiagnosticMessageId::from(lint), src, "lint level defined here"); if lint_attr_name.as_str() != name { diff --git a/src/test/ui/lint/reasons-erroneous.rs b/src/test/ui/lint/reasons-erroneous.rs new file mode 100644 index 0000000000000..68f2ce42847d7 --- /dev/null +++ b/src/test/ui/lint/reasons-erroneous.rs @@ -0,0 +1,16 @@ +#![warn(absolute_paths_not_starting_with_crate, reason = 0)] +//~^ ERROR malformed lint attribute +//~| HELP reason must be a string literal +#![warn(anonymous_parameters, reason = b"consider these, for we have condemned them")] +//~^ ERROR malformed lint attribute +//~| HELP reason must be a string literal +#![warn(bare_trait_objects, reasons = "leaders to no sure land, guides their bearings lost")] +//~^ ERROR malformed lint attribute +#![warn(box_pointers, blerp = "or in league with robbers have reversed the signposts")] +//~^ ERROR malformed lint attribute +#![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))] +//~^ ERROR malformed lint attribute +#![warn(ellipsis_inclusive_range_patterns, reason)] +//~^ WARN unknown lint + +fn main() {} diff --git a/src/test/ui/lint/reasons-erroneous.stderr b/src/test/ui/lint/reasons-erroneous.stderr new file mode 100644 index 0000000000000..25bdda178eee0 --- /dev/null +++ b/src/test/ui/lint/reasons-erroneous.stderr @@ -0,0 +1,45 @@ +error[E0452]: malformed lint attribute + --> $DIR/reasons-erroneous.rs:1:58 + | +LL | #![warn(absolute_paths_not_starting_with_crate, reason = 0)] + | ^ + | + = help: reason must be a string literal + +error[E0452]: malformed lint attribute + --> $DIR/reasons-erroneous.rs:4:40 + | +LL | #![warn(anonymous_parameters, reason = b"consider these, for we have condemned them")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: reason must be a string literal + +error[E0452]: malformed lint attribute + --> $DIR/reasons-erroneous.rs:7:29 + | +LL | #![warn(bare_trait_objects, reasons = "leaders to no sure land, guides their bearings lost")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0452]: malformed lint attribute + --> $DIR/reasons-erroneous.rs:9:23 + | +LL | #![warn(box_pointers, blerp = "or in league with robbers have reversed the signposts")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error[E0452]: malformed lint attribute + --> $DIR/reasons-erroneous.rs:11:36 + | +LL | #![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: unknown lint: `reason` + --> $DIR/reasons-erroneous.rs:13:44 + | +LL | #![warn(ellipsis_inclusive_range_patterns, reason)] + | ^^^^^^ + | + = note: #[warn(unknown_lints)] on by default + +error: aborting due to 5 previous errors + +For more information about this error, try `rustc --explain E0452`. diff --git a/src/test/ui/lint/reasons-forbidden.rs b/src/test/ui/lint/reasons-forbidden.rs new file mode 100644 index 0000000000000..cad62b3a3c16c --- /dev/null +++ b/src/test/ui/lint/reasons-forbidden.rs @@ -0,0 +1,19 @@ +#![forbid( + unsafe_code, + //~^ NOTE `forbid` level set here + reason = "our errors & omissions insurance policy doesn't cover unsafe Rust" +)] + +use std::ptr; + +fn main() { + let a_billion_dollar_mistake = ptr::null(); + + #[allow(unsafe_code)] + //~^ ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code) + //~| NOTE overruled by previous forbid + //~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust + unsafe { + *a_billion_dollar_mistake + } +} diff --git a/src/test/ui/lint/reasons-forbidden.stderr b/src/test/ui/lint/reasons-forbidden.stderr new file mode 100644 index 0000000000000..cc9e787b2d42d --- /dev/null +++ b/src/test/ui/lint/reasons-forbidden.stderr @@ -0,0 +1,14 @@ +error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code) + --> $DIR/reasons-forbidden.rs:12:13 + | +LL | unsafe_code, + | ----------- `forbid` level set here +... +LL | #[allow(unsafe_code)] + | ^^^^^^^^^^^ overruled by previous forbid + | + = note: our errors & omissions insurance policy doesn't cover unsafe Rust + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0453`. diff --git a/src/test/ui/lint/reasons.rs b/src/test/ui/lint/reasons.rs new file mode 100644 index 0000000000000..9166caf9ebec6 --- /dev/null +++ b/src/test/ui/lint/reasons.rs @@ -0,0 +1,31 @@ +// compile-pass + +#![warn(elided_lifetimes_in_paths, + //~^ NOTE lint level defined here + reason = "explicit anonymous lifetimes aid reasoning about ownership")] +#![warn( + nonstandard_style, + //~^ NOTE lint level defined here + reason = r#"people shouldn't have to change their usual style habits +to contribute to our project"# +)] +#![allow(unused, reason = "unused code has never killed anypony")] + +use std::fmt; + +pub struct CheaterDetectionMechanism {} + +impl fmt::Debug for CheaterDetectionMechanism { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + //~^ WARN hidden lifetime parameters in types are deprecated + //~| NOTE explicit anonymous lifetimes aid + //~| HELP indicate the anonymous lifetime + fmt.debug_struct("CheaterDetectionMechanism").finish() + } +} + +fn main() { + let Social_exchange_psychology = CheaterDetectionMechanism {}; + //~^ WARN should have a snake case name such as + //~| NOTE people shouldn't have to change their usual style habits +} diff --git a/src/test/ui/lint/reasons.stderr b/src/test/ui/lint/reasons.stderr new file mode 100644 index 0000000000000..bdaf848c340e3 --- /dev/null +++ b/src/test/ui/lint/reasons.stderr @@ -0,0 +1,28 @@ +warning: hidden lifetime parameters in types are deprecated + --> $DIR/reasons.rs:19:29 + | +LL | fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + | ^^^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>` + | + = note: explicit anonymous lifetimes aid reasoning about ownership +note: lint level defined here + --> $DIR/reasons.rs:3:9 + | +LL | #![warn(elided_lifetimes_in_paths, + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: variable `Social_exchange_psychology` should have a snake case name such as `social_exchange_psychology` + --> $DIR/reasons.rs:28:9 + | +LL | let Social_exchange_psychology = CheaterDetectionMechanism {}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: people shouldn't have to change their usual style habits + to contribute to our project +note: lint level defined here + --> $DIR/reasons.rs:7:5 + | +LL | nonstandard_style, + | ^^^^^^^^^^^^^^^^^ + = note: #[warn(non_snake_case)] implied by #[warn(nonstandard_style)] + From dc0609c2473c01f521c5a3b9959edf7dd11f2d86 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Sat, 29 Sep 2018 18:00:50 -0700 Subject: [PATCH 2/4] feature-gate lint reasons We take stability seriously, so we shy away from making even seemingly "trivial" features insta-stable. --- src/librustc/lint/levels.rs | 14 +++++++++++++- src/libsyntax/feature_gate.rs | 3 +++ .../ui/feature-gates/feature-gate-lint-reasons.rs | 4 ++++ .../feature-gates/feature-gate-lint-reasons.stderr | 11 +++++++++++ src/test/ui/lint/reasons-erroneous.rs | 2 ++ src/test/ui/lint/reasons-erroneous.stderr | 12 ++++++------ src/test/ui/lint/reasons-forbidden.rs | 2 ++ src/test/ui/lint/reasons-forbidden.stderr | 2 +- src/test/ui/lint/reasons.rs | 2 ++ src/test/ui/lint/reasons.stderr | 8 ++++---- 10 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 src/test/ui/feature-gates/feature-gate-lint-reasons.rs create mode 100644 src/test/ui/feature-gates/feature-gate-lint-reasons.stderr diff --git a/src/librustc/lint/levels.rs b/src/librustc/lint/levels.rs index 86d0a5a479006..ee878da00660e 100644 --- a/src/librustc/lint/levels.rs +++ b/src/librustc/lint/levels.rs @@ -225,11 +225,23 @@ impl<'a> LintLevelsBuilder<'a> { match item.node { ast::MetaItemKind::Word => {} // actual lint names handled later ast::MetaItemKind::NameValue(ref name_value) => { + let gate_reasons = !self.sess.features_untracked().lint_reasons; let name_ident = item.ident.segments[0].ident; let name = name_ident.name.as_str(); + if name == "reason" { if let ast::LitKind::Str(rationale, _) = name_value.node { - reason = Some(rationale); + if gate_reasons { + feature_gate::emit_feature_err( + &self.sess.parse_sess, + "lint_reasons", + item.span, + feature_gate::GateIssue::Language, + "lint reasons are experimental" + ); + } else { + reason = Some(rationale); + } } else { let mut err = bad_attr(name_value.span); err.help("reason must be a string literal"); diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 2cd4fd92bc81e..da0ec33030e06 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -504,6 +504,9 @@ declare_features! ( // `extern crate foo as bar;` puts `bar` into extern prelude. (active, extern_crate_item_prelude, "1.31.0", Some(54658), None), + + // `reason = ` in lint attributes and `expect` lint attribute + (active, lint_reasons, "1.31.0", Some(54503), None), ); declare_features! ( diff --git a/src/test/ui/feature-gates/feature-gate-lint-reasons.rs b/src/test/ui/feature-gates/feature-gate-lint-reasons.rs new file mode 100644 index 0000000000000..1a7b9c990fa64 --- /dev/null +++ b/src/test/ui/feature-gates/feature-gate-lint-reasons.rs @@ -0,0 +1,4 @@ +#![warn(nonstandard_style, reason = "the standard should be respected")] +//~^ ERROR lint reasons are experimental + +fn main() {} diff --git a/src/test/ui/feature-gates/feature-gate-lint-reasons.stderr b/src/test/ui/feature-gates/feature-gate-lint-reasons.stderr new file mode 100644 index 0000000000000..6a36d9fd5a8e5 --- /dev/null +++ b/src/test/ui/feature-gates/feature-gate-lint-reasons.stderr @@ -0,0 +1,11 @@ +error[E0658]: lint reasons are experimental (see issue #54503) + --> $DIR/feature-gate-lint-reasons.rs:1:28 + | +LL | #![warn(nonstandard_style, reason = "the standard should be respected")] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: add #![feature(lint_reasons)] to the crate attributes to enable + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0658`. diff --git a/src/test/ui/lint/reasons-erroneous.rs b/src/test/ui/lint/reasons-erroneous.rs index 68f2ce42847d7..90111a9c4e1e9 100644 --- a/src/test/ui/lint/reasons-erroneous.rs +++ b/src/test/ui/lint/reasons-erroneous.rs @@ -1,3 +1,5 @@ +#![feature(lint_reasons)] + #![warn(absolute_paths_not_starting_with_crate, reason = 0)] //~^ ERROR malformed lint attribute //~| HELP reason must be a string literal diff --git a/src/test/ui/lint/reasons-erroneous.stderr b/src/test/ui/lint/reasons-erroneous.stderr index 25bdda178eee0..f66999e927203 100644 --- a/src/test/ui/lint/reasons-erroneous.stderr +++ b/src/test/ui/lint/reasons-erroneous.stderr @@ -1,5 +1,5 @@ error[E0452]: malformed lint attribute - --> $DIR/reasons-erroneous.rs:1:58 + --> $DIR/reasons-erroneous.rs:3:58 | LL | #![warn(absolute_paths_not_starting_with_crate, reason = 0)] | ^ @@ -7,7 +7,7 @@ LL | #![warn(absolute_paths_not_starting_with_crate, reason = 0)] = help: reason must be a string literal error[E0452]: malformed lint attribute - --> $DIR/reasons-erroneous.rs:4:40 + --> $DIR/reasons-erroneous.rs:6:40 | LL | #![warn(anonymous_parameters, reason = b"consider these, for we have condemned them")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -15,25 +15,25 @@ LL | #![warn(anonymous_parameters, reason = b"consider these, for we have condem = help: reason must be a string literal error[E0452]: malformed lint attribute - --> $DIR/reasons-erroneous.rs:7:29 + --> $DIR/reasons-erroneous.rs:9:29 | LL | #![warn(bare_trait_objects, reasons = "leaders to no sure land, guides their bearings lost")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0452]: malformed lint attribute - --> $DIR/reasons-erroneous.rs:9:23 + --> $DIR/reasons-erroneous.rs:11:23 | LL | #![warn(box_pointers, blerp = "or in league with robbers have reversed the signposts")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0452]: malformed lint attribute - --> $DIR/reasons-erroneous.rs:11:36 + --> $DIR/reasons-erroneous.rs:13:36 | LL | #![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ warning: unknown lint: `reason` - --> $DIR/reasons-erroneous.rs:13:44 + --> $DIR/reasons-erroneous.rs:15:44 | LL | #![warn(ellipsis_inclusive_range_patterns, reason)] | ^^^^^^ diff --git a/src/test/ui/lint/reasons-forbidden.rs b/src/test/ui/lint/reasons-forbidden.rs index cad62b3a3c16c..19ab76707d408 100644 --- a/src/test/ui/lint/reasons-forbidden.rs +++ b/src/test/ui/lint/reasons-forbidden.rs @@ -1,3 +1,5 @@ +#![feature(lint_reasons)] + #![forbid( unsafe_code, //~^ NOTE `forbid` level set here diff --git a/src/test/ui/lint/reasons-forbidden.stderr b/src/test/ui/lint/reasons-forbidden.stderr index cc9e787b2d42d..ea09e591cba0f 100644 --- a/src/test/ui/lint/reasons-forbidden.stderr +++ b/src/test/ui/lint/reasons-forbidden.stderr @@ -1,5 +1,5 @@ error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code) - --> $DIR/reasons-forbidden.rs:12:13 + --> $DIR/reasons-forbidden.rs:14:13 | LL | unsafe_code, | ----------- `forbid` level set here diff --git a/src/test/ui/lint/reasons.rs b/src/test/ui/lint/reasons.rs index 9166caf9ebec6..eba91d92afb5b 100644 --- a/src/test/ui/lint/reasons.rs +++ b/src/test/ui/lint/reasons.rs @@ -1,5 +1,7 @@ // compile-pass +#![feature(lint_reasons)] + #![warn(elided_lifetimes_in_paths, //~^ NOTE lint level defined here reason = "explicit anonymous lifetimes aid reasoning about ownership")] diff --git a/src/test/ui/lint/reasons.stderr b/src/test/ui/lint/reasons.stderr index bdaf848c340e3..df0f9cb9b61e8 100644 --- a/src/test/ui/lint/reasons.stderr +++ b/src/test/ui/lint/reasons.stderr @@ -1,18 +1,18 @@ warning: hidden lifetime parameters in types are deprecated - --> $DIR/reasons.rs:19:29 + --> $DIR/reasons.rs:21:29 | LL | fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { | ^^^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>` | = note: explicit anonymous lifetimes aid reasoning about ownership note: lint level defined here - --> $DIR/reasons.rs:3:9 + --> $DIR/reasons.rs:5:9 | LL | #![warn(elided_lifetimes_in_paths, | ^^^^^^^^^^^^^^^^^^^^^^^^^ warning: variable `Social_exchange_psychology` should have a snake case name such as `social_exchange_psychology` - --> $DIR/reasons.rs:28:9 + --> $DIR/reasons.rs:30:9 | LL | let Social_exchange_psychology = CheaterDetectionMechanism {}; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -20,7 +20,7 @@ LL | let Social_exchange_psychology = CheaterDetectionMechanism {}; = note: people shouldn't have to change their usual style habits to contribute to our project note: lint level defined here - --> $DIR/reasons.rs:7:5 + --> $DIR/reasons.rs:9:5 | LL | nonstandard_style, | ^^^^^^^^^^^^^^^^^ From f90de1110d93ea389342220caad19e05c4e6ad10 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Fri, 12 Oct 2018 00:21:21 -0700 Subject: [PATCH 3/4] in which lint reasons are restricted to come last in the attribute Vadim Petrochenkov suggested this in review ("an error? just to be conservative"), and it turns out to be convenient from the implementer's perspective: in the initial proposed implementation (or `HEAD~2`, as some might prefer to call it), we were doing an entire whole iteration over the meta items just to find the reason (before iterating over them to set the actual lint levels). This way, we can just peek at the end rather than adding that extra loop (or restructuring the existing code). The RFC doesn't seem to take a position on this, and there's some precedent for restricting things to be at the end of a sequence (we only allow `..` at the end of a struct pattern, even if it would be possible to let it appear anywhere in the sequence). --- src/librustc/lint/levels.rs | 73 +++++++++++++---------- src/test/ui/lint/reasons-erroneous.rs | 8 ++- src/test/ui/lint/reasons-erroneous.stderr | 24 ++++++-- 3 files changed, 69 insertions(+), 36 deletions(-) diff --git a/src/librustc/lint/levels.rs b/src/librustc/lint/levels.rs index ee878da00660e..d44facedc8b7c 100644 --- a/src/librustc/lint/levels.rs +++ b/src/librustc/lint/levels.rs @@ -21,6 +21,7 @@ use rustc_data_structures::stable_hasher::{HashStable, ToStableHashKey, use session::Session; use syntax::ast; use syntax::attr; +use syntax::feature_gate; use syntax::source_map::MultiSpan; use syntax::symbol::Symbol; use util::nodemap::FxHashMap; @@ -210,7 +211,7 @@ impl<'a> LintLevelsBuilder<'a> { let meta = unwrap_or!(attr.meta(), continue); attr::mark_used(attr); - let metas = if let Some(metas) = meta.meta_item_list() { + let mut metas = if let Some(metas) = meta.meta_item_list() { metas } else { let mut err = bad_attr(meta.span); @@ -218,44 +219,43 @@ impl<'a> LintLevelsBuilder<'a> { continue }; - // Before processing the lint names, look for a reason (RFC 2383). + // Before processing the lint names, look for a reason (RFC 2383) + // at the end. let mut reason = None; - for li in metas { - if let Some(item) = li.meta_item() { - match item.node { - ast::MetaItemKind::Word => {} // actual lint names handled later - ast::MetaItemKind::NameValue(ref name_value) => { - let gate_reasons = !self.sess.features_untracked().lint_reasons; - let name_ident = item.ident.segments[0].ident; - let name = name_ident.name.as_str(); - - if name == "reason" { - if let ast::LitKind::Str(rationale, _) = name_value.node { - if gate_reasons { - feature_gate::emit_feature_err( - &self.sess.parse_sess, - "lint_reasons", - item.span, - feature_gate::GateIssue::Language, - "lint reasons are experimental" - ); - } else { - reason = Some(rationale); - } + let tail_li = &metas[metas.len()-1]; + if let Some(item) = tail_li.meta_item() { + match item.node { + ast::MetaItemKind::Word => {} // actual lint names handled later + ast::MetaItemKind::NameValue(ref name_value) => { + let gate_reasons = !self.sess.features_untracked().lint_reasons; + if item.ident == "reason" { + // found reason, reslice meta list to exclude it + metas = &metas[0..metas.len()-1]; + if let ast::LitKind::Str(rationale, _) = name_value.node { + if gate_reasons { + feature_gate::emit_feature_err( + &self.sess.parse_sess, + "lint_reasons", + item.span, + feature_gate::GateIssue::Language, + "lint reasons are experimental" + ); } else { - let mut err = bad_attr(name_value.span); - err.help("reason must be a string literal"); - err.emit(); + reason = Some(rationale); } } else { - let mut err = bad_attr(item.span); + let mut err = bad_attr(name_value.span); + err.help("reason must be a string literal"); err.emit(); } - }, - ast::MetaItemKind::List(_) => { + } else { let mut err = bad_attr(item.span); err.emit(); } + }, + ast::MetaItemKind::List(_) => { + let mut err = bad_attr(item.span); + err.emit(); } } } @@ -263,7 +263,18 @@ impl<'a> LintLevelsBuilder<'a> { for li in metas { let word = match li.word() { Some(word) => word, - None => { continue; } + None => { + let mut err = bad_attr(li.span); + if let Some(item) = li.meta_item() { + if let ast::MetaItemKind::NameValue(_) = item.node { + if item.ident == "reason" { + err.help("reason in lint attribute must come last"); + } + } + } + err.emit(); + continue; + } }; let tool_name = if let Some(lint_tool) = word.is_scoped() { if !attr::is_known_lint_tool(lint_tool) { diff --git a/src/test/ui/lint/reasons-erroneous.rs b/src/test/ui/lint/reasons-erroneous.rs index 90111a9c4e1e9..e42b329338b5a 100644 --- a/src/test/ui/lint/reasons-erroneous.rs +++ b/src/test/ui/lint/reasons-erroneous.rs @@ -12,7 +12,13 @@ //~^ ERROR malformed lint attribute #![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))] //~^ ERROR malformed lint attribute -#![warn(ellipsis_inclusive_range_patterns, reason)] +#![warn(ellipsis_inclusive_range_patterns, reason = "born barren", reason = "a freak growth")] +//~^ ERROR malformed lint attribute +//~| HELP reason in lint attribute must come last +#![warn(keyword_idents, reason = "root in rubble", macro_use_extern_crate)] +//~^ ERROR malformed lint attribute +//~| HELP reason in lint attribute must come last +#![warn(missing_copy_implementations, reason)] //~^ WARN unknown lint fn main() {} diff --git a/src/test/ui/lint/reasons-erroneous.stderr b/src/test/ui/lint/reasons-erroneous.stderr index f66999e927203..6842686ecbab5 100644 --- a/src/test/ui/lint/reasons-erroneous.stderr +++ b/src/test/ui/lint/reasons-erroneous.stderr @@ -32,14 +32,30 @@ error[E0452]: malformed lint attribute LL | #![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -warning: unknown lint: `reason` +error[E0452]: malformed lint attribute --> $DIR/reasons-erroneous.rs:15:44 | -LL | #![warn(ellipsis_inclusive_range_patterns, reason)] - | ^^^^^^ +LL | #![warn(ellipsis_inclusive_range_patterns, reason = "born barren", reason = "a freak growth")] + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = help: reason in lint attribute must come last + +error[E0452]: malformed lint attribute + --> $DIR/reasons-erroneous.rs:18:25 + | +LL | #![warn(keyword_idents, reason = "root in rubble", macro_use_extern_crate)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: reason in lint attribute must come last + +warning: unknown lint: `reason` + --> $DIR/reasons-erroneous.rs:21:39 + | +LL | #![warn(missing_copy_implementations, reason)] + | ^^^^^^ | = note: #[warn(unknown_lints)] on by default -error: aborting due to 5 previous errors +error: aborting due to 7 previous errors For more information about this error, try `rustc --explain E0452`. From f66ea66acd1c5e24e16fafe28021af1b723d6824 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Mon, 15 Oct 2018 23:35:58 -0700 Subject: [PATCH 4/4] wherein the status of empty and reason-only lint attributes is clarified We avoid an ICE by checking for an empty meta-item list before we index into the meta-items, and leave commentary about where we'd like to issue unused-attributes lints in the future. Note that empty lint attributes are already accepted by the stable compiler; generalizing this to weird reason-only lint attributes seems like the conservative/consilient generalization. --- src/librustc/lint/levels.rs | 9 ++++++++- src/test/ui/lint/empty-lint-attributes.rs | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/lint/empty-lint-attributes.rs diff --git a/src/librustc/lint/levels.rs b/src/librustc/lint/levels.rs index d44facedc8b7c..732b32cc35d68 100644 --- a/src/librustc/lint/levels.rs +++ b/src/librustc/lint/levels.rs @@ -216,9 +216,14 @@ impl<'a> LintLevelsBuilder<'a> { } else { let mut err = bad_attr(meta.span); err.emit(); - continue + continue; }; + if metas.is_empty() { + // FIXME (#55112): issue unused-attributes lint for `#[level()]` + continue; + } + // Before processing the lint names, look for a reason (RFC 2383) // at the end. let mut reason = None; @@ -231,6 +236,8 @@ impl<'a> LintLevelsBuilder<'a> { if item.ident == "reason" { // found reason, reslice meta list to exclude it metas = &metas[0..metas.len()-1]; + // FIXME (#55112): issue unused-attributes lint if we thereby + // don't have any lint names (`#[level(reason = "foo")]`) if let ast::LitKind::Str(rationale, _) = name_value.node { if gate_reasons { feature_gate::emit_feature_err( diff --git a/src/test/ui/lint/empty-lint-attributes.rs b/src/test/ui/lint/empty-lint-attributes.rs new file mode 100644 index 0000000000000..1f0a9538d88b1 --- /dev/null +++ b/src/test/ui/lint/empty-lint-attributes.rs @@ -0,0 +1,17 @@ +#![feature(lint_reasons)] + +// run-pass + +// Empty (and reason-only) lint attributes are legal—although we may want to +// lint them in the future (Issue #55112). + +#![allow()] +#![warn(reason = "observationalism")] + +#[forbid()] +fn devoir() {} + +#[deny(reason = "ultion")] +fn waldgrave() {} + +fn main() {}