From 34050374e49db2fc732706d3aecc878c8dd378d7 Mon Sep 17 00:00:00 2001 From: overlookmotel Date: Tue, 5 Dec 2023 00:27:25 +0000 Subject: [PATCH] Instrument: Prevent side-effects serializing classes with properties [fix] Fixes #549. --- lib/instrument/tracking.js | 23 +- lib/instrument/visitors/class.js | 41 ++-- test/classes.test.js | 348 ++++++++++++++++++++++++++++++- 3 files changed, 391 insertions(+), 21 deletions(-) diff --git a/lib/instrument/tracking.js b/lib/instrument/tracking.js index 0ddbfba3..663377db 100644 --- a/lib/instrument/tracking.js +++ b/lib/instrument/tracking.js @@ -57,7 +57,7 @@ function insertBlockVarsIntoBlockStatement(block, blockNode, state) { * @param {Object} paramsBlock - Function params block object * @param {Object} [bodyBlock] - Function body block object * (`undefined` if arrow function with no body block) - * @param {Object} trackerNode - AST node for tracker call + * @param {Object} [trackerNode] - AST node for tracker call (or undefined if no tracker to insert) * @param {Object} state - State object * @returns {undefined} */ @@ -78,20 +78,24 @@ function insertTrackerCodeIntoFunction(fn, fnNode, paramsBlock, bodyBlock, track * @param {Object} paramsBlock - Function params block object * @param {Object} [bodyBlock] - Function body block object * (`undefined` if arrow function with no body block) - * @param {Object} trackerNode - AST node for tracker call + * @param {Object} [trackerNode] - AST node for tracker call (or undefined if no tracker to insert) * @param {Object} state - State object * @returns {undefined} */ function insertTrackerCodeIntoFunctionBody(fnNode, paramsBlock, bodyBlock, trackerNode, state) { + // Exit if no tracker, scope ID var, or temp vars to insert + const block = bodyBlock || paramsBlock; + if (!trackerNode && !block.scopeIdVarNode) return; + // If body is not a block statement, convert it to one let bodyNode = fnNode.body; if (!bodyBlock) bodyNode = fnNode.body = t.blockStatement([t.returnStatement(bodyNode)]); // Insert scope ID and temp var nodes - insertBlockVarsIntoBlockStatement(bodyBlock || paramsBlock, bodyNode, state); + insertBlockVarsIntoBlockStatement(block, bodyNode, state); // Insert tracker call - bodyNode.body.unshift(t.expressionStatement(trackerNode)); + if (trackerNode) bodyNode.body.unshift(t.expressionStatement(trackerNode)); } /** @@ -143,7 +147,7 @@ function insertTrackerCodeIntoFunctionBody(fnNode, paramsBlock, bodyBlock, track * @param {Object} paramsBlock - Function params block object * @param {Object} [bodyBlock] - Function body block object * (`undefined` if arrow function with no body block) - * @param {Object} trackerNode - AST node for tracker call + * @param {Object} [trackerNode] - AST node for tracker call (or undefined if no tracker to insert) * @param {number} firstComplexParamIndex - Index of first param which is a complex param * @param {Object} state - State object * @returns {undefined} @@ -151,14 +155,20 @@ function insertTrackerCodeIntoFunctionBody(fnNode, paramsBlock, bodyBlock, track function insertTrackerCodeIntoFunctionParams( fnNode, paramsBlock, bodyBlock, trackerNode, firstComplexParamIndex, state ) { + // Exit if no tracker, scope ID var, or temp vars to insert + const {scopeIdVarNode} = paramsBlock; + if (!trackerNode && !scopeIdVarNode) return; + // Create object pattern to use as rest element. // `{ [ livepack_tracker(...) ]: [] = [] }` // Assignments will be added to each side of `[] = []` to init vars and assign to them. + // If no tracker to insert (class constructor where class has prototype properties), + // create `livepack_tracker()` call with no arguments. const leftNodes = [], rightNodes = [], propNodes = [ t.objectProperty( - trackerNode, + trackerNode || t.callExpression(state.trackerVarNode, []), t.assignmentPattern(t.arrayPattern(leftNodes), t.arrayExpression(rightNodes)), true ) @@ -171,7 +181,6 @@ function insertTrackerCodeIntoFunctionParams( } // Insert assignments for scope ID var node and temp var nodes - const {scopeIdVarNode} = paramsBlock; if (scopeIdVarNode) { addAssignment(scopeIdVarNode, t.callExpression(state.getScopeIdVarNode, [])); diff --git a/lib/instrument/visitors/class.js b/lib/instrument/visitors/class.js index 75adc489..f558ce7c 100644 --- a/lib/instrument/visitors/class.js +++ b/lib/instrument/visitors/class.js @@ -178,7 +178,7 @@ function visitClass(classNode, parent, key, className, state) { constructorParamsBlock.varsBlock = constructorBodyBlock; // Visit constructor and prototype properties - let protoThisBlock; + let protoThisBlock, propForTrackerNode; if (constructorNode || protoPropertyIndexes.length !== 0) { // Enter function state.currentFunction = fn; @@ -210,6 +210,10 @@ function visitClass(classNode, parent, key, className, state) { for (const index of protoPropertyIndexes) { visitKey(memberNodes, index, ClassPropertyMaybePrivate, state); } + + // If class does not extend a super class, prototype properties will be evaluated + // before class constructor, so tracker needs to go in 1st proto property + if (!classNode.superClass) propForTrackerNode = memberNodes[protoPropertyIndexes[0]]; } // Exit function @@ -286,8 +290,8 @@ function visitClass(classNode, parent, key, className, state) { // Queue instrumentation of class in 2nd pass state.secondPass( instrumentClass, - classNode, fn, parent, key, constructorNode, constructorParamsBlock, constructorBodyBlock, - superBlock, state + classNode, fn, parent, key, constructorNode, propForTrackerNode, + constructorParamsBlock, constructorBodyBlock, superBlock, state ); } @@ -417,6 +421,7 @@ function serializeClassAst(classNode, constructorNode, constructorIndex) { * @param {Object|Array} parent - Parent AST node/container * @param {string|number} key - Node's key on parent AST node/container * @param {Object} [constructorNode] - Class constructor AST node (or `undefined` if no constructor) + * @param {Object} [propForTrackerNode] - Property node to insert tracker in * @param {Object} constructorParamsBlock - Constructor params block object * @param {Object} constructorBodyBlock - Constructor body block object * @param {Object} superBlock - `super` target block object @@ -424,15 +429,31 @@ function serializeClassAst(classNode, constructorNode, constructorIndex) { * @returns {undefined} */ function instrumentClass( - classNode, fn, parent, key, constructorNode, constructorParamsBlock, constructorBodyBlock, - superBlock, state + classNode, fn, parent, key, constructorNode, propForTrackerNode, + constructorParamsBlock, constructorBodyBlock, superBlock, state ) { - // If class has no constructor, create one for tracker code to be inserted in - if (!constructorNode) { + // Create tracker + let trackerNode = createTrackerCall(fn, state); + + if (propForTrackerNode) { + // Insert tracker into prototype property + propForTrackerNode.value = propForTrackerNode.value + ? t.sequenceExpression([trackerNode, propForTrackerNode.value]) + : t.unaryExpression('void', trackerNode); + trackerNode = undefined; + } else if (!constructorNode) { + // Class has no constructor - create one for tracker code to be inserted in constructorNode = createClassConstructor(fn, state); classNode.body.body.push(constructorNode); } + // Add tracker code + block vars to constructor + if (constructorNode) { + insertTrackerCodeIntoFunction( + fn, constructorNode, constructorParamsBlock, constructorBodyBlock, trackerNode, state + ); + } + // Insert `static {}` block to define temp var for `super` target if `super` used in class const superVarNode = getSuperVarNode(superBlock); if (superVarNode) { @@ -447,12 +468,6 @@ function instrumentClass( // Restore to original place in AST parent[key] = classNode; - // Add tracker code + block vars to constructor - const trackerNode = createTrackerCall(fn, state); - insertTrackerCodeIntoFunction( - fn, constructorNode, constructorParamsBlock, constructorBodyBlock, trackerNode, state - ); - // Insert tracking comment const commentHolderNode = classNode.id || classNode.superClass || classNode.body; insertTrackerComment(fn.id, FN_TYPE_CLASS, commentHolderNode, 'leading', state); diff --git a/test/classes.test.js b/test/classes.test.js index 1d42ccff..a70a28a4 100644 --- a/test/classes.test.js +++ b/test/classes.test.js @@ -8,7 +8,8 @@ 'use strict'; // Imports -const {itSerializes} = require('./support/index.js'); +const {serialize} = require('livepack'), + {itSerializes} = require('./support/index.js'); // Tests @@ -5575,6 +5576,351 @@ describe('Classes', () => { }); }); + describe('class properties', () => { + // Tests for https://github.com/overlookmotel/livepack/issues/549 + describe('prototype properties do not produce side effects when serializing class', () => { + describe('class with no super class', () => { + describe('with no constructor', () => { + it('property with value', () => { + const calls = []; + class C { + x = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('property with no value first', () => { + const calls = []; + class C { + x; + y = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with value', () => { + const calls = []; + class C { + #x = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with no value first', () => { + const calls = []; + class C { + #x; + #y = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + }); + + describe('with constructor', () => { + it('property with value', () => { + const calls = []; + class C { + x = calls.push('prop'); + constructor({y}) { + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('property with no value first', () => { + const calls = []; + class C { + x; + y = calls.push('prop'); + constructor({y}) { + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with value', () => { + const calls = []; + class C { + #x = calls.push('prop'); + constructor({y}) { + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with no value first', () => { + const calls = []; + class C { + #x; + #y = calls.push('prop'); + constructor({y}) { + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + }); + }); + + describe('class with super class', () => { + describe('with no constructor', () => { + it('property with value', () => { + const calls = []; + class S { + x = calls.push('super prop'); + } + class C extends S { + y = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('property with no value first', () => { + const calls = []; + class S { + w; + x = calls.push('super prop'); + } + class C extends S { + y; + z = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with value', () => { + const calls = []; + class S { + #x = calls.push('super prop'); + } + class C extends S { + #y = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with no value first', () => { + const calls = []; + class S { + #w; + #x = calls.push('super prop'); + } + class C extends S { + #y; + #z = calls.push('prop'); + } + serialize(C); + expect(calls).toEqual([]); + }); + }); + + describe('with constructor', () => { + it('property with value', () => { + const calls = []; + class S { + x = calls.push('super prop'); + } + class C extends S { + y = calls.push('prop'); + constructor({y}) { + super(); + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('property with no value first', () => { + const calls = []; + class S { + w; + x = calls.push('super prop'); + } + class C extends S { + y; + z = calls.push('prop'); + constructor({y}) { + super(); + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with value', () => { + const calls = []; + class S { + #x = calls.push('super prop'); + } + class C extends S { + #y = calls.push('prop'); + constructor({y}) { + super(); + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + + it('private property with no value first', () => { + const calls = []; + class S { + #w; + #x = calls.push('super prop'); + } + class C extends S { + #y; + #z = calls.push('prop'); + constructor({y}) { + super(); + this.f = () => y; + } + } + serialize(C); + expect(calls).toEqual([]); + }); + }); + }); + }); + + describe('prototype properties do not affect instrumentation of class constructor', () => { + describe('property with no value', () => { + itSerializes('function referencing complex param', { + in() { + class C { + x; + constructor({y, f = (0, () => y)}) { + this.f = f; + } + } + const obj = new C({y: 123}); + return obj.f; + }, + out: '(a=>()=>a)(123)', + validate(fn) { + expect(fn).toBeFunction(); + expect(fn()).toBe(123); + } + }); + + itSerializes('class using `super` in constructor params', { + in() { + class C { + x; + constructor( + c = class extends Object { foo() { return super.toString; }} + ) { + this.c = c; + } + } + const obj = new C(); + return obj.c; + }, + out: `(()=>{ + const a=Object, + b=a.setPrototypeOf, + c=b(class c extends class{}{},a), + d=(a=>[ + b=>a=b, + { + foo(){ + return Reflect.get(Object.getPrototypeOf(a.prototype),"toString",this) + } + }.foo + ])(); + d[0](c); + b( + a.defineProperties(c.prototype,{foo:{value:d[1],writable:true,configurable:true}}), + a.prototype + ); + return c + })()`, + validate(Klass) { + expect(Klass).toBeFunction(); + const obj = new Klass(); + expect(obj.foo()).toBe(Object.prototype.toString); + } + }); + }); + + describe('property with value', () => { + itSerializes('function referencing complex param', { + in() { + class C { + x = 123; + constructor({y, f = (0, () => y)}) { + this.f = f; + } + } + const obj = new C({y: 123}); + return obj.f; + }, + out: '(a=>()=>a)(123)', + validate(fn) { + expect(fn).toBeFunction(); + expect(fn()).toBe(123); + } + }); + + itSerializes('class using `super` in constructor params', { + in() { + class C { + x = 123; + constructor( + c = class extends Object { foo() { return super.toString; }} + ) { + this.c = c; + } + } + const obj = new C(); + return obj.c; + }, + out: `(()=>{ + const a=Object, + b=a.setPrototypeOf, + c=b(class c extends class{}{},a), + d=(a=>[ + b=>a=b, + { + foo(){ + return Reflect.get(Object.getPrototypeOf(a.prototype),"toString",this) + } + }.foo + ])(); + d[0](c); + b( + a.defineProperties(c.prototype,{foo:{value:d[1],writable:true,configurable:true}}), + a.prototype + ); + return c + })()`, + validate(Klass) { + expect(Klass).toBeFunction(); + const obj = new Klass(); + expect(obj.foo()).toBe(Object.prototype.toString); + } + }); + }); + }); + }); + describe('instances', () => { itSerializes('of base class', { in() {