Rework @typescript-eslint/no-invalid-void-type: enable type checking and correct docs + option naming

[JoshuaKGoldberg]

Overview

@typescript-eslint/no-invalid-void-type has a couple of surprisingly tricky issues associated with it:

• #6547
• #7227

I think the root issue is that when the rule was originally implemented (#1847), practices around the void type were much simpler than they are now (edit: and, as @G-Rath notes, it was largely a port of the even-older TSLint rule!). Conditional types had only been in the language for a couple of years (2.8 release notes). We also hadn't yet solidified our docs and rule options on correctly referring to arguments vs. parameters (#146 -> #5384) and not redundantly referring to them as "a generic" (https://www.totaltypescript.com/no-such-thing-as-a-generic).

Proposal (merging https://github.com/typescript-eslint/typescript-eslint/issues/7227#issuecomment-1636854488 & https://github.com/typescript-eslint/typescript-eslint/issues/7227#issuecomment-1836726297): for ~v7~ v8, let's..

• Switch the rule to using type information to determine whether a type argument is passed to a parameter that's eventually used in a return type
• Remove the allowInGenericTypeArguments option...
• ...and replace it with an allow option that uses the format from https://github.com/typescript-eslint/typescript-eslint/discussions/6017 -> #4436

Thoughts, all? cc @bradzacher @auvred @G-Rath @omril1

[G-Rath]

I think the root issue is that when the rule was originally implemented (https://github.com/typescript-eslint/typescript-eslint/pull/1847)

Worth noting too I think that the original implementation was primarily a port of the TSLint rule, meaning it's age is actually older then #1847 (I'm pretty sure that I didn't change the behaviour much if at all from the original rule...)

[auvred]

Linking a couple of related issues: https://github.com/typescript-eslint/typescript-eslint/issues/4998, https://github.com/typescript-eslint/typescript-eslint/issues/3612

---

Switch the rule to using type information to determine whether a type argument is passed to a parameter that's eventually used in a return type

Sounds great!

...a type argument is passed to a parameter that's eventually used in a return type - I think we can also cover cases when it is used as a this parameter type (if allowAsThisParameter is enabled)

[Josh-Cena]

void should only be used as a return type, but it can also be a part of a return type. For example, f(): X extends Y ? void : number, f(): { done: false, value: void } | { done: true, value: number }, f(): Promise<void>, etc. should all be safe. But, I think f(): void | string and f(): void & string should stay invalid—it seems hard to specify...

[jrandolf]

But, I think f(): void | string and f(): void & string should stay invalid—it seems hard to specify...

void | string is a contract that the return type will be a string or something UB (Undefined Behavior). So the user would be required to write typeof f() === 'string'. This provides better semantics at runtime.

[Josh-Cena]

But when is UB good? Even if you do typeof x === "string", you may accidentally include garbage values from the void branch because that void function is secretly returning a string.

[jrandolf]

But when is UB good?

Suppose you have

function a(): string | void;

if (typeof a() === 'string') {
  console.log("Log for strings only");
}

If you add number to string | void, then the above code still works! Now suppose you have

function a(): string | undefined;

if (typeof a() !== 'undefined') {
  console.log("Log for strings only");
}

If you add number to string | void, then the above code breaks.

Now ofc you can argue that instead of using 'undefined' for the comparison, one could use 'string', but expecting people to know this nuance is not scalable IMO.

Also, for

you may accidentally include garbage values from the void branch because that void function is secretly returning a string.

Can you give me an example?

[bradzacher]

const b: () => void = () => 'oops';

function a(): string | void {
  if (Math.random() > 0.5) {
    return 'hi';
  }
  return b();
}

const result = a();

This code is valid and typechecks fine because void can also mean "any value". The value returned by b() is garbage and because it was a string it's now infected the return value from a() with its garbage.

It's obviously a convoluted example but it shows how the UB can easily poison things. But if you only use undefined in the union then void is not assignable to undefined and this case is impossible.

[Josh-Cena]

This is smelly code and would eventually break somewhere. It's not worthwhile to "prevent" it with yet another smelly type. You are asserting what doesn't happen and then assuming what happens instead; the intuitive way is always to assert what happens, and then rely on the type you have already proven. Think about the cases such as { type: "a", value: number } | { type: "b", value: boolean }—similarly people may write if (res.type !== "b") and then assume it's "a" and then break if you add more things to the union—well they are writing bugs in the first place!

Can you give me an example?

function maybeCall(cb: () => void): string | void {
    if (Math.random()) return "Hey";
    return cb();
}

const x = maybeCall(() => "Unexpected");

[jrandolf]

...it shows how the UB can easily poison things.

Definitely won't deny that UB poisons things. It does in every other language.

This code is valid and typechecks fine because void can also mean "any value".

This example only works because functions are covariant in the return type. For example, let b: void = 'string' won't work.

But I think the above examples convince me the void is bad in a type union (ends up reasoning to the same reason void is bad as a return type). I think then this rule ought to invalidate usages of covariant voids. For example, T | void should be bad as well as

type A<T> = T | void;

// Ban this type.
type E = () => A<T>

since () => A<string> will be assignable to () => A<void>.

[ngbrown]

Maybe this issue is the right one for this problem. allowInGenericTypeArguments: true does not seem to be working as expected on function calls.

I have a situation where @typescript-eslint/no-invalid-void-type is disagreeing with the only non-error solution for what would otherwise be a typescript error. This is specific to a tsconfig of "strict": true.

Playground Link

Repo code:

class Logger {
}

interface LogFunc {
  (this: Logger, msg: string): void;
  (this: Logger, fields: Record<string, unknown>, msg?: string): void;
}

const logFn: LogFunc = () => { };

const childLogger = new Logger();

// TS2345: Argument of type 'string' is not assignable to parameter of type 'Record<string, unknown>'.
logFn.call(childLogger, "string");

// @typescript-eslint/no-invalid-void-type: void is only valid as a return type or generic type argument.
logFn.call<Logger, [string], void>(childLogger, "string");

eslintrc.json:

{
  "rules": {
    "@typescript-eslint/no-invalid-void-type": [
      "error",
      {
        "allowInGenericTypeArguments": true
      }
    ]
  }
}

tsconfig.json:

{
  "compilerOptions": {
    "strict": true,
    "strictNullChecks": true
  }
}

[mnaoumov]

The rules should also allow the following

type X = () => Promise<void | null>;

const f: X = async () => {
  // do nothing;
};

const g: X = async () => {
  return null;
};

[olucafont6]

I think this might be another case of false positives that I haven't seen brought up anywhere else - let me know if there's an existing issue for this or if I should file another one though.

Reproducer

async function doAsyncWork(): Promise<void> {
  await Promise.resolve();
}

const promise1 = doAsyncWork();
const promise2 = doAsyncWork();

// error  void is only valid as a return type or generic type argument  @typescript-eslint/no-invalid-void-type
const combinedPromises: Promise<[void, void]> = Promise.all([promise1, promise2]);

const promises = [1, 2, 3].map(async () => doAsyncWork());

// error  void is only valid as a return type or generic type argument  @typescript-eslint/no-invalid-void-type
const combinedPromiseArray: Promise<void[]> = Promise.all(promises);

It doesn't seem like there's a way to have a promise containing multiple void return types - only a single void works.