Skip to content

Commit 4e1e7e3

Browse files
r7kamurar7kamura
committed
Make Rails/RootPathnameMethods autocorrection safe
1 parent 2235d8c commit 4e1e7e3

File tree

4 files changed

+97
-32
lines changed

4 files changed

+97
-32
lines changed

‎changelog/change_make_rails_root_pathname_methods.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#993](https://github.com/rubocop/rubocop-rails/pull/993): Make `Rails/RootPathnameMethods` autocorrection safe. ([@r7kamura][])

‎config/default.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -878,8 +878,8 @@ Rails/RootJoinChain:
878878
Rails/RootPathnameMethods:
879879
Description: 'Use `Rails.root` IO methods instead of passing it to `File`.'
880880
Enabled: pending
881-
SafeAutoCorrect: false
882881
VersionAdded: '2.16'
882+
VersionChanged: '<<next>>'
883883

884884
Rails/RootPublicPath:
885885
Description: "Favor `Rails.public_path` over `Rails.root` with `'public'`."

‎lib/rubocop/cop/rails/root_pathname_methods.rb

Lines changed: 46 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,6 @@ module Rails
1111
# This cop works best when used together with
1212
# `Style/FileRead`, `Style/FileWrite` and `Rails/RootJoinChain`.
1313
#
14-
# @safety
15-
# This cop is unsafe for autocorrection because `Dir`'s `children`, `each_child`, `entries`, and `glob`
16-
# methods return string element, but these methods of `Pathname` return `Pathname` element.
17-
#
1814
# @example
1915
# # bad
2016
# File.open(Rails.root.join('db', 'schema.rb'))
@@ -32,17 +28,20 @@ module Rails
3228
# Rails.root.join('db', 'schema.rb').write(content)
3329
# Rails.root.join('db', 'schema.rb').binwrite(content)
3430
#
35-
class RootPathnameMethods < Base
31+
class RootPathnameMethods < Base # rubocop:disable Metrics/ClassLength
3632
extend AutoCorrector
3733
include RangeHelp
3834

3935
MSG = '`%<rails_root>s` is a `Pathname` so you can just append `#%<method>s`.'
4036

41-
DIR_METHODS = %i[children delete each_child empty? entries exist? glob mkdir open rmdir unlink].to_set.freeze
37+
DIR_NON_PATHNAMES_RETURNED_METHODS = %i[delete empty? exist? mkdir open rmdir unlink].to_set.freeze
38+
39+
DIR_PATHNAMES_RETURNED_METHODS = %i[children each_child entries glob].to_set.freeze
4240

43-
FILE_METHODS = %i[
41+
DIR_METHODS = (DIR_PATHNAMES_RETURNED_METHODS + DIR_NON_PATHNAMES_RETURNED_METHODS).freeze
42+
43+
FILE_NON_PATHNAME_RETURNED_METHODS = %i[
4444
atime
45-
basename
4645
binread
4746
binwrite
4847
birthtime
@@ -53,19 +52,16 @@ class RootPathnameMethods < Base
5352
ctime
5453
delete
5554
directory?
56-
dirname
5755
empty?
5856
executable?
5957
executable_real?
6058
exist?
61-
expand_path
6259
extname
6360
file?
6461
fnmatch
6562
fnmatch?
6663
ftype
6764
grpowned?
68-
join
6965
lchmod
7066
lchown
7167
lstat
@@ -77,9 +73,6 @@ class RootPathnameMethods < Base
7773
readable?
7874
readable_real?
7975
readlines
80-
readlink
81-
realdirpath
82-
realpath
8376
rename
8477
setgid?
8578
setuid?
@@ -102,6 +95,18 @@ class RootPathnameMethods < Base
10295
zero?
10396
].to_set.freeze
10497

98+
FILE_PATHNAME_RETURNED_METHODS = %i[
99+
basename
100+
dirname
101+
expand_path
102+
join
103+
readlink
104+
realdirpath
105+
realpath
106+
].to_set.freeze
107+
108+
FILE_METHODS = (FILE_PATHNAME_RETURNED_METHODS + FILE_NON_PATHNAME_RETURNED_METHODS).freeze
109+
105110
FILE_TEST_METHODS = %i[
106111
blockdev?
107112
chardev?
@@ -160,13 +165,24 @@ class RootPathnameMethods < Base
160165
(send (const {nil? cbase} :Rails) {:root :public_path})
161166
PATTERN
162167

168+
# @!method dir_pathnames_returned_method?(node)
169+
def_node_matcher :dir_pathnames_returned_method?, <<~PATTERN
170+
(send (const {nil? cbase} :Dir) DIR_PATHNAMES_RETURNED_METHODS ...)
171+
PATTERN
172+
173+
# @!method file_pathname_returned_method?(node)
174+
def_node_matcher :file_pathname_returned_method?, <<~PATTERN
175+
(send (const {nil? cbase} {:IO :File}) FILE_PATHNAME_RETURNED_METHODS ...)
176+
PATTERN
177+
163178
def on_send(node)
164179
evidence(node) do |method, path, args, rails_root|
165180
add_offense(node, message: format(MSG, method: method, rails_root: rails_root.source)) do |corrector|
181+
suffix = build_replacement_suffix(node)
166182
replacement = if dir_glob?(node)
167-
build_path_glob_replacement(path, method)
183+
build_path_glob_replacement(path, method, suffix)
168184
else
169-
build_path_replacement(path, method, args)
185+
build_path_replacement(path, method, args, suffix)
170186
end
171187

172188
corrector.replace(node, replacement)
@@ -183,15 +199,15 @@ def evidence(node)
183199
yield(method, path, args, rails_root)
184200
end
185201

186-
def build_path_glob_replacement(path, method)
202+
def build_path_glob_replacement(path, method, suffix)
187203
receiver = range_between(path.source_range.begin_pos, path.children.first.loc.selector.end_pos).source
188204

189205
argument = path.arguments.one? ? path.first_argument.source : join_arguments(path.arguments)
190206

191-
"#{receiver}.#{method}(#{argument})"
207+
"#{receiver}.#{method}(#{argument})#{suffix}"
192208
end
193209

194-
def build_path_replacement(path, method, args)
210+
def build_path_replacement(path, method, args, suffix)
195211
path_replacement = path.source
196212
if path.arguments? && !path.parenthesized_call?
197213
path_replacement[' '] = '('
@@ -200,9 +216,20 @@ def build_path_replacement(path, method, args)
200216

201217
replacement = "#{path_replacement}.#{method}"
202218
replacement += "(#{args.map(&:source).join(', ')})" unless args.empty?
219+
replacement += suffix
203220
replacement
204221
end
205222

223+
def build_replacement_suffix(node)
224+
if dir_pathnames_returned_method?(node)
225+
'.map(&:to_s)'
226+
elsif file_pathname_returned_method?(node)
227+
'.to_s'
228+
else
229+
''
230+
end
231+
end
232+
206233
def include_interpolation?(arguments)
207234
arguments.any? do |argument|
208235
argument.children.any? { |child| child.respond_to?(:begin_type?) && child.begin_type? }

‎spec/rubocop/cop/rails/root_pathname_methods_spec.rb

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,13 @@
22

33
RSpec.describe RuboCop::Cop::Rails::RootPathnameMethods, :config do
44
{
5-
Dir: described_class::DIR_METHODS,
6-
File: described_class::FILE_METHODS,
5+
Dir: described_class::DIR_NON_PATHNAMES_RETURNED_METHODS,
6+
File: described_class::FILE_NON_PATHNAME_RETURNED_METHODS,
77
FileTest: described_class::FILE_TEST_METHODS,
88
FileUtils: described_class::FILE_UTILS_METHODS,
9-
IO: described_class::FILE_METHODS
9+
IO: described_class::FILE_NON_PATHNAME_RETURNED_METHODS
1010
}.each do |receiver, methods|
1111
methods.each do |method|
12-
next if method == :glob
13-
1412
it "registers an offense when using `#{receiver}.#{method}(Rails.public_path)` (if arity exists)" do
1513
expect_offense(<<~RUBY, receiver: receiver, method: method)
1614
%{receiver}.%{method}(Rails.public_path)
@@ -54,7 +52,7 @@
5452
RUBY
5553

5654
expect_correction(<<~RUBY)
57-
Rails.root.glob('**/*.rb')
55+
Rails.root.glob('**/*.rb').map(&:to_s)
5856
RUBY
5957
end
6058

@@ -65,7 +63,7 @@
6563
RUBY
6664

6765
expect_correction(<<~RUBY)
68-
Rails.root.glob('**/*.rb')
66+
Rails.root.glob('**/*.rb').map(&:to_s)
6967
RUBY
7068
end
7169

@@ -76,7 +74,7 @@
7674
RUBY
7775

7876
expect_correction(<<~'RUBY')
79-
Rails.root.glob("**/#{path}/*.rb")
77+
Rails.root.glob("**/#{path}/*.rb").map(&:to_s)
8078
RUBY
8179
end
8280

@@ -87,7 +85,7 @@
8785
RUBY
8886

8987
expect_correction(<<~RUBY)
90-
Rails.root.glob('**/*.rb')
88+
Rails.root.glob('**/*.rb').map(&:to_s)
9189
RUBY
9290
end
9391

@@ -103,7 +101,7 @@
103101
RUBY
104102

105103
expect_correction(<<~RUBY)
106-
Rails.root.glob("**/*.rb")
104+
Rails.root.glob("**/*.rb").map(&:to_s)
107105
RUBY
108106
end
109107
end
@@ -115,7 +113,7 @@
115113
RUBY
116114

117115
expect_correction(<<~'RUBY')
118-
Rails.root.glob("**/#{path}/*.rb")
116+
Rails.root.glob("**/#{path}/*.rb").map(&:to_s)
119117
RUBY
120118
end
121119

@@ -128,13 +126,52 @@
128126
RUBY
129127

130128
expect_correction(<<~'RUBY')
131-
Rails.root.glob("db/seeds/#{Rails.env}/*.rb").sort.each do |file|
129+
Rails.root.glob("db/seeds/#{Rails.env}/*.rb").map(&:to_s).sort.each do |file|
132130
load file
133131
end
134132
RUBY
135133
end
136134
end
137135

136+
{
137+
Dir: described_class::DIR_PATHNAMES_RETURNED_METHODS - %i[glob]
138+
}.each do |receiver, methods|
139+
methods.each do |method|
140+
context "when `#{receiver}.#{method}(Rails.root)` is used" do
141+
it 'registers an offense' do
142+
expect_offense(<<~RUBY, receiver: receiver, method: method)
143+
%{receiver}.%{method}(Rails.root)
144+
^{receiver}^^{method}^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#%{method}`.
145+
RUBY
146+
147+
expect_correction(<<~RUBY)
148+
Rails.root.#{method}.map(&:to_s)
149+
RUBY
150+
end
151+
end
152+
end
153+
end
154+
155+
{
156+
File: described_class::FILE_PATHNAME_RETURNED_METHODS,
157+
IO: described_class::FILE_PATHNAME_RETURNED_METHODS
158+
}.each do |receiver, methods|
159+
methods.each do |method|
160+
context "when `#{receiver}.#{method}(Rails.root)` is used" do
161+
it 'registers an offense' do
162+
expect_offense(<<~RUBY, receiver: receiver, method: method)
163+
%{receiver}.%{method}(Rails.root)
164+
^{receiver}^^{method}^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#%{method}`.
165+
RUBY
166+
167+
expect_correction(<<~RUBY)
168+
Rails.root.#{method}.to_s
169+
RUBY
170+
end
171+
end
172+
end
173+
end
174+
138175
# This is handled by `Rails/RootJoinChain`
139176
it 'does not register an offense when using `File.read(Rails.root.join(...).join(...))`' do
140177
expect_no_offenses(<<~RUBY)

0 commit comments

Comments
 (0)