Callable arguments of methods/functions are checked if they are valid callbacks by MartinMystikJonas · Pull Request #716 · phpstan/phpstan

MartinMystikJonas · 2017-12-26T21:43:51Z

No description provided.

… callbacks

ondrejmirtes

This is a first preliminary review. I probably didn't catch everything, but pointed out the obvious, easy, broken stuff.

ondrejmirtes · 2017-12-27T10:21:12Z

+	public function checkCallableArgument(
+		Arg $argument,
+		Scope $scope,
+		string $msgPrefix


Please don't use abbreviations.

ondrejmirtes · 2017-12-27T10:22:29Z

+	public function checkCallableArgument(
+		Arg $argument,
+		Scope $scope,
+		string $msgPrefix


Please don't use abbreviations.

ondrejmirtes · 2017-12-27T10:23:51Z

+				return [$msgPrefix . "array doesn't have required 2 items."];
+			}
+			if ($arrayItemsCount > 2) {
+				return [$msgPrefix . 'array has too much items, only two items expected.'];


has too many

ondrejmirtes · 2017-12-27T10:25:30Z

+				$targetType = new ObjectType($item1->value);
+				$targetValue = $item1->value;
+				$targetIsStaticMethod = true;
+			} elseif ($item1 instanceof Variable) {


Does not have to be variable, e.g. [$this->object, 'method']. This should be the fallback else branch.

ondrejmirtes · 2017-12-27T10:25:30Z

+				$targetType = new ObjectType($item1->value);
+				$targetValue = $item1->value;
+				$targetIsStaticMethod = true;
+			} elseif ($item1 instanceof Variable) {


Does not have to be variable, e.g. [$this->object, 'method']. This should be the fallback else branch.

ondrejmirtes · 2017-12-27T10:32:54Z

+			try {
+				$this->broker->getFunction(new Name($targetMethod), $scope);
+			} catch (\PHPStan\Broker\FunctionNotFoundException $e) {
+				return [$msgPrefix . sprintf('%s is not a existing function name.', $targetMethod)];


Function %s not found. is enough.

ondrejmirtes · 2017-12-27T10:34:09Z

+				continue;
+			}
+
+			$msgPrefix = sprintf('Argument #%d of %s should be callable, but passed ', $i + 1, $node->name);


The name of the argument is also added, for example: 'Parameter #%d %s of method ' . $messagesMethodName . ' expects %s, %s given.', (see usages of FunctionCallParametersCheck).

ondrejmirtes · 2017-12-27T10:34:58Z

+		$typeResult = $this->ruleLevelHelper->findTypeToCheck($scope, $node->var, '');
+		$type = $typeResult->getType();
+		if ($type instanceof ErrorType) {
+			return [];


return $typeResult->getUnknownClassErrors()

ondrejmirtes · 2017-12-27T10:38:32Z

+		$lineOffset2 = 74;
+
+		$this->analyse([__DIR__ . '/../data/callable-exists.php'], [
+					[


The test is really weirdly formatted, please look at it on GitHub.

ondrejmirtes · 2017-12-27T10:38:45Z

+		$this->analyse([__DIR__ . '/../data/callable-exists.php'], [
+					[
+				'Argument #1 of call_user_func should be callable, but passed array has too much items, only two items expected.',
+				$lineOffset, //109


Please don't use this, write the literal line where the error occurs.

I will change it in final version, I often add new test cases whoch changes line numbers so i create offset to fix tests more easily.

ondrejmirtes · 2017-12-27T12:14:59Z

I see, please also add support for ::class (ClassConstFetch).

On Wed, 27 Dec 2017 at 13:09, Martin Mystik Jonáš ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Rules/CallableExistsCheck.php <#716 (comment)>: > + } + $item1 = $argument->value->items[0]->value; + $item2 = $argument->value->items[1]->value; + + if ($item1 instanceof String_) { + $targetType = new ObjectType($item1->value); + $targetValue = $item1->value; + $targetIsStaticMethod = true; + } elseif ($item1 instanceof Variable) { + $targetType = $scope->getType($item1); + $targetIsStaticMethod = false; + } elseif ($item1 instanceof ConstFetch) { + $targetType = new ObjectType(constant($item1->name->toString())); + $targetValue = constant($item1->name->toString()); + $targetIsStaticMethod = true; + } elseif ($item1 instanceof Class_) { This should catch *CLASS* (PhpParser\Node\Scalar\MagicConst\Class_) and it works in tests. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#716 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuBv9kKtZM3aMi_rIbNZQd_jeoRhWks5tEjNpgaJpZM4RNBbd> .

-- Ondřej Mirtes

… callbacks - v2

MartinMystikJonas · 2017-12-27T16:53:40Z

New version commited:

updated error messages
support for ::class
support for properties and static properties in args
support for expressions in args (only check types)
fixed formatting
expanded tests to cover more edge cases

MartinMystikJonas · 2017-12-28T09:33:12Z

I'm working on new version which will support wider variety of parametr combinations (class constant as method name, ...) and I also try to add support for concatenated values like:

  call_user_func(self::class . "::someMethod");

ondrejmirtes · 2017-12-28T09:46:03Z

Please don’t make this over-ambitious, the current feature set is fine. The other cases will probably start working automatically once @JanTvrdik implements constant values into the typesystem.

On Thu, 28 Dec 2017 at 10:33, Martin Mystik Jonáš ***@***.***> wrote: I'm working on new version which will support wider variety of parametr combinations (class constant as method name, ...) and I also try to add support for concatenated values like: call_user_func(self::class . "::someMethod"); — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#716 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuMg6293UNhuwwr73Ngy27gOtrPQkks5tE2BYgaJpZM4RNBbd> .

-- Ondřej Mirtes

… callbacks - v3

MartinMystikJonas · 2017-12-28T10:05:32Z

Commited new version with refactored CallableExistsCheck nad better support for value resolving.

@ondrejmirtes Method resolveValue is candidate for moving to Scope class if it would be made more universal. Currenty in only supports strings but can be extended to other scalars. This method resolves value of Expr to scalar if possible (const, class const, literal, concatenation, ...) or return Type if not (variable, function call, operator, ...).

MartinMystikJonas · 2017-12-28T10:22:41Z

@ondrejmirtes Any idea how to get list of all core PHP functions that accepts callable as argument?

iluuu1994 · 2017-12-28T10:27:19Z

@MartinMystikJonas Stubs will be needed for that.

https://github.com/JetBrains/phpstorm-stubs

ondrejmirtes · 2017-12-28T10:29:37Z

@MartinMystikJonas Yep, just list a few, add support for functions/methods with callable argument (userland ones) and it will automatically work once we add these stubs in some form or another. (Do not attempt to add the stubs here, that would be out of scope of this PR.)

… callbacks - v4

MartinMystikJonas · 2017-12-28T11:20:19Z

@ondrejmirtes Listed all main functions i can find. So for now this feature is complete from my side. Please review it and let me know is anything more is required. Then I will clean up tests code, squash commits and prepare final version.

MartinMystikJonas · 2018-01-07T13:46:00Z

Should I prepare final version or shpuld I wait fir review? This afternoon could be last I could work on this for few months.

mavimo · 2018-01-21T11:15:32Z

+			['array_uintersect_assoc', 4],
+			['array_intersect_uassoc', 4],
+			['array_uintersect_uassoc', 4],
+			['array_diff_ukey', 4],


As I understand we are indicate that parameter 4 into function should be a callable, but php doc indicate that the callable should be the last item of the function, so it call be the 3, the 4 or the N-th element. Same issue on array_udiff, ..

$a = [1, 2, 3]; $b = [4, 5, 6]; $c = [7, 8, 9]; $d = [10, 11, 12]; $e = [13, 14, 15]; $res = array_udiff($a, $b, $c, $d, $e, function (int $first, int $second): int { return $first - $second; });

http://php.net/array_diff_ukey

Good point.

@MartinMystikJonas Maybe should consider to get last element of args and process it when needed?

ondrejmirtes · 2018-02-03T15:09:40Z

+				'Argument #1 callableParam of funcWithCallableParam should be callable, but passed non-static method CallableExists\Bar::knownMethod as static.',
+				$lineOffset2 + 64,
+			],
+						[


Wrong indentation.

ondrejmirtes · 2018-02-03T15:11:58Z

+				$targetIsStaticMethod = true;
+			} elseif ($item1 instanceof ObjectType || $item1 instanceof ThisType) {
+				$targetType = $item1;
+				$targetIsStaticMethod = false;


Did you consider the case where the static method is called on an object? See: https://3v4l.org/HQ1cp

This will result in error:
Argument #1 callableParam of funcWithCallableParam should be callable, but passed static method CallableExists\Foo::knownStaticMethod as non-static.

Should I change this? From my PoW this type of call is propably a mistake.

ondrejmirtes · 2018-02-03T15:14:26Z

+			if (is_string($item1)) {
+				$targetType = new ObjectType($item1);
+				$targetIsStaticMethod = true;
+			} elseif ($item1 instanceof ObjectType || $item1 instanceof ThisType) {


I feel like this should be a different condition. What about not having the else branch and just asking the Type about hasMethod/getMethod?

This will solve union/intersection types, StaticType...

I'm not sure what you mean here. How do you think this conditionshould look like?

Like this:

if (is_string($item1)) { ... } elseif ($item1 instanceof MixedType || $item1 instanceof StringType) { ... } else { // fallback for all other types - make sure to call $item1->hasMethod() + getMethod somewhere lower in this file }

ondrejmirtes · 2018-02-03T18:33:29Z

Please note that I updated master to PHP 7.1, adding nullable and void typehints, so be careful when rebasing 😊 Run vendor/bin/phing cs to find out anything you might have missed.

ondrejmirtes · 2018-02-13T22:25:23Z

This might require some rework and/or simplification in light of #828 😊 isCallable now works more universally, see for example ConstantStringType::isCallable().

Note: #828 will be merged soon.

ondrejmirtes · 2018-02-17T18:47:26Z

Well, it's even more advanced than I suspected, #828 now fully recognizes arrays and strings as callables. It does not have such nice error messages as this pull request but gets the job done 😊 Don't close this yet, I'll go through it to find out whether we forgot something.

With function stubs of native functions that are also in the works, this feature will be completely covered 😊

MartinMystikJonas mentioned this pull request Dec 26, 2017

Callback::closure - check if method exists possible? #714

Closed

Callable arguments of methods/functions are checked if they are valid…

5ea0d4c

… callbacks

MartinMystikJonas force-pushed the callable-check branch from 8aae1ba to 5ea0d4c Compare December 26, 2017 22:09

ondrejmirtes requested changes Dec 27, 2017

View reviewed changes

ondrejmirtes added this to the 0.10 milestone Dec 27, 2017

Callable arguments of methods/functions are checked if they are valid…

9f75cd9

… callbacks - v2

Callable arguments of methods/functions are checked if they are valid…

542beb9

… callbacks - v3

Callable arguments of methods/functions are checked if they are valid…

6693c8d

… callbacks - v4

mavimo reviewed Jan 21, 2018

View reviewed changes

mavimo mentioned this pull request Jan 21, 2018

refs #TECH: force aray_filter to use a callable function that return boolean phpstan/phpstan-strict-rules#7

Closed

ondrejmirtes reviewed Feb 3, 2018

View reviewed changes

ondrejmirtes closed this Feb 3, 2018

ondrejmirtes reopened this Feb 3, 2018

ondrejmirtes force-pushed the master branch from 67639cf to f486280 Compare February 18, 2018 23:12

ondrejmirtes closed this Apr 13, 2018

Conversation

MartinMystikJonas commented Dec 26, 2017

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Dec 27, 2017 via email

Uh oh!

MartinMystikJonas commented Dec 27, 2017

Uh oh!

MartinMystikJonas commented Dec 28, 2017

Uh oh!

ondrejmirtes commented Dec 28, 2017 via email

Uh oh!

MartinMystikJonas commented Dec 28, 2017

Uh oh!

MartinMystikJonas commented Dec 28, 2017

Uh oh!

iluuu1994 commented Dec 28, 2017

Uh oh!

ondrejmirtes commented Dec 28, 2017

Uh oh!

MartinMystikJonas commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinMystikJonas commented Jan 7, 2018

Uh oh!

mavimo Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinMystikJonas Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Feb 3, 2018

Uh oh!

ondrejmirtes commented Feb 13, 2018

Uh oh!

ondrejmirtes commented Feb 17, 2018

Uh oh!

Reviewers

MartinMystikJonas commented Dec 28, 2017 •

edited

Loading

mavimo Jan 21, 2018 •

edited

Loading

MartinMystikJonas Feb 10, 2018 •

edited

Loading