Skip to content

Strict check arity of method that implements interface, type or class #46881

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

Closed
topce opened this issue Nov 20, 2021 · 12 comments
Closed

Strict check arity of method that implements interface, type or class #46881

topce opened this issue Nov 20, 2021 · 12 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@topce
Copy link

topce commented Nov 20, 2021

Bug Report

πŸ”Ž Search Terms

is:issue is:open strict check interface implementation

πŸ•— Version & Regression Information

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

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

// We can quickly address your report if:
//  - The code sample is short. Nearly all TypeScript bugs can be demonstrated in 20-30 lines of code!
//  - It doesn't use external libraries. These are often issues with the type definitions rather than TypeScript bugs.
//  - The incorrectness of the behavior is readily apparent from reading the sample.
// Reports are slower to investigate if:
//  - We have to pare too much extraneous code.
//  - We have to clone a large repo and validate that the problem isn't elsewhere.
//  - The sample is confusing or doesn't clearly demonstrate what's wrong.
interface I {
  hi(a: string, b: string): void;
}

class A implements I {


  hi(a: string): void { // NO ERROR HERE
    throw new Error("Method not implemented." + a);
  }

}

class B implements I {


  hi(a: string, b: string, c: string): void {
    throw new Error("Method not implemented." + a + b + c);
  }

}

πŸ™ Actual behavior

class A compiles even hi method does not have 2 arguments

πŸ™‚ Expected behavior

class A should not compiles
similar like class B maybe some new strict flag need to be added in case
strictCheckImplementationArity or something like this
It would be nice to catch this errors in compile time
otherwise in runtime arguments can be wrongly passed

@MartinJohns
Copy link
Contributor

This is intentional and mentioned in the FAQ. You can use properties instead.

@topce
Copy link
Author

topce commented Nov 20, 2021

Thank you for fast response .
Did not found it in FAQ.
Can you give some example how to use properties instead ?

@MartinJohns
Copy link
Contributor

interface I {
  hi: (a: string, b: string) => void;
}

@topce
Copy link
Author

topce commented Nov 20, 2021

@MartinJohns
Try you suggestion still does not work (it compiles ;-) )
code link
Where did you find it in FAQ?

@MartinJohns
Copy link
Contributor

I completely misread your issue, I'm sorry. I thought this was about implementations being able to accept more "wide" types than the interface (bivariance).

But your issue is mentioned in the FAQ as well:
Why are functions with fewer parameters assignable to functions that take more parameters?

Why do you believe this should be an error? If the implementation can perform the work with less parameters, then so be it.

@topce
Copy link
Author

topce commented Nov 20, 2021

Thank you for link!
Probably you are right it should not be bug but improvement.
I think new strict flag should be added for this issue.

Notice I did not make
second parameter optional with ? and still do not have compile error.

If type or interface define function
and class implement that function to have exactly same signature like function.

Actually I found this issue while working on some project
I refactor code to add support to AWS translation
before it works only with google translation

// old interface 
//
export interface ITranslate {
  isValidLocale(targetLocale: string): Promise<boolean>;
  translateText(
    text: string,
    targetLocale: string
  ): Promise<string>;
}

//new interface
export interface ITranslate {
  isValidLocale(targetLocale: string): Promise<boolean>;
  translateText(
    text: string,
    sourceLocale: string, //add this because of aws
    targetLocale: string
  ): Promise<string>;
}
// old google implementation compiles even I add sourceLocale
// google.ts
async translateText(
    text: string,
    targetLocale: string
  ): Promise<string> 
// aws.ts
async translateText(
    text: string,
    sourceLocale: string,
    targetLocale: string
  ): Promise<string>

So you see in new interface I add parameter sourceLocale , because
aws need this parameter but first version of google did not need it (probably they auto detect language).
Application was compiling no ts errors but I had runtime error
because I pass sourceLocale to google translateText and it expected targetLocale.

It would be nice that ts compiler warn me about this .

In link that you provided they are speaking about callback
but here I speak about implementing type or interface.
As type and interface are typescript concepts not javascript
not see reason why they could not strict check implementation of
of methods declared in type or interface.
and report error if implementation has less arguments.

@topce
Copy link
Author

topce commented Nov 20, 2021

Is there is some way to change issue type instead of Bug
to be Improvement (new feature) ?

@topce topce changed the title Strict check arity of method that implements interface Strict check arity of method that implements interface or type Nov 20, 2021
@fatcerberus
Copy link

Yeah, it's not a bug. There's no practical difference between

hi(a: string): void {
    /* do stuff */
}

and

hi(a: string, b: string): void {
    /* do the exact same stuff and completely ignore `b` */
}

On the other hand, adding a third parameter c is properly an error because the caller is only ever going to give you a and b.

This logic applies equally for both callbacks and interfaces.

@topce
Copy link
Author

topce commented Nov 20, 2021

@fatcerberus agree it is not a bug
but it could be nice new feature

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 22, 2021
@RyanCavanaugh
Copy link
Member

Turning this feature on, you would quickly discover that you want to turn it off again. Ignoring trailing parameters is common and idiomatic JS that presents no runtime problems.

@topce
Copy link
Author

topce commented Nov 23, 2021

Hi @RyanCavanaugh thank you for reply, but I do not agree.

Personally I would always turn it on in my code base.
Not sure how you know what I would do !?

I give real example where it could help catching runtime error during compile time:
Change interface (or type ) method signature add parameter in the middle.
Add new implementation and forget to update old ones.

From my example above: add second parameter to method signature translateText above
it is more logical to add source before target (I know if I add it on end would not need to change anything in google class).

Not sure why you speak about runtime problems, maybe you did not understand my request !?

I was more speaking about catching runtime errors during compile time,
and TypeScript is good in this.

Even turning on feature, review code and turning off it again it is better then
nothing!
Quite often I do this with existing TS strict flags.

I think it could be nice to have this flag , but maybe it is complicated to implement it.

Notice also that TypeScript has power to specify optional parameters
So with new flag turn on
this would not be same interfaces :

`
interface I {
hi(a: string, b: string): void;
}

interface I {
hi(a: string, b?: string): void;
}
`
Sorry I opened bug but it should be new feature.

Quite often I have similar discussion with my collogues:
is this a bug or feature ;-)

@topce topce changed the title Strict check arity of method that implements interface or type Strict check arity of method that implements interface, type or class Nov 24, 2021
@topce
Copy link
Author

topce commented Jan 7, 2023

Even ChatGPT is confused by this one : think it should be error
like in other strong typed languages
Maybe @ahejlsberg can take a look if had some time ...

@microsoft microsoft locked as resolved and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants