Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow omitting @param type #127

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
ondrejmirtes merged 6 commits into phpstan:1.6.x from rvanvelzen:optional-param-type
Jun 8, 2022

Conversation

Copy link
Contributor

@rvanvelzen rvanvelzen commented Jun 8, 2022
edited
Loading

Fixes #77

Copy link
Member

We should be able to do this without breaking BC. Because @param without a type is essentially useless, I feel like we shouldn't mix it with other useful @param tags that have a type.

So my suggestion is:

  1. Do not make $type in src/Ast/PhpDoc/ParamTagValueNode.php nullable.
  2. Create a separate TypelessParamTagValueNode that can still carry $description.
  3. PhpDocNode::getParamTagValues() WIL NOT return TypelessParamTagValueNode, it needs to be filtered out.
  4. Add PhpDocNode::getTypelessParamTagValues().
  5. I'll release that in minor phpdoc-parser version.
  6. Add some tests to phpstan-src that demonstrate this is no longer a parse error, and potentially get and make use of some data carried by getTypelessParamTagValues() (but I don't think we use parameter description anywhere).

Thank you!

hrach reacted with thumbs up emoji

@rvanvelzen rvanvelzen marked this pull request as ready for review June 8, 2022 12:24
];

yield [
'invalid without type and description',
Copy link
Member

@ondrejmirtes ondrejmirtes Jun 8, 2022

Choose a reason for hiding this comment

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

I think the conclusion of #77 is that /** @param $foo */ shouldn't be a parse error either. The description can be an empty string, and if someone wants, they can have a rule to mark such tag as useless, but it would no longer be an objective bug to have such PHPDoc in the code...

So the grammar description + parser should be update to allow this tag.

@ondrejmirtes ondrejmirtes merged commit 9d45205 into phpstan:1.6.x Jun 8, 2022
Copy link
Member

Perfect, thank you!

Copy link

wouterj commented Jun 8, 2022

Thank you Ondřej and Richard!

Copy link

Wirone commented Jun 10, 2022

This one made our baseline files thinner by ~3k entries 🙈 I am not 100% convinced that this is good behaviour but I understand the reasoning behind it. Cheers! 🍻

Copy link

Is it possible to revert the old behavior by configuration flag?

Copy link
Contributor Author

Why?

Copy link

It was useful for us to detect this errors because they are usually show old code with bad type hinting (in our code base). We check and fix them step by step. So we need to stay at 1.7.11 version before will fix them all.

Copy link
Member

@p-golovin As said earlier in this thread, you'll get the same errors on level 6 (missing parameter typehint).

Copy link

@ondrejmirtes Thank you. We'll try to raise level to 6 (now we make fixes on level 3).

Copy link

Wirone commented Jun 16, 2022
edited
Loading

As said earlier in this thread, you'll get the same errors on level 6 (missing parameter typehint).

Really? We're on level 6 and upgrading caused many "ignored ... not found", so we had to regenerate baselines and maaaaaaany ignores were removed. 🤔

EDIT: remember, do not comment when too tired to understand 😉

Copy link

wouterj commented Jun 16, 2022

Wirone reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

@param type can be omitted

AltStyle によって変換されたページ (->オリジナル) /