Auto merge of #54683 - zackmdavis:critique_of_pure_lints, r=petrochenkov

bors · bors · commit 18311a6c4789 · 2018-10-28T00:28:03.000Z
lint reasons (RFC 2883, part 1) This implements the `reason =` functionality described in [the RFC](https://github.com/rust-lang/rfcs/blob/master/text/2383-lint-reasons.md) under a `lint_reasons` feature gate. ![lint_reasons_pt_1](https://user-images.githubusercontent.com/1076988/46252097-eed51000-c418-11e8-8212-939d3f02f95d.png)
diff --git 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;
@@ -199,8 +200,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()) {
@@ -211,19 +211,76 @@ 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 {
-                bad_attr(meta.span);
-                continue
+                let mut err = bad_attr(meta.span);
+                err.emit();
+                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;
+            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];
+                            // 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(
+                                        &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");
+                                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
+                        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() {
@@ -245,7 +302,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 +312,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 +345,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 +429,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 +445,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
@@ -470,15 +470,15 @@ pub enum LintSource {
     Default,
 
     /// Lint level was set by an attribute.
-    Node(ast::Name, Span),
+    Node(ast::Name, Span, Option<Symbol> /* RFC 2383 reason */),
 
     /// Lint level was set by a command-line flag.
     CommandLine(Symbol),
 }
 
 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/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
@@ -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
@@ -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/empty-lint-attributes.rs 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() {}
diff --git a/src/test/ui/lint/reasons-erroneous.rs b/src/test/ui/lint/reasons-erroneous.rs
@@ -0,0 +1,24 @@
+#![feature(lint_reasons)]
+
+#![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 = "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
@@ -0,0 +1,61 @@
+error[E0452]: malformed lint attribute
+  --> $DIR/reasons-erroneous.rs:3: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:6: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: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: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:13:36
+   |
+LL | #![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))]
+   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error[E0452]: malformed lint attribute
+  --> $DIR/reasons-erroneous.rs:15:44
+   |
+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 7 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
@@ -0,0 +1,21 @@
+#![feature(lint_reasons)]
+
+#![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
@@ -0,0 +1,14 @@
+error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
+  --> $DIR/reasons-forbidden.rs:14: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
diff --git a/src/test/ui/lint/reasons.stderr b/src/test/ui/lint/reasons.stderr
