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

Only mark getMessage as mutation free #3960

Closed
wants to merge 1 commit into from
Closed

Conversation

greg0ire
Copy link
Member

Q A
Type improvement
BC Break no
Fixed issues

Summary

We do not know if children classes do or do not mutate state.
This reverts commit 5a61341.
This translates into many issues on master: having to mark much more exceptions as immutable, one of which actually isn't detected as such.

See #3954 (comment)

We do not know if children classes do or do not mutate state.
This reverts commit 5a61341.
@morozov
Copy link
Member

morozov commented Apr 17, 2020

This translates into many issues on master: having to mark much more exceptions as immutable […]

I don't think this is something we should be avoiding.

[…] one of which actually isn't detected as such.

I think this is (or may be) because an interface method marked as immutable doesn't guarantee that its implementors are immutable as well. Maybe there is a Psalm annotation that forces the implementors to respect the parent's constraints, I don't know.

I'd rather suppress the error(s) that don't look legit than make annotation changes that don't look legit either. Exception classes must be immutable by design.

@greg0ire
Copy link
Member Author

greg0ire commented Apr 18, 2020

Okay.

  1. I know why there is an issue, and I fixed it: Mark throwable methods as pure vimeo/psalm#3171
  2. It should work in the CI now, but, not when developing because we are using master in the CI, and I think that's bad (just got bitten by it in sql-formatter because there is a bug with array_keys in master, that I have yet to report, but that you can witness here: https://github.com/doctrine/sql-formatter/pull/43/checks?check_run_id=597345249#step:4:76).
  3. There are exceptions in 2.10.x that I did not mark as immutable

Here is the course of action I feel would be best:

  1. Stabilize things by merging Stop relying on the master version of Psalm #3961
  2. Get Psalm on all branches including master by merging this PR
  3. Wait for a release of Psalm that has my fix and does not have the array_keys bug (I don't know if we actually suffer from it here, but chances are high).
  4. Revert this PR and mark all exceptions as immutable, and let that flow up into master.

@morozov
Copy link
Member

morozov commented Apr 18, 2020

Get Psalm on all branches including master by merging this PR

Thank you for the research @greg0ire. I'm still not sure why we need to revert valid annotations instead of suppressing invalid errors. Another approach might be to fork a stable Psalm branch that doesn't have the array_keys() bug and apply your fixes on top. We could use it in Composer and on CI before your fixes are released.

@greg0ire
Copy link
Member Author

I think the array_keys bug is gone from master now (I did another contribution and did not observe it). I'm going to add more immutable annotations to 2.10.x

@greg0ire greg0ire closed this Apr 18, 2020
@greg0ire greg0ire deleted the more-precise-immutable branch April 18, 2020 16:48
@morozov morozov removed their request for review October 23, 2020 17:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants