Skip to content
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.

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

Enchancement: [strict-boolean-expressions] Treat !!x same as Boolean(x) #9060

Closed
4 tasks done
Faithfinder opened this issue May 7, 2024 · 6 comments
Closed
4 tasks done
Labels
bug Something isn't working duplicate This issue or pull request already exists package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@Faithfinder
Copy link
Contributor

Faithfinder commented May 7, 2024

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=.tsx&code=GYVwdgxgLglg9mABACwKYBt1wBQFsCeAbgIYBOAXIgM5SkxgDmiAPomCJgJSIDeAUIkQxgibAEIxBEqW49EAXz6KgA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6ZfKsugIwHs%2BSAIZN6AD2LQUySnyaoMkRNGh9okcGAC%2BILUA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

function hello(myvar: string | null) {
  if (!!myvar) { }
}

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/strict-boolean-expressions": ["error"],
  },
};

Additional Info

Not strictly a bug, but IMO this is a valid, less verbose way to explicitly convert things to boolean.

@Faithfinder Faithfinder added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 7, 2024
@auvred
Copy link
Member

auvred commented May 7, 2024

Duplicate of #7444, #3470

@auvred auvred closed this as not planned Won't fix, can't repro, duplicate, stale May 7, 2024
@auvred auvred added duplicate This issue or pull request already exists wontfix This will not be worked on and removed triage Waiting for maintainers to take a look labels May 7, 2024
@Faithfinder
Copy link
Contributor Author

Weird, I searched by rule name and found nothing, sorry.

Still, in #7444 the author's point isn't addressed and I'd like to second it. Boolean() is in the proposed fixes list, and !! is equivalent. Either remove Boolean() as a solution, or allow !!?

#7444 (comment)

@bradzacher
Copy link
Member

My question for you would be:
If you want to allow weak, unstrict boolean expressions then why aren't you using the rule with allowNullableString et al?

playground

The main reasons that I think this goes against the rule is because it's (a) an uncommon syntax in modern TS and (b) unclear and easy to miss as a choice of syntax - it's two characters (c) it's a loose coercion where you're not strictly and explicitly handling the falsey cases.

But ultimately the rule is configurable. But if you want to ban nullable strings and allow exceptions then you're in a small minority of users that were not actively looking to support. Realistically you can and should use a disable comment.

As I mentioned in the other issue - IMO we should really be banning Boolean() behind an option. The reason we didn't originally is because it is a very loud and explicit way of opting out if the rule (as in "this expression right here is being coerced") that is nicer than a disable comment.

@Faithfinder
Copy link
Contributor Author

Faithfinder commented May 8, 2024

I'm still getting a feel for the rule. I'm in a new job and there was neither linting, nor Typescript here before me. Previously I've used mostly the recommended config, and learned that some fairly useful and obvious things are missing from it, so this time I've explored every single rule in eslint, typescript-eslint and react plugins, enabling those that looked reasonable to me. I've hit this one in the first file conversion to Typescript I've made. In that case, I've opted into doing a !== null check instead of !!. I'm not sure I won't disable this rule, or relax it as much as possible in the future.

Still, IMO, allowing Boolean() as a bypass, and forbidding !! is just weird. Now, I don't know if what you're saying about it being uncommon syntax is true, but it was widely used by my previous team, there's 119 hits in the JS codebase of my current one, and 56 on a naive search of this repo right here (vs just 19 hits on Boolean(). It very clearly tells me this is an explicit coercion.

I don't really care whether you remove Boolean bypass or allow !! though, I do strongly believe they should be treated the same.

@bradzacher
Copy link
Member

bradzacher commented May 8, 2024

Sorry - I should clarify.
"In codebases that use strict-boolean-expressions usage of !! to loosely coerce a value to a boolean is uncommon"

The syntax itself is reasonably common but when people opt-in to the SBE rule they explicitly want to NOT have weak coercion of values - thus it's rare that they use the rule and also want to allow the syntax.

Still, IMO, allowing Boolean() as a bypass, and forbidding !! is just weird.

There are multiple reasons that it works this way.

The first (and main) reason is that the argument passed to Boolean() is not a boolean context - it's a function that accepts any and returns a boolean. So it's not explicitly handled by any of the rule's logic by default as the rule was designed to inspect boolean contexts aka logical expressions.

OTOH !value is a logical expression that the rule checks intentional and by design. So following from that !!value is also checked by design as !!value is no different to !value from the rule's perspective.

The reason we haven't made a change to specifically allow the !! case is as mentioned - it's very uncommon people actually want to allow it when using the rule - as evidenced by the fact that we've only had a few requests over the years.

The difference is the verbosity of it.
!!value
vs
Boolean(value)

The former is very easy to misread as !value and miss the fact that it's in-fact doing a weak coercion.

I could see the possibility of an option for it - but as per the above - I don't think it's broadly applicable enough for us to move and implement it in our project.

@Faithfinder
Copy link
Contributor Author

I see. Well, thank you for the explanation, it's very nice of you to go into such detail :) I still think they should match regardless, but at least it doesn't look like a missed case anymore. Nothing more I can say, so, see you around :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working duplicate This issue or pull request already exists package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants