Skip to content

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented Nov 17, 2022

This pull request covers
phpstan/phpstan#8360

  • Solve the infinite loop problem
  • Make createConditionalExpressions work for all expressions

improves result of phpstan/phpstan#4173 but not completely fixed yet

@rajyan rajyan force-pushed the fix-create-conditional-expressions branch from d17c4c1 to 5b9cf52 Compare November 17, 2022 12:41
@rajyan
Copy link
Contributor Author

rajyan commented Nov 17, 2022

Issue bot seems not working well. Looks like a cache problem?
This change should notify issue-7705 and issue-4173 changes like in
#1950 (comment)

@staabm
Copy link
Contributor

staabm commented Nov 17, 2022

Issue bot seems not working well. Looks like a cache problem?
This change should notify issue-7705 and issue-4173 changes like in

hmm I have the impression it works:

https://github.com/phpstan/phpstan-src/actions/runs/3488347704

grafik

@rajyan
Copy link
Contributor Author

rajyan commented Nov 17, 2022

Oh, thank you for noticing! Not sure what I was looking at...

@rajyan
Copy link
Contributor Author

rajyan commented Nov 17, 2022

@ondrejmirtes
I think the result of issue bot is wrong when there is a infinite loop. issue-7705 was causing an infinite loop before dd478c0 , but the issue bot was saying no error.

@herndlm
Copy link
Contributor

herndlm commented Nov 17, 2022

Loops are generalised and skipped after a couple of iterations AFAIK. Could that be related?

@rajyan
Copy link
Contributor Author

rajyan commented Nov 17, 2022

Not that "loop" I think.
This change causes something like
Resolve conditionalExpressionType of $foo => Need to getType of contional $isFoo || $isBar => Resolve type of $isFoo || $isBar => getType of BooleanOr uses filterByFalsyScope => In filterByFalsyScope conditionalExpressionType of $foo is evaluate again...

@rajyan rajyan force-pushed the fix-create-conditional-expressions branch from fa64209 to e3c84ef Compare November 17, 2022 13:57
@herndlm
Copy link
Contributor

herndlm commented Nov 17, 2022

Ah sorry, yeah, completely wrong loop I was thinking about.. xD

@rajyan rajyan changed the title Fix create conditional expressions Improve create conditional expressions to handle all expressions Nov 18, 2022
@rajyan rajyan force-pushed the fix-create-conditional-expressions branch from 472df61 to b2bea90 Compare November 18, 2022 04:28
@rajyan rajyan marked this pull request as ready for review November 18, 2022 04:38
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit e870778 into phpstan:1.9.x Nov 18, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@rajyan rajyan deleted the fix-create-conditional-expressions branch November 18, 2022 22:29
return $field;
}

if (count($row) === 2) {

Choose a reason for hiding this comment

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

This PR seems to cache result of function result 1:1 and retrive the result from cache later. I observed this in phpstan v 1.9.2 (the latest rel).

Is that desired? For count() it is legit and thank you. However for expression like TraitUtil::hasTrait($object, DebugTrait::class) (a custom function) how does php know/guarantee the result will be the same, does it reason it based on the TraitUtil::hasTrait impl or just guess it when the passed args are the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is intended

how does php know/guarantee the result will be the same

When

  • TraitUtil::hasTrait is not marked impure
  • The exact same TraitUtil::hasTrait($object, DebugTrait::class) is called
  • $object is not invalidated

just guess it when the passed args are the same?

so it's something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, remembering function results was working before this PR already.
https://phpstan.org/blog/remembering-and-forgetting-returned-values
this PR made them work for more complex conditions too.

Choose a reason for hiding this comment

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

Thank you, I did not know user functions are "pure"/remembered by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

6 participants