Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/native type specification #1372

Merged
merged 2 commits into from
Jun 1, 2022

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented May 31, 2022

There are lot of specifyExpressionType that is not correct to use type as native type.

return $this->specifyExpressionType(
$expr->var,
$this->getType($expr->var)->unsetOffset($this->getType($expr->dim)),
)->invalidateExpression(

$scope = $this->specifyExpressionType(
$expr->var,
TypeCombinator::union(...$setArrays),
);

This PR changes to specify native types only if specified separately.

@ondrejmirtes
Copy link
Member

Makes sense to me 👍 Ping me please once you mark it as ready.

@rajyan
Copy link
Contributor Author

rajyan commented Jun 1, 2022

@ondrejmirtes
I think I can mark this ready and continue refactoring in a different PR.
What I'm wondering is that these lines of changes (specifying same type with phpdoc type and native type to keep the current behavior)
https://github.com/phpstan/phpstan-src/pull/1372/files#diff-0c3f50d118357d9cb6d6f4d0eade75b83797d57056ff3b9c58ec881a13eaa6feL1806-R1922
is not always correct? and we have to create native type to specify separately every time we call specifyExpressionType

What I'm thinking right now is that adding a bool parameter to getType with a default value of $treatPhpDocTypesAsCertain (is this a BC break?) which can switch between phpdoc types and native types with just by branching out in one place. (Maybe $nativeExpressionTypes can be deleted?)

@rajyan rajyan marked this pull request as ready for review June 1, 2022 00:24
@ondrejmirtes
Copy link
Member

My idea is that there's Scope::getType() and Scope::getNativeType(). In the future it'd be great if Scope::getNativeType() answered truthfully even about expressions like MethodCall.

Let's say we have:

/** @return positive-int */
public function doFoo(): int
{
}

Scope::getType(MethodCall) would answer positive-int, but Scope::getNativeType(MethodCall) would answer just int.

It doesn't matter much to me if it's done via some boolean parameters, or by code duplication. I'm more fan of code duplication because it's more easier to maintain in the future :)

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

@ondrejmirtes
Copy link
Member

Also - it's not exactly the same thing, but today you can call $scope->doNotTreatPhpDocTypesAsCertain()->getType(...) - it's used in a few places.

@ondrejmirtes
Copy link
Member

Also, once Scope::getNativeType() truthfully answers to all expressions, we can do a really cool new rule:

/** @var string $foo */
$foo = $this->returnsInt(); // has int in native return type

We can mark this @var as always wrong :)

@ondrejmirtes ondrejmirtes merged commit 5719046 into phpstan:1.7.x Jun 1, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@rajyan
Copy link
Contributor Author

rajyan commented Jun 2, 2022

Also - it's not exactly the same thing, but today you can call $scope->doNotTreatPhpDocTypesAsCertain()->getType(...) - it's used in a few places.

Oh, I was not aware of this. Thanks!

@rajyan rajyan deleted the fix/native-type-specification branch June 2, 2022 00:19
@rajyan
Copy link
Contributor Author

rajyan commented Jun 2, 2022

@ondrejmirtes

Thank you very much for your comments, I'll take them into account 👍

Let's say we have:
/** @return positive-int */
public function doFoo(): int
{
}
Scope::getType(MethodCall) would answer positive-int, but Scope::getNativeType(MethodCall) would answer just int

I'm a little confused with this comment, maybe not understanding what "native type" is correctly.
I was thinking that "native type" should not refer to the PHPDocs.

@ondrejmirtes
Copy link
Member

Yes, I don't see any contradiction in that :) "Native type" is a type we can rely on, it doesn't take PHPDocs into account.

@rajyan rajyan mentioned this pull request Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants