From 8917616e6a295fb4080c8e29362dc1e5a477c479 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Fri, 22 Sep 2017 15:45:47 -0700 Subject: [PATCH 1/2] add comparison operators to must-use lint (under `fn_must_use` feature) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Although RFC 1940 is about annotating functions with `#[must_use]`, a key part of the motivation was linting unused equality operators. (See https://github.com/rust-lang/rfcs/pull/1812#issuecomment-265695898—it seems to have not been clear to discussants at the time that marking the comparison methods as `must_use` would not give us the lints on comparison operators, at least in (what the present author understood as) the most straightforward implementation, as landed in #43728 (3645b062).) To rectify the situation, we here lint unused comparison operators as part of the unused-must-use lint (feature gated by the `fn_must_use` feature flag, which now arguably becomes a slight (tolerable in the opinion of the present author) misnomer). This is in the matter of #43302. --- src/librustc_lint/unused.rs | 18 +++++++++++++++++- src/libsyntax/feature_gate.rs | 2 +- .../fn_must_use.rs | 7 +++---- .../fn_must_use.stderr | 8 +++++++- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 439cc3a4b84..b97920dd18b 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -153,6 +153,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { }; let mut fn_warned = false; + let mut op_warned = false; if cx.tcx.sess.features.borrow().fn_must_use { let maybe_def = match expr.node { hir::ExprCall(ref callee, _) => { @@ -172,9 +173,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults { let def_id = def.def_id(); fn_warned = check_must_use(cx, def_id, s.span, "return value of "); } + + if let hir::ExprBinary(bin_op, ..) = expr.node { + match bin_op.node { + // Hardcoding the comparison operators here seemed more + // expedient than the refactoring that would be needed to + // look up the `#[must_use]` attribute which does exist on + // the comparison trait methods + hir::BiEq | hir::BiLt | hir::BiLe | hir::BiNe | hir::BiGe | hir::BiGt => { + let msg = "unused comparison which must be used"; + cx.span_lint(UNUSED_MUST_USE, expr.span, msg); + op_warned = true; + }, + _ => {}, + } + } } - if !(ty_warned || fn_warned) { + if !(ty_warned || fn_warned || op_warned) { cx.span_lint(UNUSED_RESULTS, s.span, "unused result"); } diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 1fef382c83a..5c730aaa8d0 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -380,7 +380,7 @@ declare_features! ( // #[doc(masked)] (active, doc_masked, "1.21.0", None), - // allow `#[must_use]` on functions (RFC 1940) + // allow `#[must_use]` on functions and comparison operators (RFC 1940) (active, fn_must_use, "1.21.0", Some(43302)), // allow '|' at beginning of match arms (RFC 1925) diff --git a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs index 7eb4c32972a..821cd30c8df 100644 --- a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs +++ b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs @@ -61,10 +61,9 @@ fn main() { m.need_to_use_this_method_value(); m.is_even(); // trait method! - m.replace(3); + m.replace(3); // won't warn (annotation needs to be in trait definition) - 2.eq(&3); + 2.eq(&3); // comparison methods are `must_use` - // FIXME: operators should probably be `must_use` if underlying method is - 2 == 3; + 2 == 3; // lint includes comparison operators } diff --git a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr index 69755c89b48..7fc0a4ce1ed 100644 --- a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr +++ b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr @@ -25,6 +25,12 @@ warning: unused return value of `EvenNature::is_even` which must be used: no sid warning: unused return value of `std::cmp::PartialEq::eq` which must be used --> $DIR/fn_must_use.rs:66:5 | -66 | 2.eq(&3); +66 | 2.eq(&3); // comparison methods are `must_use` | ^^^^^^^^^ +warning: unused comparison which must be used + --> $DIR/fn_must_use.rs:68:5 + | +68 | 2 == 3; // lint includes comparison operators + | ^^^^^^ + From 6734d39b49dd00fe5ba1ea875a3e03f9eca23ac0 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Sat, 23 Sep 2017 10:11:39 -0700 Subject: [PATCH 2/2] update `fn_must_use` UI test to exercise nonprimitive comparisons --- .../fn_must_use.rs | 11 +++++-- .../fn_must_use.stderr | 32 +++++++++++++------ 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs index 821cd30c8df..3741ba4f3ae 100644 --- a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs +++ b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs @@ -11,6 +11,7 @@ #![feature(fn_must_use)] #![warn(unused_must_use)] +#[derive(PartialEq, Eq)] struct MyStruct { n: usize, } @@ -58,12 +59,18 @@ fn main() { need_to_use_this_value(); let mut m = MyStruct { n: 2 }; + let n = MyStruct { n: 3 }; + m.need_to_use_this_method_value(); m.is_even(); // trait method! m.replace(3); // won't warn (annotation needs to be in trait definition) - 2.eq(&3); // comparison methods are `must_use` + // comparison methods are `must_use` + 2.eq(&3); + m.eq(&n); - 2 == 3; // lint includes comparison operators + // lint includes comparison operators + 2 == 3; + m == n; } diff --git a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr index 7fc0a4ce1ed..fdd0a591bc7 100644 --- a/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr +++ b/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.stderr @@ -1,7 +1,7 @@ warning: unused return value of `need_to_use_this_value` which must be used: it's important - --> $DIR/fn_must_use.rs:58:5 + --> $DIR/fn_must_use.rs:59:5 | -58 | need_to_use_this_value(); +59 | need_to_use_this_value(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ | note: lint level defined here @@ -11,26 +11,38 @@ note: lint level defined here | ^^^^^^^^^^^^^^^ warning: unused return value of `MyStruct::need_to_use_this_method_value` which must be used - --> $DIR/fn_must_use.rs:61:5 + --> $DIR/fn_must_use.rs:64:5 | -61 | m.need_to_use_this_method_value(); +64 | m.need_to_use_this_method_value(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ warning: unused return value of `EvenNature::is_even` which must be used: no side effects - --> $DIR/fn_must_use.rs:62:5 + --> $DIR/fn_must_use.rs:65:5 | -62 | m.is_even(); // trait method! +65 | m.is_even(); // trait method! | ^^^^^^^^^^^^ warning: unused return value of `std::cmp::PartialEq::eq` which must be used - --> $DIR/fn_must_use.rs:66:5 + --> $DIR/fn_must_use.rs:70:5 | -66 | 2.eq(&3); // comparison methods are `must_use` +70 | 2.eq(&3); + | ^^^^^^^^^ + +warning: unused return value of `std::cmp::PartialEq::eq` which must be used + --> $DIR/fn_must_use.rs:71:5 + | +71 | m.eq(&n); | ^^^^^^^^^ warning: unused comparison which must be used - --> $DIR/fn_must_use.rs:68:5 + --> $DIR/fn_must_use.rs:74:5 | -68 | 2 == 3; // lint includes comparison operators +74 | 2 == 3; + | ^^^^^^ + +warning: unused comparison which must be used + --> $DIR/fn_must_use.rs:75:5 + | +75 | m == n; | ^^^^^^