From dee1b76d550092a4e5d5809515affe09968226e9 Mon Sep 17 00:00:00 2001 From: Yauheni Dakuka Date: Sun, 19 Jan 2025 20:11:51 +0400 Subject: [PATCH] [Fix rubocop#1053] Fix an error occurred for Rails/FilePath cop when nested File.join --- .rubocop_todo.yml | 2 +- ...ils_file_path_cop_when_nested_file_join.md | 1 + lib/rubocop/cop/rails/file_path.rb | 92 ++++++- spec/rubocop/cop/rails/file_path_spec.rb | 234 ++++++++++++++++++ 4 files changed, 323 insertions(+), 6 deletions(-) create mode 100644 changelog/fix_an_error_occurred_for_rails_file_path_cop_when_nested_file_join.md diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 098642cde0..4c9f429a3b 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -29,7 +29,7 @@ InternalAffairs/OnSendWithoutOnCSend: # Offense count: 10 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 179 + Max: 163 # Offense count: 41 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. diff --git a/changelog/fix_an_error_occurred_for_rails_file_path_cop_when_nested_file_join.md b/changelog/fix_an_error_occurred_for_rails_file_path_cop_when_nested_file_join.md new file mode 100644 index 0000000000..54599cbd5c --- /dev/null +++ b/changelog/fix_an_error_occurred_for_rails_file_path_cop_when_nested_file_join.md @@ -0,0 +1 @@ +* [#1053](https://github.com/rubocop/rubocop-rails/issues/1053): Fix an error occurred for `Rails/FilePath` cop when nested `File.join`. ([@ydakuka][]) diff --git a/lib/rubocop/cop/rails/file_path.rb b/lib/rubocop/cop/rails/file_path.rb index 375f43e7b0..bfe27c760f 100644 --- a/lib/rubocop/cop/rails/file_path.rb +++ b/lib/rubocop/cop/rails/file_path.rb @@ -34,7 +34,7 @@ module Rails # # good # Rails.root.join('app', 'models', 'goober').to_s # - class FilePath < Base + class FilePath < Base # rubocop:disable Metrics/ClassLength extend AutoCorrector include ConfigurableEnforcedStyle @@ -48,6 +48,14 @@ class FilePath < Base (send (const {nil? cbase} :File) :join ...) PATTERN + def_node_matcher :file_join_with_rails_root_nodes?, <<~PATTERN + (send (const {nil? cbase} :File) :join (_ $_)* {(send #rails_root_nodes? :to_s) #rails_root_nodes?} ...) + PATTERN + + def_node_matcher :file_join_with_rails_root_join_nodes?, <<~PATTERN + (send (const {nil? cbase} :File) :join (_ $_)* {(send #rails_root_join_nodes? :to_s) #rails_root_join_nodes?} ...) + PATTERN + def_node_search :rails_root_nodes?, <<~PATTERN (send (const {nil? cbase} :Rails) :root) PATTERN @@ -65,8 +73,11 @@ def on_dstr(node) end def on_send(node) + check_for_file_join_with_rails_root_join(node) check_for_file_join_with_rails_root(node) + return unless node.receiver + return if node.ancestors.any? { |ancestor| file_join_nodes?(ancestor) } check_for_rails_root_join_with_slash_separated_path(node) check_for_rails_root_join_with_string_arguments(node) @@ -96,11 +107,21 @@ def check_for_extension_after_rails_root_join_in_dstr(node) end def check_for_file_join_with_rails_root(node) - return unless file_join_nodes?(node) + return unless file_join_with_rails_root_nodes?(node) return unless node.arguments.any? { |e| rails_root_nodes?(e) } + return if node.descendants.any? { |descendant| file_join_nodes?(descendant) } + + register_offense(node, require_to_s: true) do |corrector| + autocorrect_file_join_with_rails_root(corrector, node) unless node.first_argument.array_type? + end + end + + def check_for_file_join_with_rails_root_join(node) + return unless file_join_with_rails_root_join_nodes?(node) + register_offense(node, require_to_s: true) do |corrector| - autocorrect_file_join(corrector, node) unless node.first_argument.array_type? + autocorrect_file_join_with_rails_root_join(corrector, node) unless node.first_argument.array_type? end end @@ -120,7 +141,7 @@ def check_for_rails_root_join_with_slash_separated_path(node) return unless style == :arguments return unless rails_root_nodes?(node) return unless rails_root_join_nodes?(node) - return unless node.arguments.any? { |arg| string_with_slash?(arg) } + return unless node.arguments.any? { |argument| string_with_slash?(argument) } register_offense(node, require_to_s: false) do |corrector| autocorrect_rails_root_join_with_slash_separated_path(corrector, node) @@ -172,7 +193,7 @@ def autocorrect_extension_after_rails_root_join_in_dstr(corrector, node, rails_r corrector.remove(extension_node) end - def autocorrect_file_join(corrector, node) + def autocorrect_file_join_with_rails_root(corrector, node) replace_receiver_with_rails_root(corrector, node) remove_first_argument_with_comma(corrector, node) process_arguments(corrector, node.arguments) @@ -209,6 +230,67 @@ def append_to_string_conversion(corrector, node) corrector.insert_after(node, '.to_s') end + def autocorrect_file_join_with_rails_root_join(corrector, node) + rails_root_join_node = find_rails_root_join_node(node) + return unless rails_root_join_node + + combined_arguments = combine_arguments(rails_root_join_node, node) + corrector.replace(node, "Rails.root.join(#{combined_arguments}).to_s") + end + + def find_rails_root_join_node(node) + node.arguments.find { |argument| rails_root_join_typical?(argument) } + end + + def rails_root_join_typical?(node) + rails_root_join_nodes?(node) || (node.send_type? && rails_root_join_nodes?(node.receiver)) + end + + def extract_rails_root_call(rails_root_join_node) + if rails_root_join_node.send_type? && rails_root_join_node.method?(:to_s) + rails_root_join_node.receiver + else + rails_root_join_node + end + end + + def combine_arguments(rails_root_join_node, node) + rails_root_call = extract_rails_root_call(rails_root_join_node) + rails_root_arguments = rails_root_call.arguments + other_arguments = node.arguments.reject { |argument| argument == rails_root_join_node } + + case style + when :arguments + combine_as_arguments(rails_root_arguments, other_arguments) + when :slashes + combine_as_slashes(rails_root_arguments, other_arguments) + end + end + + def combine_as_arguments(rails_root_arguments, other_arguments) + first_argument = other_arguments.first + + other_arguments_values = + if first_argument.dstr_type? + first_argument.children.map do |child| + "'#{child.value.delete_prefix('/')}'" + end + else + ["'#{first_argument.value.delete_prefix('/')}'"] + end + + (rails_root_arguments.map(&:source) + other_arguments_values).join(', ') + end + + def combine_as_slashes(rails_root_arguments, other_arguments) + return "'#{rails_root_arguments.map(&:value).join}'" if other_arguments.empty? + + first_value = other_arguments.first.value + separator = first_value.start_with?(File::SEPARATOR) ? '' : '/' + + "'#{(rails_root_arguments + other_arguments).map(&:value).join(separator)}'" + end + def autocorrect_rails_root_join_with_string_arguments(corrector, node) corrector.replace(node.first_argument, %("#{node.arguments.map(&:value).join('/')}")) node.arguments[1..].each do |argument| diff --git a/spec/rubocop/cop/rails/file_path_spec.rb b/spec/rubocop/cop/rails/file_path_spec.rb index 6bcdd88dea..248b8a7920 100644 --- a/spec/rubocop/cop/rails/file_path_spec.rb +++ b/spec/rubocop/cop/rails/file_path_spec.rb @@ -322,6 +322,123 @@ RUBY end end + + context 'when File.join wraps Rails.root.join with string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app', 'models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/`' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models'), '/vehicle' '/car.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/vehicle/car.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with string arguments and .to_s' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app', 'models').to_s, 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments and .to_s' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models').to_s, 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/` and .to_s' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models').to_s, '/vehicle' '/car.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/vehicle/car.rb').to_s + RUBY + end + end + + context 'when using nested File.join with Rails.root with string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(File.join(Rails.root, 'app', 'models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/user.rb').to_s + RUBY + end + end + + context 'when using nested File.join with Rails.root with non-string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(File.join(Rails.root, 'app/models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/user.rb').to_s + RUBY + end + end + + context 'when using nested File.join with Rails.root with non-string arguments and path starting with `/`' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(File.join(Rails.root, '/app/models'), '/user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path/to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app/models/user.rb').to_s + RUBY + end + end end context 'when EnforcedStyle is `arguments`' do @@ -592,5 +709,122 @@ RUBY end end + + context 'when File.join wraps Rails.root.join with string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app', 'models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app', 'models', 'user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app', "models", 'user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/`' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models'), '/vehicle' '/car.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app', "models", 'vehicle', 'car.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with string arguments and .to_s' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app', 'models').to_s, 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app', 'models', 'user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments and .to_s' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models').to_s, 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app', "models", 'user.rb').to_s + RUBY + end + end + + context 'when File.join wraps Rails.root.join with non-string arguments and path starting with `/` and .to_s' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(Rails.root.join('app/models').to_s, '/vehicle' '/car.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join('app', "models", 'vehicle', 'car.rb').to_s + RUBY + end + end + + context 'when using nested File.join with Rails.root with string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(File.join(Rails.root, 'app', 'models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join("app", "models", 'user.rb').to_s + RUBY + end + end + + context 'when using nested File.join with Rails.root with non-string arguments' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(File.join(Rails.root, 'app/models'), 'user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join("app", "models", 'user.rb').to_s + RUBY + end + end + + context 'when using nested File.join with Rails.root with non-string arguments and path starting with `/`' do + it 'registers an offense once' do + expect_offense(<<~RUBY) + File.join(File.join(Rails.root, '/app/models'), '/user.rb') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `Rails.root.join('path', 'to').to_s`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.join("app", "models", 'user.rb').to_s + RUBY + end + end end end