Correct super property access is loose mode#3536
Correct super property access is loose mode#3536kriszyp wants to merge 4 commits intobabel:masterfrom
Conversation
… on calls, but revert to spec handling for property access/assignments. Fixes T7389
Current coverage is 88.17%@@ master #3536 diff @@
==========================================
Files 193 193
Lines 10072 10203 +131
Methods 1553 1558 +5
Messages 0 0
Branches 2199 2287 +88
==========================================
+ Hits 8875 8997 +122
- Misses 1197 1206 +9
Partials 0 0
|
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
@kriszyp Thank you for this PR and sorry if the review took... well... a bit too long 😅
This PR needs to be rebased and I left some comments. If you prefer, I can do it myself.
|
|
||
| _Foo.prototype.test; | ||
| _Foo.prototype.test.whatever; | ||
| babelHelpers.get(Object.getPrototypeOf(Test.prototype), "test", _this); |
There was a problem hiding this comment.
Since we are in loose mode, here it would be better to use babelHelpers.get(_Foo, "test", _this).
There was a problem hiding this comment.
That would be accessing the static test property wouldn't it? The reason for using Test.prototype is to actually simulate super property access, not just normal property access.
There was a problem hiding this comment.
Whoops, you are right. I meant babelHelpers.get(_Foo.prototype, "test", _this).
Since we are in loose mode, we can assume that the user didn't modify Test's prototype, so this is true:
class Foo {}
class Test extends Foo {}
console.log(Object.getPrototypeOf(Test.prototype) === Foo.prototype);| _Foo.prototype.test; | ||
| _Foo.prototype.test.whatever; | ||
| babelHelpers.get(Object.getPrototypeOf(Test.prototype), "test", _this); | ||
| babelHelpers.get(Object.getPrototypeOf(Test.prototype), "test", _this).whatever; |
There was a problem hiding this comment.
Can you also add a test for super property setting?
|
Would be also great to include |
|
I do appreciate you getting it to this, but yeah, after 18 months, we have already moved onto TS and this is pretty rusty in my recollection. I'll take a look at the comments though. |
|
Just let us know if you want to finish this or should we take over, sorry for letting this hang for so long. |
|
rebased in #7553 |
In loose mode super property access is wrong, accessing and changing properties on the prototype instance instead of
this:https://phabricator.babeljs.io/T7389
This fixes super property access, by using fast/loose lookups of methods on calls, but reverts to spec handling for property access/assignments, where it is necessary to call the setter/getter on the correct
thisinstance.EDIT @aaronang:
Fixes #4312