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.

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

Lint/MissingSuper false positive #8506

Closed
jcoyne opened this issue Aug 10, 2020 · 14 comments
Closed

Lint/MissingSuper false positive #8506

jcoyne opened this issue Aug 10, 2020 · 14 comments

Comments

@jcoyne
Copy link

jcoyne commented Aug 10, 2020

There are cases where I'm explicitly avoiding calling super in an initializer, however this cop complains on all of them. This doesn't seem very useful.

@marcandre
Copy link
Contributor

There's also a request to have a list of accepted superclasse exceptions, would that resolve the issue for you?

I'm curious of examples where there are good reasons to explicitly not call super.

@fatkodima
Copy link
Contributor

I'm curious of examples where there are good reasons to explicitly not call super.

👍

@jcoyne
Copy link
Author

jcoyne commented Aug 10, 2020

There's also a request to have a list of accepted superclasse exceptions, would that resolve the issue for you?

Not really. I just don't think this should be a rubocop.

I'm curious of examples where there are good reasons to explicitly not call super.

One concern would be for performance (though this is not a primary concern). Another is that this makes the code harder to read as I might have to navigate to another file to see what logic is happening there (and this could be hard to find if it's in a gem). Mostly I'm concerned that this is not idiomatic for libraries like view_component (https://github.com/github/view_component#implementation) If a new coder sees super calls they may wonder why the last developer didn't use the patterns promoted by the library authors.

@marcandre
Copy link
Contributor

I might have to navigate to another file to see what logic is happening there

The idea is that if there was no initialize method, say, would you actually go to another file to see what the inherited initialize does?

That said, if others feel this Cop should be disabled by default, I won't stand in the way.

@searls
Copy link

searls commented Aug 26, 2020

Sorbet is one counter-example where you don't want to call super necessarily, born out of @will's post on standard standardrb/standard#195

It'd be really awesome if this was configurable for which callbacks, because I think there are fewer cases where you'd intentionally skip calling super on method_missing than on initialize. Any chance of that?

@pirj
Copy link
Member

pirj commented Sep 16, 2020

Speaking of those two cases (notice no parent class):

        class Foo
          def method_missing(*args)
            super
        class Foo
          def self.inherited(base)
            super

Is super really necessary? Why?
@fatkodima can you please provide an example of what will break for a missing super with no parent class?

PS Also here

module M
  def self.included(base)
    super

@fatkodima
Copy link
Contributor

Yes, without parent classes those are not really strictly necessary.
For the example with M module, that was fixed on master, if I remember correctly.

@marcandre
Copy link
Contributor

Not necessary, but can't hurt.
I also don't think we are dealing with:

        class Foo
          extend ModDependingOnInheritedCallback
          def self.inherited(base)
            super

or

# base.rb
      class Foo < Bar
      # ...

# extension.rb
       class Foo
         def self.inherited

@amatsuda
Copy link
Contributor

Here's an example of method_missing not calling super but perfectly working. https://github.com/faker-ruby/faker/blob/9ea5a3d/lib/helpers/unique_generator.rb#L17-L27

Falling back to raise instead of super seems like a possible scenario for a method_missing (but I'm not sure if we should allow this here in this cop).

kevindew added a commit to alphagov/content-publisher that referenced this issue Apr 15, 2021
This creates a new mixin, Callable, which can be used to provide a class
with the boilerplate so that it can have a public class method of
`.call` which calls `#initialize` with the arguments and then `#call`.

This mixin replaces the inheritance approach used in services with an
ApplicationService class. These classes now all utilise this mixin
instead.

The reason for doing this was prompted by a linting violation with one
of rubocop's newer rules: `Lint/MissingSuper`, which is an offence when
a class inherits but does not call `super` in it's initialize method.
This violation seemed to only affect some of GOV.UK's newer code, such as
this service pattern, which led to some debate [1] about what the most
appropriate means to share this behaviour is, whether our
inheritance approach was actually idiomatic and how using super in every
`initialize` would be unnecessary boilerplate.

The Rubocop thread [2] regarding this rule raised an interesting point
"I'm curious of examples where there are good reasons to explicitly not
call super.", which led to me considering why we didn't think `super`
would be appropriate in these classes. My conclusion to this was that we
weren't actually intending to create a subtype with inheritance, we
actually just wanted standardisation with the interface, and that the
inherited classes have very little behavioural consistency, breaking
Liskov substitution principle [3]. When looking at the Ruby standard
library a module seems the most appropriate way to implement it,
considering similarities with modules such as Singleton and Enumerable.

Taking inspiration from the Ruby
standard library there is an in

[1]: alphagov/rubocop-govuk#125
[2]: rubocop/rubocop#8506
[3]: https://thoughtbot.com/blog/back-to-basics-solid#liskov-substitution-principle
kevindew added a commit to alphagov/content-publisher that referenced this issue Apr 26, 2021
This creates a new mixin, Callable, which can be used to provide a class
with the boilerplate so that it can have a public class method of
`.call` which calls `#initialize` with the arguments and then `#call`.

This mixin replaces the inheritance approach used in services with an
ApplicationService class. These classes now all utilise this mixin
instead.

The change was prompted by a linting violation with one of rubocop's newer
rules: `Lint/MissingSuper`, which is an offence when a class inherits but
does not call `super` in it's initialize method. This violation seemed
to only affect some of GOV.UK's newer code, such as this service
pattern, which led to some debate [1] about what the most
appropriate means to share this behaviour is, whether our
inheritance approach was actually idiomatic and how using super in every
`initialize` would be unnecessary boilerplate.

The Rubocop thread [2] regarding this rule raised an interesting point
"I'm curious of examples where there are good reasons to explicitly not
call super.", which led to me considering why we didn't think `super`
would be appropriate in these classes. My conclusion to this was that we
weren't actually intending to create a subtype with inheritance, we
actually just wanted standardisation with the interface, and that the
inherited classes have very little behavioural consistency, breaking
Liskov substitution principle [3]. When looking at the Ruby standard
library a module seems the most appropriate way to implement it,
considering similarities with Singleton and Enumerable.

[1]: alphagov/rubocop-govuk#125
[2]: rubocop/rubocop#8506
[3]: https://thoughtbot.com/blog/back-to-basics-solid#liskov-substitution-principle
kevindew added a commit to alphagov/email-alert-api that referenced this issue May 5, 2021
This creates a new mixin, Callable, which can be used to provide a class
with the boilerplate so that it can have a public class method of
`.call` which calls `#initialize` with the arguments and then `#call`.

The creation of this class is a port of the same approach applied in
Content Publisher [1] (and most of this commit message is a port too in
case you're feeling a touch of déjà vu).

This mixin replaces the inheritance approach used in builders,
presenters and services.

The change was prompted by a linting violation with one of rubocop's newer
rules: `Lint/MissingSuper`, which is an offence when a class inherits but
does not call `super` in it's initialize method. This violation seemed
to only affect some of GOV.UK's newer code, such as this service
pattern, which led to some debate [2] about what the most
appropriate means to share this behaviour is, whether our
inheritance approach was actually idiomatic and how using super in every
`initialize` would be unnecessary boilerplate.

The Rubocop thread [3] regarding this rule raised an interesting point
"I'm curious of examples where there are good reasons to explicitly not
call super.", which led to me considering why we didn't think `super`
would be appropriate in these classes. My conclusion to this was that we
weren't actually intending to create a subtype with inheritance, we
actually just wanted standardisation with the interface, and that the
inherited classes have very little behavioural consistency, breaking
Liskov substitution principle [4]. When looking at the Ruby standard
library a module seems the most appropriate way to implement it,
considering similarities with Singleton and Enumerable.

[1]: alphagov/content-publisher@5933f87
[2]: alphagov/rubocop-govuk#125
[3]: rubocop/rubocop#8506
[4]: https://thoughtbot.com/blog/back-to-basics-solid#liskov-substitution-principle
kevindew added a commit to alphagov/email-alert-api that referenced this issue May 7, 2021
This creates a new mixin, Callable, which can be used to provide a class
with the boilerplate so that it can have a public class method of
`.call` which calls `#initialize` with the arguments and then `#call`.

The creation of this class is a port of the same approach applied in
Content Publisher [1] (and most of this commit message is a port too in
case you're feeling a touch of déjà vu).

This mixin replaces the inheritance approach used in builders,
presenters and services.

The change was prompted by a linting violation with one of rubocop's newer
rules: `Lint/MissingSuper`, which is an offence when a class inherits but
does not call `super` in it's initialize method. This violation seemed
to only affect some of GOV.UK's newer code, such as this service
pattern, which led to some debate [2] about what the most
appropriate means to share this behaviour is, whether our
inheritance approach was actually idiomatic and how using super in every
`initialize` would be unnecessary boilerplate.

The Rubocop thread [3] regarding this rule raised an interesting point
"I'm curious of examples where there are good reasons to explicitly not
call super.", which led to me considering why we didn't think `super`
would be appropriate in these classes. My conclusion to this was that we
weren't actually intending to create a subtype with inheritance, we
actually just wanted standardisation with the interface, and that the
inherited classes have very little behavioural consistency, breaking
Liskov substitution principle [4]. When looking at the Ruby standard
library a module seems the most appropriate way to implement it,
considering similarities with Singleton and Enumerable.

[1]: alphagov/content-publisher@5933f87
[2]: alphagov/rubocop-govuk#125
[3]: rubocop/rubocop#8506
[4]: https://thoughtbot.com/blog/back-to-basics-solid#liskov-substitution-principle
kevindew added a commit to alphagov/email-alert-api that referenced this issue May 17, 2021
This creates a new mixin, Callable, which can be used to provide a class
with the boilerplate so that it can have a public class method of
`.call` which calls `#initialize` with the arguments and then `#call`.

The creation of this class is a port of the same approach applied in
Content Publisher [1] (and most of this commit message is a port too in
case you're feeling a touch of déjà vu).

This mixin replaces the inheritance approach used in builders,
presenters and services.

The change was prompted by a linting violation with one of rubocop's newer
rules: `Lint/MissingSuper`, which is an offence when a class inherits but
does not call `super` in it's initialize method. This violation seemed
to only affect some of GOV.UK's newer code, such as this service
pattern, which led to some debate [2] about what the most
appropriate means to share this behaviour is, whether our
inheritance approach was actually idiomatic and how using super in every
`initialize` would be unnecessary boilerplate.

The Rubocop thread [3] regarding this rule raised an interesting point
"I'm curious of examples where there are good reasons to explicitly not
call super.", which led to me considering why we didn't think `super`
would be appropriate in these classes. My conclusion to this was that we
weren't actually intending to create a subtype with inheritance, we
actually just wanted standardisation with the interface, and that the
inherited classes have very little behavioural consistency, breaking
Liskov substitution principle [4]. When looking at the Ruby standard
library a module seems the most appropriate way to implement it,
considering similarities with Singleton and Enumerable.

[1]: alphagov/content-publisher@5933f87
[2]: alphagov/rubocop-govuk#125
[3]: rubocop/rubocop#8506
[4]: https://thoughtbot.com/blog/back-to-basics-solid#liskov-substitution-principle
@NathanZook
Copy link

I just bumped into this with some Exception classes. To me, the exception to this rule is that the ancestors of a class do not initialize ivars. Of course, a static tool is going to be hard pressed to keep up with every possible case. However, it seems to me that the vast bulk of these exceptions can be caught without too much trouble.

  1. During scanning, an ancestor map is created. (This cannot be absolutely perfect, but will work generally.)
  2. A list of classes and modules from ruby and the ruby core can be maintained which do not initialize ivars.
  3. Any class with a high-confidence ancestor map whose ancestors either lack an initialize instance method, or which are on "the list" are good.

Because it's REALLY annoying to write:

class MyError < RuntimeError
  # rubocop:disable Lint/MissingSuper
  def initialize(var)
    @ivar = var
  end
  # rubocop:enable Lint/MissingSuper

@apainintheneck
Copy link

I've just run into the same issue when creating a dsl and using self.inherited() only once in the base class. It would be nice if that edge case was considered by this cop. Though at the end of the day it's not too hard to just write a one line exception to the rule.

def self.inherited(subclass) # rubocop:disable Lint/MissingSuper

@philliplongman
Copy link

@marcandre, how come nothing happened with this?

There's also #8376 (comment) to have a list of accepted superclasse exceptions, would that resolve the issue for you?

This would definitely solve my problem, while allowing me to keep the cop everywhere else.

@iMacTia
Copy link
Contributor

iMacTia commented Jun 8, 2023

I bumped into this same issue today, my use-case is also related to Sorbet and it's ability to define abstract classes.
In my case it doesn't make sense to call super in concrete implementations of the abstract class, making this cop raising false positives.

So I've taken the freedom to open a PR to make StatelessClasses configurable 😄
With this setting, I would be able to configure the cop to ignore subclasses of abstract classes 👍

@koic koic closed this as completed in 09d0ff5 Jun 21, 2023
koic added a commit that referenced this issue Jun 21, 2023
…rable-stateless-classes

[Fix #8506] Add AllowedParentClasses config to Lint/MissingSuper.
@will
Copy link

will commented Jun 21, 2023

Thanks @koic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests