Re-enable Lint/MissingSuper

[will]

Hi, I just updated and noticed this linter was turned on in 0.5.0.

It has a ton of problems for me since I use abstract! from sorbet. It generates an initialize method in the abstract parent class to raise an exception if anyone accidentally tries to instantiate it. All of the child classes have their own initialize methods and are correctly not calling super, but the linter thinks they should be calling it.

I'm going to disable it for myself in the yaml file, but wanted to open the issue here in case others have the same problem.

[searls]

Thanks for calling this out! Enough people use sorbet that I think this alone is worth rolling back. Even as I added it I could imagine a couple corner cases where not calling super was the right thing to do

[searls]

Landed in 0.5.2

[will]

Thanks!

Also, rubocop may have disabled Style/MethodMissingSuper when they put in Lint/MissingSuper https://github.com/rubocop-hq/rubocop/pull/8376

I do remember it yelling at me about the method missing one, but I cant remember if it was useful or not.

[searls]

IIRC the MissingSuper is a recent replacement for the other one

[will]

That's my understanding, but if the new MissingSuper is disabled here, you might want to turn back on MethodMissingSuper manually?

[searls]

My understanding is MethodMissingSuper was removed in favor of MissingSuper and there is unfortunately no apparent way to configure it to only cover method missing 😦

https://docs.rubocop.org/rubocop/0.89/cops_lint.html

[marcandre]

It has a ton of problems for me since I use abstract! from sorbet. It generates an initialize method in the abstract parent class to raise an exception if anyone accidentally tries to instantiate it.

Interesting. ~~I couldn't read this in the reference given, do you know of another source?~~

Unless I'm missing something, it seems completely mistaken from sorbet. They should instead override MyAbstractClass.new instead of MyAbstractClass#initialize. An abstract class could have very valid reason to do initializations, and a concrete class shouldn't have to create an empty initialize if it doesn't require initialization parameters.

Edit: I opened https://github.com/sorbet/sorbet/issues/3388

[searls]

Will reopen and see if this is a sorbet-specific thing and they're open to changing it. If so and nobody else raises issues I'll turn it back on

[ttilberg]

@searls For what it's worth, I ran into this and was surprised outside of Sorbet a few months back: https://www.reddit.com/r/ruby/comments/hw8t6a/how_to_please_standardrbrubocop/. It also references another user in Rubocops tracker with a similar issue: https://github.com/rubocop-hq/rubocop/issues/3760#issuecomment-353103957.

Do you feel that super should get called in instances like this? (I don't feel like that's the case)

[marcandre]

@ttilberg I agree, the rule doesn't quit work for method_missing and for repond_to_missing?; it sometimes makes sense to unconditionally return from those without calling super. I'll fix that later today 👍

[searls]

@ttilberg if you look at @marcandre's PR, the feedback received was to document why this hook should be an exception. Since you've written about this before and care a lot about it, could you provide a documentation update for inclusion in that PR so that it can be merged upstream?

[marcandre]

No worries, I'm on it :-)

On Fri, Nov 20, 2020, 08:31 Justin Searls [email protected] wrote:

@ttilberg https://github.com/ttilberg if you look at @marcandre https://github.com/marcandre's PR https://github.com/rubocop-hq/rubocop/pull/9072, the feedback received was to document why this hook should be an exception. Since you've written about this before and care a lot about it, could you provide a documentation update for inclusion in that PR so that it can be merged upstream?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/testdouble/standard/issues/195#issuecomment-731171585, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIH2XV7UKR2TDB2JYO7LTSQZVTRANCNFSM4QLOMJEA .

[jmkoni]

@will We are looking at turning this on because it's good for the average Rubyist. Has Sorbet changed? Or would this still be an issue?

[will]

@jmkoni do you know of a way that I can test turning it back on in a single project, to see what happens? Something in that yaml file maybe?

[jmkoni]

@will yeah! You should be able to do something like this, but instead enable Lint/MissingSuper. I'm guessing you already have a yaml file that looks very similar. https://github.com/testdouble/standard/issues/158#issuecomment-640118656

[will]

with a .standard.yml file of

parallel: true
ignore:
  - 'sorbet/**/*'
  - '**/*':
    - Lint/ConstantDefinitionInBlock # sorbet enums

Lint/MissingSuper:
  Enabled: true

standardrb 0.11.0 doesn't report any problems, however I would have expected these two classes

# typed: strict
# frozen_string_literal: true

class Factory
  extend T::Sig
  extend T::Helpers
  abstract!

  # sorbet workaround, ref: https://github.com/sorbet/sorbet/issues/2374
  alias_method :initialize_abstract, :initialize

  sig { params(params: T::Hash[Symbol, T.untyped]).void }
  def initialize(params = {})
    initialize_abstract
  end
end

class ClusterFactory < Factory
  extend T::Sig

  sig { params(params: T::Hash[Symbol, T.untyped]).void }
  def initialize(params = {})
    # no super
  end
end

to have generated an error with this on? Unless that alias trick is also solving that problem, but it seems like it wouldn't.

[will]

Oh I see it was supposed to be in .rubocop.yml not standard, and I was supposed to run rubocop directly and not standard.

This still breaks for me in that case above. Also in another area I'm on purpose not calling super in subclasses, but this is not related to anything sorbet.

Either way I'm okay with whatever you all decide as long as I can still opt out of this check with an ignore, which I think would work like I'm doing for Lint/ConstantDefinitionInBlock

[searls]

TIL (well, re-learned) that we allowed rule-specific ignores in Standard. That being the case I think we should turn this back on and hope Sorbet users who need this will find this issue.

[marcandre]

hope Sorbet users who need this will find this issue

and upvote / comment on the issue I opened on Sorbet...