Skip to content

Test the ability to define global types with ExpressionTypeResolverExtension #4233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: 2.1.x
Choose a base branch
from
+43 −1
Open

Test the ability to define global types with ExpressionTypeResolverExtension #4233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4d7504
Permit to define global types with `ExpressionTypeResolverExtension`
cedric-anne Aug 10, 2025
69f9c30
revert useless change
cedric-anne Aug 10, 2025
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
2 changes: 1 addition & 1 deletion src/Analyser/NodeScopeResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1965,7 +1965,7 @@ static function (Node $node, Scope $scope) use ($nodeCallback): void {
continue;
}

$scope = $scope->assignVariable($var->name, new MixedType(), new MixedType(), TrinaryLogic::createYes());
Copy link
Contributor

@staabm staabm Aug 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

are you sure this change is necessary?

I think it causes the test errors

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume without this it's always considered as MixedType and do not use the ExpressionTypeResolverExtension

Looking at the failing test, it need to add an extra check to transform ErrorType into MixedType.

But I think the scope::getType is "just" here to apply

foreach ($this->expressionTypeResolverExtensionRegistry->getExtensions() as $extension) {
			$type = $extension->getType($var, $scope);
			if ($type !== null) {
				return $type;
			}
		}

so it seems better to write

$type = null;
foreach ($this->expressionTypeResolverExtensionRegistry->getExtensions() as $extension) {
     $type = $extension->getType($var, $scope);
     if ($type !== null) {
        break;
      }
}

$scope = $scope->assignVariable($var->name, $type ?? new MixedType(), new MixedType(), TrinaryLogic::createYes());

Copy link
Contributor

Choose a reason for hiding this comment

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

the PRs tests do not fail when we delete this change here, therefore it seems either the tests are wonky or phpstan already works like expected before this PR

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. It looks like I missed something. At first I did non found in the documentation how to specify global types, so I did some reverse engeneering to see if it was possible. I understood the modified line as a force MixedType that cannot be altered by any extension, but it seems that I was wrong. I will do some tests on my own extension, to see if I can achieve what I want to do.

Also, I did not found any documentation related to the usage of ExpressionTypeResolverExtension. Maybe it should be documented, as it can be really usefull for extension developers.

Copy link
Author

Choose a reason for hiding this comment

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

The ExpressionTypeResolverExtension works as expected, as it permits to force any type during a variable declaration, but it does not completely fits my needs as I am not able to differenciate a global var statement from a function parameter.

Indeed both global $MY_VAR; and function foo ($MY_VAR) {} are similar from the extension point of view. However, it is probably out of scope of the current PR and I will use some hacks to achieve my goals.

I removed the modified line. Feel free to close this PR if the new test is considered useless.

Copy link
Contributor

@staabm staabm Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@cedric-anne please add the example (and failling assertions) which does not work as expected into your test-case of this PR, so we can look into what needs to be done to make it pass

Copy link
Author

Choose a reason for hiding this comment

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

The failing test case would be the following:

function bar() {
    global $MY_FRAMEWORK_GLOBAL;
    // This one works as expected.
    assertType('mixed', $MY_FRAMEWORK_GLOBAL);
}

function foo($MY_FRAMEWORK_GLOBAL) {
    // I have no idea how I could distinguish the `$MY_FRAMEWORK_GLOBAL` parameter definition
    // from a `global $MY_FRAMEWORK_GLOBAL` statement in my `GlobalExpressionTypeResolverExtension::getType()` method.
    //
    // The following assert with fail, as the extension currently defines it as a `bool`.
    assertType('mixed', $MY_FRAMEWORK_GLOBAL);
}

As far as I understand, it is not possible to access the parent PhpParser\Node\Stmt\Global_ statement of the PhpParser\Node\Expr\Variable expression. Without this, there is no way to distinguish a global statement like the one in the bar() function from a parameter definition like the one in the foo() function.

IMHO, we can consider that the extension works as expected, from the PHPStan point of view. There is no bug here, just a limitation.

Copy link
Contributor

@staabm staabm Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it might be possible to turn $scope->isGlobalVariable() public, after we added support for recognizing the global keyword.

one idea which comes to mind is, to mark variables which are preceeded by Global_ AST node with a attribute and in MutatingScope->isGlobalVariable look whether the given attribute is set.
see e.g. src/Parser/ArrayFilterArgVisitor.php for inspiration on how this can be achieved.

beware: ondrej might have a different idea of the problem and might not be happy with the above suggestion :)

$scope = $scope->assignVariable($var->name, $scope->getType($var), new MixedType(), TrinaryLogic::createYes());
$vars[] = $var->name;
}
$scope = $this->processVarAnnotation($scope, $vars, $stmt);
Expand Down
3 changes: 2 additions & 1 deletion tests/PHPStan/Analyser/ExpressionTypeResolverExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ class ExpressionTypeResolverExtensionTest extends TypeInferenceTestCase

public static function dataFileAsserts(): iterable
{
yield from self::gatherAssertTypes(__DIR__ . '/data/expression-type-resolver-extension.php');
yield from self::gatherAssertTypes(__DIR__ . '/data/expression-type-resolver-extension-method-call-returns-bool.php');
yield from self::gatherAssertTypes(__DIR__ . '/data/expression-type-resolver-extension-global-statement.php');
}

/**
Expand Down
27 changes: 27 additions & 0 deletions tests/PHPStan/Analyser/data/GlobalExpressionTypeResolverExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

namespace ExpressionTypeResolverExtension;

use PhpParser\Node\Expr;
use PhpParser\Node\Expr\Variable;
use PHPStan\Analyser\Scope;
use PHPStan\Type\BooleanType;
use PHPStan\Type\ExpressionTypeResolverExtension;
use PHPStan\Type\Type;

class GlobalExpressionTypeResolverExtension implements ExpressionTypeResolverExtension {

public function getType(Expr $expr, Scope $scope): ?Type
{
if (!$expr instanceof Variable) {
return null;
}

if ($expr->name === 'MY_FRAMEWORK_GLOBAL') {
return new BooleanType();
}

return null;
}

}
10 changes: 10 additions & 0 deletions tests/PHPStan/Analyser/data/expression-type-resolver-extension-global-statement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

// test file for ExpressionTypeResolverExtensionTest

use function PHPStan\Testing\assertType;

global $MY_FRAMEWORK_GLOBAL, $ANOTHER_GLOBAL;

assertType('bool', $MY_FRAMEWORK_GLOBAL);
assertType('mixed', $ANOTHER_GLOBAL);
0 ...ta/expression-type-resolver-extension.php → ...er-extension-method-call-returns-bool.php
View file
Open in desktop
File renamed without changes.
4 changes: 4 additions & 0 deletions tests/PHPStan/Analyser/expression-type-resolver-extension.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# config for ExpressionTypeResolverExtensionTest
services:
-
class: ExpressionTypeResolverExtension\GlobalExpressionTypeResolverExtension
tags:
- phpstan.broker.expressionTypeResolverExtension
-
class: ExpressionTypeResolverExtension\MethodCallReturnsBoolExpressionTypeResolverExtension
tags:
Expand Down
Loading