Skip to content

Drop psalm #18340

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

Merged
merged 1 commit into from
Apr 16, 2025
Merged

Drop psalm #18340

merged 1 commit into from
Apr 16, 2025

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Apr 15, 2025

No description provided.

@ADmad ADmad added this to the 5.2.2 milestone Apr 15, 2025
@@ -74,7 +74,7 @@ class Cache
* class names.
*
* @var array<string, string>
* @psalm-var array<string, class-string>
* @phpstan-var array<string, class-string>
Copy link
Contributor

Choose a reason for hiding this comment

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

with stuff like that can't it be merged into the normal @var phpdoc block?
I believe @var and @return phpdoc definitions support stuff like class-string and generics

Copy link
Contributor

@LordSimal LordSimal Apr 15, 2025

Choose a reason for hiding this comment

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

at least phpstan understands it. don't know how our api docs generator handles it though

Copy link
Member

Choose a reason for hiding this comment

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

We would have to check how far also IDE are in understanding the more stan based annotations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, thats exactly how you dont do it :) Ever.
It means you lose all capability of your IDE to give you feedback at real time.
Especially since it is easy enough to add them prefixed on top for STAN tools to also work, and basically have the perfect tradeoff.

Copy link
Member Author

@ADmad ADmad Apr 15, 2025

Choose a reason for hiding this comment

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

with stuff like that can't it be merged into the normal @var phpdoc block?

Yes it can be merged. These annotations most likely predate the support for this form in phpstorm. Hence the prefix.

I ran a simple replace of @psalm- to @phpstan-. Not going to review each annotation manually :) We can clean them up as and when anyone is working on that part of the code.

Copy link
Contributor

@LordSimal LordSimal Apr 15, 2025

Choose a reason for hiding this comment

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

At least with the free PHP-Tools VS Code extension it does autocompletion correctly for class-string definitions

image

I can't tell how other LSP's handle that (like Neovim's) but PHPStorm definitely understands this.

Copy link
Member

Choose a reason for hiding this comment

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

class-string is fine
I think the other examples you showed are bit too crazy :)

@dereuromark dereuromark merged commit 32f82a6 into 5.x Apr 16, 2025
13 checks passed
@dereuromark dereuromark deleted the drop-psalm branch April 16, 2025 02:50
@mirko-pagliai
Copy link
Contributor

mirko-pagliai commented Apr 18, 2025

Hi guys. Just for my personal knowledge, is it possible to know what led you to this choice?

@LordSimal
Copy link
Contributor

LordSimal commented Apr 18, 2025

  • if you run psalm with --alter you get syntax errors like public function getDisplayField(): array|string|string|null
  • Psalm 6.10 introduced a very questionable sniff related to MissingOverrideAttribute which caused a lot of unnecessary changes
  • Psalm does not support modern PHP 8.4 features like property hooks
  • The gap between phpstan and psalm has narrowed down over the years (in the past psalm reported valid errors which phpstan did not, but phpstan caught up pretty well)
  • maintaining 2 separate static analysis tools over the core and all our plugins can be quite cumbersome

@ADmad
Copy link
Member Author

ADmad commented Apr 18, 2025

Psalm does not support modern PHP 8.4 features like vimeo/psalm#11208

Addresing it isn't a priority for them either vimeo/psalm#11340 (comment).

There's also issues like this psalm/phar#20

@mirko-pagliai
Copy link
Contributor

Thank you so much for the explanations. I also learned something new from this community today :D

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.

4 participants