Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
4 tasks done
Mister-Hope opened this issue May 12, 2024 · 4 comments · Fixed by #9924
Closed
4 tasks done

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

Mister-Hope opened this issue May 12, 2024 · 4 comments · Fixed by #9924
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@Mister-Hope
Copy link
Contributor

Mister-Hope commented May 12, 2024 •
edited
Loading

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

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • 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"
@Mister-Hope Mister-Hope added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels May 12, 2024
@Mister-Hope Mister-Hope changed the title Bug: [prefer-nullish-coalescing] should ignore boolean Bug: [prefer-nullish-coalescing] should ignore Boolean and ! May 12, 2024
@bradzacher
Copy link
Member

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.

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule accepting prs Go ahead, send a pull request that resolves this issue and removed bug Something isn't working triage Waiting for team members to take a look labels May 12, 2024
@bradzacher bradzacher changed the title Bug: [prefer-nullish-coalescing] should ignore Boolean and ! Enhancement: [prefer-nullish-coalescing] should ignore Boolean and ! May 12, 2024
@bradzacher bradzacher changed the title Enhancement: [prefer-nullish-coalescing] should ignore Boolean and ! Enhancement: [prefer-nullish-coalescing] should ignore Boolean May 12, 2024
@bradzacher bradzacher changed the title Enhancement: [prefer-nullish-coalescing] should ignore Boolean Enhancement: [prefer-nullish-coalescing] should ignore Boolean constructor May 12, 2024
@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented May 13, 2024 •
edited
Loading

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
Copy link
Contributor Author

@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
Copy link
Member

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.

@developer-bandi developer-bandi mentioned this issue Sep 3, 2024
Merged
3 tasks
@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Nov 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
2 participants
@bradzacher @Mister-Hope