From 94e126daf8f6a6e64e2e6821c4bb730ec7b5fa4f Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 10 Sep 2022 12:52:28 +0900 Subject: [PATCH] [Fix #763] Fix a false positive for `Rails/RootPathnameMethods` Fixes #763. This PR fix a false positive for `Rails/RootPathnameMethods` when using `Dir.glob`. And marks unsafe for autocorrection because `Dir`'s methods return string element, but `Pathname`'s methods return `Pathname` element. --- ...correct_for_rails_root_pathname_methods.md | 1 + config/default.yml | 1 + .../cop/rails/root_pathname_methods.rb | 43 +++++++++++++- .../cop/rails/root_pathname_methods_spec.rb | 59 +++++++++++++++++++ 4 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 changelog/fix_an_incorrect_autocorrect_for_rails_root_pathname_methods.md diff --git a/changelog/fix_an_incorrect_autocorrect_for_rails_root_pathname_methods.md b/changelog/fix_an_incorrect_autocorrect_for_rails_root_pathname_methods.md new file mode 100644 index 0000000000..682f446323 --- /dev/null +++ b/changelog/fix_an_incorrect_autocorrect_for_rails_root_pathname_methods.md @@ -0,0 +1 @@ +* [#763](https://github.com/rubocop/rubocop-rails/issues/763): Mark `Rails/RootPathnameMethods` as unsafe and fix an incorrect autocorrect when using `Dir.glob`. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index bb8a6376a3..c5587794a2 100644 --- a/config/default.yml +++ b/config/default.yml @@ -845,6 +845,7 @@ Rails/RootJoinChain: Rails/RootPathnameMethods: Description: 'Use `Rails.root` IO methods instead of passing it to `File`.' Enabled: pending + SafeAutocorrect: false VersionAdded: '2.16' Rails/RootPublicPath: diff --git a/lib/rubocop/cop/rails/root_pathname_methods.rb b/lib/rubocop/cop/rails/root_pathname_methods.rb index 5509d5eefd..b3122be505 100644 --- a/lib/rubocop/cop/rails/root_pathname_methods.rb +++ b/lib/rubocop/cop/rails/root_pathname_methods.rb @@ -11,6 +11,10 @@ module Rails # This cop works best when used together with # `Style/FileRead`, `Style/FileWrite` and `Rails/RootJoinChain`. # + # @safety + # This cop is unsafe for autocorrection because `Dir`'s `children`, `each_child`, `entries`, and `glob` + # methods return string element, but these methods of `Pathname` return `Pathname` element. + # # @example # # bad # File.open(Rails.root.join('db', 'schema.rb')) @@ -30,6 +34,7 @@ module Rails # class RootPathnameMethods < Base extend AutoCorrector + include RangeHelp MSG = '`%s` is a `Pathname` so you can just append `#%s`.' @@ -138,6 +143,11 @@ class RootPathnameMethods < Base } PATTERN + def_node_matcher :dir_glob?, <<~PATTERN + (send + (const {cbase nil?} :Dir) :glob ...) + PATTERN + def_node_matcher :rails_root_pathname?, <<~PATTERN { $#rails_root? @@ -153,8 +163,12 @@ class RootPathnameMethods < Base def on_send(node) evidence(node) do |method, path, args, rails_root| add_offense(node, message: format(MSG, method: method, rails_root: rails_root.source)) do |corrector| - replacement = "#{path.source}.#{method}" - replacement += "(#{args.map(&:source).join(', ')})" unless args.empty? + if dir_glob?(node) + replacement = build_path_glob(path, method) + else + replacement = "#{path.source}.#{method}" + replacement += "(#{args.map(&:source).join(', ')})" unless args.empty? + end corrector.replace(node, replacement) end @@ -169,6 +183,31 @@ def evidence(node) yield(method, path, args, rails_root) end + + def build_path_glob(path, method) + receiver = range_between(path.loc.expression.begin_pos, path.children.first.loc.selector.end_pos).source + + argument = if path.arguments.one? + path.first_argument.source + else + join_arguments(path.arguments) + end + + "#{receiver}.#{method}(#{argument})" + end + + def include_interpolation?(arguments) + arguments.any? do |argument| + argument.children.any? { |child| child.respond_to?(:begin_type?) && child.begin_type? } + end + end + + def join_arguments(arguments) + quote = include_interpolation?(arguments) ? '"' : "'" + joined_arguments = arguments.map(&:value).join('/') + + "#{quote}#{joined_arguments}#{quote}" + end end end end diff --git a/spec/rubocop/cop/rails/root_pathname_methods_spec.rb b/spec/rubocop/cop/rails/root_pathname_methods_spec.rb index 889cea5914..019185f154 100644 --- a/spec/rubocop/cop/rails/root_pathname_methods_spec.rb +++ b/spec/rubocop/cop/rails/root_pathname_methods_spec.rb @@ -9,6 +9,8 @@ IO: described_class::FILE_METHODS }.each do |receiver, methods| methods.each do |method| + next if method == :glob + it "registers an offense when using `#{receiver}.#{method}(Rails.public_path)` (if arity exists)" do expect_offense(<<~RUBY, receiver: receiver, method: method) %{receiver}.%{method}(Rails.public_path) @@ -44,6 +46,63 @@ end end + context 'when using `Dir.glob`' do + it "registers an offense when using `Dir.glob(Rails.root.join('**/*.rb'))`" do + expect_offense(<<~RUBY) + Dir.glob(Rails.root.join('**/*.rb')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#glob`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.glob('**/*.rb') + RUBY + end + + it "registers an offense when using `::Dir.glob(Rails.root.join('**/*.rb'))`" do + expect_offense(<<~RUBY) + ::Dir.glob(Rails.root.join('**/*.rb')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#glob`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.glob('**/*.rb') + RUBY + end + + it "registers an offense when using `Dir.glob(Rails.root.join('**/\#{path}/*.rb'))`" do + expect_offense(<<~'RUBY') + Dir.glob(Rails.root.join("**/#{path}/*.rb")) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#glob`. + RUBY + + expect_correction(<<~'RUBY') + Rails.root.glob("**/#{path}/*.rb") + RUBY + end + + it "registers an offense when using `Dir.glob(Rails.root.join('**', '*.rb'))`" do + expect_offense(<<~RUBY) + Dir.glob(Rails.root.join('**', '*.rb')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#glob`. + RUBY + + expect_correction(<<~RUBY) + Rails.root.glob('**/*.rb') + RUBY + end + + it "registers an offense when using `Dir.glob(Rails.root.join('**', \"\#{path}\", '*.rb'))`" do + expect_offense(<<~'RUBY') + Dir.glob(Rails.root.join('**', "#{path}", '*.rb')) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Rails.root` is a `Pathname` so you can just append `#glob`. + RUBY + + expect_correction(<<~'RUBY') + Rails.root.glob("**/#{path}/*.rb") + RUBY + end + end + # This is handled by `Rails/RootJoinChain` it 'does not register an offense when using `File.read(Rails.root.join(...).join(...))`' do expect_no_offenses(<<~RUBY)