Add lint for confusing use of `^` instead of `.pow` #9506

blyxyas · 2022-09-21T17:19:23Z

fixes #4205
Adds a lint named [confusing_xor_and_pow], it warns the user when a ^ b is used as the .pow() function, it doesn't warn for Hex, Binary... etc.

changelog: New lint: [confusing_xor_and_pow]

rust-highfive · 2022-09-21T17:19:26Z

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

Please see the contribution instructions for more information.

kraktus

That lint really look useful, thanks for adding it!

clippy_lints/src/confusing_xor_and_pow.rs

tests/ui/confusing_xor_and_pow.rs

blyxyas · 2022-09-25T09:17:18Z

All changes and suggestions made by @kraktus were commited (some in other commits)

giraffate · 2022-09-26T00:05:55Z

clippy_lints/src/renamed_lints.rs

@@ -6,6 +6,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
    ("clippy::block_in_if_condition_expr", "clippy::blocks_in_if_conditions"),
    ("clippy::block_in_if_condition_stmt", "clippy::blocks_in_if_conditions"),
    ("clippy::box_vec", "clippy::box_collection"),
+    ("clippy::confusing_xor_and_pow", "clippy::suspicious_xor"),


This is a new lint, so it doesn't need renaming.

clippy_lints/src/suspicious_xor.rs

tests/ui/eq_op.stderr

tests/ui/suspicious_xor.stderr

giraffate

Can this lint warn only the case where 2^x and 10^y? Its context is #4205 (comment).

#6182 would be helpful in implementing it.

clippy_lints/src/suspicious_xor.rs

giraffate · 2022-09-26T23:53:28Z

clippy_lints/src/suspicious_xor.rs

+    /// let x = 3_i32.pow(4);
+    /// ```
+    #[clippy::version = "1.65.0"]
+    pub SUSPICIOUS_XOR,


IMO it would be better to point out misuse as a power, so its lint name should include pow. For example, suspicious_xor_as_pow or xor_used_as_pow from #6182.

blyxyas · 2022-09-27T14:19:23Z

I think the lint should warn for every decimal number, personally I think that the XOR operator is sufficiently rare that any use of it between two decimal numbers should give a warning.

bors · 2022-09-27T18:32:32Z

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

kraktus · 2022-09-27T18:49:39Z

I think the lint should warn for every decimal number, personally I think that the XOR operator is sufficiently rare that any use of it between two decimal numbers should give a warning.

Agreed, I think we should lint every <expr> ^ decimal_lit, decimal_lit ^ <expr>, decimal_lit ^ decimal_lit, at least at first, run a lintcheck and see the number of false positive

blyxyas · 2022-09-27T19:21:21Z

run a lintcheck and see the number of false positive

I ran a lintcheck, but I cannot understand the logs. They don't seem to contain anything triggering this lint (the lint isn't even mentioned in lintcheck_crates_logs.txt, so I assume there isn't any false positives)

giraffate · 2022-09-27T23:58:33Z

I think the lint should warn for every decimal number, personally I think that the XOR operator is sufficiently rare that any use of it between two decimal numbers should give a warning.

IMO, it depends on the context because x^y is valid rust codes. If this lint warn for every decimal number, the category should be restriction.

Agreed, I think we should lint every <expr> ^ decimal_lit, decimal_lit ^ <expr>, decimal_lit ^ decimal_lit, at least at first, run a lintcheck and see the number of false positive

As a first step, it would be better to cover only the decimal_lit ^ decimal_lit case.

kraktus · 2022-09-28T06:41:50Z

IMO, it depends on the context because x^y is valid rust codes. If this lint warn for every decimal number, the category should be restriction.

I think warning for decimal literal would be fine, suggesting to use hex or bin instead for better clarity

bors · 2022-09-29T10:24:46Z

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

giraffate · 2022-09-29T23:53:46Z

We follow the no merge-commit policy, so could you rebase, not merge?
https://github.com/rust-lang/rust-clippy/blob/master/book/src/development/basics.md#pr

blyxyas · 2022-09-30T17:00:58Z

Sorry about that.
I tried doing this for a few days and I'm pretty lost, in the test the last test that should warn (let _ = 2i32 ^ macro_test!(); being macro_test!() 13) kraktus mentioned that the lint suggestion should talk about 2i32.pow(macro_test!()) instead of 2i32.pow(13), but I can't find a way to do it. Any hint here?

kraktus · 2022-10-02T13:02:33Z

I've tried myself too to fix the suggestion without luck due to how the suggestion is built. In the end I think it's better not to lint in those case, at least at first. Sorry for the dead end. Made a PR against yours to get you back on track 👍 If you have more questions do not hesitate!

bors · 2022-10-03T11:46:48Z

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

blyxyas · 2022-10-04T15:05:46Z

I've been having issues with Git lately but I think I've fixed them, I think I haven't merged anything

giraffate · 2022-10-04T23:33:15Z

Sorry for the late reviewing.

I tried doing this for a few days and I'm pretty lost, in the test the last test that should warn (let _ = 2i32 ^ macro_test!(); being macro_test!() 13) kraktus mentioned that the lint suggestion should talk about 2i32.pow(macro_test!()) instead of 2i32.pow(13), but I can't find a way to do it. Any hint here?

As a first step, I don't think we need to warn the case where let _ = 2i32 ^ macro_test!(); .

giraffate · 2022-10-06T23:43:47Z

tests/ui/suspicious_xor.stderr

+error: '^' is not the exponentiation operator
+  --> $DIR/suspicious_xor.rs:22:13
+   |
+LL |     let _ = 50i32 ^ 3i32;


I don't think this case should be warned. #9506 (review)

I think 50 ^ 3 should be warned because, is really that common xoring 50 and 3? In my opinion the cases where this happens is as common as using a bitwise OR in an if statement. Both cases are just confusion with other operators in a vast majority of cases.

It depends on contexts. If we warn any x^y (x, y = literal integer), the category should be restriction or pedantic.

We’re only talking about linting literal using the decimal form which I’ve never seen used when knowingly xoring.

Message could suggest ‘if this is intentional, use the hexadecimal or binary form as this is more explicit’

Message could suggest ‘if this is intentional, use the hexadecimal or binary form as this is more explicit’

Are you suggesting to replace the current message (did you mean to write: 50.pow(3)) or to have both?

blyxyas · 2022-10-10T06:39:55Z

@rustbot ready

giraffate

Overall looks good, thanks! I made small comments.

clippy_lints/src/suspicious_xor_used_as_pow.rs

src/docs/suspicious_xor_used_as_pow.txt

clippy_lints/src/suspicious_xor_used_as_pow.rs

giraffate

Could you squash commits?

tests/ui/suspicious_xor_used_as_pow.rs

blyxyas · 2022-10-18T17:46:54Z

Could you squash commits

Do you mean squashing all commits? Sorry but I've never squashed. So I don't know if I need to squash all, or do a selection.

If I need to do a selection, which commits should I select?

kraktus · 2022-10-18T18:27:09Z

Hey, you should look at git interactive rebase. git rebase -i. Alternatively I can do it for you then PR it

blyxyas · 2022-10-18T19:11:06Z

I've looked at it a little bit, but I don't know if I should squash all commits (including ones not made on this PR) from the first commit on this PR (65f6029) until HEAD, or I should select commits just made in this PR.

kraktus · 2022-10-18T19:21:42Z

Just those from the PR, otherwise you’ll have merging conflits

blyxyas · 2022-10-20T10:41:48Z

The last two commits were me trying to revert the errors I did (Just wasted several hours trying to squash).
If someone can help me here, I'd appreciate it very much

bors · 2022-10-20T15:20:55Z

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

kraktus · 2022-10-20T22:16:15Z

Given the history of your branch I believe the easiest option is just to copy-paste your change in a new branch

giraffate · 2022-10-21T00:13:11Z

Given the history of your branch I believe the easiest option is just to copy-paste your change in a new branch

I agree.

blyxyas · 2022-10-21T12:07:41Z

