From 02fba0d19939a94c3c0751a9ee8ddfb0f81118ba Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 9 Feb 2022 22:57:19 +0900 Subject: [PATCH] Fix a false positive for `Lint/InheritException` Fixes https://github.com/testdouble/standard/issues/388 and reverts https://github.com/rubocop/rubocop/pull/3427. This PR prevents the following false positive and auto-correction. ```diff class TestClass - FatalError = Class.new(Interrupt) + FatalError = Class.new(RuntimeError) end ``` Below is an inheritance tree for the exception classes. ``` --------------- | BasicObject | --------------- ^ | --------------- | Kernel | --------------- ^ | --------------- | Object | --------------- ^ | --------------- | Exception | --------------- ^ | |------------------| --------------- ----------------- |StandardError| |SignalException| --------------- ----------------- ^ ^ | | --------------- ----------------- |RuntimeError | | Interrupt | --------------- ----------------- ``` As the inheritance tree shows, `Interrupt` is not `is_a?` in either `StandardError` or `RuntimeError`. ```ruby RuntimeError.new.is_a?(StandardError) #=> true Interrupt.new.is_a?(StandardError) #=> false Interrupt.new.is_a?(RuntimeError) #=> false ``` This goes against The Liskov Substitution Principle. `Interup` should not be replaced with `StandardError` or its subclasses. Also, according to The Open/Closed Principle, `Interrup` class should be open to inheritance. The same is true for `SystemStackError`, `NoMemoryError`, `SecurityError`, `NotImplementedError`, `LoadError`, `SyntaxError`, `ScriptError`, `SignalException`, and `SystemExit` classes. This PR allows for exception handling according to Object-oriented principles and Ruby exception handling mechanism. --- ...lse_positive_for_lint_inherit_exception.md | 1 + lib/rubocop/cop/lint/inherit_exception.rb | 30 +++++-------------- .../cop/lint/inherit_exception_spec.rb | 16 ++++++++++ 3 files changed, 25 insertions(+), 22 deletions(-) create mode 100644 changelog/fix_a_false_positive_for_lint_inherit_exception.md diff --git a/changelog/fix_a_false_positive_for_lint_inherit_exception.md b/changelog/fix_a_false_positive_for_lint_inherit_exception.md new file mode 100644 index 00000000000..a39689dbe16 --- /dev/null +++ b/changelog/fix_a_false_positive_for_lint_inherit_exception.md @@ -0,0 +1 @@ +* [#10406](https://github.com/rubocop/rubocop/pull/10406): Fix a false positive for `Lint/InheritException` when inheriting a standard lib exception class that is not a subclass of `StandardError`. ([@koic][]) diff --git a/lib/rubocop/cop/lint/inherit_exception.rb b/lib/rubocop/cop/lint/inherit_exception.rb index 53b406c1c0a..2c7b32486bb 100644 --- a/lib/rubocop/cop/lint/inherit_exception.rb +++ b/lib/rubocop/cop/lint/inherit_exception.rb @@ -3,10 +3,9 @@ module RuboCop module Cop module Lint - # This cop looks for error classes inheriting from `Exception` - # and its standard library subclasses, excluding subclasses of - # `StandardError`. It is configurable to suggest using either - # `StandardError` (default) or `RuntimeError` instead. + # This cop looks for error classes inheriting from `Exception`. + # It is configurable to suggest using either `StandardError` (default) or + # `RuntimeError` instead. # # @safety # This cop's autocorrection is unsafe because `rescue` that omit @@ -42,24 +41,11 @@ class InheritException < Base include ConfigurableEnforcedStyle extend AutoCorrector - MSG = 'Inherit from `%s` instead of `%s`.' + MSG = 'Inherit from `%s` instead of `Exception`.' PREFERRED_BASE_CLASS = { runtime_error: 'RuntimeError', standard_error: 'StandardError' }.freeze - ILLEGAL_CLASSES = %w[ - Exception - SystemStackError - NoMemoryError - SecurityError - NotImplementedError - LoadError - SyntaxError - ScriptError - Interrupt - SignalException - SystemExit - ].freeze RESTRICT_ON_SEND = %i[new].freeze @@ -71,7 +57,7 @@ class InheritException < Base PATTERN def on_class(node) - return unless node.parent_class && illegal_class_name?(node.parent_class) + return unless node.parent_class && exception_class?(node.parent_class) message = message(node.parent_class) @@ -82,7 +68,7 @@ def on_class(node) def on_send(node) constant = class_new_call?(node) - return unless constant && illegal_class_name?(constant) + return unless constant && exception_class?(constant) message = message(constant) @@ -97,8 +83,8 @@ def message(node) format(MSG, prefer: preferred_base_class, current: node.const_name) end - def illegal_class_name?(class_node) - ILLEGAL_CLASSES.include?(class_node.const_name) + def exception_class?(class_node) + class_node.const_name == 'Exception' end def preferred_base_class diff --git a/spec/rubocop/cop/lint/inherit_exception_spec.rb b/spec/rubocop/cop/lint/inherit_exception_spec.rb index 2133652e444..1d6c36e6509 100644 --- a/spec/rubocop/cop/lint/inherit_exception_spec.rb +++ b/spec/rubocop/cop/lint/inherit_exception_spec.rb @@ -28,6 +28,14 @@ class C < RuntimeError; end RUBY end end + + context 'when inheriting a standard lib exception class that is not a subclass of `StandardError`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class C < Interrupt; end + RUBY + end + end end context 'with enforced style set to `standard_error`' do @@ -56,6 +64,14 @@ class C < StandardError; end RUBY end end + + context 'when inheriting a standard lib exception class that is not a subclass of `StandardError`' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + class C < Interrupt; end + RUBY + end + end end end end