brick / date-time

Date and time library for PHP
MIT License
337 stars 30 forks source link

Add Psalm immutability annotations #32

Open nederdirk opened 3 years ago

nederdirk commented 3 years ago

Hi, I just found out about this library by searching for JSR310-inspired libraries for PHP.

This patch helps to ensure that classes marked as immutable will actually be immutable^1. Downstream projects using both brick/date-time and Psalm will have the benefit of allowing for more thorough analysis.

nederdirk commented 3 years ago

By the way, the psalm CI job rightly fails, because

  1. there are some calls to *::now() (which can never be pure, since it can, and is usually, tied to the wall clock) in immutable classes.
  2. The DateTimeParseResult seems to depend on its state mutating, I am not knowledgable enough about brick/date-time to figure out a way to improve this class

These problems can either be fixed by adding them into a psalm-baseline file, or by adding @psalm-suppress docblock lines in strategic places.

BenMorel commented 3 years ago

Hi, thanks for your PR. I would love to annotate immutable classes as such, however it looks like your attempt outlined a number of issues that may prevent us from doing so:

nederdirk commented 3 years ago

Hey @BenMorel, I just pushed and updated (and rebased on top of current master). As you can see in https://github.com/brick/date-time/pull/32/checks?check_run_id=2397952389, it's only the DateTimeParseResult that taints the immutable/mutation-free annotations. Could you elaborate a bit on why the stack-based design is necessary for that class? From reading the other code in this package, that doesn't seem to be necessary, and I think DateTimeParseResult could be refactored to be properly immutable, while retaining the interface, or by creating a new immutable interface ($dateTimeParseResult->withField($name, $value), $dataTimeParseResult->getFieldIterator($name)) and deprecating the current functions.

BenMorel commented 3 years ago

Hi, thanks for round 2!

I'm not sure to keep DateTimeParseResult as is TBH, I simply inherited it from the Java version. I'm not particularly happy with this mechanism and may replace it entirely at some point.

I'm wondering though: how is it a problem to call getters on a mutable class from a factory method of an immutable class?