Bug: False positives with no-constant-condition

[captbaritone]

Environment

Online Playground

What parser are you using?

Default (Espree)

What did you do?

if(`${[...a]}`) {
  //
};

if(`${[{toString: () => a}]}`) {
  //
};


if("" + {toString: () => a}) {
  //
}

if("" + [{toString: () => a}]) {
  //
};

if(+[{toString: () => a}]) {
  //
};

Playground Link

What did you expect to happen?

No errors

What actually happened?

Each condition triggers no-constant-condition

Participation

• I am willing to submit a pull request for this issue.

Additional comments

I was looking into the implementation of the isConstant function in no-constant-condition as part of #15296. Specifically I was trying to understand the meaning of the case where the inBooleanPosition flag is set to false.

I realized that the reason I was having such a hard time understanding it is that it's not really sound. For example, we call isConstant with inBooleanPosition = false in cases where values are going to get coerced into numbers or strings and we want to know if the resulting string/number will have constant truthiness. However, the checks are not sufficient to actually validate that.

I think that currently isConstant with inBooleanPosition set to false is supposed to check if a value has constant truthiness, even when that value is being first coerced to some other type (string/number/...). I'm not sure that's a function which is feasible to write correctly. Instead, I suspect we will want individual functions that handle the string and number cases, or something along those lines.

Rather than trying to solve this by adding more special cases to isConstant (which is already rather hard to grock), I wonder if we could break it up into a collection of simpler helper functions which check for more explicit cases.

[nzakas]

Can you explain why you think the current behavior is incorrect for the cases you provided? It’s not immediately clear to me.

[captbaritone]

None of these conditions are constant. They can all vary based on the value of a:

// The condition will be true if `a = [1]` but false if `a = []`
if(`${[...a]}`) {
  //
};

// The condition will be true if `a = "1"` but false if `a = ""`
if(`${[{toString: () => a}]}`) {
  //
};

// The condition will be true if `a = "1"` but false if `a = ""`
if("" + {toString: () => a}) {
  //
}

// The condition will be true if `a = "1"` but false if `a = ""`
if("" + [{toString: () => a}]) {
  //
};

// The condition will be true if `a = "1"` but false if `a = "0"`  or `a = ""`
if(+[{toString: () => a}]) {
  //
};

[aladdin-add]

seems a bug to me - it should not warn as eslint does not know its value.

[nzakas]

Got it. Thanks. This does look like a bug, though I’m not sure the toString override cases are things we should endeavor to catch. Are you picturing that we would just omit any template string with an inline variable reference?

[captbaritone]

(Link to isConstant for context.)

There are three immediate issues that I can see:

1. When we recurse into expressions when handling TemplateLiteral, we should at least always be passing inBooleanPosition as false since we are not concerned with truthiness or falsyness of the sub-expression, but rather whether it might conditionally coerce to an empty or non-empty string.
2. When handling an ArrayExpression and inBooleanPosition is false, we should require that all elements be constant no matter what the parent node is. (This would also catch the case where the only element is a SpreadExpression)
3. When handling an ObjectExpression when inBooleanPosition is false, we should return false if the object defines valueOf or toString (or contains a spread).

I think issue underlying numbers 2 and 3 is the fact that calling isConstant with inBooleanPosition is poorly defined.

To help remedy this, I'd like to propose a more precise definition:

For both string and number, if coerced to that type, the value will be constant.

I believe if we adopt this definition, and then ensured the function conforms to it, isConstant can be made sound.

[nzakas]

That sounds like a reasonable approach to me. I’d be interested if there would be any effects on other rules where isConstant is being used.

[captbaritone]

isConstant is not currently used by any other rules,

[captbaritone]

isConstant is not currently used by any other rules, but we were considering pulling it out into a utility function for use in no-constant-binary-expression, which is what lead me down the path of trying to better define its behavior.

[nzakas]

Ah ok. Sounds like a good plan to me.

[captbaritone]

I've opened a PR here: #15486