fix(eslint-plugin): [promise-function-async] handle function overloading

[ronami]

PR Checklist

• Addresses an existing open issue: fixes Bug: @typescript-eslint/promise-function-async function overloading #5709
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

This PR resolves #5709 and takes function overloading into consideration.

Currently, the rule flags the following as an issue and auto-fixes it with async, even though the actual return type of the function is Promise<number> | number:

function b(): Promise<number>
function b(a: boolean): number
function b(a?: boolean): Promise<number> | number {
  return Promise.resolve(1);
}

This PR changes the rule to treat the above similarly to how it treats the following:

function b(a?: boolean): Promise<number> | number {
  return Promise.resolve(1);
}

[typescript-eslint]

Thanks for the PR, @ronami!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

[netlify]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	8f790ba
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6747520cefbc0f0008ca94dc
😎 Deploy Preview	https://deploy-preview-10304--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 94 (🔴 down 5 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8f790ba. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 2 targets

• nx run-many --target=build --exclude website --exclude website-eslint
• nx run-many --target=clean

---

Sent with 💌 from NxCloud.

[ronami]

This and the test below aren't exactly valid TypeScript code (will also be flagged by no-redundant-type-constituents), but it did fail while I was figuring out the edge cases on my first pass at it (I've iterated the possible function signatures and their return types). Please let me know if I should drop these.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.76%. Comparing base (c13b6b4) to head (8f790ba).
Report is 53 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10304      +/-   ##
==========================================
+ Coverage   86.56%   86.76%   +0.19%     
==========================================
  Files         431      443      +12     
  Lines       15188    15305     +117     
  Branches     4418     4455      +37     
==========================================
+ Hits        13148    13279     +131     
+ Misses       1683     1672      -11     
+ Partials      357      354       -3     

Flag	Coverage Δ
unittest	86.76% <100.00%> (+0.19%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../eslint-plugin/src/rules/promise-function-async.ts	100.00% <100.00%> (ø)

... and 86 files with indirect coverage changes

[ronami]

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.55%. Comparing base (c13b6b4) to head (294711e).

Files with missing lines Patch % Lines
.../eslint-plugin/src/rules/promise-function-async.ts 75.00% 1 Missing ⚠️
Additional details and impacted files

It seems like this line is not covered on no-unnecessary-type-parameters too, which uses getSignatureFromDeclaration.

I'm not sure how getSignatureFromDeclaration() can resolve to undefined in a function expression. I don't know if it's relevant, but TypeScript's checker.ts defines the return type of the actual implementation as returning Signature.

[kirkwaiblinger]

I'm not sure how getSignatureFromDeclaration() can resolve to undefined in a function expression. I don't know if it's relevant, but TypeScript's checker.ts defines the return type of the actual implementation as returning Signature.

The actual getSignatureFromDeclaration() exposed to the API appears to be here https://github.com/microsoft/TypeScript/blob/55f1248a2052eebdea6bc0e2eef754df89a44bf7/src/compiler/checker.ts#L1776-L1779.

Looks like it's validating that the node passed in is FunctionLike, which eventually drills down to here, which should always return true.

So the | undefined does appear to be unreachable for the nodes that we are passing in here... For situations like this we'd typically use nullThrows to satisfy coverage

[ronami]

I'm not sure how getSignatureFromDeclaration() can resolve to undefined in a function expression. I don't know if it's relevant, but TypeScript's checker.ts defines the return type of the actual implementation as returning Signature.

The actual getSignatureFromDeclaration() exposed to the API appears to be here https://github.com/microsoft/TypeScript/blob/55f1248a2052eebdea6bc0e2eef754df89a44bf7/src/compiler/checker.ts#L1776-L1779.

Looks like it's validating that the node passed in is FunctionLike, which eventually drills down to here, which should always return true.

So the | undefined does appear to be unreachable for the nodes that we are passing in here... For situations like this we'd typically use nullThrows to satisfy coverage

Thanks! I've updated the PR to match this 👍

[kirkwaiblinger]

Currently, the rule flags the following as an issue and auto-fixes it with async, even though the actual return type of the function is Promise<number> | number:

function b(): Promise<number>
function b(a: boolean): number
function b(a?: boolean): Promise<number> | number {
  return Promise.resolve(1);
}

This PR changes the rule to treat the above similarly to how it treats the following:

function b(a?: boolean): Promise<number> | number {
  return Promise.resolve(1);
}

I'm wondering if this should also be the case when the implementation return type is not explicitly specified. I was playing around with this:

export function a(): Promise<void>
export function a(x: boolean): void
// should not flag (right?), does
export function a(x?: boolean) {
  if (x == null) return Promise.reject(new Error())
  throw new Error()
}

export function b(): Promise<void>
export function b(x: boolean): void
// should not flag, doesn't (fixed in this PR)
export function b(x?: boolean): Promise<void> | void {
  if (x == null) return Promise.reject(new Error())
  throw new Error()
}


// should not flag, doesn't
export function c(x?: boolean): Promise<void> | void {
  if (x == null) return Promise.reject(new Error())
  throw new Error()
}

[ronami]

Currently, the rule flags the following as an issue and auto-fixes it with async, even though the actual return type of the function is Promise<number> | number:

function b(): Promise<number>
function b(a: boolean): number
function b(a?: boolean): Promise<number> | number {
  return Promise.resolve(1);
}

This PR changes the rule to treat the above similarly to how it treats the following:

function b(a?: boolean): Promise<number> | number {
  return Promise.resolve(1);
}

I'm wondering if this should also be the case when the implementation return type is not explicitly specified. I was playing around with this:

export function a(): Promise<void>
export function a(x: boolean): void
// should not flag (right?), does
export function a(x?: boolean) {
  if (x == null) return Promise.reject(new Error())
  throw new Error()
}

export function b(): Promise<void>
export function b(x: boolean): void
// should not flag, doesn't (fixed in this PR)
export function b(x?: boolean): Promise<void> | void {
  if (x == null) return Promise.reject(new Error())
  throw new Error()
}


// should not flag, doesn't
export function c(x?: boolean): Promise<void> | void {
  if (x == null) return Promise.reject(new Error())
  throw new Error()
}

Interesting, I didn't take this into consideration. I was hoping I can get away with not having to checking every call signature.

I made some changes to the PR to address this, thanks 👍

Edit: There's no review so I can't ask for a re-review or remove the awaiting response label. I hope anyone sees this. 🙈

[kirkwaiblinger]

Looks great to me! Just touchups requested. Thanks!

[kirkwaiblinger]

Great stuff! Thank you for sending in! ❤️