memoizeMethodCallResults: Why is the default false?

[jarstelfox]

Questions

• Why is memoizeMethodCallResults defaulted to false?
  • Should memoizeMethodCallResults be defaulted to true?

Take a look at the examples below. The behavior seems incorrect by default, at least to me. I am trying to describe an assertion, but it does not work by default.

Background

See the examples here:

Problem: psalm.dev/r/f5483fbbfa
Fixed: psalm.dev/r/46b7ec02d6

Only difference:
memoizeMethodCallResults="true"
vs
memoizeMethodCallResults="false"

The docs say:

Please note that the example above only works if you enable method call memoization in the config file or annotate the class as immutable.

psalm.dev/docs/annotating_code/adding_assertions/#asserting-return-values-of-methods

Code History

It looks like it was added it back in very early psalm (v1.1.5) and it was defaulted to false. Sadly, there is no description for why it is false. Same story with the docs.

I suspect the goal for the maintainers is to respect sem-ver. Thus, memoizeMethodCallResults was not forced on existing projects.

[weirdan]

There are no guarantees that a method will return the same value every time, yet memoizeMethodCallResults="true" implies just that, for all methods. Therefore this setting is risky at best, and should not be enabled by default. Perhaps we should deprecate it and remove it in the next major version.

In this case, the right thing to do is annotate the method as mutation-free: https://psalm.dev/r/189bf1c362. This way Psalm will actually validate that the method does not change any state.

[jarstelfox]

Thanks for the quick reply! I was missing the connection with having a mutation free context. That explains a lot.

There are no guarantees that a method will return the same value every time, yet memoizeMethodCallResults="true" implies just that, for all methods

Yikes. I do agree that it may be worth removing. This seems like a huge foot gun.

I also think some small tweaks to the docs might help clarify that psalm is trying to ensure that the return value is only from the properties of the object as mutation free is implying.

I can take a stab at depreciating it and updating docs if that is helpful

[jarstelfox]

One other note:

It looks like Psalm's flow-sensitive type analysis can not detect the assertion outside of the scope: https://psalm.dev/r/6199d52678

It would be nice to be able to use object properties and methods in a @return: https://psalm.dev/r/2c9ad23657