Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Running the "standardrb --fix" command changes "Class.new(Interrupt)" to "Class.new(RuntimeError)" (v1.1.5) #388

Closed
laser opened this issue Feb 8, 2022 · 6 comments · Fixed by rubocop/rubocop#10406
Closed

Running the "standardrb --fix" command changes "Class.new(Interrupt)" to "Class.new(RuntimeError)" (v1.1.5) #388

laser opened this issue Feb 8, 2022 · 6 comments · Fixed by rubocop/rubocop#10406

Comments

@laser
Copy link

laser commented Feb 8, 2022 •
edited

First of all - thanks for the great work. We use your product all the time and we love it :)

Steps to Reproduce

  • Ensure that standardrb gem version 1.1.5 is active
  • Ensure that standardrb is on your PATH

When the following file is formatted with the standardrb --fix command:

class TestClass
  FatalError = Class.new(Interrupt)
end

Interrupt is changed to RuntimeError

Why Is This a Bug?

Standard is a Ruby style guide, linter, and formatter. The behavior outlined in this issue demonstrates that the formatter has changed code semantics and not style.
@searls
Copy link
Contributor

searls commented Feb 8, 2022

This definitely seems weird. @koic does this look like an unhandled edge case in the associated cop?

@koic
Copy link
Contributor

koic commented Feb 8, 2022

As far as I investigated, this behavior was introduced by rubocop/rubocop#3427. But the behavior seems weird to me too. I will more investigate it.

@koic koic mentioned this issue Feb 9, 2022
Merged
8 tasks
@koic
Copy link
Contributor

koic commented Feb 9, 2022

I've opened rubocop/rubocop#10406 to resolve this issue.

@bbatsov
Copy link

bbatsov commented Feb 13, 2022

I've added there a couple of comments myself. Seems to me that the gist of it as:

  • the auto-correction here was inherently unsafe
  • we probably need a different message when someone is trying to extend error classes that are not meant to be extended (e.g. LoadError) (or perhaps the list of those classes should be made configurable)

koic added a commit to koic/rubocop that referenced this issue Feb 13, 2022
@koic
Fix a false positive for Lint/InheritException
f2277fc 
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.
bbatsov pushed a commit to rubocop/rubocop that referenced this issue Feb 13, 2022
@koic @bbatsov
Fix a false positive for Lint/InheritException
02fba0d 
Fixes standardrb/standard#388 and
reverts #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.
@camilopayan
Copy link
Contributor

The fix is in the latest version of standard now too 😄

@laser
Copy link
Author

laser commented Mar 17, 2022

Thanks, folks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix a false positive for Lint/InheritException koic/rubocop
5 participants
@koic @searls @bbatsov @camilopayan @laser