Skip to content

Conversation

@phaux
Copy link
Contributor

@phaux phaux commented Dec 4, 2020

Added automatic fixes and suggestions to make this rule more easy to use.

I added following fixes and suggestions for particular types in boolean context (also added to docs):

  • boolean - Always allowed - no fix needed.
  • string - (when allowString is false) - Provides following suggestions:
    • Change condition to check string's length (strstr.length > 0)
    • Change condition to check for empty string (strstr !== "")
    • Explicitly cast value to a boolean (strBoolean(str))
  • number - (when allowNumber is false) - Provides following suggestions:
    • Change condition to check for 0 (numnum !== 0)
    • Change condition to check for NaN (num!Number.isNaN(num))
    • Explicitly cast value to a boolean (numBoolean(num))
  • object | null | undefined - (when allowNullableObject is false) - Provides autofix:
    • Change condition to check for null/undefined (maybeObjmaybeObj != null)
  • boolean | null | undefined - Provides autofix:
    • Explicitly treat nullish value the same as false (maybeBoolmaybeBool ?? false)
  • string | null | undefined - Provides following suggestions:
    • Change condition to check for null/undefined (maybeStrmaybeStr != null)
    • Explicitly treat nullish value the same as an empty string (maybeStrmaybeStr ?? "")
    • Explicitly cast value to a boolean (maybeStrBoolean(maybeStr))
  • number | null | undefined - Provides following suggestions:
    • Change condition to check for null/undefined (maybeNummaybeNum != null)
    • Explicitly treat nullish value the same as 0 (maybeNummaybeNum ?? 0)
    • Explicitly cast value to a boolean (maybeNumBoolean(maybeNum))
  • any and unknown - Provides following suggestions:
    • Explicitly cast value to a boolean (valueBoolean(value))

Edit: updated order and messages to match the actual ones


I used the word "explicitly" where it doesn't change the runtime behavior, even when types are wrong.

The autofix for nullable object theoretically changes the behavior (e.g. if you somehow get empty string or 0 instead of null or object), but it's not enabled by default and there's only one obvious way of handling it if enabled. In practice it shouldn't make a difference when you compare object to null vs implicitly cast to boolean.

The second autofix for nullable boolean doesn't change the behavior and it's usually what the programmer wants. That's why I decided to make it automatic instead of suggestion.

I left some TODO comments about suggestions which are wrong for bigint (it's treated the same as number in the rule). They shouldn't cause a huge breakage when applied (in worst case TypeScript should report error after applying the suggestions). I think they can be fixed separately in a followup PR.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @phaux!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #2847 (03fe065) into master (c32f803) will increase coverage by 0.04%.
The diff coverage is 98.11%.

@@            Coverage Diff             @@
##           master    #2847      +/-   ##
==========================================
+ Coverage   92.89%   92.94%   +0.04%     
==========================================
  Files         315      316       +1     
  Lines       10704    10802      +98     
  Branches     3036     3065      +29     
==========================================
+ Hits         9944    10040      +96     
  Misses        344      344              
- Partials      416      418       +2     
Flag Coverage Δ
unittest 92.94% <98.11%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ackages/eslint-plugin/src/util/getWrappingFixer.ts 93.93% <93.93%> (ø)
...int-plugin/src/rules/strict-boolean-expressions.ts 98.70% <100.00%> (+0.81%) ⬆️
.../eslint-plugin/src/rules/member-delimiter-style.ts 94.91% <0.00%> (+0.47%) ⬆️

@bradzacher bradzacher added the enhancement New feature or request label Dec 4, 2020
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic itself LGTM.
One thing that needs to be addressed


I agree with all of the fixes except for the autofix for nullable boolean.

Explicitly treat nullish value the same as false (maybeBool → maybeBool ?? false)

two reasons:

  1. I think that it's a stylistic choice here and a lot of people won't like the autofix to ??.
  2. (the big one which blocks this) Not everyone is on TS 3.7, which would make this a breaking change because we'd be autofixing to unsupported syntax and break their code.

Suggested solution:
make it a suggestion fixer with the following choices:

  • maybeBool ?? false
  • maybeBool === true

Side note for a (potential) future enhancement (not saying lets do it now - god no! - just saying someone might like it in the future).

Additional fixer for the verbose and explicit:
x !== null && x !== undefined
and the super verbose but super explicit and always safe:
x !== null && typeof x !== 'undefined'

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Dec 21, 2020
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 28, 2021
@phaux
Copy link
Contributor Author

phaux commented Feb 28, 2021

Done!

I changed the suggestions for nullable boolean as you suggested.

I also added a special case for number in boolean context when the number is array.length in which case it provides autofix instead of suggestion.

In addition, I moved getWrappingFixer to utils because it might be useful in other rules.

Then I also fixed a bug with getWrappingFixer so that it adds a semicolon if it detects that it inserted parens at the beginning of an ExpressionStatement.

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much for your work on this!
This is a really valuable set of changes.

This was referenced Mar 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants