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 ConstantArrayType::isCallable #3637

Open
wants to merge 6 commits into
base: 1.12.x
Choose a base branch
from

Conversation

VincentLanglet
Copy link
Contributor

I basically copied the logic findTypeAndMethodNames with the following changes:

  • I kept the methods with no as result of $type->hasMethod($method->getValue())
  • I ended up with an TrinaryLogic::extremIdentity

Closes phpstan/phpstan#12063

if (!$phpVersion->supportsCallableInstanceMethods()) {
$methodReflection = $type->getMethod($method->getValue(), new OutOfClassScope());
if ($classOrObject->isString()->yes() && !$methodReflection->isStatic()) {
$has = $has->and(TrinaryLogic::createNo());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In findTypeAndMethodNames there is a continue; I don't want to.

Copy link
Member

Choose a reason for hiding this comment

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

Why not $has = TrinaryLogic::createNo()?

src/Type/Constant/ConstantArrayType.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Contributor Author

@VincentLanglet VincentLanglet marked this pull request as ready for review November 16, 2024 13:02
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

if (!$phpVersion->supportsCallableInstanceMethods()) {
$methodReflection = $type->getMethod($method->getValue(), new OutOfClassScope());
if ($classOrObject->isString()->yes() && !$methodReflection->isStatic()) {
$has = $has->and(TrinaryLogic::createNo());
Copy link
Member

Choose a reason for hiding this comment

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

Why not $has = TrinaryLogic::createNo()?

@VincentLanglet
Copy link
Contributor Author

Why not $has = TrinaryLogic::createNo()?

Indeed ; simplified.

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.

The implementation if isCallable() needs to be synced with getCallableParametersAcceptors otherwise there are going to be bugs. That's why currently both methods use $this->findTypeAndMethodNames().

@VincentLanglet
Copy link
Contributor Author

The implementation if isCallable() needs to be synced with getCallableParametersAcceptors otherwise there are going to be bugs. That's why currently both methods use $this->findTypeAndMethodNames().

I provided another solution in 8993085

I can modify findTypeAndMethodNames directly, but that means that I need to allow calling

ConstantArrayTypeAndMethod::createConcrete($type, $method->getValue(), TrinaryLogic::createNo());

which impact the usage in

  • UnusedPrivateMethodRule
  • getCallableParametersAcceptors

So I dunno if:

  • ConstantArrayTypeAndMethod::createConcrete($type, $method->getValue(), TrinaryLogic::createNo()); would make sens
  • I need to filter out those value in UnusedPrivateMethodRule and getCallableParametersAcceptors (and add a boolean in findTypeAndMethodNames to filter out automatically)

@ondrejmirtes
Copy link
Member

I don't understand why we'd need to create ConstantArrayTypeAndMethod::createConcrete($type, $method->getValue(), TrinaryLogic::createNo()).

I'm sorry, I don't like the current state of this PR enough to merge it. I'd have to look into it myself.

@VincentLanglet
Copy link
Contributor Author

I don't understand why we'd need to create ConstantArrayTypeAndMethod::createConcrete($type, $method->getValue(), TrinaryLogic::createNo()).

Currently when we call findTypeAndMethodNames on a constantArray:

array{View, 'existingMethod'|'nonExistingMethod'}

the logic done is

  • View::existingMethod => exists, we call createConcrete on View, existingMethod, TrinaryLogic::createYes().
  • View::nonExistingMethod => doesn't exists, we skip this

Then the array returned is a single ConstantArrayTypeAndMethod so checking all the TrinaryLogic gives a Yes, and we think it's always a callable.

We need a way to track that some values were skipped.

I'm sorry, I don't like the current state of this PR enough to merge it. I'd have to look into it myself.

The other idea I have would be to check that the count of $methods->getConstantStrings() is the same than the count of the result of findTypeAndMethodNames to be sure no value were skipped (because they are TrinaryLogic::createNo()).

  • A count of 0 => No
  • A different count => Maybe
  • A same count => Keep the existing logic

@VincentLanglet
Copy link
Contributor Author

@ondrejmirtes I repushed the changes with my last idea (based on the count) ; you might prefer it.

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.

3 participants