Rollup merge of #43776 - zackmdavis:feature_gate_fn_must_use, r=alexcrichton

feature-gate #[must_use] for functions as `fn_must_use`

@eddyb I [was](#43728 (comment)) [dithering](#43728 (comment)) on this, but [your comment](#43302 (comment)) makes it sound like we do want a feature gate for this? Please advise.

r? @eddyb

Author: frewsxcv

src/librustc_lint/unused.rs

@@ -160,21 +160,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
         };
 
         let mut fn_warned = false;
-        let maybe_def = match expr.node {
-            hir::ExprCall(ref callee, _) => {
-                match callee.node {
-                    hir::ExprPath(ref qpath) => Some(cx.tables.qpath_def(qpath, callee.hir_id)),
-                    _ => None
-                }
-            },
-            hir::ExprMethodCall(..) => {
-                cx.tables.type_dependent_defs().get(expr.hir_id).cloned()
-            },
-            _ => { None }
-        };
-        if let Some(def) = maybe_def {
-            let def_id = def.def_id();
-            fn_warned = check_must_use(cx, def_id, s.span, "return value of ");
+        if cx.tcx.sess.features.borrow().fn_must_use {
+            let maybe_def = match expr.node {
+                hir::ExprCall(ref callee, _) => {
+                    match callee.node {
+                        hir::ExprPath(ref qpath) => {
+                            Some(cx.tables.qpath_def(qpath, callee.hir_id))
+                        },
+                        _ => None
+                    }
+                },
+                hir::ExprMethodCall(..) => {
+                    cx.tables.type_dependent_defs().get(expr.hir_id).cloned()
+                },
+                _ => None
+            };
+            if let Some(def) = maybe_def {
+                let def_id = def.def_id();
+                fn_warned = check_must_use(cx, def_id, s.span, "return value of ");
+            }
         }
 
         if !(ty_warned || fn_warned) {

src/libsyntax/feature_gate.rs

@@ -112,8 +112,8 @@ macro_rules! declare_features {
 // was set. This is most important for knowing when a particular feature became
 // stable (active).
 //
-// NB: The featureck.py script parses this information directly out of the source
-// so take care when modifying it.
+// NB: tools/tidy/src/features.rs parses this information directly out of the
+// source, so take care when modifying it.
 
 declare_features! (
     (active, asm, "1.0.0", Some(29722)),
@@ -372,6 +372,9 @@ declare_features! (
 
     // #[doc(cfg(...))]
     (active, doc_cfg, "1.21.0", Some(43781)),
+
+    // allow `#[must_use]` on functions (RFC 1940)
+    (active, fn_must_use, "1.21.0", Some(43302)),
 );
 
 declare_features! (
@@ -915,20 +918,27 @@ struct Context<'a> {
 }
 
 macro_rules! gate_feature_fn {
-    ($cx: expr, $has_feature: expr, $span: expr, $name: expr, $explain: expr) => {{
-        let (cx, has_feature, span, name, explain) = ($cx, $has_feature, $span, $name, $explain);
+    ($cx: expr, $has_feature: expr, $span: expr, $name: expr, $explain: expr, $level: expr) => {{
+        let (cx, has_feature, span,
+             name, explain, level) = ($cx, $has_feature, $span, $name, $explain, $level);
         let has_feature: bool = has_feature(&$cx.features);
         debug!("gate_feature(feature = {:?}, span = {:?}); has? {}", name, span, has_feature);
         if !has_feature && !span.allows_unstable() {
-            emit_feature_err(cx.parse_sess, name, span, GateIssue::Language, explain);
+            leveled_feature_err(cx.parse_sess, name, span, GateIssue::Language, explain, level)
+                .emit();
         }
     }}
 }
 
 macro_rules! gate_feature {
     ($cx: expr, $feature: ident, $span: expr, $explain: expr) => {
-        gate_feature_fn!($cx, |x:&Features| x.$feature, $span, stringify!($feature), $explain)
-    }
+        gate_feature_fn!($cx, |x:&Features| x.$feature, $span,
+                         stringify!($feature), $explain, GateStrength::Hard)
+    };
+    ($cx: expr, $feature: ident, $span: expr, $explain: expr, $level: expr) => {
+        gate_feature_fn!($cx, |x:&Features| x.$feature, $span,
+                         stringify!($feature), $explain, $level)
+    };
 }
 
 impl<'a> Context<'a> {
@@ -938,7 +948,7 @@ impl<'a> Context<'a> {
         for &(n, ty, ref gateage) in BUILTIN_ATTRIBUTES {
             if name == n {
                 if let Gated(_, name, desc, ref has_feature) = *gateage {
-                    gate_feature_fn!(self, has_feature, attr.span, name, desc);
+                    gate_feature_fn!(self, has_feature, attr.span, name, desc, GateStrength::Hard);
                 }
                 debug!("check_attribute: {:?} is builtin, {:?}, {:?}", attr.path, ty, gateage);
                 return;
@@ -1008,24 +1018,42 @@ pub enum GateIssue {
     Library(Option<u32>)
 }
 
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum GateStrength {
+    /// A hard error. (Most feature gates should use this.)
+    Hard,
+    /// Only a warning. (Use this only as backwards-compatibility demands.)
+    Soft,
+}
+
 pub fn emit_feature_err(sess: &ParseSess, feature: &str, span: Span, issue: GateIssue,
                         explain: &str) {
     feature_err(sess, feature, span, issue, explain).emit();
 }
 
 pub fn feature_err<'a>(sess: &'a ParseSess, feature: &str, span: Span, issue: GateIssue,
-                   explain: &str) -> DiagnosticBuilder<'a> {
+                       explain: &str) -> DiagnosticBuilder<'a> {
+    leveled_feature_err(sess, feature, span, issue, explain, GateStrength::Hard)
+}
+
+fn leveled_feature_err<'a>(sess: &'a ParseSess, feature: &str, span: Span, issue: GateIssue,
+                           explain: &str, level: GateStrength) -> DiagnosticBuilder<'a> {
     let diag = &sess.span_diagnostic;
 
     let issue = match issue {
         GateIssue::Language => find_lang_feature_issue(feature),
         GateIssue::Library(lib) => lib,
     };
 
-    let mut err = if let Some(n) = issue {
-        diag.struct_span_err(span, &format!("{} (see issue #{})", explain, n))
+    let explanation = if let Some(n) = issue {
+        format!("{} (see issue #{})", explain, n)
     } else {
-        diag.struct_span_err(span, explain)
+        explain.to_owned()
+    };
+
+    let mut err = match level {
+        GateStrength::Hard => diag.struct_span_err(span, &explanation),
+        GateStrength::Soft => diag.struct_span_warn(span, &explanation),
     };
 
     // #23973: do not suggest `#![feature(...)]` if we are in beta/stable
@@ -1035,7 +1063,15 @@ pub fn feature_err<'a>(sess: &'a ParseSess, feature: &str, span: Span, issue: Ga
                           feature));
     }
 
+    // If we're on stable and only emitting a "soft" warning, add a note to
+    // clarify that the feature isn't "on" (rather than being on but
+    // warning-worthy).
+    if !sess.unstable_features.is_nightly_build() && level == GateStrength::Soft {
+        err.help("a nightly build of the compiler is required to enable this feature");
+    }
+
     err
+
 }
 
 const EXPLAIN_BOX_SYNTAX: &'static str =
@@ -1092,6 +1128,12 @@ macro_rules! gate_feature_post {
         if !span.allows_unstable() {
             gate_feature!(cx.context, $feature, span, $explain)
         }
+    }};
+    ($cx: expr, $feature: ident, $span: expr, $explain: expr, $level: expr) => {{
+        let (cx, span) = ($cx, $span);
+        if !span.allows_unstable() {
+            gate_feature!(cx.context, $feature, span, $explain, $level)
+        }
     }}
 }
 
@@ -1234,6 +1276,11 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
                                         function may change over time, for now \
                                         a top-level `fn main()` is required");
                 }
+                if attr::contains_name(&i.attrs[..], "must_use") {
+                    gate_feature_post!(&self, fn_must_use, i.span,
+                                       "`#[must_use]` on functions is experimental",
+                                       GateStrength::Soft);
+                }
             }
 
             ast::ItemKind::Struct(..) => {
@@ -1271,7 +1318,7 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
                                     and possibly buggy");
             }
 
-            ast::ItemKind::Impl(_, polarity, defaultness, _, _, _, _) => {
+            ast::ItemKind::Impl(_, polarity, defaultness, _, _, _, ref impl_items) => {
                 if polarity == ast::ImplPolarity::Negative {
                     gate_feature_post!(&self, optin_builtin_traits,
                                        i.span,
@@ -1284,6 +1331,16 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {
                                        i.span,
                                        "specialization is unstable");
                 }
+
+                for impl_item in impl_items {
+                    if let ast::ImplItemKind::Method(..) = impl_item.node {
+                        if attr::contains_name(&impl_item.attrs[..], "must_use") {
+                            gate_feature_post!(&self, fn_must_use, impl_item.span,
+                                               "`#[must_use]` on methods is experimental",
+                                               GateStrength::Soft);
+                        }
+                    }
+                }
             }
 
             ast::ItemKind::MacroDef(ast::MacroDef { legacy: false, .. }) => {

src/test/compile-fail/feature-gate-fn_must_use.rs

@@ -0,0 +1,31 @@
+// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(rustc_attrs)]
+
+struct MyStruct;
+
+impl MyStruct {
+    #[must_use]
+    fn need_to_use_method() -> bool { true } //~ WARN `#[must_use]` on methods is experimental
+}
+
+#[must_use]
+fn need_to_use_it() -> bool { true } //~ WARN `#[must_use]` on functions is experimental
+
+
+// Feature gates are tidy-required to have a specially named (or
+// comment-annotated) compile-fail test (which MUST fail), but for
+// backwards-compatibility reasons, we want `#[must_use]` on functions to be
+// compilable even if the `fn_must_use` feature is absent, thus necessitating
+// the usage of `#[rustc_error]` here, pragmatically if awkwardly solving this
+// dilemma until a superior solution can be devised.
+#[rustc_error]
+fn main() {} //~ ERROR compilation successful

src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs

@@ -680,6 +680,7 @@ mod must_use {
     mod inner { #![must_use="1400"] }
 
     #[must_use = "1400"] fn f() { }
+    //~^ WARN `#[must_use]` on functions is experimental
 
     #[must_use = "1400"] struct S;
 

src/test/ui/lint/fn_must_use.rs

@@ -8,6 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
+#![feature(fn_must_use)]
 #![warn(unused_must_use)]
 
 struct MyStruct {

src/test/ui/lint/fn_must_use.stderr

@@ -1,18 +1,18 @@
 warning: unused return value of `need_to_use_this_value` which must be used: it's important
-  --> $DIR/fn_must_use.rs:30:5
+  --> $DIR/fn_must_use.rs:31:5
    |
-30 |     need_to_use_this_value();
+31 |     need_to_use_this_value();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^
    |
 note: lint level defined here
-  --> $DIR/fn_must_use.rs:11:9
+  --> $DIR/fn_must_use.rs:12:9
    |
-11 | #![warn(unused_must_use)]
+12 | #![warn(unused_must_use)]
    |         ^^^^^^^^^^^^^^^
 
 warning: unused return value of `MyStruct::need_to_use_this_method_value` which must be used
-  --> $DIR/fn_must_use.rs:33:5
+  --> $DIR/fn_must_use.rs:34:5
    |
-33 |     m.need_to_use_this_method_value();
+34 |     m.need_to_use_this_method_value();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^