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

Class private members accessible from outside with brackets notation #19335

Closed
marcomura opened this issue Oct 19, 2017 · 8 comments
Closed

Class private members accessible from outside with brackets notation #19335

marcomura opened this issue Oct 19, 2017 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@marcomura
Copy link

marcomura commented Oct 19, 2017

I'm concerned about private members being "legally" accessible from outside the class, since it could lead people to believe it's an acceptable way for doing it in production code.
Currently, my team believes this is an acceptable way for unit testing code.

This approach even detects the correct private member's type, so it's not a workaround to access the raw members through upcast to any...

TypeScript Version: 2.4

Code

class Test {
    constructor(private _x: number) { }
    getValue(): number { return this._x; }
}
let t = new Test(10);
t["_x"] = -6;

Expected behavior:
Compiler error: "Property '_x' is private and only accessible within the class 'Test'".

Actual behavior:
No errors.

@ghost ghost added the Bug A bug in TypeScript label Oct 19, 2017
@mhegazy mhegazy added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Oct 19, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Oct 19, 2017

The string index notation has been an escape hatch to allow users to get hold of the private members, e.g. to use them in unit tests, etc.. it is a slightly more type safe alternative to (t as any).x.

Our experience shows that users do not stumble into this by mistake.

@marcomura
Copy link
Author

marcomura commented Oct 20, 2017

Ok, I understand.
Can we have at least a compiler option to disallow this? I'd like to force this pattern to throw a compiler error in my codebase.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 20, 2017

Can we have at least a compiler option to disallow this? I'd like to force this pattern to throw a compiler error in my codebase.

I do not think ppl write x["privatePropertyName"] by mistake, same like (<any>x).privatePropertyName. The compiler catches the other ways of accessing/aliasing a property, e.g. x.privatePropertyName and y = x. If a user explicitly writes x["privatePropertyName"], it is not something they stumble into my mistake, they know what they are doing, and they are using this as a way to reach in and get the private member; and for that, we would rather allow it in a type sage manner than forcing them to cast to any.
Not sure what the flag would give you here.

@marcomura
Copy link
Author

Allowing it in UTs instead of casting to any is actually a good thing, and I agree this semantic is a good thing (even if I would have defined a specific different syntax for it, like a built in mapped type for exposing private fields).
But there are so many ways people could put that syntax by mistake in production code... the simplest one I could think of is people copying/pasting snippets from UT/somewhere-else without double checking what exactly they are pasting.

I'm not going to discuss philosophical topics about human behaviors, I would just feel more safe if that syntax can be disallowed by a compiler option.

Thanks.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 6, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Nov 6, 2017
@czhower
Copy link

czhower commented Nov 10, 2017

IMO there are so many holes already because of JS's nature this is a piddly one. And the reason its not done is that it would require either:

  1. Static checking of string literals which could be easily bypassed by using an intermediary variable.

  2. Run time code, which would bloat things for little benefit.

tslint can detect this (not private though, but any access) and issue an error. However it too can be bypassed by using an intermediary vale.

@marcomura
Copy link
Author

marcomura commented Nov 12, 2017

I'm not worried about holes... I'm perfectly fine that stuff like (o as any).privateMember exist, because the code itself makes very clear it's a workaround not acceptable in production.
But if stuff like o["privateMember"] is allowed and even correctly typed, then someone could think it's an acceptable way to write code for production.

I understand reasons why that syntax should stay allowed, but I honestly don't understand how can someone refuse to understand that people would like to disable it with a compiler option.

@czhower , I'm not afraid of (1.) because it would return a value of type any.
And (2.) of course is not an acceptable solution.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
@orta
Copy link
Contributor

orta commented Apr 15, 2021

As this just came up in Discord, private class fields was implemented in JavaScript and then added to TypeScript in 3.8 - https://www.typescriptlang.org/play?#example/private-class-fields

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