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

Parameters of overridden methods are made implicit any when using JSDoc @override #59320

Closed
markcellus opened this issue Jul 17, 2024 · 7 comments
Labels
Duplicate An existing issue was already created

Comments

@markcellus
Copy link

🔎 Search Terms

"jsdoc @override"
"@override"

🕗 Version & Regression Information

  • This is a crash
  • This changed between versions ______ and _______
  • This changed in commit or PR _______
  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about JSDoc comments
  • I was unable to test this on prior versions because _______

⏯ Playground Link

https://www.typescriptlang.org/play/?#code/MYGwhgzhAEBCkFMDC4rQN4ChPWgegCoCdcDoABABzACcwBbDCAFxoEsA7AcwF9pmAnpQTQS0MuRoJmAVxocY6Fu248xBPCQBmAex0AKQcICUGMdCmz5-IQhJq1mUJBhIAFmxAATFC+gIAD2YEDi8YeAhkVEVRXEJiXHEKHQA3BBp2LztEjW09Q1tTLESLaTkOG2F7TB4gA

💻 Code

class BaseClass {

  /**
   * @param {string} type 
   * @returns {string}
   */
  foo(type) {
    return type
  }
}

class ChildClass extends BaseClass { 
  /**
   * @override
   */
  foo(type) {
    return type
  }
}

🙁 Actual behavior

The type param passed to ChildClass.foo does not inherit the string type used in the same method of the BaseClass and evaluates as implicit any

🙂 Expected behavior

The type param passed to ChildClass.foo should inherit the string type used in the same method of the BaseClass

Additional information about the issue

This bug is also present when using @inheritdoc, which should not be the case.

@MartinJohns
Copy link
Contributor

This is a crash

It isn't. You probably forgot to fill out this section of the issue template.


This aligns with the behaviour of TypeScript code. Derived classes don't automatically infer the types based on the base class. This is a duplicate of #23911.

@MartinJohns
Copy link
Contributor

MartinJohns commented Jul 17, 2024

But you probably missed the part when I'm explicitly declaring that it does using @override.

I did not miss it, I just don't see the significance. You expect the overridden method to automatically infer the argument types as the base, but that's currently not supported, as per the linked issue. That you specify the type with JSDoc instead of TypeScript makes no difference.

See the documentation of @override for what it does.

I don't see anything of relevance here either. It doesn't even say that the JSDoc of the parent should be inherited.

But the team will decide if it's a duplicate or not. :-)

@snarbies
Copy link

Unfortunately, even if this deviates from JSDoc documentation, this is "working as intended". This behavior aligns with the behavior of the override keyword. It does go against intuition, hence the linked issue. If the behavior for override changes, I would hope and expect @override to change with it. As it stands, manual type annotations are required for overrides, and potentially deducing them from the overridden method is what is being referred to as "inferring" the types.

@jcalz
Copy link
Contributor

jcalz commented Jul 17, 2024

NB: Also not a TS team member, so I have no authority here.


Some notes on terminology:

A type annotation is when you use a colon followed by the type. override and @override are not type annotations.

Inference is also not the same as type checking. TS can and will check the override for compatibility, but it won't infer the parameter type for you. The override keyword just adds a bit more type checking, but none of the inference you're looking for.

I think you might not be happy with this terminology but that's the way these terms are used in TypeScript.


Everyone finds the current behavior around extends and implements clauses on classes annoying... but it's not a bug, it's a feature request that essentially already exists at the previously mentioned location.

Maybe you're asking that specifically override and @override should enable such inference, but that inference should really be happening anyway, even if you don't specify override that way. If #23911 or the like were implemented, you'd get your desired behavior automatically. I don't know if restricting the inference to the use of override would make the feature more or less likely to be implemented.

@MartinJohns
Copy link
Contributor

It confuses me that somehow the parameters are magically no longer implicitly any when I use the @param tag. Please see the last gif that I posted. That error shouldn't be there if TS is supposed to be not "inferring" anything about the arguments in the overriding method.

When you don't have a type annotation it falls back to any due to the missing type inference (linked issue). When you provide a type annotation it does not. The error you posted has nothing to do with type inference and is just a compatibility check. any is compatible (hence no error), but number is not.

@RyanCavanaugh
Copy link
Member

A type annotation is an annotation that provides a type. override, as designed today, isn't one of those -- it's a keyword that validates that you have some corresponding thing in the base type. That was its sole design. It doesn't have any type effects.

I can get why the other behavior is desirable but it's just factually not a bug. The code is working the way we wanted it to work when we wrote it. You can disagree with what was wanted! But that doesn't make it a bug.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jul 22, 2024
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

6 participants