[Style] Totally non-blocking, I just noticed that we can inline these two conditions. Feel free to ignore this, if you find the current implementation more readable.

thank you i agree this style

[Refactor] I see no reason why we would want to use targetNodeAncestorHasVoidReturnType with some other set of targets nodes. So I think we can just put those node types inside the function. WDYT?

I agreed that there was no possibility of reuse, so i moved the targets inside the function and changed the function name to be more clear.

[Bug] This function won't stop until it finds a matching parent, so the following case is false negative

function foo(): void {
  const bar = () => {
    return console.log()
  }
}

(playground)

I think that this function should look +- like this:

• if !node.parent -> return false
• if node.parent is one of the FunctionDeclaration, ArrowFunctionExpression, FunctionExpression:
  • if node.parent has void in type annotation -> return true
  • if node.parent hasn't void in type annotation -> return false
• return targetNodeAncestorHasVoidReturnType(node.parent)

in playground, this case seems false positive. anyway, this is bug, so i add test code and fix targetNodeAncestorHasVoidReturnType function

in playground, this case seems false positive

As far as I know:

false negative === the rule should report error, but it doesn't do so
false positive === the rule should not report error, but it does so

I'm not familiar with the testing, so I think I used the terminology the other way around. thank you for telling me!

[Style] Same as here: #8632 (comment)

WDYT about inlining this if statement?

same convention is make sense. thank you!

[Refactor] Why do we need the targets tuple if we don't iterate over it? IMO if we directly compare node.parent.type to AST node types, it will be more readable. What do you think?

Comparing variables directly seems much more readable!

[Bug] I just noticed that we determine whether function signature has void return type or not by doing pure syntax check.

Thus, the following case is false positive (the rule shouldn't report error, since function returns Foo - an alias of void type):

type Foo = void

function test(): Foo {
  return console.log()
}

playground

I didn't even think about it. Thank you!

[Testing] Some more cases to add:

• the case mentioned here - return console.log('foo') on the top-level
• function without void in signature contains in its body function with void in signature, and this nested function returns return console.log('foo')
• Type aliases, like in the comment above ^ (in the signature of the parent function, in the signature of the returned function, in both signatures at the same time)

I added a test case, but I may have misunderstood the last comment, so please check my new test code. thank you!

I think it's fine.

Also, I think it would be nice to add the following cases:

• function test(): void & void {
    return console.log('foo')
  }

• type Foo = void
  declare function foo(): Foo
  function test(): Foo {
    return foo()
  }

• // se UPD below. there is a typo
  type Foo = void
  const test: Foo = () => console.log('err')

UPD: there was a typo in the last code block! We shouldn't consider case with such a weird typing (the function is being assigned to variable with void type). Instead, I meant the following code:

• type Foo = void
  const test = (): Foo => console.log('err')

thank you! add all test case and last case has ignoreArrowShorthand option to pass, so add ignoreArrowShorthand option

i think last case situation is same to below comment

[Bug] Same as here #8632 (comment)...

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

The error shouldn't be reported, since arrow function has the void return type

playground

i think this error is because of ignoreArrowShorthand. so ignoreArrowShorthand option to true or change code like:

type Foo = void
const test = (): Foo => { return console.log(); }

I add test case with ignoreArrowShorthand option true.

playground

i think this error is because of ignoreArrowShorthand. so ignoreArrowShorthand option to true or change code like:

The rule reports error because it does pure syntax check instead of type-level check.

type Foo = void
const test1 = (): void => console.log() // not reported - ok
const test2 = (): Foo => console.log() // reported - ok

playground

ignoreArrowShorthand ignores all arrow functions, but rule users may want to ignore only void <-> void cases as mentioned in the original issue.

thank you i understand it! and since it seems to be valid even when declaring a type in a variable of a function expression, I modified the code and test case.

type Foo = () => void;
const test: Foo = function () {
  return console.log('err');
};

[Refactor] This code may be converted into a single return statement. What do you think?

[Bug] I somehow missed this in previous reviews: we can't rely on syntactic check here

(): any => console.log(); // reported - nice!
(): unknown => console.log(); // reported - nice!

type Foo = any
(): Foo => console.log(); // not reported - bug!
type Bar = unknown
(): Bar => console.log(); // not reported - bug!

playground

When specifying a type using a type alias, it is difficult to find a way to determine what type it is.

Could you please give me a hint?

type Bar = unknown
(): Bar => console.log();  // how to know Bar is unknown type in create fn

@auvred Can you help with the issue left in the comment above?

I'm not 100% sure about this, but I'd try playing around with function signatures. There is nice utility function in the ts-api-utils package - getCallSignaturesOfType.

See: https://github.com/JoshuaKGoldberg/ts-api-utils/blob/d808eba09baa3ba093cd31b85aad57bdcc539867/src/types/getters.ts#L23

Search with some examples of usage: https://github.com/search?q=repo%3Atypescript-eslint%2Ftypescript-eslint%20getCallSignaturesOfType&type=code

it works to me very well. thank you!

[Testing] I think it would be nice to add few tests after #8809 is merged. This PR fixes the ancestorHasReturnType behavior.

We can add something like this:

function foo(): void {
  () => () => console.log();
}

function foo(): any {
  () => () => console.log();
}

(both of the above cases should be reported)

[Refactor] (optional) hasValidReturnType function is declared in block where context is visible. What do you think about calling const services = getParserServices(context); inside hasValidReturnType (like canFix function above does) instead of passing services as an argument?

I too believe that using context is better than passing a service.

[Bug] Why do we get a contextual type for function expressions and arrow functions?

The rule you took this code from has a detailed explanation of why it calls getContextualType for function expressions and arrow functions:

This means that the following case is reported:

const test: () => any  = (): void => console.log()

(it should not be reported since (): void => console.log() is allowed with ignoreVoidInVoid: true)

playground

[Bug] This code returns from for loop on the first iteration. So if a function has several signatures, the rule will check only the first signature:

function test1(): void
function test1(arg: string): any
function test1(arg?: string): any | void {
  if (arg) {
    return arg
  }

  return console.log() // not reported - ok
}

function test2(arg: string): any
function test2(): void
function test2(arg?: string): any | void {
  if (arg) {
    return arg
  }

  return console.log() // reported - bug
}

playground

[Refactor] Let's do a bitwise OR of these flags instead of calling isTypeFlagSet twice?

I did not understand that the isTypeFlagSet function utilizes bitwise operators, so I called it twice. thank you for telling me!

If we check that function signature don't contain any and unknown in the return type, we should do the same for typed functions/variable declarations. The following cases are not reported:

type HigherOrderType = () => any;
(): HigherOrderType => () => console.log();

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

playground

Please see my comment from the one of the previous reviews #8632 (comment). 🙂

The code samples I provide, which contain some error messages, are for reference only. They are not intended to be directly copied into real tests. I'm including comments on these code samples to better illustrate what the bug I found is. The same goes for variable names: test1 and test2 are named to avoid name collisions in the playground. Having the test2 function in a separate test case without test1 looks a bit confusing.

I ask you to pay attention to the reviews and not just blindly insert some stuff into the code, assuming that someone will notice it in one of the next reviews anyway.

I apologize for making the same mistake by not checking the code thoroughly after writing it.

In the future, I will look more closely before submitting a review.