Bug: possible `no-constructor-return` false negative with nested blocks

[rzvxa]

Environment

Playground

What parser are you using?

Default (Espree)

What did you do?

Run this code in the playground

class A { constructor() { return } }
class B { constructor() { { return } } }

What did you expect to happen?

I was expecting class A and B constructors to both produce errors for the Unexpected return statement in constructor. (no-constructor-return).

What actually happened?

Class A results in an error but as soon as you start wrapping the return statement in empty blocks it confuses the rule(class B). It might very well be the intended behavior I'm just posting this issue here for confirmation since it isn't discussed anywhere in PRs or issues mentioning this rule and it is an undocumented behavior.

Link to Minimal Reproducible Example

https://eslint.org/play/#eyJ0ZXh0IjoiY2xhc3MgQSB7IGNvbnN0cnVjdG9yKCkgeyByZXR1cm4gfSB9XG5jbGFzcyBCIHsgY29uc3RydWN0b3IoKSB7IHsgcmV0dXJuIH0gfSB9Iiwib3B0aW9ucyI6eyJydWxlcyI6eyJuby1jb25zdHJ1Y3Rvci1yZXR1cm4iOlsiZXJyb3IiXX0sImxhbmd1YWdlT3B0aW9ucyI6eyJwYXJzZXJPcHRpb25zIjp7ImVjbWFGZWF0dXJlcyI6e319fX19

Participation

• [ ] I am willing to submit a pull request for this issue.
• [x] Yes I'm willing to submit a pull request for this issue, But seems it is a small yet opinionated change, So it wouldn't be necessary.

Additional comments

No response

[mdjermanovic]

I think it isn't a bug that this rule doesn't report class B { constructor() { { return } } }, but it is a bug that this rule reports class A { constructor() { return } }. I think this rule should report only return <value>, as the documentation states:

In JavaScript, returning a value in the constructor of a class may be a mistake

This rule disallows return statements in the constructor of a class. Note that returning nothing with flow control is allowed.

So, return; is just a control flow statement and thus should be allowed by this rule. Useless ones are reported by no-useless-return.

@eslint/eslint-team thoughts?

[rzvxa]

I think it isn't a bug that this rule doesn't report class B { constructor() { { return } } }, but it is a bug that this rule reports class A { constructor() { return } }. I think this rule should report only return , as the documentation states:

I agree. Either way, both the redundant block and return statement are reported by other rules. In my opinion, it is fine as long as it is consistent.

Btw great job on that quick response, It is my first issue here and I have to say; I'm impressed.

[fasttime]

So, return; is just a control flow statement and thus should be allowed by this rule. Useless ones are reported by no-useless-return.

I also think that it makes sense to allow return statements without a value in a constructor. But I doubt that this was the intention of the author of that rule. See this invalid unit test:

https://github.com/eslint/eslint/blob/946ae00457265eb298eb169d6d48ca7ec71b9eef/tests/lib/rules/no-constructor-return.js#L47-L50

I assume that this sentence in the rule description:

Note that returning nothing with flow control is allowed.

means that returning nothing in a flow-control statement (e.g. if, while, switch etc.) is allowed. This doesn't include empty blocks, which do not serve for flow control. So I think that the original intent was to forbid things like class B { constructor() { { return } } } as well, and to only allow void return statements nested in a conditional statement, for example:

https://github.com/eslint/eslint/blob/946ae00457265eb298eb169d6d48ca7ec71b9eef/tests/lib/rules/no-constructor-return.js#L41

I'd be open to changing the current logic if the team reaches an agreement.

[Tanujkanti4441]

means that returning nothing in a flow-control statement (e.g. if, while, switch etc.) is allowed. This doesn't include empty blocks, >which do not serve for flow control. So I think that the original intent was to forbid things like class B { constructor() { { return } } } >as well,

from this perspective i also think rule should report class B { constructor() { { return } } }.

[mdjermanovic]

Note that other rules that disallow return <value> always allow return;:

/* eslint array-callback-return: ["error", { checkForEach: true }] */
arr.forEach(item => {
    foo(item);
    return; // ok
});

/* eslint no-promise-executor-return: "error" */
new Promise(resolve => {
    resolve(foo);
    return; // ok
});

/* eslint no-setter-return: "error" */
class Foo {
    set a(value) {
        this._a = a;
        return; // ok
    }
}

Playground demo

[nzakas]

I agree -- the rule should allow return without a return value regardless of its location inside of a constructor. 👍

[nzakas]

@rzvxa would you like to submit a PR for this change?

[rzvxa]

@rzvxa would you like to submit a PR for this change?

I don't mind it, But since it is a one-liner, there is not much substance to this change; And I don't have the eslint checked out and ready to work on, That's why I think if a regular contributor takes this over it would be more productive.

With all that said if there are not enough resources to fix this just let me know and I'll submit a PR tonight.

[nzakas]

We are actively managing over 100 issues and PRs, so if you can implement this change, it would help us out a lot.

[rzvxa]

No worries, I'll take care of it.