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

Allow omitting @param type #127

Merged
merged 6 commits into from Jun 8, 2022

Conversation

rvanvelzen
Copy link
Contributor

@rvanvelzen rvanvelzen commented Jun 8, 2022

Fixes #77

@ondrejmirtes
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!

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

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

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
@ondrejmirtes
Copy link
Member

Perfect, thank you!

@wouterj
Copy link

wouterj commented Jun 8, 2022

Thank you Ondřej and Richard!

@Wirone
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! 🍻

@p-golovin
Copy link

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

@rvanvelzen
Copy link
Contributor Author

Why?

@p-golovin
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.

@ondrejmirtes
Copy link
Member

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

@p-golovin
Copy link

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

@Wirone
Copy link

Wirone commented Jun 16, 2022

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 😉

@wouterj
Copy link

wouterj commented Jun 16, 2022

see #77 (comment)

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.

@param type can be omitted
5 participants