Skip to content
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
33 changes: 31 additions & 2 deletions src/Analyser/ConstantResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@

use PhpParser\Node\Name;
use PHPStan\DependencyInjection\AutowiredService;
use PHPStan\DependencyInjection\Container;
use PHPStan\Php\ComposerPhpVersionFactory;
use PHPStan\Php\PhpVersion;
use PHPStan\PhpDoc\TypeStringResolver;
use PHPStan\Reflection\NamespaceAnswerer;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\Reflection\ReflectionProvider\ReflectionProviderProvider;
Expand Down Expand Up @@ -50,6 +52,7 @@ public function __construct(
private array $dynamicConstantNames,
private int|array|null $phpVersion,
private ComposerPhpVersionFactory $composerPhpVersionFactory,
private ?Container $container,
Copy link
Contributor

@staabm staabm Nov 8, 2024

Choose a reason for hiding this comment

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

make this new param the TypeStringResolver itself.

It will be injected by the DIC automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought that might be the way to do it. However, when I tried it, I wound up getting an error about circular dependencies. So that's why I switched to the entire container. Do you have any ideas on how to get around the circular dependency error? It's also possible I just did it wrong. I assumed I needed to update the places that called the constructor, so I was using the container there to get the TypeStringResolver.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I can see it now.

TypeStringResolver has a TypeNodeResolver contruction dependency which in turn has a ConstantResolver dependency.

so when you add a TypeStringResolver dependency on ConstantResolver you build a cycle.

I will think about it and report back

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess one way we could break the cycle is to remove the ConstantResolver construction-time dependency of TypeNodeResolver and turn it into a setter injection (similar how we do it in TypeSpecifierFactory for the TypeSpecifier).

in my local testing I did not yet succeed in doing so :)

Copy link
Contributor

@staabm staabm Nov 8, 2024

Choose a reason for hiding this comment

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

or maybe better instead remove TypeNodeResolver from TypeStringResolver construction-time deps and move it to setter-injection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, @staabm. I don't have experience with DI in PHP, so I'm not entirely sure how to do this. I assume you meant to add a setTypeNodeResolver function to TypeStringResolver. I'm not sure where I would call setTypeNodeResolver though. I thought maybe I needed to add a TypeStringResolverFactory. I'm guessing that maybe I need to update conf/config.neon, so I changed it to this:

-
	class: PHPStan\PhpDoc\TypeStringResolver
typeStringResolverFactory:
	class: PHPStan\PhpDoc\TypeStringResolverFactory

In a new TypeStringResolverFactory class, I added this:

public function create(): TypeStringResolver
{
	$typeStringResolver = new TypeStringResolver(
		$this->typeLexer,
		$this->typeParser
	);
	$typeStringResolver->setTypeNodeResolver($this->container->getByType(TypeNodeResolver::class));

	return $typeStringResolver;
}

I still wound up with this when running the test though:

In Container.php line 327:

  Circular reference detected for: reflectionProvider, betterReflectionProvider, 093,
  058, 042, 040.

Am I on the right track? Do you have any suggestions? Thanks.

)
{
}
Expand Down Expand Up @@ -404,8 +407,18 @@ private function getMaxPhpVersion(): ?PhpVersion

public function resolveConstantType(string $constantName, Type $constantType): Type
{
if ($constantType->isConstantValue()->yes() && in_array($constantName, $this->dynamicConstantNames, true)) {
return $constantType->generalize(GeneralizePrecision::lessSpecific());
if ($constantType->isConstantValue()->yes()) {
if (array_key_exists($constantName, $this->dynamicConstantNames)) {
$phpdocTypes = $this->dynamicConstantNames[$constantName];
if ($this->container !== null) {
$typeStringResolver = $this->container->getByType(TypeStringResolver::class);
return $typeStringResolver->resolve($phpdocTypes, new NameScope(null, [], null));
}
return $constantType;
}
if (in_array($constantName, $this->dynamicConstantNames, true)) {
return $constantType->generalize(GeneralizePrecision::lessSpecific());
}
}

return $constantType;
Expand All @@ -414,6 +427,22 @@ public function resolveConstantType(string $constantName, Type $constantType): T
public function resolveClassConstantType(string $className, string $constantName, Type $constantType, ?Type $nativeType): Type
{
$lookupConstantName = sprintf('%s::%s', $className, $constantName);
if (array_key_exists($lookupConstantName, $this->dynamicConstantNames)) {
if ($constantType->isConstantValue()->yes()) {
$phpdocTypes = $this->dynamicConstantNames[$lookupConstantName];
if ($this->container !== null) {
$typeStringResolver = $this->container->getByType(TypeStringResolver::class);
return $typeStringResolver->resolve($phpdocTypes, new NameScope(null, [], $className));
}
}

if ($nativeType !== null) {
return $nativeType;
}

return $constantType;
}

if (in_array($lookupConstantName, $this->dynamicConstantNames, true)) {
if ($nativeType !== null) {
return $nativeType;
Expand Down
1 change: 1 addition & 0 deletions src/Analyser/ConstantResolverFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function create(): ConstantResolver
$this->container->getParameter('dynamicConstantNames'),
$this->container->getParameter('phpVersion'),
$composerFactory,
$this->container,
);
}

Expand Down
5 changes: 5 additions & 0 deletions src/Analyser/MutatingScope.php
Original file line number Diff line number Diff line change
Expand Up @@ -6191,6 +6191,11 @@ public function getConstantReflection(Type $typeWithConstant, string $constantNa
return $typeWithConstant->getConstant($constantName);
}

public function getConstantExplicitTypeFromConfig(string $constantName, Type $constantType): Type
{
return $this->constantResolver->resolveConstantType($constantName, $constantType);
}

/**
* @return array<string, ExpressionTypeHolder>
*/
Expand Down
2 changes: 2 additions & 0 deletions src/Analyser/Scope.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ public function getMethodReflection(Type $typeWithMethod, string $methodName): ?

public function getConstantReflection(Type $typeWithConstant, string $constantName): ?ClassConstantReflection;

public function getConstantExplicitTypeFromConfig(string $constantName, Type $constantType): Type;

public function getIterableKeyType(Type $iteratee): Type;

public function getIterableValueType(Type $iteratee): Type;
Expand Down
2 changes: 1 addition & 1 deletion src/DependencyInjection/ValidateIgnoredErrorsExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public function loadConfiguration(): void
ReflectionProviderStaticAccessor::registerInstance($reflectionProvider);
PhpVersionStaticAccessor::registerInstance(new PhpVersion(PHP_VERSION_ID));
$composerPhpVersionFactory = new ComposerPhpVersionFactory([]);
$constantResolver = new ConstantResolver($reflectionProviderProvider, [], null, $composerPhpVersionFactory);
$constantResolver = new ConstantResolver($reflectionProviderProvider, [], null, $composerPhpVersionFactory, null);

$phpDocParserConfig = new ParserConfig([]);
$ignoredRegexValidator = new IgnoredRegexValidator(
Expand Down
2 changes: 1 addition & 1 deletion src/Testing/PHPStanTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public static function createScopeFactory(ReflectionProvider $reflectionProvider

$reflectionProviderProvider = new DirectReflectionProviderProvider($reflectionProvider);
$composerPhpVersionFactory = $container->getByType(ComposerPhpVersionFactory::class);
$constantResolver = new ConstantResolver($reflectionProviderProvider, $dynamicConstantNames, null, $composerPhpVersionFactory);
$constantResolver = new ConstantResolver($reflectionProviderProvider, $dynamicConstantNames, null, $composerPhpVersionFactory, $container);

$initializerExprTypeResolver = new InitializerExprTypeResolver(
$constantResolver,
Expand Down
8 changes: 7 additions & 1 deletion src/Type/Php/DefineConstantTypeSpecifyingExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,17 @@ public function specifyTypes(
return new SpecifiedTypes([], []);
}

$valueType = $scope->getType($node->getArgs()[1]->value);
$finalType = $scope->getConstantExplicitTypeFromConfig(
$constantName->getValue(),
$valueType,
);

return $this->typeSpecifier->create(
new Node\Expr\ConstFetch(
new Node\Name\FullyQualified($constantName->getValue()),
),
$scope->getType($node->getArgs()[1]->value),
$finalType,
TypeSpecifierContext::createTruthy(),
$scope,
)->setAlwaysOverwriteTypes();
Expand Down
16 changes: 13 additions & 3 deletions tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8244,6 +8244,10 @@ public static function dataDynamicConstants(): array
'string',
'DynamicConstants\DynamicConstantClass::DYNAMIC_CONSTANT_IN_CLASS',
],
[
'string|null',
'DynamicConstants\DynamicConstantClass::DYNAMIC_CONSTANT_WITH_EXPLICIT_TYPES_IN_CLASS',
],
[
"'abc123def'",
'DynamicConstants\DynamicConstantClass::PURE_CONSTANT_IN_CLASS',
Expand All @@ -8253,13 +8257,17 @@ public static function dataDynamicConstants(): array
'DynamicConstants\NoDynamicConstantClass::DYNAMIC_CONSTANT_IN_CLASS',
],
[
'false',
'bool',
'GLOBAL_DYNAMIC_CONSTANT',
],
[
'123',
'GLOBAL_PURE_CONSTANT',
],
[
'string|null',
'GLOBAL_DYNAMIC_CONSTANT_WITH_EXPLICIT_TYPES',
],
];
}

Expand All @@ -8275,8 +8283,10 @@ public function testDynamicConstants(
$expression,
'die',
[
'DynamicConstants\\DynamicConstantClass::DYNAMIC_CONSTANT_IN_CLASS',
'GLOBAL_DYNAMIC_CONSTANT',
0 => 'DynamicConstants\\DynamicConstantClass::DYNAMIC_CONSTANT_IN_CLASS',
1 => 'GLOBAL_DYNAMIC_CONSTANT',
'DynamicConstants\\DynamicConstantClass::DYNAMIC_CONSTANT_WITH_EXPLICIT_TYPES_IN_CLASS' => 'string|null',
'GLOBAL_DYNAMIC_CONSTANT_WITH_EXPLICIT_TYPES' => 'string|null',
],
);
}
Expand Down
2 changes: 2 additions & 0 deletions tests/PHPStan/Analyser/data/dynamic-constant.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@

define('GLOBAL_PURE_CONSTANT', 123);
define('GLOBAL_DYNAMIC_CONSTANT', false);
define('GLOBAL_DYNAMIC_CONSTANT_WITH_EXPLICIT_TYPES', null);

class DynamicConstantClass
{
const DYNAMIC_CONSTANT_IN_CLASS = 'abcdef';
const DYNAMIC_CONSTANT_WITH_EXPLICIT_TYPES_IN_CLASS = 'xyz';
const PURE_CONSTANT_IN_CLASS = 'abc123def';
}

Expand Down
Loading