Allow mixed type dynamic constants #3583

ssigwart · 2024-10-26T03:36:15Z

ssigwart · 2024-10-26T03:43:25Z

tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php

+				'MixedTypeConstants\NoMixedTypeConstantClass::MIXED_TYPE_CONSTANT_IN_CLASS',
+			],
+			[
+				'false',


This doesn't seem right to me, but this test is copied and modified from testDynamicConstants where it also has false. That changed from bool to false in 285b49f#diff-52d2cce81a3daff27b7d6d8ba619b045420ab9a9acffc57c1c379af4eba0b127R8384.

I tried tracking down how I might be able to fix it. I found that the constant gets defined here:

phpstan-src/src/Type/Php/DefineConstantTypeSpecifyingExtension.php

Lines 45 to 60 in d73acef

$constantName = $scope->getType($node->getArgs()[0]->value);

if (

!$constantName instanceof ConstantStringType

|| $constantName->getValue() === ''

) {

return new SpecifiedTypes([], []);

}

return $this->typeSpecifier->create(

new Node\Expr\ConstFetch(

new Node\Name\FullyQualified($constantName->getValue()),

),

$scope->getType($node->getArgs()[1]->value),

TypeSpecifierContext::createTruthy(),

$scope,

)->setAlwaysOverwriteTypes();

The issue is that $scope->getType($node->getArgs()[1]->value) is just going based off the value, so it never reaches where I think it would determine that the constant is dynamic:

phpstan-src/src/Analyser/MutatingScope.php

Line 1943 in d73acef

$constantType = $this->constantResolver->resolveConstant($node->name, $this);

ondrejmirtes · 2024-10-26T16:09:12Z

It'd make sense to me to allow this syntax to force a specific constant type:

parameters:
	dynamicConstantNames:
		- DATABASE_ENGINE
		- Foo::BAR_CONSTANT
		ES_ENGINE: string|null
		Foo::BAZ_CONSTANT: mixed

ssigwart · 2024-10-27T02:27:22Z

It'd make sense to me to allow this syntax to force a specific constant type:
parameters:
	dynamicConstantNames:
		- DATABASE_ENGINE
		- Foo::BAR_CONSTANT
		ES_ENGINE: string|null
		Foo::BAZ_CONSTANT: mixed

That makes sense. I can get it as far as detecting that it's in dynamicConstantNames and getting the string value (e.g. string|null for ES_ENGINE), but haven't figured out how ConstantResolver can take that string and convert it to a PHPStan\Type\Type. Any suggestions?

staabm · 2024-10-27T06:56:14Z

That makes sense. I can get it as far as detecting that it's in dynamicConstantNames and getting the string value (e.g. string|null for ES_ENGINE), but haven't figured out how ConstantResolver can take that string and convert it to a PHPStan\Type\Type. Any suggestions?

I think you can take inspiration from

phpstan-src/src/Reflection/SignatureMap/SignatureMapParser.php

Lines 49 to 56 in 830b6d5

    
           private function getTypeFromString(string $typeString, ?string $className): Type 
        
           { 
        
           	if ($typeString === '') { 
        
           		return new MixedType(true); 
        
           	} 
        
           	return $this->typeStringResolver->resolve($typeString, new NameScope(null, [], $className)); 
        
           }

ssigwart · 2024-10-27T17:17:47Z

Thanks, @staabm! That got me on the right track. I got this working. I even fixed #3583 (comment). I'm not 100% sure if the new interfaces I added are desired, but it gets the job done. Please let me know if you want me to make any changes.

I could use some help with the 1 failing test though:
https://github.com/phpstan/phpstan-src/actions/runs/11542351640/job/32125482914?pr=3583

ondrejmirtes · 2024-11-05T13:28:05Z

src/Analyser/ConstantResolver.php

+		if ($constantType->isConstantValue()->yes()) {
+			if (array_key_exists($constantName, $this->dynamicConstantNames)) {
+				$phpdocTypes = $this->dynamicConstantNames[$constantName];
+				return $this->reflectionProviderProvider->getReflectionProvider()->getSignatureMapProvider()->getTypeFromString($phpdocTypes, null);


You should use TypeStringResolver for this, not SignatureMapProvider.

How would I get an instance of TypeStringResolver from here?

ondrejmirtes · 2024-11-05T13:28:14Z

src/Analyser/ConstantResolver.php

+
+			if ($constantType->isConstantValue()->yes()) {
+				$phpdocTypes = $this->dynamicConstantNames[$lookupConstantName];
+				return $this->getReflectionProvider()->getSignatureMapProvider()->getTypeFromString($phpdocTypes, $className);


ondrejmirtes · 2024-11-05T13:28:28Z

src/Reflection/ReflectionProvider.php

@@ -36,4 +37,6 @@ public function getConstant(Node\Name $nameNode, ?NamespaceAnswerer $namespaceAn

 	public function resolveConstantName(Node\Name $nameNode, ?NamespaceAnswerer $namespaceAnswerer): ?string;

+	public function getSignatureMapProvider(): SignatureMapProvider;


Don't add these methods please.

Addresses phpstan/phpstan#7520 Revert "Allow mixed type dynamic constants." This reverts commit 1854985. Allow setting types for dynamic constants Unit test fixes Unit test fix 2 Lint fixes Use TypeStringResolver directly Remove old code

ssigwart · 2024-11-08T03:22:12Z

Thanks, @ondrejmirtes. I made updates. I'm not sure if passing around the container is the right thing to do, but I got it working.

staabm · 2024-11-08T06:19:01Z

src/Analyser/ConstantResolver.php

@@ -41,6 +43,7 @@ public function __construct(
 		private array $dynamicConstantNames,
 		private ?PhpVersion $composerMinPhpVersion,
 		private ?PhpVersion $composerMaxPhpVersion,
+		private ?Container $container,


make this new param the TypeStringResolver itself.

It will be injected by the DIC automatically

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.

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

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 :)

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

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.

ssigwart commented Oct 26, 2024

View reviewed changes

ondrejmirtes requested changes Nov 5, 2024

View reviewed changes

Allow mixed type dynamic constants.

4c71fb5

Addresses phpstan/phpstan#7520 Revert "Allow mixed type dynamic constants." This reverts commit 1854985. Allow setting types for dynamic constants Unit test fixes Unit test fix 2 Lint fixes Use TypeStringResolver directly Remove old code

ssigwart force-pushed the mixedTypeConst branch from a654da5 to 4c71fb5 Compare November 8, 2024 03:09

PHP 7.4 fix

8710e09

staabm reviewed Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow mixed type dynamic constants #3583

Allow mixed type dynamic constants #3583

ssigwart commented Oct 26, 2024

ssigwart Oct 26, 2024

ondrejmirtes commented Oct 26, 2024

ssigwart commented Oct 27, 2024

staabm commented Oct 27, 2024

ssigwart commented Oct 27, 2024

ondrejmirtes Nov 5, 2024

ssigwart Nov 8, 2024 •

edited

Loading

ondrejmirtes Nov 5, 2024

ondrejmirtes Nov 5, 2024

ssigwart commented Nov 8, 2024

staabm Nov 8, 2024 •

edited

Loading

ssigwart Nov 8, 2024

staabm Nov 8, 2024

staabm Nov 8, 2024

staabm Nov 8, 2024 •

edited

Loading

ssigwart Nov 9, 2024

	$constantName = $scope->getType($node->getArgs()[0]->value);
	if (
	!$constantName instanceof ConstantStringType
	\|\| $constantName->getValue() === ''
	) {
	return new SpecifiedTypes([], []);
	}

	return $this->typeSpecifier->create(
	new Node\Expr\ConstFetch(
	new Node\Name\FullyQualified($constantName->getValue()),
	),
	$scope->getType($node->getArgs()[1]->value),
	TypeSpecifierContext::createTruthy(),
	$scope,
	)->setAlwaysOverwriteTypes();

		@@ -36,4 +37,6 @@ public function getConstant(Node\Name $nameNode, ?NamespaceAnswerer $namespaceAn

		public function resolveConstantName(Node\Name $nameNode, ?NamespaceAnswerer $namespaceAnswerer): ?string;

		public function getSignatureMapProvider(): SignatureMapProvider;

Allow mixed type dynamic constants #3583

Are you sure you want to change the base?

Allow mixed type dynamic constants #3583

Conversation

ssigwart commented Oct 26, 2024

Choose a reason for hiding this comment

ondrejmirtes commented Oct 26, 2024

ssigwart commented Oct 27, 2024

staabm commented Oct 27, 2024

ssigwart commented Oct 27, 2024

Choose a reason for hiding this comment

ssigwart Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssigwart commented Nov 8, 2024

staabm Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssigwart Nov 8, 2024 •

edited

Loading

staabm Nov 8, 2024 •

edited

Loading

staabm Nov 8, 2024 •

edited

Loading