diff --git a/changelog/new_add_openstruct_use_cop.md b/changelog/new_add_openstruct_use_cop.md new file mode 100644 index 00000000000..b5a2fbac9e2 --- /dev/null +++ b/changelog/new_add_openstruct_use_cop.md @@ -0,0 +1 @@ +* [#10217](https://github.com/rubocop/rubocop/pull/10217): Add new `Style/OpenStructUse` cop. ([@mttkay][]) diff --git a/config/default.yml b/config/default.yml index 572710c6aea..ec3f5c15ef1 100644 --- a/config/default.yml +++ b/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: '<>' + Style/OptionHash: Description: "Don't use option hashes when you can use keyword arguments." Enabled: false diff --git a/lib/rubocop.rb b/lib/rubocop.rb index c6930cfc346..dcd734cd258 100644 --- a/lib/rubocop.rb +++ b/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' diff --git a/lib/rubocop/cop/style/open_struct_use.rb b/lib/rubocop/cop/style/open_struct_use.rb new file mode 100644 index 00000000000..e9f350e2cd8 --- /dev/null +++ b/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 diff --git a/lib/rubocop/formatter/html_formatter.rb b/lib/rubocop/formatter/html_formatter.rb index fe3b70ec6fc..c013106ee01 100644 --- a/lib/rubocop/formatter/html_formatter.rb +++ b/lib/rubocop/formatter/html_formatter.rb @@ -23,12 +23,15 @@ 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) @@ -36,7 +39,7 @@ def started(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 diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb index 6d3979dda5c..8eb77cc8ea4 100644 --- a/spec/rubocop/config_loader_spec.rb +++ b/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 diff --git a/spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb b/spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb index 0dcf135f64a..3fe5b7f4277 100644 --- a/spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb +++ b/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') ] diff --git a/spec/rubocop/cop/style/open_struct_use_spec.rb b/spec/rubocop/cop/style/open_struct_use_spec.rb new file mode 100644 index 00000000000..b0a65edf8f7 --- /dev/null +++ b/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 diff --git a/spec/rubocop/formatter/disabled_config_formatter_spec.rb b/spec/rubocop/formatter/disabled_config_formatter_spec.rb index d454196ebd4..8bf87a046d6 100644 --- a/spec/rubocop/formatter/disabled_config_formatter_spec.rb +++ b/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( diff --git a/spec/rubocop/formatter/pacman_formatter_spec.rb b/spec/rubocop/formatter/pacman_formatter_spec.rb index cd9e50a60f1..8042ddc0db7 100644 --- a/spec/rubocop/formatter/pacman_formatter_spec.rb +++ b/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')] } diff --git a/spec/support/fake_location.rb b/spec/support/fake_location.rb new file mode 100644 index 00000000000..6e753bba204 --- /dev/null +++ b/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)