Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint/BinaryOperatorWithIdenticalOperands makes no sense with bitwise shift operators #8478

Closed
mvz opened this issue Aug 6, 2020 · 12 comments
Closed
Assignees

Comments

@mvz
Copy link
Contributor

@mvz mvz commented Aug 6, 2020

Lint/BinaryOperatorWithIdenticalOperands reports an offense with the following code:

    bit_mask :IFunctionInfoFlags,
             is_method:      (1 << 0),
             is_constructor: (1 << 1),
             is_getter:      (1 << 2),
             is_setter:      (1 << 3),
             wraps_vfunc:    (1 << 4),
             throws:         (1 << 5)

an offense is flagged in the line is_constructor: (1 << 1),.

While for most binary operators, the result of foo [op] foo can be replaced by a constant or foo regardless of the value of foo, this is not the case for the shift operators: 1 << 1 has a completely different result from ['a'] << ['a'].

Expected behavior

No offense is flagged in the above code

Actual behavior

RuboCop flags an offense, e.g.:

lib/ffi-gobject_introspection/lib.rb:67:31: W: Lint/BinaryOperatorWithIdenticalOperands: Binary operator << has identical operands.
             is_constructor: (1 << 1),
                              ^^^^^^

Steps to reproduce the problem

Put the code at the top in a ruby file and run rubocop on it.

RuboCop version

$ bundle exec rubocop -V
0.89.0 (using Parser 2.7.1.4, rubocop-ast 0.3.0, running on ruby 2.7.1 x86_64-linux)
@marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 6, 2020

Agreed, we could lookout for that exact case 1 << 1 and ignore it.

@marcandre marcandre self-assigned this Aug 6, 2020
marcandre added a commit to marcandre/rubocop that referenced this issue Aug 6, 2020
…s for mathematical operations
@marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 6, 2020

I've relaxed the checks for any integers and binary operators; I doubt there will be many invalid uses of 42 >> 42 in the wild...

marcandre added a commit to marcandre/rubocop that referenced this issue Aug 6, 2020
…s for mathematical operations

Simplified one of the `basic_literal?`, since we'd only raise an offense if they are
equal, in which case `basic_literal?` has the same result on both `lhs` and `rhs`...
@marcandre marcandre closed this in be47dc7 Aug 6, 2020
@marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 6, 2020

Fixed. Thanks for raising this issue 👍

@mvz
Copy link
Contributor Author

@mvz mvz commented Aug 7, 2020

Thanks, @marcandre!

@mathieujobin
Copy link

@mathieujobin mathieujobin commented Aug 12, 2020

same applies to ** operator

rubocop now complains about foo ** foo which is perfectly fine

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Aug 12, 2020

I see @marcandre covered **, but I think it's ignored only for literals currently.

@marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 12, 2020

rubocop now complains about foo ** foo which is perfectly fine

Right... Is this a theoretical issue or do you actually have such a need? I'm curious to use cases.

@mathieujobin
Copy link

@mathieujobin mathieujobin commented Aug 12, 2020

it { expect( batch_job.reduce(0) { |val, item_val| val + item_val**item_val } ).to eq 287 }
@mathieujobin
Copy link

@mathieujobin mathieujobin commented Aug 12, 2020

@marcandre maybe it isn't a bad thing that rubocop catch those and we can add them to our ignore list if it make sense.

I'm sure happy to see foo || foo being reported to me as I didn't know we had those in our code base...

@marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 12, 2020

@marcandre maybe it isn't a bad thing that rubocop catch those and we can add them to our ignore list if it make sense.

Cool. I must admit I still have no idea what your it example is trying to achieve. If at least instead of 287 it was written 2 ** 2 + 3 ** 3 + 4 ** 4 😅 Still, it looks like could be rewritten as it { expect(batch_job).to =~ [2, 3, 4] }?

@mathieujobin
Copy link

@mathieujobin mathieujobin commented Aug 13, 2020

true that, honestly, I didn't wrote it, its part of the code base I am maintaining. and you are probably right, maybe its worth trashing ;)

@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Aug 13, 2020

Well, I guess this proves that the cop is helpful in its current form. 😉

JustinAiken added a commit to usertesting/ut_rubocop that referenced this issue Aug 26, 2020
- It was just added in 0.89.0
- It finds errors in simple math operations, has many errors reported,
such as:
  - rubocop-hq/rubocop#8482
  - rubocop-hq/rubocop#8478

It looks like the next version of rubocop may loosen it a bit, but let's
just disable it for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.