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

Allow strict function parameter definitions in interfaces and callbacks #30876

Closed
richardbullin opened this issue Apr 12, 2019 · 10 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@richardbullin
Copy link

richardbullin commented Apr 12, 2019

I would like function parameters declared in an interface (and callbacks) to match its implementation exactly and if not generate a compile time error.

I believe the solution could make use of the spread operator as well as marking optional parameters with '?' for the situations where the parameters are indeed variable or optional.

Can someone give me an example of why you would allow less parameters to be defined other then not having the energy to copy the definition exactly? Is this an issue with implementation?

For example I would like the below to generate a compile time error due to the Clone method in TDictionary missing the 'deepCopy' parameter:

interface IClonable
{ 
  Clone(deepCopy: boolean): any; 
}

class TDictionary implements IClonable
{
  public Clone(): TDictionary // An error should be generated here since 'deepCopy' is missing
  {
    return something;
  }
}

You could allow marking optional parameters in the interface like below.

interface IClonable
{ 
  Clone(deepCopy?: boolean): any; // deepCopy parameter does not have to be defined in implementations
}

Also if the parameter names don't match an error should be generated. The below should generate an error.

interface IClonable
{ 
  Clone(deepCopy: boolean): any; 
}

class TDictionary implements IClonable
{
  public Clone(wouldYouLikeACoffee: boolean): TDictionary // An error should be genereated here since 'deepCopy' !== 'wouldYouLikeACoffee' 
  {
    return something;
  }
}

This also applies to callback definitions, i.e.

  class TClickEvent
  {
    public Subscribe(aCallback: (X: number, Y: number) => void)
    {
      //
    }
  }

  function OnClick()
  {
    //
  }

  const CLICK_EVENT = new TClickEvent();
  CLICK_EVENT.Subscribe(OnClick); // An error should be generated here because OnClick should have x and y parameters defined.
@richardbullin richardbullin changed the title Really strict typescript (aka TypeStrict) Allow strict function parameter definitions in interfaces and callbacks Apr 13, 2019
@Psidium
Copy link

Psidium commented Apr 13, 2019

Oh trust me, I would like that feature as well.

But I believe that this is more suitable for a linter than the language itself because Typescript aims to be a superset of Javascript.

You see, passing an argument to a function and not using that argument in the said function will not generate a runtime error. So why should the compiler generate one?

When we think in Object Oriented code we must think of methods as "messages" that are passed to objects, if said object will do anything with that message, that is not the caller's concern (if you have time please watch the talk Polly want a message by Sandi Metz, it is wonderful).

So, if I'm the caller and want to send a message to an object that implements an interface, I am going to send the message as established in the contract, with the arguments it needs. But if the object I am sending the message to does not want anything to do with the said message, then who am I to judge?

Typescript allowing the omission of such arguments in the implementing classes is actually right, as it will make sense in the view of the class implementing the said interface.

BUT, apart from that, we can clearly see that interfaces are contracts between classes. Some people find that contracts should be strictly followed, other people see them as more flexible.

I'm one of the people that agree with you, when I stablish a contract I want all classes that follow said contract really follow the contract. On big projects with a large number of interfaces, when having to change a interface to have another method or changing the return type, I want to be warned by my tooling that I forgot to change some classes, even if they would compile and not generate a runtime error.

That's why I proposed the rule strict-interface-implementation in tslint: palantir/tslint#4175

Right now I am blocked by the lack of a type comparison API from typescript's part (#9879), but I will create the rule someway or another.

@richardbullin
Copy link
Author

If its in a linter or in the compiler Id be pretty happy but I do question whether the generation of a runtime error should be used to specify where it should be solved? This question is a good one though.

The below example will generate compile time errors and they wouldnt generate runtime errors?

 interface ILogger
  {
    Log(message: string, associatedObject: Object);
  }

  class TFoo implements ILogger
  {
    public Log(message: string, associatedObject: Object)
    {
      console.log(message);

      if (associatedObject !== undefined)
      {
        // Do something with object
      }
    }

    public OtherMethod()
    {
      this.Log('Error'); // Compile time error here, at runtime wouldnt cause an error 
    }
  }

As you say there will definitely be people especially if they're from a javascript background who prefer the flexible/loose typings and then there will be others who probably come from desktop development who are used to strict contract definitions. An option to use strict which defaults to false will make everybody happy.

Heres an analogy between the two approaches: If I create a few copies of a hand written letter (the message) and send it off:

  • 'Mr Loose' has a blade installed on his letterbox so when the message arrives it chops off the bottom part of the letter, all the contact details of the sender are now gone This doesn't bother him though because he doesn't intend to reply to the sender.

  • 'Mr Strict' allows his message to arrive intact. He can decide to ignore the senders data or use it.

