diff --git a/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php b/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php index cb8411fed35..0ae11f6cdfa 100644 --- a/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php +++ b/src/Analyser/ExprHandler/Helper/ClosureTypeResolver.php @@ -72,12 +72,15 @@ final class ClosureTypeResolver implements PerFileAnalysisResettable * file's analysis ends - the per-file reset releases the Types and * throw/impure points with the rest of the file's result graph. * - * Keyed by the closure node's spl_object_id(). The keys are AST nodes that - * live for the whole file's analysis (the parser cache retains them), so - * ids of live entries never collide; the per-file reset empties the map - * before another file could reuse them. + * Keyed by the closure node's spl_object_id(), which PHP hands out again + * as soon as a node is freed. The parsed file's nodes live for its whole + * analysis, but a closure node that is built or parsed and dropped mid-file + * - by a handler desugaring a call, or by an extension or a test walking + * its own nodes through NodeScopeResolver - frees its id for the next + * allocation. Each entry therefore pins the node it was built for, and + * findCachedTypes() answers for that very node only. * - * @var array> + * @var array}> */ private array $cachedTypes = []; @@ -93,6 +96,19 @@ public function resetFileAnalysisState(): void $this->cachedTypes = []; } + /** + * @return array + */ + private function findCachedTypes(Node\Expr\Closure|ArrowFunction $expr): array + { + $entry = $this->cachedTypes[spl_object_id($expr)] ?? null; + if ($entry === null || $entry['expr'] !== $expr) { + return []; + } + + return $entry['types']; + } + /** * Resolves a closure/arrow function type by walking its body itself. Used by * the paths that have no prior walk to read return/yield types from. A @@ -128,7 +144,7 @@ public function getClosureType( ); } - $cachedTypes = $this->cachedTypes[spl_object_id($expr)] ?? []; + $cachedTypes = $this->findCachedTypes($expr); $cacheKey = $this->closureContextCacheKey($scope, $expr, $callableParameters, $parameters); if (array_key_exists($cacheKey, $cachedTypes)) { return $this->createClosureTypeFromCache($expr, $parameters, $isVariadic, $cachedTypes[$cacheKey]); @@ -387,7 +403,7 @@ public function seedCacheFromClosureWalk(MutatingScope $scope, Node\Expr\Closure [$parameters, , $callableParameters] = $this->buildParametersAndAcceptors($scope, $expr); $phpdocKey = $this->closureContextCacheKey($scope, $expr, $callableParameters, $parameters); - $cachedTypes = $this->cachedTypes[spl_object_id($expr)] ?? []; + $cachedTypes = $this->findCachedTypes($expr); if (!array_key_exists($phpdocKey, $cachedTypes)) { return; } @@ -395,7 +411,7 @@ public function seedCacheFromClosureWalk(MutatingScope $scope, Node\Expr\Closure $promotedScope = $scope->doNotTreatPhpDocTypesAsCertain(); [$promotedParameters, , $promotedCallableParameters] = $this->buildParametersAndAcceptors($promotedScope, $expr); $cachedTypes[$this->closureContextCacheKey($promotedScope, $expr, $promotedCallableParameters, $promotedParameters)] = $cachedTypes[$phpdocKey]; - $this->cachedTypes[spl_object_id($expr)] = $cachedTypes; + $this->cachedTypes[spl_object_id($expr)] = ['expr' => $expr, 'types' => $cachedTypes]; } /** @@ -912,7 +928,7 @@ private function assembleClosureType( $impurePointsForClosureType = array_map(static fn (ImpurePoint $impurePoint) => new SimpleImpurePoint($impurePoint->getIdentifier(), $impurePoint->getDescription(), $impurePoint->isCertain()), $impurePoints); if ($writeCache) { - $cachedTypes = $this->cachedTypes[spl_object_id($expr)] ?? []; + $cachedTypes = $this->findCachedTypes($expr); $cacheKey ??= $this->closureContextCacheKey($scope, $expr, null, $parameters); $cachedTypes[$cacheKey] = [ 'returnType' => $returnType, @@ -921,7 +937,7 @@ private function assembleClosureType( 'invalidateExpressions' => $invalidateExpressions, 'usedVariables' => $usedVariables, ]; - $this->cachedTypes[spl_object_id($expr)] = $cachedTypes; + $this->cachedTypes[spl_object_id($expr)] = ['expr' => $expr, 'types' => $cachedTypes]; } $mustUseReturnValue = TrinaryLogic::createNo(); diff --git a/src/Analyser/ExprHandlerRegistry.php b/src/Analyser/ExprHandlerRegistry.php index e04dcac8cbc..66709a33960 100644 --- a/src/Analyser/ExprHandlerRegistry.php +++ b/src/Analyser/ExprHandlerRegistry.php @@ -18,6 +18,18 @@ final class ExprHandlerRegistry /** @var array|false>> */ private static array $exprHandlersByClass = []; + /** + * Handlers belong to the container they came from, and spl_object_id() is + * recycled as soon as a container is freed - so ContainerFactory drops the + * memo whenever another container takes over the global state. + * + * @internal + */ + public static function clearCache(): void + { + self::$exprHandlersByClass = []; + } + /** * @return ExprHandler|null */ diff --git a/src/Analyser/StmtHandlerRegistry.php b/src/Analyser/StmtHandlerRegistry.php index aac7b43c669..793e9f4cfa7 100644 --- a/src/Analyser/StmtHandlerRegistry.php +++ b/src/Analyser/StmtHandlerRegistry.php @@ -23,6 +23,15 @@ final class StmtHandlerRegistry /** @var array, StmtHandler|false>> */ private static array $stmtHandlersByClass = []; + /** + * @see ExprHandlerRegistry::clearCache() + * @internal + */ + public static function clearCache(): void + { + self::$stmtHandlersByClass = []; + } + /** * @return StmtHandler|null */ diff --git a/src/DependencyInjection/ContainerFactory.php b/src/DependencyInjection/ContainerFactory.php index 25bac54af6b..653e493b906 100644 --- a/src/DependencyInjection/ContainerFactory.php +++ b/src/DependencyInjection/ContainerFactory.php @@ -17,6 +17,8 @@ use Nette\Utils\Validators; use Phar; use PhpParser\Parser; +use PHPStan\Analyser\ExprHandlerRegistry; +use PHPStan\Analyser\StmtHandlerRegistry; use PHPStan\BetterReflection\BetterReflection; use PHPStan\BetterReflection\Reflector\Reflector; use PHPStan\BetterReflection\SourceLocator\SourceStubber\PhpStormStubsSourceStubber; @@ -32,6 +34,7 @@ use PHPStan\ShouldNotHappenException; use PHPStan\Type\ObjectType; use PHPStan\Type\TypeCombinator; +use WeakReference; use function array_diff_key; use function array_intersect; use function array_key_exists; @@ -49,7 +52,6 @@ use function is_file; use function is_readable; use function is_string; -use function spl_object_id; use function sprintf; use function str_ends_with; use function substr; @@ -66,7 +68,8 @@ final class ContainerFactory private string $configDirectory; - private static ?int $lastInitializedContainerId = null; + /** @var WeakReference|null */ + private static ?WeakReference $lastInitializedContainer = null; private bool $journalContainer = false; @@ -172,12 +175,22 @@ public function create( /** @internal */ public static function postInitializeContainer(Container $container): void { - $containerId = spl_object_id($container); - if ($containerId === self::$lastInitializedContainerId) { + // Comparing spl_object_id()s would be wrong: an id is recycled as soon as its + // container is freed, so a fresh container could inherit the last initialized + // one's id and silently skip installing its own global state. + $lastInitializedContainer = self::$lastInitializedContainer !== null ? self::$lastInitializedContainer->get() : null; + if ($lastInitializedContainer === $container) { return; } - self::$lastInitializedContainerId = $containerId; + self::$lastInitializedContainer = WeakReference::create($container); + + // Both registries memoize handler instances per container, keyed by the + // container's spl_object_id() - a recycled id would otherwise hand this + // container the freed one's handlers, which carry the other container's + // services and parameters. + ExprHandlerRegistry::clearCache(); + StmtHandlerRegistry::clearCache(); /** @var SourceLocator $sourceLocator */ $sourceLocator = $container->getService('betterReflectionSourceLocator'); diff --git a/src/Testing/PHPStanTestCase.php b/src/Testing/PHPStanTestCase.php index d06c9e3b41a..b80e313e11e 100644 --- a/src/Testing/PHPStanTestCase.php +++ b/src/Testing/PHPStanTestCase.php @@ -22,6 +22,7 @@ use PHPStan\Reflection\ReflectionProvider; use PHPStan\Reflection\ReflectionProvider\DirectReflectionProviderProvider; use PHPStan\Rules\Properties\PropertyReflectionFinder; +use PHPStan\Testing\PHPUnit\ContainerInitializer; use PHPStan\Type\Constant\OversizedArrayBuilder; use PHPStan\Type\ExpressionTypeResolverExtension; use PHPStan\Type\OperatorTypeSpecifyingExtensionRegistry; @@ -43,21 +44,28 @@ abstract class PHPStanTestCase extends TestCase use PHPStanTestCaseTrait; /** - * Re-register the runtime container as the global static reflection provider before - * every test. Enable this in tests that construct Type objects directly and assert - * PHP-version-dependent reflection results, so a foreign PhpVersion leaked by another - * test can't flake them. See https://github.com/phpstan/phpstan/issues/14860 + * Re-register this test class's container as the owner of the global static state + * (the reflection provider, the PhpVersion, the bleeding edge toggle) before every + * test, so a container another test class installed can't flake tests that build + * Type objects directly. See https://github.com/phpstan/phpstan/issues/14860 + * + * This is the only hook that does it, on every PHPUnit version. Data providers + * are covered separately - see PHPStanTestCaseTrait::restoreBaseContainer(). */ - protected bool $reinitializeContainerBeforeEachTest = false; - #[Override] protected function setUp(): void { - if (!$this->reinitializeContainerBeforeEachTest) { - return; - } + ContainerInitializer::initialize(static::class); + } + + #[Override] + public static function tearDownAfterClass(): void + { + parent::tearDownAfterClass(); - self::getContainer(); + // The next test class's data providers run before any hook this test case has + // on PHPUnit 9, in the same ParaTest worker process - see restoreBaseContainer(). + self::restoreBaseContainer(); } public static function getParser(): Parser diff --git a/src/Testing/PHPStanTestCaseTrait.php b/src/Testing/PHPStanTestCaseTrait.php index 14a0e68cc63..8a0e45a0a27 100644 --- a/src/Testing/PHPStanTestCaseTrait.php +++ b/src/Testing/PHPStanTestCaseTrait.php @@ -22,11 +22,43 @@ trait PHPStanTestCaseTrait public static function getContainer(): Container { - $additionalConfigFiles = [__DIR__ . '/TestCase.neon']; + $additionalConfigFiles = [self::getBaseConfigFile()]; foreach (static::getAdditionalConfigFiles() as $configFile) { $additionalConfigFiles[] = $configFile; } - $composerAutoloaderProjectPaths = static::getComposerAutoloaderProjectPaths(); + + return self::getContainerForConfigFiles($additionalConfigFiles, static::getComposerAutoloaderProjectPaths()); + } + + /** + * Installs the global state of the container built from just the base config - + * the bleeding edge toggle, the static reflection provider, the PhpVersion and + * the type caches all live in static properties that the last initialized + * container owns. + * + * PHPStanTestCase::setUp() installs the right container before every test. Data + * providers are called while the test suite is being built, so on PHPUnit >= 10 + * InitContainerBeforeDataProviderSubscriber installs it before each of them. + * PHPUnit 9 - which the "Tests with old PHPUnit" jobs run - rejects that extension + * and has no hook that fires before a data provider, while ParaTest reuses one + * worker process for many test files. A + * class whose container turns on e.g. bleeding edge would therefore leak that + * state into the data providers of the next test file, which build Type objects + * that read the toggle in their constructor. Restoring the base state when a test + * class is done keeps those data providers deterministic on every supported + * PHPUnit version. + */ + public static function restoreBaseContainer(): void + { + self::getContainerForConfigFiles([self::getBaseConfigFile()], []); + } + + /** + * @param string[] $additionalConfigFiles + * @param string[] $composerAutoloaderProjectPaths + */ + private static function getContainerForConfigFiles(array $additionalConfigFiles, array $composerAutoloaderProjectPaths): Container + { $cacheKey = hash('sha256', implode("\n", $additionalConfigFiles) . "\0" . implode("\n", $composerAutoloaderProjectPaths)); if (!isset(self::$containers[$cacheKey])) { @@ -66,6 +98,11 @@ public static function getContainer(): Container return self::$containers[$cacheKey]; } + private static function getBaseConfigFile(): string + { + return __DIR__ . '/TestCase.neon'; + } + /** * @return string[] */ diff --git a/src/Testing/PHPUnit/InitContainerBeforeTestSubscriber.php b/src/Testing/PHPUnit/InitContainerBeforeTestSubscriber.php deleted file mode 100644 index 768d6d3e456..00000000000 --- a/src/Testing/PHPUnit/InitContainerBeforeTestSubscriber.php +++ /dev/null @@ -1,32 +0,0 @@ -test(); - if (!$test->isTestMethod()) { - // skip PHPT tests - return; - } - - $testClassName = $test->className(); - - if (!is_a($testClassName, PHPStanTestCase::class, true)) { - return; - } - - ContainerInitializer::initialize($testClassName); - } - -} diff --git a/src/Testing/PHPUnit/PHPStanPHPUnitExtension.php b/src/Testing/PHPUnit/PHPStanPHPUnitExtension.php index 7a4ec992670..ff52d637b92 100644 --- a/src/Testing/PHPUnit/PHPStanPHPUnitExtension.php +++ b/src/Testing/PHPUnit/PHPStanPHPUnitExtension.php @@ -21,9 +21,6 @@ public function bootstrap( $facade->registerSubscriber( new InitContainerBeforeDataProviderSubscriber(), ); - $facade->registerSubscriber( - new InitContainerBeforeTestSubscriber(), - ); } } diff --git a/tests/PHPStan/Analyser/ExpressionResultTest.php b/tests/PHPStan/Analyser/ExpressionResultTest.php index 4da555b362f..c612ddd55eb 100644 --- a/tests/PHPStan/Analyser/ExpressionResultTest.php +++ b/tests/PHPStan/Analyser/ExpressionResultTest.php @@ -2,6 +2,9 @@ namespace PHPStan\Analyser; +use PhpParser\Node\Expr\ArrowFunction; +use PhpParser\Node\Expr\Exit_; +use PhpParser\Node\Expr\FuncCall; use PhpParser\Node\Stmt; use PHPStan\Parser\Parser; use PHPStan\ShouldNotHappenException; @@ -193,9 +196,33 @@ public function testIsAlwaysTerminating( if (!$stmts[0] instanceof Stmt\Expression) { throw new ShouldNotHappenException('Expecting code contains a single statement expression, got: ' . get_class($stmts[0])); } - $stmt = $stmts[0]; - $expr = $stmt->expr; + $this->assertSame($expectedIsAlwaysTerminating, $this->processExpressionStatement($stmts[0])->isAlwaysTerminating()); + } + + public function testClosureTypeIsNotReusedForClosureWithRecycledObjectId(): void + { + /** @var Parser $parser */ + $parser = self::getContainer()->getService('currentPhpVersionRichParser'); + /** @var Stmt\Expression $generatorStmt */ + $generatorStmt = $parser->parseString(' yield (exit());')[0]; + $generatorArrowFunction = $generatorStmt->expr; + $this->assertInstanceOf(ArrowFunction::class, $generatorArrowFunction); + $this->assertFalse($this->processExpressionStatement($generatorStmt)->isAlwaysTerminating()); + + // PHP hands the most recently freed object id to the next allocation - drop the + // generator arrow function last, and build the next arrow function right after, + // so it gets the same spl_object_id() unless something still holds the first one + $exit = new Exit_(); + unset($generatorStmt); + unset($generatorArrowFunction); + $arrowFunction = new ArrowFunction(['expr' => $exit], ['isImmediatelyInvokedClosure' => true, 'immediatelyInvokedClosureArgs' => []]); + + $this->assertTrue($this->processExpressionStatement(new Stmt\Expression(new FuncCall($arrowFunction)))->isAlwaysTerminating()); + } + + private function processExpressionStatement(Stmt\Expression $stmt): ExpressionResult + { /** @var NodeScopeResolver $nodeScopeResolver */ $nodeScopeResolver = self::getContainer()->getByType(NodeScopeResolver::class); /** @var ScopeFactory $scopeFactory */ @@ -204,16 +231,15 @@ public function testIsAlwaysTerminating( ->assignVariable('x', new IntegerType(), new IntegerType(), TrinaryLogic::createYes()) ->assignVariable('arr', new ArrayType(new MixedType(), new MixedType()), new ArrayType(new MixedType(), new MixedType()), TrinaryLogic::createYes()); - $result = $nodeScopeResolver->processExprNode( + return $nodeScopeResolver->processExprNode( $stmt, - $expr, + $stmt->expr, $scope, new ExpressionResultStorage(), static function (): void { }, ExpressionContext::createTopLevel(), ); - $this->assertSame($expectedIsAlwaysTerminating, $result->isAlwaysTerminating()); } } diff --git a/tests/PHPStan/Testing/BleedingEdgeContainerStateTest.php b/tests/PHPStan/Testing/BleedingEdgeContainerStateTest.php new file mode 100644 index 00000000000..222abde328a --- /dev/null +++ b/tests/PHPStan/Testing/BleedingEdgeContainerStateTest.php @@ -0,0 +1,31 @@ +assertTrue(BleedingEdgeToggle::isBleedingEdge()); + } + +} diff --git a/tests/PHPStan/Testing/ContainerStateTest.php b/tests/PHPStan/Testing/ContainerStateTest.php new file mode 100644 index 00000000000..d00e071e662 --- /dev/null +++ b/tests/PHPStan/Testing/ContainerStateTest.php @@ -0,0 +1,18 @@ +assertFalse(BleedingEdgeToggle::isBleedingEdge()); + } + +} diff --git a/tests/PHPStan/Type/Accessory/HasPropertyTypeTest.php b/tests/PHPStan/Type/Accessory/HasPropertyTypeTest.php index 5771e7d52da..8219bc5eab3 100644 --- a/tests/PHPStan/Type/Accessory/HasPropertyTypeTest.php +++ b/tests/PHPStan/Type/Accessory/HasPropertyTypeTest.php @@ -23,11 +23,6 @@ class HasPropertyTypeTest extends PHPStanTestCase { - // Pin the runtime container so a foreign PhpVersion leaked by another test - // can't flake the version-dependent Closure data set below. - // See https://github.com/phpstan/phpstan/issues/14860 - protected bool $reinitializeContainerBeforeEachTest = true; - public static function dataIsSuperTypeOf(): array { return [ diff --git a/tests/PHPStan/Type/Generic/GenericObjectTypeTest.php b/tests/PHPStan/Type/Generic/GenericObjectTypeTest.php index faca30db63a..16c80efdc7a 100644 --- a/tests/PHPStan/Type/Generic/GenericObjectTypeTest.php +++ b/tests/PHPStan/Type/Generic/GenericObjectTypeTest.php @@ -32,11 +32,6 @@ class GenericObjectTypeTest extends PHPStanTestCase { - // Pin the runtime container so a foreign PhpVersion leaked by another test - // can't flake the version-dependent data sets (reflected variance of built-in - // generics). See https://github.com/phpstan/phpstan/issues/14860 - protected bool $reinitializeContainerBeforeEachTest = true; - public static function dataIsSuperTypeOf(): array { return [ diff --git a/tests/PHPStan/Type/ObjectTypeTest.php b/tests/PHPStan/Type/ObjectTypeTest.php index 9df34437fcf..0c218d5063b 100644 --- a/tests/PHPStan/Type/ObjectTypeTest.php +++ b/tests/PHPStan/Type/ObjectTypeTest.php @@ -53,11 +53,6 @@ class ObjectTypeTest extends PHPStanTestCase { - // Pin the runtime container so a foreign PhpVersion leaked by another test - // can't flake the version-dependent Closure data sets (dynamic-property - // handling). See https://github.com/phpstan/phpstan/issues/14860 - protected bool $reinitializeContainerBeforeEachTest = true; - public static function dataIsIterable(): array { return [ diff --git a/tests/PHPStan/Type/TypeCombinatorTest.php b/tests/PHPStan/Type/TypeCombinatorTest.php index ad6dfe96a33..c4da5fd93ee 100644 --- a/tests/PHPStan/Type/TypeCombinatorTest.php +++ b/tests/PHPStan/Type/TypeCombinatorTest.php @@ -74,11 +74,6 @@ class TypeCombinatorTest extends PHPStanTestCase { - // Pin the runtime container so a foreign PhpVersion leaked by another test - // can't flake the version-dependent data sets (dynamic-property handling of - // final classes). See https://github.com/phpstan/phpstan/issues/14860 - protected bool $reinitializeContainerBeforeEachTest = true; - public static function dataAddNull(): array { return [ diff --git a/tests/PHPStan/Type/TypeGetFiniteTypesTest.php b/tests/PHPStan/Type/TypeGetFiniteTypesTest.php index e7e0ae2de6e..eb934142f8b 100644 --- a/tests/PHPStan/Type/TypeGetFiniteTypesTest.php +++ b/tests/PHPStan/Type/TypeGetFiniteTypesTest.php @@ -17,6 +17,12 @@ class TypeGetFiniteTypesTest extends PHPStanTestCase public static function dataGetFiniteTypes(): iterable { + // The expected ConstantArrayTypes below read the bleeding edge toggle in their + // constructor, so this class's container has to be the installed one already + // while they are built. On PHPUnit 9 nothing installs it before a data provider + // runs - see PHPStanTestCaseTrait::restoreBaseContainer(). + self::getContainer(); + yield [ IntegerRangeType::fromInterval(0, 5), [ diff --git a/tests/bootstrap.php b/tests/bootstrap.php index aeb8edcb42e..8f777c18c12 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -1,5 +1,6 @@