From 0981211c629d0d9f59c5164d8e2c42974d9449e2 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Tue, 15 Aug 2017 16:21:28 -0700 Subject: [PATCH 1/4] hard feature-gate for #[must_use] on functions We'll actually want a new "soft" warning-only gate to maintain backwards-compatibility, but it's cleaner to start out with the established, well-understood gate before implementing the alternative warn-only behavior in a later commit. This is in the matter of #43302. --- src/librustc_lint/unused.rs | 34 +++++++++++-------- src/libsyntax/feature_gate.rs | 9 ++++- .../compile-fail/feature-gate-fn_must_use.rs | 14 ++++++++ .../issue-43106-gating-of-builtin-attrs.rs | 1 + src/test/ui/lint/fn_must_use.rs | 1 + src/test/ui/lint/fn_must_use.stderr | 12 +++---- 6 files changed, 49 insertions(+), 22 deletions(-) create mode 100644 src/test/compile-fail/feature-gate-fn_must_use.rs diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 195bd2acce0fe..cbc4ebe90fd09 100644 --- a/src/librustc_lint/unused.rs +++ b/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) { diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 801343689b728..ee791609e8f79 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -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! ( @@ -1014,7 +1017,7 @@ pub fn emit_feature_err(sess: &ParseSess, feature: &str, span: Span, issue: Gate } pub fn feature_err<'a>(sess: &'a ParseSess, feature: &str, span: Span, issue: GateIssue, - explain: &str) -> DiagnosticBuilder<'a> { + explain: &str) -> DiagnosticBuilder<'a> { let diag = &sess.span_diagnostic; let issue = match issue { @@ -1234,6 +1237,10 @@ 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"); + } } ast::ItemKind::Struct(..) => { diff --git a/src/test/compile-fail/feature-gate-fn_must_use.rs b/src/test/compile-fail/feature-gate-fn_must_use.rs new file mode 100644 index 0000000000000..7bb533251dc08 --- /dev/null +++ b/src/test/compile-fail/feature-gate-fn_must_use.rs @@ -0,0 +1,14 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#[must_use] +fn need_to_use_it() -> bool { true } //~ ERROR `#[must_use]` on functions is experimental + +fn main() {} diff --git a/src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs b/src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs index 29a2b0609fcd2..021daf88420d1 100644 --- a/src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs +++ b/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() { } + //~^ ERROR `#[must_use]` on functions is experimental #[must_use = "1400"] struct S; diff --git a/src/test/ui/lint/fn_must_use.rs b/src/test/ui/lint/fn_must_use.rs index 5aea5f2ca0642..c549ded4db218 100644 --- a/src/test/ui/lint/fn_must_use.rs +++ b/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 { diff --git a/src/test/ui/lint/fn_must_use.stderr b/src/test/ui/lint/fn_must_use.stderr index 20eb7452aea71..242837793a0bf 100644 --- a/src/test/ui/lint/fn_must_use.stderr +++ b/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(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 7b6e9b4b8424e8ef722a3fa6388fe3bc8414bab0 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Tue, 15 Aug 2017 18:52:04 -0700 Subject: [PATCH 2/4] correct comment re feature-checking tooling The featureck.py that this comment referred to was removed in 9dd3c54a (March 2016). --- src/libsyntax/feature_gate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index ee791609e8f79..c82f0554393dc 100644 --- a/src/libsyntax/feature_gate.rs +++ b/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)), From 8492ad2479379d2e07ccf2d3439ec29b19d164b8 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Tue, 22 Aug 2017 16:05:01 -0700 Subject: [PATCH 3/4] "soft" (warn instead of error) feature-gate for #[must_use] on functions Before `#[must_use]` for functions was implemented, a `#[must_use]` attribute on a function was a no-op. To avoid a breaking change in this behavior, we add an option for "this-and-such feature is experimental" feature-gate messages to be a mere warning rather than a compilation-halting failure (so old code that used to have a useless no-op `#[must_use]` attribute now warns rather than breaking). When we're on stable, we add a help note to clarify that the feature isn't "on." This is in support of #43302. --- src/libsyntax/feature_gate.rs | 60 +++++++++++++++---- .../compile-fail/feature-gate-fn_must_use.rs | 14 ++++- .../issue-43106-gating-of-builtin-attrs.rs | 2 +- 3 files changed, 63 insertions(+), 13 deletions(-) diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index c82f0554393dc..79f15f2fe1958 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -918,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> { @@ -941,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; @@ -1011,6 +1018,14 @@ pub enum GateIssue { Library(Option) } +#[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(); @@ -1018,6 +1033,11 @@ pub fn emit_feature_err(sess: &ParseSess, feature: &str, span: Span, issue: Gate pub fn feature_err<'a>(sess: &'a ParseSess, feature: &str, span: Span, issue: GateIssue, 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 { @@ -1025,10 +1045,15 @@ pub fn feature_err<'a>(sess: &'a ParseSess, feature: &str, span: Span, issue: Ga 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 @@ -1038,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 = @@ -1095,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) + } }} } @@ -1239,7 +1278,8 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> { } if attr::contains_name(&i.attrs[..], "must_use") { gate_feature_post!(&self, fn_must_use, i.span, - "`#[must_use]` on functions is experimental"); + "`#[must_use]` on functions is experimental", + GateStrength::Soft); } } diff --git a/src/test/compile-fail/feature-gate-fn_must_use.rs b/src/test/compile-fail/feature-gate-fn_must_use.rs index 7bb533251dc08..a222f36614535 100644 --- a/src/test/compile-fail/feature-gate-fn_must_use.rs +++ b/src/test/compile-fail/feature-gate-fn_must_use.rs @@ -8,7 +8,17 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +#![feature(rustc_attrs)] + #[must_use] -fn need_to_use_it() -> bool { true } //~ ERROR `#[must_use]` on functions is experimental +fn need_to_use_it() -> bool { true } //~ WARN `#[must_use]` on functions is experimental + -fn main() {} +// 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 diff --git a/src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs b/src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs index 021daf88420d1..204190d64acc1 100644 --- a/src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs +++ b/src/test/compile-fail/feature-gate/issue-43106-gating-of-builtin-attrs.rs @@ -680,7 +680,7 @@ mod must_use { mod inner { #![must_use="1400"] } #[must_use = "1400"] fn f() { } - //~^ ERROR `#[must_use]` on functions is experimental + //~^ WARN `#[must_use]` on functions is experimental #[must_use = "1400"] struct S; From 35c449419cfbd2bc1abc81077c95ef43ed766f97 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Tue, 22 Aug 2017 17:27:00 -0700 Subject: [PATCH 4/4] fn_must_use soft feature-gate warning on methods too, not only functions This continues to be in the matter of #43302. --- src/libsyntax/feature_gate.rs | 12 +++++++++++- src/test/compile-fail/feature-gate-fn_must_use.rs | 7 +++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 79f15f2fe1958..09574d5ba129e 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -1318,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, @@ -1331,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, .. }) => { diff --git a/src/test/compile-fail/feature-gate-fn_must_use.rs b/src/test/compile-fail/feature-gate-fn_must_use.rs index a222f36614535..2dd6b90407267 100644 --- a/src/test/compile-fail/feature-gate-fn_must_use.rs +++ b/src/test/compile-fail/feature-gate-fn_must_use.rs @@ -10,6 +10,13 @@ #![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