Skip to content

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

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

Closed
4 tasks done
JoshuaKGoldberg opened this issue Feb 23, 2024 · 8 comments · Fixed by #10067
Closed
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@JoshuaKGoldberg
Copy link
Member

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/no-confusing-void-expression

Description

Following from #8375 (review): we tried enabling @typescript-eslint/no-confusing-void-expression internally and very quickly found it to be inconvenient:

Looking at the changes - TBH I don't really like this rule. I don't think any of these changes improve clarity or readability of code.

The biggest issue I see is that if the function is implicitly typed or explicitly marked as returning void the rule still requires you to change things.

For example this code should pass the rule, IMO:

function returnsVoid(): void {}

function test1(): void {
  return returnsVoid(); // should be fine
}
const test2 = (): void => returnsVoid(); // should be fine

Because there's nothing confusing about that code - the expressions are all typed as void and the functions are typed as void.

Also changes like () => expr => () => { expr } actively reduce readability, IMO, esp when considering the above.

Agreed. @bradzacher and I chatted briefly, and Brad suggested:

  1. Adding an option to ignore the void <-> void return case
  2. Making that option the default in v8

Agreed! Filing this issue to add that option. We can file a followup if & when it gets in.

Fail

declare function returnsVoid(): void;

function outerReturnsVoid(): void {
  return returnsVoid();
}

Pass

declare function returnsString(): string;

function outerReturnsString(): string {
  return returnsString();
}

Additional Info

No response

@JoshuaKGoldberg JoshuaKGoldberg added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look enhancement: plugin rule option New rule option for an existing eslint-plugin rule labels Feb 23, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title Enhancement: [no-confusing-void-expression] <a short description of my proposal> Enhancement: [no-confusing-void-expression] Add option to ignore void<->void Feb 23, 2024
@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Feb 25, 2024
@developer-bandi
Copy link
Contributor

developer-bandi commented Mar 14, 2024

@JoshuaKGoldberg @bradzacher I'm currently working on resolving the issue, but I have one question.

The issue was about implementing an option to ignore the rule when the Void type is specified, but
it also be ignore rule when assigning a function type like () => void?

For example, should the following cases be ignore rule? (cc. @auvred @yeonjuan )

type Foo = () => void;
const test: Foo = () => console.log('foo')

declare function foo(arg: () => void): void;
foo(() => console.log());

type HigherOrderType = () => () => () => void;
const x: HigherOrderType = () => () => () => console.log();

interface Foo {
  foo: () => void
}
function bar(): Foo {
  return {
    foo: () => console.log(),
  };
}

const q = {
  foo: { bar: () => console.log() },
} as {
  foo: { bar: () => void }
}

relatedPRComment1
relatedPRComment2

@JoshuaKGoldberg
Copy link
Member Author

Hmm, yeah, I'd think that if there's an explicit void return type then it's ok. So all those cases would be fine.

We should wait to hear from @bradzacher too, if Brad has time.

@bradzacher
Copy link
Member

I'd agree - if the contextual type includes a void return type then we should ideally treat it the same as an explicit void return type annotation (eg ignore it).

@developer-bandi
Copy link
Contributor

Thank you. When applying the new option, I will work to ignore the rule in the case mentioned in the comment above!

@phaux
Copy link
Contributor

phaux commented Aug 16, 2024

Is this option doc good?


### `ignoreReturnInVoidFunction`

Explicit void return type in a function signature might be enough to indicate
that the function will not return anything,
even though the function body might contain a return with an expression.
In such case, it's easy to deduce that the returned value must be of type void too.

This option allows returning void expressions from functions
if the function itself is annotated with a void return type.

This option also changes the automatic fix for void expressions returned from void functions
to add an explicit void return type annotation to the function signature.

Examples of additional **correct** code with this option enabled:

```ts option='{ "ignoreReturnInVoidFunction": true }' showPlaygroundButton
// now it's obvious that we don't expect any response
const promise = fetch('...')
  .then(resp => resp.json())
  .then((data): void => window.postMessage({ data }));

// now it's explicit that we don't want to return anything
function doSomething(): void {
  if (Math.random() < 0.1) {
    return console.error('Nothing to do!');
  }

  console.log('Doing a thing...');
}
```

If so, I can send PR later.

@JoshuaKGoldberg
Copy link
Member Author

For the option name: since we'd like to eventually have it enabled by default, how about checkReturnInVoidFunction? I'm a fan of having optional things default to falsy, not default to truthy.

@phaux
Copy link
Contributor

phaux commented Nov 5, 2024

Looks like the option implemented in #10067 ignores too much. Examples:

declare function onError<T>(callback: () => T): void;

onError(() => console.log('oops'));

const p = Promise.resolve(1).then(() => console.log())

playground

I was expecting that it would require adding (): void => to the arrow functions. It also doesn't mention that a possible fix is to add void type annotation in functions that it does report, and it still autofixes them to add braces.

Maybe leave this open so we can have both: ignore all returns or ignore returns in void functions with literal void type annotation (as described in a comment above).

If not, maybe rename this option to simply ignoreReturn (all) and simplify it dramatically? That would make sense if that rule wants to be in strict config since all void returns are correct and just a stylistic preference. Also option name should be in singular form because all others already are.

@kirkwaiblinger
Copy link
Member

@phaux Could you kindly open a new issue with playground links for each of the concerns?

@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Nov 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
5 participants