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): add rule no-confusing-void-expression #2605

Merged
merged 26 commits into from Nov 2, 2020
Merged

feat(eslint-plugin): add rule no-confusing-void-expression #2605

merged 26 commits into from Nov 2, 2020

Conversation

phaux
Copy link
Contributor

@phaux phaux commented Sep 27, 2020

I used this rule with TSLint a lot so it makes sense that I'm responsible for porting it. I ported it from TSLint and then added some features:

  • More descriptive messages for the common cases.
    These cases are: returning void from arrow function shorthand; and early return with void function from other void function.
  • Provides autofix for the common cases.
    These usually aren't mistakes, but only confusing code written by developers who themselves know what they're doing.
  • Ignores only the RHS of the short-circuiting operators, instead of both operands.
    It only makes sense to put the side effect code into the right-hand side of &&, ||, ?? or ternary operator.
    That's why I decided to only allow void expressions in the RHS.
  • Rename ignore-arrow-function-shorthand option to ignoreArrowShorthand.
    It's less verbose but still obvious enough and it's camelCase.
  • Extra option: ignoreVoidOperator similar to no-misused-promises ignoreVoid option.
    Allow silencing the error by using the void operator and make it explicit in the code that you actually want this behavior.

Fixes #2425

@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.

@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Sep 28, 2020
Copy link
Contributor

@tadhgmister tadhgmister left a comment

Choose a reason for hiding this comment

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

The fact that you violate this rule with it's own implementation is very silly, do you intent to leave it like that?

packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #2605 into master will increase coverage by 0.04%.
The diff coverage is 97.89%.

@@            Coverage Diff             @@
##           master    #2605      +/-   ##
==========================================
+ Coverage   92.75%   92.79%   +0.04%     
==========================================
  Files         296      297       +1     
  Lines        9739     9833      +94     
  Branches     2733     2762      +29     
==========================================
+ Hits         9033     9125      +92     
  Misses        332      332              
- Partials      374      376       +2     
Flag Coverage Δ
unittest 92.79% <97.89%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
packages/eslint-plugin/src/configs/all.ts 100.00% <ø> (ø)
...lugin/src/rules/consistent-indexed-object-style.ts 88.88% <80.00%> (-1.36%) ⬇️
...t-plugin/src/rules/no-confusing-void-expression.ts 98.82% <98.82%> (ø)
packages/eslint-plugin/src/rules/no-shadow.ts 94.04% <100.00%> (+0.37%) ⬆️

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.

Don't forget to update the ROADMAP appropriately


Do we want to rename the rule?
IMO no-void-expression is super confusing. To the uninitiated it sounds like it might be banning the void operator.

Possible alternatives:

  • no-void-assignment (my preference)
  • no-void-return (probably too specific)
  • no-void-usage (a little bit vague and confusing)

packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-void-expression.md Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-void-expression.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Oct 4, 2020
@phaux
Copy link
Contributor Author

phaux commented Oct 4, 2020

Updated ROADMAP and left the rule name as is for now. I don't have a strong preference about the name.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 5, 2020
@bradzacher bradzacher changed the title feat(eslint-plugin): new rule: no-void-expression feat(eslint-plugin): add rule no-void-expression Oct 25, 2020
@bradzacher
Copy link
Member

The code itself LGTM - I think the name should be changed to be clearer.

I'm okay with any of these two names:

  • no-void-assignment
  • no-void-type-assignment
  • no-void-typed-assignment

Change the name and fix the merge conflict and we can merge this!
Thanks for your work

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Oct 25, 2020
@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed awaiting response Issues waiting for a reply from the OP or another party labels Nov 1, 2020
@phaux
Copy link
Contributor Author

phaux commented Nov 1, 2020

Before I change the name I'd like to propose these names as well. From my understanding the problem with the current name is that it can be interpreted as "ban the void operator" or "ban everything that evaluates to void". So how about:

  • no-undefined-expression - makes it clear that it's not about the void operator.
  • restrict-void-expressions - doesn't tell much, but at least it's not misleading. (I vote for this one)
  • require-void-expression-as-statement - very verbose but it's literally part of the rule's description and it's obvious.

@bradzacher
Copy link
Member

no-undefined-expression

This one isn't quite right, because undefined and void are different.


expression is somewhat broad because this rule is only checking the results function-like calls.
I.e.

declare const x: {
    a: void
};
const y = x.a; // this will be ignored by the rule

It does leave us room to expand in future though.


This is ultimately bike-shedding. I don't have that much of an opinion!

I'm okay with any name you choose (except for the original no-void-expressions 😄 )
Choose whatever you like, and we're good to go!

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.

LGTM - Thanks for your work as always!

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Nov 2, 2020
@bradzacher bradzacher merged commit c8a4dad into typescript-eslint:master Nov 2, 2020
@glen-84
Copy link
Contributor

glen-84 commented Nov 10, 2020

@JamesHenry,

The title of this PR was not updated to reflect the new rule name (no-confusing-void-expression), so the release notes refer to this incorrect name.

@bradzacher bradzacher changed the title feat(eslint-plugin): add rule no-void-expression feat(eslint-plugin): add rule no-confusing-void-expression Nov 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Disallow returning a void-returning function result
4 participants