Skip to content
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

Jump to bottom

Detect static calls to non-static methods #727

Closed
wants to merge 12 commits into from
Closed

Detect static calls to non-static methods #727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6c16a9f
Type adjustments for checking static calls to non-static methods.
mglaman Oct 21, 2021
d9b96f3
Check $has is yes before getting method reflection
mglaman Oct 21, 2021
4049df0
Fix test data for LegacyNodeScopeResolverTest
mglaman Oct 28, 2021
5074271
testBug1971 data fixture not marked as static
mglaman Oct 28, 2021
61b2fe3
Revert test data changes, add new test data
mglaman Nov 4, 2021
54285ce
PR feedback around tests
mglaman Nov 11, 2021
bd6f989
Handle allowed static invocations when in the class.
mglaman Nov 11, 2021
c32698e
Revert changes to LegacyNodeScopeResolverTest.php
mglaman Nov 18, 2021
0388901
Fix accidental deletion of testBug5259
mglaman Nov 18, 2021
c5a6e8b
Revert changes to testBug1971
mglaman Nov 18, 2021
6db6c4b
Notes on questions for next steps
mglaman Nov 18, 2021
487c7ca
Add InAnyClassScope scope
mglaman Feb 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 44 additions & 0 deletions src/Analyser/InAnyClassScope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?php declare(strict_types = 1);

namespace PHPStan\Analyser;

use PHPStan\Reflection\ClassMemberAccessAnswerer;
use PHPStan\Reflection\ClassReflection;
use PHPStan\Reflection\ConstantReflection;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Reflection\PropertyReflection;

class InAnyClassScope implements ClassMemberAccessAnswerer
{

/** @api */
public function __construct()
{
}

public function isInClass(): bool
{
return true;
}

public function getClassReflection(): ?ClassReflection
{
return null;
}

public function canAccessProperty(PropertyReflection $propertyReflection): bool
{
return $propertyReflection->isPublic();
}

public function canCallMethod(MethodReflection $methodReflection): bool
{
return $methodReflection->isPublic();
}

public function canAccessConstant(ConstantReflection $constantReflection): bool
{
return $constantReflection->isPublic();
}

}
2 changes: 1 addition & 1 deletion src/Rules/DeadCode/UnusedPrivateMethodRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public function processNode(Node $node, Scope $scope): array
if (!$arrayType instanceof ConstantArrayType) {
continue;
}
$typeAndMethod = $arrayType->findTypeAndMethodName();
$typeAndMethod = $arrayType->findTypeAndMethodName($scope);
if ($typeAndMethod === null) {
continue;
}
Expand Down
21 changes: 18 additions & 3 deletions src/Type/Constant/ConstantArrayType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace PHPStan\Type\Constant;

use PHPStan\Analyser\InAnyClassScope;
use PHPStan\Reflection\ClassMemberAccessAnswerer;
use PHPStan\Reflection\InaccessibleMethod;
use PHPStan\Reflection\ParametersAcceptor;
Expand Down Expand Up @@ -315,7 +316,7 @@ public function equals(Type $type): bool

public function isCallable(): TrinaryLogic
{
$typeAndMethod = $this->findTypeAndMethodName();
$typeAndMethod = $this->findTypeAndMethodName(new InAnyClassScope());
if ($typeAndMethod === null) {
return TrinaryLogic::createNo();
}
Expand All @@ -328,7 +329,7 @@ public function isCallable(): TrinaryLogic
*/
public function getCallableParametersAcceptors(ClassMemberAccessAnswerer $scope): array
{
$typeAndMethodName = $this->findTypeAndMethodName();
$typeAndMethodName = $this->findTypeAndMethodName($scope);
if ($typeAndMethodName === null) {
throw new ShouldNotHappenException();
}
Expand All @@ -347,7 +348,7 @@ public function getCallableParametersAcceptors(ClassMemberAccessAnswerer $scope)
return $method->getVariants();
}

public function findTypeAndMethodName(): ?ConstantArrayTypeAndMethod
public function findTypeAndMethodName(?ClassMemberAccessAnswerer $scope = null): ?ConstantArrayTypeAndMethod
{
if (count($this->keyTypes) !== 2) {
return null;
Expand All @@ -367,13 +368,16 @@ public function findTypeAndMethodName(): ?ConstantArrayTypeAndMethod
return ConstantArrayTypeAndMethod::createUnknown();
}

$isClassString = false;
if ($classOrObject instanceof ConstantStringType) {
$isClassString = true;
$reflectionProvider = ReflectionProviderStaticAccessor::getInstance();
if (!$reflectionProvider->hasClass($classOrObject->getValue())) {
return ConstantArrayTypeAndMethod::createUnknown();
}
$type = new ObjectType($reflectionProvider->getClass($classOrObject->getValue())->getName());
} elseif ($classOrObject instanceof GenericClassStringType) {
$isClassString = true;
$type = $classOrObject->getGenericType();
} elseif ((new ObjectWithoutClassType())->isSuperTypeOf($classOrObject)->yes()) {
$type = $classOrObject;
Expand All @@ -386,6 +390,17 @@ public function findTypeAndMethodName(): ?ConstantArrayTypeAndMethod
if ($this->isOptionalKey(0) || $this->isOptionalKey(1)) {
$has = $has->and(TrinaryLogic::createMaybe());
}
if ($scope === null) {
$scope = new InAnyClassScope();
}
if (
$isClassString
&& $has->yes()
&& !$scope->isInClass()
&& !$type->getMethod($method->getValue(), $scope)->isStatic()
) {
$has = $has->and(TrinaryLogic::createMaybe());
}

mglaman marked this conversation as resolved.
Show resolved Hide resolved
return ConstantArrayTypeAndMethod::createConcrete($type, $method->getValue(), $has);
}
Expand Down
13 changes: 13 additions & 0 deletions src/Type/Constant/ConstantStringType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,19 @@ public function getCallableParametersAcceptors(ClassMemberAccessAnswerer $scope)
return [new InaccessibleMethod($method)];
}

if (!$scope->isInClass() && !$method->isStatic()) {
return [new InaccessibleMethod($method)];
}

if (
!$method->isStatic()
&& $scope->isInClass()
&& $scope->getClassReflection()->getName() !== $classReflection->getName()
&& !$scope->getClassReflection()->isSubclassOf($classReflection->getName())
) {
return [new InaccessibleMethod($method)];
}

return $method->getVariants();
}

Expand Down
22 changes: 22 additions & 0 deletions tests/PHPStan/Rules/Methods/CallStaticMethodsRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,28 @@ public function testBug5259(): void
$this->analyse([__DIR__ . '/data/bug-5259.php'], []);
}

public function testBug5782(): void
{
$this->checkThisOnly = false;
$this->analyse([__DIR__ . '/data/bug-5782.php'], [
[
'Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{\'Bug5782\\\HelloWorld\', \'sayGoodbye\'} given.',
23,
],
[
'Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{\'Bug5782\\\HelloWorld\', \'sayGoodbye\'} given.',
30,
],
// @todo this should also error on mixed type, but doesn't?
// ConstantStringType::getCallableParametersAcceptors returns a
// MixedType but that doesn't cause an error like ConstantArrayType.
[
'Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{\'Bug5782\\\HelloWorld\', \'sayGoodbye\'} given.',
31,
],
Comment on lines +459 to +465
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 believe adding the scope to ConstantStringType::isCallable will also cause this test failure to be fixed. Without the scope to know if we're in a class or not, this always returns as yes for the method being callable when it actually may not be.

			$classRef = $reflectionProvider->getClass($matches[1]);
			if ($classRef->hasMethod($matches[2])) {
				return TrinaryLogic::createYes();
			}

]);
}

public function testBug5536(): void
{
$this->checkThisOnly = false;
Expand Down
32 changes: 32 additions & 0 deletions tests/PHPStan/Rules/Methods/data/bug-5782.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace Bug5782;

class HelloWorld
{
public static function sayHello(): void
{

}

public function sayGoodbye(): void
{

}

}

class FooWorld
{
public function bazBar(): void
{
$closure1 = \Closure::fromCallable([\Bug5782\HelloWorld::class, 'sayGoodbye']);
}
}

function baz() {
$closure1 = \Closure::fromCallable([\Bug5782\HelloWorld::class, 'sayHello']);
$closure2 = \Closure::fromCallable('\Bug5782\HelloWorld::sayHello');
$closure3 = \Closure::fromCallable([\Bug5782\HelloWorld::class, 'sayGoodbye']);
$closure4 = \Closure::fromCallable('Bug5782\HelloWorld::sayGoodbye');
}