Skip to content

[WIP] Add basic support for PHP 8 union types #4505

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
Nov 8, 2020

Conversation

wouterj
Copy link
Contributor

@wouterj wouterj commented Nov 7, 2020

I noticed that running Psalm on PHP 8 results in an error in my build:

Uncaught Error: Call to undefined method ReflectionUnionType::getName() in /home/runner/work/WouterJEloquentBundle/WouterJEloquentBundle/vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Reflection.php:415

The patch in this PR fixes the build, but more work needs to be done. (I'm also unsure about the policy of this package to unstable PHP versions)

I'm very very new with the Psalm codebase, so I'm unsure how to continue. I'm happy to finish this PR if there is some guidance, but also please feel free to take over this PR if that's easier for you :) A list of things I'm stuck with atm:

  • Psalm doesn't yet know about the ReflectionUnionType class, so the analysis errors. I found stubs/Php80.php and dictionaries/CallMap_80_deta.php. Am I supposed this add that class somewhere? (I've tried doing so, but it didn't seem to do much) For reference, this is the ReflectionUnionType structure: https://wiki.php.net/rfc/union_types_v2#reflection
  • Psalm incorrectly thinks getName() is part of ReflectionType, it's part of ReflectionNamedType instead. I couldn't find a way to tell Psalm this.
  • I also get another Psalm error with this patch: Could not verify return type 'Psalm\Type\Union' for Psalm\Internal\Codebase\Reflection::getPsalmTypeFromReflectionType I'm unsure why that is, as this method is always returning Type\Union as far as I can see.
  • This surely needs a test. I couldn't find tests directly for Reflection or php8. Can you guide my to which test class I should add a new test to cover this?

@muglug
Copy link
Collaborator

muglug commented Nov 8, 2020

I added a stub for ReflectionUnionType in 114440c

@muglug muglug merged commit 5831828 into vimeo:master Nov 8, 2020
@muglug
Copy link
Collaborator

muglug commented Nov 8, 2020

Thanks, this is good, I'll fix any remaining errors in master.

@wouterj wouterj deleted the php8/union-type-support branch November 8, 2020 22:30
@wouterj
Copy link
Contributor Author

wouterj commented Nov 8, 2020

Thank you. I can confirm this has fixed the "bug" in my build: https://github.com/wouterj/WouterJEloquentBundle/runs/1371434654

Btw, I just noticed that getTypes() returns list<ReflectionType>, instead of the typehinted ReflectionNamedType in the array_map function. Maybe the code needs to be adapted to recursively transform either ReflectionNamedType or ReflectionUnionType to a parseable string?

danog pushed a commit to danog/psalm that referenced this pull request Jan 29, 2021

Verified

This commit was signed with the committer’s verified signature. The key has expired.
danog Daniil Gentili
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.

None yet

2 participants