Enhancement: [prefer-nullish-coalescing] should ignore Boolean constructor

[Mister-Hope]

Before You File a Bug Report Please Confirm You Have Done The Following...

• [X] I have tried restarting my IDE and the issue persists.
• [X] I have updated to the latest version of the packages.
• [X] I have searched for related issues and found none that matched my issue.
• [X] I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.4.3&fileType=.ts&code=DYUwLgBAhgXBDOYBOBLAdgcwgHwsgriDhPmgCYgBm6IZA3ALABQokARnIqpsWwPZ9QUNMVIVqaWoxbgIAYzj9BIYaPJUa9ZswD0OvCEQQAjBAAWIJEQC2KtPAR9bEPpTwXobORBQOCYMwBPZjk%2Be0gwQzBTAF4IACEBITQACigcXDYM%2BQBKaVDwg0QAJgg4xOVhNJzsiuSUthrsXDq7FLkc7SYC%2BGUAOmA%2BDBTIxGM85iA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WlqYSNkAC1pkA9gEMkyMswDm6MAG1w2HAHdJ0JpAA0a9VnXZIleU3GcAwuKYATSvkp3pAFRT5UGfHESHsAF8AgF01YMCgA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

let a: string | true | undefined;
let b: string | boolean | undefined;
let c: boolean | undefined;

// test 1 here means some of the abc is truthy
const test1 = Boolean(a || b || c);
const test2 = Boolean(a) || Boolean(b) || Boolean(c)

console.log(test1);

ESLint Config

{
  "rules": {
    "@typescript-eslint/prefer-nullish-coalescing": [
      "warn",
      {
        "ignoreConditionalTests": true
      }
    ]
  }
}

tsconfig

{
  "compilerOptions": {
    "strictNullChecks": true
  }
}

Expected Result

Eslint should not try to fix the code, as this is a correct usage.

Actual Result

The fix is invalid, as when:

a = ''
b = 'something'

The fix result Boolean((a ?? b) ?? c) is false, while the snippet is meant to be true.

I understand that I can write the way in test2, but it will have performance issues that incease code exec steps, and making the output bigger. The using of Boolean is that we usually need a clean state type with boolean to use it in other places (e.g.: Array.filter)

Additional Info

IMO, when setting ignoreConditionalTests: true, the Boolean constructor and !(/* codes */) should also be checked. This means users are wanting to have a full truthy check on all of them.

I have encounted 20+ times with similar issues in my code repo, as it's really common to get a state which is "some of the conditions are truthy"

[bradzacher]

and !(/* codes */) should also be checked

The rule will ignore the not operator if its in the context of a conditional test. Otherwise it will intentionally ignore it. I firmly disagree that this should apply to a general "not" expression across the codebase.

---

As for the Boolean constructor - I can see an argument for it. People have asked for it in other rules.

Currently none of our rules consider it as a boolean/conditional context - usually intentionally so because it doesn't align with the rule's intent. In this instance I can see the draw behind it as a general-purpose opt-out from the rule's checks. Probably good value as another option.

[Mister-Hope]

Yes, for the not operator, we can omit it as we can definitely use a not operator on a boolean constructor to bypass it.

Thanks for supporting this request, As is really annoying that multiple bugs are introduced in my repo by simply turning on this rule and use the auto fix future

[Mister-Hope]

@bradzacher Still need a further confirm, I notice you renamed this as enhancement, but I refuse for this, as the auto-fix has introduce false positive into codes.

I would rather treat this as a fix, and I believe the new option should be default on. We should be extremely careful that an auto fix can introduce false positive

[bradzacher]

The rule intentionally did not treat Boolean as a conditional context. It is not a bug that it did not.

Adding support to treat it as a special case is a new feature and likely will be behind its own option.

Based on anecdotal experience your codebase's treatment is a rarer case.

Regardless - the tagging doesn't matter - that really just determines if it is a patch or a minor release when it merges. It doesn't change the priority - either way this will be waiting for a community member to action it.