Repository: eslint/eslint
Pull request #15613 (2022-02-16 06:29:16)

feat: Catch more cases in no-constant-condition
Identify `undefined` and `Boolean(somethingConstant)` as both being constant.

<!--
    Thank you for contributing!

    ESLint adheres to the [JS Foundation Code of Conduct](https://eslint.org/conduct).
-->

#### Prerequisites checklist

- [X] I have read the [contributing guidelines](https://github.com/eslint/eslint/blob/HEAD/CONTRIBUTING.md).

#### What is the purpose of this pull request? (put an "X" next to an item)

<!--
    The following template is intentionally not a markdown checkbox list for the reasons
    explained in https://github.com/eslint/eslint/pull/12848#issuecomment-580302888
-->

[X] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/HEAD/templates/rule-change-proposal.md))

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

#### What changes did you make? (Give an overview)

Expanded `no-constant-condition` to catch two types of additional cases:

```
if(undefined) {}

if(Boolean(1)) {}
```

#### Is there anything you'd like reviewers to focus on?

<!-- markdownlint-disable-file MD004 -->


captbaritone: I've updated the commit message to mark this as a `feat:` in line with @mdjermanovic's change of the title of the PR.
captbaritone: Updated.
captbaritone: cc @mdjermanovic for visibility (no hurry if you've already seen my update)
nzakas: @captbaritone are you still working on this?
captbaritone: Oh, thanks for pinging. I missed @mdjermanovic's last reply. Yes. Still working on this.
captbaritone: Updated.
captbaritone: I think the lint failure is failing on the main branch:

```
docs/user-guide/configuring/configuration-files.md: 361: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
```
mdjermanovic: > I think the lint failure is failing on the main branch

It's fixed now, I'll close & reopen to re-run checks on top of the latest `main`.
captbaritone: I've fixed the docblock and added two examples to the docs.

Patch:
diff --git a/docs/rules/no-constant-condition.md b/docs/rules/no-constant-condition.md
index 3ecf499ea..810c17bea 100644
--- a/docs/rules/no-constant-condition.md
+++ b/docs/rules/no-constant-condition.md
@@ -42,6 +42,14 @@ if (new Boolean(x)) {
     doSomethingAlways();
 }
 
+if (Boolean(1)) {
+    doSomethingAlways();
+}
+
+if (undefined) {
+    doSomethingUnfinished();
+}
+
 if (x ||= true) {
     doSomethingAlways();
 }
diff --git a/lib/rules/no-constant-condition.js b/lib/rules/no-constant-condition.js
index 8adc9bca4..3d08c689a 100644
--- a/lib/rules/no-constant-condition.js
+++ b/lib/rules/no-constant-condition.js
@@ -120,12 +120,30 @@ module.exports = {
             return false;
         }
 
+        /**
+         * Checks if an identifier is a reference to a global variable.
+         * @param {ASTNode} node An identifier node to check.
+         * @returns {boolean} `true` if the identifier is a reference to a global variable.
+         */
+        function isReferenceToGlobalVariable(node) {
+            const scope = context.getScope();
+            const reference = scope.references.find(ref => ref.identifier === node);
+
+            return Boolean(
+                reference &&
+                reference.resolved &&
+                reference.resolved.scope.type === "global" &&
+                reference.resolved.defs.length === 0
+            );
+        }
+
         /**
          * Checks if a node has a constant truthiness value.
          * @param {ASTNode} node The AST node to check.
-         * @param {boolean} inBooleanPosition `false` if checking branch of a condition.
-         *  `true` in all other cases. When `false`, checks if -- for both string and
-         *  number -- if coerced to that type, the value will be constant.
+         * @param {boolean} inBooleanPosition `true` if checking the test of a
+         * condition. `false` in all other cases. When `false`, checks if -- for
+         * both string and number -- if coerced to that type, the value will
+         * be constant.
          * @returns {Bool} true when node's truthiness is constant
          * @private
          */
@@ -215,6 +233,15 @@ module.exports = {
                     return isConstant(node.expressions[node.expressions.length - 1], inBooleanPosition);
                 case "SpreadElement":
                     return isConstant(node.argument, inBooleanPosition);
+                case "CallExpression":
+                    if (node.callee.type === "Identifier" && node.callee.name === "Boolean") {
+                        if (node.arguments.length === 0 || isConstant(node.arguments[0], true)) {
+                            return isReferenceToGlobalVariable(node.callee);
+                        }
+                    }
+                    return false;
+                case "Identifier":
+                    return node.name === "undefined" && isReferenceToGlobalVariable(node);
 
                 // no default
             }


Test patch:
diff --git a/tests/lib/rules/no-constant-condition.js b/tests/lib/rules/no-constant-condition.js
index 9cca27b90..c22b2947a 100644
--- a/tests/lib/rules/no-constant-condition.js
+++ b/tests/lib/rules/no-constant-condition.js
@@ -185,7 +185,17 @@ ruleTester.run("no-constant-condition", rule, {
         "if (`${[a]}`) {}",
         "if (+[a]) {}",
         "if (0 - [a]) {}",
-        "if (1 * [a]) {}"
+        "if (1 * [a]) {}",
+
+        // Boolean function
+        "if (Boolean(a)) {}",
+        "if (Boolean(...args)) {}",
+        "if (foo.Boolean(1)) {}",
+        "function foo(Boolean) { if (Boolean(1)) {} }",
+        "const Boolean = () => {}; if (Boolean(1)) {}",
+        { code: "if (Boolean()) {}", globals: { Boolean: "off" } },
+        "const undefined = 'lol'; if (undefined) {}",
+        { code: "if (undefined) {}", globals: { undefined: "off" } }
     ],
     invalid: [
         { code: "for(;true;);", errors: [{ messageId: "unexpected", type: "Literal" }] },
@@ -396,6 +406,18 @@ ruleTester.run("no-constant-condition", rule, {
         { code: "if(new Number(foo)) {}", errors: [{ messageId: "unexpected" }] },
 
         // Spreading a constant array
-        { code: "if(`${[...['a']]}`) {}", errors: [{ messageId: "unexpected" }] }
+        { code: "if(`${[...['a']]}`) {}", errors: [{ messageId: "unexpected" }] },
+
+        /*
+         * undefined is always falsy (except in old browsers that let you
+         * re-assign, but that's an abscure enough edge case to not worry about)
+         */
+        { code: "if (undefined) {}", errors: [{ messageId: "unexpected" }] },
+
+        // Coercion to boolean via Boolean function
+        { code: "if (Boolean(1)) {}", errors: [{ messageId: "unexpected" }] },
+        { code: "if (Boolean()) {}", errors: [{ messageId: "unexpected" }] },
+        { code: "if (Boolean([a])) {}", errors: [{ messageId: "unexpected" }] },
+        { code: "if (Boolean(1)) { function Boolean() {}}", errors: [{ messageId: "unexpected" }] }
     ]
 });