-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [strict-bool-expr] add fixes and suggestions #2847
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
feat(eslint-plugin): [strict-bool-expr] add fixes and suggestions #2847
Conversation
|
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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
bradzacher
left a comment
There was a problem hiding this 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:
- I think that it's a stylistic choice here and a lot of people won't like the autofix to
??. - (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 ?? falsemaybeBool === 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'
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
|
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 In addition, I moved Then I also fixed a bug with |
bradzacher
left a comment
There was a problem hiding this 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.
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- (whenallowStringisfalse) - Provides following suggestions:str→str.length > 0)str→str !== "")str→Boolean(str))number- (whenallowNumberisfalse) - Provides following suggestions:num→num !== 0)num→!Number.isNaN(num))num→Boolean(num))object | null | undefined- (whenallowNullableObjectisfalse) - Provides autofix:maybeObj→maybeObj != null)boolean | null | undefined- Provides autofix:maybeBool→maybeBool ?? false)string | null | undefined- Provides following suggestions:maybeStr→maybeStr != null)maybeStr→maybeStr ?? "")maybeStr→Boolean(maybeStr))number | null | undefined- Provides following suggestions:maybeNum→maybeNum != null)maybeNum→maybeNum ?? 0)maybeNum→Boolean(maybeNum))anyandunknown- Provides following suggestions:value→Boolean(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.