-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Implement private variables, getter/setters and methods using Symbols #684
Comments
🌟 🏆 for the exceptionally complete write-up and accompanying PR. Showing emit for static members might be useful. Note that instance and static members of the same name don't conflict -- should they re-use the same symbol variable if that happens? Re: making access of private members impossible, that seems more like a feature than a deficit. It's certainly what a lot of people have been asking for. |
D'oh. I did test this on code that has private statics to make sure they were handled, but I didn't think about a static and non-static with the same name. Technically, re-using the same name won't hurt - the TS source still has its own type-check pass that can distinguish between a reference to the static member and a reference to the instance member, so even if the final JS ends up using the same symbol it won't hurt. But since tsc is in complete control of the codegen anyway, it might as well generate a new name for it. I've updated the OP. |
I struck out the part about interop breaking if parts of the final JS were compiled with different values of the switch. This isn't a problem right now because all access to privates must be within the same class, and a class cannot be split across files (as of now). If open classes are implemented in the future, then this change will require that all files that contain parts of the same class be compiled with the same value of the switch. |
👍 for clarity! A few practical considerations:
So in a runtime that doesn't have a native |
Actually symbols will minify better. With the exception of Google Closure Compiler, I believe all minifiers do not rename private properties because there is no way to know when a property is private or being accessed via a string indexer. (GCC is able to do so in advanced mode because it is a whole program optimizer and so treats everything as rename-able.) With symbols, the indexer is a variable Points about the perf impact are completely valid. Edit: To clarify, the slow-down comes not from the use of Symbols but from the use of a variable indexer to access properties instead of dot-notation or a literal indexer. I edited @NoelAbrahams 's instantiation test into http://jsperf.com/privates-using-symbols-vs-current-codegen/5 to strip out everything except the constructor and the symbols it uses. Perf impact was unchanged. I then added two more classes, ClazzWithIndexer that uses string indexers to set the properties, and ClazzWithIndirection that uses variable indexers to set the properties. This is similar to @NoelAbrahams 's test except that it doesn't even involve the call to __symbol to generate the name. Perf for ClazzCurrent and ClazzWithIndexer is identically excellent. Perf for ClazzWithIndirection and ClazzWithSymbols is identically terrible. I did verify in V8 profiler (using I've opened a thread on v8 mailing list about it. |
|
Closing this given previous discussion and where we are with Symbols in general. @Arnavion had a nice PR for this linked earlier where upon further inspection we all agreed: The general consensus is to wait and see how classes and symbols get used together. Depending on how the future shapes up we can revisit this or start a new proposal based on what we learn. |
Compiler interface changes
-symbolForPrivates
Codegen example
TS source (unchanged)
JS output
Footnotes
__symbol
This is a wrapper around runtime's implementation of Symbol if it has one (global.Symbol, where global is found using eval), or a function that just returns the name string if it doesn't. For example, in a runtime that doesn't have Symbol, __symbol("_var1") will become "_var1", which is functionally identical to the current TS codegen.
I based the definition of __symbol around __extends, but it has these differences:
The variables declared to hold the Symbols are named as
"__symbol_" + originalMemberName
for instance members, and"__symbolstatic_" + originalMemberName
for static members. They are defined in the class closure, so their names are not visible in an outer scope. They are also only used at the class closure scope, so their names are visible but do not collide with any same-named variables inside constructor / method / getter / setter bodies.Of inheritance and same names:
Changes in interop with existing TS code / with existing JS libraries with .d.ts files
This is a whole program change, so everything that has private members or private member access will need to be recompiled. This doesn't affect third-party JS libraries that provide .d.ts files because .d.ts don't contain private members.
But any library that's provided as TS source for development but deployed as separately-compiled JS source will break unless it's recompiled with the same switch.For example, suppose a hypothetical library L that provides an L.js file and an L.d.ts file. In this case there's no problem, because L.d.ts will not contain any private member declarations. Only classes defined in foo.ts will have private members and private member access using Symbol.
But suppose the library provided an L.ts source instead of L.d.ts, and also a pre-compiled L.js that they compiled themselves (for example, they might provide a minified L.min.js, expecting the user to develop their code against L.ts but deploy the official L.min.js to their website). This L.ts source would contain private members.
Then the user must make sure they compile L.js themselves and use that, not use the L.js provided by the official source.On second thought, since privates cannot escape the class scope and classes can't be split across files as of now, this isn't a problem. This will however be a problem if open classes are implemented in the future (i.e., all files that contain parts of an open class must be compiled with the same value of the switch).As a result of this change, the following will have different behavior:
for key in base, derived[key] = base[key]
) will not enumerate symbol properties.Open questions
this.__symbol
first in the declaration of__symbol
?__symbol
? The presence ofeval
will cause the runtime to disable optimization for the whole scope, which is the whole file.impossibledifficult -(<any>obj)._privateMember
will no longer work. One can try to hack around it by using Object.getOwnPropertySymbols -(<any>obj)[Object.getOwnPropertySymbols(obj).filter(function (symbol) { return symbol.toString() === "Symbol(_privateMember)"; })[0]]
(obj.__proto__
for methods, getter and setters)but relying on(Relying on the output ofSymbol.toString()
this way is undefined behavior.Symbol.toString()
is defined behavior.) Should there be an escape hatch? A way to disable this rewrite for some files or some classes?this._method()
) to something completely different (this[__symbol__method]()
) based on the result of a type evaluation. In that respect, perhaps it'd be better to have the user explicitly write out the code that creates Symbols and indexes using them. However that would then require the compiler to understand Symbol specially so that it can provide type information.The text was updated successfully, but these errors were encountered: