phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

One solution

Open rajyan opened this issue 3 years ago • 10 comments

tmp pr. just want to know the results of the integration tests.

rajyan avatar Oct 09 '22 16:10 rajyan

This is one possible solution I can think of.

  1. treatPhpDocTypesAsCertain is kept in MutatingScope, but can only be switched from rules
  2. treatPhpDocTypesAsCertain: false makes getType to return native types
  3. nativeTypes uses https://github.com/phpstan/phpstan-src/pull/1805/files#diff-dc817f2bab8672057a95375a542e68599164f6ab2380e4cccd1b50583e295d85R2116 which should be mostly correct and least an improvement, because the current getNativeType is falling back to full types.

rajyan avatar Oct 09 '22 17:10 rajyan

The failing test in pocketmine for example https://github.com/phpstan/phpstan-src/actions/runs/3214922744/jobs/5255662194 is showing that the native types and specification that wasn't working before, is working with this pull request.

rajyan avatar Oct 09 '22 18:10 rajyan

This isn't the direction I want to go for. It breaks some of the ground rules I set in comments of the other PR, and is less useful for my goals.

For example - making getType return only native type when the setting is set to false, it completely breaks user expectation. Basically it says "don't use PHPDocs". Which isn't what the setting is about. We still want to infer types using PHPDocs, we just don't want to complain if the user double-checks the passed type in function arguments.

ondrejmirtes avatar Oct 09 '22 21:10 ondrejmirtes

I've got some ideas about this, I'll trynto explore them soon 😊

ondrejmirtes avatar Oct 11 '22 19:10 ondrejmirtes

Can you please tell me which place in NodeScopeResolver has to care about getType() vs getNativeType()?

ondrejmirtes avatar Oct 11 '22 19:10 ondrejmirtes

@ondrejmirtes

Can you please tell me which place in NodeScopeResolver has to care about getType() vs getNativeType()?

For example, The test failure of https://github.com/phpstan/phpstan-src/pull/1802 https://github.com/phpstan/phpstan-src/actions/runs/3227859202/jobs/5283202887

  1. PHPStan\Rules\Comparison\UnreachableIfBranchesRuleTest::testDoNotReportPhpDoc Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ '18: Elseif branch is unreachable because previous condition is always true. 28: Else branch is unreachable because previous condition is always true. 38: Elseif branch is unreachable because previous condition is always true. +54: Else branch is unreachable because previous condition is always true. +64: Elseif branch is unreachable because previous condition is always true.

Is caused by getType in this line https://github.com/phpstan/phpstan-src/blob/e42952ecc182f1cff8c17761d1efba5598555b24/src/Analyser/NodeScopeResolver.php#L704

$alwaysTerminating is determined by the $conditionType which uses full types and causing the above error.

rajyan avatar Oct 12 '22 00:10 rajyan

About UnreachableIfBranchesRuleTest - I don't think so because UnreachableIfBranchesRule already asks correctly about it and the code in NodeScopeResolver doesn't influence that:

$conditionType = $this->treatPhpDocTypesAsCertain ? $scope->getType($condition) : $scope->getNativeType($condition);
$conditionBooleanType = $conditionType->toBoolean();

I think the problem is in some mixup of TypeSpecifier/SpecifiedTypes.

ondrejmirtes avatar Oct 12 '22 08:10 ondrejmirtes

I was wrong with my explanation. The problem is caused by isAlwaysTrue The scope merge is performed based on isAlwaysTrue of full types, which is causing wrong type for phpdoc types simple example https://phpstan.org/r/a8479355-de29-4e8c-9a08-7a2612ee94e1

rajyan avatar Oct 13 '22 03:10 rajyan

https://github.com/phpstan/phpstan-src/pull/1805#issuecomment-1276968300 I'll try if this example works with this solution tonight, adding as test.

rajyan avatar Oct 13 '22 03:10 rajyan

I now realized that this solution is not correct. I think we need a separate two scopes for each full type and native type?

rajyan avatar Oct 13 '22 03:10 rajyan

What's the purpose of this PR when compared to https://github.com/phpstan/phpstan-src/pull/1802 and in context of my plan at https://github.com/phpstan/phpstan/issues/8191? Thank you :)

ondrejmirtes avatar Oct 21 '22 13:10 ondrejmirtes

If there's something in here that still makes sense, please post it as a new PR with this base: https://github.com/phpstan/phpstan-src/pull/1943 Thank you!

ondrejmirtes avatar Oct 31 '22 08:10 ondrejmirtes