Skip to content

Commit cebe1f1

Browse files
ydakukaydakuka
committed
[Fix rubocop#1053] Fix an error occurred for Rails/FilePath cop when nested File.join
1 parent 4d0e655 commit cebe1f1

File tree

3 files changed

+318
-3
lines changed

3 files changed

+318
-3
lines changed

‎changelog/fix_an_error_occurred_for_rails_file_path_cop_when_nested_file_join.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#1053](https://github.com/rubocop/rubocop-rails/issues/1053): Fix an error occurred for `Rails/FilePath` cop when nested `File.join`. ([@ydakuka][])

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

Lines changed: 83 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,14 @@ class FilePath < Base
5151
(send (const {nil? cbase} :File) :join ...)
5252
PATTERN
5353

54+
def_node_matcher :file_join_with_rails_root_nodes?, <<~PATTERN
55+
(send (const {nil? cbase} :File) :join (_ $_)* {(send #rails_root_nodes? :to_s) #rails_root_nodes?} ...)
56+
PATTERN
57+
58+
def_node_matcher :file_join_with_rails_root_join_nodes?, <<~PATTERN
59+
(send (const {nil? cbase} :File) :join (_ $_)* {(send #rails_root_join_nodes? :to_s) #rails_root_join_nodes?} ...)
60+
PATTERN
61+
5462
def_node_search :rails_root_nodes?, <<~PATTERN
5563
(send (const {nil? cbase} :Rails) :root)
5664
PATTERN
@@ -68,8 +76,10 @@ def on_dstr(node)
6876
end
6977

7078
def on_send(node)
79+
check_for_file_join_with_rails_root_join(node)
7180
check_for_file_join_with_rails_root(node)
7281
return unless node.receiver
82+
return if node.ancestors.any? { |ancestor| file_join_nodes?(ancestor) }
7383

7484
check_for_rails_root_join_with_slash_separated_path(node)
7585
check_for_rails_root_join_with_string_arguments(node)
@@ -99,11 +109,20 @@ def check_for_extension_after_rails_root_join_in_dstr(node)
99109
end
100110

101111
def check_for_file_join_with_rails_root(node)
102-
return unless file_join_nodes?(node)
112+
return unless file_join_with_rails_root_nodes?(node)
103113
return unless valid_arguments_for_file_join_with_rails_root?(node.arguments)
114+
return if node.descendants.any? { |descendant| file_join_nodes?(descendant) }
115+
116+
register_offense(node, require_to_s: true) do |corrector|
117+
autocorrect_file_join_with_rails_root(corrector, node) unless node.first_argument.array_type?
118+
end
119+
end
120+
121+
def check_for_file_join_with_rails_root_join(node)
122+
return unless file_join_with_rails_root_join_nodes?(node)
104123

105124
register_offense(node, require_to_s: true) do |corrector|
106-
autocorrect_file_join(corrector, node) unless node.first_argument.array_type?
125+
autocorrect_file_join_with_rails_root_join(corrector, node)
107126
end
108127
end
109128

@@ -201,7 +220,7 @@ def autocorrect_extension_after_rails_root_join_in_dstr(corrector, node, rails_r
201220
corrector.remove(extension_node)
202221
end
203222

204-
def autocorrect_file_join(corrector, node)
223+
def autocorrect_file_join_with_rails_root(corrector, node)
205224
replace_receiver_with_rails_root(corrector, node)
206225
remove_first_argument_with_comma(corrector, node)
207226
process_arguments(corrector, node.arguments)
@@ -238,6 +257,67 @@ def append_to_string_conversion(corrector, node)
238257
corrector.insert_after(node, '.to_s')
239258
end
240259

260+
def autocorrect_file_join_with_rails_root_join(corrector, node)
261+
rails_root_join_node = find_rails_root_join_node(node)
262+
return unless rails_root_join_node
263+
264+
combined_arguments = combine_arguments(rails_root_join_node, node)
265+
corrector.replace(node, "Rails.root.join(#{combined_arguments}).to_s")
266+
end
267+
268+
def find_rails_root_join_node(node)
269+
node.arguments.find { |argument| rails_root_join_typical?(argument) }
270+
end
271+
272+
def rails_root_join_typical?(node)
273+
rails_root_join_nodes?(node) || (node.send_type? && rails_root_join_nodes?(node.receiver))
274+
end
275+
276+
def extract_rails_root_call(rails_root_join_node)
277+
if rails_root_join_node.send_type? && rails_root_join_node.method?(:to_s)
278+
rails_root_join_node.receiver
279+
else
280+
rails_root_join_node
281+
end
282+
end
283+
284+
def combine_arguments(rails_root_join_node, node)
285+
rails_root_call = extract_rails_root_call(rails_root_join_node)
286+
rails_root_arguments = rails_root_call.arguments
287+
other_arguments = node.arguments.reject { |argument| argument == rails_root_join_node }
288+
289+
case style
290+
when :arguments
291+
combine_as_arguments(rails_root_arguments, other_arguments)
292+
when :slashes
293+
combine_as_slashes(rails_root_arguments, other_arguments)
294+
end
295+
end
296+
297+
def combine_as_arguments(rails_root_arguments, other_arguments)
298+
first_argument = other_arguments.first
299+
300+
other_arguments_values =
301+
if first_argument.dstr_type?
302+
first_argument.children.map do |child|
303+
"'#{child.value.delete_prefix('/')}'"
304+
end
305+
else
306+
["'#{first_argument.value.delete_prefix('/')}'"]
307+
end
308+
309+
(rails_root_arguments.map(&:source) + other_arguments_values).join(', ')
310+
end
311+
312+
def combine_as_slashes(rails_root_arguments, other_arguments)
313+
return "'#{rails_root_arguments.map(&:value).join}'" if other_arguments.empty?
314+
315+
first_value = other_arguments.first.value
316+
separator = first_value.start_with?(File::SEPARATOR) ? '' : '/'
317+
318+
"'#{(rails_root_arguments + other_arguments).map(&:value).join(separator)}'"
319+
end
320+
241321
def autocorrect_rails_root_join_with_string_arguments(corrector, node)
242322
corrector.replace(node.first_argument, %("#{node.arguments.map(&:value).join('/')}"))
243323
node.arguments[1..].each do |argument|

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

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,123 @@
444444
RUBY
445445
end
446446
end
447+
448+
context 'when File.join wraps Rails.root.join with string arguments' do
449+
it 'registers an offense once' do
450+
expect_offense(<<~RUBY)
451+
File.join(Rails.root.join('app', 'models'), 'user.rb')
452+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
453+
RUBY
454+
455+
expect_correction(<<~RUBY)
456+
Rails.root.join('app/models/user.rb').to_s
457+
RUBY
458+
end
459+
end
460+
461+
context 'when File.join wraps Rails.root.join with non-string arguments' do
462+
it 'registers an offense once' do
463+
expect_offense(<<~RUBY)
464+
File.join(Rails.root.join('app/models'), 'user.rb')
465+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
466+
RUBY
467+
468+
expect_correction(<<~RUBY)
469+
Rails.root.join('app/models/user.rb').to_s
470+
RUBY
471+
end
472+
end
473+
474+
context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/`' do
475+
it 'registers an offense once' do
476+
expect_offense(<<~RUBY)
477+
File.join(Rails.root.join('app/models'), '/vehicle' '/car.rb')
478+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
479+
RUBY
480+
481+
expect_correction(<<~RUBY)
482+
Rails.root.join('app/models/vehicle/car.rb').to_s
483+
RUBY
484+
end
485+
end
486+
487+
context 'when File.join wraps Rails.root.join with string arguments and .to_s' do
488+
it 'registers an offense once' do
489+
expect_offense(<<~RUBY)
490+
File.join(Rails.root.join('app', 'models').to_s, 'user.rb')
491+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
492+
RUBY
493+
494+
expect_correction(<<~RUBY)
495+
Rails.root.join('app/models/user.rb').to_s
496+
RUBY
497+
end
498+
end
499+
500+
context 'when File.join wraps Rails.root.join with non-string arguments and .to_s' do
501+
it 'registers an offense once' do
502+
expect_offense(<<~RUBY)
503+
File.join(Rails.root.join('app/models').to_s, 'user.rb')
504+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
505+
RUBY
506+
507+
expect_correction(<<~RUBY)
508+
Rails.root.join('app/models/user.rb').to_s
509+
RUBY
510+
end
511+
end
512+
513+
context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/` and .to_s' do
514+
it 'registers an offense once' do
515+
expect_offense(<<~RUBY)
516+
File.join(Rails.root.join('app/models').to_s, '/vehicle' '/car.rb')
517+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
518+
RUBY
519+
520+
expect_correction(<<~RUBY)
521+
Rails.root.join('app/models/vehicle/car.rb').to_s
522+
RUBY
523+
end
524+
end
525+
526+
context 'when using nested File.join with Rails.root with string arguments' do
527+
it 'registers an offense once' do
528+
expect_offense(<<~RUBY)
529+
File.join(File.join(Rails.root, 'app', 'models'), 'user.rb')
530+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
531+
RUBY
532+
533+
expect_correction(<<~RUBY)
534+
Rails.root.join('app/models/user.rb').to_s
535+
RUBY
536+
end
537+
end
538+
539+
context 'when using nested File.join with Rails.root with non-string arguments' do
540+
it 'registers an offense once' do
541+
expect_offense(<<~RUBY)
542+
File.join(File.join(Rails.root, 'app/models'), 'user.rb')
543+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
544+
RUBY
545+
546+
expect_correction(<<~RUBY)
547+
Rails.root.join('app/models/user.rb').to_s
548+
RUBY
549+
end
550+
end
551+
552+
context 'when using nested File.join with Rails.root with non-string arguments and path starting with `/`' do
553+
it 'registers an offense once' do
554+
expect_offense(<<~RUBY)
555+
File.join(File.join(Rails.root, '/app/models'), '/user.rb')
556+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`.
557+
RUBY
558+
559+
expect_correction(<<~RUBY)
560+
Rails.root.join('app/models/user.rb').to_s
561+
RUBY
562+
end
563+
end
447564
end
448565

449566
context 'when EnforcedStyle is `arguments`' do
@@ -754,5 +871,122 @@
754871
RUBY
755872
end
756873
end
874+
875+
context 'when File.join wraps Rails.root.join with string arguments' do
876+
it 'registers an offense once' do
877+
expect_offense(<<~RUBY)
878+
File.join(Rails.root.join('app', 'models'), 'user.rb')
879+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
880+
RUBY
881+
882+
expect_correction(<<~RUBY)
883+
Rails.root.join('app', 'models', 'user.rb').to_s
884+
RUBY
885+
end
886+
end
887+
888+
context 'when File.join wraps Rails.root.join with non-string arguments' do
889+
it 'registers an offense once' do
890+
expect_offense(<<~RUBY)
891+
File.join(Rails.root.join('app/models'), 'user.rb')
892+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
893+
RUBY
894+
895+
expect_correction(<<~RUBY)
896+
Rails.root.join('app', "models", 'user.rb').to_s
897+
RUBY
898+
end
899+
end
900+
901+
context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/`' do
902+
it 'registers an offense once' do
903+
expect_offense(<<~RUBY)
904+
File.join(Rails.root.join('app/models'), '/vehicle' '/car.rb')
905+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
906+
RUBY
907+
908+
expect_correction(<<~RUBY)
909+
Rails.root.join('app', "models", 'vehicle', 'car.rb').to_s
910+
RUBY
911+
end
912+
end
913+
914+
context 'when File.join wraps Rails.root.join with string arguments and .to_s' do
915+
it 'registers an offense once' do
916+
expect_offense(<<~RUBY)
917+
File.join(Rails.root.join('app', 'models').to_s, 'user.rb')
918+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
919+
RUBY
920+
921+
expect_correction(<<~RUBY)
922+
Rails.root.join('app', 'models', 'user.rb').to_s
923+
RUBY
924+
end
925+
end
926+
927+
context 'when File.join wraps Rails.root.join with non-string arguments and .to_s' do
928+
it 'registers an offense once' do
929+
expect_offense(<<~RUBY)
930+
File.join(Rails.root.join('app/models').to_s, 'user.rb')
931+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
932+
RUBY
933+
934+
expect_correction(<<~RUBY)
935+
Rails.root.join('app', "models", 'user.rb').to_s
936+
RUBY
937+
end
938+
end
939+
940+
context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/` and .to_s' do
941+
it 'registers an offense once' do
942+
expect_offense(<<~RUBY)
943+
File.join(Rails.root.join('app/models').to_s, '/vehicle' '/car.rb')
944+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
945+
RUBY
946+
947+
expect_correction(<<~RUBY)
948+
Rails.root.join('app', "models", 'vehicle', 'car.rb').to_s
949+
RUBY
950+
end
951+
end
952+
953+
context 'when using nested File.join with Rails.root with string arguments' do
954+
it 'registers an offense once' do
955+
expect_offense(<<~RUBY)
956+
File.join(File.join(Rails.root, 'app', 'models'), 'user.rb')
957+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
958+
RUBY
959+
960+
expect_correction(<<~RUBY)
961+
Rails.root.join("app", "models", 'user.rb').to_s
962+
RUBY
963+
end
964+
end
965+
966+
context 'when using nested File.join with Rails.root with non-string arguments' do
967+
it 'registers an offense once' do
968+
expect_offense(<<~RUBY)
969+
File.join(File.join(Rails.root, 'app/models'), 'user.rb')
970+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
971+
RUBY
972+
973+
expect_correction(<<~RUBY)
974+
Rails.root.join("app", "models", 'user.rb').to_s
975+
RUBY
976+
end
977+
end
978+
979+
context 'when using nested File.join with Rails.root with non-string arguments and path starting with `/`' do
980+
it 'registers an offense once' do
981+
expect_offense(<<~RUBY)
982+
File.join(File.join(Rails.root, '/app/models'), '/user.rb')
983+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`.
984+
RUBY
985+
986+
expect_correction(<<~RUBY)
987+
Rails.root.join("app", "models", 'user.rb').to_s
988+
RUBY
989+
end
990+
end
757991
end
758992
end

0 commit comments

Comments
 (0)