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

Style/AndOr suggests replacing "and" with "&&", and doesn't autocorrect, in places where "and" and "&&" have different semantics #2531

Closed
dmolesUC opened this issue Dec 22, 2015 · 9 comments

Comments

@dmolesUC
Copy link
Contributor

Consider the following:

@h = { a: {b: 'c'} } 

def find_value_1(k1, k2)
  h1 = @h[k1] and h1[k2]
end

def find_value_2(k1, k2)
  h1 = @h[k1] && h1[k2]
end

find_value_1(:a, :b)
# => "c"

find_value_2(:a, :b)
# NoMethodError: undefined method `[]' for nil:NilClass
#   from (irb):8:in `find_value_2'
#   from (irb):13

Rubocop suggests “Use && instead of and” for methods like find_value_1. (To its credit, it doesn't autocorrect with -a, though.)

I understand that the style guide bans use of and for any purpose, but in that case it seems like Rubocop should suggest a different idiom (and/or warn about parentheses / precedence) here, rather than a substitution that will fail.

@alexdowad
Copy link
Contributor

Did you use --auto-correct to fix the problem? If so, RC should add parentheses as needed, so the meaning of the expression is not changed.

...Though even if not autocorrecting, RC could mention precedence in cases where it makes a difference. We have the technology to do that.

@dmolesUC
Copy link
Contributor Author

It doesn't autocorrect. Maybe that's the bug?

$ bundle exec rubocop --auto-correct
Inspecting 16 files
...C............

Offenses:

lib/factory/factory/abstract_factory.rb:68:40: C: Use && instead of and.
        products = product_registry[k] and products[v.to_s]
                                       ^^^

16 files inspected, 1 offense detected

@dmolesUC
Copy link
Contributor Author

As a side note, the failure to autocorrect got me to actually read up on the and/&& difference, so I guess there's a pedagogical benefit.

@dmolesUC dmolesUC changed the title Style/AndOr suggests replacing "and" with "&&" in places where "and" and "&&" have different semantics Style/AndOr suggests replacing "and" with "&&", and doesn't autocorrect, in places where "and" and "&&" have different semantics Dec 22, 2015
@alexdowad
Copy link
Contributor

When I cut-and-paste find_value_1 into a new source file and run rubocop -a, it is auto-corrected, and parentheses are added, like this:

# encoding: utf-8
def find_value_1(k1, k2)
  (h1 = @h[k1]) && h1[k2]
end

The second example is corrected to:

# encoding: utf-8
(products = product_registry[k]) && products[v.to_s]

...Maybe there is some surrounding context in lib/factory/factory/abstract_factory.rb which makes a difference? Can you share that file (or a portion of it which can reproduce the problem)?

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 23, 2015

@dmolesUC3 what's your RuboCop version btw?

@jonas054
Copy link
Collaborator

I did a little digging. Found that the inspected code must have been and older revision (e.g. 7720451) of https://github.com/dmolesUC3/config-factory

Stripping down the code to a minimal example, I found that there's no auto-correction of

def x
end

def y
  a = b and a.c
end

but if I remove the method x, the expression inside y will be auto-corrected.

@alexdowad
Copy link
Contributor

Awesome detective work @jonas054!

@alexdowad
Copy link
Contributor

Just pushing a fix to my open PR. It turns out this is my bug.

@dmolesUC
Copy link
Contributor Author

dmolesUC commented Jan 4, 2016

@jonas054 Nice work! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants