Make traverser return a new instance for each file (#107)

As the traverser is now statefull, 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.
humbug · Sep 30, 2017 · 31f994e · 31f994e
1 parent 504712c
commit 31f994e
Show file tree

Hide file tree

Showing 5 changed files with 207 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,14 +64,45 @@ 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
      */
     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
@@ -0,0 +1,41 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the humbug/php-scoper package.
+ *
+ * Copyright (c) 2017 Théo FIDRY <[email protected]>,
+ *                    Pádraic Brady <[email protected]>
+ *
+ * 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_whitelister;
+
+/**
+ * @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);
+    }
+}