So, should I create another fork, for another PR with just the final commit?

kraktus · 2022-10-21T12:10:42Z

You can create a new local branch on your fork based on the head master branch, and pushes it to this remote branch directly. git push your_local_remote_name_origin_by_default your_local_branch:master

blyxyas · 2022-10-21T12:20:46Z

Ok, the new branch (new) was created, but I don't see any way to change the branch that this PR is originated from

kraktus · 2022-10-21T12:22:22Z

git push --force origin new:master, ⚠️ This will override all the work you've done on this branch, so be careful to have it saved on a backup branch before doing it

blyxyas · 2022-10-21T12:39:18Z

Mmmmmm... Something is wrong, I cannot push because git says that everything is up to date, which makes sense because I haven't changed anything on new.

Btw, sorry for being such an inconvenience, this is my first time contributing to a big repo

kraktus · 2022-10-21T12:45:23Z

It is because your new branch is identical to your master branch. You should squash all your commits on your new branch (hard), or better, do it by hand:

Create a new2 branch
once on your new2 branch, reset it with upstream master branch. git reset --hard head master
Apply manually all the changes you've made on this branch (ie copy paste GitHub diff). Then push it as one commit remotely to your new2 branch
git push --force origin new2:master

blyxyas · 2022-10-21T18:04:34Z

I think I did it, somehow. I would squash the second commit into the first, but it being a merge from the new2 branch into blyxyas/rust-clippy:master it doesn't appear in git rebase -i HEAD~1

bors · 2022-10-23T17:31:23Z

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

giraffate · 2022-10-25T00:39:41Z

I think I did it, somehow. I would squash the second commit into the first, but it being a merge from the new2 branch into blyxyas/rust-clippy:master it doesn't appear in git rebase -i HEAD~1

To squash commits, I tried below. Now,

*   9029aba99 (HEAD -> pr_9506) Merge pull request #3 from blyxyas/new2
|\  
| * 8109ebb23 Just lint changes
|/  
*   967f172e2 Auto merge of #9635 - smoelius:fix-9386-bug, r=Jarcho

git rebase HEAD^ and git status

*   967f172e2 (HEAD -> pr_9506) Auto merge of #9635 - smoelius:fix-9386-bug, r=Jarcho

git status
On branch pr_9506
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   CHANGELOG.md
	modified:   clippy_lints/src/lib.register_lints.rs
	modified:   clippy_lints/src/lib.register_restriction.rs
	modified:   clippy_lints/src/lib.rs
	modified:   src/docs.rs
	modified:   tests/ui/eq_op.rs
	modified:   tests/ui/eq_op.stderr

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	clippy_lints/src/suspicious_xor_used_as_pow.rs
	src/docs/suspicious_xor_used_as_pow.txt
	tests/ui/suspicious_xor_used_as_pow.rs
	tests/ui/suspicious_xor_used_as_pow.stderr

So, git add -A and git commit.

And, to resolve conflicts, git rebase master after master branch is up-to-date.

clippy_lints/src/suspicious_xor_used_as_pow.rs

Co-authored-by: NAKATA Takayuki <f.seasons017@gmail.com>

giraffate · 2022-10-31T00:18:17Z

@bors r+

Thanks for your patience!

bors · 2022-10-31T00:18:19Z

📌 Commit 151395d has been approved by giraffate

It is now in the queue for this repository.

bors · 2022-10-31T00:29:02Z

⌛ Testing commit 151395d with merge 37d338c...

bors · 2022-10-31T00:41:37Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 37d338c to master...

rust-highfive assigned giraffate Sep 21, 2022

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

kraktus reviewed Sep 24, 2022

View reviewed changes

giraffate reviewed Sep 26, 2022

View reviewed changes

kraktus reviewed Sep 26, 2022

View reviewed changes

tests/ui/eq_op.stderr Outdated Show resolved Hide resolved

tests/ui/suspicious_xor.stderr Outdated Show resolved Hide resolved

giraffate reviewed Sep 26, 2022

View reviewed changes

blyxyas closed this Sep 27, 2022

