Fix native types2 #1802

rajyan · 2022-10-08T10:31:34Z

closes #1535
closes #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

phpstan-src/src/Analyser/MutatingScope.php

Lines 506 to 511 in b2bf589

    
           if (!$this->treatPhpDocTypesAsCertain) { 
        
           	if (!array_key_exists(sprintf('$%s', $variableName), $this->nativeExpressionTypes)) { 
        
           		return new MixedType(); 
        
           	} 
        
           	return $this->nativeExpressionTypes[sprintf('$%s', $variableName)]; 
        
           }

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

I hope I've understood
#1372 (comment)

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 · 2022-10-08T10:31:49Z

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

rajyan · 2022-10-08T15:08:19Z

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

rajyan · 2022-10-08T15:09:35Z

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

rajyan · 2022-10-08T15:12:15Z

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

src/Analyser/MutatingScope.php

herndlm · 2022-10-08T16:03:51Z

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?

rajyan · 2022-10-08T16:08:07Z

@herndlm
Should be fixed:+1:

rajyan · 2022-10-08T16:09:44Z

I’ll add some regression tests tomorrow:)

ondrejmirtes · 2022-10-08T16:28:53Z

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

ondrejmirtes · 2022-10-08T21:00:16Z

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 · 2022-10-08T21:01:12Z

tests/PHPStan/Rules/Comparison/MatchExpressionRuleTest.php

+		$this->analyse([__DIR__ . '/data/bug-7622.php'], [
+			[
+				'Match expression does not handle remaining value: string',
+				21,


This is wrong

Is this? I'm not sure if native type can know that the match is exhaustive.

But the rule always asks getType, not getNativeType. The pupose of the setting treat... to false is to remove errors like "this if is always false" (when code checks again something that's only expressed in PHPDocs and not checked at runtime), not add new errors.

ok, I think

The pupose of the setting treat... to false is to remove errors like "this if is always false"

was what I was totally misunderstanding.

My thinking was to switch between native types and full types by treatPhpDocTypesAsCertain

like,
treatPhpDocTypesAsCertain: true
getNativeType() => native type
getType() => full types

treatPhpDocTypesAsCertain: false
getNativeType() = getType() => native type

I now understood that when a rule error is

- full type native type

1 no error no error

2 error no error

3 no error error

4 error error

purpose of treatPhpDocTypesAsCertain is to relax cases like 2.

I think the docs are clear on this 😊 "relaxes" https://phpstan.org/config-reference#treatphpdoctypesascertain

ondrejmirtes · 2022-10-08T21:01:35Z

tests/PHPStan/Rules/Variables/IssetRuleTest.php

@@ -123,89 +114,8 @@ public function testRule(): void
 		]);
 	}

-	public function testRuleWithoutTreatPhpDocTypesAsCertain(): void


Deleting a test is a no-go :) It should continue working as before.

ondrejmirtes · 2022-10-08T21:05:11Z

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 · 2022-10-08T23:07:35Z

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

rajyan · 2022-10-08T23:13:32Z

@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 · 2022-10-09T05:21:18Z

Yes 😊

ondrejmirtes · 2022-10-09T07:03:09Z

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.

cases from * phpstan#1802 (comment) * phpstan#1802 (comment) * phpstan/phpstan#6260

rajyan · 2022-10-09T07:47:04Z

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

ondrejmirtes · 2022-10-09T08:31:15Z

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 · 2022-10-09T11:41:47Z

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.

ondrejmirtes

I'm quite happy with this PR already. These few comments resolved + green build is all it takes for me to merge it 😊 And we can continue the work in further PRs, like moreSpecificNativeTypes, and fully implementing getNativeType for all exprs.

ondrejmirtes · 2022-10-09T21:28:06Z

src/Analyser/MutatingScope.php

@@ -506,6 +506,13 @@ public function hasVariableType(string $variableName): TrinaryLogic
 	/** @api */
 	public function getVariableType(string $variableName): Type
 	{
+		if (!$this->treatPhpDocTypesAsCertain) {


The behaviour shouldn't change here - MutatingScope shouldn't be interested in the treat option.

There could be getVariableNativeType method, but I don't think we need it.

ondrejmirtes · 2022-10-09T21:28:35Z

src/Analyser/MutatingScope.php

@@ -2147,7 +2113,7 @@ public function getNativeType(Expr $expr): Type
 			);
 		}

-		return $this->getType($expr);
+		return $this->doNotTreatPhpDocTypesAsCertain()->getType($expr);


This lind should just return new MixedType.

cases from * #1802 (comment) * #1802 (comment) * phpstan/phpstan#6260

…d anymore

Co-authored-by: Brad Miller <28307684+mad-briller@users.noreply.github.com>

…hpDocAsCertain()

…anyway

… be fixed)

rajyan · 2022-10-12T08:36:32Z

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 · 2022-10-21T13:50:32Z

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

ondrejmirtes · 2022-10-31T08:36:42Z

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

rajyan force-pushed the fix-native-types2 branch from 47f6b2e to 2d311e8 Compare October 8, 2022 14:45

rajyan changed the base branch from 1.8.x to 1.9.x October 8, 2022 15:08

rajyan marked this pull request as ready for review October 8, 2022 15:12

mad-briller reviewed Oct 8, 2022

View reviewed changes

src/Analyser/MutatingScope.php Outdated Show resolved Hide resolved

ondrejmirtes requested changes Oct 8, 2022

View reviewed changes

rajyan added a commit to rajyan/phpstan-src that referenced this pull request Oct 9, 2022

add native expression tests

e5f543c

cases from * phpstan#1802 (comment) * phpstan#1802 (comment) * phpstan/phpstan#6260

rajyan added a commit to rajyan/phpstan-src that referenced this pull request Oct 9, 2022

add native expression tests

2744c22

cases from * phpstan#1802 (comment) * phpstan#1802 (comment) * phpstan/phpstan#6260

rajyan force-pushed the fix-native-types2 branch from e5f543c to 2744c22 Compare October 9, 2022 07:39

rajyan added a commit to rajyan/phpstan-src that referenced this pull request Oct 9, 2022

add native expression tests

158319e

cases from * phpstan#1802 (comment) * phpstan#1802 (comment) * phpstan/phpstan#6260

rajyan force-pushed the fix-native-types2 branch from adea623 to 158319e Compare October 9, 2022 07:41

rajyan added a commit to rajyan/phpstan-src that referenced this pull request Oct 9, 2022

add native expression tests

32861a0

cases from * phpstan#1802 (comment) * phpstan#1802 (comment) * phpstan/phpstan#6260

rajyan force-pushed the fix-native-types2 branch from 158319e to 32861a0 Compare October 9, 2022 07:42

ondrejmirtes requested changes Oct 9, 2022

View reviewed changes

ondrejmirtes pushed a commit that referenced this pull request Oct 12, 2022

add native expression tests

3d8d0a5

cases from * #1802 (comment) * #1802 (comment) * phpstan/phpstan#6260

rajyan and others added 22 commits October 12, 2022 10:23

handle treatPhpDocTypesAsCertain in one place

cec5a9d

fix tests that are actually correct to fail

13236c6

fix ImpossibleCheckTypeHelper doNotTreatPhpDocTypesAsCertain

7cab0ec

fix and add tests for empty rule tests

0f55ae9

fix test

f683a00

add this type for native types

6d6a59f

fix static analysis warnings

ff6d807

make cs-fix

0a40b20

static analysis

097cb66

revert changes on testBug6974 because treatPhpDocAsCertain is not use…

9156e70

…d anymore

make cs-fx

65d2a7d

static analysis

fa0cfd2

static analysis

c461918

delete treatPhpDocTypesAsCertain because no need to explicitly pass it

0fd5d29

Update src/Analyser/MutatingScope.php

d4d0c9e

Co-authored-by: Brad Miller <28307684+mad-briller@users.noreply.github.com>

revert treatPhpDocTypesAsCertain in rules

57173b1

revert ImpossibleCheckTypeFunctionCallRuleTest constructor too

7d74b17

remove treatPhpDocTypesAsCertain from scope and deprecate doNotTreatP…

378627b

…hpDocAsCertain()

TypeInferenceTestCase - expected type is meant to be a constant type …

8936091

…anyway

Move getNativeType()

6a6406c

Mirror getType and getNativeType

8f7bbf7

MutatingScope - fix $this native type in a few places (more need to…

83c207f

… be fixed)

ondrejmirtes force-pushed the fix-native-types2 branch from 05aae3e to 83c207f Compare October 12, 2022 08:25

This was referenced Oct 16, 2022

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

Closed

Native types todos phpstan/phpstan#8191

Closed

ondrejmirtes closed this Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix native types2 #1802

Fix native types2 #1802

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

herndlm commented Oct 8, 2022

rajyan commented Oct 8, 2022

rajyan commented Oct 8, 2022

ondrejmirtes commented Oct 8, 2022

ondrejmirtes commented Oct 8, 2022

ondrejmirtes Oct 8, 2022

rajyan Oct 8, 2022

ondrejmirtes Oct 9, 2022

rajyan Oct 9, 2022

ondrejmirtes Oct 9, 2022

ondrejmirtes Oct 8, 2022

ondrejmirtes commented Oct 8, 2022

rajyan commented Oct 8, 2022

rajyan commented Oct 8, 2022

ondrejmirtes commented Oct 9, 2022

ondrejmirtes commented Oct 9, 2022

rajyan commented Oct 9, 2022

ondrejmirtes commented Oct 9, 2022

ondrejmirtes commented Oct 9, 2022

ondrejmirtes left a comment

ondrejmirtes Oct 9, 2022

ondrejmirtes Oct 9, 2022

rajyan commented Oct 12, 2022

ondrejmirtes commented Oct 21, 2022

ondrejmirtes commented Oct 31, 2022

	if (!$this->treatPhpDocTypesAsCertain) {
	if (!array_key_exists(sprintf('$%s', $variableName), $this->nativeExpressionTypes)) {
	return new MixedType();
	}
	return $this->nativeExpressionTypes[sprintf('$%s', $variableName)];
	}

Fix native types2 #1802

Fix native types2 #1802

Conversation

rajyan commented Oct 8, 2022 • edited

rajyan commented Oct 8, 2022 • edited

rajyan commented Oct 8, 2022 • edited

rajyan commented Oct 8, 2022 • edited

rajyan commented Oct 8, 2022 • edited

herndlm commented Oct 8, 2022

rajyan commented Oct 8, 2022

rajyan commented Oct 8, 2022

ondrejmirtes commented Oct 8, 2022

ondrejmirtes commented Oct 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Oct 8, 2022

rajyan commented Oct 8, 2022

rajyan commented Oct 8, 2022

ondrejmirtes commented Oct 9, 2022

ondrejmirtes commented Oct 9, 2022

rajyan commented Oct 9, 2022

ondrejmirtes commented Oct 9, 2022

Validate inline @var

Redesign rule levels

ondrejmirtes commented Oct 9, 2022

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajyan commented Oct 12, 2022

ondrejmirtes commented Oct 21, 2022

ondrejmirtes commented Oct 31, 2022

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

rajyan commented Oct 8, 2022 •

edited

Validate inline `@var`