feat: check more expressions in `no-extra-boolean-cast` #18222

kirkwaiblinger · 2024-03-21T22:38:07Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #18186

What changes did you make? (Give an overview)

~~Add support for checking "sequence expressions" ("comma operator") and right-hand-side operands of the nullish coalescing operator (??).~~

Note that neither is affected by the "enforceForLogicalOperands" option. The sequence expressions because it simply isn't one, and the ?? because it is not one either in this context. (Yes, technically, the AST calls it a logical expression, but it's known to users as the nullish coalescing operator, and it really has nothing to do with logical operations at all, in the sense in which this rule applies to && and ||.)

UPDATE - In addition to the cases mentioned in the issue (commas and ??), I was reminded while working on this (as a result of other linter work I've done) that checking the branches of ternaries applies yet again for the same reasons. I've updated the PR with that change, but can easily revert it if it's deemed an overstep. (see the following examples for further examples of the same recursion pattern applying to commas, ternaries, optional chains, and "logical expressions": https://github.com/typescript-eslint/typescript-eslint/blob/59bbf417702b53fe78d8a190da7054bc6c22b780/packages/eslint-plugin/src/rules/no-floating-promises.ts#L221-L324 or https://github.com/typescript-eslint/typescript-eslint/blob/59bbf417702b53fe78d8a190da7054bc6c22b780/packages/eslint-plugin/src/rules/prefer-find.ts#L44-L114)

UPDATE 2 - After significant discussion in the PR, the approach is now to preserve existing behavior, deprecate enforceForLogicalExpressions, and create a new option (tentatively enforceForInnerOperands) that checks all of the recursive syntaxes: &&, ||, RHS of ??, right-most expression of sequence expressions (a, b, c), and ternary consequent and alternate (test ? consequent : alternate)

…ions. Fixes eslint#18186

eslint-github-bot · 2024-03-21T22:38:11Z

Hi @kirkwaiblinger!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

netlify · 2024-03-21T22:38:29Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`c327282`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/662c3fd5dbd3a2000854a47c
😎 Deploy Preview	https://deploy-preview-18222--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

eslint-github-bot · 2024-03-21T22:45:22Z

Hi @kirkwaiblinger!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

eslint-github-bot · 2024-03-21T22:46:01Z

Hi @kirkwaiblinger!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

eslint-github-bot · 2024-03-21T22:46:18Z

Hi @kirkwaiblinger!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

Tanujkanti4441 · 2024-03-22T10:57:40Z

Thanks for the PR, can you also add some examples in docs too?

mdjermanovic · 2024-03-22T12:15:51Z

lib/rules/no-extra-boolean-cast.js

 case "ConditionalExpression":
 return precedence(node) <= precedence(parent);


This was assuming that previousNode is parent.test (as in the description of this function on line 167, which we should also update), but now it can be consequent/alternate, in which case only SequenceExpression would need parentheses.

For example:

/* eslint no-extra-boolean-cast: 2 */ if (a ? Boolean(b = c) : Boolean(d = e));

After the autofix:

/* eslint no-extra-boolean-cast: 2 */ if (a ? (b = c) : (d = e));

Parentheses around b = c and d = e are unnecessary.

This one took me a while to figure out trying to make use of the existing infrastructure.
I would have expected to be able to write

if (previousNode === parent.test) { return precedence(node) <= precedence(parent); } if (previousNode === parent.consequent || previousNode === parent.alternate) { return precedence(node) < precedence(parent); }

, which essentially just handles the right-associativity, but it seems that eslint's precedence() function differs from MDN's table, in that it treats AssignmentExpressions and similar as lower precedence than ternary, rather than equal... See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_precedence#table, vs

eslint/lib/rules/utils/ast-utils.js

Lines 1509 to 1602 in 239a7e2

getPrecedence(node) {

switch (node.type) {

case "SequenceExpression":

return 0;

case "AssignmentExpression":

case "ArrowFunctionExpression":

case "YieldExpression":

return 1;

case "ConditionalExpression":

return 3;

case "LogicalExpression":

switch (node.operator) {

case "||":

case "??":

return 4;

case "&&":

return 5;

// no default

}

/* falls through */

case "BinaryExpression":

switch (node.operator) {

case "|":

return 6;

case "^":

return 7;

case "&":

return 8;

case "==":

case "!=":

case "===":

case "!==":

return 9;

case "<":

case "<=":

case ">":

case ">=":

case "in":

case "instanceof":

return 10;

case "<<":

case ">>":

case ">>>":

return 11;

case "+":

case "-":

return 12;

case "*":

case "/":

case "%":

return 13;

case "**":

return 15;

// no default

}

/* falls through */

case "UnaryExpression":

case "AwaitExpression":

return 16;

case "UpdateExpression":

return 17;

case "CallExpression":

case "ChainExpression":

case "ImportExpression":

return 18;

case "NewExpression":

return 19;

default:

if (node.type in eslintVisitorKeys) {

return 20;

}

/*

* if the node is not a standard node that we know about, then assume it has the lowest precedence

* this will mean that rules will wrap unknown nodes in parentheses where applicable instead of

* unwrapping them and potentially changing the meaning of the code or introducing a syntax error.

*/

return -1;

}

},

Think that there should be a followup to change to match the MDN version, or is it better to just leave the current system?

We can consider changing this in a followup if the current logic is confusing and the new one would be better for practical use, but we'll need to check all rules that use getPrecedence() to see if they would need to be updated.

kirkwaiblinger · 2024-03-22T19:05:15Z

docs/src/rules/no-extra-boolean-cast.md

@@ -61,6 +61,19 @@ do {
 for (; !!foo; ) {
 // ...
 }
+
+// Complex expressions are also checked for their resulting expression(s).


@Tanujkanti4441 Added some examples 👍

mdjermanovic · 2024-03-27T12:58:12Z

Looks good to me, but I'm not sure if we have a consensus on this change. Per the discussion in #18186, checking sequence expressions is accepted as a bug fix, but this also adds checking operands of ?? and ?:.

I think it makes sense to check all these cases, but it looks unusual that checking operands of ||/&& is behind an option, while checking operands of ?? and ?: would be by default and without options.

kirkwaiblinger · 2024-03-27T14:13:20Z

Looks good to me, but I'm not sure if we have a consensus on this change. Per the discussion in #18186, checking sequence expressions is accepted as a bug fix, but this also adds checking operands of ?? and ?:.

Agreed. Lmk what a good course of action is. I can easily remove parts of the PR deemed out of scope (and, if requested, re-add them if a separate issue for them is accepted), or I can wait and watch for further discussion on whether to proceed with this PR as-is. I am unopinionated; making the code changes is easier than consensus-building :)

Tanujkanti4441 · 2024-03-27T15:44:02Z

I think we can accept ?? and ? operator as additional change in this PR. If we see following examples:

if ((a, b, c ?? (d, e, f ?? Boolean(g)))) {}

if ((a, b, c ?? (d, e, f ?? Boolean(g)))) {}

then somehow it's a case of sequence expression and can be reported on "enforceForLogicalOperands": true.

kirkwaiblinger · 2024-04-02T04:03:07Z

I think we can accept ?? and ? operator as additional change in this PR. If we see following examples:
if ((a, b, c ?? (d, e, f ?? Boolean(g)))) {}

if ((a, b, c ?? (d, e, f ?? Boolean(g)))) {}
then somehow it's a case of sequence expression and can be reported on "enforceForLogicalOperands": true.

Just to be clear, is there a specific action being requested here?

Tanujkanti4441 · 2024-04-02T04:10:50Z

Just to be clear, is there a specific action being requested here?

Not yet! it's just an opinion on reporting ?? and ? operator when "enforceForLogicalOperands": true i think we need more feedback here

mdjermanovic · 2024-04-02T07:12:01Z

@eslint/eslint-team thoughts about #18222 (comment)?

nzakas · 2024-04-02T18:27:10Z

I think it makes sense to check all these cases, but it looks unusual that checking operands of ||/&& is behind an option, while checking operands of ?? and ?: would be by default and without options.

I agree, this is a nice addition, but it does change this PR from a bug fix to a feature.

I also agree that it doesn't make sense to check ?? and ? by default. I think it makes sense to add ? to the enforceForLogicalOperands option, but I'm not sure about ??.

github-actions · 2024-04-12T22:31:15Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

Rec0iL99 · 2024-04-14T04:25:45Z

ping @eslint/eslint-team. What are the next steps here?

fasttime · 2024-04-16T18:57:00Z

Sorry for the late reply. I agree that it makes sense to check the operands of ?? and ? :. I don't have a strong opinion on whether those checks should be behind an option or not. If we want to put those checks behind an option to avoid reporting more problems unxpectedly, then I don't think we can reuse enforceForLogicalOperands, as that would in fact report more problems for configs that have this option set to true. If we just want to have the new checks behind an option because of the analogy with || and &&, then it's okay to add them to the cases already covered by enforceForLogicalOperands.

nzakas · 2024-04-16T20:53:49Z

@mdjermanovic thoughts on the feedback?

mdjermanovic · 2024-04-26T16:30:30Z

Looks like we all agree that it makes sense to check all these cases, and the only question is how the new checks would be enabled.

Since this is a stylistic rule, I don't think we can treat these as false negatives and thus enable the checks by default. I'd argue that the same applies to comma operands as well.

enforceForLogicalOperands also doesn't look like a good fit.

How about deprecating enforceForLogicalOperands and introducing a new option enforceForInnerOperands (or some better name)? The current behavior remains the same. When the new option is enabled, the rule checks all of ||, &&, ??, ?:, ,.

nzakas · 2024-04-26T17:35:17Z

How about deprecating enforceForLogicalOperands and introducing a new option enforceForInnerOperands (or some better name)? The current behavior remains the same. When the new option is enabled, the rule checks all of ||, &&, ??, ?:, ,.

It looks like we are in agreement to take this approach. 👍

kirkwaiblinger · 2024-04-26T21:40:40Z

@mdjermanovic @nzakas

How about deprecating enforceForLogicalOperands and introducing a new option enforceForInnerOperands (or some better name)? The current behavior remains the same. When the new option is enabled, the rule checks all of ||, &&, ??, ?:, ,.

I like this resolution!

As a suggestion, does anyone like the name enforceForIndirectExpressions?

fix: [no-extra-boolean-cast] inspect comma expressions and ?? express…

ce5ac38

…ions. Fixes eslint#18186

github-actions bot added the rule Relates to ESLint's core rules label Mar 21, 2024

kirkwaiblinger changed the title ~~fix: [no-extra-boolean-cast] inspect comma expressions and nullish coalescing expressions~~ fix: [no-extra-boolean-cast] inspect comma operator and ?? expressions Mar 21, 2024

eslint-github-bot bot added the bug ESLint is working incorrectly label Mar 21, 2024

kirkwaiblinger changed the title ~~fix: [no-extra-boolean-cast] inspect comma operator and ?? expressions~~ fix: [no-extra-boolean-cast] inspect more expressions Mar 21, 2024

kirkwaiblinger marked this pull request as ready for review March 21, 2024 22:48

kirkwaiblinger requested a review from a team as a code owner March 21, 2024 22:48

check ternaries recursively too

554caaf

Tanujkanti4441 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 22, 2024

mdjermanovic reviewed Mar 22, 2024

View reviewed changes

pr feedback

c60535b

kirkwaiblinger commented Mar 22, 2024

View reviewed changes

kirkwaiblinger requested a review from mdjermanovic March 22, 2024 19:10

mdjermanovic changed the title ~~fix: [no-extra-boolean-cast] inspect more expressions~~ feat: check more expressions in no-extra-boolean-cast Mar 27, 2024

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Mar 27, 2024

mdjermanovic added the contributor pool label Mar 27, 2024

github-actions bot added the Stale label Apr 12, 2024

Rec0iL99 removed the Stale label Apr 14, 2024

kirkwaiblinger and others added 5 commits April 26, 2024 15:51

Merge branch 'main' into no-extra-boolean-cast-inspect-more-expressions

c5c1630

change docs

8326217

add option for recursive expression checks

822ec6c

better wording

e24feeb

format

c327282

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: check more expressions in `no-extra-boolean-cast` #18222

feat: check more expressions in `no-extra-boolean-cast` #18222

kirkwaiblinger commented Mar 21, 2024 •

edited

eslint-github-bot bot commented Mar 21, 2024

netlify bot commented Mar 21, 2024 •

edited

eslint-github-bot bot commented Mar 21, 2024

eslint-github-bot bot commented Mar 21, 2024

eslint-github-bot bot commented Mar 21, 2024

Tanujkanti4441 commented Mar 22, 2024

mdjermanovic Mar 22, 2024

kirkwaiblinger Mar 22, 2024 •

edited

mdjermanovic Mar 27, 2024 •

edited

kirkwaiblinger Mar 22, 2024

mdjermanovic commented Mar 27, 2024

kirkwaiblinger commented Mar 27, 2024

Tanujkanti4441 commented Mar 27, 2024 •

edited

kirkwaiblinger commented Apr 2, 2024

Tanujkanti4441 commented Apr 2, 2024

mdjermanovic commented Apr 2, 2024

nzakas commented Apr 2, 2024

github-actions bot commented Apr 12, 2024

Rec0iL99 commented Apr 14, 2024

fasttime commented Apr 16, 2024

nzakas commented Apr 16, 2024

mdjermanovic commented Apr 26, 2024

nzakas commented Apr 26, 2024

kirkwaiblinger commented Apr 26, 2024

		case "ConditionalExpression":
		return precedence(node) <= precedence(parent);

	getPrecedence(node) {
	switch (node.type) {
	case "SequenceExpression":
	return 0;

	case "AssignmentExpression":
	case "ArrowFunctionExpression":
	case "YieldExpression":
	return 1;

	case "ConditionalExpression":
	return 3;

	case "LogicalExpression":
	switch (node.operator) {
	case "\|\|":
	case "??":
	return 4;
	case "&&":
	return 5;

	// no default
	}

	/* falls through */

	case "BinaryExpression":

	switch (node.operator) {
	case "\|":
	return 6;
	case "^":
	return 7;
	case "&":
	return 8;
	case "==":
	case "!=":
	case "===":
	case "!==":
	return 9;
	case "<":
	case "<=":
	case ">":
	case ">=":
	case "in":
	case "instanceof":
	return 10;
	case "<<":
	case ">>":
	case ">>>":
	return 11;
	case "+":
	case "-":
	return 12;
	case "*":
	case "/":
	case "%":
	return 13;
	case "**":
	return 15;

	// no default
	}

	/* falls through */

	case "UnaryExpression":
	case "AwaitExpression":
	return 16;

	case "UpdateExpression":
	return 17;

	case "CallExpression":
	case "ChainExpression":
	case "ImportExpression":
	return 18;

	case "NewExpression":
	return 19;

	default:
	if (node.type in eslintVisitorKeys) {
	return 20;
	}

	/*
	* if the node is not a standard node that we know about, then assume it has the lowest precedence
	* this will mean that rules will wrap unknown nodes in parentheses where applicable instead of
	* unwrapping them and potentially changing the meaning of the code or introducing a syntax error.
	*/
	return -1;
	}
	},

feat: check more expressions in no-extra-boolean-cast #18222

Are you sure you want to change the base?

feat: check more expressions in no-extra-boolean-cast #18222

Conversation

kirkwaiblinger commented Mar 21, 2024 • edited

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

eslint-github-bot bot commented Mar 21, 2024

netlify bot commented Mar 21, 2024 • edited

✅ Deploy Preview for docs-eslint ready!

eslint-github-bot bot commented Mar 21, 2024

eslint-github-bot bot commented Mar 21, 2024

eslint-github-bot bot commented Mar 21, 2024

Tanujkanti4441 commented Mar 22, 2024

mdjermanovic Mar 22, 2024

Choose a reason for hiding this comment

kirkwaiblinger Mar 22, 2024 • edited

Choose a reason for hiding this comment

mdjermanovic Mar 27, 2024 • edited

Choose a reason for hiding this comment

kirkwaiblinger Mar 22, 2024

Choose a reason for hiding this comment

mdjermanovic commented Mar 27, 2024

kirkwaiblinger commented Mar 27, 2024

Tanujkanti4441 commented Mar 27, 2024 • edited

kirkwaiblinger commented Apr 2, 2024

Tanujkanti4441 commented Apr 2, 2024

mdjermanovic commented Apr 2, 2024

nzakas commented Apr 2, 2024

github-actions bot commented Apr 12, 2024

Rec0iL99 commented Apr 14, 2024

fasttime commented Apr 16, 2024

nzakas commented Apr 16, 2024

mdjermanovic commented Apr 26, 2024

nzakas commented Apr 26, 2024

kirkwaiblinger commented Apr 26, 2024

feat: check more expressions in `no-extra-boolean-cast` #18222

feat: check more expressions in `no-extra-boolean-cast` #18222

kirkwaiblinger commented Mar 21, 2024 •

edited

netlify bot commented Mar 21, 2024 •

edited

kirkwaiblinger Mar 22, 2024 •

edited

mdjermanovic Mar 27, 2024 •

edited

Tanujkanti4441 commented Mar 27, 2024 •

edited