My opinion is that by making it strict when you add or modify data everyone is aware of it and has access to the information, the implementation can still ignore the passed in data. It would also show in your IDE or linter that you have an unused parameter which may or may not be ok.

This has caused me issues in particular with callbacks. Taking my previous example which I've put down below again. When you go to subscribe to the event its very easy to connect to the wrong method and its only at runtime you discover its not working properly.

  class TClickEvent
  {
    public Subscribe(aCallback: (X: number, Y: number) => void)
    {
      //
    }
  }

  function OnClick()
  {
    //
  }

  const CLICK_EVENT = new TClickEvent();
  CLICK_EVENT.Subscribe(OnClick); // An error should be generated here because OnClick should have x and y parameters defined.

@richardbullin
Copy link
Author

richardbullin commented Apr 14, 2019

@Psidium Actually one reason I just thought of why it might make more sense in the compiler is it would then allow you to specify the contract down to the function level as opposed to the linter which would be working application wide (you could disable in linter with comments everytime though).

I propose a compiler flag to specify whether strict parameter checks should be done or not, if not then it would work as it does now but if it is present then parameters would need to be marked with '?' if its optional or make use of the spread operator '...' if it takes a variable number of parameters.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 15, 2019
@RyanCavanaugh
Copy link
Member

Duplicate #370, #9300, #9765, #9825, #13043, #16871, #13529, #13977, #17868, #20274, #20541, #21868, #26324

@richardbullin
Copy link
Author

Thanks @RyanCavanaugh. After looking through the duplicates and faq it seems quite a few people are saying 'hey this doesn't feel right'.

I would like to mark some of my own function/callback definitions via a keyword like below which would force the implementations to match exactly (I'm not talking about built in javascript functions like forEach).

 interface IClonable
{ 
  strict Clone(deepCopy: boolean): any; // Has strict keyword therefore implementation must match exactly
}

This allows picking which definitions should be strict, it doesn't break any existing code but allows those who can be bothered making the signatures match to be happy. If I don't need to use some data I just ignore it in the implementation.

A benefit of doing this would be if I were to add a parameter in the future to a function definition it forces the implementation signature to get updated (I can be bothered to do that), it then means I have all of the information available to me as opposed to going hunting to find what I need and if it happens you only want to use the third parameter you have to include the first two anyway right.

I'm ignorant to how complex this may be to solve but are still confused as to why its not being added?

@RyanCavanaugh
Copy link
Member

I'm ignorant to how complex this may be to solve but are still confused as to why its not being added?

It's not complex, it's simply that we still don't think this check is a good idea.

If there's some angle not covered by the dozens of issues about this already, I'm happy to discuss, but we really have thought about it many times, and really do still believe this kind of check is not the right thing to add to the language.

@richardbullin
Copy link
Author

Please have another think about this guys! I'm on my knees!

This has caused issues for me and obviously a lot of others, adding this functionality doesn't break existing code and like you say its easy to implement, it may not be a priority but I guarantee going forward your always going to have people requesting something similar.

Its more about making sure that the implementation meets the expected design (remember the person implementing something might not be the person who designed it). Just because no runtime error is generated does not mean the implementation meets the expected design.

abstract class TChecker
{
  public abstract Check(objectToCheck: Object); // Suggest allowing addition of 'strict' keyword here
}

class TNumberChecker extends TChecker
{
  public Check() // Currently compiles with no error, having an error here would help guide the developer
  {
    //
  }
}

Most of the responses to the other requests can be summarized to "please read our FAQ"

A function with less parameters could be a valid implementation.

This is true. But a function which implements the exact number of parameters is also a valid implementation. My suggested solution is an opt-in approach by marking the line with a keyword such as 'strict'.

Cant use '?' as optional operator as the value is actually there

I agree with this and either a new character should be used to mark if done per parameter but I'm guessing marking the whole line is sufficient. Could also allow marking an entire class or interface so that all functions inside are automatically 'strict' as well as saying not strict so you could mark and entire class and then mark one or two functions as not strict

strict abstract class TChecker // strict keyword here makes all functions inside it strict by default
{
  public abstract Check(objectToCheck: Object); // This would be strict
  public ForEach(); !strict // This is not strict
}

It would be burdensome to have to explicitly declare unused parameters.

As its an opt in solution, its only those who want/need to do this and the intention is that its used for those cases where you should be using the parameters.

Also by specifying strict callback definitions another developer cant accidentally use the wrong callback and the code insight suggestions are smaller.

@typescript-bot
Copy link
Collaborator

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

@RynerNO
Copy link

RynerNO commented Jun 7, 2021

Is this ever be implemented or is there some way to do this right now? Because it looks like abstract methods are useless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

5 participants