Update: treat all literals like boolean literal in no-constant-condition #13245

Zzzen · 2020-05-01T14:35:48Z

Prerequisites checklist

I have read the contributing guidelines.
The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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

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

What changes did you make? (Give an overview)

Previously, only boolean literals are considered in isLogicalIdentity. All literals will be coerced into boolean now.

Is there anything you'd like reviewers to focus on?

jsf-clabot · 2020-05-01T14:35:49Z

All committers have signed the CLA.

Zzzen · 2020-05-01T14:41:23Z

lib/rules/no-constant-condition.js

@@ -129,7 +129,7 @@ module.exports = {
                    const isLeftConstant = isConstant(node.left, inBooleanPosition);
                    const isRightConstant = isConstant(node.right, inBooleanPosition);
                    const isLeftShortCircuit = (isLeftConstant && isLogicalIdentity(node.left, node.operator));
-                    const isRightShortCircuit = (isRightConstant && isLogicalIdentity(node.right, node.operator));
+                    const isRightShortCircuit = (inBooleanPosition && isRightConstant && isLogicalIdentity(node.right, node.operator));


#11308 did not handle following code:

if ((foo || true) === 'baz') {}

demo

This is indeed a bug that should be fixed whether or not #13238 gets accepted.

Could we maybe also this:

( !node.parent || node.parent.type !== "BinaryExpression" || !(EQUALITY_OPERATORS.includes(node.parent.operator) || RELATIONAL_OPERATORS.includes(node.parent.operator)) )

replace with just inBooleanPosition?

I think it might fix similar bugs, such as:

/*eslint no-constant-condition: "error"*/ if ((foo || 'bar' || 'bar') === 'bar'); // error, false positive

Though, it seems that the whole condition starting with node.operator === "||" && might be removed in this PR (if #13238 gets accepted), but we should also doublecheck what's happening with the recursion in isLogicalIdentity. It already looks buggy in the actual version:

/*eslint no-constant-condition: "error"*/ if (a && false || b); // error, false positive

The cause is an inappropriate propagation of short circuit.
fixed in 96d0834

mdjermanovic · 2020-05-14T09:56:40Z

@Zzzen let's wait to see if the original issue will be accepted, before reviewing.

If it doesn't get accepted, we could just fix the other bug you found in this PR.

mysticatea · 2020-05-15T12:40:41Z

lib/rules/no-constant-condition.js

+                    return (operator === "||" && !!node.value) ||
+                           (operator === "&&" && !node.value);


ESTree doesn't guarantee node.value is a valid value. For example, if you run ESLint on Node.js that doesn't support BigInt, node.value is null at bigint literals. It will cause false positive/negative. Similarly, RegExp literals that contain new syntax, bigdecimal literals (stage 1) are, too.

How about reporting an unsupported error like the following code?

class UnsupportedNodeError extends Error {} function isNodeTruthy(node) { if (node.value === null && node.raw !== 'null') { throw new UnsupportedNodeError(`unsupported node: ${node.type}`) } return !!node.value; } function isLogicalIdentity(node, operator) { switch (node.type) { case "Literal": return (operator === "||" && isNodeTruthy(node)) || (operator === "&&" && !isNodeTruthy(node)); // ... } } function reportIfConstant(node) { try { if (node.test && isConstant(node.test, true)) { context.report({ node: node.test, messageId: "unexpected" }); } } catch (error) { if (error instanceof UnsupportedNodeError) { context.report({ node: node.test, message: error.message }) } else { throw error } } }

Even if the runtime doesn't support a syntax, ESLint should check the syntax successfully if ESLint supports it. In other words, the result of linting should be the same on all supported runtimes.

In other words, the result of linting should be the same on all supported runtimes.

Can we achieve runtime consistency by not supporting bigint on advanced runtime? Though bigint will not behave the same as numbers, just like numbers not behaving the same as booleans currently.

function isNodeTruthy(node) { // TODO: bigint is not supported. wait till node v10.4.0+ is popular. if (typeof node.value === 'bigint' || (node.value === null && node.raw !== 'null')) { return false; } return !!node.value; }

As per ESTree spec, the Literal nodes of BigInt have bigint property with the text representation of the bigint value. And the Literal nodes of RegExp have regex property with { pattern: string; flags: string }. The value property of Literal nodes of those may be null, so we have to check the bigint/regex properties to distinguish null literal and the others.

Currently, eslint is using espree 6.2.1, which does not support bigint or regex with u flag. 🤦

The actual version is using espree 7.1.0

eslint/package.json

Line 59 in b7d79b1

"espree": "^7.1.0",

I was using an old version. Anyway, fallback handling is added. Your suggestions would be helpful.

https://github.com/eslint/eslint/pull/13245/files#diff-aee426385c16cd9eadb5fabd23dfa0bfR64-R84

The code below is working on node 10.0.0.

if(a); if(/foo/ui); if(0n); if(1n && a); if(1n || a);

2:4 error Unexpected constant condition no-constant-condition 3:4 error Unexpected constant condition no-constant-condition 5:4 error Unexpected constant condition no-constant-condition

tests/lib/rules/no-constant-condition.js

mdjermanovic · 2020-10-17T18:18:29Z

@Zzzen sorry for the delay!

I'm marking this as an accepted bug fix, based on comments and +1s from team members in #13238.

For example, it doesn't make sense to treat false and void foo as falsy values, but not do the same for null and 0.

This is behavior in the actual version of this rule, and it looks inconsistent:

/* eslint no-constant-condition: error */

if (false && a); // error

if (void 0 && a); // error

if (0 && a); // ok

if (null && a); // ok

This PR fixes that, and several other bugs.

Edit: also doesn't make sense to check values on the right side, but not on the left side as it is in the actual version:

/* eslint no-constant-condition: error */

if (a || 1); // error

if (1 || a); // ok

mdjermanovic · 2020-10-17T18:31:51Z

lib/rules/no-constant-condition.js

+                // https://tc39.es/ecma262/#sec-literals-numeric-literals
+                if (typeof node.bigint === "string") {
+                    return !(/^0+$/u.test(node.bigint) || /^0[box]0+$/ui.test(node.bigint));
+                }


This is well-done, but I'd prefer to remove this branch. All Node versions we support now will have the bigint value in node.value, and we're already using that fact in some rules, like yoda.

In a worst-case scenario (e.g., an old browser, though we don't officially support use of ESLint in browsers) bigint literals will be just ignored, which doesn't look that bad.

👌 That would simplify implementation a lot.

Thanks for the very quick response!

Sorry if my post was confusing, I meant to remove only the bigint part.

This was ok:

// regex is always truthy if (typeof node.regex === "object") { return true; }

Regex syntax may have more changes in the future, so we will always use node.regex in rules.

🤦‍♂️ fixed.

mdjermanovic · 2020-10-17T18:39:37Z

lib/rules/no-constant-condition.js

+                    return !(/^0+$/u.test(node.bigint) || /^0[box]0+$/ui.test(node.bigint));
+                }
+
+                throw new Error(`unsupported literal: ${node.raw}`);


We aim to not intentionally crash on unknown syntax.

Can we return null in this case, and explicitly check for true/false in isLogicalIdentity?

Since it will be a 3-state, maybe we could also rename this function to getBooleanValue.

mdjermanovic

The code looks great, I have just a small JSDoc suggestion.

lib/rules/no-constant-condition.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic

Looks great, thanks!

Also thanks for fixing several different bugs and cleaning up the code.

mdjermanovic · 2020-10-23T15:03:50Z

Great work, thanks for contributing!

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label May 1, 2020

Zzzen commented May 1, 2020

View reviewed changes

Fix: treet all literals like boolean literal in no-constant-condition

3afc750

Zzzen force-pushed the master branch from 7394849 to 3afc750 Compare May 1, 2020 14:42

mdjermanovic linked an issue May 2, 2020 that may be closed by this pull request

[no-constant-condition] all literal should be treated like boolean literal #13238

Closed

fix unexpected short circuit

96d0834

Zzzen requested a review from mdjermanovic May 6, 2020 03:00

mysticatea reviewed May 15, 2020

View reviewed changes

Zzzen added 3 commits June 23, 2020 21:26

handle bigin/regex on unsupported runtime

540bb20

Merge remote-tracking branch 'eslint/master'

6eb51a8

fix bigint tests

17787ea

kaicataldo changed the title ~~Fix: treet all literals like boolean literal in no-constant-condition~~ Fix: treat all literals like boolean literal in no-constant-condition Jun 29, 2020

aladdin-add requested changes Aug 12, 2020

View reviewed changes

tests/lib/rules/no-constant-condition.js Show resolved Hide resolved

mdjermanovic mentioned this pull request Aug 26, 2020

Update: support logical assignments in core rules (refs #13569) #13618

Merged

1 task

mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 17, 2020

mdjermanovic requested changes Oct 17, 2020

View reviewed changes

mdjermanovic changed the title ~~Fix: treat all literals like boolean literal in no-constant-condition~~ Update: treat all literals like boolean literal in no-constant-condition Oct 17, 2020

Zzzen added 2 commits October 18, 2020 13:42

simplify literal truthiness

b0c2e70

handle regex literal in unsupported environments

c4c8426

mdjermanovic mentioned this pull request Oct 18, 2020

Update: Fix && vs || short-circuiting false negatives (fixes #13634) #13769

Merged

1 task

mdjermanovic reviewed Oct 20, 2020

View reviewed changes

lib/rules/no-constant-condition.js Outdated Show resolved Hide resolved

Update lib/rules/no-constant-condition.js

ce019be

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic approved these changes Oct 21, 2020

View reviewed changes

mdjermanovic requested review from aladdin-add and mysticatea October 21, 2020 13:50

aladdin-add approved these changes Oct 22, 2020

View reviewed changes

mdjermanovic merged commit 603de04 into eslint:master Oct 23, 2020

mdjermanovic mentioned this pull request Dec 21, 2020

Update: check logical assignment in no-constant-condition #13946

Merged

1 task

eslint-github-bot bot locked and limited conversation to collaborators Apr 22, 2021

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: treat all literals like boolean literal in no-constant-condition #13245

Update: treat all literals like boolean literal in no-constant-condition #13245

Zzzen commented May 1, 2020

jsf-clabot commented May 1, 2020 •

edited

Zzzen May 1, 2020

mdjermanovic May 2, 2020

Zzzen May 2, 2020

mdjermanovic commented May 14, 2020

mysticatea May 15, 2020

Zzzen May 16, 2020

mysticatea May 17, 2020

Zzzen Jun 18, 2020

mysticatea Jun 21, 2020

Zzzen Jun 22, 2020

mdjermanovic Jun 22, 2020

Zzzen Jun 27, 2020

Zzzen Jun 27, 2020

mdjermanovic commented Oct 17, 2020 •

edited

mdjermanovic Oct 17, 2020

Zzzen Oct 18, 2020

mdjermanovic Oct 18, 2020

Zzzen Oct 18, 2020

mdjermanovic Oct 17, 2020

Zzzen Oct 18, 2020

mdjermanovic left a comment

mdjermanovic left a comment

mdjermanovic commented Oct 23, 2020

		return (operator === "\|\|" && !!node.value) \|\|
		(operator === "&&" && !node.value);

Update: treat all literals like boolean literal in no-constant-condition #13245

Update: treat all literals like boolean literal in no-constant-condition #13245

Conversation

Zzzen commented May 1, 2020

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)

Is there anything you'd like reviewers to focus on?

jsf-clabot commented May 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic commented Oct 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic commented Oct 23, 2020

jsf-clabot commented May 1, 2020 •

edited

mdjermanovic commented Oct 17, 2020 •

edited