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

@param type can be omitted #77

Closed
wouterj opened this issue Jun 30, 2021 · 16 comments · Fixed by #127
Closed

@param type can be omitted #77

wouterj opened this issue Jun 30, 2021 · 16 comments · Fixed by #127

Comments

@wouterj
Copy link

wouterj commented Jun 30, 2021

According to https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/param.html, the type may be omitted. This is especially useful in PHP 8 applications, which can almost fully typehint all arguments. The type in the PHPdoc in this case is redundant and may only lead to out-of-sync types.

PHPstorm and Psalm already appear to support this. However, this is not yet the case with PHPstan: https://phpstan.org/r/691a2424-e087-45a0-a9ce-774a57f887ec

(fyi, we're about to remove the types in Symfony, which is why I checked support in the popular static analysis libraries)

@orklah
Copy link

orklah commented Jun 30, 2021

This is actually a complaint in Psalm. It's due to a bug where Psalm doesn't even comprehend the line is a parameter tag when the type is missing (It shows when you ask Psalm to fix the code, it won't add the type, it will add an entire new @param line)

It was suggested that Psalm should emit an issue when encountering this syntax. The case where the type is in signature and we don't want to repeat it but we still want to add documentation is interesting though. Not sure what we want to make of that

@ondrejmirtes
Copy link
Member

When you don't have the type in @param, you can remove the whole tag. If you want to have @param just for description, you can write @param mixed $var description.

@param without type is worthless and often a mistake.

may only lead to out-of-sync types

No, PHPStan detects such situation.

I don't think it's useful to allow this, it's rightfully considered a PHPDoc syntax error.

@ondrejmirtes
Copy link
Member

One more point to consider is that people can use @param to narrow down types that can be expressed natively - for example native string can be complemented with @param 'foo'|'bar' $var in PHPDoc. So pointing out that @param $var is wrong is useful because the developer might realize they wanted to write something like the above instead.

@stof
Copy link

stof commented Jun 30, 2021

If you want to have @param just for description, you can write @param mixed $var description.

wouldn't this be reported as an out-of-sync type then ?

@ondrejmirtes
Copy link
Member

Yeah, if you have for example native typehint int, you have to write @param int $var too.

@derrabus
Copy link

@param without type is worthless

It isn't because @param also allows you to write human-readable documentation for a parameter. If you want to keep that documentation without repeating the type declaration, such an annotation makes sense to me.

I understand that it's worthless for a tool like PHPStan, but PHPStan is not the only tool parsing @param annotations. phpDocumentor defines the annotation as follows.

@param [<Type>] [name] [<description>]

Note that the type is optional here. Since there is no real standard about PHPDoc annotations, I would consider phpDocumentor's documentation to be the de-facto standard at the moment. The error message currently produced by PHPStan tells me that the standard is not implemented correctly.

and often a mistake.

That I can agree on. I would totally understand, if PHPStan errors on @param without a type if there is also no parameter type declaration.

Yeah, if you have for example native typehint int, you have to write @param int $var too.

And that's exactly the kind of redundancy we'd like to eliminate.


Suggestion

Passes without an error:

/**
 * @param $i Some documentation
 */
function takesAnInt(int $i): void {
}

Errors because of missing type information for $i:

/**
 * @param $i Some documentation
 */
function takesAnInt($i): void {
}

Errors because of useless @param:

/**
 * @param $i
 */
function takesAnInt(int $i): void {
}

Errors because of useless @param and because of missing type information for $i:

/**
 * @param $i
 */
function takesAnInt($i): void {
}

What do you think?

@ondrejmirtes ondrejmirtes reopened this Jul 8, 2021
@ondrejmirtes
Copy link
Member

Yeah, it shouldn't be that hard to support this if that's what people want. We don't need the @param parse error - if there's no type present, PHPStan will report that on level 6.

Only thing that should remain parse error should be @param $i without description. All of the following should be valid in the future:

  • @param int $i foo - everything's present, including description
  • @param int $i - the most common form
  • @param $i foo - no type but description is included

I hope the parser here can be modified to support this.

@dkarlovi
Copy link

dkarlovi commented Feb 25, 2022

Note, at some point PHPDoc allowed for

@param $variable Type

Which is IIRC reported / fixed by CS Fixer (or some other tool?)

This means stuff like Symfony's https://github.com/symfony/symfony/blob/08fa74a16c84895575e305b2a7ee3a03e371f79b/src/Symfony/Component/Console/Command/Command.php#L448 will get misinterpreted by tools which understand / support both, like PHPStorm and possibly others (PHPStorm thinks the linked param's type is string|array|The)

@ondrejmirtes
Copy link
Member

You can now enjoy this in: https://github.com/phpstan/phpstan/releases/tag/1.7.12

@tmotyl
Copy link

tmotyl commented Jun 15, 2022

@ondrejmirtes can this change be configured?
It was quite surprising to see errors disappear from baseline, which I consider a valid bug.
See this case:

   /**
     * @param $a
     * @param $b
     * @return int
     */
    protected function _sortMethods($a, $b)
    {
        if (is_object($a)) {
            return (int)$a->sort_order < (int)$b->sort_order ? -1 : ((int)$a->sort_order > (int)$b->sort_order ? 1 : 0);
        }
        return 0;
    }

before 1.7.12 it was reported as error:

#^PHPDoc tag @param has invalid value \(\$a\)\: Unexpected token "\$a", expected type at offset 18$#

and since 1.7.12, its not. This is a pain, as I would love phpstan to continue to report this issue (I want to have types declared here :).

@derrabus
Copy link

@tmotyl PHPStan still complains about the parameters not having a documented type: https://phpstan.org/r/c5b0d29b-c9a3-46d5-bd9a-abec237715ed

@tmotyl
Copy link

tmotyl commented Jun 15, 2022

@derrabus I see where the issue is -> My project is on phpstan level 2, which is now not reporting the issue.
You're right phpstan still complains about params not having type but only at level > 6.

@derrabus
Copy link

Sounds like a case of https://xkcd.com/1172/. 😅

@ondrejmirtes
Copy link
Member

I don't understand why people complain that an issue that THEY STILL IGNORED is stopped being reported, sounds like a win in my book :)

I consider the removed issue kind of a duplicate of the "missing typehint" errors from level 6 so this is the right behaviour going forward.

@tmotyl
Copy link

tmotyl commented Jun 15, 2022

@ondrejmirtes now that I know that it will be eventually reported (but on higher level) it's ok.
The baseline file is like a todo list for the team. So if a valid problem disappears from the baseline it's concerning :)

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants