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

Handle class properties #305

Open
overlookmotel opened this issue Nov 16, 2021 · 15 comments
Open

Handle class properties #305

overlookmotel opened this issue Nov 16, 2021 · 15 comments
Labels
feature New feature or request

Comments

@overlookmotel
Copy link
Owner

Problem

Input:

export default class C {
  x = 1;
}

Output:

export default class C {}

The class property has been ignored. new C().x on source is 1, on output it's undefined.

Not affected

This problem does not affect:

  1. Static properties. static y = 2; output as export default Object.assign( class C {}, { y: 2 } ).
  2. Class instances. new C() output as Object.assign( Object.create( class C {}.prototype ), { x: 1 } )

Side-effects

The problem is that there can be side effects when calling a class to extract its scope vars:

function f () {
  console.log('f called');
  return 1;
}

class C {
  x = f();
}

f() is called on instantiating the class with new C(). That call happens before the class constructor is called, so it's not possible to prevent f() being called by throwing an error in the tracker function in constructor (usual method Livepack uses to bail out before any side effects occur).

Solution

Need to pre-process with @babel/plugin-proposal-class-properties, or implement same logic in Livepack.

It would be even better if:

  • Babel plugin transformed code which runs at build time to move class property declarations inside class constructor (as @babel/plugin-proposal-class-properties does)
  • Serialized output restores original code with class property.

Would be easier to implement this after #65 is implemented.

@overlookmotel
Copy link
Owner Author

Advantage of implementing own version of @babel/plugin-proposal-class-properties's transform would be to avoid transpiling private class fields, which that plugin also does.

But bear in mind that sometimes vars in constructor need to be renamed to allow access to external vars used in property initialization.

e.g.:

class C {
  constructor(f) {
    this.y = f;
  }
  
  x = f();
}

Babel transpiles this to:

class C {
  constructor(_f) {
    Object.defineProperty( this, 'x', f() );
    this.y = _f;
  }
}

(Babel REPL)

This would not work if class constructor contains eval() which can access f e.g.:

class C extends SuperC {
  x = f();

  constructor(f) {
    super();
    this.z = eval('f');
  }

  y = super.foo();
}

An alternative which:

  • doesn't require var renaming
  • supports use of this in initializations
  • support use of super in initializations
  • maintains order of code (so line numbers in transpiled output can be same as original)
function _extractClassInit(Klass, initSymbols, setInit) {
  const { prototype } = Klass;
  const initMethods = initSymbols.map( symbol => {
    const method = prototype[symbol];
    delete prototype[symbol];
    return method;
  } );

  setInit( instance => {
    initMethods.forEach( method => method.call(instance) };
    return instance;
  } );

  return Klass;
}

const _initSymbol1 = new Symbol(),
  _initSymbol2 = new Symbol();
let _init;
let C = _extractClassInit(
  class C extends SuperC {
    [_initSymbol1]() {
      this.x = f();
    }
    constructor(f) {
      _init( super() );
      this.z = eval('f');
    }
    [_initSymbol2]() {
      this.y = super.foo();
    }
  },
  [ _initSymbol1, _initSymbol2 ],
  init => _init = init
);

@overlookmotel
Copy link
Owner Author

overlookmotel commented Nov 16, 2021

Another alternative: Avoid need to call class constructor at all when serializing classes:

const _classes = new WeakMap();
const _recordClass = (Klass, tracker) => {
  _classes.set(Klass, tracker);
  return Klass;
};

const livepack_scopeId_1 = livepack_getScopeId();
let C = _recordClass(
  class C {
    constructor() {
      this.x = x;
    }
  },
  () => /*livepack_track:{"id":2,"scopes":[{"blockId":1,"varNames":["x"]}]}*/ [[livepack_scopeId_1, x]]
)

The tracker function is recorded in _classes and, when serializing the class, tracker can be retrieved from _classes and called directly, rather than having to call the class constructor.

This is a lot simpler!

@overlookmotel
Copy link
Owner Author

overlookmotel commented Dec 10, 2021

There is a simpler solution to preventing side effects:

1. Classes with extends super class

Class constructor is called before field initializers. So tracker in constructor (same as at present) will be called and short circuit execution before class field values are evaluated.

2. Classes with no extends

Put tracker in first class field e.g.:

class X {
  x = f()
}

becomes:

class X {
  x = livepack_tracker() || f()
}

This will call livepack_tracker() before any other code executes, and so prevent side effects.

@overlookmotel
Copy link
Owner Author

overlookmotel commented Dec 11, 2021

There is one tricky complication with class properties - dynamic field names.

e.g. Input:

let counter = 0;
const getFieldName = () => `x${counter++}`;

const getClass = () => {
  return class {
    [getFieldName()] = 1;
  };
};

export default [ getClass(), getClass() ];

getFieldName() is evaluated when the class is defined, not when it's constructed.

So output would need to be:

const createScope = _fieldName => class {
  [_fieldName] = 1
};
export default [ createScope('x0'), createScope('x1') ];

_fieldName can only be a string or a Symbol, so can't be a circular reference.

This would require Babel plugin to insert code to evaluate field key expressions before class declaration, and for them to be passed to serializer via the tracker.

// Babelized
function livepack_conformToStringOrSymbol(value) {
  if (typeof value === 'symbol') return value;
  return value + '';
}

const getClass = () => {
  let livepack_temp_1;
  return class {
    [
      livepack_temp_1 = livepack_conformToStringOrSymbol( getFieldName() )
    ] = livepack_tracker( () => [ /* ... */, [livepack_temp_1] ] ) || 1;
  };
};

@overlookmotel
Copy link
Owner Author

overlookmotel commented Mar 3, 2022

Arrow functions defined in class properties need access to this:

// Input
class C {
  getThis = () => this
}
export default new C().getThis;

Class properties containing arrow functions accessing this is common in React class components.

Solution 1

This requires Babel plugin to wrap field declarations in methods which inject in this and scope ID var for this:

// Babelized
function _extractClassFieldInitializers( klass, initializerSymbols, setInitializer ) {
  const proto = klass.prototype;
  const initializers = initializerSymbols.map( (initializerSymbol) => {
    const initializer = proto[initializerSymbol];
    delete proto[initializerSymbol];
    return initializer;
  } );
  setInitializer( (_this, scopeId) => {
    for (const initializer of initializers) {
      initializer.call(_this, scopeId);
    }
    return _this;
  } );
  return klass;
}

let livepack_temp_1, livepack_temp_2;
let C = _extractClassFieldInitializers(
  class /*livepack_track:5;c;*/ X {
    [ livepack_temp_1 = Symbol() ]( livepack_scopeId_8 ) {
      this.getThis = {
        getThis: () => /*livepack_track:9;f;*/ {
          livepack_tracker( livepack_getFnInfo_9, () => [ [livepack_scopeId_8, this] ] );
          return this;
        }
      }.getThis;
    }

    constructor() {
      livepack_tracker( livepack_getFnInfo_5, () => [] );
      const livepack_scopeId_8 = livepack_getScopeId();
      livepack_temp_2(this, livepack_scopeId_8);
    }
  },
  [livepack_temp_1],
  initalizer => livepack_temp_2 = initalizer
};
export default new C().getThis;

NB If class constructor has complex params, call to field initializer would need to be in constructor params to ensure side effects of class fields execute before side effects of function params. See #108 (comment).

Solution 2

Alternatively, could avoid nesting fields in methods by injecting scope ID var directly into any arrow functions:

// Input
class C {
  getThis = () => this;
  constructor() {
    this.getThis2 = () => this;
  }
}
const c = new C();
export default [ c.getThis, c.getThis2 ];
// Babelized
function _bindWithScopeId(fn, scopeId, name) {
  const boundFn = fn.bind(undefined, scopeId);
  Object.defineProperty(boundFn, 'name', {value: name || ''});
  return boundFn;
}

let livepack_temp_1;
class /*livepack_track:5;c;*/ C {
  getThis = (
    livepack_tracker( livepack_getFnInfo_5, () => [] ),
    livepack_temp_1 = livepack_getScopeId(),
    _bindWithScopeId(
      (livepack_scopeId_8) => /*livepack_track:10;f;*/ {
        livepack_tracker( livepack_getFnInfo_10, () => [ [ livepack_scopeId_8, this ] ] );
        return this;
      },
      livepack_temp_1,
      'getThis'
    )
  );

  constructor() {
    const livepack_scopeId_8 = livepack_temp_1;
    this.getThis2 = () => /*livepack_track:9;f;*/ {
      livepack_tracker( livepack_getFnInfo_9, () => [ [ livepack_scopeId_8, this ] ] );
      return this;
    };
  }
}
const c = new C();
export default [ c.getThis, c.getThis2 ];

This solution relies on class construction being synchronous. livepack_temp_1 holds the scope ID only during initial synchronous construction and it's transferred to long-lasting livepack_scopeId_8 vars. If the class is instantiated a 2nd time, livepack_temp_1 will be overwritten with a different scope ID for the 2nd class instance, but it doesn't matter as the tracker calls refer to livepack_scopeId_8 which has already captured the first scope ID and held it in local scope.

This solution has 2 advantages over the first:

  1. Doesn't introducing an additional function wrapper so will not add an entry the stack trace if an error is thrown in the field initializer.
  2. Will be more performant as no adding methods and then deleting them.

Disadvantage is that's it's a bit tricky to wrap the arrow functions. Only top level arrow functions should have scope ID injected. i.e. for class C { getGetThis = () => () => this }, only the outer arrow function would have scope ID injected. The inner arrow function can access it from the outer one.

@overlookmotel
Copy link
Owner Author

overlookmotel commented Mar 3, 2022

If a class property contains a class which uses super, Solution 2 above won't work. Need to introduce a closure to contain the temp var for super target.

// Input
class C {
  D = class {
    foo() {
      return super.toString;
    }
  };
}
export default new C().D.prototype.foo;
// Babelized
class /*livepack_track:5;c;*/ C {
  D = (
    livepack_tracker( livepack_getFnInfo_5, () => [] ),
    ( (livepack_scopeId_8, livepack_temp_1) => (
      livepack_temp_1 = {
        D: class /*livepack_track:10;c;*/ {
          foo /*livepack_track:14;f;*/ () {
            livepack_tracker( livepack_getFnInfo_14, () => [ [ livepack_scopeId_8, livepack_temp_1] ] );
            return super.toString;
          }
          constructor() {
            livepack_tracker( livepack_getFnInfo_10, () => [] );
          }
        }
      }.D
    ) )( livepack_getScopeId() )
  );
}
export default new C().D.prototype.foo;

Each class prototype property needs its own block so can determine which properties need which temp vars.

It would be possible to avoid the closure by injecting temp var into the inner class constructor and every class method which uses super using .bind(), as in Solution 2 above. But that'd be a lot of work just to avoid a closure!

@overlookmotel
Copy link
Owner Author

overlookmotel commented Mar 3, 2022

In response to last point in comment above: No actually using .bind() wouldn't work for methods. Only works for arrow functions because .bind() does not alter the value of this in an arrow function. Not so for methods. There is no avoiding closures in these cases.

@overlookmotel
Copy link
Owner Author

overlookmotel commented Mar 3, 2022

Binding/block for this actually doesn't need to be shared between class constructor and prototype properties. this is not mutable, so each could have its own independent this block which would also include any temp vars. This would simplify implementation.

@overlookmotel
Copy link
Owner Author

There is a way to deal with arrow functions in class properties without closures or .bind() - private properties.

Modification of "Solution 2" example above:

let livepack_temp_1;
class /*livepack_track:5;c;*/ C {
  #livepack_scopeId_8 = livepack_tracker( livepack_getFnInfo_5, () => [] ) || livepack_getScopeId();
  getThis = () => /*livepack_track:10;f;*/ {
    livepack_tracker( livepack_getFnInfo_10, () => [ [ this.#livepack_scopeId_8, this ] ] );
    return this;
  }

  constructor() {
    this.getThis2 = () => /*livepack_track:9;f;*/ {
      livepack_tracker( livepack_getFnInfo_9, () => [ [ this.#livepack_scopeId_8, this ] ] );
      return this;
    };
  }
}
const c = new C();
export default [ c.getThis, c.getThis2 ];

It's not difficult to ensure no var name clashes for the private property used by Livepack.

However, this doesn't solve the problem for classes using super in class properties. They can't use private fields on outer class for temp vars, as this within the inner class is not the right this.

@overlookmotel
Copy link
Owner Author

After closing #541, I think class properties are now supported.

Private properties are also now supported I believe (though they're pretty useless as they can't be accessed by class methods).

Side-effects when serializing classes (#549) is still an open issue, but solution is clear.

Once that's solved, will just need tests to make sure class properties work as intended.

@overlookmotel
Copy link
Owner Author

Actually, support is not complete. Computed property keys aren't supported. e.g.:

const propName = 'x';
class X {
  [propName] = 123
}

Computed property keys are calculated at time class is defined, so would need to be saved in temp vars in instrumentation, and then passed to serializer so it can recreate them.

Complicated case:

function createClass(getPropName) {
  return class {
    ['_' + getPropName()] = 123;
  };
}
export default [
  createClass(() => 'x'),
  createClass(() => 'y')
];

'_' + getPropName() is evaluated at build time, and getPropName should not be included in output.

Would need to be serialized as:

const scope = prop => class { [prop] = 123; };
export default [scope('_x'), scope('_y')];

This is tricky as prop is a var that doesn't exist in the original source. There is no mechanism for handling this in Livepack at present.

@overlookmotel
Copy link
Owner Author

overlookmotel commented Dec 4, 2023

Ah no. this in arrow functions in class properties is also not working. e.g.:

class C {
  x = () => this;
}
export default new C().x;

Instrumentation is not adding a scope ID inside the class which scopes this to within the particular class instance.

@overlookmotel
Copy link
Owner Author

To solve the problem of requiring a closure around classes defined inline in class fields, temp vars for class (super target) and associated scope ID could be avoided as follows:

Input:

const C = class {
  foo() {
    super.foo();
  }
};

Instrumented:

const _scopeIdsForClasses = new WeakMap();

const C = class livepack_temp_1 {
  static {
    _scopeIdsForClasses.set(this, livepack_get_scopeId());
    Object.defineProperty(this, 'name', {value: 'C'});
  }
  foo() {
    super.foo();
  }
};

Tracker in foo() method can access the super target as livepack_temp_1 and scope ID as _scopeIdsForClasses.get(livepack_temp_1).

NB: .name is set in a static block inserted at top of the class, in case it's later over written by a static field e.g. static name = 'foo' or code in another static block.

This would re-introduce some of the complexity in determining implicit class names which was removed in solution to #483. But it should work.

@overlookmotel
Copy link
Owner Author

Solution 2 in comment above concerning binding scope IDs into arrow functions has one bug.

As I said above, it relies on the synchronous nature of class instance construction. But could be messed with if a class field creates an instance of same class:

class C {
  foo = new C();
  getThis = () => this;
}

If this was instrumented as

let livepack_temp_1;
class /*livepack_track:5;c;*/ C {
  foo = (
    livepack_tracker( livepack_getFnInfo_5, () => [] ),
    livepack_temp_1 = livepack_getScopeId(),
    new C()
  );
  getThis = _bindWithScopeId(
    (livepack_scopeId_8) => /*livepack_track:10;f;*/ {
      livepack_tracker( livepack_getFnInfo_10, () => [ [ livepack_scopeId_8, this ] ] );
      return this;
    },
    livepack_temp_1,
    'getThis'
  );
}

then new C() will overwrite livepack_temp_1.

Using a private field to contain scope ID (also suggested above) solves this. As would using a different block and scope ID for each class field.

Private field solution is simpler. Only disadvantage is that the private field could be accessed with an eval() somewhere in the class, and only way to name the private field to prevent this would be to rename any matching private field accesses when instrumenting the eval() code, which would introduce complexity again.

@overlookmotel
Copy link
Owner Author

overlookmotel commented Dec 21, 2023

Actually, using a private field where there were no private fields previous will cause legal code to become illegal in this edge case:

const o = {x: 1};
class S {
  constructor() {
    return o;
  }
}

class C extends S {}

new C();
new C();

That executes OK, but if a private field #livepack_scopeId is added to C, then it fails with TypeError: Cannot initialize #livepack_scopeId twice on the same object.

For the same kind of reason, while it's tempting to think you could use a WeakMap to associate this values with scope IDs, it wouldn't work because this is not necessarily unique in each class instantiation.

So the only solution which avoids all the edge cases that I can see is make this a separate block for every arrow function in a class field, and to bind each arrow function with a fresh scope ID. Or maybe the scope ID can be the same each time, but each needs its own block to contain this.

Edit: Maybe a WeakMap would work after all. As long as each this has its own block, then getting the same scope ID for the same this value shouldn't matter, as the only purpose of scope IDs is to distinguish between different values for the same binding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant