Add ignore-rhs option for strict-boolean-expressions

[dobesv]

PR checklist

• Addresses an existing issue: strict-boolean-expressions should ignore RHS of && and || #4158
• New feature, bugfix, or enhancement
  • Includes tests
• Documentation update

Overview of change:

If specified, the right-hand operand of && and || will not be checked
as a strict boolean expression.

CHANGELOG.md entry:

[new-rule-option] strict-boolean-expressions accepts ignore-rhs option to disable checking the right-hand side of the && and || operators as strictly boolean.

[palantirtech]

Thanks for your interest in palantir/tslint, @dobesv! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[dobesv]

Is there anything else I need to (or can) do for this PR to move forward?

[krishan]

I would love for #4158 to get solved, as it's blocking me from using strict-boolean-expresssions in my project.

So I'm wondering, what's up with this PR?
What would be needed to move this forward?

[Kinrany]

Has @palantirtech already updated this PR, or is it still waiting for the contributor license agreement to be signed?

[dobesv]

It's just waiting for a team member review, I guess. I haven't heard any feedback about it and I signed the CLA, so assuming I read it right it's just a matter of someone with write access reviewing and approving it.

[dobesv]

Fixes #3279

[dobesv]

@JoshuaKGoldberg can you review this one?

[JoshuaKGoldberg]

Will do, soon!

Note: I'm new to reviewing & maintaining TSLint, so I won't merge it in soon if it passes review. I'll just leave comments for a more experienced maintainer to double-check. Same with other PRs.

[rhys-vdw]

Any movement on this one?

[JoshuaKGoldberg]

@rhys-vdw unfortunately no 🙁. The Palantir folks are looking for someone to maintain TSLint full time, but until then there's no SLA on PR reviews.

[adidahiya]

I think the goal here is good and this PR is well-implemented, but unfortunately this is a breaking change which should be introduced in the next major version. Can you please retain the existing behavior by default and instead introduce an option called "ignore-rhs" which addresses #4158? After this merges, I can create a tracking issue for making "ignore-rhs" the default behavior in v6.0

[dobesv]

@adidahiya I reversed the option as requested. Let me know if there's anything else I can do.

[krishan]

Thank you, looking forward to using this 🎉

[jmoseley]

How/when does this change get published?

[lukewlms]

@dobesv This is useful, thank you! The ideal for us would be to still disallow RHS non-boolean items in places that require a boolean (e.g. ternary, if/else...) while allowing RHS non-boolean in any other locations (particularly React rendering - someBool && ). Do you think this is a common enough case to address?

[dobesv]

@lukewlms Are you trying to say that you want the rhs to be forced as a boolean in case like:

if(x && 1) return;  // <-- error: 1 is not a boolean

but still ignored in other expressions, like:

const whatever = { b: (Date.now() > 5000) && 1 }; // <-- OK because the surrounding expression is not a conditional

My change only ignores the rhs in the sense that it does not complain if the rhs or && or || is not a boolean. However, the overall expression itself would still not be a boolean, and it's likely an existing rule requiring the condition of if() or the ternary operator would still report a problem independantly of this change.

Have you verified that your case isn't already covered by the rule as it is ?

[lukewlms]

@dobesv Yes, checked it - here's an example. I'd like the first two to get flagged but the third to not be flagged - so in places where we absolutely must have a boolean, we're sure to get a strict one.

const func = (b: boolean, s: string) => {
  // Gets flagged with ignore-rhs on
  if (s) {
    // ...
  }

  // Does not get flagged with ignore-rhs on, but I'd like it to!
  if (b || s) {
    // ...
  }

  // This does not get flagged and that's good!
  return b || s;
}

[dobesv]

I see. That does seem like a bug or oversight. I'd suggest filling a new issue or a PR for it, since this one is closed.

[adidahiya]

I don't think we reached a consensus on whether boolean-coerced expressions in conditional statements should be treated differently from expressions in other positions in #3279. So I don't know if it's a "bug" per se. Please do file a new issue if you'd like to keep discussing your proposed rule behavior change.

[lukewlms]

Logged #4671 for addressing ignoring RHS only when a boolean value is not required.