Add .descendants and .subclasses advice

[alexanderadam]

A wise man, let's call him Ben, once said:

There are two reasons why this is an unreliable solution:

• it doesn't know about things that have yet to be autoloaded

• it's non-deterministic with regards to Garbage Collection of classes. If you use Class.descendants in Test, where there is a pattern to dynamically define classes, GC is unpredictable for when those classes are cleaned up and removed by the GC.

And I totally trust him on that because he's making a… GoodJob 🥁

[pirj]

Let me play the devil's advocate here. If this method is so unreliable, why not propose to remove it from Rails altogether?
Or ?

Related discussion rspec/rspec-mocks#1568

Why would descendants be GC'd ? Can this happen in runtime? Or just in tests/specs?

Class has subclasses, descendants is a Railsism, right?

[alexanderadam]

Let me play the devil's advocate here. If this method is so unreliable, why not propose to remove it from Rails altogether? Or ?

Well, we have a lot of methods and classes that we shouldn't use in most cases of "regular" application development but then there's a rare occasion where there's no other choice.

I'd also never suggest anyone iterating over ObjectSpace yet I recently was in Rails project where someone just did that.
Thus I'd say that your question is a very good reason for a Cop, as we should not always remove functionality but make developers aware that they might come with some risks (i.e. having significant impacts on performance or memory).

If somebody is fully aware what they are doing and they still want to do it, they can still disable the warning.
For everybody else the warning should come up as early as possible in the development stage, to make sure that this won't become an important part of what an application will rely on.

Related discussion rspec/rspec-mocks#1568

Indeed! Initially I had the Cop suggesting .subclasses until I realised that this isn't a good idea either. 😉
Now that you brought that up, I am thinking that I should probably extend it to discourage the usage of .subclasses as well, no (that's also what Ben implied)? 🤔

For the detail questions, Ben is surely the one who can answer the details better than I can and I saw that you already mentioned him in the other issue.

[Earlopain]

There are many things one should probably not use, and that includes ObjectSpace. And yet I would not want to enforce (either through a cop or guide) that one should not use it. It is better served for documentation to point out the pitfalls (Rubys subclass does this already, Rails descendants does not). ObjectSpace also has a rather large disclaimer about it as a whole.

There are absolutely valid usecases for these (if not, they would probably not exist). You just have to be aware of why using them might not be such a good idea.

BTW, your added example will always work as expected, there are no autoloads/GC present.

[alexanderadam]

There are many things one should probably not use, and that includes ObjectSpace. And yet I would not want to enforce (either through a cop or guide) that one should not use it.

A guide does not enforce anything anyway, no? 🤔
And Cops are just giving advice and can be easily disabled inline or in the the config.

But I agree that Rails's documentation should be improved as well in this regards.

There are absolutely valid usecases for these (if not, they would probably not exist). You just have to be aware of why using them might not be such a good idea.

That's what Rubocop's cops are meant to, no? To raise awareness?

BTW, your added example will always work as expected, there are no autoloads/GC present.

I agree. That's a very valid point.
Tbh, I don't have any idea how make it 'clearer' though. 🤔
Do you have a proposal?

[Earlopain]

A guide does not enforce anything anyway, no?

I guess it's not directly related to this PR, yeah.

And Cops are just giving advice and can be easily disabled inline or in the the config.

Thing is, there is not much you can do better. You either use these methods, or maybe do it in a different way that exhibits the same problem. If you have to know subclasses/descendants then there is really no 100% reliable way to do so. The problem is that there is no advice to give on what to do except not to touch it.

Do you have a proposal?

Ruby docs have an example https://docs.ruby-lang.org/en/master/Class.html#method-i-subclasses

[pirj]

Rubocop's cops are meant to, no? To raise awareness?

There are so many cops that only a few get configured, the most offending ones get silenced, and it happens that only touched code gets linted. Whether the code is "corrected" when the project updates their RuboCop dependency and new cops are introduced, or not - depends on the project.

With this in mind, I doubt that such a cop, or a guideline will be helpful. @Earlopain has a good point that the method's documentation needs to be improved.

[koic]

Yeah, it doesn't seem like a rule that should be stated in the Rails Style Guide. Please also refer to @rafaelfranca's comment posted in the RuboCop Rails.
rubocop/rubocop-rails#1545 (comment)

Thank you.