Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from Oct 23, 2020

Conversation

@Zzzen
Copy link
Contributor

@Zzzen Zzzen commented May 1, 2020

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
Copy link

@jsf-clabot jsf-clabot commented May 1, 2020

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label May 1, 2020
@@ -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));

This comment has been minimized.

@Zzzen

Zzzen May 1, 2020
Author Contributor

#11308 did not handle following code:

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

demo

This comment has been minimized.

@mdjermanovic

mdjermanovic May 2, 2020
Member

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

This comment has been minimized.

@Zzzen

Zzzen May 2, 2020
Author Contributor

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

@Zzzen Zzzen requested a review from mdjermanovic May 6, 2020
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented May 14, 2020

@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.

return (operator === "||" && !!node.value) ||
(operator === "&&" && !node.value);
Comment on lines 66 to 67

This comment has been minimized.

@mysticatea

mysticatea May 15, 2020
Member

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.

This comment has been minimized.

@Zzzen

Zzzen May 16, 2020
Author Contributor

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
        }
    }
}

This comment has been minimized.

@mysticatea

mysticatea May 17, 2020
Member

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.

This comment has been minimized.

@Zzzen

Zzzen Jun 18, 2020
Author Contributor

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;
}

This comment has been minimized.

@mysticatea

mysticatea Jun 21, 2020
Member

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.

This comment has been minimized.

@Zzzen

Zzzen Jun 22, 2020
Author Contributor

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

This comment has been minimized.

@mdjermanovic

mdjermanovic Jun 22, 2020
Member

The actual version is using espree 7.1.0

"espree": "^7.1.0",

This comment has been minimized.

@Zzzen

Zzzen Jun 27, 2020
Author Contributor

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

This comment has been minimized.

@Zzzen

Zzzen Jun 27, 2020
Author Contributor

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
@kaicataldo 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
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 17, 2020

@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 mdjermanovic added accepted and removed evaluating labels Oct 17, 2020
// 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));
}
Comment on lines 79 to 82

This comment has been minimized.

@mdjermanovic

mdjermanovic Oct 17, 2020
Member

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.

This comment has been minimized.

@Zzzen

Zzzen Oct 18, 2020
Author Contributor

👌 That would simplify implementation a lot.

This comment has been minimized.

@mdjermanovic

mdjermanovic Oct 18, 2020
Member

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.

This comment has been minimized.

@Zzzen

Zzzen Oct 18, 2020
Author Contributor

🤦‍♂️ fixed.

return !(/^0+$/u.test(node.bigint) || /^0[box]0+$/ui.test(node.bigint));
}

throw new Error(`unsupported literal: ${node.raw}`);

This comment has been minimized.

@mdjermanovic

mdjermanovic Oct 17, 2020
Member

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.

This comment has been minimized.

@Zzzen

Zzzen Oct 18, 2020
Author Contributor

Done

@mdjermanovic 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
Copy link
Member

@mdjermanovic mdjermanovic left a comment

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

lib/rules/no-constant-condition.js Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Looks great, thanks!

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

@mdjermanovic mdjermanovic requested review from aladdin-add and mysticatea Oct 21, 2020
@mdjermanovic mdjermanovic merged commit 603de04 into eslint:master Oct 23, 2020
12 checks passed
12 checks passed
Verify Files
Details
Test (ubuntu-latest, 14.x)
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 10.12.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Oct 23, 2020

Great work, thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.