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

Improve PHPDoc types support #4914

Merged
merged 1 commit into from
May 13, 2021

Conversation

julienfalque
Copy link
Member

@julienfalque julienfalque commented Apr 11, 2020

Related to #4913.

@julienfalque julienfalque marked this pull request as draft April 12, 2020 20:45
@julienfalque julienfalque changed the title Add support for PHPDoc constant types Improve PHPDoc types support Apr 12, 2020
@julienfalque
Copy link
Member Author

julienfalque commented Apr 12, 2020

I've worked a bit more on this and made several improvements. Now this PR adds support for:

  • spaces around |, e.g.: A | B
  • class constants, e.g. Foo::BAR_*
  • scalar constants, e.g. 'a' | 'b'
  • types containing a -, e.g. class-string
  • intersection types, e.g. A & B
  • object-like arrays, e.g.: array{a: bool, b?: string}

@julienfalque julienfalque marked this pull request as ready for review April 13, 2020 08:31
@Slamdunk
Copy link
Contributor

  • spaces around |, e.g.: A | B
  • class constants, e.g. Foo::BAR_*
  • scalar constants, e.g. 'a' | 'b'
  • types containing a -, e.g. class-string
  • intersection types, e.g. A & B
  • object-like arrays, e.g.: array{a: bool, b?: string}

I'm sorry @keradus to ping you, but v2.16.2 Yellow Bird kinda broke almost every project I'm aware of, since these notations are now widespread due to static analysis tools.

Hope to see a release with this bugfix as soon as possible 😢

@julienfalque
Copy link
Member Author

@Slamdunk Note that the actual fix for the unexpected removal of annotations is #4915. This PR will rather unlock enhancements in e.g. phpdoc_to_return_type.

@Slamdunk
Copy link
Contributor

Yup, sorry

SpacePossum
SpacePossum previously approved these changes Apr 15, 2020
@SpacePossum SpacePossum added the RTM Ready To Merge label Apr 15, 2020
@julienfalque julienfalque removed the RTM Ready To Merge label Apr 15, 2020
@julienfalque julienfalque force-pushed the phpdoc-psalm-types branch 2 times, most recently from 088f73c to dd1181b Compare April 16, 2020 06:52
@SpacePossum SpacePossum dismissed their stale review April 19, 2020 11:35

after gitter talk

@kubawerlos
Copy link
Contributor

@julienfalque can we add tests from #5049 here?

@julienfalque
Copy link
Member Author

after gitter talk

Didn't get a notification for this review being dismissed. What was the talk about?

@julienfalque can we add tests from #5049 here?

Sure thing. Are you allowed to push on my branch? Otherwise I'll cherry-pick your commit.

@SpacePossum
Copy link
Contributor

SpacePossum commented Oct 22, 2020

indeed, I don't see why the spacing would matter when extracting the type info

@julienfalque
Copy link
Member Author

Using whitespaces is not a widespread practice but it should not be complicated to add support for it. I'll try to do it shortly.

@SpacePossum
Copy link
Contributor

👍 it will saves the trouble of figuring out all the possible priority issues with fixers that clean up the spacing

@julienfalque
Copy link
Member Author

Test case added. Though whitespaces are not stripped from the result because the type is parsed as a whole.

@SpacePossum
Copy link
Contributor

Is the "type" " int " not exactly the same as "type" "int" ? Especially as the type " int " can never exist within PHP, same with class names that cannot contain pending or trialing spacing. I'm not sure why we would extract these differently but maybe I'm missing the use case here.

@julienfalque
Copy link
Member Author

Not sure what you mean. The regex just extracts the type definition as-is, without changing anything to it. Those whitespaces have no special meaning, they are not part of a type, e.g. there is no " int " but "int" + whitespaces.

Removing the whitespaces would prevent, e.g. a fixer to know the original syntax in order to change it.

@SpacePossum
Copy link
Contributor

@kubawerlos @julienfalque RTM than? can someone run this code on the some other code bases like SF/Twig/Doctrine for example to see if all goes well?

@SpacePossum
Copy link
Contributor

and one more request, can some look into https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/5095/files to see if the issue there is no longer present in this PR?

@kubawerlos
Copy link
Contributor

and one more request, can some look into https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/5095/files to see if the issue there is no longer present in this PR?

Can't we merge this and update the other one and we will all see?

@SpacePossum
Copy link
Contributor

I prefer to resolve both PR's here as 5095 hasn't got any comments what so ever and I don't want it to sit there forever

@julienfalque
Copy link
Member Author

@SpacePossum I tried applying changes from #5095 and all tests were green. I approved #5095, we can merge them separately IMO.

@keradus keradus changed the base branch from 2.15 to 2.16 November 20, 2020 15:24
@keradus keradus changed the base branch from 2.16 to master December 17, 2020 13:43
@coveralls
Copy link

coveralls commented Apr 30, 2021

Coverage Status

Coverage increased (+0.0009%) to 91.549% when pulling fd4a770 on julienfalque:phpdoc-psalm-types into 25f9db1 on FriendsOfPHP:2.19.

@keradus
Copy link
Member

keradus commented May 3, 2021

can you rebase, @julienfalque ?

@julienfalque
Copy link
Member Author

Done.

@keradus keradus changed the base branch from master to 2.19 May 3, 2021 22:30
@keradus keradus added this to the 2.19.1 milestone May 3, 2021
@keradus
Copy link
Member

keradus commented May 13, 2021

Thank you @julienfalque.

@keradus keradus merged commit f196ab0 into PHP-CS-Fixer:2.19 May 13, 2021
@julienfalque julienfalque deleted the phpdoc-psalm-types branch May 13, 2021 19:17
@Slamdunk
Copy link
Contributor

Slamdunk commented Aug 3, 2021

@julienfalque do you think this could be behind #5836?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants