Added warning for unused arithmetic expressions #50149

aaronaaeng · 2018-04-21T22:21:25Z

The compiler now displays a warning when a binary arithmetic operation is evaluated but not used. This resolves #50124 by following the instructions outlined in the issue. The changes are as follows:

Added new pattern matching for unused arithmetic expressions in src/librustc_lint/unused.rs
Added #[must_use] attributes to the binary operation methods in src/libcore/internal_macros.rs
Added #[must_use] attributes to the non-assigning binary operators in src/libcore/ops/arith.rs

rust-highfive · 2018-04-21T22:21:28Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aidanhs (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

scottmcm · 2018-04-21T22:26:08Z

src/libcore/internal_macros.rs

@@ -38,6 +38,7 @@ macro_rules! forward_ref_binop {
    };
    (impl $imp:ident, $method:ident for $t:ty, $u:ty, #[$attr:meta]) => {
        #[$attr]
+        #[must_use]


Is the impl the right place for this attribute? I thought the lint only looked at methods on trait definitions.

Sorry, I didn't read the mentoring instructions before — this fix needs a bit of tweaking:

Instead of adding #[must_use] to method definitions in impls, you need to add them to the trait definitions, e.g. here:

rust/src/libcore/ops/arith.rs

Line 97 in 699eb95

fn add(self, rhs: RHS) -> Self::Output;

You don't need any of the #[must_use] in the macros, for the same reason.

I don't fully understand what is going on in the impl, though I'm a little unsure of where else I would put the attribute. The recurrence of #[$attr] in each impl made it seem as though the attributes need to be repeated before each possible combination of T and U states.

Oh, ok. I can go fix that. Thanks for the quick support. I also just learned that even though GitHub will automatically show new messages as they come in, I need to refresh to see edits.

Take a look at PartialOrd, which implements lt (<):

rust/src/libcore/cmp.rs

Lines 642 to 650 in 699eb95

#[inline]

#[must_use]

#[stable(feature = "rust1", since = "1.0.0")]

fn lt(&self, other: &Rhs) -> bool {

match self.partial_cmp(other) {

Some(Less) => true,

_ => false,

}

}

The #[must_use] attribute is on the definition in trait PartialOrd, but not on any of the impl PartialOrd. The way #[must_use] works at the moment, it's only the trait declaration which is important — everything else inherits from the trait. (That way, anyone who implements an operator will get the advantage of #[must_use] without having to add it themselves.

So, to be clear, you can add the attribute just above the stability attribute here:

rust/src/libcore/ops/arith.rs

Lines 96 to 97 in 699eb95

#[stable(feature = "rust1", since = "1.0.0")]

fn add(self, rhs: RHS) -> Self::Output;

(And the same for all the other operator-related traits.)

Ah, that was my misunderstanding, sorry. Adding #[must_use] to the trait methods affects all implementations downstream, not just arithmetic on numerical primitives.

If someone has #[deny(unused_must_use)] this would be a breaking change. I don't know what we do for expanding the scopes of lints.

varkor · 2018-04-21T22:30:14Z

src/librustc_lint/unused.rs

@@ -103,6 +103,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
                        cx.span_lint(UNUSED_MUST_USE, expr.span, msg);
                        op_warned = true;
                    },
+                    hir::BiAdd | hir::BiSub | hir::BiDiv | hir::BiMul | hir::BiRem => {


You might as well add the remaining binary operators here (https://github.com/rust-lang/rust/blob/master/src/librustc/hir/mod.rs#L1029-L1046) at the same time.

I suggested keeping the match to determine the error message.

Yeah, different messages sound good 👍
We want the remaining operators too, though (the same message as the other non-comparison operators is fine).

scottmcm · 2018-04-21T22:34:44Z

src/librustc_lint/unused.rs

@@ -103,6 +103,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnusedResults {
                        cx.span_lint(UNUSED_MUST_USE, expr.span, msg);
                        op_warned = true;
                    },
+                    hir::BiAdd | hir::BiSub | hir::BiDiv | hir::BiMul | hir::BiRem => {
+                        let msg = "unused arithmetic operation which must be used";


While you're here, maybe add the bitops too? a^b; giving a warning also makes sense to me

Unary ops too? -x seems to also fit "unused arithmetic operation which must be used"

At that point, is it just all ops? The only three left are *, &&, and ||. (The short-circuiting ones I could see an argument for not having the lint , since foo() || panic!() is possible, but...)

Edit: And @varkor beat me to at least one of the comments 😆

I think the logical and/or should still be covered by the lint as there is a result being ignored there. People should be preferring if for conditional execution.

In fact for bonus points you could print a help message in this specific case, although I would check if the right side of the op isn't just a boolean literal or something.

I agree with covering unary ops; just look at where this method is called and create an equivalent for ExprKind::UnaryOp.

I'm working on the UnaryOp pattern matching. While matching on binary operators, it matches on bin_op.node. However, the UnOp type doesn't have this field. I've tried to find the definition of this type but I've come up with nothing so far. Any suggestion as to what file I should check out?

UnOp is an enum defined here. BinOp is an enum as well but it's wrapped in a struct that gives it span information, which I assume was considered unnecessary for unary ops.

All the hir:: types are defined in src/librust/hir.mod.rs. For convenience all the enums have their variants reexported in this module as well.

aaronaaeng · 2018-04-22T01:49:49Z

I made the changes discussed. Warnings are now thrown whenever an operator is evaluated but not used. The warning is specific to the type of operator that went unused. The #[must_use] attribute has been moved to the correct location for each operator. It passes all of the x.py tests and works as intended with sample code that evaluates expressions using binary and unary operators but does not use the results.

abonander · 2018-04-22T02:37:05Z

Putting #[must_use] on the traits themselves is probably the right move but I don't know what the policy is for expanding the scope of lints. If someone has #[deny(unused_must_use)] in their crate and they made a mistake with one of these operators, this change will break their build. It's a good breakage, but a breakage nonetheless.

abonander · 2018-04-22T02:40:46Z

src/librustc_lint/unused.rs

+                    },
+                }
+            }
+            if let hir::ExprUnary(un_op, ..) = expr.node {


I would combine this and the if let hir::ExprBinary bit above into a single match, along with deduplicating the cx.span lint(...); op_warned = true; lines.

What is the best practice for combining these two match statements? Combining them seems like it would require nested matches since ExprBinary and ExprUnary are different types. I have to match on op.node for binary operators but only on op for unary operators. Should I implement this nested system or is there a better way?

A nested match is fine. It's mostly about eliminating the redundant code here.

abonander · 2018-04-22T02:42:51Z

src/librustc_lint/unused.rs

+                    hir::BiAdd | hir::BiSub | hir::BiDiv | hir::BiMul | hir::BiRem => {
+                        let msg = "unused arithmetic operation which must be used";
+                        cx.span_lint(UNUSED_MUST_USE, expr.span, msg);
+                        op_warned = true;


I would extract the above two lines out of each arm and just have each arm return its relevant message and then emit the lint at the end.

abonander · 2018-04-22T02:43:41Z

src/librustc_lint/unused.rs

+                    hir::BiAnd | hir::BiOr => {
+                        let msg = "unused logical operation which must be used";
+                        cx.span_lint(UNUSED_MUST_USE, expr.span, msg);
+                        op_warned = true;


For bonus points you could emit a help message here saying to use if (first_op) { second_op } instead, but that's a bit involved. I might tack that on myself in a separate PR if you don't want to get into it.

abonander · 2018-04-22T02:48:15Z

I don't see a lot of UI tests for lints in general so I don't know how we want to test these additions.

zackmdavis · 2018-04-22T04:52:06Z

Putting #[must_use] on the traits themselves is probably the right move but

Note that #[must_use] for functions is still unstable. (Although probably not for long: the stabilization final comment period ended with no objections, and who can say but that I might even have time to rebase the stabilization PR tomorrow??)

I don't know what the policy is for expanding the scope of lints

Given that RFC 1193 lint capping exists specifically to preven the breakage from spreading to dependent crates, it's probably OK sometimes (but is definitely OK here given fn_must_use not even being stable). (See #48717 for an example of Crater-detected breakage from a lint behavior change.)

I don't see a lot of UI tests for lints in general

There's a src/test/ui/lint directory, but this would probably go with rust/src/test/ui/rfc_1940-must_use_on_functions/fn_must_use.rs (which should be moved to src/test/ui/lint/ on stabilization).

abonander · 2018-04-22T05:04:28Z

@zackmdavis This change also affects the arithmetic, the logical/bitwise operators and the unary operators. Where would those lint tests go.

zackmdavis · 2018-04-22T05:14:07Z

new file src/test/ui/lint/must_use_ops.rs??

aaronaaeng · 2018-04-22T05:53:06Z

I updated the match statements to reduce redundant code. Because I am not incredibly familiar with pattern matching, I introduced a bool that says whether to send the lint message. Without this check, it could throw empty warnings on expressions that were correct.

rust-highfive · 2018-04-22T06:00:37Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:51] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:51] tidy error: /checkout/src/librustc_lint/unused.rs:95: trailing whitespace
[00:05:51] tidy error: /checkout/src/librustc_lint/unused.rs:98: trailing whitespace
[00:05:51] tidy error: /checkout/src/librustc_lint/unused.rs:99: trailing whitespace
[00:05:51] tidy error: /checkout/src/librustc_lint/unused.rs:115: trailing whitespace
[00:05:53] some tidy checks failed
[00:05:53] 
[00:05:53] 
[00:05:53] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:53] 
[00:05:53] 
[00:05:53] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:53] Build completed unsuccessfully in 0:02:33
[00:05:53] Build completed unsuccessfully in 0:02:33
[00:05:53] Makefile:79: recipe for target 'tidy' failed
[00:05:53] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:136cc378
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

