Remember narrowed types from the constructor when analysing other methods by staabm · Pull Request #3930 · phpstan/phpstan-src

staabm · 2025-04-11T08:52:37Z

closes phpstan/phpstan#12860
closes phpstan/phpstan#10048
closes phpstan/phpstan#11828
closes phpstan/phpstan#9075
closes phpstan/phpstan#6063
closes phpstan/phpstan#12723

src/Analyser/MutatingScope.php

ondrejmirtes · 2025-04-11T08:57:35Z

src/Analyser/NodeScopeResolver.php

+					return 0;
+				}
+
+				return [!$a->isStatic(), $a->name->toLowerString() !== '__construct'] <=> [!$b->isStatic(), $b->name->toLowerString() !== '__construct'];


We have a better way to ask for constructor, that works for same-named constructors as class on PHP 7.

ExtendedMethodReflection has no isConstructor method.

I took inspiration from

phpstan-src/src/Analyser/NodeScopeResolver.php

Line 654 in 375f68e

$isConstructor = $isFromTrait || $stmt->name->toLowerString() === '__construct';

which seems to be also wrong then?

I looked into adding ExtendedMethodReflection->isConstructor(): TrinaryLogic, but this is incompatible with
PhpMethodFromParserNodeReflection->isConstructor(): bool

any preferences how you want this to be resolved?

I think we can leave as it is. This is mostly useful for readonly properties anyway, which means __construct() constructors...

staabm · 2025-04-11T08:59:07Z

re-ordering the order in which methods get analyzed affected the ordering of errors in tests... I am not yet sure where/how to compensate that

ondrejmirtes · 2025-04-11T09:04:07Z

Here's this line:

phpstan-src/src/Testing/RuleTestCase.php

Line 244 in 375f68e

    
           $finalizer->finalize($analyserResult, false, true)->getAnalyserResult()->getUnorderedErrors(),

You could call getErrors instead of getUnorderedErrors. Not sure what the side effects might be, hopefully it's not brutal.

staabm · 2025-04-11T14:48:58Z

I have put it behind bleeding edge and added corresponding opt-in methods for the test-base-classes so we can move forward without the need to adjust all tests

phpstan-bot · 2025-04-11T14:50:24Z

This pull request has been marked as ready for review.

staabm · 2025-04-12T07:35:16Z

sorry for push bombing :-) - I am finished now

conf/bleedingEdge.neon

src/Analyser/NodeScopeResolver.php

ondrejmirtes · 2025-04-13T11:40:10Z

I just realized there's one more thing we could use this for: with natively typed properties (non-nullable), expressions like isset or ?? do not report errors. Because these properties might be uninitialized and isset or ?? should report an error.

We should preserve PropertyInitializationExpr and then check for that in IssetCheck.

This is for non-readonly properties too.

staabm · 2025-04-13T16:19:30Z

We should preserve PropertyInitializationExpr and then check for that in IssetCheck.

great idea. done

ondrejmirtes

This is getting really close to merging 👍

ondrejmirtes · 2025-04-13T17:46:36Z

src/Rules/IssetCheck.php

+					&& $expr->name instanceof Node\Identifier
+					&& $expr->var instanceof Expr\Variable
+					&& $expr->var->name === 'this'
+					&& !TypeCombinator::containsNull($propertyReflection->getNativeType())


Doing Type::isNull() would make more sense. This example doesn't cover mixed (which can include null).

But I think we should pay more attention to $typeMessageCallback passed to IssetCheck. For both the type check and the error message. Something a little bit different happens in EmptyRule which should be covered by a set of tests. (Because empty($x) is !isset($x) || $x == false).

ondrejmirtes · 2025-04-13T17:47:02Z

src/Rules/IssetCheck.php

+							$this->propertyDescriptor->describeProperty($propertyReflection, $scope, $expr),
+							$operatorDescription,
+						),
+					)->identifier(sprintf('%s.neverNullOrUninitialized', $identifier))->build();


This identifier should include property

…hods

staabm · 2025-04-13T21:03:52Z

the 2 failling doctrine tests look valid

ondrejmirtes · 2025-04-14T08:58:03Z

Perfect, thank you!

herndlm · 2025-04-18T11:23:21Z

Hi, this looks pretty neat, thx! I was just wondering why e.g. https://phpstan.org/r/9c85e54d-6c87-42ee-84d5-6db7447c3e4e does not work in the same way? Is it possible to remember such expressions too? UPDATE: changed playground snippet, there was a bug..

Sorry, haven't thought too much about how this works, is it related to the fact that other methods might write / change the property of the wrapper object anytime in between the constructor and method call?

staabm · 2025-04-18T11:34:07Z

we should be able to remember anything which cannot change after constructor was executed (e.g. because its readonly or because the runtime does not allow to change it once set by design).

here is the code which decides what will be remembered and what skipped:
https://github.com/phpstan/phpstan-src/pull/3930/files#diff-dc817f2bab8672057a95375a542e68599164f6ab2380e4cccd1b50583e295d85R305-R334

herndlm · 2025-04-18T11:42:00Z

cool, thx! I think my real case I experienced yesterday was not using readonly props, so it wouldn't work.. and this one is somewhat super special. I'll think about it maybe :)

ondrejmirtes requested changes Apr 11, 2025

View reviewed changes

clxmstaab force-pushed the constr-scope branch from a8acf5a to 65c1696 Compare April 11, 2025 10:35

staabm mentioned this pull request Apr 11, 2025

RuleTestCase: Order errors #3931

Closed

staabm marked this pull request as ready for review April 11, 2025 14:50

ondrejmirtes requested changes Apr 13, 2025

View reviewed changes

conf/bleedingEdge.neon Outdated Show resolved Hide resolved

src/Analyser/NodeScopeResolver.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Apr 13, 2025

View reviewed changes

staabm and others added 17 commits April 13, 2025 21:54

Remember narrowed types from the constructor when analysing other met…

877eb3a

…hods

test 2nd class does not inherit previous __construct() scope

e39b60e

More tests

01975bd

Remember types of readonly properties

4842d2b

test property hooks

6344cc1

Remember class_exists() from constructor

e4bc05b

Remember function_exists() from constructor

baa78f3

Another test

8c9fa55

Added bleeding-edge feature flag

21bfc39

test readonly class

9745da0

test interface_exists()

1ac6f76

test narrowing oft *_exists() functions

d775762

more precise comment

6628049

test remembers define() from __construct()

a04a5fd

test more scenarios which should not remember scope

3f793d9

enable narrowMethodScopeFromConstructor in AnalyserTest

b140ce4

test narrowMethodScopeFromConstructor can be disabled

1756f85

staabm added 3 commits April 13, 2025 21:54

enable for everyone

0fbc568

fix determination of final scope

534b11b

error on initialized non-nullable properties

4e1889b

staabm force-pushed the constr-scope branch 2 times, most recently from 75ca7d5 to e9343d7 Compare April 13, 2025 20:03

Cover EmptyRule

e33a574

staabm force-pushed the constr-scope branch from e9343d7 to e33a574 Compare April 13, 2025 20:20

staabm added 2 commits April 14, 2025 08:50

Added regression test

49e771e

Added regression test

e4d70b8

clxmstaab force-pushed the constr-scope branch from e850617 to e4d70b8 Compare April 14, 2025 07:01

ondrejmirtes merged commit da74773 into phpstan:2.1.x Apr 14, 2025
415 of 417 checks passed

staabm deleted the constr-scope branch April 14, 2025 08:59

staabm mentioned this pull request Jul 8, 2025

Don't remember broader types from __construct() phpstan/phpstan#13220

Closed

staabm mentioned this pull request Sep 10, 2025

Remember narrowed types from the constructor from deep PropertyFetches #4315

Merged

staabm mentioned this pull request Jan 16, 2026

Drop MutatingScope->rememberConstructorScope() #4768

Merged

Conversation

staabm commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ondrejmirtes Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

staabm Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

staabm Apr 14, 2025 • 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 Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

staabm commented Apr 11, 2025

Uh oh!

ondrejmirtes commented Apr 11, 2025

Uh oh!

staabm commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phpstan-bot commented Apr 11, 2025

Uh oh!

staabm commented Apr 12, 2025

Uh oh!

Uh oh!

Uh oh!

ondrejmirtes commented Apr 13, 2025

Uh oh!

staabm commented Apr 13, 2025

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

staabm commented Apr 13, 2025

Uh oh!

Uh oh!

ondrejmirtes commented Apr 14, 2025

Uh oh!

herndlm commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staabm commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

herndlm commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

staabm commented Apr 11, 2025 •

edited

Loading

staabm Apr 14, 2025 •

edited

Loading

staabm commented Apr 11, 2025 •

edited

Loading

herndlm commented Apr 18, 2025 •

edited

Loading

staabm commented Apr 18, 2025 •

edited

Loading