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

\JanGregor\Prophecy\Reflection\ObjectProphecyMethodReflection::hasSideEffects() should be 'yes' for PHP >7.0 #112

Closed
func0der opened this issue Feb 5, 2020 · 7 comments · Fixed by #119
Assignees
Labels

Comments

@func0der
Copy link

func0der commented Feb 5, 2020

\JanGregor\Prophecy\Reflection\ObjectProphecyMethodReflection::hasSideEffects() returns no. As I found not documentation as for what \PHPStan\Reflection\MethodReflection::hasSideEffects() should really mark, I can only guess, that this marks methods that simply return stuff or work on copies of internal values.

As for \Prophecy\Prophecy\ObjectProphecy::__call() has side effects. I returns a new \Prophecy\Prophecy\MethodProphecy, which's __constructor() method changes the ObjectProphecy it was was called on as of PHP >7.0.
I tries to guess the return type of the method that is prophecied registers itself on the ObjectProphepcy in the \Prophecy\Prophecy\MethodProphecy::will() call.

I might be wrong here, because I really can not find ANY definition of sideEffect regarding a method, but as this not only creates a new object, but also changes the object it was created AND also changes the outcome of the test it is constructed in (changes return type to X instead of null for unregistered calls), it should not considered to be side effect free.

@localheinz localheinz mentioned this issue Feb 5, 2020
1 task
@localheinz
Copy link
Contributor

@func0der

To be honest, I have no idea!

Perhaps @ondrejmirtes can help us out here?

@localheinz localheinz self-assigned this Feb 5, 2020
@ondrejmirtes
Copy link
Contributor

Some internal methods from PHP are marked as having/not having side effects (https://github.com/phpstan/phpstan-src/blob/master/src/Reflection/SignatureMap/functionMetadata.php), so you can draw some inspiration from that.

How it currently manifests:

  1. All methods returning void always have side effects.
  2. Calling a method on a separate line with no side effects is marked as dead code (https://phpstan.org/r/c0f21464-a900-4c51-866e-fc0184fea782).
  3. Calling a method with side effects resets memorized state of other methods and properties (https://phpstan.org/r/5ee512dd-ecff-4d88-b0d2-1be03fb49dce vs. https://phpstan.org/r/80bbafed-e5f1-4bc8-a329-5a38a3975cfd).

If you don't want either of these things, return TrinaryLogic::maybe().

@localheinz
Copy link
Contributor

Thank you for the explanation, @ondrejmirtes!

@func0der
Copy link
Author

func0der commented Feb 5, 2020

@ondrejmirtes
Thank you for your clarification.
I am not sure that I am on board with 2.
I do not see how methods like this (https://phpstan.org/r/fca1b63c-5056-4bee-8317-51f357c2dee0) may be a problem.
Can you elaborate or point me to a place where I can read up on that?

@ondrejmirtes
Copy link
Contributor

@func0der What do you mean by "a problem"? When you're calling something that's not supposed to have a side effect (like a getter or a method on an immutable object), so the user is supposed to read the return value of the method, but they don't do it, then you get the error "on a separate line has no effect." because you cen simply remove the line.

@func0der
Copy link
Author

func0der commented Feb 6, 2020

Ah, sorry. I overlooked the immutable object part. My bad. :)

@localheinz Pretty sure it should be yes in this case, at least for PHP >7.0 :)
But maybe is good for now.

Thanks for the work.

@localheinz
Copy link
Contributor

@func0der

No problem!

Let me know if you run into any issues - I will be here to quickly turn things around when necessary!

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