Modulo arithmetic #4867

mgr-inz-rafal · 2019-11-30T15:38:46Z

changelog: Added modulo_arithmetic lint

mgr-inz-rafal · 2019-11-30T15:39:15Z

Hi!
Recently I stumbled upon a problem of using modulo operator on negative values. I've been exchanging data between Python and C++ and it took me some time to find out about that inconsistency and fix a bug.

I thought it would be good to add such lint in order to:

Share the knowledge about differences in the handling of negative values with % operator
Let Rust developers easily find such places in code
Get acquainted with how to create your own lints :)

llogiq · 2019-11-30T19:54:20Z

Hi! Do you want to lint a) modulo on a negative const value or b) modulo on signed integers?

mgr-inz-rafal · 2019-12-01T14:30:49Z

Well, I guess to lint on both.

My first idea was to simply lint all modulo arithmetic, but from your question, I see that it can indeed be done in a more sophisticated way, i.e. no lint on positive const and/or unsigned integers. I'll see what I can do about it.

llogiq · 2019-12-01T18:25:36Z

An easy thing to do would be linting on ExprKind::Binary with an op of BinaryOp::Rem where the left-hand side operand's type (which you can get with cx.tcx.expr_ty(_)) is TyKind::Int(_) (any signed integer type).

mgr-inz-rafal · 2019-12-02T14:01:10Z

I had issues with cx.tcx.expr_ty(_) and TyKind::Int(_), maybe due to some changes in API. But by looking at the implementation of other lints I came up with the solution using cx.tables.expr_ty and is_integral() and alike functions. Implementation is now pushed to the branch.

BTW: I see the build failing on the last commit with only changes in comments introduced. This is probably not related to this PR.

Additionally, I would like this lint not to warn on const values when both sides are either positive or negative, such as (123+5)%(-100+200). Any hints on how I should approach that will be greatly appreciated.

Two more things:

I addition to ExprKind::Binary I also check on ExprKind::AssignOp
I also check for floating point numbers, since apparently they may cause problems as well, for example -18.3 % 6.7

llogiq · 2019-12-02T14:36:24Z

There's a rustup in the queue, which means there's likely breakage and you may have to rebase after we merge it.

tests/ui/modulo_arithmetic.rs

flip1995 · 2019-12-03T08:43:29Z

Additionally, I would like this lint not to warn on const values when both sides are either positive or negative

You can use functions from the clippy_lints/src/const.rs module. I think you want to use the constant(..) function to evaluate if an expression is a constant and what its value is.

I had issues with cx.tcx.expr_ty(_) and TyKind::Int(_)

This code should work:

use rustc::ty;

if let ty::Int(_) = cx.tcx.expr_ty(..).kind {
    // lint
}

mgr-inz-rafal · 2019-12-03T21:25:38Z

Thanks for you suggestions. I modified the code not to lint on consts that give predictable results (both operand of the same sign). This applies to both signed integral and floating point types.

bors · 2019-12-08T02:42:51Z

☔ The latest upstream changes (presumably #4889) made this pull request unmergeable. Please resolve the merge conflicts.

mgr-inz-rafal · 2019-12-10T18:48:21Z

After rebase and fine-tuning the build is finally green :)

flip1995

LGTM now. Some nitpicking.

clippy_lints/src/modulo_arithmetic.rs

krishna-veerareddy · 2019-12-12T20:04:37Z

clippy_lints/src/modulo_arithmetic.rs

+                                        "you are using modulo operator on constants with different signs: `{} % {}`", lhs_negative.1.unwrap(), rhs_negative.1.unwrap()
+                                    ),
+                                    expr.span,
+                                    "double check for expected result especially when interoperating with different languages",


What are your thoughts on also suggesting usage of rem_euclid or checked_rem_euclid when the signs are different? Sometime that's what the user intended when performing modulo arithmetic on signed integers and they may not be aware that this method exists.

Well, I myself was not aware that such functions exist :)
Let me check how they relate to the content of this PR.

OK, so I see that wrapping_rem_euclid() and checked_rem_euclid() yield results that are consistent with what we can see in Python.

I will add info about these methods in the lint note later today.

Thanks!

BTW: Do I see correctly that these functions are available for integral types only?

