Add lint for use of ^ operator as pow. #4213

KamilaBorowska · 2019-06-17T10:33:17Z

Resolves #4205

Inspired by https://twitter.com/jfbastien/status/1139298419988549632. Only checking for 2^ is intentional, I was considering checking for 10^ as well, but more often than not, the incorrect code is using 2^.

changelog: Add lint for using xor operator when exponentiation was intended.

flip1995 · 2019-06-17T12:06:46Z

Citing #4205 (comment)

xoring is all about bits, so the lint shouldn't apply to non-decimal notations (e.g. 0xA^x or x^0b10)

This lint should not trigger on hex or binary literals, since these are probably intended.

flip1995 · 2019-06-17T12:07:38Z

tests/ui/xor_used_as_pow.rs

+    println!("{}", 2 ^ 16);
+    // Should be allowed
+    let x = 16;
+    println!("{}", 2 ^ x);


This should also get linted, shouldn't it?

notriddle · 2019-06-18T01:25:07Z

I was considering checking for 10^ as well, but more often than not, the incorrect code is using 2^.

So? It's still very unlikely that someone intentionally wrote 10^3 when they could've just written 9. The whole point of writing formulas that are entirely made up of literals is either because the formula is easier to understand (in which case you should probably also use binary or hex) or because the resulting number is enormous (which makes no sense for xor which can never have a higher significant bit than the biggest of its parameters, and, as a result, the result of an xor can never be more than twice as large as its largest parameter).

Walther · 2019-06-18T03:20:21Z

clippy_lints/src/xor_used_as_pow.rs

+                    expr.span,
+                    "`^` is not an exponentiation operator but was used as one",
+                    "did you mean to write",
+                    format!("1 << {}", right),


Should the suggestion be about .pow() instead? The premise is, quote, "^ is not an exponentiation operator but was used as one", so it might make more sense to suggest an actual exponentiation operator than a seemingly completely unrelated bit shift?

