Rule Change: no-extra-boolean-cast should check final expression in comma operator expressions

[kirkwaiblinger]

A comma operator/sequence expression should have its last expression checked in a boolean context. For example, the following should flag:

if ((1, 2, Boolean(3))) {
    // do something
}

since it is equivalent to

if ((1, 2, 3)) {
    // do something
}

The original issue description was significantly different from the decided upon course of action. It is preserved below for reference:

What rule do you want to change?

no-extra-boolean-cast

What change do you want to make?

Generate more warnings

How do you think the change should be implemented?

A new default behavior

Example code

if (Boolean("don't flag") && Boolean("should flag!")) {
}

What does the rule currently do for this code?

by default: no error
with { "enforceForLogicalOperands": true}: flags on both Boolean() calls

What will the rule do after it's changed?

By default: flag on Boolean("should flag!")
with { "enforceForLogicalOperands": true}: same as current

Participation

• I am willing to submit a pull request to implement this change.

Additional comments

This can be extended to the other "logical" operator varieties, i.e. || and ??, and can also apply to "sequence expressions"/comma operator expressions (if ((1, 2, Boolean(3))))

[kirkwaiblinger]

Actually, now that I think about, there's an argument that both should flag by default. No matter which branch of if (x && y) is truthy/falsey, either one will always, only be used in a boolean context, so converting either to a boolean is redundant. For the comma operator example, it is true that only the last element should be flagged. (as well as the ??, though obviously for different reasons in that case)

[Tanujkanti4441]

Hi @kirkwaiblinger, thanks for this issue, as no-extra-boolean-cast rule aims to disallow unnecessary boolean casts then i think reporting only one by default and leave another is not convincing, as you also said both should flag by default then not sure completely because the option already does that or may be we can take it as exception.
But rule doesn't report boolean casts in comma operator, @eslint/eslint-team can we consider this?

[JoshuaKGoldberg]

there's an argument that both should flag by default

Agreed. Right now, if (Boolean("don't flag") && Boolean("should flag!")) {} does report both of them with enforceForLogicalOperands: true. [Playground link with no options] / [Playground link with `enforceForLogicalOperands

So it feels to me like the current if ( ... && ... ) behavior is the correct one, right?

if ((1, 2, Boolean(3)))
comma operator

The rule not flagging the Boolean(3) there seems like a straightforward bug to me. The 1, 2, are essentially irrelevant as I understand it. 👍 to fixing the rule's logic to ignore them and treat this the same as if (Boolean(3)).

[kirkwaiblinger]

@JoshuaKGoldberg

there's an argument that both should flag by default

Agreed. Right now, if (Boolean("don't flag") && Boolean("should flag!")) {} does report both of them with enforceForLogicalOperands: true. [Playground link with no options] / [Playground link with `enforceForLogicalOperands

So it feels to me like the current if ( ... && ... ) behavior is the correct one, right?

Oh, these were very helpful examples. I understand better now. I had assumed that the point of the enforceForLogicalOperands option would be to flag on the value = Boolean(x) && Boolean(y) expression, but your example demonstrated otherwise. So, I agree, yeah, it's working as intended (though, knowing what it does, I would personally never set that option to be false if I were using this rule!).

if ((1, 2, Boolean(3)))
comma operator

The rule not flagging the Boolean(3) there seems like a straightforward bug to me. The 1, 2, are essentially irrelevant as I understand it. 👍 to fixing the rule's logic to ignore them and treat this the same as if (Boolean(3)).

Should we change the title to just be about that or is it different enough from the original post that it should be tracked under a new issue?

[nzakas]

@kirkwaiblinger we can reuse this issue. Can you update the title and description to focus specifically on sequence expressions?

I agree that this is a bug with sequence expressions in if statement that should be fixed.