zackmdavis · 2018-04-22T06:43:42Z

src/librustc_lint/unused.rs

-                    _ => {},
+            match expr.node {
+                hir::ExprBinary(bin_op, ..)  => {
+                    should_warn_op = true;


let must_use_op = match expr.node { hir::ExprBinary(bin_op, ..) => { match bin_op.node { hir::BiEq | hir::BiLt | hir::BiLe | hir::BiNe | hir::BiGe | hir::BiGt => { Some("comparison") }, hir::BiAdd | hir::BiSub | hir::BiDiv | hir::BiMul | hir::BiRem => { Some("arithmetic operation") }, hir::BiAnd | hir::BiOr => { Some("logical operation") }, hir::BiBitXor | hir::BiBitAnd | hir::BiBitOr | hir::BiShl | hir::BiShr => { Some("bitwise operation") }, } }, hir::ExprUnary(..) => { Some("unary operation") }, _ => None } if let Some(must_use_op) { cx.span_lint(UNUSED_MUST_USE, expr.span, format!("unused {} which must be used", must_use_op)); op_warned = true; }

aaronaaeng · 2018-04-22T14:43:53Z

After seeing that, it seemed pretty obvious. I made the necessary changes and it is a lot cleaner than it was. Hopefully it all looks good now.

rust-highfive · 2018-04-22T14:48:56Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:56] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:57] tidy error: /checkout/src/librustc_lint/unused.rs:117: line longer than 100 chars
[00:04:58] some tidy checks failed
[00:04:58] 
[00:04:58] 
[00:04:58] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:58] 
[00:04:58] 
[00:04:58] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:58] Build completed unsuccessfully in 0:01:59
[00:04:58] Build completed unsuccessfully in 0:01:59
[00:04:58] make: *** [tidy] Error 1
[00:04:58] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1254f026
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

