WIP/RFC - Extend support for treatPhpDocTypesAsCertain to more expressions (initial attempt)

[thg2k]

WIP/RFC - do not merge!

I took a crack at the issue phpstan/phpstan#7075 (also as phpstan/phpstan#7795) but I have a lot of doubts
about this patch, so I'm posting the PR to get some initial feedback in particular
on the following points:

1) What's the difference between promoteNativeTypes(), and the
conditional use of getType() vs getNativeType() based on the setting
treatPhpDocTypesAsCertain?

2) Should we unify all of this for all expressions? It sounds like there
are more cases where we should honor treatPhpDocTypesAsCertain, that why I
introduced the new getTypeOrNative() function. So if this is the way to go,
I would first refactor all existing cases to use this newly introduced method

3) What do you think about the name getTypeOrNative()? I don't like it but
I still would like something short as it's going to be used a lot. Also, should
it be private, protected, or public?

4) This change broke zero tests, that means new tests should be introduced
before making this change. Any recommended strategy? Where should they go?
How should they look like? They clearly need to be ran twice with the different
configuration.

[herndlm]

It's not for me to decide obviously but I like the new helper method. I'd make it private and use it internally wherever possible to simplify things. But better wait for Ondrej's opinion there.

Regarding the test I should be able to help out - you have to add a test for the rule that errors in the linked issue. That test class most likely has a private prop that you can use to enable or disable "treat phpdoc types as certain". And I assume here it should show that no errors are reported.

[ondrejmirtes]

My main interest is to have MutatingScope::getNativeType() to always answer truthfully. There are two parts to that:

1. Handle all expression types in MutatingScope::getNativeType() to give correct answers. There will be some code duplication with MutatingScope::getType() but that doesn't bother me as long as it's thoroughly tested. See https://github.com/phpstan/phpstan-src/blob/1.8.x/src/Reflection/InitializerExprTypeResolver.php to what level code duplication doesn't bother me :) It gives us so much freedom to fix bugs in operations separately, and it's very readable too.
2. Make sure MutatingScope::$nativeExpressionTypes always has the correct information too. This means that methods' MutatingScope::assignExpression() and MutatingScope::specifyExpressionType() $nativeType argument shouldn't be optional to force us to always set it correctly.

The benefit I'm looking for here is to be able to implement a rule that tells us when /** @var Type $var */ is always wrong. Consider this:

/** @var string|null $var */
$var = $this->returnsString();

We'd know the @var is wrong if the method has : string native return type.

And once we have that, any treatPhpDocTypesAsCertain situation is along for the ride and starts working automatically thanks to this piece of code:

phpstan-src/src/Rules/Comparison/ConstantConditionRuleHelper.php

Lines 72 to 76 in bd971db

	if ($this->treatPhpDocTypesAsCertain) {
	return $scope->getType($expr)->toBoolean();
	}
	return $scope->getNativeType($expr)->toBoolean();

I also realize that the MutatingScope::getType() path shouldn't ask for $this->treatPhpDocTypesAsCertain at all (and this PR too), so for example this code is wrong

phpstan-src/src/Analyser/MutatingScope.php

Lines 734 to 738 in bd971db

	if ($this->treatPhpDocTypesAsCertain) {
	$exprBooleanType = $this->getType($node->expr)->toBoolean();
	} else {
	$exprBooleanType = $this->getNativeType($node->expr)->toBoolean();
	}

. Once MutatingScope::getNativeType() is fully implemented, it won't be necessary.

Of course this is a massive undertaking and will need like 1k-2k lines of code at least.

[rajyan]

I'm not sure if my pull request is on the right track, but I have a WIP pull request that may help? and is related to this.
#1535
Right now I don't have enough time to complete this, so I welcome anyone to overtake this pull request:+1:

[rajyan]

My pull request is mainly trying to solve 2. that ondrej mentions in #1627 (comment)
and lacking 1. I think.

[herndlm]

sorry to hijack this, I thought it's a good place for further getType / getNativeType questions. I noticed that TypeSpecifier mostly uses getType() which basically ignores the native type, right? And I noticed that e.g. SpecifiedTypes::normalize() does use only getType() too, which can lead to wrong type normalization before intersection (e.g. specifying !isset($foo) || !$foo kind of expressions).

I guess the question is: Should getType() return the native type in case treatPhpDocTypesAsCertain is off and I found a bug or do I need to adapt SpecifiedTypes::normalize() maybe to consider treatPhpDocTypesAsCertain too?

[ondrejmirtes]

@herndlm I'm not sure about these arguments. Please try to find any wrong behaviour with phpstan.org/try and open a separate issue for it.

[ondrejmirtes]

Hi, I'm cleaning up old and stale PRs. Please send a new PR if you're still interested, thanks.

This attempt doesn't work for us, there's some work being done in #1802.