Skip to content

Commit

Permalink
Use ReferenceSnapshots for call arguments consistently
Browse files Browse the repository at this point in the history
  • Loading branch information
@asmblah
asmblah committed Apr 18, 2024
1 parent 738430a commit 4d0cef5
Show file tree
Hide file tree
Showing 8 changed files with 130 additions and 10 deletions.
24 changes: 22 additions & 2 deletions src/Function/FunctionSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ var _ = require('microdash'),
TOO_FEW_ARGS_BUILTIN = 'core.too_few_args_builtin',
TOO_FEW_ARGS_BUILTIN_SINGLE = 'core.too_few_args_builtin_single',
PHPError = phpCommon.PHPError,
Reference = require('../Reference/Reference'),
ReferenceSnapshot = require('../Reference/ReferenceSnapshot'),
UNKNOWN = 'core.unknown',
Value = require('../Value').sync();

Expand All @@ -28,6 +30,7 @@ var _ = require('microdash'),
* @param {CallStack} callStack
* @param {Translator} translator
* @param {ValueFactory} valueFactory
* @param {ReferenceFactory} referenceFactory
* @param {FutureFactory} futureFactory
* @param {Flow} flow
* @param {FunctionContextInterface} context
Expand All @@ -43,6 +46,7 @@ function FunctionSpec(
callStack,
translator,
valueFactory,
referenceFactory,
futureFactory,
flow,
context,
Expand Down Expand Up @@ -100,6 +104,10 @@ function FunctionSpec(
* @type {Parameter[]|null[]}
*/
this.parameterList = parameterList;
/**
* @type {ReferenceFactory}
*/
this.referenceFactory = referenceFactory;
/**
* @type {boolean}
*/
Expand Down Expand Up @@ -152,13 +160,25 @@ _.extend(FunctionSpec.prototype, {
*
* Note that it will be resolved to a value at this point if not already.
* For by-reference parameters in weak-type checking mode, the coerced value will be written back
* to the reference, ie. <reference>.setValue(<coerced value>).
* to the reference, i.e. <reference>.setValue(<coerced value>).
*/
return parameter.coerceArgument(argumentReference)
.next(function (coercedArgument) {
coercedArguments[index] = coercedArgument;

if (!parameter.isPassedByReference()) {
if (parameter.isPassedByReference()) {
if (
argumentReference instanceof Reference &&
!(argumentReference instanceof ReferenceSnapshot)
) {
// Argument is a non-snapshot reference: wrap it in a snapshot
// to allow consistent synchronous access to the snapshotted value.
argumentReferenceList[index] = spec.referenceFactory.createSnapshot(
argumentReference,
coercedArgument
);
}
} else {
// Arguments for this parameter are passed by value, so also
// overwrite with the coerced argument in the reference list passed to the function.
argumentReferenceList[index] = coercedArgument;
Expand Down
10 changes: 10 additions & 0 deletions src/Function/FunctionSpecFactory.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var _ = require('microdash');
* @param {ParameterListFactory} parameterListFactory
* @param {ReturnTypeProvider} returnTypeProvider
* @param {ValueFactory} valueFactory
* @param {ReferenceFactory} referenceFactory
* @param {FutureFactory} futureFactory
* @param {Flow} flow
* @constructor
Expand All @@ -37,6 +38,7 @@ function FunctionSpecFactory(
parameterListFactory,
returnTypeProvider,
valueFactory,
referenceFactory,
futureFactory,
flow
) {
Expand Down Expand Up @@ -72,6 +74,10 @@ function FunctionSpecFactory(
* @type {ParameterListFactory}
*/
this.parameterListFactory = parameterListFactory;
/**
* @type {ReferenceFactory}
*/
this.referenceFactory = referenceFactory;
/**
* @type {ReturnTypeProvider}
*/
Expand Down Expand Up @@ -115,6 +121,7 @@ _.extend(FunctionSpecFactory.prototype, {
factory.callStack,
factory.translator,
factory.valueFactory,
factory.referenceFactory,
factory.futureFactory,
factory.flow,
context,
Expand Down Expand Up @@ -177,6 +184,7 @@ _.extend(FunctionSpecFactory.prototype, {
factory.callStack,
factory.translator,
factory.valueFactory,
factory.referenceFactory,
factory.futureFactory,
factory.flow,
context,
Expand Down Expand Up @@ -227,6 +235,7 @@ _.extend(FunctionSpecFactory.prototype, {
factory.callStack,
factory.translator,
factory.valueFactory,
factory.referenceFactory,
factory.futureFactory,
factory.flow,
context,
Expand Down Expand Up @@ -279,6 +288,7 @@ _.extend(FunctionSpecFactory.prototype, {
factory.callStack,
factory.translator,
factory.valueFactory,
factory.referenceFactory,
factory.futureFactory,
factory.flow,
context,
Expand Down
1 change: 1 addition & 0 deletions src/PHPState.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,7 @@ module.exports = require('pauser')([
parameterListFactory,
get('return_type_provider'),
valueFactory,
referenceFactory,
futureFactory,
flow
)),
Expand Down
1 change: 1 addition & 0 deletions src/ReferenceFactory.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ module.exports = require('pauser')([
factory.flow,
wrappedReference,
value || null,
// If the wrapped reference has a reference rather than a value assigned, extract and use it here.
wrappedReference.isReference() ? wrappedReference.getReference() : null
);
},
Expand Down
2 changes: 2 additions & 0 deletions src/builtin/opcodes/calculation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ module.exports = function (internals) {
* Used by "my_function(...)" syntax.
*/
callFunction: internals.typeHandler(
// Note that arguments are snapshotted by FunctionSpec.coerceArguments(...)
// later on, as at that point we know whether a parameter is by-reference.
'string name, slot ...argReferences : slot',
function (name, argReferences) {
var namespaceScope = callStack.getEffectiveNamespaceScope(),
Expand Down
39 changes: 38 additions & 1 deletion test/integration/ffi/api/builtin/functions/nonCoercing/byReferenceParameterTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ var expect = require('chai').expect,
nowdoc = require('nowdoc'),
phpCommon = require('phpcommon'),
tools = require('../../../../../tools'),
PHPFatalError = phpCommon.PHPFatalError;
PHPFatalError = phpCommon.PHPFatalError,
ReferenceSnapshot = require('../../../../../../../src/Reference/ReferenceSnapshot');

describe('PHP builtin FFI function non-coercion by-reference parameter integration', function () {
it('should support passing undefined variables to by-reference parameters', async function () {
Expand Down Expand Up @@ -51,6 +52,42 @@ EOS
expect(engine.getStderr().readAll()).to.equal('');
});

it('should provide ReferenceSnapshots of by-reference arguments', async function () {
var php = nowdoc(function () {/*<<<EOS
<?php
$result = [];
$myVar = 21;
add_one($myVar);
return $myVar;
EOS
*/;}), //jshint ignore:line
module = tools.asyncTranspile('/path/to/my_module.php', php),
engine = module(),
capturedReference = null;
engine.defineNonCoercingFunction(
'add_one',
function (myNumberReference) {
var value;

capturedReference = myNumberReference;

// Value should be available synchronously.
value = myNumberReference.getValue();

// Write the result back to the variable, testing by-reference parameters.
myNumberReference.setValue(this.valueFactory.createInteger(value.getNative() + 1));
},
'int &$myNumber : void'
);

expect((await engine.execute()).getNative()).to.equal(22);
expect(engine.getStdout().readAll()).to.equal('');
expect(engine.getStderr().readAll()).to.equal('');
expect(capturedReference).to.be.an.instanceOf(ReferenceSnapshot);
});

it('should raise a fatal error when custom function is passed primitive value in weak type-checking mode', async function () {
var php = nowdoc(function () {/*<<<EOS
<?php
Expand Down
10 changes: 10 additions & 0 deletions test/unit/Function/FunctionSpecFactoryTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ var expect = require('chai').expect,
ObjectValue = require('../../../src/Value/Object').sync(),
Parameter = require('../../../src/Function/Parameter'),
ParameterListFactory = require('../../../src/Function/ParameterListFactory'),
ReferenceFactory = require('../../../src/ReferenceFactory').sync(),
ReferenceSlot = require('../../../src/Reference/ReferenceSlot'),
ReturnTypeProvider = require('../../../src/Function/ReturnTypeProvider'),
Translator = phpCommon.Translator,
Expand All @@ -35,6 +36,7 @@ describe('FunctionSpecFactory', function () {
futureFactory,
namespaceScope,
parameterListFactory,
referenceFactory,
returnTypeProvider,
translator,
typeFactory,
Expand All @@ -50,6 +52,7 @@ describe('FunctionSpecFactory', function () {
futureFactory = sinon.createStubInstance(FutureFactory);
namespaceScope = sinon.createStubInstance(NamespaceScope);
parameterListFactory = sinon.createStubInstance(ParameterListFactory);
referenceFactory = sinon.createStubInstance(ReferenceFactory);
returnTypeProvider = sinon.createStubInstance(ReturnTypeProvider);
translator = sinon.createStubInstance(Translator);
typeFactory = sinon.createStubInstance(TypeFactory);
Expand All @@ -69,6 +72,7 @@ describe('FunctionSpecFactory', function () {
parameterListFactory,
returnTypeProvider,
valueFactory,
referenceFactory,
futureFactory,
flow
);
Expand Down Expand Up @@ -96,6 +100,7 @@ describe('FunctionSpecFactory', function () {
sinon.match.same(callStack),
sinon.match.same(translator),
sinon.match.same(valueFactory),
sinon.match.same(referenceFactory),
sinon.match.same(futureFactory),
sinon.match.same(flow),
sinon.match.same(functionContext),
Expand Down Expand Up @@ -186,6 +191,7 @@ describe('FunctionSpecFactory', function () {
sinon.match.same(callStack),
sinon.match.same(translator),
sinon.match.same(valueFactory),
sinon.match.same(referenceFactory),
sinon.match.same(futureFactory),
sinon.match.same(flow),
sinon.match.same(closureContext),
Expand Down Expand Up @@ -228,6 +234,7 @@ describe('FunctionSpecFactory', function () {
sinon.match.same(callStack),
sinon.match.same(translator),
sinon.match.same(valueFactory),
sinon.match.same(referenceFactory),
sinon.match.same(futureFactory),
sinon.match.same(flow),
sinon.match.same(closureContext),
Expand Down Expand Up @@ -269,6 +276,7 @@ describe('FunctionSpecFactory', function () {
sinon.match.same(callStack),
sinon.match.same(translator),
sinon.match.same(valueFactory),
sinon.match.same(referenceFactory),
sinon.match.same(futureFactory),
sinon.match.same(flow),
sinon.match.same(closureContext),
Expand Down Expand Up @@ -346,6 +354,7 @@ describe('FunctionSpecFactory', function () {
sinon.match.same(callStack),
sinon.match.same(translator),
sinon.match.same(valueFactory),
sinon.match.same(referenceFactory),
sinon.match.same(futureFactory),
sinon.match.same(flow),
sinon.match.same(functionContext),
Expand Down Expand Up @@ -424,6 +433,7 @@ describe('FunctionSpecFactory', function () {
sinon.match.same(callStack),
sinon.match.same(translator),
sinon.match.same(valueFactory),
sinon.match.same(referenceFactory),
sinon.match.same(futureFactory),
sinon.match.same(flow),
sinon.match.same(methodContext),
Expand Down
53 changes: 46 additions & 7 deletions test/unit/Function/FunctionSpecTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ var expect = require('chai').expect,
Parameter = require('../../../src/Function/Parameter'),
PHPError = phpCommon.PHPError,
Reference = require('../../../src/Reference/Reference'),
ReferenceFactory = require('../../../src/ReferenceFactory').sync(),
ReferenceSlot = require('../../../src/Reference/ReferenceSlot'),
ReferenceSnapshot = require('../../../src/Reference/ReferenceSnapshot'),
Scope = require('../../../src/Scope').sync(),
TypeInterface = require('../../../src/Type/TypeInterface'),
Translator = phpCommon.Translator,
Expand All @@ -43,6 +45,7 @@ describe('FunctionSpec', function () {
parameter1,
parameter2,
parameterList,
referenceFactory,
returnType,
spec,
state,
Expand All @@ -64,6 +67,7 @@ describe('FunctionSpec', function () {
parameter1 = sinon.createStubInstance(Parameter);
parameter2 = sinon.createStubInstance(Parameter);
parameterList = [parameter1, parameter2];
referenceFactory = sinon.createStubInstance(ReferenceFactory);
returnType = sinon.createStubInstance(TypeInterface);
valueFactory = state.getValueFactory();

Expand Down Expand Up @@ -102,6 +106,7 @@ describe('FunctionSpec', function () {
callStack,
translator,
valueFactory,
referenceFactory,
futureFactory,
flow,
context,
Expand All @@ -118,18 +123,37 @@ describe('FunctionSpec', function () {

describe('coerceArguments()', function () {
var argument1,
argument2;
argument2,
coercedArgument1,
coercedArgument2,
snapshot1,
snapshot2;

beforeEach(function () {
argument1 = valueFactory.createString('first uncoerced');
argument2 = valueFactory.createString('second uncoerced');
argument1 = sinon.createStubInstance(Reference);
argument2 = sinon.createStubInstance(Reference);
snapshot1 = sinon.createStubInstance(ReferenceSnapshot);
snapshot2 = sinon.createStubInstance(ReferenceSnapshot);

argument1.getValue.returns(valueFactory.createString('first uncoerced'));
argument2.getValue.returns(valueFactory.createString('second uncoerced'));

coercedArgument1 = valueFactory.createString('first coerced');
coercedArgument2 = valueFactory.createString('second coerced');

parameter1.coerceArgument
.withArgs(sinon.match.same(argument1))
.returns(valueFactory.createString('first coerced'));
.returns(coercedArgument1);
parameter2.coerceArgument
.withArgs(sinon.match.same(argument2))
.returns(valueFactory.createString('second coerced'));
.returns(coercedArgument2);

referenceFactory.createSnapshot
.withArgs(argument1, coercedArgument1)
.returns(snapshot1);
referenceFactory.createSnapshot
.withArgs(argument2, coercedArgument2)
.returns(snapshot2);
});

it('should return a new array of the coerced arguments', async function () {
Expand Down Expand Up @@ -176,8 +200,23 @@ describe('FunctionSpec', function () {
expect(result[0].getNative()).to.equal('first coerced');
expect(result[1].getNative()).to.equal('second coerced');
expect(argumentReferences).to.have.length(2);
// Not overwritten, as we need to preserve the reference to pass through.
expect(argumentReferences[0].getNative()).to.equal('first uncoerced');
// Snapshotted, as we need to preserve both the reference and its snapshotted value to pass through.
expect(argumentReferences[0]).to.equal(snapshot1);
expect(argumentReferences[1].getNative()).to.equal('second coerced');
});

it('should overwrite by-value arguments in the reference list with their resolved values', async function () {
var argumentReferences,
result;
argumentReferences = [argument1, argument2];

result = await spec.coerceArguments(argumentReferences).toPromise();

expect(result).to.have.length(2);
expect(result[0].getNative()).to.equal('first coerced');
expect(result[1].getNative()).to.equal('second coerced');
expect(argumentReferences).to.have.length(2);
expect(argumentReferences[0].getNative()).to.equal('first coerced');
expect(argumentReferences[1].getNative()).to.equal('second coerced');
});
});
Expand Down

0 comments on commit 4d0cef5

Please sign in to comment.