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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Lint/ToEnumArguments cop #8941

Merged
merged 1 commit into from Oct 27, 2020

Conversation

fatkodima
Copy link
Contributor

Closes #7753

This cop ensures that to_enum/enum_for, called for the current method, has correct arguments.

# bad
def method(x, y = 1)
  return to_enum(__method__, x) # `y` is missing
end
      
# good
def method(x, y = 1)
  return to_enum(__method__, x, y)
end

I have made this mistake myself 馃槃 - proof

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 26, 2020

I don't think I've ever used those methods, so I'll leave it to the others to provide feedback for that cop. Some more rationale might be useful, as others like me might first think this is something about enumerations. :D

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cop! I've made this kind of mistake a few times.

Need to get to bed, just checked the tests, looks good, I'd be even stricter.

@marcandre
Copy link
Contributor

Also, could you support forwarding please? (..., Ruby 2.7+)

@fatkodima
Copy link
Contributor Author

Did you mean something like

def meth(...)
  return to_enum(...) unless block_given?
end

Is there a usecase for this?

It does not make much sense to me, compared to

def meth(...)
  return to_enum(:meth, ...) unless block_given?
end

which is an invalid syntax, unfortunately.

Also, currently the first example passes without violation.

@marcandre
Copy link
Contributor

marcandre commented Oct 26, 2020

It does not make much sense to me, compared to

def meth(...)
  return to_enum(:meth, ...) unless block_given?
end

This is what I meant, and it is Ruby 3.0+ only, my bad. Still would be nice to do (maybe just missing a test)

@fatkodima
Copy link
Contributor Author

Added a TODO for the future, as it is easy to implement, but hard to test before 3.0 is released.

@marcandre
Copy link
Contributor

Added a TODO for the future, as it is easy to implement, but hard to test before 3.0 is released.

As you wish. FYI, we already run our CI against ruby-head, and testing locally is easy with rvm install ruby-head.

@marcandre
Copy link
Contributor

@bbatsov ok to merge?

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 27, 2020

Yep.

@bbatsov bbatsov merged commit 21e4062 into rubocop:master Oct 27, 2020
koic added a commit to koic/rubocop that referenced this pull request Oct 27, 2020
Follow rubocop#8941 (comment).

This PR fixes the following false positive for `Lint/ToEnumArguments`
when enumerator is created for another method.

```console
% cd path/to/rubocop-performance
% bundle exec rubocop lib/rubocop/cop/performance/redundant_block_call.rb
Inspecting 1 file
W

Offenses:

lib/rubocop/cop/performance/redundant_block_call.rb:83:19: W:
Lint/ToEnumArguments: Ensure you correctly provided all the arguments.
          calls = to_enum(:blockarg_calls, body, argname)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected
```

The following is a snippet of `Performance/RedundantBlockCall` cop
that is a realworld use-case.

```ruby
def_node_search :blockarg_calls, <<~PATTERN
  (send (lvar %1) :call ...)
PATTERN

...

def calls_to_report(argname, body)
  return [] if blockarg_assigned?(body, argname)

  calls = to_enum(:blockarg_calls, body, argname)

  return [] if calls.any? { |call| args_include_block_pass?(call) }

  calls
end
```

https://github.com/rubocop-hq/rubocop-performance/blob/v1.8.1/
lib/rubocop/cop/performance/redundant_block_call.rb#L80-L88
@bquorning
Copy link
Contributor

I am not sure I completely understand this cop. I am not sure that the following code should be flagged:

def foo
  bar = to_enum(:baz).count
  bar > 42
end

Is it a bug, or have I just misunderstood this cop?

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 this pull request may close these issues.

Cop idea: Not enough arguments for enum_for/to_enum
4 participants