From b1bbc44904f2695910fdd2b3f8bb9c3d72c10928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20FIDRY?= Date: Sat, 30 Sep 2017 09:16:00 +0100 Subject: [PATCH] Make traverser return a new instance for each file As the traverser is now stateful, even though it won't break things as the code is robust enough right now, it severely impacts performances. For example, the collection of namespace was kept which led to a lot of namespaces at some point forcing the NamespaceCollection to go through the node parents to find the proper namespace, even though only one namespace was found in the file. --- src/Scoper/PhpScoper.php | 7 +- src/Scoper/TraverserFactory.php | 31 +++--- src/functions.php | 4 +- tests/Scoper/PhpScoperTest.php | 148 +++++++++++++++++++++++++- tests/Scoper/TraverserFactoryTest.php | 45 ++++++++ 5 files changed, 211 insertions(+), 24 deletions(-) create mode 100644 tests/Scoper/TraverserFactoryTest.php diff --git a/src/Scoper/PhpScoper.php b/src/Scoper/PhpScoper.php index 2311dfe4..e13ad544 100644 --- a/src/Scoper/PhpScoper.php +++ b/src/Scoper/PhpScoper.php @@ -29,11 +29,11 @@ final class PhpScoper implements Scoper private $decoratedScoper; private $traverserFactory; - public function __construct(Parser $parser, Scoper $decoratedScoper) + public function __construct(Parser $parser, Scoper $decoratedScoper, TraverserFactory $traverserFactory) { $this->parser = $parser; $this->decoratedScoper = $decoratedScoper; - $this->traverserFactory = new TraverserFactory(); + $this->traverserFactory = $traverserFactory; } /** @@ -51,9 +51,10 @@ public function scope(string $filePath, string $prefix, array $patchers, array $ $content = file_get_contents($filePath); + $statements = $this->parser->parse($content); + $traverser = $this->traverserFactory->create($prefix, $whitelist, $globalWhitelister); - $statements = $this->parser->parse($content); $statements = $traverser->traverse($statements); $prettyPrinter = new Standard(); diff --git a/src/Scoper/TraverserFactory.php b/src/Scoper/TraverserFactory.php index 2e00fd27..c0b22a40 100644 --- a/src/Scoper/TraverserFactory.php +++ b/src/Scoper/TraverserFactory.php @@ -21,7 +21,10 @@ use PhpParser\NodeTraverser; use PhpParser\NodeTraverserInterface; -final class TraverserFactory +/** + * @final + */ +class TraverserFactory { /** * Functions for which the arguments will be prefixed. @@ -31,8 +34,6 @@ final class TraverserFactory 'interface_exists', ]; - private $traverser; - /** * @param string $prefix Prefix to apply to the files. * @param string[] $whitelist List of classes to exclude from the scoping. @@ -44,31 +45,27 @@ final class TraverserFactory */ public function create(string $prefix, array $whitelist, callable $globalWhitelister): NodeTraverserInterface { - if (null !== $this->traverser) { - return $this->traverser; - } - - $this->traverser = new NodeTraverser(); + $traverser = new NodeTraverser(); $namespaceStatements = new NamespaceStmtCollection(); $useStatements = new UseStmtCollection(); $nameResolver = new FullyQualifiedNameResolver($namespaceStatements, $useStatements); - $this->traverser->addVisitor(new NodeVisitor\UseStmt\GroupUseStmtTransformer()); + $traverser->addVisitor(new NodeVisitor\UseStmt\GroupUseStmtTransformer()); - $this->traverser->addVisitor(new NodeVisitor\AppendParentNode()); + $traverser->addVisitor(new NodeVisitor\AppendParentNode()); - $this->traverser->addVisitor(new NodeVisitor\NamespaceStmtPrefixer($prefix, $namespaceStatements, $whitelist)); + $traverser->addVisitor(new NodeVisitor\NamespaceStmtPrefixer($prefix, $namespaceStatements, $whitelist)); - $this->traverser->addVisitor(new NodeVisitor\UseStmt\UseStmtCollector($namespaceStatements, $useStatements)); - $this->traverser->addVisitor(new NodeVisitor\UseStmt\UseStmtPrefixer($prefix, $whitelist, $globalWhitelister)); + $traverser->addVisitor(new NodeVisitor\UseStmt\UseStmtCollector($namespaceStatements, $useStatements)); + $traverser->addVisitor(new NodeVisitor\UseStmt\UseStmtPrefixer($prefix, $whitelist, $globalWhitelister)); - $this->traverser->addVisitor(new NodeVisitor\NameStmtPrefixer($prefix, $whitelist, $globalWhitelister, $nameResolver)); - $this->traverser->addVisitor(new NodeVisitor\StringScalarPrefixer($prefix, self::WHITELISTED_FUNCTIONS, $whitelist, $globalWhitelister, $nameResolver)); + $traverser->addVisitor(new NodeVisitor\NameStmtPrefixer($prefix, $whitelist, $globalWhitelister, $nameResolver)); + $traverser->addVisitor(new NodeVisitor\StringScalarPrefixer($prefix, self::WHITELISTED_FUNCTIONS, $whitelist, $globalWhitelister, $nameResolver)); - $this->traverser->addVisitor(new NodeVisitor\WhitelistedClassAppender($whitelist)); + $traverser->addVisitor(new NodeVisitor\WhitelistedClassAppender($whitelist)); - return $this->traverser; + return $traverser; } } diff --git a/src/functions.php b/src/functions.php index b257184e..87d43325 100644 --- a/src/functions.php +++ b/src/functions.php @@ -23,6 +23,7 @@ use Humbug\PhpScoper\Scoper\NullScoper; use Humbug\PhpScoper\Scoper\PatchScoper; use Humbug\PhpScoper\Scoper\PhpScoper; +use Humbug\PhpScoper\Scoper\TraverserFactory; use Humbug\SelfUpdate\Exception\RuntimeException as SelfUpdateRuntimeException; use Humbug\SelfUpdate\Updater; use PackageVersions\Versions; @@ -100,7 +101,8 @@ function create_scoper(): Scoper new InstalledPackagesScoper( new PhpScoper( create_parser(), - new NullScoper() + new NullScoper(), + new TraverserFactory() ) ) ) diff --git a/tests/Scoper/PhpScoperTest.php b/tests/Scoper/PhpScoperTest.php index ac53f713..ee99a441 100644 --- a/tests/Scoper/PhpScoperTest.php +++ b/tests/Scoper/PhpScoperTest.php @@ -17,7 +17,11 @@ use Generator; use Humbug\PhpScoper\PhpParser\FakeParser; use Humbug\PhpScoper\Scoper; +use LogicException; use PhpParser\Error as PhpParserError; +use PhpParser\Node\Name; +use PhpParser\NodeTraverserInterface; +use PhpParser\Parser; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; @@ -60,6 +64,36 @@ class PhpScoperTest extends TestCase */ private $decoratedScoper; + /** + * @var TraverserFactory|ObjectProphecy + */ + private $traverserFactoryProphecy; + + /** + * @var TraverserFactory + */ + private $traverserFactory; + + /** + * @var NodeTraverserInterface|ObjectProphecy + */ + private $traverserProphecy; + + /** + * @var NodeTraverserInterface + */ + private $traverser; + + /** + * @var Parser|ObjectProphecy + */ + private $parserProphecy; + + /** + * @var Parser + */ + private $parser; + /** * @inheritdoc */ @@ -67,7 +101,8 @@ public function setUp() { $this->scoper = new PhpScoper( create_parser(), - new FakeScoper() + new FakeScoper(), + new TraverserFactory() ); if (null === $this->tmp) { @@ -78,6 +113,15 @@ public function setUp() $this->decoratedScoperProphecy = $this->prophesize(Scoper::class); $this->decoratedScoper = $this->decoratedScoperProphecy->reveal(); + $this->traverserFactoryProphecy = $this->prophesize(TraverserFactory::class); + $this->traverserFactory = $this->traverserFactoryProphecy->reveal(); + + $this->traverserProphecy = $this->prophesize(NodeTraverserInterface::class); + $this->traverser = $this->traverserProphecy->reveal(); + + $this->parserProphecy = $this->prophesize(Parser::class); + $this->parser = $this->parserProphecy->reveal(); + chdir($this->tmp); } @@ -136,9 +180,15 @@ public function test_does_not_scope_file_if_is_not_a_PHP_file() ) ; + $this->traverserFactoryProphecy + ->create(Argument::cetera()) + ->willThrow(new LogicException('Unexpected call.')) + ; + $scoper = new PhpScoper( new FakeParser(), - $this->decoratedScoper + $this->decoratedScoper, + $this->traverserFactory ); $actual = $scoper->scope($filePath, $prefix, $patchers, $whitelist, $whitelister); @@ -209,9 +259,15 @@ public function test_does_not_scope_a_non_PHP_binary_files() ) ; + $this->traverserFactoryProphecy + ->create(Argument::cetera()) + ->willThrow(new LogicException('Unexpected call.')) + ; + $scoper = new PhpScoper( new FakeParser(), - $this->decoratedScoper + $this->decoratedScoper, + $this->traverserFactory ); $actual = $scoper->scope($filePath, $prefix, $patchers, $whitelist, $whitelister); @@ -253,6 +309,92 @@ public function test_cannot_scope_an_invalid_PHP_file() } } + public function test_creates_a_new_traverser_for_each_file() + { + $files = [ + 'file1.php' => 'file1', + 'file2.php' => 'file2', + ]; + + $prefix = 'Humbug'; + $patchers = [create_fake_patcher()]; + $whitelist = ['Foo']; + $whitelister = create_fake_whitelister(); + + $this->decoratedScoperProphecy + ->scope(Argument::any(), $prefix, $patchers, $whitelist, $whitelister) + ->willReturn( + $expected = 'Scoped content' + ) + ; + + $this->parserProphecy + ->parse('file1') + ->willReturn($file1Stmts = [ + new Name('file1'), + ]) + ; + $this->parserProphecy + ->parse('file2') + ->willReturn($file2Stmts = [ + new Name('file2'), + ]) + ; + + /** @var NodeTraverserInterface|ObjectProphecy $firstTraverserProphecy */ + $firstTraverserProphecy = $this->prophesize(NodeTraverserInterface::class); + /** @var NodeTraverserInterface $firstTraverser */ + $firstTraverser = $firstTraverserProphecy->reveal(); + + /** @var NodeTraverserInterface|ObjectProphecy $secondTraverserProphecy */ + $secondTraverserProphecy = $this->prophesize(NodeTraverserInterface::class); + /** @var NodeTraverserInterface $secondTraverser */ + $secondTraverser = $secondTraverserProphecy->reveal(); + + $i = 0; + $this->traverserFactoryProphecy + ->create($prefix, $whitelist, Argument::that( + function (...$args) use (&$i): bool { + $i++; + + return 1 === $i; + } + )) + ->willReturn($firstTraverser) + ; + $this->traverserFactoryProphecy + ->create($prefix, $whitelist, Argument::that( + function (...$args) use (&$i): bool { + $i++; + + return 4 === $i; + } + )) + ->willReturn($secondTraverser) + ; + + $firstTraverserProphecy->traverse($file1Stmts)->willReturn([]); + $secondTraverserProphecy->traverse($file2Stmts)->willReturn([]); + + $scoper = new PhpScoper( + $this->parser, + new FakeScoper(), + $this->traverserFactory + ); + + foreach ($files as $file => $content) { + touch($file); + file_put_contents($file, $content); + + $scoper->scope($file, $prefix, $patchers, $whitelist, $whitelister); + } + + $this->parserProphecy->parse(Argument::cetera())->shouldHaveBeenCalledTimes(2); + $this->traverserFactoryProphecy->create(Argument::cetera())->shouldHaveBeenCalledTimes(2); + $firstTraverserProphecy->traverse(Argument::cetera())->shouldHaveBeenCalledTimes(1); + $secondTraverserProphecy->traverse(Argument::cetera())->shouldHaveBeenCalledTimes(1); + } + /** * @dataProvider provideValidFiles */ diff --git a/tests/Scoper/TraverserFactoryTest.php b/tests/Scoper/TraverserFactoryTest.php new file mode 100644 index 00000000..fa00ab5e --- /dev/null +++ b/tests/Scoper/TraverserFactoryTest.php @@ -0,0 +1,45 @@ +, + * Pádraic Brady + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Humbug\PhpScoper\Scoper; + +use Humbug\PhpScoper\Scoper; +use PHPUnit\Framework\TestCase; +use function Humbug\PhpScoper\create_fake_patcher; +use function Humbug\PhpScoper\create_fake_whitelister; +use function Humbug\PhpScoper\escape_path; +use function Humbug\PhpScoper\make_tmp_dir; +use function Humbug\PhpScoper\remove_dir; + +/** + * @covers \Humbug\PhpScoper\Scoper\TraverserFactory + */ +class TraverserFactoryTest extends TestCase +{ + public function test_creates_a_new_traverser_at_each_call() + { + $prefix = 'Humbug'; + + $whitelist = ['Foo']; + + $whitelister = create_fake_whitelister(); + + $traverserFactory = new TraverserFactory(); + + $firstTraverser = $traverserFactory->create($prefix, $whitelist, $whitelister); + $secondTraverser = $traverserFactory->create($prefix, $whitelist, $whitelister); + + $this->assertNotSame($firstTraverser, $secondTraverser); + } +}