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

Fix bugs in the _wrapNativeSuper helper #7533

Merged
merged 10 commits into from May 17, 2018

Conversation

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 9, 2018

Q                       A
Fixed Issues? Fixes #7506
Patch: Bug Fix? yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 9, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8008/

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:bug-7527 branch from 72e1743 to 83f1a34 Mar 9, 2018
@nicolo-ribaudo nicolo-ribaudo changed the title [typeof-symbol] Guard against undefined built-in globals Fix bugs with the _wrapNativeSuper helper Mar 9, 2018
@nicolo-ribaudo nicolo-ribaudo changed the title Fix bugs with the _wrapNativeSuper helper Fix bugs in the _wrapNativeSuper helper Mar 9, 2018
@@ -472,7 +472,7 @@ helpers.wrapNativeSuper = () => template.program.ast`
constructor: {
value: Wrapper,
enumerable: false,
writeable: true,

This comment has been minimized.

Copy link
@existentialism
@@ -0,0 +1,4 @@
{
"plugins": [],
"presets": ["es2015"]

This comment has been minimized.

Copy link
@existentialism

existentialism Mar 9, 2018

Member

Should this use "../../../../lib"?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 9, 2018

Author Member

Right!

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:bug-7527 branch 2 times, most recently from 43fcd6c to 357cd1a Mar 9, 2018
@@ -0,0 +1,4 @@
{
"plugins": [],
"presets": ["../../../../lib"]

This comment has been minimized.

Copy link
@Andarist

Andarist Mar 9, 2018

Member

shouldnt it be possible to specify preset name here instead of the hardcoded path?

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 10, 2018

Author Member

I don't know, in the other tests of this package it is done like this.
We should modify our tests to accept real plugin and preset names anyway (with @babel/plugin-*).

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Mar 10, 2018

While we're here, want to fix the extends null bug on Line 461?

Copy link
Member

jridgewell left a comment

Actually, we have bigger bugs... Try running the output of this through Chrome:

class CoolElement extends HTMLElement {
  connectedCallback () {
    this.innerText = 'Hello World';
  }
}

customElements.define('cool-element', CoolElement);
const c = document.createElement('cool-element');
@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Mar 10, 2018

Test Case:

let called = false;
window.Array = function() {
  called = true;
};

class List extends Array {
  push(value) {
    super.push(value);
    return this;
  }
}

assert.equal(called, true);

The issue is our wrapper, we've got too many.

    function Wrapper() {
      // Inline Super's code here
      return _construct(Class, arguments, _gPO(this).constructor);
    }
    Wrapper.prototype = Object.create(Class.prototype, {
      constructor: {
        value: Wrapper,
        enumerable: false,
        writable: true,
        configurable: true,
      }
    });
    // Get rid of that Super function
    return _sPO(Wrapper, Class);
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Mar 10, 2018

While we're here, want to fix the extends null bug on Line 461?

This helper is only used when Babel knows that the extended class is a built-in class. It won't be used in any of these cases, so the helper doesn't need to be updated:

class A extends null {}

var Array = null;
class B extends Array {}

class C extends (() => null) {}
@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Mar 10, 2018

This helper is only used when Babel knows that the extended class is a built-in class.

I can overwrite the Builtins. It's definitely an edge case.

window.Array = null;
class Test extends Array {
}
super(time);
}
}
let myDate = new MyDate();

This comment has been minimized.

Copy link
@jridgewell

jridgewell Mar 10, 2018

Member

Can we also add an assertion here? To make sure this is a real date instance.

Something like assert.equal(typeof myDate.toString(), 'string'); should work.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Mar 11, 2018

I need some help with the failing tests... If I use node 4 and run make test-only I can see them fail, but If I transpile them and then I manually run the output file it works 🤔

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:bug-7527 branch from 36e9f2d to 72714a5 Mar 11, 2018
// doesn't exist if you try to run the compiled code, it only exists in exec
// tests).
// @nicolo-ribaudo
function _construct(Parent, args, Class) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Mar 11, 2018

Member

Whoa, can’t do this. Reflect.construct cannot be properly polyfilled, but it’s necessary for non-constructable constructors (like HTMLElement).

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 11, 2018

Author Member

I think that it won't work anyway:

> function Wrapper() {
    return Reflect.construct(HTMLElement, [], Wrapper);
  }
  Wrapper.__proto__ = HTMLElement;
  Wrapper.prototype = Object.create(HTMLElement.prototype, { constructor: { value: Wrapper, configurable: true } })

-> HTMLElement {constructor: ƒ}

> new Wrapper()

-> VM195:2 Uncaught TypeError: Illegal constructor
     at new Wrapper (<anonymous>:2:18)
     at <anonymous>:1:1
   Wrapper @ VM195:2
   (anonymous) @ VM199:1

This comment has been minimized.

Copy link
@jridgewell

jridgewell Mar 11, 2018

Member

It's because you can't directly construct an HTMLElement, it has to go through the custom elements APIs.

function Wrapper() {
  return Reflect.construct(HTMLElement, [], Wrapper);
}
Wrapper.__proto__ = HTMLElement;
Wrapper.prototype = Object.create(HTMLElement.prototype, { constructor: { value: Wrapper, configurable: true } })

customElements.define('x-w', Wrapper)

// no errors should be raised
w = document.createElement('x-w');

As opposed to using a bind constructor:

function Binder() {
    var Constructor, a = [null];
    a.push.apply(a, arguments);
    Constructor = HTMLElement.bind.apply(HTMLElement, a);
    return Object.setPrototypeOf(new Constructor, Binder.prototype);
}
Binder.__proto__ = HTMLElement;
Binder.prototype = Object.create(HTMLElement.prototype, { constructor: { value: Binder, configurable: true } })

customElements.define('x-b', Binder)

// TypeError: Illegal constructor
b = document.createElement('x-b');

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 11, 2018

Member

Back when this was first implemented, I had actually checked if node 4 had a Reflect.construct (because I know node 4 has buggy classes) and it did not have Reflect at all.

Is core-js maybe providing a buggy one? Or is the one they provide doing approximately the same thing as the helper, and thus would not work just like the helper wouldn't?

This comment has been minimized.

Copy link
@jridgewell

jridgewell Mar 12, 2018

Member

It's doing roughly what we are.

}
let myDate = new MyDate();

assert.equal(myDate.toString, Date.prototype.toString);

This comment has been minimized.

Copy link
@jridgewell

jridgewell Mar 11, 2018

Member

This is not equivalent to my proposed test. The method has to be executed, because their an internal branding check (that’s the actual test, not the typeof).

This comment has been minimized.

Copy link
@Jessidhia

Jessidhia Mar 11, 2018

Member

I think you need both.

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:bug-7527 branch from e3a23eb to ebd4027 Mar 11, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Mar 11, 2018

(#7533 (comment) hasn't been resolved yet)

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Mar 13, 2018

@jridgewell The test you proposed (which tests if the internal slots are correct) is failing on node 4. I think that it can't not fail 🤔

The code I tested in this screenshot is the Reflect.construct polyfill + setPrototypeOf hack to make instanceof work.

reflect-polyfill

// This wrapper is needed because Reflect.construct can't be properly
// polyfilled, thus core-js doesn't set the correct __proto__.
var result = Reflect.construct(Parent, args, Class);
if (!result instanceof Class) result = _sPO(result, Class.prototype);

This comment has been minimized.

Copy link
@jridgewell

jridgewell Mar 13, 2018

Member

This seems like a core-js bug, not ours.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Mar 13, 2018

Core-js's polyfill is buggy then. You can't construct proper date objects using Function#call (or #apply), it has to be a new Ctor.bind.apply(Ctor, [null].concat(args)).

/cc @zloirock to see if core-js can update.

// This wrapper is needed because Reflect.construct can't be properly
// polyfilled, thus core-js doesn't set the correct __proto__.
var result = Reflect.construct(Parent, args, Class);
if (!(result instanceof Class)) result = _sPO(result, Class.prototype);

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Mar 13, 2018

Author Member

(By @jridgewell )

This seems like a core-js bug, not ours.

It's something that core-js can't safely polyfill. We can do it because we know that the super constructor is a built-in constructor, so its return value hasn't been overridden.

e.g.

function A() {
  return {};
}

function Wrapper() {
  return _construct(A, [], Wrapper);
}
Wrapper.prototype = Object.create(...);

// This should be false, but if core-js always did setPrototypeOf it would be true.
new Wrapper() instanceof Wrapper;
@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Mar 13, 2018

@jridgewell it's not a bug, it just can't be completely polyfilled, see this logic. If you have some ideas how to improve this polyfill - feel free to propose it. .bind + .setPrototypeOf is not a solution and can't be used inside the polyfill because it will break this prototype chain in constructors.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Mar 13, 2018

.bind + .setPrototypeOf is not a solution and can't be used inside the polyfill because it will break this prototype chain in constructors.

Is this #7533 (comment)?

It can be solved with instanceof, no? Which reminds me, we should probably be doing this ourselves to guard against overridden Builtins.

function _construct(Parent, args, Class) {
  var Constructor, a = [null];
  a.push.apply(a, args);
  Constructor = Parent.bind.apply(Parent, a);
  var c = new Constructor;
  return c instanceof Class ? _sPO(c, Class.prototype) : c;
}
@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Mar 13, 2018

@jridgewell I mean that it's not a solution for this polyfill, it can be used in helpers. About your code, try:

class A {
  constructor() {
    this.foo();
  }
  foo() {
    console.log('A');
  }
}

class B extends A {
  foo() {
    console.log('B');
  }
}

Reflect.construct(A, [], B); // => 'B'

function _construct(Parent, args, Class) {
  var Constructor, a = [null];
  a.push.apply(a, args);
  Constructor = Parent.bind.apply(Parent, a);
  var c = new Constructor;
  return c instanceof Class ? _sPO(c, Class.prototype) : c;
}

_construct(A, [], B); // => 'A'

This is the problem I wrote about.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member Author

nicolo-ribaudo commented Mar 13, 2018

c instanceof Class ? _sPO(c, Class.prototype) : c

If c is an instance of Class, it's __proto__ will already be Class.prototype.

) {
_construct = function _construct(Parent, args, Class) {
// This wrapper is needed because Reflect.construct can't be properly
// polyfilled, thus core-js doesn't set the correct __proto__.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo May 15, 2018

Author Member

Is there another way other than try/catch?

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 15, 2018

Member

Something like this is the only foolproof way to detect if they're not using a Binder polyfill like we are.

core-js@3 added a sham property to the method, but that won't help anyone using an older version.

if (Class) setPrototypeOf(instance, Class.prototype);
return instance;
Constructor = Parent.bind.apply(Parent, a);
return setPrototypeOf(new Constructor, Class.prototype);

This comment has been minimized.

Copy link
@jridgewell

jridgewell May 14, 2018

Member

Where were these reverted? This supports null as a super class (other transforms use this helper).

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:bug-7527 branch from 01d1a34 to 63532f4 May 15, 2018
// Proxy can't be polyfilled. Every browser implemented
// proxies before or at the same time of Reflect.construct,
// so if they support Proxy they also support Reflect.construct.
if (typeof Proxy === "funcion") return true;

This comment has been minimized.

Copy link
@bicknellr

bicknellr May 17, 2018

"funcion"

@nicolo-ribaudo nicolo-ribaudo merged commit 2351a63 into babel:master May 17, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.78% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:bug-7527 branch May 17, 2018
@duailibe duailibe mentioned this pull request May 24, 2018
@trusktr

This comment has been minimized.

Copy link

trusktr commented Aug 24, 2018

@nicolo-ribaudo Can you point me to the change that makes your test work?

@trusktr

This comment has been minimized.

Copy link

trusktr commented Aug 24, 2018

I see, it uses Reflect.construct when available. So otherwise it is impossible to fix (f.e. the Date toString test) if there's no native Reflect.construct?

if (typeof Class !== "function") {
throw new TypeError("Super expression must either be null or a function");
}
if (typeof _cache !== "undefined") {
if (_cache.has(Class)) return _cache.get(Class);
_cache.set(Class, Wrapper);
}
function Wrapper() {}
function Wrapper() {
return _construct(Class, arguments, _gPO(this).constructor)

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Aug 24, 2018

Author Member

@trusktr The main bugfix is this line, which calls the super constructor.

export default function _construct(Parent, args, Class) {
if (typeof Reflect !== "undefined" && Reflect.construct) {
if (isNativeReflectConstruct()) {
_construct = Reflect.construct;
} else {
_construct = function _construct(Parent, args, Class) {

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Aug 24, 2018

Author Member

@trusktr If Reflect.construct doesn't exist we use this polyfill, which isn't 100% correct but works well for extending builtins.

@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.