Skip to content

Introduce class_implements dynamic return type #2023

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

Merged
merged 10 commits into from
Jan 11, 2023

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Nov 21, 2022

@VincentLanglet VincentLanglet marked this pull request as ready for review November 21, 2022 21:21
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@VincentLanglet
Copy link
Contributor Author

The failing build seems legit.
This assert https://github.com/doctrine/persistence/blob/3.1.x/src/Persistence/Mapping/RuntimeReflectionService.php#L44 won't be necessary thanks to the dynamic return type extension.

@VincentLanglet
Copy link
Contributor Author

I just checked the failing build for the integration builds and this is expected.

class_uses() or class_implements() never return false where they are used so a !== false check is now useless.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are situations the function will always return false. Which aren't covered here.

}

$objectOrClassType = $scope->getType($functionCall->getArgs()[0]->value);
$autoload = !isset($functionCall->getArgs()[1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$autoload should be a TrinaryLogic. So you should do (new ConstantBooleanType(true))->isSuperTypeOf($secondArg). Because you might have true (yes), bool or mixed (maybe), or false (no).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ?
If I'm not sure that true is passed, I cannot remove the false return type.
So bool, mixed or false (ie, maybe and no) are working the same way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test these scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests added

$autoload = !isset($functionCall->getArgs()[1])
|| $scope->getType($functionCall->getArgs()[1]->value)->equals(new ConstantBooleanType(true));

if ($objectOrClassType instanceof UnionType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to do instanceof *Type here. A single $this->canFunctionReturnFalse($objectOrClassType, $autoload) call should be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I just write

if ($this->canFunctionReturnFalse($objectOrClassType, $autoload)) {
     return null;
}

my tests are failing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I found a way to do it.

@VincentLanglet
Copy link
Contributor Author

Also there are situations the function will always return false. Which aren't covered here.

Do you have examples ? How can I know that the class is not loaded ?

@ondrejmirtes
Copy link
Member

@VincentLanglet A constant string that isn't a class.

@VincentLanglet
Copy link
Contributor Author

@VincentLanglet A constant string that isn't a class.

Done :)

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the array values should be class-string in all cases.

@VincentLanglet
Copy link
Contributor Author

I think the array values should be class-string in all cases.

I used TypeCombinator::remove to rely on the original definition:
https://github.com/phpstan/phpstan-src/blob/1.10.x/resources/functionMap.php#L951-L953

I suspect that class-uses return array<string> because it's a list of trait.
But maybe FooTrait::class is considered as a class-string too ; I didn't check.

@ondrejmirtes
Copy link
Member

For this purpose it is. You can use more specific trait-string and interface-string which currently mean the same thing but in the future they might be different.

@VincentLanglet
Copy link
Contributor Author

For this purpose it is. You can use more specific trait-string and interface-string which currently mean the same thing but in the future they might be different.

Thanks for the precision ; I did the changes

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression test please for 4335, based on the issue bot output.

Can you also please go through these failures and think about whether they're valid or not? Thanks.

  1. https://github.com/phpstan/phpstan-src/actions/runs/3891490930/jobs/6641853062
  2. https://github.com/phpstan/phpstan-src/actions/runs/3891490930/jobs/6641856141

@VincentLanglet
Copy link
Contributor Author

Regression test please for 4335, based on the issue bot output.

Added

Can you also please go through these failures and think about whether they're valid or not? Thanks.

  1. phpstan/phpstan-src/actions/runs/3891490930/jobs/6641853062

This is expected since the false check is not needed for PHPStan.
https://github.com/nextras/orm/blob/main/src/Entity/Reflection/MetadataParser.php#L138-L139
https://github.com/nextras/orm/blob/main/src/Entity/Reflection/MetadataParser.php#L153-L154
https://github.com/nextras/orm/blob/main/src/Entity/Reflection/MetadataParser.php#L422-L423

  1. phpstan/phpstan-src/actions/runs/3891490930/jobs/6641856141

This is expected too
https://github.com/doctrine/persistence/blob/3.1.x/src/Persistence/Mapping/RuntimeReflectionService.php#L38-L44
Since the class exists, the class_parents won't return false.

@ondrejmirtes ondrejmirtes merged commit 865a4ba into phpstan:1.9.x Jan 11, 2023
@ondrejmirtes
Copy link
Member

Thank you.

1 similar comment
@ondrejmirtes
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

class_implements can return false only under some circumstances
3 participants