zackmdavis · 2018-04-22T18:07:24Z

src/librustc_lint/unused.rs

+                        },
+                    }
+                },
+                hir::ExprUnary(..) => {


I just realized we can save two lines by writing this as hir::ExprUnary(..) => Some("unary operation"), rather than with a block (obviously this is a criticism of my earlier review comment, not you)

zackmdavis · 2018-04-22T18:14:00Z

Needs tests (probably a new UI test file at src/test/ui/lint/must_use_ops.rs or similar; don't forget the #[feature(fn_must_use)]),

For the benefit of future software-archeologists, I argue that it's also worth squashing these 6+ commits down into 1. (GitHub truncates the commit message summary line at 72 characters, so something like "make must-use lint cover binary operations" (42 chars) is better than 12d227aff's 95 chars.)

zackmdavis · 2018-04-22T18:19:00Z

src/librustc_lint/unused.rs

-                    // 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


This comment (or a rephrasing of it; the word "comparison", at least, is no longer appropriate) should be preserved. (It was specifically requested by a reviewer.)

aaronaaeng · 2018-04-22T20:15:04Z

Should I include each operator or just one from each category, i.e., do I need to include an expected warning for +, -, *, and /, or should I just include one operator from the arithmetic category?

zackmdavis · 2018-04-22T20:21:48Z

I'd say all of them. (The marginal cost (in test run times) of additional bytes of source in a test file that would exist anyway is going to be very small.)

rust-highfive · 2018-04-22T23:11:35Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:05:42] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:43] tidy error: /checkout/src/test/ui/lint/must-use-ops.rs: missing trailing newline
[00:05:43] tidy error: /checkout/src/librustc_lint/unused.rs:96: trailing whitespace
[00:05:44] some tidy checks failed
[00:05:44] 
[00:05:44] 
[00:05:44] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:44] 
[00:05:44] 
[00:05:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:44] Build completed unsuccessfully in 0:02:32
[00:05:44] Build completed unsuccessfully in 0:02:32
[00:05:44] make: *** [tidy] Error 1
[00:05:44] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0021e6f5
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

aaronaaeng · 2018-04-23T15:30:04Z

I rebased the commits into one that has a better name. This new commit also includes added tests.

zackmdavis · 2018-04-23T15:32:33Z

looks good to me (but I don't have the authority to approve PRs)

zackmdavis · 2018-04-26T04:27:41Z

wants to merge 1 commit into rust-lang:master from aaronaaeng:master

(Standard practice is to start a new branch in your own repository, rather than working on master; this surprised me just now as I began to rebase #48925 off this.)

aaronaaeng · 2018-04-26T05:36:57Z

That makes sense. I'll do that next time I submit a PR somewhere. Thank you for helping me out with all of this.

estebank · 2018-04-27T00:00:32Z

@bors r+

bors · 2018-04-27T00:00:34Z

📌 Commit 91aa267 has been approved by estebank

bors · 2018-04-28T04:14:19Z

⌛ Testing commit 91aa267 with merge 1eb0cef...

Added warning for unused arithmetic expressions The compiler now displays a warning when a binary arithmetic operation is evaluated but not used. This resolves #50124 by following the instructions outlined in the issue. The changes are as follows: - Added new pattern matching for unused arithmetic expressions in `src/librustc_lint/unused.rs` - Added `#[must_use]` attributes to the binary operation methods in `src/libcore/internal_macros.rs` - Added `#[must_use]` attributes to the non-assigning binary operators in `src/libcore/ops/arith.rs`

bors · 2018-04-28T06:26:43Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 1eb0cef to master...

leonardo-m · 2018-04-30T17:46:24Z

In the last Nightly I still don't see those warnings. Perhaps I am missing something?

kennytm · 2018-04-30T17:53:21Z

@leonardo-m You need #![feature(fn_must_use)] to see those warnings for now. After #48925 is merged (within 1 to 2 days) they should be enabled automatically.

rust-highfive assigned aidanhs Apr 21, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2018

scottmcm reviewed Apr 21, 2018

View reviewed changes

varkor reviewed Apr 21, 2018

View reviewed changes

scottmcm reviewed Apr 21, 2018

View reviewed changes

abonander reviewed Apr 22, 2018

View reviewed changes

zackmdavis reviewed Apr 22, 2018

View reviewed changes

aaronaaeng force-pushed the master branch 2 times, most recently from ee8abeb to e8864ba Compare April 22, 2018 22:59

Make must_use lint cover all binary/unary operators

91aa267

aaronaaeng force-pushed the master branch from e8864ba to 91aa267 Compare April 22, 2018 23:17

zackmdavis mentioned this pull request Apr 26, 2018

stabilize #[must_use] for functions and must-use comparison operators (RFC 1940) #48925

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2018

bors merged commit 91aa267 into rust-lang:master Apr 28, 2018

LukasKalbertodt mentioned this pull request Jul 10, 2018

Add warning about side-effect free expression-statement #32108

Closed

tormol mentioned this pull request Apr 8, 2019

wrapping_$op() and saturating_$op() should be #[must_use] #59787

Closed

lqd mentioned this pull request Jul 17, 2019

ICE: Generic type alias to invalid type crashes during type check on latest stable #62742

Closed

	#[inline]
	#[must_use]
	#[stable(feature = "rust1", since = "1.0.0")]
	fn lt(&self, other: &Rhs) -> bool {
	match self.partial_cmp(other) {
	Some(Less) => true,
	_ => false,
	}
	}

	#[stable(feature = "rust1", since = "1.0.0")]
	fn add(self, rhs: RHS) -> Self::Output;

Added warning for unused arithmetic expressions #50149

Added warning for unused arithmetic expressions #50149

Conversation

aaronaaeng commented Apr 21, 2018

rust-highfive commented Apr 21, 2018

Choose a reason for hiding this comment

varkor Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varkor Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

abonander Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronaaeng commented Apr 22, 2018

abonander commented Apr 22, 2018

abonander Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abonander Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

abonander commented Apr 22, 2018

zackmdavis commented Apr 22, 2018

abonander commented Apr 22, 2018

zackmdavis commented Apr 22, 2018

aaronaaeng commented Apr 22, 2018

rust-highfive commented Apr 22, 2018

Choose a reason for hiding this comment

aaronaaeng commented Apr 22, 2018

rust-highfive commented Apr 22, 2018

Choose a reason for hiding this comment

zackmdavis commented Apr 22, 2018

Choose a reason for hiding this comment

aaronaaeng commented Apr 22, 2018

zackmdavis commented Apr 22, 2018

rust-highfive commented Apr 22, 2018

aaronaaeng commented Apr 23, 2018

zackmdavis commented Apr 23, 2018

zackmdavis commented Apr 26, 2018

aaronaaeng commented Apr 26, 2018

estebank commented Apr 27, 2018

bors commented Apr 27, 2018

bors commented Apr 28, 2018

bors commented Apr 28, 2018

leonardo-m commented Apr 30, 2018

kennytm commented Apr 30, 2018

varkor Apr 21, 2018 •

edited

Loading

varkor Apr 21, 2018 •

edited

Loading

scottmcm Apr 21, 2018 •

edited

Loading

abonander Apr 21, 2018 •

edited

Loading

abonander Apr 22, 2018 •

edited

Loading

abonander Apr 22, 2018 •

edited

Loading