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

Allow using Money, Currency and Context in Psalm's immutable contexts #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

someniatko
Copy link

@someniatko someniatko commented May 22, 2023

The motivation of this PR is the inability to use the value objects this lib provides in projects which use Psalm as a static analyzer, in its "immutable" contexts.

Added @psalm-immutable and @psalm-pure annotations to appropriate classes and static methods.

I took some shortcuts: for example, for the places where \NumberFormatter is used, psalm-suppress annotations was required.

Also, unfortunately, Currency::of() is using ISOCurrencyProvider which is heavily impure internally, therefore all static methods using Currency::of() were not marked as pure. Even more unfortunate is that one of the Money's non-static method is using Currency::of() and this place had to be marked with a @psalm-suppress. I am not sure what to do about this.

some shortcuts were taken - for example, for the places where \NumberFormatter is used, @psalm-suppress annotations was required

also, unfortunately, Currency::of() is using ISOCurrencyProvider which is heavily impure internally, therefore all static methods using Currency::of() were not marked as pure.
@someniatko
Copy link
Author

someniatko commented May 22, 2023

The ISOCurrencyProvider is of course just providing the information hardcoded into the lib, but uses optimization trickery which is "impure". Maybe marking the two usages of this provider in the Currency::of() and Currency::ofCountry() as @psalm-suppress ImpureMethodCall and marking the methods themselves as @psalm-pure is an okay-ish tradeoff?

From the Psalm's point of view it would be ideal to force the client of the library to call the provider explicitly (basically, treating the provider as a sort of a repository), and/or the other classes should not accept currency codes and automatically transform them into Currency instances from the provider, instead only take the Currency instance, but this arguably is a worse developer experience.

@someniatko someniatko marked this pull request as ready for review May 24, 2023 19:22
@someniatko
Copy link
Author

Before you merge it, what do you think about this problem:

The ISOCurrencyProvider is of course just providing the information hardcoded into the lib, but uses optimization trickery which is "impure". Maybe marking the two usages of this provider in the Currency::of() and Currency::ofCountry() as @psalm-suppress ImpureMethodCall and marking the methods themselves as @psalm-pure is an okay-ish tradeoff?

@BenMorel
Copy link
Member

The ISOCurrencyProvider is of course just providing the information hardcoded into the lib, but uses optimization trickery which is "impure". Maybe marking the two usages of this provider in the Currency::of() and Currency::ofCountry() as @psalm-suppress ImpureMethodCall and marking the methods themselves as @psalm-pure is an okay-ish tradeoff?

I guess marking Currency::of() as pure and muting Psalm here is OK.
As I understand it, even if its implementation is not pure, this method still exhibits a pure behaviour?

@someniatko
Copy link
Author

I guess marking Currency::of() as pure and muting Psalm here is OK. As I understand it, even if its implementation is not pure, this method still exhibits a pure behaviour?

I think yes, since the source data used by the ISOCurrencyProvider is used as immutable, but just lazy-loaded.

I will add @psalm-pure annotation then.

@@ -769,6 +789,7 @@ public function convertedTo(
*/
public function formatWith(\NumberFormatter $formatter) : string
{
/** @psalm-suppress ImpureMethodCall */
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to suppress this?

As I understand it, this method is not pure as you could pass the same NumberFormatter instance twice but configured differently, and the method would return different results?

Copy link
Author

Choose a reason for hiding this comment

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

this is bad news, because formatWith() will prevent Money from being a @psalm-immutable object otherwise :(

/**
* @psalm-suppress ImpureMethodCall
* FIXME: remove the suppression after BigInteger::gcdMultiple() is marked @psalm-pure
*/
Copy link
Member

Choose a reason for hiding this comment

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

What prevents BigInteger::gcdMultiple() from being marked as pure?

Copy link
Author

@someniatko someniatko Sep 28, 2023

Choose a reason for hiding this comment

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

this is another library, hence out of scope of this PR

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