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

Suggestion: Permit an implementing class to ignore private methods of the implementee class #471

Closed
NoelAbrahams opened this issue Aug 18, 2014 · 30 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Revisit An issue worth coming back to Suggestion An idea for TypeScript

Comments

@NoelAbrahams
Copy link

Hi, we have a requirement to create mocks of real classes defined in code. The exact requirements are

  • A specific mock should define the public API of the real class.
  • If the public API of the real class changes in any way then the compiler should detect the corresponding breakage in the mock class.

A possible solution might be as follows:

declare var fileWriter: any; 

// Real class
class Foo {

    public writeToFile(){
        fileWriter.writeToFile('');
    }
}

// Mock
class MockFoo implements Foo {

     public writeToFile(){
         // do nothing
     }
}

This appears to solve the problem because changing Foo.writeToFile will trigger a compilation error along the lines of "Class MockFoo declares interface Foo but does not implement it..."

The problem with this approach is that class Foo is not permitted to have any private methods or fields. If we were to add a private method foo() to class Foo then it's no longer possible for MockFoo to implement Foo because the compiler doesn't permit it.

I suggest that when a class implements another class the implementing class be allowed to ignore the private fields and methods (both static and instance) of the implementee.

(I am aware that there are workarounds, such as declaring an interface that both Foo and MockFoo implement, but that introduces an unnecessary maintenance overhead.)

@danquirk
Copy link
Member

Is there a reason MockFoo must implement Foo and not extend it?

@NoelAbrahams
Copy link
Author

@danquirk, yes, it would be ideal for MockFoo to extend rather than implement Foo.

However, that doesn't satisfy the second requirement:

If the public API of the real class changes in any way then the compiler should detect the corresponding breakage in the mock class.

To elaborate, suppose we now extend Foo:

class MockFoo extends Foo {

     public writeToFile(){
         // do nothing
     }
}

Somewhere else in code, a function that we wish to test:

function bar(foo: Foo){
    foo.writeToFile();  
}

Here's a test case for function bar:

function test(){
    bar(new MockFoo()); // All good
}

Following on from this, if in the next iteration we carry out the following refactor:

class Foo {
   // Renamed from "writeToFile"
    public writeToFileSync(){
        fileWriter.writeToFile('');
    }
}

The compiler does not issue an error for bar(new MockFoo()), and the test case will end up executing the live method.

Viewed from a different perspective, the compiler isn't able to issue an error, because the deriving class has no mechanism to indicate that it is overriding a base class method. If such a mechanism were to exist then that would also solve this problem.

@NoelAbrahams
Copy link
Author

A possible solution that occurred to me was to define MockFoo as:

class MockFoo extends Foo {

     public writeToFile(){
         super.writeToFile;
         // do nothing
     }
}

Apart from the slightly hackish nature of the solution, there was a problem with this, which I cannot recall...

@ivogabe
Copy link
Contributor

ivogabe commented Aug 20, 2014

You could also create an interface IFoo:

interface IFoo {
    writeToFile(): void;
}
class Foo implements IFoo {
    // ...
}
class MockFoo implements IFoo {
    // ...
}
function bar(foo: IFoo) {
    foo.writeToFile();
}

@NoelAbrahams
Copy link
Author

@ivogabe, that's a step that we're trying to avoid as I mentioned right at the end of the original post.

@danquirk, it will be interesting to hear the argument against relaxing the need to implement private methods.

@RyanCavanaugh
Copy link
Member

The problem is that if you have a private field, MockFoo really is not a Foo:

class Foo {
    private p = 'hello';

    static doSomething(n: Foo) {
        console.log(n.p);
    }
}
var  x = new MockFoo();
Foo.doSomething(x); // Fails

We don't want to water down the meaning of implements to mean something other than what it is.

@NoelAbrahams
Copy link
Author

That's a fair point.

On the other hand, I cannot see any use-cases for the idea of "one class implementing another class", because it's limited to only those classes that don't have any private fields or methods - which are not really common in the real world.

Perhaps we could invoke this design goal:

[Not] Apply a sound or "provably correct" type system. Instead, strike a balance between correctness and productivity.

To me the idea of being able to use the public API of a class as an interfaces is quite appealing; and probably has many interesting use-cases in the real world.

@RyanCavanaugh
Copy link
Member

On the other hand, I cannot see any use-cases for the idea of "one class implementing another class", because it's limited to only those classes that don't have any private fields or methods

This isn't true. The useful example for this is:

class Animal {
    private something;
}

class Dog extends Animal {
    woof() {}
}

// Error, Wolf doesn't have 'woof' method
class Wolf extends Animal implements Dog {
}

@NoelAbrahams
Copy link
Author

I'm not entirely convinced that that example has significantly increased the usefulness of "implementing a class", because now Dog can't have any of its own privates (even though in the real world dogs do have privates 😃 ).

@NoelAbrahams
Copy link
Author

The other problem that I mentioned with extending the real class was to do with the fact that if the real class had complex constructor parameters then that would be inherited by the mock class. Ideally one would want to simply write var x = new MockFoo() and not have to bother with complex construction. Essentially, we just want MockFoo to respond to the public API of the real class and that behaviour is ideally achieved if we can treat the real Foo as any other interface.

@RyanCavanaugh
Copy link
Member

Tagging Suggestion, but understand we're unlikely to make a breaking change here. Other languages use interfaces for this scenario and I think that's still an appropriate solution for TypeScript. We'd need a compelling use case here along with a non-breaking design proposal.

@mwisnicki
Copy link

Why not simply allow implementing private members of class ?

class Foo {
  private x = 1;
}
class MockFoo implements Foo {
  private x = 1;
}

This is less nice than being able to ignore private members but at least you can do mocks and it doesn't break anything.

@NoelAbrahams
Copy link
Author

Why not simply allow implementing private members of class ?

Yes, this would be an improvement on the current situation, because as it stands the idea of "implementing another class" is not useful.

@danquirk
Copy link
Member

Note #1101 asks the same question of protected members.

@aciccarello
Copy link

I am still trying to understand what the language of implementingClass implements baseClass means and am not following why private attributes need to be in the implementing class.

Can someone reiterate why you can't just "ignore private variables when implementing interfaces"?

I agree with @NoelAbrahams that I look at interfaces as a public API definition.

To me the idea of being able to use the public API of a class as an interfaces is quite appealing; and probably has many interesting use-cases in the real world.

@DanielRosenwasser
Copy link
Member

Can someone reiterate why you can't just "ignore private variables when implementing interfaces".

Even though it isn't distinguishable whether a type has privates from outside the class, an instance of Foo is allowed to access the privates of any other Foo. For this reason, it needs to be the case that when implementing Foo, the privates need to be there or else a method in Foo using the private of another Foo will fail.

See @RyanCavanaugh's example in the above #471 (comment).

@aciccarello
Copy link

@DanielRosenwasser Okay thank you. That clarifies it for me. I saw that example but was confused because I was under the impression that Foo could not access the private variables of another instance.

@benliddicott
Copy link

I've opened a related issue #3854 which would solve the originators motivating problem.

@benliddicott
Copy link

see alsp #4278

@benliddicott
Copy link

This is NeedsProposal. Here's a proposal which would allow the workaround posted to #3854 and #4278 work for private members as well as protected members.

Proposal: Allow an extending class to modify the protection of a private member.

Currently an extending class can modify a protected member to be public:

class X{
    protected m1: string;
}
class Y extends X {
    public m1: string;
}

The proposal is to allow the same syntax to expose a private member as protected or public.

class P{
    private m1: string;
}
class Q extends P {
    protected m1: string; // Currently an error
}

@RyanCavanaugh RyanCavanaugh removed the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label Nov 9, 2015
@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision Revisit An issue worth coming back to and removed In Discussion Not yet reached consensus labels Mar 15, 2016
@RyanCavanaugh
Copy link
Member

These are reasonable use cases, but the problem is that we don't want class X implements Y { ... } to produce an X that is not assignable to Y (as would be the case if this worked as proposed).

If we get a general notion of type operators (many issues wanting this), a simple operator e.g. publicShapeOf could solve these use cases.

@Elephant-Vessel
Copy link

I'm trying to understand the motivations regarding the language design here. Again I wish to reduce brain pain, hope you don't mind.

I do understand why privates currently need to be exposed, as the scope of the privates are bound to the class and not the instance. I don't understand, considering the consequences, why the scope of privates are bound to the class and not to the instance.

Is there any significant value in that a Foo can manipulate the implementation details of any Foo? Something that makes up for the non-intuitiveness and the disturbed ability of the language to easily provide abstractional reusability that comes from mixing in privates with types/interfaces?

I would like to be able to explain to my colleagues what makes this strange deal with exposed privates worth it after all.

@Elephant-Vessel
Copy link

Why not simply allow implementing private members of class ?

Yes, this would be an improvement on the current situation, because as it stands the idea of "implementing another class" is not useful.

Although that would further complicate this whole concept of privates and what it actually means.

Same thing with letting subclasses upgrade private and protected fields. Wasn't even aware of that subclasses already can update protected fields, feels like a complete violation of why we have those modifiers in the first place. Feels like this can get messy.

@RyanCavanaugh
Copy link
Member

@Elephant-Vessel

I do understand why privates currently need to be exposed, as the scope of the privates are bound to the class and not the instance. I don't understand, considering the consequences, why the scope of privates are bound to the class and not to the instance.

Certainly TypeScript is not the first language deciding to scope private to the type rather than the instance. The trade-offs are fairly obvious IMO. Probably it's going to be easier to find discussion of the merits of this when applied to Java, C#, C++, etc. rather than us going over that again here.

@Elephant-Vessel
Copy link

@RyanCavanaugh

Certainly TypeScript is not the first language deciding to scope private to the type rather than the instance. The trade-offs are fairly obvious IMO. Probably it's going to be easier to find discussion of the merits of this when applied to Java, C#, C++, etc. rather than us going over that again here.

Thank you for your response Ryan.

Sure, but TypeScript differs from those languages as it's structurally typed. And that means that we suddenly have this problem with privates having to be propagated in the type system, a problem that these other languages do not experience.

I personally believe that solid abstractural integrity is significantly more important than allowing a Foo to mess around with the internals of other Foos. But if the reason for this is that the other languages do it, then I at least know what to say to my colleagues. Brain still not happy, but less pain. Thanks.

@kitsonk
Copy link
Contributor

kitsonk commented Oct 10, 2016

I personally believe that solid abstractural integrity is significantly more important than allowing a Foo to mess around with the internals of other Foos.

I don't think a class having access to/visibility of other instances breaks encapsulation at all. The proposal for ES private properties also includes this class level visibility. It is simply a different form of logical state which the class has access to. I like to think of this private state as a class visible WeakMap, and in fact for @dojo this is actually what we do with our private state, versus using private properties, which ensures at run-time private is really private, as we don't leak our WeakMaps outside of the module the class is defined in.

The main use case is that the class having access to the private properties allows for static methods to potentially operate on instances without needing to break this encapsulation.

@Elephant-Vessel
Copy link

Elephant-Vessel commented Oct 11, 2016

@kitsonk
Sure, in regards of design constraints the current policy is more a nuisance in regards of reusability than breaking abstractions*. But I think it makes the conceptual model of the language unnecessary convoluted as we now have this unintuitive behavior and the mixup of public contracts with internal implementation within abstractions. How easy wouldn't it have been if we had instance privates instead of class privates, none of this would have been an issue, and the conceptual model would be less complex. May it be that those static methods would have to be instance methods instead.

* And then we have this stuff about promoting protected members, which I'm really curious about why we got in the first place, and the suggestion about doing the same thing with private members. Talk about breaking encapsulation. For a language whose selling point is to be able to scale, it doesn't feel good at all.

@ricklove
Copy link

I like the idea of publicShapeOf where can I track that?

If we get a general notion of type operators (many issues wanting this), a simple operator e.g. publicShapeOf could solve these use cases.
From: #471 (comment)

@AlexGalays
Copy link

If this problem is going to remain, you should at least prevent class from "implementing" other classes, imo.

@DanielRosenwasser
Copy link
Member

Since mapped object types strip out private/protected fields (though they also strip out call/construct signatures), @wycats had the idea of writing the following to achieve this:

type Interface<T> = {
    [P in keyof T]: T[P]
}

class C {
    private foo(): string;

    bar(): number;
}

// Look ma, no 'foo'!
class D implements Interface<C> {
    bar() {
        // ...
    }
}

I've also called that mapped object type PropsOf, and you could call it PublicOf if you so desired.

@microsoft microsoft locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Declined The issue was declined as something which matches the TypeScript vision Revisit An issue worth coming back to Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests