Test the ability to define global types with ExpressionTypeResolverExtension

[cedric-anne]

See phpstan/phpstan#13243.

Being able to act on global statements through the ExpressionTypeResolverExtension may help to improve static analysis on applications that are massively using global variables.
I work in an ecosystem with a huge number of plugins. Our long-term goal is to eliminate the use of these global variables, but in the meantime, we want to improve code analysis by enforcing their typing. This will enable us to detect method calls using incorrect parameters, calls to deprecated methods, etc.

Some tests are broken, and I do not yet understand why. A little help on this part would be appreciated.

[staabm]

are you sure this change is necessary?

I think it causes the test errors

[VincentLanglet]

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());

[staabm]

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

[cedric-anne]

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.

[cedric-anne]

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.

[staabm]

@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

[cedric-anne]

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.

[staabm]

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

[phpstan-bot]

This pull request has been marked as ready for review.