use conditional parameter for is_a() via stub #1306

staabm · 2022-05-12T13:34:27Z

staabm · 2022-05-12T13:34:38Z

stubs/core.stub

@@ -1,5 +1,12 @@
 <?php

+/**


stub taken 1:1 from psalm

update: psalm was missing the type for $allow_string, which will be added with vimeo/psalm#7951

ondrejmirtes · 2022-05-12T13:39:54Z

Please read the title of the linked issue: is_a: "if third argument is false then do not allow string in first argument"

Which means that there should be a new test in CallToFunctionParametersRuleTest (or ImpossibleCheckTypeFunctionCallRuleTest, that depends) that reports a new error here https://phpstan.org/r/44b40ab5-7c7f-4ced-8048-e084d3d63f1d on line 10.

staabm · 2022-05-12T13:41:59Z

yeah, it seems the initially reported bug in 4371 is different from the one I had in phpstan/phpstan#4371 (comment)

but I am fine, with also covering the original issue here then

staabm · 2022-05-13T07:30:17Z

tests/PHPStan/Rules/Comparison/data/check-type-function-call.php

@@ -32,7 +32,7 @@ public function doFoo(

 		}
 		$className = 'Foo';
-		if (is_a($className, \Throwable::class, true)) { // should be fine
+		if (is_a($className, \Throwable::class, true)) { // always false


is no longer considered "fine" because Foo is not a subclass of Throwable

But also please test Foo::class (which is different thanks to namespace resolution), the class isn't final so a subclass might implement Throwable.

hm I guess you mean I should test class-string<Foo> as a parameter not Foo::class.. at least thats what I did for now.. see added tests at the end of this file

tests/PHPStan/Rules/Comparison/data/bug-4371.php

staabm

this gets a few rough edges, therefore I pushed the current development state and hope for feedback.

staabm · 2022-05-13T10:45:27Z

tests/PHPStan/Rules/Comparison/data/check-type-function-call.php

@@ -32,7 +32,7 @@ public function doFoo(

 		}
 		$className = 'Foo';
-		if (is_a($className, \Throwable::class, true)) { // should be fine
+		if (is_a($className, \Throwable::class, true)) { // always false


hm I guess you mean I should test class-string<Foo> as a parameter not Foo::class.. at least thats what I did for now.. see added tests at the end of this file

staabm · 2022-05-13T10:47:23Z

tests/PHPStan/Analyser/data/bug-4371.php

+		$exceptionClass = get_class($exception);
+		assertType('false', is_a($exceptionClass, $classString));
+		assertType('bool', is_a($exceptionClass, $classString, true));
+		assertType('false', is_a($exceptionClass, $classString, false));


with 3e955af I dropped the conditional return type stub again, as I am no longer sure its worth to report about string vs. class-string type errors for $class.. might be tedious.

since we already have extensions for is_a in place the stub itself was nevertheless only partly usefull, I think.

staabm · 2022-05-13T10:48:20Z

src/Rules/Comparison/ImpossibleCheckTypeFunctionCallRule.php

@@ -35,9 +34,6 @@ public function processNode(Node $node, Scope $scope): array
 		}

 		$functionName = (string) $node->name;
-		if (strtolower($functionName) === 'is_a') {


because I dropped this early-return, I had to compensate a few false positives in the IsAFunctionTypeSpecifyingExtension - added a few early returns.

staabm · 2022-05-13T10:50:25Z

src/Type/Php/IsAFunctionTypeSpecifyingExtension.php

@@ -42,6 +46,16 @@ public function specifyTypes(FunctionReflection $functionReflection, FuncCall $n
 		$allowStringType = isset($node->getArgs()[2]) ? $scope->getType($node->getArgs()[2]->value) : new ConstantBooleanType(false);
 		$allowString = !$allowStringType->equals(new ConstantBooleanType(false));

+		// early exit in some cases, so we don't create false positives via IsAFunctionTypeSpecifyingHelper
+		if ($classType instanceof ClassStringType && !$classType instanceof GenericClassStringType) {


especially this condition is pretty ugly .. would love feedback on how to improve it.

at first I only had the $classType instanceof ClassStringType part, but later on needed to add !$classType instanceof GenericClassStringType to prevent regressions of existing tests

…ving a early return in ImpossibleCheckTypeFunctionCallRule

ondrejmirtes · 2022-05-13T12:52:42Z

This is already quite messy again :/ I think the stub solved a problem so I really don't want to have it reverted here 3e955af.

But this is truly a minor problem. I'd rather be if you worked on the NamedArgumentsHelper improvements :) Thanks.

staabm · 2022-05-13T13:08:07Z

I started workin on this topic, because we had a production issue, which would be great if beeing covered by phpstan the next time.

it looked easy at first, but it seems I have no luck in picking topics to work on, since most of the time it turns into walking thru rabbit holes, like we can see here :-/.

will do NamedArguments stuff, when free time allows again.

ondrejmirtes · 2022-05-13T13:09:12Z

You can just send the conditional stub in a separate PR alongside with the tests as you initially did, without marking phpstan/phpstan#4371 as closing :)

ondrejmirtes · 2022-05-13T13:09:35Z

Basically this commit 6bbb2a6 :)

ondrejmirtes · 2022-05-13T13:10:49Z

I have no luck in picking topics to work on

That's normal, I just spent several hours today trying to clean up something about named arguments and I'm not happy with the result either. The combination of php-8-stubs, phpstorm-stubs and php-8-stubs is a mess in some situations. And PHP itself is a mess too :)

staabm commented May 12, 2022

View reviewed changes

clxmstaab force-pushed the is-a-cond branch from 3db6b90 to 7aeb7eb Compare May 13, 2022 07:29

staabm commented May 13, 2022

View reviewed changes

staabm marked this pull request as draft May 13, 2022 07:34

staabm commented May 13, 2022

View reviewed changes

tests/PHPStan/Rules/Comparison/data/bug-4371.php Outdated Show resolved Hide resolved

staabm force-pushed the is-a-cond branch from 3e955af to 8ea8956 Compare May 13, 2022 10:49

staabm commented May 13, 2022

View reviewed changes

staabm force-pushed the is-a-cond branch from 44cd89d to d26096c Compare May 13, 2022 12:01

clxmstaab added 12 commits May 13, 2022 14:17

use conditional parameter for is_a() via stub

6bbb2a6

fix missing param in stub

b17aa8d

added failling test

49610a6

fix

49cb07b

impl fix

6078953

fix tests

93b4674

added tests

b3dd3f3

missing use

e569f1e

separate scope for test

3460b04

ugly hack arround newly introduced false positives, triggered by remo…

d1155f3

…ving a early return in ImpossibleCheckTypeFunctionCallRule

fix regression in TypeSpecifierTest

34029c5

cs

7c30478

staabm force-pushed the is-a-cond branch from 2a31b87 to 7c30478 Compare May 13, 2022 12:17

staabm closed this May 13, 2022

staabm mentioned this pull request May 13, 2022

use conditional return type for is_a() via stub #1310

Merged

staabm deleted the is-a-cond branch May 13, 2022 13:38

use conditional parameter for is_a() via stub #1306

use conditional parameter for is_a() via stub #1306

Uh oh!

Conversation

staabm commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm May 12, 2022

Choose a reason for hiding this comment

Uh oh!

staabm May 12, 2022 • 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!

ondrejmirtes commented May 12, 2022

Uh oh!

staabm commented May 12, 2022

Uh oh!

staabm May 13, 2022

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes May 13, 2022

Choose a reason for hiding this comment

Uh oh!

staabm May 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

staabm left a comment

Choose a reason for hiding this comment

Uh oh!

staabm May 13, 2022

Choose a reason for hiding this comment

Uh oh!

staabm May 13, 2022 • 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!

staabm May 13, 2022

Choose a reason for hiding this comment

Uh oh!

staabm May 13, 2022 • 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!

ondrejmirtes commented May 13, 2022

Uh oh!

staabm commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented May 13, 2022

Uh oh!

ondrejmirtes commented May 13, 2022

Uh oh!

ondrejmirtes commented May 13, 2022

Uh oh!

Uh oh!

staabm commented May 12, 2022 •

edited

Loading

staabm May 12, 2022 •

edited

Loading

staabm May 13, 2022 •

edited

Loading

staabm May 13, 2022 •

edited

Loading

staabm commented May 13, 2022 •

edited

Loading