Yup these methods are only for integer types so we would only want to suggest these when operating on integers.

Relevant changes are now pushed to the branch.

krishna-veerareddy · 2019-12-15T21:27:58Z

clippy_lints/src/modulo_arithmetic.rs

+declare_clippy_lint! {
+    /// **What it does:** Checks for modulo arithemtic.
+    ///
+    /// **Why is this bad?** The results of moudlo (%) operation migth differ


Hey you have some typos here: modulo and might.

Good catch, thanks! Fixed in 6084c53

mgr-inz-rafal · 2019-12-18T22:50:26Z

To be honest, I don't quite like the fragment below (too much repetition) and I would like to merge the match arms. What would be the best way to do it?

        Some((Constant::F32(f), _)) => {
            return Some(OperandInfo {
                string_representation: Some(format!("{:.3}", f)),
                is_negative: f < 0.0,
                is_integral: false,
            });
        },
        Some((Constant::F64(f), _)) => {
            return Some(OperandInfo {
                string_representation: Some(format!("{:.3}", f)),
                is_negative: f < 0.0,
                is_integral: false,
            });
        },

mgr-inz-rafal · 2019-12-19T20:23:45Z

Regarding my previous comment, I reduced the code duplication by extracting the method, see 56f507b

bors · 2019-12-22T09:56:33Z

☔ The latest upstream changes (presumably #4930) made this pull request unmergeable. Please resolve the merge conflicts.

flip1995

Looks great now. Only formatting and using another lint function.

flip1995 · 2019-12-22T10:39:02Z

clippy_lints/src/modulo_arithmetic.rs

+                                            lhs_operand.string_representation.unwrap(),
+                                            rhs_operand.string_representation.unwrap()


NIT: 1 indentation level too much

Reduced by extracting helper functions: check_const_operands() and check_non_const_operands()

flip1995 · 2019-12-22T10:42:46Z

clippy_lints/src/modulo_arithmetic.rs

+                                    expr.span,
+                                    &format!(
+                                        "double check for expected result especially when interoperating with different languages{}",
+                                            if lhs_operand.is_integral {" or consider using `rem_euclid` or similar function"} else {""}


You should use the function

span_lint_and_then( .., |db| { db.note("double check for expected result especially when interoperating with different languages"); if lhs_operand.is_integral { db.note("or consider using `rem_euclid` or a similar function"); } }, );

OK, now using span_lint_and_then() and db.note() functions

mgr-inz-rafal · 2019-12-22T11:09:19Z

Looks like I polluted the PR with rebase... I'll try to clean this up, but if that doesn't work I'll close the PR and create a fresh one with only the necessary changes.

flip1995 · 2019-12-22T11:19:32Z

If you want, I can fix it for you real quick. Or you can do it with:

# assuming your on the modulo_arithmetic branch
$ git fetch upstream  # assuming upstream is the remote of this repo
$ git rebase upstream/master  # you may have to fix some merge conflicts:
$ git mergetool
$ git rebase --continue  # repeat until no more merge conflicts
$ git diff mgr-inz-rafal/modulo_arithmetic  # should be empty
$ git push --force-with-lease

mgr-inz-rafal · 2019-12-22T11:30:14Z

@flip1995 I'll be more than grateful if you can fix it if it's not too much hassle.

I'm far from being a git wizard and copy&pasting your commands without a full understanding doesn't sound like a great idea.

flip1995 · 2019-12-22T11:31:10Z

copy&pasting your commands without a full understanding doesn't sound like a great idea.

Yeah most of the time it isn't.

Fixed it :)

krishna-veerareddy · 2019-12-22T16:36:22Z

Hey since you have a huge number of commits consider squashing them into a single commit. Not sure what the squash policy of this repository is though so maybe one of the maintainers could chip in.

krishna-veerareddy · 2019-12-22T16:57:47Z

tests/ui/modulo_arithmetic.rs

@@ -26,7 +26,6 @@ fn main() {
    1i16 % -2i16;
    -1i32 % 2i32;
    1i32 % -2i32;
-    -1i64 % 2i64;


Please don't remove test cases to satisfy the linter. You can move the test cases into separate files like so: modulo_arithmetic_integer.rs and modulo_arithmetic_float.rs to keep the file sizes within bounds. Having the modulo_arithmetic prefix or any other prefix in your test case file name ensures that both files are tested by running TESTNAME=modulo_arithmetic cargo uitest.

Thanks for the hint, didn't know it works that way. I will reintroduce the removed tests.

Done in 8d3efd6

mgr-inz-rafal · 2019-12-23T09:56:53Z

Hey since you have a huge number of commits consider squashing them into a single commit. Not sure what the squash policy of this repository is though so maybe one of the maintainers could chip in.

This PR turned out to require many more commits than I initially anticipated. Even though my personal preference is to keep the commits small, let's wait for the maintainers' decision indeed.

flip1995 · 2019-12-23T16:17:01Z

Thanks, LGTM now. We prefer to have less commits. I usually make 2 commits for fixing something: 1 for the fix and 1 for tests. For new lints I usually use 3-5 commits. 1 for the implementation, 1 for tests, 1 for running commands for updating lints, formatting, ... and a few additional commits to fix something if necessary.

It would be great if you could squash your commits down to under 10 approximately.

mgr-inz-rafal · 2019-12-23T20:07:54Z

It would be great if you could squash your commits down to under 10 approximately.

Due to my previous "messy" operation, there are currently duplicated commits in the PR, for example:
74cc7e1
ba537da
Like part of the history is duplicated...

How would you recommend progressing with the squash operation in such a scenario? My first idea was just to squash the duplicated commits respectively and create squashed duplicates (which I will further squash into single commit) , but maybe there is a better way...

flip1995 · 2019-12-24T13:21:26Z

I'd recommend to just squash everything in one commit, then git reset HEAD~1 and create new file based commits (lint module, tests, everything else)

bors · 2019-12-24T13:51:42Z

☔ The latest upstream changes (presumably #4885) made this pull request unmergeable. Please resolve the merge conflicts.

mgr-inz-rafal · 2019-12-26T12:48:26Z

Code rebased, commits squashed and cleaned-up.

phansch · 2019-12-27T20:06:19Z

@bors r=flip1995

bors · 2019-12-27T20:06:20Z

📌 Commit 5346954 has been approved by flip1995

bors · 2019-12-27T20:21:48Z

⌛ Testing commit 5346954 with merge 53ae6ad...

Modulo arithmetic changelog: Added modulo_arithmetic lint

bors · 2019-12-27T20:37:19Z

💔 Test failed - checks-travis

flip1995 · 2019-12-28T10:37:17Z

@bors retry

bors · 2019-12-28T10:37:25Z

⌛ Testing commit 5346954 with merge f64c088...

Modulo arithmetic changelog: Added modulo_arithmetic lint

bors · 2019-12-28T10:50:57Z

💔 Test failed - checks-travis

flip1995 · 2019-12-28T12:00:23Z

You have to fix the warnings regarding Expr -> Expr<'_>

mgr-inz-rafal · 2019-12-28T16:26:34Z

For some reason, these warnings were reluctant to surface on my local build, but I finally managed to reproduce (and fix) them on the clean repo checkout.

flip1995 · 2019-12-28T18:26:11Z

You probably had to update to the latest rustc master.

Thanks! @bors r+

bors · 2019-12-28T18:26:12Z

📌 Commit a208906 has been approved by flip1995

bors · 2019-12-28T18:26:19Z

⌛ Testing commit a208906 with merge 8e493c6...

Modulo arithmetic changelog: Added modulo_arithmetic lint

bors · 2019-12-28T18:41:35Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 8e493c6 to master...

mgr-inz-rafal · 2019-12-28T18:51:57Z

Thank you, I've learned a lot here ;)

flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Dec 1, 2019

llogiq reviewed Dec 2, 2019

View reviewed changes

tests/ui/modulo_arithmetic.rs Outdated Show resolved Hide resolved

flip1995 requested changes Dec 12, 2019

View reviewed changes

krishna-veerareddy reviewed Dec 12, 2019

View reviewed changes

krishna-veerareddy reviewed Dec 15, 2019

View reviewed changes

phansch approved these changes Dec 22, 2019

View reviewed changes

flip1995 reviewed Dec 22, 2019

View reviewed changes

krishna-veerareddy reviewed Dec 22, 2019

View reviewed changes

flip1995 approved these changes Dec 23, 2019

View reviewed changes

bors added a commit that referenced this pull request Dec 27, 2019

Auto merge of #4867 - mgr-inz-rafal:modulo_arithmetic, r=flip1995

53ae6ad

Modulo arithmetic changelog: Added modulo_arithmetic lint

bors added a commit that referenced this pull request Dec 28, 2019

Auto merge of #4867 - mgr-inz-rafal:modulo_arithmetic, r=flip1995

f64c088

Modulo arithmetic changelog: Added modulo_arithmetic lint

mgr-inz-rafal added 3 commits December 28, 2019 16:46

Add new lint (modulo_arithmetic)

f191e91

Add tests for new lint (modulo_arithmetic)

6223391

Fixes for elided lifetimes

a208906

bors added a commit that referenced this pull request Dec 28, 2019

Auto merge of #4867 - mgr-inz-rafal:modulo_arithmetic, r=flip1995

8e493c6

Modulo arithmetic changelog: Added modulo_arithmetic lint

bors merged commit a208906 into rust-lang:master Dec 28, 2019

bors mentioned this pull request Dec 28, 2019

Add lint to improve floating-point expressions #4897

Merged

mgr-inz-rafal deleted the modulo_arithmetic branch December 28, 2019 18:52

mgr-inz-rafal mentioned this pull request Jan 2, 2020

New lint idea: suggest ^ when comparing bools #4983

Closed

		lhs_operand.string_representation.unwrap(),
		rhs_operand.string_representation.unwrap()

Modulo arithmetic #4867

Modulo arithmetic #4867

Conversation

mgr-inz-rafal commented Nov 30, 2019

mgr-inz-rafal commented Nov 30, 2019 • edited Loading

llogiq commented Nov 30, 2019

mgr-inz-rafal commented Dec 1, 2019

llogiq commented Dec 1, 2019

mgr-inz-rafal commented Dec 2, 2019 • edited Loading

llogiq commented Dec 2, 2019

flip1995 commented Dec 3, 2019

mgr-inz-rafal commented Dec 3, 2019

bors commented Dec 8, 2019

mgr-inz-rafal commented Dec 10, 2019

flip1995 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgr-inz-rafal Dec 18, 2019 • 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

mgr-inz-rafal commented Dec 18, 2019

mgr-inz-rafal commented Dec 19, 2019

bors commented Dec 22, 2019

flip1995 left a 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

Choose a reason for hiding this comment

mgr-inz-rafal commented Dec 22, 2019

flip1995 commented Dec 22, 2019

mgr-inz-rafal commented Dec 22, 2019

flip1995 commented Dec 22, 2019

krishna-veerareddy commented Dec 22, 2019

krishna-veerareddy Dec 22, 2019 • edited Loading

Choose a reason for hiding this comment

mgr-inz-rafal Dec 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgr-inz-rafal commented Dec 23, 2019

flip1995 commented Dec 23, 2019

mgr-inz-rafal commented Dec 23, 2019 • edited Loading

flip1995 commented Dec 24, 2019

bors commented Dec 24, 2019

mgr-inz-rafal commented Dec 26, 2019

phansch commented Dec 27, 2019

bors commented Dec 27, 2019

bors commented Dec 27, 2019

bors commented Dec 27, 2019

flip1995 commented Dec 28, 2019

bors commented Dec 28, 2019

bors commented Dec 28, 2019

flip1995 commented Dec 28, 2019

mgr-inz-rafal commented Dec 28, 2019

flip1995 commented Dec 28, 2019

bors commented Dec 28, 2019

bors commented Dec 28, 2019

bors commented Dec 28, 2019

mgr-inz-rafal commented Dec 28, 2019

mgr-inz-rafal commented Nov 30, 2019 •

edited

Loading

mgr-inz-rafal commented Dec 2, 2019 •

edited

Loading

mgr-inz-rafal Dec 18, 2019 •

edited

Loading

krishna-veerareddy Dec 22, 2019 •

edited

Loading

mgr-inz-rafal Dec 22, 2019 •

edited

Loading

mgr-inz-rafal commented Dec 23, 2019 •

edited

Loading