feat(eslint-plugin): [no-confusing-void-expression] add an option to ignore void<->void #10067

ronami · 2024-09-27T16:39:41Z

PR Checklist

Addresses an existing open issue: fixes Enhancement: [no-confusing-void-expression] Add option to ignore void<->void #8538
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR addresses #8538 and adds an option (namely ignoreVoidReturningFunctions) that allows returning void expressions from either:

Functions explicitly annotated with void as one of their return types.
Their contextual type includes void.

I enabled the rule back on @typescript-eslint's monorepo (since it's currently disabled) with the option and had to fix one error that looks valid. Running the rule on @typescript-eslint's monorepo helped me figure out the various edge cases of the rule (hopefully, I didn't miss a whole lot).

I added a "game plan" comment after seeing @kirkwaiblinger do this on several other rules, and it has been helpful to read through.

One open question I have is whether or not this should apply to async functions. I mean, if the following should be valid or not:

function returnsVoid(): void {}

async function foo(): Promise<void> {
  return returnsVoid();
}

Please let me know if this makes sense or if I missed anything.

Edit: Only when I finished most of the work I noticed the linked PR of #8632. I noticed I was missing a bunch of tests and added the ones I was missing, thanks @developer-bandi 🙏

typescript-eslint · 2024-09-27T16:39:44Z

Thanks for the PR, @ronami!

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.

netlify · 2024-09-27T16:39:57Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`8b85c14`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/670a15a2d4fac700082ea430
😎 Deploy Preview	https://deploy-preview-10067--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 94 (no change from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-09-27T16:40:40Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8b85c14. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

codecov · 2024-09-27T16:56:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.12%. Comparing base (974d9ce) to head (8b85c14).
Report is 80 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10067      +/-   ##
==========================================
+ Coverage   86.09%   86.12%   +0.02%     
==========================================
  Files         428      429       +1     
  Lines       14969    14995      +26     
  Branches     4343     4351       +8     
==========================================
+ Hits        12888    12914      +26     
  Misses       1734     1734              
  Partials      347      347

Flag	Coverage Δ
unittest	`86.12% <100.00%> (+0.02%)`	⬆️

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

Files with missing lines	Coverage Δ
...t-plugin/src/rules/no-confusing-void-expression.ts	`100.00% <100.00%> (ø)`
...ckages/eslint-plugin/src/rules/no-unsafe-return.ts	`97.22% <100.00%> (-0.22%)`	⬇️
...es/eslint-plugin/src/util/getParentFunctionNode.ts	`100.00% <100.00%> (ø)`

JoshuaKGoldberg

Looks like a great start to me! Mostly requesting changes on docs & testing, the game plan seems very reasonable. 🏈

packages/eslint-plugin/docs/rules/no-confusing-void-expression.mdx

JoshuaKGoldberg · 2024-10-10T20:27:44Z

packages/eslint-plugin/src/util/getParentFunctionNode.ts

[Praise] I like the reuse here 🙂 nice.

JoshuaKGoldberg · 2024-10-10T20:33:09Z

packages/eslint-plugin/src/rules/no-confusing-void-expression.ts

+      if (
+        ts.isFunctionExpression(functionTSNode) ||
+        ts.isArrowFunction(functionTSNode)
+      ) {


[Style] Nit: a little less code to read & run here, as getContextualType is just looking for expressions:

Suggested change

if (

ts.isFunctionExpression(functionTSNode) ||

ts.isArrowFunction(functionTSNode)

) {

if (ts.isExpression(functionTSNode)) {

I've seen this done here. Would it make sense to send a tiny PR to shorten it in a similar way? Looks like it works out locally.

If you want, I'd 👍 it!

packages/eslint-plugin/src/rules/no-confusing-void-expression.ts

….mdx Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

JoshuaKGoldberg · 2024-10-21T14:49:11Z

@ronami is this ready for re-review?

We normally wait to be explicitly re-requested, so we're holding off posting more comments for now.

ronami · 2024-10-21T14:59:57Z

@ronami is this ready for re-review?

We normally wait to be explicitly re-requested, so we're holding off posting more comments for now.

Oh, I wasn't aware of this. Yes! It is ready for review.

JoshuaKGoldberg · 2024-10-22T00:02:07Z

Great thanks!

In the future, you can use the actual GitHub feature: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews#re-requesting-a-review. That'll put it back on our queue. We have automations around it to remove the awaiting response label.

JoshuaKGoldberg

Looks great, thanks for all the changes!

@gyumong

##### [v8.14.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8140-2024-11-11) ##### 🚀 Features - **eslint-plugin:** \[await-thenable] report unnecessary `await using` statements ([#10209](typescript-eslint/typescript-eslint#10209)) - **eslint-plugin:** \[no-confusing-void-expression] add an option to ignore void<->void ([#10067](typescript-eslint/typescript-eslint#10067)) ##### 🩹 Fixes - **scope-manager:** fix asserted increments not being marked as write references ([#10271](typescript-eslint/typescript-eslint#10271)) - **eslint-plugin:** \[no-misused-promises] improve report loc for methods ([#10216](typescript-eslint/typescript-eslint#10216)) - **eslint-plugin:** \[no-unnecessary-condition] improve error message for literal comparisons ([#10194](typescript-eslint/typescript-eslint#10194)) ##### ❤️ Thank You - Gyumong [@gyumong](https://github.com/Gyumong) - Jan Ochwat [@janek515](https://github.com/janek515) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Ronen Amiel You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@gyumong

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.13.0 | 8.14.0 | | npm | @typescript-eslint/parser | 8.13.0 | 8.14.0 | ## [v8.14.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8140-2024-11-11) ##### 🚀 Features - **eslint-plugin:** \[await-thenable] report unnecessary `await using` statements ([#10209](typescript-eslint/typescript-eslint#10209)) - **eslint-plugin:** \[no-confusing-void-expression] add an option to ignore void<->void ([#10067](typescript-eslint/typescript-eslint#10067)) ##### 🩹 Fixes - **scope-manager:** fix asserted increments not being marked as write references ([#10271](typescript-eslint/typescript-eslint#10271)) - **eslint-plugin:** \[no-misused-promises] improve report loc for methods ([#10216](typescript-eslint/typescript-eslint#10216)) - **eslint-plugin:** \[no-unnecessary-condition] improve error message for literal comparisons ([#10194](typescript-eslint/typescript-eslint#10194)) ##### ❤️ Thank You - Gyumong [@gyumong](https://github.com/Gyumong) - Jan Ochwat [@janek515](https://github.com/janek515) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Ronen Amiel You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@gyumong

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.13.0 | 8.14.0 | | npm | @typescript-eslint/parser | 8.13.0 | 8.14.0 | ## [v8.14.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8140-2024-11-11) ##### 🚀 Features - **eslint-plugin:** \[await-thenable] report unnecessary `await using` statements ([#10209](typescript-eslint/typescript-eslint#10209)) - **eslint-plugin:** \[no-confusing-void-expression] add an option to ignore void<->void ([#10067](typescript-eslint/typescript-eslint#10067)) ##### 🩹 Fixes - **scope-manager:** fix asserted increments not being marked as write references ([#10271](typescript-eslint/typescript-eslint#10271)) - **eslint-plugin:** \[no-misused-promises] improve report loc for methods ([#10216](typescript-eslint/typescript-eslint#10216)) - **eslint-plugin:** \[no-unnecessary-condition] improve error message for literal comparisons ([#10194](typescript-eslint/typescript-eslint#10194)) ##### ❤️ Thank You - Gyumong [@gyumong](https://github.com/Gyumong) - Jan Ochwat [@janek515](https://github.com/janek515) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Ronen Amiel You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

ronami added 13 commits September 27, 2024 19:13

initial implementation

27126ca

enable rule

98830b9

improvements and fixes

3830a24

use checker.getContextualType over getContextualType

5b9655d

more fixes

e47395e

improve implementation

4d0bf7c

add docs

117d244

update snapshots

b4be799

enable rule with option

1c49780

rename function names to be a bit less confusing

5dc7600

reword game-plan

2c26323

move rule def to be under its proper title

6800dba

remove invalid tests

5f7833f

ronami changed the title ~~[no-confusing-void-expression] Add option to ignore void<->void~~ feat(eslint-plugin): [no-confusing-void-expression] Add option to ignore void<->void Sep 27, 2024

ronami changed the title ~~feat(eslint-plugin): [no-confusing-void-expression] Add option to ignore void<->void~~ feat(eslint-plugin): [no-confusing-void-expression] Add an option to ignore void<->void Sep 27, 2024

ronami changed the title ~~feat(eslint-plugin): [no-confusing-void-expression] Add an option to ignore void<->void~~ feat(eslint-plugin): Add an option to ignore void<->void on [no-confusing-void-expression] Sep 27, 2024

ronami changed the title ~~feat(eslint-plugin): Add an option to ignore void<->void on [no-confusing-void-expression]~~ feat(eslint-plugin): add an option to ignore void<->void on [no-confusing-void-expression] Sep 27, 2024

ronami changed the title ~~feat(eslint-plugin): add an option to ignore void<->void on [no-confusing-void-expression]~~ feat(eslint-plugin): [no-confusing-void-expression] add an option to ignore void<->void Sep 27, 2024

ronami added 6 commits September 27, 2024 20:08

add more tests

37201b8

add more tests

5e235c8

add more tests

a8ca492

refactor tests

60dc04c

refactor tests

3b66043

wording

b788954

ronami marked this pull request as ready for review September 27, 2024 18:54

JoshuaKGoldberg requested changes Oct 10, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Oct 10, 2024

ronami and others added 9 commits October 11, 2024 15:38

merge conflicts

d735247

Update packages/eslint-plugin/docs/rules/no-confusing-void-expression…

c5f5e12

….mdx Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

Update packages/eslint-plugin/docs/rules/no-confusing-void-expression…

b61ab1f

….mdx Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

Update packages/eslint-plugin/src/rules/no-confusing-void-expression.ts

af9bf87

Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>

format

7ac4a37

format

dcbb392

add missing test

1a52c22

remove code to handle multiple call signatures

f190b15

figure out the case of multiple call signatures in a function expression

8b85c14

ronami requested a review from JoshuaKGoldberg October 22, 2024 06:18

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 22, 2024

JoshuaKGoldberg approved these changes Nov 4, 2024

View reviewed changes

JoshuaKGoldberg merged commit e2e9ffc into typescript-eslint:main Nov 4, 2024
63 checks passed

ronami deleted the ignore-void-void branch November 4, 2024 17:31

phaux mentioned this pull request Nov 5, 2024

Enhancement: [no-confusing-void-expression] Add option to ignore void<->void #8538

Closed

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [no-confusing-void-expression] add an option to ignore void<->void #10067

feat(eslint-plugin): [no-confusing-void-expression] add an option to ignore void<->void #10067

ronami commented Sep 27, 2024 •

edited

Loading

typescript-eslint bot commented Sep 27, 2024

netlify bot commented Sep 27, 2024 •

edited

Loading

nx-cloud bot commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Oct 10, 2024

JoshuaKGoldberg Oct 10, 2024

ronami Oct 11, 2024

ronami Oct 11, 2024 •

edited

Loading

JoshuaKGoldberg Nov 4, 2024

JoshuaKGoldberg commented Oct 21, 2024

ronami commented Oct 21, 2024

JoshuaKGoldberg commented Oct 22, 2024

JoshuaKGoldberg left a comment

feat(eslint-plugin): [no-confusing-void-expression] add an option to ignore void<->void #10067

feat(eslint-plugin): [no-confusing-void-expression] add an option to ignore void<->void #10067

Conversation

ronami commented Sep 27, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Sep 27, 2024

netlify bot commented Sep 27, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Sep 27, 2024 • edited Loading

☁️ Nx Cloud Report

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Oct 10, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Oct 10, 2024

Choose a reason for hiding this comment

ronami Oct 11, 2024

Choose a reason for hiding this comment

ronami Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 4, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg commented Oct 21, 2024

ronami commented Oct 21, 2024

JoshuaKGoldberg commented Oct 22, 2024

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

ronami commented Sep 27, 2024 •

edited

Loading

netlify bot commented Sep 27, 2024 •

edited

Loading

nx-cloud bot commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

ronami Oct 11, 2024 •

edited

Loading