blyxyas reopened this Sep 27, 2022

giraffate added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 28, 2022

giraffate reviewed Oct 6, 2022

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 10, 2022

giraffate reviewed Oct 13, 2022

View reviewed changes

clippy_lints/src/suspicious_xor_used_as_pow.rs Outdated Show resolved Hide resolved

src/docs/suspicious_xor_used_as_pow.txt Outdated Show resolved Hide resolved

clippy_lints/src/suspicious_xor_used_as_pow.rs Outdated Show resolved Hide resolved

giraffate reviewed Oct 18, 2022

View reviewed changes

tests/ui/suspicious_xor_used_as_pow.rs Show resolved Hide resolved

blyxyas closed this Oct 21, 2022

blyxyas reopened this Oct 21, 2022

blyxyas added 2 commits October 25, 2022 20:30

Just lint changes

32ded57

Lints updated

64308d6

giraffate reviewed Oct 27, 2022

View reviewed changes

clippy_lints/src/suspicious_xor_used_as_pow.rs Outdated Show resolved Hide resolved

Update clippy_lints/src/suspicious_xor_used_as_pow.rs

151395d

Co-authored-by: NAKATA Takayuki <f.seasons017@gmail.com>

bors merged commit 37d338c into rust-lang:master Oct 31, 2022

bors mentioned this pull request Oct 31, 2022

Fixes #9677 - Lint against mod lib; #9708

Closed

Add lint for confusing use of ^ instead of .pow #9506

Add lint for confusing use of ^ instead of .pow #9506

Conversation

blyxyas commented Sep 21, 2022

rust-highfive commented Sep 21, 2022

kraktus left a comment • edited Loading

Choose a reason for hiding this comment

blyxyas commented Sep 25, 2022

giraffate Sep 26, 2022

Choose a reason for hiding this comment

giraffate left a comment

Choose a reason for hiding this comment

giraffate Sep 26, 2022

Choose a reason for hiding this comment

blyxyas commented Sep 27, 2022

bors commented Sep 27, 2022

kraktus commented Sep 27, 2022

blyxyas commented Sep 27, 2022

giraffate commented Sep 27, 2022

kraktus commented Sep 28, 2022

bors commented Sep 29, 2022

giraffate commented Sep 29, 2022

blyxyas commented Sep 30, 2022

kraktus commented Oct 2, 2022

bors commented Oct 3, 2022

blyxyas commented Oct 4, 2022

giraffate commented Oct 4, 2022

giraffate Oct 6, 2022

Choose a reason for hiding this comment

blyxyas Oct 7, 2022

Choose a reason for hiding this comment

giraffate Oct 13, 2022

Choose a reason for hiding this comment

kraktus Oct 13, 2022

Choose a reason for hiding this comment

blyxyas Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

blyxyas commented Oct 10, 2022

giraffate left a comment

Choose a reason for hiding this comment

giraffate left a comment

Choose a reason for hiding this comment

blyxyas commented Oct 18, 2022

kraktus commented Oct 18, 2022

blyxyas commented Oct 18, 2022

kraktus commented Oct 18, 2022

blyxyas commented Oct 20, 2022

bors commented Oct 20, 2022

kraktus commented Oct 20, 2022

giraffate commented Oct 21, 2022

blyxyas commented Oct 21, 2022

kraktus commented Oct 21, 2022

blyxyas commented Oct 21, 2022

kraktus commented Oct 21, 2022

blyxyas commented Oct 21, 2022

kraktus commented Oct 21, 2022 • edited Loading

blyxyas commented Oct 21, 2022

bors commented Oct 23, 2022

giraffate commented Oct 25, 2022

giraffate commented Oct 31, 2022

bors commented Oct 31, 2022

bors commented Oct 31, 2022

bors commented Oct 31, 2022

Add lint for confusing use of `^` instead of `.pow` #9506

Add lint for confusing use of `^` instead of `.pow` #9506

kraktus left a comment •

edited

Loading

blyxyas Oct 13, 2022 •

edited

Loading

kraktus commented Oct 21, 2022 •

edited

Loading