2^x == 1 << x (That's only true for powers of 2). You don't want to use pow for powers of 2, because of performance reasons.

If we add a suggestion for 10^x, the lint should obviously suggest 10.pow(x).

Nod, I'm aware of how the bitshift works for powers of two, was just wondering if that makes for a somewhat confusing suggestion - telling the user they probably intended exponentiation, and then suggesting a bitshift. Especially, but not limited to, if the mistake is made by a beginner, the suggestion might seem strange 🤔

Good point! I would suggest the bitshift, but write this in the documentation of the lint.

(Additionally, arguably, why should bitshift be more efficient than .pow(), especially for known-at-compile-time values, shouldn't that be a compiler optimization that the programmer doesn't need to worry about? 🤔 )

Another interesting thought here for consideration, perhaps recommending the std::_::MAX values, if applicable 🤔 rust-lang/rust#61934 (comment)

shouldn't that be a compiler optimization

Yeah it should be one for pow(some_power_of_two, x). But using a bitshift is always performant (and a pretty standard thing to do). Relying on the compiler might not always produce performant code. And Clippy should always suggest the best possible code IMO.

If I end up picking this up (assuming that's desired or needed), would it be worthwhile to put out a different error based on whether the number is 2 or not? If it's two, output the suggestion of bitshift, if it's not, mention pow?

We don't need an extra lint for this, I think. But you can do something like:

// NOTE: This is pseudo code and the messages need improvement. ;) if lhs == 2 { span_lint(LINT, "this looks like you want to pow", "try bitshift"); } else { span_lint(LINT, "..", "try pow"); }

Yeah, that's akin to what I was thinking. I think I'll also look at incorporating Walther's suggestion (and the comment from this thread) to recommend std::_::MAX in the case of rhs being an Int literal of 8, 16, 32, or 64.

phansch · 2019-06-18T07:49:31Z

clippy_lints/src/xor_used_as_pow.rs

+            if let LitKind::Int(2, _) = lit.node;
+            if let ExprKind::Lit(lit) = &right.node;
+            if let LitKind::Int(right, _) = lit.node;
+            then {


Please also make sure that it doesn't trigger in external macros (using in_external_macro)

bors · 2019-07-17T08:09:23Z

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

kornelski · 2019-08-06T23:27:02Z

@xfix what's the status of this PR?

jolson88 · 2019-09-13T01:12:19Z

@flip1995 @kornelski As this has been closed as inactive, does anybody mind if I pick this up and run with it?

If that sounds good, with this PR being unmergeable, is it worth trying to pull in xfix's branch and continue forward, or should I go ahead and just create a fresh branch to make these changes and address some of the comments at the same time?

flip1995 · 2019-09-13T07:30:51Z

Sure, you can pick it up, thanks!

I think the best thing to do would be to start with the current clippy master branch and cherry-pick the commit of @xfix on top of it*. The merge conflicts should only be in files with auto generated code (with ./util/dev update_lints). So it won't really matter if the conflicts get resolved wrong.

* if you need help on how to do this, let me know. :)

jolson88 · 2019-09-13T18:27:36Z

Thanks Philipp. Got the commit cherry-picked just fine. You were correct that the only conflicts were from auto-generated files. I'll get a PR opened tomorrow (Saturday) for this.

HKalbasi · 2021-07-29T22:36:37Z

continued in #4541
@rustbot label -S-inactive-closed

KamilaBorowska force-pushed the xor_used_as_pow branch from b009aef to 76dc98e Compare June 17, 2019 10:35

Add lint for use of ^ operator as exponentiation.

f9687e0

KamilaBorowska force-pushed the xor_used_as_pow branch from 76dc98e to f9687e0 Compare June 17, 2019 10:50

flip1995 reviewed Jun 17, 2019

View reviewed changes

Walther reviewed Jun 18, 2019

View reviewed changes

phansch reviewed Jun 18, 2019

View reviewed changes

jonas-schievink mentioned this pull request Jun 18, 2019

Lint on uses of XOR as if it were POW: 2 ^ 32 rust-lang/rust#61934

Closed

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

KamilaBorowska closed this Jul 18, 2019

flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 18, 2019

jolson88 mentioned this pull request Sep 14, 2019

Add lint for use of ^ operator as exponentiation. #4541

Closed

rustbot removed the S-inactive-closed Status: Closed due to inactivity label Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint for use of ^ operator as pow. #4213

Add lint for use of ^ operator as pow. #4213

KamilaBorowska commented Jun 17, 2019 •

edited by flip1995

Loading

flip1995 commented Jun 17, 2019

flip1995 Jun 17, 2019 •

edited

Loading

notriddle commented Jun 18, 2019 •

edited

Loading

Walther Jun 18, 2019

flip1995 Jun 18, 2019

Walther Jun 18, 2019

flip1995 Jun 18, 2019

Walther Jun 18, 2019

Walther Jun 18, 2019

flip1995 Jun 19, 2019

jolson88 Sep 13, 2019

flip1995 Sep 13, 2019

jolson88 Sep 14, 2019

phansch Jun 18, 2019

bors commented Jul 17, 2019

kornelski commented Aug 6, 2019

jolson88 commented Sep 13, 2019

flip1995 commented Sep 13, 2019

jolson88 commented Sep 13, 2019 •

edited

Loading

HKalbasi commented Jul 29, 2021

Add lint for use of ^ operator as pow. #4213

Add lint for use of ^ operator as pow. #4213

Conversation

KamilaBorowska commented Jun 17, 2019 • edited by flip1995 Loading

flip1995 commented Jun 17, 2019

flip1995 Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

notriddle commented Jun 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

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

Choose a reason for hiding this comment

bors commented Jul 17, 2019

kornelski commented Aug 6, 2019

jolson88 commented Sep 13, 2019

flip1995 commented Sep 13, 2019

jolson88 commented Sep 13, 2019 • edited Loading

HKalbasi commented Jul 29, 2021

KamilaBorowska commented Jun 17, 2019 •

edited by flip1995

Loading

flip1995 Jun 17, 2019 •

edited

Loading

notriddle commented Jun 18, 2019 •

edited

Loading

jolson88 commented Sep 13, 2019 •

edited

Loading