Skip to content

Commit

Permalink
[Fix #4478] Fix confusing message of Performance/Caller cop
Browse files Browse the repository at this point in the history
See #4478

Note: the spec checks a message is correct. For example:

```ruby
expect(caller(2)[1]).to eq(caller(3..3).first)
```

I think the checks are necessary, because `caller` is difficult to me.
  • Loading branch information
pocke committed Jun 11, 2017
1 parent b1edcdf commit 753b2af
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 19 deletions.
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,7 @@ Metrics/BlockLength:
Metrics/ModuleLength:
Exclude:
- 'spec/**/*.rb'

Performance/Caller:
Exclude:
- spec/rubocop/cop/performance/caller_spec.rb
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* [#4452](https://github.com/bbatsov/rubocop/pull/4452): Add option to `Rails/Delegate` for enforcing the prefixed method name case. ([@klesse413][])
* [#4493](https://github.com/bbatsov/rubocop/pull/4493): Make `Lint/Void` cop aware of `Enumerable#each` and `for`. ([@pocke][])
* [#4492](https://github.com/bbatsov/rubocop/pull/4492): Make `Lint/DuplicateMethods` aware of `alias` and `alias_method`. ([@pocke][])
* [#4478](https://github.com/bbatsov/rubocop/issues/4478): Fix confusing message of `Performance/Caller` cop. ([@pocke][])

## 0.49.1 (2017-05-29)

Expand Down
46 changes: 35 additions & 11 deletions lib/rubocop/cop/performance/caller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,56 @@ module Performance
#
# @example
# # bad
# caller[n]
# caller[1]
# caller.first
#
# # good
# caller(n..n).first
# caller(2..2).first
# caller(1..1).first
class Caller < Cop
MSG = 'Use `caller(n..n)` instead of `caller[n]`.'.freeze
SCOPE_METHODS = %i[first []].freeze
MSG_BRACE =
'Use `caller(%<n>d..%<n>d).first` instead of `caller[%<m>d]`.'.freeze
MSG_FIRST =
'Use `caller(%<n>d..%<n>d).first` instead of `caller.first`.'.freeze

def_node_matcher :slow_caller?, <<-PATTERN
{
(send nil :caller)
(send nil :caller int)
}
PATTERN

def_node_matcher :caller_with_scope_method?, <<-PATTERN
(send (send nil :caller ...) ${#{SCOPE_METHODS.map(&:inspect).join(' ')}} ...)
{
(send $_recv :first)
(send $_recv :[] int)
}
PATTERN

def on_send(node)
return unless caller_with_scope_method?(node) && slow_caller?(node)
add_offense(node, :selector)
recv = caller_with_scope_method?(node)
return unless slow_caller?(recv)

add_offense(node)
end

private

def slow_caller?(node)
arguments = node.receiver.arguments
def message(node)
caller_arg = node.receiver.arguments[0]
n = caller_arg ? int_value(caller_arg) : 1

if node.method_name == :[]
m = int_value(node.arguments[0])
n += m
format(MSG_BRACE, n: n, m: m)
else
format(MSG_FIRST, n: n)
end
end

arguments.empty? ||
(arguments.length == 1 && arguments[0].int_type?)
def int_value(node)
node.children[0]
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions manual/cops_performance.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ can be replaced by `caller(n..n).first`.

```ruby
# bad
caller[n]
caller[1]
caller.first

# good
caller(n..n).first
caller(2..2).first
caller(1..1).first
```

Expand Down
18 changes: 12 additions & 6 deletions spec/rubocop/cop/performance/caller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,44 +12,50 @@
end

it 'registers an offense when :first is called on caller' do
expect(caller.first).to eq(caller(1..1).first)
expect_offense(<<-RUBY.strip_indent)
caller.first
^^^^^ Use `caller(n..n)` instead of `caller[n]`.
^^^^^^^^^^^^ Use `caller(1..1).first` instead of `caller.first`.
RUBY
end

it 'registers an offense when :first is called on caller with 1' do
expect(caller(1).first).to eq(caller(1..1).first)
expect_offense(<<-RUBY.strip_indent)
caller(1).first
^^^^^ Use `caller(n..n)` instead of `caller[n]`.
^^^^^^^^^^^^^^^ Use `caller(1..1).first` instead of `caller.first`.
RUBY
end

it 'registers an offense when :first is called on caller with 2' do
expect(caller(2).first).to eq(caller(2..2).first)
expect_offense(<<-RUBY.strip_indent)
caller(2).first
^^^^^ Use `caller(n..n)` instead of `caller[n]`.
^^^^^^^^^^^^^^^ Use `caller(2..2).first` instead of `caller.first`.
RUBY
end

it 'registers an offense when :[] is called on caller' do
expect(caller[1]).to eq(caller(2..2).first)
expect_offense(<<-RUBY.strip_indent)
caller[1]
^^^ Use `caller(n..n)` instead of `caller[n]`.
^^^^^^^^^ Use `caller(2..2).first` instead of `caller[1]`.
RUBY
end

it 'registers an offense when :[] is called on caller with 1' do
expect(caller(1)[1]).to eq(caller(2..2).first)
expect_offense(<<-RUBY.strip_indent)
caller(1)[1]
^^^ Use `caller(n..n)` instead of `caller[n]`.
^^^^^^^^^^^^ Use `caller(2..2).first` instead of `caller[1]`.
RUBY
end

it 'registers an offense when :[] is called on caller with 2' do
expect(caller(2)[1]).to eq(caller(3..3).first)
expect_offense(<<-RUBY.strip_indent)
caller(2)[1]
^^^ Use `caller(n..n)` instead of `caller[n]`.
^^^^^^^^^^^^ Use `caller(3..3).first` instead of `caller[1]`.
RUBY
end
end

0 comments on commit 753b2af

Please sign in to comment.