Detect static calls to non-static methods #727

mglaman · 2021-10-21T21:00:41Z

This initial commit is definitely not the right way, but I'm going in circles and want to at least get something up for review and direction.

mglaman

I tried writing a test, but to be honest I had no idea what rule or test this would affect. So I finally just made some changes and tossed it up.

src/Type/Constant/ConstantArrayType.php

src/Type/Constant/ConstantStringType.php

src/Type/Constant/ConstantArrayType.php

mglaman · 2021-10-21T21:28:16Z

I made the quick changes for $has->yes(), I'll inspect test failures when I have my PHPStan dev time. Looks like 1 test added more failures, the other is an error. I didn't have time to inspect the test cases.

tests/PHPStan/Analyser/data/callables.php

src/Type/Constant/ConstantArrayType.php

tests/PHPStan/Analyser/data/callables.php

tests/PHPStan/Rules/Methods/CallStaticMethodsRuleTest.php

mglaman · 2021-11-04T19:41:04Z

tests/PHPStan/Rules/Methods/CallStaticMethodsRuleTest.php

+			[
+				'Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{\'Bug1971\\\HelloWorld\', \'sayHello\'} given.',
+				14,
+			],
+			[
+				'Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{class-string<static(Bug1971\\HelloWorld)>, \'sayHello\'} given.',
+				15,
+			],


Did not update the test data, but cover the fact these instance methods called statically do not exist.

The problem here is that this actually works: https://3v4l.org/mTa9S

It only doesn't work from the outside: https://3v4l.org/dUmHk

The same problem is covered in CallStaticMethodRule here:

phpstan-src/src/Rules/Methods/CallStaticMethodsRule.php

Lines 215 to 236 in 625cb84

$method = $classType->getMethod($methodName, $scope);

if (!$method->isStatic()) {

$function = $scope->getFunction();

if (

!$function instanceof MethodReflection

|| $function->isStatic()

|| !$scope->isInClass()

|| (

$classType instanceof TypeWithClassName

&& $scope->getClassReflection()->getName() !== $classType->getClassName()

&& !$scope->getClassReflection()->isSubclassOf($classType->getClassName())

)

) {

return array_merge($errors, [

RuleErrorBuilder::message(sprintf(

'Static call to instance method %s::%s().',

$method->getDeclaringClass()->getDisplayName(),

$method->getName()

))->build(),

]);

}

}

So we should replicate the same logic here. We can take advantage of Scope being passed to getCallableParametersAcceptors and also pass it along to findTypeAndMethodName but as an optional parameter because of backward compatibility (but go through all the current call sites and pass Scope in it).

One problem I'm hitting: isCallable does not have a scope, and that's where findTypeAndMethodName seems to be invoked from. So I added OutOfClassScope, to check if we're in a class. If not, return null, but that always returns false.

EDIT: The fix appears to update $has to maybe if we are not in a class context.

Having ConstantArrayType return maybe on a static call outside of the class or in class with mismatching class names seems to have resolved this. Thanks for the tip!

tests/PHPStan/Rules/Methods/data/bug-727.php

tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php

ondrejmirtes · 2021-11-05T21:26:19Z

tests/PHPStan/Rules/Methods/CallStaticMethodsRuleTest.php

+			[
+				'Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{\'Bug1971\\\HelloWorld\', \'sayHello\'} given.',
+				14,
+			],
+			[
+				'Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{class-string<static(Bug1971\\HelloWorld)>, \'sayHello\'} given.',
+				15,
+			],


The problem here is that this actually works: https://3v4l.org/mTa9S

It only doesn't work from the outside: https://3v4l.org/dUmHk

The same problem is covered in CallStaticMethodRule here:

phpstan-src/src/Rules/Methods/CallStaticMethodsRule.php

Lines 215 to 236 in 625cb84

$method = $classType->getMethod($methodName, $scope);

if (!$method->isStatic()) {

$function = $scope->getFunction();

if (

!$function instanceof MethodReflection

|| $function->isStatic()

|| !$scope->isInClass()

|| (

$classType instanceof TypeWithClassName

&& $scope->getClassReflection()->getName() !== $classType->getClassName()

&& !$scope->getClassReflection()->isSubclassOf($classType->getClassName())

)

) {

return array_merge($errors, [

RuleErrorBuilder::message(sprintf(

'Static call to instance method %s::%s().',

$method->getDeclaringClass()->getDisplayName(),

$method->getName()

))->build(),

]);

}

}

So we should replicate the same logic here. We can take advantage of Scope being passed to getCallableParametersAcceptors and also pass it along to findTypeAndMethodName but as an optional parameter because of backward compatibility (but go through all the current call sites and pass Scope in it).

mglaman · 2021-11-11T22:28:25Z

src/Type/Constant/ConstantArrayType.php

-		$typeAndMethod = $this->findTypeAndMethodName();
+		$typeAndMethod = $this->findTypeAndMethodName(new OutOfClassScope());


Passing out of class scope here is causing grief for CallStaticMethodsRuleTest::testBug1971

src/Type/Constant/ConstantArrayType.php

mglaman

@ondrejmirtes I'd like your feedback on a proposal. I believe we need to add the scope to \PHPStan\Type\Type::isCallable like \PHPStan\Type\Type::getCallableParametersAcceptors.

This way we can determine the class context if a static method is truly callable or not.

mglaman · 2021-11-18T20:12:00Z

src/Type/Constant/ConstantArrayType.php

 	public function isCallable(): TrinaryLogic
 	{
-		$typeAndMethod = $this->findTypeAndMethodName();
+		// @todo is there a way to fetch the full scope here.
+		// An OutOfClassScope causes frivolous errors when checking if a
+		// non-static method was invoked statically in its own class, which
+		// is allowed per: https://3v4l.org/mTa9S.
+		// Should add an optional scope parameter to isCallable and if null,
+		// create a new OutOfClassScope instance.
+		$typeAndMethod = $this->findTypeAndMethodName(new OutOfClassScope());


After circling around this issue for the past few weeks, I think we need to pass the scope to isCallable. Knowing if something is callable depends on the scope so we can check the current class. A non-static method on a class can be invoked statically if it is within the same class. This is will always be false given OutOfClassScope.

This would also improve detection in ConstantStringType. I didn't want to make the big change without first discussing.

The OutOfClassScope scope here is what is causing the test failures for CallStaticMethodsRuleTest::testBug1971.

1) PHPStan\Rules\Methods\CallStaticMethodsRuleTest::testBug1971 Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'16: Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{class-string<static(Bug1971\HelloWorld)>, 'sayHello2'} given. +'14: Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{'Bug1971\\HelloWorld', 'sayHello'} given. +15: Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{class-string<static(Bug1971\HelloWorld)>, 'sayHello'} given. +16: Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{class-string<static(Bug1971\HelloWorld)>, 'sayHello2'} given. '

What about a new class called InAnyClassScope that returns true from all the can* methods?

We'd still get the actual validation because getCallableParametersAcceptors accepts the real scope.

mglaman · 2021-11-18T20:12:45Z

src/Type/Constant/ConstantArrayType.php

+			if ($scope === null) {
+				$scope = new OutOfClassScope();
+			}


I wasn't sure if this should be pushed to the top of the method – to provide a value for a default nullable meant for backward compatibility. Or instantiated when it's finally needed.

mglaman · 2021-11-18T20:14:52Z

tests/PHPStan/Rules/Methods/CallStaticMethodsRuleTest.php

+			// @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,
+			],


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

ondrejmirtes · 2021-11-19T09:08:29Z

src/Type/Constant/ConstantArrayType.php

 	public function isCallable(): TrinaryLogic
 	{
-		$typeAndMethod = $this->findTypeAndMethodName();
+		// @todo is there a way to fetch the full scope here.
+		// An OutOfClassScope causes frivolous errors when checking if a
+		// non-static method was invoked statically in its own class, which
+		// is allowed per: https://3v4l.org/mTa9S.
+		// Should add an optional scope parameter to isCallable and if null,
+		// create a new OutOfClassScope instance.
+		$typeAndMethod = $this->findTypeAndMethodName(new OutOfClassScope());


What about a new class called InAnyClassScope that returns true from all the can* methods?

We'd still get the actual validation because getCallableParametersAcceptors accepts the real scope.

mglaman · 2022-01-05T22:47:03Z

I forgot about this and will work on follow ups – for that InAnyClass object.

ondrejmirtes · 2022-02-08T12:31:23Z

Do you plan to finish this or can I close the PR? :)

mglaman · 2022-02-08T12:38:56Z

I will finish, I forgot (😬). Started a new job and things fell apart in that shuffle. Added back to my Todoist

Adds a new test case with static methods for PR727 matching test data for Issue 1971.

mglaman · 2022-02-11T15:49:11Z

Rebased and re-uploading this to my head. I made the InAnyClassScope locally and running the tests.

mglaman · 2022-02-11T18:46:02Z

Blah. InAnyClassScope fixed the failures I caused, but doesn't report the failures in the test I created:

1) PHPStan\Rules\Methods\CallStaticMethodsRuleTest::testBug5782
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'23: Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{'Bug5782\\HelloWorld', 'sayGoodbye'} given.
-30: Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{'Bug5782\\HelloWorld', 'sayGoodbye'} given.
-31: Parameter #1 $callback of static method Closure::fromCallable() expects callable(): mixed, array{'Bug5782\\HelloWorld', 'sayGoodbye'} given.
+'
 '

mglaman · 2022-02-11T19:39:12Z

I am just going to close this. It's over my head, and I keep bumping into other areas. Now I've entered this realm:

RuleLevelHelper::accepts:

		$accepts = $acceptingType->accepts($acceptedType, $strictTypes);
		if (!$accepts->yes() && $acceptingType instanceof UnionType && !$acceptedType instanceof CompoundType) {

The $accepts trinary logic is always YES because of CallableType:

	private function isSuperTypeOfInternal(Type $type, bool $treatMixedAsAny): TrinaryLogic
	{
		$isCallable = $type->isCallable();
		if ($isCallable->no() || $this->isCommonCallable) {
			return $isCallable;
		}

The isCommonCallable is always true since there are no parameters.

So even if getCallableParametersAcceptors returns TrivialParametersAcceptor it isn't reported correctly.

mglaman commented Oct 21, 2021

View reviewed changes

src/Type/Constant/ConstantArrayType.php Outdated Show resolved Hide resolved

src/Type/Constant/ConstantStringType.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Oct 21, 2021

View reviewed changes

src/Type/Constant/ConstantArrayType.php Outdated Show resolved Hide resolved

mglaman force-pushed the non-static-methods-call-directly branch from 91ef10d to 67c6ac7 Compare October 28, 2021 19:05

mglaman commented Oct 28, 2021

View reviewed changes

tests/PHPStan/Analyser/data/callables.php Outdated Show resolved Hide resolved

mglaman requested a review from ondrejmirtes October 28, 2021 19:25

ondrejmirtes requested changes Nov 3, 2021

View reviewed changes

src/Type/Constant/ConstantArrayType.php Outdated Show resolved Hide resolved

tests/PHPStan/Analyser/data/callables.php Outdated Show resolved Hide resolved

mglaman commented Nov 4, 2021

View reviewed changes

mglaman requested a review from ondrejmirtes November 4, 2021 19:42

ondrejmirtes requested changes Nov 5, 2021

View reviewed changes

mglaman force-pushed the non-static-methods-call-directly branch from a9cfcd5 to 6b932d9 Compare November 11, 2021 22:14

mglaman commented Nov 11, 2021

View reviewed changes

mglaman commented Nov 18, 2021

View reviewed changes

mglaman requested a review from ondrejmirtes November 18, 2021 20:17

ondrejmirtes requested changes Nov 19, 2021

View reviewed changes

ondrejmirtes force-pushed the master branch from 97c2f21 to a03a78f Compare December 19, 2021 16:34

mglaman added 11 commits February 11, 2022 09:37

Type adjustments for checking static calls to non-static methods.

6c16a9f

Check $has is yes before getting method reflection

d9b96f3

Fix test data for LegacyNodeScopeResolverTest

4049df0

testBug1971 data fixture not marked as static

5074271

Revert test data changes, add new test data

61b2fe3

Adds a new test case with static methods for PR727 matching test data for Issue 1971.

PR feedback around tests

54285ce

Handle allowed static invocations when in the class.

bd6f989

Revert changes to LegacyNodeScopeResolverTest.php

c32698e

Fix accidental deletion of testBug5259

0388901

Revert changes to testBug1971

c5a6e8b

Notes on questions for next steps

6db6c4b

mglaman force-pushed the non-static-methods-call-directly branch from c815c71 to 6db6c4b Compare February 11, 2022 15:37

Add InAnyClassScope scope

487c7ca

mglaman force-pushed the non-static-methods-call-directly branch from 38a7c84 to 487c7ca Compare February 11, 2022 17:42

mglaman closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect static calls to non-static methods #727

Detect static calls to non-static methods #727

mglaman commented Oct 21, 2021 •

edited

mglaman left a comment

mglaman commented Oct 21, 2021

mglaman Nov 4, 2021

ondrejmirtes Nov 5, 2021

mglaman Nov 11, 2021 •

edited

mglaman Nov 11, 2021

ondrejmirtes Nov 5, 2021

mglaman Nov 11, 2021

mglaman left a comment

mglaman Nov 18, 2021

mglaman Nov 18, 2021

ondrejmirtes Nov 19, 2021

mglaman Nov 18, 2021

mglaman Nov 18, 2021

ondrejmirtes Nov 19, 2021

mglaman commented Jan 5, 2022

ondrejmirtes commented Feb 8, 2022

mglaman commented Feb 8, 2022

mglaman commented Feb 11, 2022

mglaman commented Feb 11, 2022

mglaman commented Feb 11, 2022

	$method = $classType->getMethod($methodName, $scope);
	if (!$method->isStatic()) {
	$function = $scope->getFunction();
	if (
	!$function instanceof MethodReflection
	\|\| $function->isStatic()
	\|\| !$scope->isInClass()
	\|\| (
	$classType instanceof TypeWithClassName
	&& $scope->getClassReflection()->getName() !== $classType->getClassName()
	&& !$scope->getClassReflection()->isSubclassOf($classType->getClassName())
	)
	) {
	return array_merge($errors, [
	RuleErrorBuilder::message(sprintf(
	'Static call to instance method %s::%s().',
	$method->getDeclaringClass()->getDisplayName(),
	$method->getName()
	))->build(),
	]);
	}
	}

		$typeAndMethod = $this->findTypeAndMethodName();
		$typeAndMethod = $this->findTypeAndMethodName(new OutOfClassScope());

Detect static calls to non-static methods #727

Detect static calls to non-static methods #727

Conversation

mglaman commented Oct 21, 2021 • edited

mglaman left a comment

Choose a reason for hiding this comment

mglaman commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mglaman Nov 11, 2021 • edited

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

mglaman left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mglaman commented Jan 5, 2022

ondrejmirtes commented Feb 8, 2022

mglaman commented Feb 8, 2022

mglaman commented Feb 11, 2022

mglaman commented Feb 11, 2022

mglaman commented Feb 11, 2022

mglaman commented Oct 21, 2021 •

edited

mglaman Nov 11, 2021 •

edited