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

feat(eslint-plugin): [strict-boolean-expressions] refactor, add clearer error messages #1480

Merged
merged 11 commits into from Feb 12, 2020

Conversation

@phaux
Copy link
Contributor

phaux commented Jan 20, 2020

  • Added more error messages for each case with helpful tips.
  • Added tests and fixed #1118

The positions of report messages also changed as a side effect of different logic for handling nested logical expressions. This was necessary to fix #1118

@typescript-eslint

This comment has been minimized.

Copy link

typescript-eslint bot commented Jan 20, 2020

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.

@phaux phaux force-pushed the phaux:strict-bool-expr-refactor branch from ec2dc74 to c22b570 Jan 20, 2020
@phaux

This comment has been minimized.

Copy link
Contributor Author

phaux commented Jan 22, 2020

BTW I also noticed that allowSafe option doesn't do what I thought it does at all. I thought it would allow exactly TruthyType | FalsyType (so e.g. object | function | symbol | null | undefined), but it actually doesn't do that – it allows TruthyType | boolean so it's only useful with allowNullable, but even then it's too lenient because then TruthyType | boolean | FalsyType is also okay.

I want to add an option for this in a followup PR and a few others for cases that actually happen often in the wild and could be considered annoying and safe to allow via option.

@glen-84

This comment has been minimized.

Copy link

glen-84 commented Jan 24, 2020

That PR mentioned:

Note that setting allowNullable & allowSafe to true allows boolean | null as a boolean expression, which is not safe.

... it would be great if that was disallowed.

@phaux

This comment has been minimized.

Copy link
Contributor Author

phaux commented Jan 24, 2020

It's now trivial to have separate logic for each combination of types. The only question is whether we want a breaking change here instead of creating a new option that does this right.

Here are the cases that I encounter often and could have an option to allow them:

  • boolean – always allowed
  • boolean | null | undefined aka optional/nullable boolean – developers often want to treat the nullish case the same as false instead of explicitly saying value ?? false or value == null ? false : value
    • allowed by allowNullable
  • string | number – developers often want to test for zero/empty string without explicit comparison value != 0 etc.
    • would be allowed by e.g. allowPrimitive or something
  • object | function | null | undefined aka nullable object/function – developers often check against the nullish case without explicitly comparing to null. I've met a lot of people that don't even know that value == null also checks for undefined.
    • would be allowed by allowSafe if we can fix it, or allowNullableObject or something
@bradzacher

This comment has been minimized.

Copy link
Member

bradzacher commented Jan 24, 2020

Just to keep this PR focused, I've spun up #1515 to discuss new rule options

@phaux

This comment has been minimized.

Copy link
Contributor Author

phaux commented Jan 26, 2020

I changed the check for a primitive type so that the string and number cases are handled separately.

I also improved the handling of nested logical operators so that it now recurses over the operands all the way instead of checking only one level deep. This prevents double-reporting the whole expression and one of its operands at once, no matter how deeply nested they are.

I think I'm done with this PR 👌

@phaux phaux requested a review from bradzacher Jan 26, 2020
@bradzacher bradzacher added the refactor label Jan 30, 2020
Copy link
Member

bradzacher left a comment

question:
How does this rule handle never types?
It looks like inspectVariantTypes will not handle it.


Mostly LGTM. A few comments.
Thanks for your work here

@phaux phaux force-pushed the phaux:strict-bool-expr-refactor branch from 5fdee51 to e296ac0 Feb 1, 2020
@phaux

This comment has been minimized.

Copy link
Contributor Author

phaux commented Feb 1, 2020

Improved code style and made it so that the never type is always allowed 😎

@phaux phaux requested a review from bradzacher Feb 1, 2020
Copy link
Member

bradzacher left a comment

LGTM - thanks for your work here

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 12, 2020

Codecov Report

Merging #1480 into master will increase coverage by 0.07%.
The diff coverage is 98.65%.

@@            Coverage Diff             @@
##           master    #1480      +/-   ##
==========================================
+ Coverage   95.47%   95.54%   +0.07%     
==========================================
  Files         140      142       +2     
  Lines        6449     6579     +130     
  Branches     1851     1880      +29     
==========================================
+ Hits         6157     6286     +129     
+ Misses        107      106       -1     
- Partials      185      187       +2
Impacted Files Coverage Δ
...estree/src/create-program/createIsolatedProgram.ts 75% <ø> (ø) ⬆️
...plugin/src/rules/explicit-module-boundary-types.ts 85.71% <100%> (+0.25%) ⬆️
...pt-estree/src/create-program/createWatchProgram.ts 92.39% <100%> (+0.04%) ⬆️
...es/eslint-plugin/src/rules/no-floating-promises.ts 100% <100%> (ø) ⬆️
...s/eslint-plugin/src/rules/no-dupe-class-members.ts 100% <100%> (ø)
...-estree/src/create-program/createDefaultProgram.ts 76.19% <100%> (ø) ⬆️
...nt-plugin/src/rules/switch-exhaustiveness-check.ts 100% <100%> (ø)
.../eslint-plugin/src/util/explicitReturnTypeUtils.ts 100% <100%> (ø) ⬆️
packages/eslint-plugin/src/rules/unbound-method.ts 96.66% <100%> (+2.22%) ⬆️
...-estree/src/create-program/createProjectProgram.ts 92.85% <85.71%> (-1.43%) ⬇️
... and 5 more
@bradzacher bradzacher changed the title feat(eslint-plugin): refactor [strict-boolean-expressions] feat(eslint-plugin): [strict-boolean-expressions] refactor, add clearer error messages Feb 12, 2020
@bradzacher bradzacher merged commit db4b530 into typescript-eslint:master Feb 12, 2020
7 checks passed
7 checks passed
Semantic Pull Request ready to be squashed
Details
codecov/patch 98.65% of diff hit (target 90%)
Details
codecov/project 95.54% (+0.07%) compared to 9dcd5be
Details
typescript-eslint.typescript-eslint Build #20200212.2 succeeded
Details
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.