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

Update nikic/php-parser to 5.0 #10567

Merged
merged 17 commits into from
Feb 4, 2024
Merged

Update nikic/php-parser to 5.0 #10567

merged 17 commits into from
Feb 4, 2024

Conversation

edsrzf
Copy link
Contributor

@edsrzf edsrzf commented Jan 18, 2024

I saw #10562, and I've also been playing around with this.

The main issues I encountered in this update were:

  • Removal of PhpParser\Node\Stmt\Throw. Throws are now always treated as expressions.
  • Doc comments are only associated with one node. This affects expression statements and expressions passed as arguments. I'm not super happy with my fixes for this inside ReflectorVisitor, so very open to suggestions.
  • Some node inheritance hierarchies have changed.

@edsrzf
Copy link
Contributor Author

edsrzf commented Jan 31, 2024

I've marked this as ready for review. I still have some doubts about the way I've solved a few of these issues, but am hopeful that feedback will help ensure that the best solution is the one that gets merged.

I've incorporated several changes upstream in the 5.x branch and rebased this PR, so the diff is a little smaller than it originally was. I'm not sure there's anything else I could reasonably pull out, but let me know if you see anything!

@edsrzf edsrzf marked this pull request as ready for review January 31, 2024 09:07
@@ -33,15 +33,12 @@
"felixfbecker/language-server-protocol": "^1.5.2",
"fidry/cpu-core-counter": "^0.4.1 || ^0.5.1 || ^1.0.0",
"netresearch/jsonmapper": "^1.0 || ^2.0 || ^3.0 || ^4.0",
"nikic/php-parser": "^4.16",
"nikic/php-parser": "^5.0.0",
Copy link

Choose a reason for hiding this comment

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

This is a dependency that Psalm shares with other tools like PHPUnit. In order to allow downstream projects to upgrade their dependencies gracefully, Psalm should produce a release first that is compatible with both major versions.

Suggested change
"nikic/php-parser": "^5.0.0",
"nikic/php-parser": "^4.18 || ^5.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to trying to create a version of this PR that would land on the 5.x branch that would work with both parser v4 and v5. I think the main thing would be going back to the deprecated node names (eg Encapsed vs Interpolated), which are still supported in v5 via class aliases.

The thing I'd worry about is additional complexity, maintenance, and testing involved in supporting two versions. The differences are non-trivial, as evidenced by this PR. PHPStan also looks like it's planning to jump to parser v5 on its 2.0 release, with its 1.x releases only supporting parser v4.

I'd like feedback from the maintainers before making any major changes to this PR or creating a new one.

@danog danog merged commit d916aa6 into vimeo:master Feb 4, 2024
40 of 42 checks passed
@danog
Copy link
Collaborator

danog commented Feb 4, 2024

Thank you for this PR.
I also initially intended to support both v4 and v5 in my php-parser upgrade PR, but then also noticed the non-triviality of the changes and the extra maintenance burden of supporting both versions, and pivoted towards v5-only support in my PR.

If you would want to provide a second PR to 5.x to support both v4 and v5, that would of course be appreciated, but personally I don't think dropping v4 support for Psalm v6 is that big of a deal (regardless, I would absolutely merge a PR adding support for it).

@weirdan
Copy link
Collaborator

weirdan commented Feb 4, 2024

@danog it appears it broke psalm.dev sandbox. All snippets return

Internal Psalm error on line 25 - /src/OnlineChecker.php: Call to undefined method PhpParser\ParserFactory::create()

@weirdan
Copy link
Collaborator

weirdan commented Feb 4, 2024

it broke psalm.dev sandbox

Fixed now.

@BenMorel
Copy link
Contributor

Will this be merged into 5.x?

@edsrzf edsrzf deleted the php-parser-5 branch February 17, 2024 23:21
@edsrzf
Copy link
Contributor Author

edsrzf commented Feb 17, 2024

I don't have any plans to backport this to 5.x, personally.

@Slamdunk
Copy link
Contributor

@edsrzf with no rush and no pressure, may I ask you if there's an expected release date for v6?

@edsrzf
Copy link
Contributor Author

edsrzf commented Feb 27, 2024

I'm not a maintainer, so it's not up to me, but I started a discussion here: #10701

@VincentLanglet
Copy link
Contributor

I tried to work on the Back port of this PR on 5.x.
#11035

I just have one error to fix, any help is welcomed.
https://github.com/vimeo/psalm/actions/runs/10094010337/job/27910940740?pr=11035

@VincentLanglet
Copy link
Contributor

PR on 5.x is ready #11035 open to review or tests on real project :)

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.

7 participants