🐛 Unintended behaviour for `js/useSimplifiedLogicExpression`

[Boshen]

Environment information

Playground

What happened?

These expressions should not trigger js/useSimplifiedLogicExpression

let a = b || false
let c = d || true

warning[[js/useSimplifiedLogicExpression](https://rome.tools/docs/lint/rules/useSimplifiedLogicExpression/)]: Logical expression contains unnecessary complexity.
  ┌─ main.js:1:9
  │
1 │ let a = b || false
  │         ----------

Suggested fix: Discard redundant terms from the logical expression.
    | @@ -1,2 +1,2 @@
0   | - let a = b || false
  0 | + let a = b
1 1 |   let c = d || true

warning[[js/useSimplifiedLogicExpression](https://rome.tools/docs/lint/rules/useSimplifiedLogicExpression/)]: Logical expression contains unnecessary complexity.
  ┌─ main.js:2:9
  │
2 │ let c = d || true
  │         ---------

Suggested fix: Discard redundant terms from the logical expression.
    | @@ -1,2 +1,2 @@
0 0 |   let a = b || false
1   | - let c = d || true
  1 | + let c = true                

Expected result

These expressions should not trigger js/useSimplifiedLogicExpression

Code of Conduct

• [X] I agree to follow Rome's Code of Conduct

[Boshen]

Found another test case

!a || !b || !c || !d

warning[[js/useSimplifiedLogicExpression](https://rome.tools/docs/lint/rules/useSimplifiedLogicExpression/)]: Logical expression contains unnecessary complexity.
  ┌─ main.js:1:1
  │
1 │ !a || !b || !c || !d
  │ --------

Suggested fix: Reduce the complexity of the logical expression.
    | @@ -1 +1 @@
0   | - !a || !b || !c || !d
  0 | + !(a && b ) || !c || !d

I'm not expecting this code to trigger a warning.

[leops]

There are a few things to unpack here:

• First the code actions being emitted here are marked as "Suggested fix" and not "Safe fix", this is something we need to improve the documentation for but it basically means applying the fix may have unintended consequences on the code: in this case it could change the type of the expression if the left hand side of the expression is nullable, or change the behavior of the program if the removed sub-expression has side-effects
• The linter is correct in emitting diagnostics for these expressions, however the explanations surrounding it could be improved: even if the intended goal of the expression is to provide a default value for a nullable expression, the ?? operator should be used instead
• For the final case the diagnostics is also correct but is not emitted on the correct expression: it should rewrite the entire or-chain into !(a && b && c && d)

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity.

[ematipico]

@leops could you advice about what we should do with this issue?

[leops]

@leops could you advice about what we should do with this issue?

From what I mentioned in my previous comment, I think the general longstanding issue we've had with the "Safe Fix" / "Suggested Fix" confusion and the specific issue with the explanations for this diagnostic are tasks that are part of the ongoing diagnostics refactor. For the last part I think the simplify_de_morgan logic in this rule should either be improved to process entire chains of binary expressions, or as a last resort we could disable this logic entirely

[github-actions[bot]]

This issue is stale because it has been open 14 days with no activity.