Skip to content

Bug: [unified-signatures] thinks overloads with and without this type annotation can be merged #10982

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

Open
4 tasks done
jedwards1211 opened this issue Mar 21, 2025 · 16 comments · May be fixed by #11005
Open
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@jedwards1211
Copy link

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.7.2&fileType=.tsx&code=KYDwDg9gTgLgBDAnmYcAKUIFsCWBnYAdRxgAsAlYPCAGwDdgo8AeAFQD44BeOAbwCg4cAPTC4AOkn8Avv37CAVAsEK4AVQJQaAQwB2AEzg4sYGsCzBdMbTBwRdcCADM4AbQzZ8wcQHcSFKloGJgBdXgABGhxdAGs4UhgYMDwALlF9YAYaCBQocSwIAC8cGh1xaABzYUsAWjUAZWF9CABjPGFCYAAjYQApbTptepaoHDAYYUonRksW4GEAcWyu7RoAfQB5LoArYBaYdo9cAmE-MkpqekY8aXEVOA3dObgfVABXMAqobQyECDgAHIQX4AJhBABoXqgWno4Hgzi1SH8EKRUF03iVbLo7nAFMJ%2BKBILA4E43k9bPYXv4LkFrmx2AAKACUKXQmGORGpgSuTHpBPA0HgpPJdgcZwCl2CLA4DMEKPwrKOXgAwvY8DAoG99tB%2BCy2Z4CMRztypXzCYKSWT9qKqcbJXSZXKyAr9RzVbp1ZrtVA4AAfOBkjJOaLAfS6xXsrxGiW03kcPhyqDAGBvKAOARCISiCRSISyWRAA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6WJygM0sQBNaySgHMmAQ3yxoKdFETRoAe2iRwYAL4h1QA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

export type PromiseWithResolvers<T> = {
  // ...
}

/**
 * Userland implementation of [Promise.withResolvers]{@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers}.
 * Once we upgrade to Node 22, we can switch to the builtin.
 */
export function withResolvers<T>(): PromiseWithResolvers<T>
export function withResolvers<T>(
  this: PromiseConstructor
): PromiseWithResolvers<T>
export function withResolvers<T>(
  this: PromiseConstructor | undefined
): PromiseWithResolvers<T> {
  return {
    // ...
  }
}

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/unified-signatures": "error",
  },
};

tsconfig

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

Expected Result

It's not possible to merge these overloads, so I expected no error

Actual Result

These overloads can be combined into one signature with an optional parameter. 11:3 - 11:27

Additional Info

No response

@jedwards1211 jedwards1211 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 Mar 21, 2025
@jedwards1211
Copy link
Author

jedwards1211 commented Mar 21, 2025

Okay I discovered that unifying them with this: PromiseConstructor | void, so I guess not a false positive

@jedwards1211
Copy link
Author

Actually...reopening because this: PromiseConstructor | void currently triggers @typescript-eslint/no-invalid-void-type.

@jedwards1211 jedwards1211 reopened this Mar 21, 2025
@JoshuaKGoldberg
Copy link
Member

Why isn't it possible to unify them as this: PromiseConstructor | undefined?

@JoshuaKGoldberg 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 Mar 24, 2025
@jedwards1211
Copy link
Author

TypeScript doesn't seem to like undefined when calling a function without any this binding

@jedwards1211
Copy link
Author

@kirkwaiblinger kirkwaiblinger removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 24, 2025
@JoshuaKGoldberg
Copy link
Member

Great thanks, that's helpful. This is starting to look like a bug to me.

  1. Playground: the original code plus withResolvers()
  2. Playground: fixed code plus withResolvers() showing the type error on the this context

I think what's happening is that the rule isn't understanding the difference between the implicit this: void context in withResolves() and the explicit this: undefined context in a potential combined withResolvers(this: PromiseConstructor | undefined). It thinks the two are equivalent and so thinks the overloads are unnecessary. But, this: void isn't the same as this: undefined.

IMO: bug, but I'm not confident - seeking more input from @typescript-eslint/triage-team?

@JoshuaKGoldberg JoshuaKGoldberg added the triage Waiting for team members to take a look label Mar 25, 2025
@jedwards1211
Copy link
Author

This does beg the question why TypeScript should give this the type void instead of undefined when calling a function without a binding...I'm struggling to think of any concrete problem it would cause if TypeScript used the undefined type

@kirkwaiblinger
Copy link
Member

If I understand correctly, undefined is saying.... that this must have value undefined, whereas void is saying "don't use this!" (playground)

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Mar 25, 2025

Relates to #10959 and the TS issues mentioned there, too. void | PromiseConstructor is kinda meaningless since void dominates the union in terms of possible values. IIRC the TS team mentioned that they'd see unknown as the better type (where they were discussing the case of void | Promise<void>).

@jedwards1211
Copy link
Author

jedwards1211 commented Mar 25, 2025

Well I know TS can catch certain errors when you try to consume a return value of type void that it wouldn't be able to catch if the function was typed as returning undefined.

But I haven't managed to think of a scenario where this: void would catch errors that this: undefined wouldn't on a function call with no this binding.

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Mar 25, 2025

But I haven't managed to think of a scenario where this: void would catch errors that this: undefined wouldn't on a function call with no this binding.

Not a direct answer, but here's a playground link with lots of cases relating to this issue and #10984: link

@kirkwaiblinger
Copy link
Member

I think, tentatively, that basically both this issue and #10984 are pedantically "working as intended" tbh. Not convinced of this, though, so perhaps there's a counterexample, or some way that the reports can be improved.

@JoshuaKGoldberg
Copy link
Member

Would preferring unknown over void not be a different rule's realm though?

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented Mar 25, 2025

Would preferring unknown over void not be a different rule's realm though?

Yes... these things are all closely related though so it's a bit tricky to avoid discussing related things.

Looking again, neither this: void | T nor this: undefined | T are equivalent to the overloaded form. Furthermore, this?: T is a syntax error. Therefore, pedantically, the overloaded form cannot be replaced with any unified signature I can come up with.

(playground)

function overloaded(): void;
function overloaded(this: String): void;
function overloaded() { }

function unionWithUndefined(this: String | undefined): void { }

function unionWithVoid(this: String | void): void { } 

function optionalThis(this?: String) { } // ERROR, comma expected

const o = { overloaded, unionWithUndefined, unionWithVoid }

overloaded();
o.overloaded();
overloaded.bind(returnsVoid(() => 'lol'))(); // ERROR

unionWithUndefined(); // ERROR
o.unionWithUndefined(); // ERROR 
unionWithUndefined.bind(returnsVoid(() => 'lol')); // ERROR

unionWithVoid();
o.unionWithVoid(); // ERROR
unionWithVoid.bind(returnsVoid(() => 'lol'));

function returnsVoid(cb: () => void): void {
  return cb();
}

export { };

Unfortunately, the overloaded form is technically unsafe to implement, since it doesn't error on o.overloaded(), and therefore, any this value may be provided at runtime. That's not really this rule's place to say, but it does sort of force you into writing a safer form.

On balance, I'm now thinking, yes, this is a bug and the this parameter should be exempted from the optional parameter overload check, even though the only cases where this comes up are likely unsafe.

@kirkwaiblinger
Copy link
Member

I guess there might be an idea of a separate issue to have no-invalid-void-type or some other rule check for overloads with this values that might include void, and request the implementation use this: unknown.

@kirkwaiblinger kirkwaiblinger added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Mar 25, 2025
@jedwards1211
Copy link
Author

Wow, okay I'll work on a PR to close this can of worms. I guess y'all are right to question what's the point of using this pattern, considering TS doesn't even give the desired errors in a lot of cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants