Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling an overloaded function should resolve to best match, not first #48077

Closed
JoshuaKGoldberg opened this issue Mar 1, 2022 · 5 comments
Closed

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Mar 1, 2022

Bug Report

πŸ”Ž Search Terms

function overload signature void promise

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about overloads

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

declare const it: {
    (callback: () => void): void;
    (callback: () => Promise<void>): void;
};

it(async () => { });

πŸ™ Actual behavior

Autocomplete and hover show the first matching signature:

(callback: () => void): void;

πŸ™‚ Expected behavior

I would have hoped for the more precise signature:

(callback: () => Promise<void>): void;

More (Downstream) Details

This actually causes some issues in typescript-eslint's no-misused-promises rule. See typescript-eslint/typescript-eslint#4620:

  1. When someone calls it( like above, we'd want to be able to check the precise resolved signature for the call to know whether they're dangerously passing a () => Promise<void> to an argument that takes () => void
  2. The type checker getResolvedSignature API returns the () => void overload instead first
  3. That overload would make calls that provide () => Promise<void> seem unsafe

Our workaround is to check all call signatures, which is imprecise and can lead to false negatives in cases like:

declare const it: {
    (syncName: string, callback: () => void): void;
    (asyncName: number, callback: () => Promise<void>): void;
};

// We'll see that an overload exists that has callback: () => Promise<void>.
// Without knowing it's the specific resolved one for this call,
// we'll consider the call safe (even though it's not)
it('', async () => { });

See standalone repro here: https://github.com/JoshuaKGoldberg/repros/tree/typescript-promise-returning-overload-resolve

I think this is different from #41563 because this issue also repros in 4.1.0-beta.

@JoshuaKGoldberg

This comment was marked as resolved.

@tjjfvi

This comment was marked as off-topic.

@MartinJohns
Copy link
Contributor

MartinJohns commented Mar 2, 2022

This is not a bug, it's working as intended. You're supposed to write the order of signatures from most specific to least specific. https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#ordering

You might want to rephrase it as a feature request instead of a bug report.

@JoshuaKGoldberg
Copy link
Contributor Author

Aha, thank you - I had forgotten/missed that. I guess changing it would be a pretty breaking change - will close this issue and instead work on fixing the Mocha type definitions. Cheers!

@RyanCavanaugh
Copy link
Member

We tried various "best" algorithms at first, and they were just disasters, even before generics were in the mix. Ask the C# team about overload resolution if you want to hear a rant :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants