Fix native types2

[rajyan]

closes https://github.com/phpstan/phpstan-src/pull/1535 closes https://github.com/phpstan/phpstan-src/pull/1627

and a lot of native type related issues!

Native types should always be specified along phpdoc types. Also, there should be only a single treatPhpDocTypesAsCertain handling to switch between variableTypes and nativeTypeExpression https://github.com/phpstan/phpstan-src/blob/b2bf589eface47075bb83d7e75f582f8aa218d72/src/Analyser/MutatingScope.php#L506-L511

With this pull request, native types and phpdoc types are completely separated by treatPhpDocTypesAsCertain.

I hope I've understood https://github.com/phpstan/phpstan-src/pull/1372#issuecomment-1143182394

So - if you're interested in $treatPhpDocTypesAsCertain=true, call Scope::getType(), if false, call Scope::getNativeType(). Can you imagine this working? Thanks :)

this correctly...

[rajyan]

I think it’s almost done. Several native type specification are missing.

[rajyan]

Finally done it? I hope this pull request makes sense:smile:

[rajyan]

I prefer heading to 1.9.x because it's gonna be a huge change for treatPhpDocTypesAsCertain: false

[rajyan]

The failing tests are from treatPhpDocTypesAsCertain: false. I've haven't checked all errors yet, but looks mostly correct to me.

[herndlm]

I don't want to disturb the discussion here too much, so it's not important, but do you maybe know how this relates to https://github.com/phpstan/phpstan-src/pull/1781#pullrequestreview-1129131318? Would there be more changes needed or should that work already then?

[rajyan]

@herndlm Should be fixed:+1:

[rajyan]

I’ll add some regression tests tomorrow:)

[ondrejmirtes]

I'm looking forward to review this, looks promising 😊

[ondrejmirtes]

Alright, I like some parts but don't like some others :)

I'd like to establish some ground rules that should be true when this PR is in progress and after it's merged:

• Scope::getType() returns "complete type" or "full type", however you want to call it.
• Scope::getNativeType() returns "native type".
• Scope/MutatingScope shouldn't be interested in treatPhpDocTypesAsCertain at all. Meaning it's OK to delete doNotTreatPhpDocTypesAsCertain() method, even promoteNativeTypes(). Well, doNotTreatPhpDocTypesAsCertain() should be kept for backward compatibility, but deprecated in favour of calling Scope::getNativeType(). It's OK if it does return $this;.
• MutatingScope::getNativeType() should mirror MutatingScope::getType(). At the end of the method (for unhandled expression types), it shouldn't call getType() (because that's lying), but instead return new MixedType().
• Scope has to keep two sets of types (complete types and full types) for all operations. Besides $moreSpecificTypes property we'll also need $moreSpecificNativeTypes. For example filterBySpecifiedTypes() has to operate on these two. Let's take this example:

/**
 * @param positive-int|non-empty-string $a
 */
function foo(int|string $a): void
{
    assertType('positive-int|non-empty-string', $a);
    assertNativeType('int|string', $a);
    if (is_string($a)) {
        assertType('non-empty-string', $a);
        assertNativeType('string', $a);
    }
}

The implementation of MutatingScope::getNativeType() has to handle all the same expressions MutatingScope::getType() handles, but slightly differently. Here are some examples:

if ($node instanceof Expr\BinaryOp\Smaller) {
    return $this->getNativeType($node->left)->isSmallerThan($this->getNativeType($node->right))->toBooleanType();
}

if ($node instanceof Node\Expr\BitwiseNot) {
    return $this->initializerExprTypeResolver->getBitwiseNotType($node->expr, fn (Expr $expr): Type => $this->getNativeType($expr));
}

• For FuncCall/MethodCall/StaticCall/New_, MutatingScope::getType() does a lot of complicated things, like calling dynamic return type extensions and resolving generics. These expressions in MutatingScope::getNativeType() will be handled completely differently. For example, MethodCall has to get ClassReflection from ReflectionProvider, ask about hasNativeMethod(), get the getNativeMethod() and ask for native return type. (This will need getNativeReturnType() added on ExtendedMethodReflection - should be a separate PR that's gonna be merged/reviewed first. Right now the only ExtendedMethodReflection impl that has this method is PhpMethodReflection.)
• What's a bit worrying for me now is that you changed and written a lot of code but there isn't a single new test using assertNativeType. This is gonna have to be tested a lot. Like the things around array dim fetch assign (NodeScopeResolver around line 3400), it's ideal to test them with assertNativeType.
• I don't understand how the rules can still do their job if they don't know what the value of $treatPhpDocTypesAsCertain is. I imagine that ConstantConditionRuleHelper should continue to use $treatPhpDocTypesAsCertain and do their job with logic as $this->treatPhpDocTypesAsCertain ? $this->getType($expr) : $this->getNativeType($expr). If you deleted this logic, I don't know where it went and why (if) it still works as it should.

[ondrejmirtes]

It's interesting to know that the example with type narrowing on variables already works: https://phpstan.org/r/59b890fb-466d-4926-b060-15859b520c63

That's because today there's MutatingScope::$variableTypes and MutatingScope::$nativeExpressionTypes. It should actually be called MutatingScope::$variableNativeTypes because that's what it is.

The test wouldn't work for other expressions like property fetches and array dim fetches. That's why we need MutatingScope::$moreSpecificNativeTypes alongside MutatingScope::$moreSpecificTypes.

[rajyan]

memo Think this pull request can close these https://github.com/phpstan/phpstan/issues/7075 https://github.com/phpstan/phpstan/issues/6260 https://github.com/phpstan/phpstan/issues/5695 https://github.com/phpstan/phpstan/issues/4969 https://github.com/phpstan/phpstan/issues/4689

[rajyan]

@ondrejmirtes Thank you for the review!

Scope/MutatingScope shouldn't be interested in treatPhpDocTypesAsCertain at all. Meaning it's OK to delete doNotTreatPhpDocTypesAsCertain() method, even promoteNativeTypes(). Well, doNotTreatPhpDocTypesAsCertain() should be kept for backward compatibility, but deprecated in favour of calling Scope::getNativeType(). It's OK if it does return $this;.

You mean that Scope always have both native and phpdoc type, and treatPhpDocTypesAsCertain should be handled in the Rule side to switch between native and phpdoc types?

[ondrejmirtes]

Yes 😊

[ondrejmirtes]

Another important test is this:

function doFoo(): string
{
}

function (): void {
    /** @var non-empty-string $a */
    $a = doFoo();
    assertType('non-empty-string', $a);
    assertNativeType('string', $a);
};

Meaning that everything around processStmtVarAnnotation and processVarAnnotation in NodeScopeResolver has to be skipped for native types.

[rajyan]

@ondrejmirtes Added some tests to see that the cases you mentioned https://github.com/phpstan/phpstan-src/pull/1802/#issuecomment-1272472873 is working with the current implementation.

i'm going to fix the comments in https://github.com/phpstan/phpstan-src/pull/1802/#issuecomment-1272396669 :+1:

[ondrejmirtes]

To give you a bit more context, thanks to more precise Scope::getNativeType(), I'll be able to do two awesome things:

Validate inline @var

Let's take this example:

function doFoo(): string
{
}

function (): void {
    /** @var int $a */
   $a = doFoo();
};

I'll be able to report "Type int in PHPDoc tag @var is not a subtype of native type string". Right now we can't do this because @var is used for different purposes:

• To narrow down types. This is better served by actually fixing the PHPDocs, using generics, and conditional return types.
• To fix wrong 3rd party PHPDocs. This is better served by using stub files.

Because of this our options of reporting wrong code are pretty limited. But I came up with this rule (That depends on correct Scope::getNativeType()) and we'll be able to report cases that are definitely wrong thanks to native types.

Redesign rule levels

Right now the design of levels mirrors how I proceeded with PHPStan development. The rules I implemented first are checked on lower levels.

Calling a function with wrong argument types is reported on level 5.

But what would make more sense is to report "this is definitely an error" on lower levels. And Scope::getNativeType() will help us with that.

In continuous delivery fashion, I'll be able to do this on 1.x while keeping PHPStan stable. Because these new levels will have different names. Right now you set a level with --level 2 for example. These new levels will have some prefix or suffix, so people would be able to switch to them by doing --level x2 or similar.

[ondrejmirtes]

One thing I can suggest here: We can finish the current impact of this PR once we're happy with it and it's tested in sufficient manner. And merge it.

We don't have to implement every Expr type in getNativeType() right now, it's a lot of code and I worry it would become unmanagable. That can be done in the next PRs.

[rajyan]

@ondrejmirtes Thank you for the suggestions, but what I'm struggling now is that

This lind should just return new MixedType.

is making getNativeType more correct, but breaking too much test related to ImpossibleCheckType and hard to make them green, because the line was returning getType() there before and it is relying on it.

[rajyan]

I'm very sorry, but I could not make this done... I need someones help to make the tests green and working...

[ondrejmirtes]

Of course, I'll help you, I'll look at it and see 😊

[ondrejmirtes]

You're not alone meant to figure it out, open-source is a collaborative effort and I appreciate the help from others, so when I see something I really want, I want to dive into the problem too and push commits to the PR to bring it over the finish line :)

[rajyan]

You're not alone meant to figure it out

I really appreciate your support! I will continue to look into it too.

[ondrejmirtes]

What needs to happen now to get rid od the wrong error tips is to implement MutatingScope::getNativeType() for all expressions. I'll look into it this week if time permits it 😊

[ondrejmirtes]

I force-pushed some commits here. It makes a lot of test failures go away.

The main commit is 919d276 (#1802). It's 💩 and duplicated code but it's fine by me.

There's a lot of todos. I believe these todos will make the remaining failures go away.

• Todo comments in the linked commit - some expressions aren't handled correctly yet.
• $this native variable isn't yet handled correctly
• We should really make $nativeExpressionTypes property work like $variableTypes and rename it to $variableNativeTypes.
• We need $moreSpecificNativeTypes - TypeSpecifier should work on both full and native types.
• filterByTruthyValue and filterByFalsyValue needs to work on native types too (some of the todos in MutatingScope are about this)
• ImpossibleCheckTypeHelper also doesn't work for treatPhpDocTypesAsCertain=false

Feel free to orient yourself in my new commits, understand how and why they work, and feel free to chip away from those todos :) Also some code cleanup suggestions are welcome.

Thank you for now.

[rajyan]

Thank you very much for your help! I investigated where some errors come from. Maybe can fix them.

For example,

/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/data/bug-4099.php:33

is from wrong moreSpecificTypes which you mentioned too as todo.

The remaining errors are related to getType used in NodeScopeResolver. I now remembered that the reason that I selected my approach (making getNativeType === doNotTreatPhpDocTypesAsCertain()->getType) first in https://github.com/phpstan/phpstan-src/pull/1535 was because NodeScopeResolver, TypeSpecifier, SpecifiedTypes are relying on getType.

I understand that this is against your ground rules, but https://github.com/phpstan/phpstan-src/pull/1805 uses getNativeType === doNotTreatPhpDocTypesAsCertain()->getType approach which can fix the errors (and most todos you mentioned) above, and working almost same as this one (it is using getType and resolveTypes with nativeVariables instead of the code duplication)

[ondrejmirtes]

FYI these changes should be safe to merge https://github.com/phpstan/phpstan-src/pull/1823 so I'll do it and then rebase the rest of this PR :)

[ondrejmirtes]

About your last comment: https://github.com/phpstan/phpstan-src/pull/1802#issuecomment-1274908424

Yes, I'm starting to realize we'll need something like Scope::doNotTreatPhpDocTypesAsCertain() because of TypeSpecifier/SpecifiedTypes, BUT the name might be misleading, because the usual Scope that's used since the start of the analysis must have $treatPhpDocTypesAsCertain=true REGARDLESS of the project setting in phpstan.neon.

So I'm inclining towards something like bool $nativeTypesPromoted or something like that. I'll try to prototype it.

[rajyan]

TypeSpecifier/SpecifiedTypes, BUT the name might be misleading, because the usual Scope that's used since the start of the analysis must have $treatPhpDocTypesAsCertain=true REGARDLESS of the project setting in phpstan.neon. So I'm inclining towards something like bool $nativeTypesPromoted or something like that. I'll try to prototype it.

Totally agreed.

[ondrejmirtes]

I've written down some context about this and my plan: https://github.com/phpstan/phpstan/issues/8191

[ondrejmirtes]

Everything from this PR is either already in 1.9.x or in https://github.com/phpstan/phpstan-src/pull/1943 :)