New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix a false positive for Lint/InheritException
#10406
Fix a false positive for Lint/InheritException
#10406
Conversation
This PR marks `Lint/InheritException` as unsafe auto-correction. This cop's autocorrection is unsafe because `rescue` that omit exception class handle `StandardError` and its subclasses, but not `Exception` and its subclasses. ```ruby begin rescue # Handles only `StandardError` and its subclasses because exception class is omitted. end ``` It's still unsafe after rubocop#10406 is resolved.
8c5f9b1
to
0834571
Compare
This PR marks `Lint/InheritException` as unsafe auto-correction. This cop's autocorrection is unsafe because `rescue` that omit exception class handle `StandardError` and its subclasses, but not `Exception` and its subclasses. ```ruby begin rescue # Handles only `StandardError` and its subclasses because exception class is omitted. end ``` It's still unsafe after #10406 is resolved.
This PR change `EnforcedStyle` from `runtime_error` to `standard_error` for `Lint/InheritException`. I noticed this while investigating rubocop#10406. Below is the inheritance tree for the exception classes. ``` --------------- | Exception | --------------- ^ | --------------- |StandardError| (default for `rescue`) --------------- ^ | --------------- |RuntimeError | (default for `raise`) --------------- ``` AFAIK, it seems that custom exceptions generally inherit from `StandardError`. Subclasses of `StandardError` are handled with `rescue` by default. `raise` creates a `RuntimeError` when the exception class is omitted. If custom exception inherits from `RuntimeError`, the custom exception and `RuntimeError` will be is-a. If custom exception inherits from `StandardError`, the custom exception and `RuntimeError` will not be is-a. In other words, inheriting `StandardError` will not be mistakenly handled as `raise` which omits the exception class. So, inheriting `StandardError` rather than `RuntimeError` does not include unnecessary inheritance for `rescue` handling.
This PR change `EnforcedStyle` from `runtime_error` to `standard_error` for `Lint/InheritException`. I noticed this while investigating #10406. Below is the inheritance tree for the exception classes. ``` --------------- | Exception | --------------- ^ | --------------- |StandardError| (default for `rescue`) --------------- ^ | --------------- |RuntimeError | (default for `raise`) --------------- ``` AFAIK, it seems that custom exceptions generally inherit from `StandardError`. Subclasses of `StandardError` are handled with `rescue` by default. `raise` creates a `RuntimeError` when the exception class is omitted. If custom exception inherits from `RuntimeError`, the custom exception and `RuntimeError` will be is-a. If custom exception inherits from `StandardError`, the custom exception and `RuntimeError` will not be is-a. In other words, inheriting `StandardError` will not be mistakenly handled as `raise` which omits the exception class. So, inheriting `StandardError` rather than `RuntimeError` does not include unnecessary inheritance for `rescue` handling.
I was the impression that those classes should not be extended by application code, as they are internal to Ruby, plus usually they can't be handled in a meaningful way. I think this was also the main point of the special check for them - if you end up using those most likely you're doing something wrong. |
I think this On the other hand, I agree that it is rare for these exception classes to be extended at user layer, but these exception classes (e.g. |
Fixes standardrb/standard#388 and reverts rubocop#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.
0834571
to
f2277fc
Compare
Fair enough, let's simplify this. |
Fixes standardrb/standard#388 and reverts #3427.
This PR prevents the following false positive and auto-correction.
Below is an inheritance tree for the exception classes.
As the inheritance tree shows,
Interrupt
is notis_a?
in eitherStandardError
orRuntimeError
.This goes against The Liskov Substitution Principle.
Interup
should not be replaced withStandardError
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
, andSystemExit
classes.This PR allows for exception handling according to Object-oriented principles and Ruby exception handling mechanism.
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.