🐛 noConstructorReturn reasoning is wrong

[lgarron]

Environment information

Rome 11.0.0

What happened?

https://docs.rome.tools/lint/rules/noconstructorreturn/ states:

While returning a value from a constructor does not produce an error, the returned value is being ignored.

This is inaccurate:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/constructor

The constructor method may have a return value. While the base class may return anything from its constructor, the derived class must return an object or undefined, or a TypeError will be thrown.

I'm not saying it's a great idea to do so, but it's not particularly harmful to the type safety of the program if the returned object is of the same class (that the constructor would already return).

Expected result

The rule should either avoid erroring on returned objects (at least when the return type matches the constructor's class), or the documentation should be updated.

Code of Conduct

• I agree to follow Rome's Code of Conduct

[ematipico]

@Conaclos what do you think? You implemented the rule in the first place, your input would be really valuable!

[leops]

We should note though that a linter is not a compiler, what we consider to be errors is not strictly bound to the ECMAScript spec, and we can afford to be more strict if it helps catch potential errors, that's what the suspicious group is all about. We may emit diagnostics for code pattern that are perfectly valid but are deemed to be unusual and probably not what the user intended, with the rationale that if the code is actually doing what's intended, then you should put a suppression comment on top of it with a reason explaining why it's valid (this is the motivation behind having rules like noDangerouslySetInnerHtml, we don't discourage users from using dangerouslySetInnerHTML but we enforce that they add a comment explaining why it's safe)

[Conaclos]

but it's not particularly harmful to the type safety of the program if the returned object is of the same class (that the constructor would already return).

Yes. However, it is harmful regarding the mental model that developers generally have about classes.
If the constructor returns something, then the caller could expect to get the returned value. This is not the case: with new you obtain the instance, and by calling the constructor as a regular function you obtain an error.

There is only one case or the returned value affects the program: when derived classes enter the play.
I was not aware about this oddness. Never too late to learn one more JS oddness ;)
According to the MDN, the returned value becomes èthis` in derived classes. This is certainly a relic of prototype-based inheritance…

By the way, MDN discourages the use of this "feature":

Warning: This is a potentially very confusing thing to do. You are generally advised to avoid returning anything from the constructor — especially something unrelated to this.

We could update the rule description in a way that does not contradict MDN docs. However, I think we should avoid describing this "feature".

We could relax the rule and allow returning this. However, I did not see the usefulness of this. Moreover, this could diverge from the ESLint implementation.

[lgarron]

I think it's totally reasonable for a linter to be rather parental about a "technically could be valid, but usually a bad idea" case like this — I don't feel strongly about the rule, I would just advocate updating the docs if the intention is to warn on "valid" use cases!