Merge pull request #10217 from mttkay/mk-avoid-openstruct

Add Style/OpenStructUse Cop

changelog/new_add_openstruct_use_cop.md

@@ -0,0 +1 @@
+* [#10217](https://github.com/rubocop/rubocop/pull/10217): Add new `Style/OpenStructUse` cop. ([@mttkay][])

config/default.yml

@@ -4216,6 +4216,16 @@ Style/OneLineConditional:
   VersionAdded: '0.9'
   VersionChanged: '0.90'
 
+Style/OpenStructUse:
+  Description: >-
+                 Avoid using OpenStruct. As of Ruby 3.0, use is officially discouraged due to performance,
+                 version compatibility, and potential security issues.
+  Reference:
+    - https://docs.ruby-lang.org/en/3.0.0/OpenStruct.html#class-OpenStruct-label-Caveats
+
+  Enabled: pending
+  VersionAdded: '<<next>>'
+
 Style/OptionHash:
   Description: "Don't use option hashes when you can use keyword arguments."
   Enabled: false

lib/rubocop.rb

@@ -514,6 +514,7 @@
 require_relative 'rubocop/cop/style/method_call_with_args_parentheses'
 require_relative 'rubocop/cop/style/multiline_in_pattern_then'
 require_relative 'rubocop/cop/style/numbered_parameters'
+require_relative 'rubocop/cop/style/open_struct_use'
 require_relative 'rubocop/cop/style/redundant_assignment'
 require_relative 'rubocop/cop/style/redundant_fetch_block'
 require_relative 'rubocop/cop/style/redundant_file_extension_in_require'

lib/rubocop/cop/style/open_struct_use.rb

@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Style
+      # This cop flags uses of OpenStruct, as it is now officially discouraged
+      # to be used for performance, version compatibility, and potential security issues.
+      #
+      # @safety
+      #
+      #   Note that this cop may flag false positives; for instance, the following legal
+      #   use of a hand-rolled `OpenStruct` type would be considered an offense:
+      #
+      #   ```
+      #   module MyNamespace
+      #     class OpenStruct # not the OpenStruct we're looking for
+      #     end
+      #
+      #     def new_struct
+      #       OpenStruct.new # resolves to MyNamespace::OpenStruct
+      #     end
+      #   end
+      #   ```
+      #
+      # @example
+      #
+      #   # bad
+      #   point = OpenStruct.new(x: 0, y: 1)
+      #
+      #   # good
+      #   Point = Struct.new(:x, :y)
+      #   point = Point.new(0, 1)
+      #
+      #   # also good
+      #   point = { x: 0, y: 1 }
+      #
+      #   # bad
+      #   test_double = OpenStruct.new(a: 'b')
+      #
+      #   # good (assumes test using rspec-mocks)
+      #   test_double = double
+      #   allow(test_double).to receive(:a).and_return('b')
+      #
+      class OpenStructUse < Base
+        MSG = 'Avoid using `OpenStruct`; use `Struct`, `Hash`, a class or test doubles instead.'
+
+        # @!method uses_open_struct?(node)
+        def_node_matcher :uses_open_struct?, <<-PATTERN
+          (const {nil? (cbase)} :OpenStruct)
+        PATTERN
+
+        def on_const(node)
+          return unless uses_open_struct?(node)
+          return if custom_class_or_module_definition?(node)
+
+          add_offense(node)
+        end
+
+        private
+
+        def custom_class_or_module_definition?(node)
+          parent = node.parent
+
+          (parent.class_type? || parent.module_type?) && node.left_siblings.empty?
+        end
+      end
+    end
+  end
+end

lib/rubocop/formatter/html_formatter.rb

@@ -23,20 +23,23 @@ def fade_out(amount)
         end
       end
 
+      Summary = Struct.new(:offense_count, :inspected_files, :target_files, keyword_init: true)
+      FileOffenses = Struct.new(:path, :offenses, keyword_init: true)
+
       attr_reader :files, :summary
 
       def initialize(output, options = {})
         super
         @files = []
-        @summary = OpenStruct.new(offense_count: 0)
+        @summary = Summary.new(offense_count: 0)
       end
 
       def started(target_files)
         summary.target_files = target_files
       end
 
       def file_finished(file, offenses)
-        files << OpenStruct.new(path: file, offenses: offenses)
+        files << FileOffenses.new(path: file, offenses: offenses)
         summary.offense_count += offenses.count
       end
 

spec/rubocop/config_loader_spec.rb

@@ -1007,9 +1007,9 @@ class Loop < Base
 
       context 'and the gem is globally installed' do
         before do
-          gem_class = Struct.new(:gem_dir)
           %w[gemone gemtwo].each do |gem_name|
-            mock_spec = gem_class.new(File.join(gem_root, gem_name))
+            mock_spec = double
+            allow(mock_spec).to receive(:gem_dir).and_return(File.join(gem_root, gem_name))
             allow(Gem::Specification).to receive(:find_by_name).with(gem_name).and_return(mock_spec)
           end
           allow(Gem).to receive(:path).and_return([gem_root])
@@ -1036,15 +1036,19 @@ class Loop < Base
       end
 
       context 'and the gem is bundled' do
+        let(:gem_one) { double }
+        let(:gem_two) { double }
+
         before do
           require 'bundler'
 
-          specs = {
-            'gemone' => [OpenStruct.new(full_gem_path: File.join(gem_root, 'gemone'))],
-            'gemtwo' => [OpenStruct.new(full_gem_path: File.join(gem_root, 'gemtwo'))]
-          }
+          specs = { 'gemone' => [gem_one], 'gemtwo' => [gem_two] }
 
-          allow(Bundler).to receive(:load).and_return(OpenStruct.new(specs: specs))
+          allow(gem_one).to receive(:full_gem_path).and_return(File.join(gem_root, 'gemone'))
+          allow(gem_two).to receive(:full_gem_path).and_return(File.join(gem_root, 'gemtwo'))
+          result = double
+          allow(result).to receive(:specs).and_return(specs)
+          allow(Bundler).to receive(:load).and_return(result)
         end
 
         it 'returns values from the gem config with local overrides' do
@@ -1082,7 +1086,8 @@ class Loop < Base
 
         create_file("#{gem_root}/#{gem_name}/default.yml", ["Layout/LineLength:\n    Max: 48"])
 
-        mock_spec = OpenStruct.new(gem_dir: File.join(gem_root, gem_name))
+        mock_spec = double
+        allow(mock_spec).to receive(:gem_dir).and_return(File.join(gem_root, gem_name))
         allow(Gem::Specification).to receive(:find_by_name).with(gem_name).and_return(mock_spec)
         allow(Gem).to receive(:path).and_return([gem_root])
       end

spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb

@@ -100,7 +100,7 @@
             let(:offenses) do
               [
                 RuboCop::Cop::Offense.new(:convention,
-                                          OpenStruct.new(line: 7, column: 0),
+                                          FakeLocation.new(line: 7, column: 0),
                                           'Class has too many lines.',
                                           'Metrics/ClassLength')
               ]
@@ -125,7 +125,7 @@
             let(:offenses) do
               [
                 RuboCop::Cop::Offense.new(:convention,
-                                          OpenStruct.new(line: 7, column: 0),
+                                          FakeLocation.new(line: 7, column: 0),
                                           'Method has too many lines.',
                                           'Metrics/MethodLength')
               ]
@@ -148,7 +148,7 @@
               let(:offenses) do
                 [
                   RuboCop::Cop::Offense.new(:convention,
-                                            OpenStruct.new(line: 4, column: 0),
+                                            FakeLocation.new(line: 4, column: 0),
                                             'Method has too many lines.',
                                             'Metrics/MethodLength')
                 ]
@@ -178,7 +178,7 @@
               let(:offenses) do
                 [
                   RuboCop::Cop::Offense.new(:convention,
-                                            OpenStruct.new(line: 4, column: 0),
+                                            FakeLocation.new(line: 4, column: 0),
                                             'Method has too many lines.',
                                             'Metrics/MethodLength')
                 ]
@@ -239,7 +239,7 @@
         let(:offenses) do
           offense_lines.map do |line|
             RuboCop::Cop::Offense.new(:convention,
-                                      OpenStruct.new(line: line, column: 3),
+                                      FakeLocation.new(line: line, column: 3),
                                       message,
                                       cop_name)
           end
@@ -288,7 +288,7 @@ class One
         let(:offenses) do
           [
             RuboCop::Cop::Offense.new(:convention,
-                                      OpenStruct.new(line: 3, column: 0),
+                                      FakeLocation.new(line: 3, column: 0),
                                       'Tab detected.',
                                       'Layout/IndentationStyle')
           ]
@@ -508,7 +508,7 @@ def bar
       let(:offenses) do
         [
           RuboCop::Cop::Offense.new(:convention,
-                                    OpenStruct.new(line: 2, column: 0),
+                                    FakeLocation.new(line: 2, column: 0),
                                     'Class has too many lines.',
                                     'Metrics/ClassLength')
         ]

spec/rubocop/cop/style/open_struct_use_spec.rb

@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Style::OpenStructUse, :config do
+  context 'when using OpenStruct' do
+    ['OpenStruct', '::OpenStruct'].each do |klass|
+      context "for #{klass}" do
+        context 'when used in assignments' do
+          it 'registers an offense' do
+            expect_offense(<<~RUBY, klass: klass)
+              a = %{klass}.new(a: 42)
+                  ^{klass} Avoid using `OpenStruct`;[...]
+            RUBY
+          end
+        end
+
+        context 'when inheriting from it via <' do
+          it 'registers an offense' do
+            expect_offense(<<~RUBY, klass: klass)
+              class SubClass < %{klass}
+                               ^{klass} Avoid using `OpenStruct`;[...]
+              end
+            RUBY
+          end
+        end
+
+        context 'when inheriting from it via Class.new' do
+          it 'registers an offense' do
+            expect_offense(<<~RUBY, klass: klass)
+              SubClass = Class.new(%{klass})
+                                   ^{klass} Avoid using `OpenStruct`;[...]
+            RUBY
+          end
+        end
+      end
+    end
+  end
+
+  context 'when using custom namespaced OpenStruct' do
+    context 'when inheriting from it' do
+      specify { expect_no_offenses('class A < SomeNamespace::OpenStruct; end') }
+    end
+
+    context 'when defined in custom namespace' do
+      context 'when class' do
+        specify do
+          expect_no_offenses(<<~RUBY)
+            module SomeNamespace
+              class OpenStruct
+              end
+            end
+          RUBY
+        end
+      end
+
+      context 'when module' do
+        specify do
+          expect_no_offenses(<<~RUBY)
+            module SomeNamespace
+              module OpenStruct
+              end
+            end
+          RUBY
+        end
+      end
+    end
+
+    context 'when used in assignments' do
+      it 'registers no offense' do
+        expect_no_offenses(<<~RUBY)
+          a = SomeNamespace::OpenStruct.new
+        RUBY
+      end
+    end
+  end
+
+  context 'when not using OpenStruct' do
+    it 'registers no offense', :aggregate_failures do
+      expect_no_offenses('class A < B; end')
+      expect_no_offenses('a = 42')
+    end
+  end
+end

spec/rubocop/formatter/disabled_config_formatter_spec.rb

@@ -21,7 +21,7 @@ def io.path
      RuboCop::Cop::Offense.new(:convention, location, 'message', 'Test/Cop2')]
   end
 
-  let(:location) { OpenStruct.new(line: 1, column: 5) }
+  let(:location) { FakeLocation.new(line: 1, column: 5) }
 
   let(:heading) do
     format(

spec/rubocop/formatter/pacman_formatter_spec.rb

@@ -18,7 +18,7 @@
     end
 
     context 'when a offense is detected in a file' do
-      let(:location) { OpenStruct.new(line: 1, column: 5) }
+      let(:location) { FakeLocation.new(line: 1, column: 5) }
       let(:expected_character) { Rainbow(described_class::GHOST).red }
       let(:offenses) { [RuboCop::Cop::Offense.new(:error, location, 'message', 'CopA')] }
 

spec/support/fake_location.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+# Tests may use this to fake out a location structure in an Offense.
+FakeLocation = Struct.new(:line, :column, keyword_init: true)