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 access to protected class members if the 'this' type of an (extension) function is specified #10302

Closed
lhecker opened this issue Aug 12, 2016 · 14 comments
Labels
Committed The team has roadmapped this issue Effort: Difficult Good luck. Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@lhecker
Copy link
Member

lhecker commented Aug 12, 2016

First of all: Please excuse me if this issue is a duplicate of some sort, but I've searched for quite some time for a similar issue or at least an answer to the following question/bug/proposal and found nothing.

If I specify that a function has a certain (customized) 'this' type I expect that the function has full access to protected or even private fields of passed instances. Reason being that due to JavaScripts dynamic nature and it's capability to overwrite the this pointer, a function that is freestanding can still act like an extension method to a class.

Below you can find a simple JavaScript-like style of extending a existing class, which fails in TypeScript, because it strictly forbids access to private and protected fields outside of the actual class:

class MyClass {
    protected p: number;

    constructor() {
        this.p = 123;
    }
}

interface MyClass {
    extension(p: number): void;
}

MyClass.prototype.extension = function (this: MyClass, p: number) {
    // FAILS with:
    //   error TS2445: Property 'p' is protected and only accessible within class 'MyClass' and its subclasses.
    this.p = p;
}

const instance = new MyClass();
instance.extension(456);

I realize that some kind of "official" class extension technique will get into TypeScript sooner or later, but all of the proposals I've seen extend the actual JavaScript language with new keywords. This question/bug/proposal here though, basically does exactly the same as the existing extension proposals but within the boundaries of the JavaScript language (apart from the existing language extensions for the type checker).

I thus humbly suggest to loosen this strict access control and allow us to write extension methods to at least some extend before v2.0 is finally released.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 12, 2016
@zpdDG4gta8XKpMCd
Copy link

thumb down: this undermines the purpose of private as a way to hide the internal state

@lhecker
Copy link
Member Author

lhecker commented Aug 15, 2016

@Aleksey-Bykov If your current function has access using this to an instance shouldn't you consider that function an extension of the class itself? I mean: I'm well aware that this can undermine the isolation of members when using functions like .map or .forEach, but usually the only time in JavaScript where you explicitly override the this reference is when you want to extend something - usually even in the way my example showed above. At least I can't remember the last time I overrode the this reference just for the heck of accessing private members. Would anyone else do it?

As another point: There is also an escape hatch for type casting in TypeScript by casting something to any - that undermines private in the exact same way (just much less comfortably).

So yeah: I personally consider every function whose this type is a class an extension of that class. JavaScript's dynamic nature allows us to do so to write compact code, so why forbid it? Maybe I'm wrong but right now I believe the advantage for having the above would be far greater than the cost of it.

@zpdDG4gta8XKpMCd
Copy link

why do we need private if it doesn't protect your internal state?

what you are suggesting is that any 3rd party code can inject some logic into your class and feel like home there

public and protected members is the answer to your question, if you meant something to be seen to 3rd party code then you make it either public or protected, the private modifier is for your eyes only

@lhecker
Copy link
Member Author

lhecker commented Aug 15, 2016

what you are suggesting is that any 3rd party code can inject some logic into your class and feel like home there

Well first of all that's already possible since private is only a TypeScript-specific annotation, but does not affect actual visibility of object fields, which makes this argument kinda moot. If you're talking about directly integrating 3rd party code into your actual local project, then yeah: That would be awesome! If I integrate such code in my project with the intention of e.g. extending my class I want my class to be extended.

public and protected members is the answer to your question

protected is the same as private in this case, because you can't access protected members from outside of the class (or it's derivatives) either. If you keep in mind how deriving a class actually works in JavaScript, you'll see that there is basically no difference if 3rd party code derives the class. This means: No manual extension of the prototype (or having other freestanding functions), or generating member functions dynamically.

Just take my use case: I have 2 classes for a certain parser. One of the central classes is about 4000 LOC. What I did is to split up the class in a "core" and multiple cleanly seperated extensions, but now I've got the problem that all members have to be public, or otherwise I can't access them without <any>. The second class is only 300 LOC, but if I'd replace my "hacky" function generator with regular member functions I'd easily end up with at least 10 times the code of which everything is redundant - just with slightly adjusted parameters. If this suggestion gets through I'd lose nothing while finally being able to replace those <any> hacks.

@zpdDG4gta8XKpMCd
Copy link

so what? are you suggesting to legalize a hack (accessing a member which is not supposed to be accessed)

the fact that you still can do it from JavaScript is that TypeScript is ahead of what's coming next in ES2016, 2017 or whatever

if the class is your own - you have it all you don't need this hack
if the class is of someone else - you have no business to see what's there, because it is not yours

protected is not the same as private because protected members can be legally accessed in a derived class

i hear that you have problems with the size of your class, you options as always are:

  • composition: breaking down a single class into multiple smaller ones passing private state via constructors
  • IoC: when you can inject your extensions and push necessary private context into them via parameters

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Aug 15, 2016

// composition
interface PrivateMatters { x: number }
class A {
    constructor(
         private state: PrivateMatters
    ) {
         this.b = new B(this.state);
         this.c = new C();
    }
    public doThis() {
         this.b.doThat(); // state is already in B
         this.c.doThis(this.state);
    }
}

// ioc
class A {
    constructor(
         private state: PrivateMatters
    ) {
    }
    public legallyUsingPrivateState<r>(extension: (x: number) => r): r {
         return extension(this.state.x);
    }
}

@lhecker
Copy link
Member Author

lhecker commented Aug 15, 2016

so what? are you suggesting to legalize a hack

It's not a hack because it would imply that you can do something that you couldn't do before. And yes I'm advocating for this obviously or why should I have created the issue then?

protected is not the same as private because protected members can be legally accessed in a derived class

Would you be more comfortable with the proposal if I'd edit the title to read protected instead of private?

i hear that you have problems with the size of your class

Your legallyUsingPrivateState method is kinda bad, because it can be accessed by "3rd party code" to do anything with the private state. But okay... Let's assume that the method was a private one and that the A class always initiates the call to an extension: It still doesn't work for accessing private/protected member functions, which takes up the majority of my code (obviously). If you want to do that you'd have to make PrivateMatters a class and move those methods to it. What you might realize now is that you have the same situation as before: Your PrivateMatters is what A was before and your A class now is only a wrapper around PrivateMatters to protect it from unwanted access...

Is it really so surreal to consider that there might be classes larger than a dozen LOC? Being able to split those up is one of the major benefits of extensions.

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Aug 15, 2016

it is a hack, because you cannot access a private member from outside of a class without waiving typechecking which is what TypeScript is for

yes, protecteds would be a better fit for your proposal

it's not bad as long as you

  1. guarantee read-only access (ok you know my internal state, so what?)
  2. do not allow extension to do direct uncontrolled writes to it however still permit it via taking the result of the extension and placing it into the private state:
public legallyUsingPrivateState(extension: (x: number) => number): void {
    const result = extension(this.state.x); // be aware of my private x if you can't help it
    if (result > 0) { // let me check what you want to write back
         this.state.x = result; 
    } 
}

not being able to decompose a 4000 LOC class into smaller classes is a bad sign that your code is overlycoupled, you need to reconsider the design

@lhecker
Copy link
Member Author

lhecker commented Aug 15, 2016

The discussion right now is seriously drifting away from the original issue, which makes this my last comment about this.

  1. guarantee read-only access (ok you know my internal state, so what?)

You forgot about the member functions.

  1. do not allow extension to do direct uncontrolled writes to it

What? Should I special-case every single access? The code size just doubled for no apparent reason. Or should I do the FP way, make the state object immutable and return a new one from the extension to diff it with the previous one? While that would certainly solve it I don't see how that would make it any better. Oh and runtime cost: It's a parser. Those have to fast. Deep comparisons and/or modification bookkeeping? Not fast.

not being able to decompose a 4000 class into smaller classes is a bad sign that your code is overlycoupled, you need to reconsider the design

Just a minor nitpick: There are about 50 different packet types in my case and they all belong to the parser class. Even if you make a rough estimate and divide 4000 / 50 you only get about 80 LOC per "sub-parser" which isn't that surprising, isn't it? And it shouldn't be so surprising that I don't want to put all of that into a single file either.

Anyways: You can't just split up some things into multiple classes without causing either a serious overhead, increased complexity, increased code size, and/or violation of some core principles like DRY. In fact I'd argue that my code is as little coupled as possible because every sub-parser is only depending on the core and on nothing else. What you probably suggest is that I should do some form of indirection between the "core" and the parts (like IoC using a seperate state object), which just doesn't solve the general problem at hand of having the capability to extend a class in a dynamic way, since this is a dynamic language.

I'm interested if you can come up with a software pattern that doesn't require the main class to be wrapped for access control (which is pointless for a user-facing class, in a language which gets compiled to JS anyways), doesn't sacrifice performance and allows me to keep my code DRY in a situation where the main class is composited out of a large number of classes with shared state.

Don't go around assuming that others don't know their craft.

@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Aug 15, 2016

you can extend your class in a dynamic way as much as you want provided you do it via:

    1. public interfaces
    1. carefully guarded facilities over the internal state (as was shown)

there are languages like C# and Java where hacking it to the private state (although still possible) is a hands down bad practice no questions asked, and nevertheless people are still able to craft decent parsers without running into problems like yours

i cannot give you a better advice without looking at the code

bottom line, even if everything you said is true and there is simply no way better to do it, it's not a reason for a feature that everyone else can live without, bad luck

@lhecker lhecker changed the title Allow access to private class members if the 'this' type of an (extension) function is specified Allow access to protected class members if the 'this' type of an (extension) function is specified Aug 15, 2016
@RyanCavanaugh
Copy link
Member

Allowing access to private is in direct contradiction to the dozens of bugs we've had requesting we emit (impossible) code to efficiently make private things truly inaccessible. protected makes a lot of sense, though.

@lhecker
Copy link
Member Author

lhecker commented Aug 15, 2016

@RyanCavanaugh Yup I edited the title and example already. I really agree that protected fits the narrative of an extension much better. I think if implemented it could solve most of the pain for the people in the other issues wishing for partial/extendable classes, while still being quite a minor change.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Aug 22, 2016
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 2.1 milestone Aug 22, 2016
@RyanCavanaugh
Copy link
Member

👍 : treat functions annotated (this: T ...) { as if they were inside the lexical scope of T's class declaration for the purposes of protected members

@mhegazy mhegazy modified the milestones: Community, TypeScript 2.1 Sep 29, 2016
@mhegazy mhegazy modified the milestones: Community, TypeScript 3.0 Jun 11, 2018
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jun 11, 2018
@lhecker
Copy link
Member Author

lhecker commented Jun 11, 2018

@Kingwl @mhegazy Thanks for tackling this issue. 🎉🎉🎉
Sadly I'm hardly ever writing TS anymore nowadays, but I'm so glad you could solve this for me (and others)! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Effort: Difficult Good luck. Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants