Bug: [@typescript-eslint/no-invalid-void-type] false positive on generics #7227

G-Rath · 2023-07-14T20:55:18Z

Before You File a Bug Report Please Confirm You Have Done The Following...

I have tried restarting my IDE and the issue persists.
I have updated to the latest version of the packages.
I have searched for related issues and found none that matched my issue.
I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.1.6&fileType=.tsx&code=CYUwxgNghgTiAEAzArgOzAFwJYHtXwFsAPAHgBUA%2BACgEoAueMgbgFgAod4kgNxy2Go1WHNhBwBzAIKpgAJRAZkMVDz4CqaUIiyoQwIe3YQF8XvwAKMHASwBnEA0vW7IVfwrwAvPF0B3eE429m7qtF4eAN7wAL4GoiZmwACyUAAODCmpJLYYMDriADSmah7efvCZ2bn5RYmCTEA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1uYDcBDRgBNaPDpWFFS6KImjQO0SODABfECqA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

declare function mx<T>(): T;

mx<void>();

logAndReturn<void>(undefined);

let voidPromise: Promise<void> = new Promise<void>(() => { });
let voidMap: Map<string, void> = new Map<string, void>();

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/no-invalid-void-type": "error"
  },
};

tsconfig

{
  "compilerOptions": {
    // ...
  }
}

Expected Result

I expected all of these to be considered valid uses of void

Actual Result

Only the last two uses are considered valid.

Additional Info

Maybe I'm missing something but I've read the docs a couple of times and the issues, and I can't find anything explaining why this general use of void is being flagged - or inversely, anything confirming that there is more logic than just "is this a generic?".

Importantly the logAndReturn is taken from the documentation itself for showcasing

The following patterns are considered warnings with { allowInGenericTypeArguments: false }:

Which implies to me those patterns should not be considered warnings if allowInGenericTypeArguments is true. This seems to be happening from v5 onwards.

The text was updated successfully, but these errors were encountered:

bradzacher · 2023-07-15T01:54:07Z

This is working as expected - though the documentation is quite unclear on this point.

The option should probably be allowInGenericTypeArgumentsForTypes because it specifically only applies to generic types as part of a type reference and does not apply to function calls and constructors.

I don't remember why it was built like this specifically. @JoshuaKGoldberg this was your baby - so you remember?

JoshuaKGoldberg · 2023-07-15T18:48:31Z

void is only valid as a return type or generic type argument. 3:4 - 3:8

Amusingly, I think the message is mixing up argument and parameter. Today the rule allows parameters. The complaint is on arguments.

allowInGenericTypeArgumentsForTypes

Maybe we can:

v6.x: Add that option as proposed
v7.x: Breaking change the rule options:
- Rename allowInGenericTypeArguments & allowInGenericTypeParameters
- Allow them to be either catch-all booleans or granular objects, similar to @typescript-eslint/prefer-nullish-coalescing's ignorePrimitives post-Enhancement: [prefer-nullish-coalescing] Allow ignorePrimitives option to be true #7180?

Thoughts?

baby

...was this one of my rules? I don't remember getting too involved with it. Maybe more like a street cat I've been feeding? 😄

G-Rath · 2023-07-15T19:21:16Z

Sounds good to me!

bradzacher · 2023-07-17T00:48:17Z

@JoshuaKGoldberg sorry I thought you implemented this because you love to hate void 😄

But no turns out it was you, @G-Rath!
And based on your original implementation - this is working as expected!
f667ff1#diff-0399d23c95309991615110bfd289120b920df533b4202452cb6ce10b6c89cc51R211-R220

typescript-eslint/packages/eslint-plugin/tests/rules/no-invalid-void-type.test.ts

Lines 197 to 206 in 587ac30

    
           { 
        
             code: 'functionGeneric<void>(undefined);', 
        
             errors: [ 
        
               { 
        
                 messageId: 'invalidVoidNotReturnOrGeneric', 
        
                 line: 1, 
        
                 column: 17, 
        
               }, 
        
             ], 
        
           },

G-Rath · 2023-07-17T00:53:38Z

ah crap - I should have known, past-me is always out to get me...

As the original implementor though, does that mean I can arbitrarily change it? 😝

Seriously though I'll look over my original PR and try to figure out what I was thinking - I assume that I was bring over https://palantir.github.io/tslint/rules/invalid-void/ though I've not mentioned that in the PR body (but you @bradzacher mention "the original rule" in your initial review comment), and so that's probably where the logic came from rather than any strong personal opinions on my part that I've since unknowingly changed my mind about

G-Rath · 2023-07-27T21:13:15Z

I've had a bit of a further look over this, and I think actually it could be argued this is a general bug maybe combined with some bad/too-generic naming because new Func<void> is allowed for some reason (I assume for new Promise<void>).

If you're not happy to just change the logic to include functions without new as it being a bug, then I think probably it makes sense to add allowInGenericTypeArgumentsForFunctions - arguably the ideal would be that the existing option was actually allowInGenericTypeArgumentsForTypes, and then maybe allowInGenericTypeArguments just passes on to both options but that's probably not worth it.

What do folks think?

binoche9 · 2023-07-31T16:00:28Z

Stumbling upon here because I also just ran into this while doing the void removal conquest in my own codebase 😄

I'd be happy with any solution that allows me to make this rule stop flagging these, but I'd add a vote for having these all controlled by a single allowInGenerics option (it seems to be how TSLint documented it at least), as the advantages/disadvantages of the different cases (argument vs parameter etc.) seems pretty unclear to me.

In particular, I found it really confusing that

const x = mx<void>

works, but

const x = mx<void>()

doesn't

binoche9 · 2023-08-24T15:11:40Z

If there's a PR that allows void in all type parameters would it be accepted?

JoshuaKGoldberg · 2023-09-12T19:09:02Z

#7227 (comment) makes sense to me. We can treat it as a breaking change for v7 to give ourselves a bit more flexibility in option naming. Thoughts @typescript-eslint/triage-team?

If there's a PR that allows void in all type parameters would it be accepted?

No, I think that'd be too much. Much of the time void isn't the right option (e.g. Array<void>).

43081j · 2023-12-01T11:50:52Z

if nobody else tackles this, i'd be happy to open a PR whatever way you want it doing

this rule currently doesn't make much sense IMO because of this behaviour, and will mark many sinon calls as errors in tseslint strict config.

is the tldr to add allowInGenericTypeArgumentsForFunctions bool, which allows all occurrences of void in type parameters for functions?

but i don't understand why anyone would ever have good reason to set that to false. in which case, why is it even an option? the rule is just wrong, right? this is a bug. i feel like we'd be introducing an option to keep a bug around which was never meant to be there.

and why TypeArguments? they're type parameters in all cases we're talking about here. they may be later used for arguments, but the types themselves were type parameters, no?

bradzacher · 2023-12-01T20:20:42Z

but i don't understand why anyone would ever have good reason to set that to false

declare function foo<T>(arg: T): string;

foo<void>(undefined);

An example of why the rule bans it.

A large numbe 8000 r of generics are not specifically in a return type position in functions. There's a lot that are as well though. The isssue is that we can't tell purely based on syntax - the only way to "correctly" detect would be with type info and even then things can get dubious.

43081j · 2023-12-02T09:15:09Z

That's true. I would have probably resorted to using type info, resolving function parameters' types and checking if they resolve to only void.

For the suggested solution though, If we add an option, do we want to default it to true? i.e allow void in all type parameters of call expressions?

JoshuaKGoldberg · 2023-12-22T18:45:49Z

Brad and I chatted briefly - we're slowly starting to sour on this rule as there are a couple issues (#6547, #7227) that are surprisingly intricate with it.

I also filed #8113 to track reworking the rule more holistically. I'm going to close & lock this issue for now so that we can focus on that discussion. Please do post there if you have more information to add. If it goes nowhere I'll re-open this one. Thanks all!

If we add an option, do we want to default it to true? i.e allow void in all type parameters of call expressions?

Great question. The proposal in #8113 is that it's always the behavior, and not an option. Let's discuss over there if there are reasons why we wouldn't want that to be the case.

G-Rath added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jul 14, 2023

JoshuaKGoldberg added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Jul 17, 2023

JoshuaKGoldberg added triage Waiting for team members to take a look and removed awaiting response Issues waiting for a reply from the OP or another party labels Jul 27, 2023

JoshuaKGoldberg added breaking change This change will require a new major version to be released accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Sep 14, 2023

G-Rath mentioned this issue Sep 19, 2023

Bug: [@typescript-eslint/no-invalid-void-type] void is only valid as a return type or generic type argument #6547

Closed

4 tasks

JoshuaKGoldberg mentioned this issue Dec 22, 2023

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

Open

JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Dec 22, 2023

typescript-eslint locked and limited conversation to collaborators Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Bug: [@typescript-eslint/no-invalid-void-type] false positive on generics #7227

Bug: [@typescript-eslint/no-invalid-void-type] false positive on generics #7227

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bug: [@typescript-eslint/no-invalid-void-type] false positive on generics #7227

Bug: [@typescript-eslint/no-invalid-void-type] false positive on generics #7227

Comments

Before You File a Bug Report Please Confirm You Have Done The Following...

Playground Link

Repro Code

ESLint Config

tsconfig

Expected Result

Actual Result

Additional Info

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!