Inconsistent evaluation of strings in no-constant-condition

[MerlinMason]

Tell us about your environment

• ESLint Version: 5.9.0
• Node Version: 11.3.0
• npm Version: 6.4.1

What parser are you using?
babel-eslint

Please show your full configuration:

Configuration

{
    "parser": "babel-eslint",
    "parserOptions": {
        "sourceType": "module",
        "allowImportExportEverywhere": true
    },
    "extends": [
        "airbnb",
        "plugin:lodash/recommended"
    ],
    "env": {
        "browser": true,
        "jquery": true,
        "node": true,
        "mocha": true
    },
    "globals": {
        "expect": true,
        "cy": true,
        "Cypress": true,
        "__VERSION__": true,
        "__RELEASED__": true,
        "__INTERCOMID__": true
    },
    "plugins": [
        "backbone",
        "compat",
        "mocha",
        "chai-friendly",
        "lodash",
        "json"
    ],
    "settings": {
        "backbone": {
            "Collection": ["BaseCollection"],
            "Model": ["BaseModel"],
            "View": ["BaseView"]
        }
    },
    "rules": {
        "no-unused-vars": [1, { "vars": "all", "args": "after-used", "ignoreRestSiblings": true, "varsIgnorePattern": "^_", "argsIgnorePattern": "^_" }],
        "no-unused-expressions": 0,
        "chai-friendly/no-unused-expressions": 2,
        "arrow-parens": ["error", "always"],
        "indent": ["error", 4, { "SwitchCase": 1 }],
        "quotes": [1, "double"],
        "comma-dangle": ["error", "never"],
        "new-cap": ["error", { "capIsNew": false }],
        "brace-style": [1, "1tbs", { "allowSingleLine": true }],
        "camelcase": 0,
        "no-plusplus": 0,
        "no-param-reassign": 0,
        "no-underscore-dangle": 0,
        "max-len": 0,
        "no-console": 0,
        "default-case": 0,
        "func-names": 0,
        "no-new": 0,
        "global-require": 0,
        "eol-last": 1,
        "class-methods-use-this": 0,
        "no-cond-assign": [1, "except-parens"],
        "newline-per-chained-call": [2, {"ignoreChainWithDepth": 5}],
        "eqeqeq": ["error", "always"],
        "valid-jsdoc": ["error", { "prefer": { "return": "returns", "extends": "extends" }, "preferType": { "object": "Object" }, "requireReturn": false, "requireReturnType": true, "requireReturnDescription": false, "matchDescription": ".+" }],
        "react/require-extension": [0, { "extensions": [".js"] }],
        "prefer-destructuring": [2, { "array": false, "object": true }, { "enforceForRenamedProperties": false }],
        "import/no-extraneous-dependencies": [0, {}],
        "import/no-unresolved": [2, { "ignore": ["env"] }],
        "import/extensions": 0,
        "import/no-cycle": 0,
        "mocha/no-exclusive-tests": "error",
        "backbone/collection-model": 0,
        "backbone/defaults-on-top": 2,
        "backbone/events-on-top": [2, ["el"]],
        "backbone/initialize-on-top": [2, { "View": ["el", "events"] }],
        "backbone/model-defaults": 0,
        "backbone/no-constructor": 2,
        "backbone/no-el-assign": 2,
        "backbone/no-native-jquery": [2, "selector"],
        "lodash/prefer-lodash-method": 0,
        "lodash/prefer-noop": 1,
        "lodash/import-scope": 0,
        "lodash/prefer-constant": 0,
        "lodash/prefer-filter": 0,
        "lodash/prefer-flat-map": 1,
        "lodash/collection-method-value": 0,
        "lodash/collection-return": 0,
        "lodash/prefer-lodash-chain": 1,
        "lodash/prop-shorthand": 0,
        "lodash/matches-syntax": 0,
        "lodash/matches-shorthand": 0
    }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

if (category === "voucher" || "custom") { }

eslint --fix app

What did you expect to happen?
An error should be thrown as a string will always evaluate to true. This would be consistent with the following examples where errors are thrown.

if (category === "voucher" || true) { }

if ("foo" || "bar") { }

What actually happened? Please include the actual, raw output from ESLint.
No error was thrown.

Are you willing to submit a pull request to fix this bug?
Happy to have a go if somebody can point me in the right direction :)

[nzakas]

Thanks for the report. When I try this locally by enabling no-constant-condition, the code you mention does result in ESLint reporting an error. Is it possible that no-constant-condition isn't enabled in your project or is turned off in another location?

[MerlinMason]

Hi @nzakas, thanks for getting back to me. To clarify, the rule is definitely enabled but is not triggered by the first example:

However it is triggered for the following examples:

Hope that helps 😄

[nzakas]

Thanks for clarifying. This does look like a bug.

If you'd like to take a look, I'm guessing the issue is somewhere in here:
https://github.com/eslint/eslint/blob/master/lib/rules/no-constant-condition.js#L106

[MerlinMason]

Cool ok, I’ll give it a go at some point over the Christmas break!

