Native types todos

[ondrejmirtes]

Feature request

1.9.x is now pretty broken for projects with treatPhpDocTypesAsCertain: false: https://github.com/phpstan/phpstan/actions/runs/3297631860/jobs/5438715560

This is because there's some work underway, but more needs to be done to fully implement it. From phpstan/phpstan-src#1802 I already merged some commits into 1.9.x: phpstan/phpstan-src@f097ca9...bf34e24

Before the rest of phpstan/phpstan-src#1802 is finished and merged, we have some work to do (in this order with no skipping):

1. Get rid of $nativeExpressionTypes property in MutatingScope, in favour of $variableNativeTypes.
2. Add and correctly populate $moreSpecificNativeTypes to MutatingScope. The hard question is what to do about TypeSpecifier. We probably need to call it twice in filterByTruthyValue/filterByFalseyValue/filterBySpecifiedTypes for complete and native types in order to have correct information.
3. Some preliminary work too in order to retrieve native return type from ExtendedMethodReflection nicely, same for PropertyReflection.

And on top of phpstan/phpstan-src#1802:

4. Get rid of phpstan/phpstan-src@8f7bbf7 (#1802) and do it in a smarter way. MutatingScope::getNativeType() should probably be implemented with a simple one-liner: return $this->promoteNativeTypes()->getType($node); and for expressions where the implementation is different do an if-else based on bool $this->nativeTypesPromoted. But most expressions will be the same.

To remind, having Scope::getNativeType() to answer truthfully for all expressions will allow us to do some cool features:

• I'll be able to implement a rule that checks when an inline @var is always wrong, because the thing on the right side of = has a different type than what it's cast to.
• I'll be able to eventually redesign rule levels and do a different slice across the whole rules catalogue to report useful errors earlier.

[ondrejmirtes]

Good news: turns out the Composer project failure linked above wasn't really fault of native types but rather of this bug: phpstan/phpstan-src@8369197

So this feature isn't blocking PHPStan 1.9.0 release and we can work on it afterwards.

[rajyan]

Thank you for your descriptive information!

Thank you for

return $this->promoteNativeTypes()->getType($node);

I think this is a smart and explicit solution compared to phpstan/phpstan-src#1805. If my understanding is correct, should we promote $moreSpecificNativeTypes in promoteNativeTypes() too?

Also some things to note

• correctly handle 'this' type for $variableNativeType
• we probably need native version of $conditionalExpressions like $moreSpecificNativeTypes too?

[ondrejmirtes]

I brainstormed about this and realized we're stuck because of how things are currently organized. There's too much stuff and we'd have to duplicate all of it for native types:

• $variableTypes
• $moreSpecificTypes
• $conditionalExpressions

We should clean it up first and then implement native types on top of a clean foundation. It follows an advice that always worked great for me: First make the change easy (refactor and clean up so future work is easier) and then make the easy change (once the foundation is prepared, it's much easier to implement it). And ideally do that with small, iterative and managable PRs with separate refactorings :)

So what I propose to do first before we even think about native types is:

1. Merge $variableTypes and $moreSpecificTypes into new $expressionTypes. There's no reason to treat variables differently for example from property fetches.
2. What I think should be possible is to merge $conditionalExpressions with $expressionTypes as well, using LateResolvableTypes. Current examples of LateResolvableType (https://apiref.phpstan.org/1.9.x/PHPStan.Type.LateResolvableType.html) are conditional types. What current ConditionalExpressionHolder does is holding information "if variable $x is of this type then $y is going to be of this type", so nothing complicated, it should be possible to express it using LateResolvableType too.

This is gonna be a lot of cleanup. But in the end the code will be much simpler. And we're definitely gonna learn a lot in the process :)

So after these refactorings we're gonna end up with a single $expressionTypes property instead of 3. And that should make working with native types with a new $expressionNativeTypes property much easier :)

@rajyan I think you're definitely qualified for these refactorings so feel free to dive in, should be fun :)

[rajyan]

@ondrejmirtes

I remember we have discussed this before 👍
phpstan/phpstan-src#1270 (comment)

What I brain stormed at that time was, there might be a better data structure to handle these invalidation or we have to loop through all the expression types to invalidate them.
https://github.com/phpstan/phpstan-src/blob/eecc5e070be4645f01b4397982e07a0db8fc1e2d/src/Analyser/MutatingScope.php#L3275-L3289
but, what I'm now thinking is that the first implementation can be an simple associative array like now, it shouldn't degrade the performance that much.

[ondrejmirtes]

Please note that the regex is an old way of doing invalidation and would only work for variables.

The modern way is NodeFinder: https://github.com/phpstan/phpstan-src/blob/65f452d461cd7dbbbc8ff304b127fa2f95aedc46/src/Analyser/MutatingScope.php#L3676

Also note that we need to preserve methods like hasVariableType and getVariableType but since there's no longer be a separate storage for them, they simply need to construct new Variable node and ask a more general method. Same applies to assignVariable...

[ondrejmirtes]

BTW: As part of the $conditionalExpressions refactoring, we should be able to get rid of some of these bugs, and implement some of these feature requests :) https://github.com/phpstan/phpstan/milestone/12

[ondrejmirtes]

Pulling current todos from comments around GitHub:

Blocking:

• Implement handling Foo::BAR and static::BAR in defined()
• Composer failure in Merge constant types phpstan-src#1938 (comment)

Some inconsistencies in handling $this->expressionTypes and $this->nativeExpressionTypes. For example in:

• specifyExpressionType (lines 3457, 3458) - one is assigned, another one is unset
• invalidateExpression - it goes over keys in $this->expressionTypes, does some logic and then tries to unset the keys on both expressionTypes and nativeExpressionTypes. I'd rather see the same foreach run twice separately.
• Same in invalidateMethodsOnExpression
• Sometimes large blocks of logic are unnecesarily duplicated for expressionTypes and nativeExpressionTypes, it'd be nicer to have some new private methods instead. (for example in processAlwaysIterableForeachScopeWithoutPollute)
• Private method addMoreSpecificTypes only handles expressionTypes and doesn't do anything with nativeExpressionTypes. It's only called from specifyExpressionType but I feel like something should be done here, especially when the method has Type $nativeType in the parameters.
• processClosureScope - same $variableType is used for both expressionTypes and nativeExpressionTypes which is certainly wrong.
• $this in enterClosureBind, restoreOriginalScopeAfterClosureBind, and enterClosureCall

And then:

• ConditionalExpressionHolder should work for all expressions, not just variables
• Also the places that call MutatingScope::addConditionalExpressions() are quite weird and work only for assigning variables, this should be cleaned up
• Rework ConditionalExpressionHolder to become part of $expressionTypes too
• Call TypeSpecifier in filterByTruthyValue/filterByFalseyValue/specifyTypesInCondition so that nativeExpressionTypes are correct
• Things in Promote native types phpstan-src#1943
• MutatingScope::resolveType() should respond correctly when native types are promoted for FuncCall/StaticCall/MethodCall/PropertyFetch/StaticPropertyFetch. Needs some preliminary work so we can easily ask for native types.

[rajyan]

I’m gonna address the first two blocking ones in tonight and tomorrow night.

[ondrejmirtes]

The Composer issue is reproduced in this simple snippet: https://phpstan.org/r/c875db3e-3640-4288-b9ff-92a6a13906cd

[ondrejmirtes]

Well, that was easy: phpstan/phpstan-src@bf1ef35

[phpstan-bot]

@ondrejmirtes After the latest push in 1.9.x, PHPStan now reports different result with your code snippet:

@@ @@
-28: Expected native type mixed, actual: (int|string)
-29: Strict comparison using === between (int|string) and null will always evaluate to false.
-31: Expected native type null, actual: *NEVER*
-38: Expected native type mixed, actual: (int|string)
+38: Expected native type mixed, actual: mixed~null

Full report

Line	Error
38	Expected native type mixed, actual: mixed~null

[rajyan]

@ondrejmirtes
Thanks!
Sorry I should had cherry-picked that from
rajyan/phpstan-src@eecc5e0
phpstan/phpstan-src#1805
totally forgot about it.

[ondrejmirtes]

This is almost done. Yeah, sure, some things could still be done but we don't need to keep this huge issue open :)

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.