Install the right container before every test and data provider on PHPUnit 9 too

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<int, array<string, array{returnType: Type, throwPoints: SimpleThrowPoint[], impurePoints: SimpleImpurePoint[], invalidateExpressions: InvalidateExprNode[], usedVariables: string[]}>>
+	 * @var array<int, array{expr: Node\Expr\Closure|ArrowFunction, types: array<string, array{returnType: Type, throwPoints: SimpleThrowPoint[], impurePoints: SimpleImpurePoint[], invalidateExpressions: InvalidateExprNode[], usedVariables: string[]}>}>
 	 */
 	private array $cachedTypes = [];
 
@@ -93,6 +96,19 @@ public function resetFileAnalysisState(): void
 		$this->cachedTypes = [];
 	}
 
+	/**
+	 * @return array<string, array{returnType: Type, throwPoints: SimpleThrowPoint[], impurePoints: SimpleImpurePoint[], invalidateExpressions: InvalidateExprNode[], usedVariables: string[]}>
+	 */
+	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,15 +403,15 @@ 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;
 		}
 
 		$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();

src/Analyser/ExprHandlerRegistry.php

@@ -18,6 +18,18 @@ final class ExprHandlerRegistry
 	/** @var array<int, array<non-empty-string, ExprHandler<Expr>|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<Expr>|null
 	 */

src/Analyser/StmtHandlerRegistry.php

@@ -23,6 +23,15 @@ final class StmtHandlerRegistry
 	/** @var array<int, array<class-string<Stmt>, StmtHandler<Stmt>|false>> */
 	private static array $stmtHandlersByClass = [];
 
+	/**
+	 * @see ExprHandlerRegistry::clearCache()
+	 * @internal
+	 */
+	public static function clearCache(): void
+	{
+		self::$stmtHandlersByClass = [];
+	}
+
 	/**
 	 * @return StmtHandler<Stmt>|null
 	 */

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<Container>|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');

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

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[]
 	 */

src/Testing/PHPUnit/PHPStanPHPUnitExtension.php

@@ -21,9 +21,6 @@ public function bootstrap(
 		$facade->registerSubscriber(
 			new InitContainerBeforeDataProviderSubscriber(),
 		);
-		$facade->registerSubscriber(
-			new InitContainerBeforeTestSubscriber(),
-		);
 	}
 
 }

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('<?php fn() => 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());
 	}
 
 }