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

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

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

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

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 #1781 (review)? Would there be more changes needed or should that work already then?

@herndlm
Should be fixed:+1:

I’ll add some regression tests tomorrow:)

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

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.

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.

memo
Think this pull request can close these
phpstan/phpstan#7075
phpstan/phpstan#6260
phpstan/phpstan#5695
phpstan/phpstan#4969
phpstan/phpstan#4689

@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?

Yes 😊

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.

@ondrejmirtes
Added some tests to see that the cases you mentioned
#1802
is working with the current implementation.

i'm going to fix the comments in
#1802
👍

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.

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.

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.

I've written down some context about this and my plan: phpstan/phpstan#8191

Everything from this PR is either already in 1.9.x or in #1943 :)