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

Endless Method Keyword/Operator Precedence Check #12988

Closed
phene opened this issue Jun 12, 2024 · 6 comments · Fixed by #13288
Closed

Endless Method Keyword/Operator Precedence Check #12988

phene opened this issue Jun 12, 2024 · 6 comments · Fixed by #13288

Comments

@phene
Copy link

phene commented Jun 12, 2024

Is your feature request related to a problem?

Consider the following code

def foo = true and false
def bar = true && false

foo # => true
bar # => false

Because the = operator has higher precedence than and or or, they will not be included as part the method.

Describe the solution you'd like

A rubocop check to detect operators/keywords with lower precedence than = on the same line as an endless expression would be incredibly helpful in avoiding this sort of footgun.

@phene phene changed the title Endless Method Footgun Check Endless Method Keyword/Operator Precedence Check Jun 12, 2024
@vlad-pisanov
Copy link
Contributor

vlad-pisanov commented Jun 17, 2024

Such a cop is badly needed. Our team has run into if/unless gotchas several times:

def foo = 42 if cond
# NoMethodError: undefined method `cond' for main

@dvandersluis
Copy link
Member

@vlad-pisanov I'm looking into this one. There is a somewhat common pattern of defining methods unless defined, such as:

def foo
  42
end unless defined?(:foo)

which could be written in endless form potentially:

def foo = 42 unless defined?(:foo)

How would you handle that? Exception in the cop specifically for defined??

@vlad-pisanov
Copy link
Contributor

@dvandersluis I haven't encountered this pattern before. Maybe it could be conditionally permitted for endless methods with a cop option like AllowInlineModifiers or something? 🤔

@dvandersluis
Copy link
Member

dvandersluis commented Oct 2, 2024

Perhaps I'm misremembering its prevalance. A github code search reveals some uses but not as prevalent as I thought. There's also unless respond_to? in any case.

I think I'll just leave it for now and if issues come up we can address.

dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Oct 2, 2024
dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Oct 2, 2024
dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Oct 2, 2024
@dvandersluis dvandersluis linked a pull request Oct 2, 2024 that will close this issue
8 tasks
@phene
Copy link
Author

phene commented Oct 2, 2024

FWIW, def foo = 42 unless defined?(:foo) and

def foo
  42
end unless defined?(:foo)

seem like major anti-patterns.

@dvandersluis
Copy link
Member

Agreed it's an anti-pattern, just wanted to avoid unexpected behaviour.

dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Oct 7, 2024
dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Oct 7, 2024
dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Oct 15, 2024
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

Successfully merging a pull request may close this issue.

3 participants