…

On 19 Dec 2018, at 3:27 pm, Nicholas C. Zakas ***@***.***> wrote: Thanks for clarifying. This does look like a bug. If you'd like to take a look, I'm guessing the issue is somewhere in here: https://github.com/eslint/eslint/blob/master/lib/rules/no-constant-condition.js#L106 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

-- *This e-mail is confidential, please read our related legal terms <https://thanksbox.co/legal/email/>.*

[MerlinMason]

Hey, so I've had a look at this but thought I'd like to sense check some stuff before doing any more.

My first step was to add some examples of invalid code to the tests:

{ code: "if(abc==='str' || 'str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if('str' || abc==='str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] },
{ code: "if(a || 'str' || abc==='str'){}", errors: [{ messageId: "unexpected", type: "LogicalExpression" }] }

This was then causing the tests to fail (as expected) so I started having a look at the rule. The following line (https://github.com/eslint/eslint/blob/master/lib/rules/no-constant-condition.js#L112):

return (isLeftConstant && isRightConstant) || isLeftShortCircuit || isRightShortCircuit;

...doesn't look correct to me. If I've understood the rule correctly, then any constant in a conditional should be marked as an error, so I updated it to this:

return isLeftConstant || isRightConstant || isLeftShortCircuit || isRightShortCircuit;

This causes my newly added tests to pass, but also causes some existing tests to fail. However I'm not entirely sure these tests are correct. The following are all marked as valid examples, yet all of them contain a constant:

"if (void a || a);",
"if (a || void a);",
"if(false || abc==='str'){}",
"if(true && abc==='str'){}",
"if(typeof 'str' && abc==='str'){}",
"if(abc==='str' || false || def ==='str'){}",
"if(true && abc==='str' || def ==='str'){}",
"if(true && typeof abc==='string'){}",
"if(a === 'str' && typeof b){}"

If you agree these tests are incorrect I'll be happy to change them to be incorrect examples and raise a PR. If not, then some clarification around the intention of this rule would be useful. Thanks!

[platinumazure]

Hi @MerlinBB, thank you for sharing your detailed research!

I don't think the intent of the rule is to look for any constants in the condition. Rather, the goal should be to see if the condition (as a whole) will always result in the same value.

Let's take this example: if (void a || a);

In this case, void a is a constant in that it will always return undefined. As you probably know, undefined is falsy. In the case of an or conditional, that means we have to look at the right side. The right side here is a variable reference, so it's not constant and the rule shouldn't report. However, I would expect if (void a && a); to be invalid, since the undefined value will short-circuit in that case.

It seems more likely to be that we need to look at the logic that flags a single given expression as constant and ensure string literals are detected as constants. At this point, based on what you've shown so far, I don't think there's anything wrong with the line that checks the logical expression (and/or) in terms of whether both terms are constant or there's a constant short circuit.

Hope this helps! Let me know if I can clarify anything.

[nzakas]

That's correct. The intent of the rule is to determine if the condition can ever change based on the inputs. In the case of logical OR, something that always evaluated to false on the left means that the right will always be used.

…

On Wed, Dec 26, 2018 at 4:58 PM Kevin Partington ***@***.***> wrote: Hi @MerlinBB <https://github.com/MerlinBB>, thank you for sharing your detailed research! I don't think the intent of the rule is to look for any constants in the condition. Rather, the goal should be to see if the condition will always result in the same value. Let's take this example: if (void a || a); In this case, void a is a constant in that it will always return undefined. As you probably know, undefined is falsy. In the case of an *or* conditional, that means we have to look at the right side. The right side here is a variable reference, so it's not constant and the rule shouldn't report. However, I would expect if (void a && a); to be invalid, since the undefined value will short-circuit in that case. It seems more likely to be that we need to look at the logic that flags a single given expression as constant and ensure string literals are detected as constants. At this point, based on what you've shown so far, I don't think there's anything wrong with the line that checks the logical expression (and/or) in terms of whether both terms are constant or there's a constant short circuit. Hope this helps! Let me know if I can clarify anything. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWksWybd3xBKXVazEzwNYcuKIG7Ab6ks5u9BtDgaJpZM4ZLqe2> .

--

______________________________ Nicholas C. Zakas @SlickNet Author, Principles of Object-Oriented JavaScript <http://amzn.to/29Pmfrm> Author, Understanding ECMAScript 6 <http://amzn.to/29K1mIy>

[MerlinMason]

Hey guys, thanks for the update...

In the case of an or conditional, that means we have to look at the right side

Yep this makes sense, I think in this case we also need to know if right side is truthy too. The following seems to do the job, does this look right to you?

return (isLeftConstant && isRightConstant) ||
    (node.operator === "||" && isRightConstant && node.right.value) ||
    isLeftShortCircuit ||
    isRightShortCircuit;

[nzakas]

That seems correct to me. I would say that we probably need a comment there to indicate that node.right.value is intentionally testing for a truthy value for clarity, but otherwise, 👍

[MerlinMason]

Awesome! Ok I'll add that and raise a PR.