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

Supported exporting closures with use() variables #7

Merged
merged 1 commit into from
Jan 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -336,10 +336,34 @@ function (\My\App\Service $service) : \My\App\Model\Entity {

Note how all namespaced classes, and explicitly namespaced functions and constants, have been rewritten, while the non-namespaced function `strlen()` and the non-namespaced constant have been left as is. This brings us to the first caveat:

## Use statements

By default, exporting closures that have variables bound through `use()` will throw an `ExportException`. This is intentional, because exported closures can be executed in another context, and as such must not rely on the context they've been originally defined in.

When using the `CLOSURE_SNAPSHOT_USE` option, `VarExporter` will export the current value of each `use()` variable instead of throwing an exception. The exported variables are added as expression inside the exported closure.

```php
$planet = 'world';

echo VarExporter::export([
'callback' => function() use ($planet) {
return 'Hello, ' . $planet . '!';
}
], VarExporter::CLOSURE_SNAPSHOT_USE);
```

```php
[
'callback' => function () {
$planet = 'world';
return 'Hello, ' . $planet . '!';
}
]
```

### Caveats

- **Functions and constants that are not not explicitly namespaced**, either directly or through a `use function` or `use const` statement, **are always exported as is**. This is because the parser does not have the runtime context to check if a definition for this function or constant exists in the current namespace, and as such cannot reliably predict the behaviour of PHP's [fallback to global function/constant](https://www.php.net/manual/en/language.namespaces.fallback.php). Be really careful here if you're using namespaced functions or constants: **always explicitly import your namespaced functions and constants**, if any.
- **Closures that have variables bound through `use()` cannot be exported**, and will throw an `ExportException`. This is intentional, because exported closures can be executed in another context, and as such must not rely on the context they've been originally defined in.
- Closures can use `$this`, but **will not be bound to an object once exported**. You must explicitly bind them through [`bindTo()`](https://www.php.net/manual/en/closure.bindto.php) if required, after running the exported code.
- **You cannot have 2 closures on the same line in your source file**, or an `ExportException` will be thrown. This is because `VarExporter` cannot know which one holds the definition for the `\Closure` object it encountered.
- **Closures defined in eval()'d code cannot be exported** and throw an `ExportException`, because there is no source file to parse.
Expand Down
6 changes: 6 additions & 0 deletions src/Internal/GenericExporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ final class GenericExporter
*/
public $inlineNumericScalarArray;

/**
* @var bool
*/
public $closureSnapshotUses;

/**
* @param int $options
*/
Expand Down Expand Up @@ -76,6 +81,7 @@ public function __construct(int $options)
$this->addTypeHints = (bool) ($options & VarExporter::ADD_TYPE_HINTS);
$this->skipDynamicProperties = (bool) ($options & VarExporter::SKIP_DYNAMIC_PROPERTIES);
$this->inlineNumericScalarArray = (bool) ($options & VarExporter::INLINE_NUMERIC_SCALAR_ARRAY);
$this->closureSnapshotUses = (bool) ($options & VarExporter::CLOSURE_SNAPSHOT_USES);
}

/**
Expand Down
92 changes: 81 additions & 11 deletions src/Internal/ObjectExporter/ClosureExporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
use PhpParser\NodeTraverser;
use PhpParser\NodeVisitor\FindingVisitor;
use PhpParser\NodeVisitor\NameResolver;
use PhpParser\Parser;
use PhpParser\ParserFactory;
use ReflectionFunction;

/**
* Handles closures.
Expand All @@ -21,6 +23,11 @@
*/
class ClosureExporter extends ObjectExporter
{
/**
* @var Parser|null
*/
private $parser;

/**
* {@inheritDoc}
*/
Expand All @@ -42,7 +49,7 @@ public function export($object, \ReflectionObject $reflectionObject, array $path
$ast = $this->parseFile($file, $path);
$ast = $this->resolveNames($ast);

$closure = $this->getClosure($ast, $file, $line, $path);
$closure = $this->getClosure($reflectionFunction, $ast, $file, $line, $path);

$prettyPrinter = new ClosureExporter\PrettyPrinter();
$prettyPrinter->setVarExporterNestingLevel(count($path));
Expand All @@ -55,6 +62,18 @@ public function export($object, \ReflectionObject $reflectionObject, array $path
return [$code];
}

/**
* @return Parser
*/
private function getParser()
{
if ($this->parser === null) {
$this->parser = (new ParserFactory)->create(ParserFactory::ONLY_PHP7);
}

return $this->parser;
}

/**
* Parses the given source file.
*
Expand All @@ -79,10 +98,8 @@ private function parseFile(string $filename, array $path) : array
// @codeCoverageIgnoreEnd
}

$parser = (new ParserFactory)->create(ParserFactory::ONLY_PHP7);

try {
return $parser->parse($source);
return $this->getParser()->parse($source);
// @codeCoverageIgnoreStart
} catch (Error $e) {
throw new ExportException("Cannot parse file \"$filename\" for reading closure code.", $path, $e);
Expand All @@ -109,17 +126,23 @@ private function resolveNames(array $ast) : array
/**
* Finds a closure in the source file and returns its node.
*
* @param array $ast The AST.
* @param string $file The file name.
* @param int $line The line number where the closure is located in the source file.
* @param string[] $path The path to the closure in the array/object graph.
* @param ReflectionFunction $reflectionFunction Reflection of the closure.
* @param array $ast The AST.
* @param string $file The file name.
* @param int $line The line number where the closure is located in the source file.
* @param string[] $path The path to the closure in the array/object graph.
*
* @return Node\Expr\Closure
*
* @throws ExportException
*/
private function getClosure(array $ast, string $file, int $line, array $path) : Node\Expr\Closure
{
private function getClosure(
ReflectionFunction $reflectionFunction,
array $ast,
string $file,
int $line,
array $path
) : Node\Expr\Closure {
$finder = new FindingVisitor(function(Node $node) use ($line) : bool {
return $node instanceof Node\Expr\Closure
&& $node->getStartLine() === $line;
Expand All @@ -145,9 +168,56 @@ private function getClosure(array $ast, string $file, int $line, array $path) :
$closure = $closures[0];

if ($closure->uses) {
throw new ExportException("The closure has bound variables through 'use', this is not supported.", $path);
$this->closureHandleUses($reflectionFunction, $closure, $path);
}

return $closure;
}

/**
* Handle `use` part of closure.
*
* @param ReflectionFunction $reflectionFunction Reflection of the closure.
* @param Node\Expr\Closure $closure Parsed closure.
* @param string[] $path The path to the closure in the array/object graph.
*
* @throws ExportException
*/
private function closureHandleUses(
ReflectionFunction $reflectionFunction,
Node\Expr\Closure $closure,
array $path
) : void {
if (! $this->exporter->closureSnapshotUses) {
throw new ExportException(
"The closure has bound variables through 'use', this is not supported by default. " .
"Use the `CLOSURE_SNAPSHOT_USE` option to export them.",
$path
);
}

$static = $reflectionFunction->getStaticVariables();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that getStaticVariables() returned use statements, too. Is this documented anywhere? The manual doesn't say anything about it, so this makes me a bit chilly to rely on it!

This also made me curious: what happens if you have both a use statement and a static variable with the same name? It looks like use wins, so at least we should be safe here:

https://3v4l.org/jbGIf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getStaticVariables() returns op_array.static_variables. This is intended for variables bound with static, but closures also use this array for the use variables.

I asked @nikic; This is an implementation detail and might be changed in the future (PHP 8). However, it should always be possible to get these vars using reflection. So if it's changed, a method like ReflectionFunction::getUseVariables() would be added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, with good unit tests we should be covered anyway. Maybe it's time to start testing against nightly (with allow_failures), so that we know right away if this implementation detail changes?

$stmts = [];

$parser = $this->getParser();

foreach ($closure->uses as $use) {
$var = $use->var->name;

$export = array_merge(['<?php '], $this->exporter->export($static[$var], $path, []), [';']);
$nodes = $parser->parse(implode(PHP_EOL, $export));

/** @var Node\Stmt\Expression $expr */
$expr = $nodes[0];

$assign = new Node\Expr\Assign(
new Node\Expr\Variable($var),
$expr->expr
);
$stmts[] = new Node\Stmt\Expression($assign);
}

$closure->uses = [];
$closure->stmts = array_merge($stmts, $closure->stmts);
}
}
5 changes: 5 additions & 0 deletions src/VarExporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ final class VarExporter
*/
public const INLINE_NUMERIC_SCALAR_ARRAY = 1 << 7;

/**
* Export static vars defined via `use` as variables.
*/
public const CLOSURE_SNAPSHOT_USES = 1 << 8;

/**
* @param mixed $var The variable to export.
* @param int $options A bitmask of options. Possible values are `VarExporter::*` constants.
Expand Down
51 changes: 50 additions & 1 deletion tests/ExportClosureTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,56 @@ public function testExportClosureWithUse()
return $foo;
};

$this->assertExportThrows("The closure has bound variables through 'use', this is not supported.", $var);
$this->assertExportThrows(
"The closure has bound variables through 'use', this is not supported by default. " .
"Use the `CLOSURE_SNAPSHOT_USE` option to export them.",
$var
);
}

public function testExportClosureWithUseAsVars()
{
$foo = 'b' . 'a' . 'r';

$var = function() use ($foo) {
return $foo;
};

$expected = <<<'PHP'
return function () {
$foo = 'bar';
return $foo;
};

PHP;

$this->assertExportEquals($expected, $var, VarExporter::ADD_RETURN | VarExporter::CLOSURE_SNAPSHOT_USES);
}

public function testExportClosureWithUseClosure()
{
$foo = 'b' . 'a' . 'r';

$sub = function () use ($foo) {
return $foo;
};

$var = function() use ($sub) {
return $sub();
};

$expected = <<<'PHP'
return function () {
$sub = function () {
$foo = 'bar';
return $foo;
};
return $sub();
};

PHP;

$this->assertExportEquals($expected, $var, VarExporter::ADD_RETURN | VarExporter::CLOSURE_SNAPSHOT_USES);
}

public function testExportClosureDefinedInEval()
Expand Down