typescript-eslint icon indicating copy to clipboard operation
typescript-eslint copied to clipboard

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

Open JoshuaKGoldberg opened this issue 1 year ago • 6 comments

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

My proposal is suitable for this project

  • [X] 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 https://github.com/typescript-eslint/typescript-eslint/pull/8375#pullrequestreview-1897027584: 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 avatar Feb 23 '24 12:02 JoshuaKGoldberg

@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

developer-bandi avatar Mar 14 '24 16:03 developer-bandi

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.

JoshuaKGoldberg avatar Mar 18 '24 13:03 JoshuaKGoldberg

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

bradzacher avatar Mar 18 '24 13:03 bradzacher

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

developer-bandi avatar Mar 18 '24 13:03 developer-bandi

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.

phaux avatar Aug 16 '24 12:08 phaux

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.

JoshuaKGoldberg avatar Aug 19 '24 12:08 JoshuaKGoldberg

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.

phaux avatar Nov 05 '24 15:11 phaux

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

kirkwaiblinger avatar Nov 05 '24 15:11 kirkwaiblinger