Skip to content

Commit b43a7fe

Browse files
authored
Merge pull request #208 from eugeneius/index_by_index_with
Add new Rails/IndexBy and Rails/IndexWith cops
2 parents 3dfebcd + bc2d85e commit b43a7fe

File tree

10 files changed

+574
-0
lines changed

10 files changed

+574
-0
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
### New features
66

77
* [#197](https://github.com/rubocop-hq/rubocop-rails/pull/197): Add `Rails/UniqueValidationWithoutIndex` cop. ([@pocke][])
8+
* [#208](https://github.com/rubocop-hq/rubocop-rails/pull/208): Add new `Rails/IndexBy` and `Rails/IndexWith` cops. ([@djudd][], [@eugeneius][])
89

910
### Bug fixes
1011

@@ -144,3 +145,4 @@
144145
[@fidalgo]: https://github.com/fidalgo
145146
[@hanachin]: https://github.com/hanachin
146147
[@joshpencheon]: https://github.com/joshpencheon
148+
[@djudd]: https://github.com/djudd

config/default.yml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,16 @@ Rails/IgnoredSkipActionFilterOption:
268268
Include:
269269
- app/controllers/**/*.rb
270270

271+
Rails/IndexBy:
272+
Description: 'Prefer `index_by` over `each_with_object` or `map`.'
273+
Enabled: true
274+
VersionAdded: '2.5'
275+
276+
Rails/IndexWith:
277+
Description: 'Prefer `index_with` over `each_with_object` or `map`.'
278+
Enabled: true
279+
VersionAdded: '2.5'
280+
271281
Rails/InverseOf:
272282
Description: 'Checks for associations where the inverse cannot be determined automatically.'
273283
Enabled: true
Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
# Common functionality for Rails/IndexBy and Rails/IndexWith
6+
module IndexMethod # rubocop:disable Metrics/ModuleLength
7+
def on_block(node)
8+
on_bad_each_with_object(node) do |*match|
9+
handle_possible_offense(node, match, 'each_with_object')
10+
end
11+
end
12+
13+
def on_send(node)
14+
on_bad_map_to_h(node) do |*match|
15+
handle_possible_offense(node, match, 'map { ... }.to_h')
16+
end
17+
18+
on_bad_hash_brackets_map(node) do |*match|
19+
handle_possible_offense(node, match, 'Hash[map { ... }]')
20+
end
21+
end
22+
23+
def on_csend(node)
24+
on_bad_map_to_h(node) do |*match|
25+
handle_possible_offense(node, match, 'map { ... }.to_h')
26+
end
27+
end
28+
29+
def autocorrect(node)
30+
lambda do |corrector|
31+
correction = prepare_correction(node)
32+
execute_correction(corrector, node, correction)
33+
end
34+
end
35+
36+
private
37+
38+
# @abstract Implemented with `def_node_matcher`
39+
def on_bad_each_with_object(_node)
40+
raise NotImplementedError
41+
end
42+
43+
# @abstract Implemented with `def_node_matcher`
44+
def on_bad_map_to_h(_node)
45+
raise NotImplementedError
46+
end
47+
48+
# @abstract Implemented with `def_node_matcher`
49+
def on_bad_hash_brackets_map(_node)
50+
raise NotImplementedError
51+
end
52+
53+
def handle_possible_offense(node, match, match_desc)
54+
captures = extract_captures(match)
55+
56+
return if captures.noop_transformation?
57+
58+
add_offense(
59+
node,
60+
message: "Prefer `#{new_method_name}` over `#{match_desc}`."
61+
)
62+
end
63+
64+
def extract_captures(match)
65+
argname, body_expr = *match
66+
Captures.new(argname, body_expr)
67+
end
68+
69+
def new_method_name
70+
raise NotImplementedError
71+
end
72+
73+
def prepare_correction(node)
74+
if (match = on_bad_each_with_object(node))
75+
Autocorrection.from_each_with_object(node, match)
76+
elsif (match = on_bad_map_to_h(node))
77+
Autocorrection.from_map_to_h(node, match)
78+
elsif (match = on_bad_hash_brackets_map(node))
79+
Autocorrection.from_hash_brackets_map(node, match)
80+
else
81+
raise 'unreachable'
82+
end
83+
end
84+
85+
def execute_correction(corrector, node, correction)
86+
correction.strip_prefix_and_suffix(node, corrector)
87+
correction.set_new_method_name(new_method_name, corrector)
88+
89+
captures = extract_captures(correction.match)
90+
correction.set_new_arg_name(captures.transformed_argname, corrector)
91+
correction.set_new_body_expression(
92+
captures.transforming_body_expr,
93+
corrector
94+
)
95+
end
96+
97+
# Internal helper class to hold match data
98+
Captures = Struct.new(
99+
:transformed_argname,
100+
:transforming_body_expr
101+
) do
102+
def noop_transformation?
103+
transforming_body_expr.lvar_type? &&
104+
transforming_body_expr.children == [transformed_argname]
105+
end
106+
end
107+
108+
# Internal helper class to hold autocorrect data
109+
Autocorrection = Struct.new(:match, :block_node, :leading, :trailing) do # rubocop:disable Metrics/BlockLength
110+
def self.from_each_with_object(node, match)
111+
new(match, node, 0, 0)
112+
end
113+
114+
def self.from_map_to_h(node, match)
115+
strip_trailing_chars = node.parent&.block_type? ? 0 : '.to_h'.length
116+
new(match, node.children.first, 0, strip_trailing_chars)
117+
end
118+
119+
def self.from_hash_brackets_map(node, match)
120+
new(match, node.children.last, 'Hash['.length, ']'.length)
121+
end
122+
123+
def strip_prefix_and_suffix(node, corrector)
124+
expression = node.loc.expression
125+
corrector.remove_leading(expression, leading)
126+
corrector.remove_trailing(expression, trailing)
127+
end
128+
129+
def set_new_method_name(new_method_name, corrector)
130+
range = block_node.send_node.loc.selector
131+
if (send_end = block_node.send_node.loc.end)
132+
# If there are arguments (only true in the `each_with_object` case)
133+
range = range.begin.join(send_end)
134+
end
135+
corrector.replace(range, new_method_name)
136+
end
137+
138+
def set_new_arg_name(transformed_argname, corrector)
139+
corrector.replace(
140+
block_node.arguments.loc.expression,
141+
"|#{transformed_argname}|"
142+
)
143+
end
144+
145+
def set_new_body_expression(transforming_body_expr, corrector)
146+
corrector.replace(
147+
block_node.body.loc.expression,
148+
transforming_body_expr.loc.expression.source
149+
)
150+
end
151+
end
152+
end
153+
end
154+
end

lib/rubocop/cop/rails/index_by.rb

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Rails
6+
# This cop looks for uses of `each_with_object({}) { ... }`,
7+
# `map { ... }.to_h`, and `Hash[map { ... }]` that are transforming
8+
# an enumerable into a hash where the values are the original elements.
9+
# Rails provides the `index_by` method for this purpose.
10+
#
11+
# @example
12+
# # bad
13+
# [1, 2, 3].each_with_object({}) { |el, h| h[foo(el)] = el }
14+
# [1, 2, 3].map { |el| [foo(el), el] }.to_h
15+
# Hash[[1, 2, 3].collect { |el| [foo(el), el] }]
16+
#
17+
# # good
18+
# [1, 2, 3].index_by { |el| foo(el) }
19+
class IndexBy < Cop
20+
include IndexMethod
21+
22+
def_node_matcher :on_bad_each_with_object, <<~PATTERN
23+
(block
24+
({send csend} _ :each_with_object (hash))
25+
(args (arg $_el) (arg _memo))
26+
({send csend} (lvar _memo) :[]= $_ (lvar _el)))
27+
PATTERN
28+
29+
def_node_matcher :on_bad_map_to_h, <<~PATTERN
30+
({send csend}
31+
(block
32+
({send csend} _ {:map :collect})
33+
(args (arg $_el))
34+
(array $_ (lvar _el)))
35+
:to_h)
36+
PATTERN
37+
38+
def_node_matcher :on_bad_hash_brackets_map, <<~PATTERN
39+
(send
40+
(const _ :Hash)
41+
:[]
42+
(block
43+
({send csend} _ {:map :collect})
44+
(args (arg $_el))
45+
(array $_ (lvar _el))))
46+
PATTERN
47+
48+
private
49+
50+
def new_method_name
51+
'index_by'
52+
end
53+
end
54+
end
55+
end
56+
end
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Rails
6+
# This cop looks for uses of `each_with_object({}) { ... }`,
7+
# `map { ... }.to_h`, and `Hash[map { ... }]` that are transforming
8+
# an enumerable into a hash where the keys are the original elements.
9+
# Rails provides the `index_with` method for this purpose.
10+
#
11+
# @example
12+
# # bad
13+
# [1, 2, 3].each_with_object({}) { |el, h| h[el] = foo(el) }
14+
# [1, 2, 3].map { |el| [el, foo(el)] }.to_h
15+
# Hash[[1, 2, 3].collect { |el| [el, foo(el)] }]
16+
#
17+
# # good
18+
# [1, 2, 3].index_with { |el| foo(el) }
19+
class IndexWith < Cop
20+
extend TargetRailsVersion
21+
include IndexMethod
22+
23+
minimum_target_rails_version 6.0
24+
25+
def_node_matcher :on_bad_each_with_object, <<~PATTERN
26+
(block
27+
({send csend} _ :each_with_object (hash))
28+
(args (arg $_el) (arg _memo))
29+
({send csend} (lvar _memo) :[]= (lvar _el) $_))
30+
PATTERN
31+
32+
def_node_matcher :on_bad_map_to_h, <<~PATTERN
33+
({send csend}
34+
(block
35+
({send csend} _ {:map :collect})
36+
(args (arg $_el))
37+
(array (lvar _el) $_))
38+
:to_h)
39+
PATTERN
40+
41+
def_node_matcher :on_bad_hash_brackets_map, <<~PATTERN
42+
(send
43+
(const _ :Hash)
44+
:[]
45+
(block
46+
({send csend} _ {:map :collect})
47+
(args (arg $_el))
48+
(array (lvar _el) $_)))
49+
PATTERN
50+
51+
private
52+
53+
def new_method_name
54+
'index_with'
55+
end
56+
end
57+
end
58+
end
59+
end

lib/rubocop/cop/rails_cops.rb

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# frozen_string_literal: true
22

33
require_relative 'mixin/active_record_helper'
4+
require_relative 'mixin/index_method'
45
require_relative 'mixin/target_rails_version'
56

67
require_relative 'rails/action_filter'
@@ -33,6 +34,8 @@
3334
require_relative 'rails/http_positional_arguments'
3435
require_relative 'rails/http_status'
3536
require_relative 'rails/ignored_skip_action_filter_option'
37+
require_relative 'rails/index_by'
38+
require_relative 'rails/index_with'
3639
require_relative 'rails/inverse_of'
3740
require_relative 'rails/lexically_scoped_action_filter'
3841
require_relative 'rails/link_to_blank'

manual/cops.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
* [Rails/HttpPositionalArguments](cops_rails.md#railshttppositionalarguments)
3232
* [Rails/HttpStatus](cops_rails.md#railshttpstatus)
3333
* [Rails/IgnoredSkipActionFilterOption](cops_rails.md#railsignoredskipactionfilteroption)
34+
* [Rails/IndexBy](cops_rails.md#railsindexby)
35+
* [Rails/IndexWith](cops_rails.md#railsindexwith)
3436
* [Rails/InverseOf](cops_rails.md#railsinverseof)
3537
* [Rails/LexicallyScopedActionFilter](cops_rails.md#railslexicallyscopedactionfilter)
3638
* [Rails/LinkToBlank](cops_rails.md#railslinktoblank)

manual/cops_rails.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1137,6 +1137,52 @@ Include | `app/controllers/**/*.rb` | Array
11371137

11381138
* [https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options](https://api.rubyonrails.org/classes/AbstractController/Callbacks/ClassMethods.html#method-i-_normalize_callback_options)
11391139

1140+
## Rails/IndexBy
1141+
1142+
Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
1143+
--- | --- | --- | --- | ---
1144+
Enabled | Yes | Yes | 2.5 | -
1145+
1146+
This cop looks for uses of `each_with_object({}) { ... }`,
1147+
`map { ... }.to_h`, and `Hash[map { ... }]` that are transforming
1148+
an enumerable into a hash where the values are the original elements.
1149+
Rails provides the `index_by` method for this purpose.
1150+
1151+
### Examples
1152+
1153+
```ruby
1154+
# bad
1155+
[1, 2, 3].each_with_object({}) { |el, h| h[foo(el)] = el }
1156+
[1, 2, 3].map { |el| [foo(el), el] }.to_h
1157+
Hash[[1, 2, 3].collect { |el| [foo(el), el] }]
1158+
1159+
# good
1160+
[1, 2, 3].index_by { |el| foo(el) }
1161+
```
1162+
1163+
## Rails/IndexWith
1164+
1165+
Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
1166+
--- | --- | --- | --- | ---
1167+
Enabled | Yes | Yes | 2.5 | -
1168+
1169+
This cop looks for uses of `each_with_object({}) { ... }`,
1170+
`map { ... }.to_h`, and `Hash[map { ... }]` that are transforming
1171+
an enumerable into a hash where the keys are the original elements.
1172+
Rails provides the `index_with` method for this purpose.
1173+
1174+
### Examples
1175+
1176+
```ruby
1177+
# bad
1178+
[1, 2, 3].each_with_object({}) { |el, h| h[el] = foo(el) }
1179+
[1, 2, 3].map { |el| [el, foo(el)] }.to_h
1180+
Hash[[1, 2, 3].collect { |el| [el, foo(el)] }]
1181+
1182+
# good
1183+
[1, 2, 3].index_with { |el| foo(el) }
1184+
```
1185+
11401186
## Rails/InverseOf
11411187

11421188
Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

0 commit comments

Comments
 (0)