Fix false positive dead catch on property assignment by rajyan · Pull Request #1047 · phpstan/phpstan-src

rajyan · 2022-03-04T11:39:34Z

I hope I understood the concept of ThrowPoint correctly...

rajyan · 2022-03-04T13:35:35Z

Rerunning tests because the failing tests seemed not related to this PR.

rajyan · 2022-03-04T14:57:48Z

@ondrejmirtes
Hi, this PR is ready for review.
This is my first PR, and I'm looking forward to contribute more!

ondrejmirtes · 2022-03-05T10:38:17Z

 					$scope = $scope->assignExpression($var, $assignedExprType);
 				}
+				if ($assignedExprType->isSuperTypeOf($propertyReflection->getWritableType())->no()) {
+					$throwPoints[] = ThrowPoint::createImplicit($scope, $assignedExpr);


The error might occur if we have a property with a native type int and we try to assign int|string into it. It means that the condition should be !$assignedExprType->isSuperTypeOf($propertyReflection->getWritableType())->yes() instead. Please add a test and modify the implementation.

Also there might be a situation where the property is not defined at all. In that case we should simulate MethodCall to __set and see what throw points that produces (when the method exists). You can do that by calling $this->processExprNode(new MethodCall($var->var, '__set'), $scope, static function (): void {}, $context)->getThrowPoints().

There's a similar bug that you can look to in the next PR :) phpstan/phpstan#4852

Thank you!

rajyan · 2022-03-06T04:03:49Z

@ondrejmirtes
Thank you very much for your reviews.
I modified the tests and logic with your suggestions. (By the way, my $assignedExprType>isSuperTypeOf($propertyReflection->getWritableType()) was checking in the opposite direction. The tests were useful to understand those cases.)

rajyan · 2022-03-06T06:24:08Z

@ondrejmirtes
I'm looking deeper into throw points.
May I ask several questions?

Difference between explicit/implicit throw points
Do you think this case is a explicit one? (I think it can only throw TypeError)
If I should create an explicit throw point, is there a better way to get predefined classes type from scope than this?

ThrowPoint::createExplicit($scope, $scope->getType(new New_(new Name(\TypeError::class))), $assignedExpr, false)

ondrejmirtes · 2022-03-06T08:32:03Z

Instead of $scope->getType(new New_(new Name(\TypeError::class))) you can do new ObjectType(\TypeError::class). See the documentation: https://phpstan.org/developing-extensions/type-system

Explicit/implicit throw points - "explicit" is for cases where the @throws tag is declared and we know the exact type that is thrown.
Implicit throw point is for cases where some kind of exception might be thrown but we're not sure which one. It's for cases where you're calling a function foo() and it has no @throws tag. Also see the blog post: https://phpstan.org/blog/bring-your-exceptions-under-control#what-does-absent-%40throws-above-a-function-mean%3F

So I guess in this case it's an explicit throw point.

rajyan · 2022-03-06T09:11:28Z

Thank you very much for your information!

Instead of $scope->getType(new New_(new Name(\TypeError::class))) you can do new ObjectType(\TypeError::class).

phpstan-src/src/Type/ObjectType.php

Line 61 in d32942a

private static array $superTypes = [];

Thanks, I had a misunderstanding watching property $superTypes here. I was thinking that calling new ObjectType(\TypeError::class) is not setting the $superTypes, but this property was only a cache.

ondrejmirtes · 2022-03-06T10:16:17Z

 				$scope = $scope->assignExpression($var, $assignedExprType);
+				// simulate dynamic property assign to get throw points
+				$throwPoints = array_merge($throwPoints, $this->processExprNode(
+					new MethodCall($var->var, '__set'),


This needs some tests :)

Class without the __set magic method - does not throw anything (dead catch reported).

Class with __set magic method and @throws void annotation (dead catch reported).

Class with __set magic method and @throws SomeException (dead catch not reported).

👍

For the first case, there is a implicit ThrowPoint already, when method reflection is not found.
I'm not sure with the purpose of this throw point, so I'm gonna check it.

phpstan-src/src/Analyser/NodeScopeResolver.php

Line 1994 in 2167438

$throwPoints[] = ThrowPoint::createImplicit($scope, $expr);

It was from this case
84b0934

Ok, understood
I shouldn’t process __set if its not declared.

ondrejmirtes · 2022-03-07T12:39:39Z

Thank you!

rajyan changed the title ~~Fix/issue 6256~~ Fix false positive dead catch on property assignment Mar 4, 2022

rajyan force-pushed the fix/issue-6256 branch from 7f09f11 to 5036ea6 Compare March 4, 2022 13:34

rajyan marked this pull request as draft March 4, 2022 14:17

rajyan added 6 commits March 4, 2022 23:24

add failing test

199bf80

add test fixtures

8c868a6

create throw point if assigned type is not a writable type

1a4c74e

skip test for versions before 7.4

0e68654

skip lint for versions before 7.4

b88f2e1

enable editorconfig, fix spaces to tab

f574f72

rajyan force-pushed the fix/issue-6256 branch from 02622ae to f574f72 Compare March 4, 2022 14:24

rajyan marked this pull request as ready for review March 4, 2022 14:57

ondrejmirtes requested changes Mar 5, 2022

View reviewed changes

rajyan added 4 commits March 6, 2022 12:54

modify tests

0dc6abb

add test cases for isSuperTypeOf()->maybe()

7b75396

fix throw points for property assign

6d8ff57

add throw points for dynamic property assign

1e1212d

rajyan added 2 commits March 6, 2022 18:10

make property assign throw point explicit

38f08af

add comments to make test case clear at glance

c2570b4

rajyan force-pushed the fix/issue-6256 branch from 942e383 to c2570b4 Compare March 6, 2022 09:10

Merge branch 'master' into fix/issue-6256

2167438

rajyan requested a review from ondrejmirtes March 6, 2022 09:18

make cs-fix

b574fb4

ondrejmirtes requested changes Mar 6, 2022

View reviewed changes

add test cases for __set

74fa61e

rajyan added 2 commits March 6, 2022 20:36

make fix-cs

b844e5b

call __set if only declared

0c52bb5

rajyan requested a review from ondrejmirtes March 6, 2022 23:37

ondrejmirtes approved these changes Mar 7, 2022

View reviewed changes

ondrejmirtes merged commit 4450e46 into phpstan:master Mar 7, 2022

ondrejmirtes mentioned this pull request Mar 7, 2022

PHPStan ignores magic methods in assign undefined property of object phpstan/phpstan#6686

Closed

rajyan deleted the fix/issue-6256 branch March 7, 2022 13:08

ondrejmirtes mentioned this pull request Apr 4, 2023

TypeError dead catch when assigning mixed to int in property phpstan/phpstan#9146

Closed

Navigation Menu

Repository navigation

Fix false positive dead catch on property assignment - #1047

Fix false positive dead catch on property assignment #1047
ondrejmirtes merged 17 commits into
phpstan:masterfrom
rajyan:fix/issue-6256

rajyan commented Mar 4, 2022

Uh oh!

rajyan commented Mar 4, 2022 •

edited

Loading

Uh oh!

rajyan commented Mar 4, 2022

Uh oh!

ondrejmirtes Mar 5, 2022

Uh oh!

rajyan commented Mar 6, 2022 •

edited

Loading

Uh oh!

rajyan commented Mar 6, 2022 •

edited

Loading

Uh oh!

ondrejmirtes commented Mar 6, 2022

Uh oh!

rajyan commented Mar 6, 2022

Uh oh!

ondrejmirtes Mar 6, 2022

Uh oh!

rajyan Mar 6, 2022 •

edited

Loading

Uh oh!

rajyan Mar 6, 2022

Uh oh!

rajyan Mar 6, 2022

Uh oh!

ondrejmirtes commented Mar 7, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rajyan commented Mar 4, 2022

Uh oh!

rajyan commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajyan commented Mar 4, 2022

Uh oh!

ondrejmirtes Mar 5, 2022

Choose a reason for hiding this comment

Uh oh!

rajyan commented Mar 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajyan commented Mar 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ondrejmirtes commented Mar 6, 2022

Uh oh!

rajyan commented Mar 6, 2022

Uh oh!

ondrejmirtes Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

rajyan Mar 6, 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!

rajyan Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

rajyan Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented Mar 7, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rajyan commented Mar 4, 2022 •

edited

Loading

rajyan commented Mar 6, 2022 •

edited

Loading

rajyan commented Mar 6, 2022 •

edited

Loading

rajyan Mar 6, 2022 •

edited

Loading