-
Notifications
You must be signed in to change notification settings - Fork 117
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Psalm: switch from Phive to Composer + fix (most) issues #284
Psalm: switch from Phive to Composer + fix (most) issues #284
Conversation
This switches the installation method for Psalm from Phive to Composer, while still using a Phar file for running Psalm. Includes: * Removing Psalm from the Phive configuration. * Adding Psalm to the Composer configuration. Includes upgrading from version `3.11.2` to version `4.8.1`. * Adjusting the script used in the `Makefile`. 馃憠 Please verify and test this as things work differently on different OS-es and this should work for you. * Adjusting the GH Actions script to use the Composer installed version of Psalm. Note: due to the committed `composer.lock` file, Psalm will not automatically upgrade when newer versions are available. Refs: * https://github.com/vimeo/psalm/releases * https://github.com/psalm/phar/releases
Correctly flagged by Psalm: ``` ERROR: PossiblyNullArgument - src\Utils.php:50:53 - Argument 3 of preg_split cannot be null, possibly null value provided (see https://psalm.dev/078) $parts = php_preg_split($pattern, $subject, $limit, $flags); ``` The `$limit` argument of the PHP native `preg_split()` function is not nullable. Ref: https://www.php.net/manual/en/function.preg-split
Psalm flags these type casts as redundant: ``` ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Author.php:80:23 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263) $authorName = (string) $this->authorName; ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Example.php:150:21 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263) $filePath = (string) $this->filePath; ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Link.php:74:17 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263) $link = (string) $this->link; ERROR: RedundantCastGivenDocblockType - src/DocBlock/Tags/Method.php:228:23 - Redundant cast to string given docblock-provided type (see https://psalm.dev/263) $methodName = (string) $this->methodName; ``` I have verified each and can confirm that these are redundant. They are probably a left-over from the time when the `__construct()` method in these classes did not yet have type declarations.
Psalm flags this condition as redundant: ``` ERROR: RedundantCondition - src/DocBlock/Tags/Return_.php:62:48 - "" can never contain non-empty-lowercase-string (see https://psalm.dev/122) return $type . ($description !== '' ? ($type !== '' ? ' ' : '') . $description : ''); ``` Based on the statement in the line above - `$type = $this->type ? '' . $this->type : 'mixed';` -, Psalm is correct and the `$type` variable can never be an empty string.
The current version of Psalm flags the following issues: ``` ERROR: InvalidReturnType - src\Utils.php:44:16 - The declared return type 'array<array-key, string>' for phpDocumentor\Reflection\Utils::pregSplit is incorrect, got 'list<list<int|string>|string>' (see https://psalm.dev/011) * @return string[] Returns an array containing substrings of subject split along boundaries matched by pattern ERROR: InvalidReturnStatement - src\Utils.php:55:16 - The inferred type 'list<list<int|string>|string>' does not match the declared return type 'array<array-key, string>' for phpDocumentor\Reflection\Utils::pregSplit (see https://psalm.dev/128) return $parts; ``` I'm suggest ignoring this as `list` isn't an officially supported type.
a37c85b
to
9078e85
Compare
Rebased to get past merge conflict |
Thanks for all your hard work! |
@jaapio No problem. Let me know what you want me to do with that last remaining issue (see description at the bottom of the PR description). |
@jrfnl you should be able to solve that with propertly in psalm with |
@orklah I can't find any documentation on those notations. I can only find a generic paragraph about using Also, as I say above, IMO annotating this or adding input validation is nonsensical. The docblock contains all the relevant info about the supported values already and PHP will handle the rest. Either way, I'll defer to whatever solution is preferred by the maintainer team. |
@jrfnl Yeah, Psalm can sometimes be poor in documentation... The types are mentionned there (in context of plugins): https://psalm.dev/docs/running_psalm/plugins/plugins_type_system/
But yeah, I admit it's pretty advanced notation. I'm surprised that PHP accepts any flag though, and this is a recipe for a disaster if PHP ever adds a new flag for this function. Any unvalidated input could change behavior without notice... |
that is wrong here is not the Regarding the bitmask, I think it is safe to just ignore this error. Anyone using our utils library should be punished 馃槈, reminding me that adding a Other changes look good to me. 馃憤 |
As discussed in the PR: > > Argument 4 of preg_split expects 0|1|2|3|4|5|6|7, parent type int provided (see https://psalm.dev/193) > > I believe this issue is for the `phpDocumentor\Reflection\Utils` class and expects the `pregSplit()` method to apply input validation to the value received for `$flags` before passing it off to the PHP native `preg_split()` function. > > IMO that's taking things a little too far as PHP will handle this internally without errors. > See: https://3v4l.org/NdDRK
Still not completely clear to me what to change this to in that case. Are you saying it should be
Done ;-) |
|
... and fix it instead.
Done. 馃馃徎 |
Argh... eh... so that type change... nope. What am I doing wrong ? |
@jrfnl I will pick this one :-) |
Not sure what you mean by that, but shall I rebase the PR first so at least it's out of conflict ? |
I will rebase this and check what is needed to make psalm pass. It looks like there is some bug in here, detected with psalm. |
The issue is, For now I add an extra assert. |
Rebased and merged manually. I don't know why this is not detected by github. Thanks for these contributions. |
Psalm: switch from Phive to Composer
This switches the installation method for Psalm from Phive to Composer, while still using a Phar file for running Psalm.
Includes:
3.11.2
to version4.8.1
.Makefile
.馃憠 Please verify and test this as things work differently on different OS-es and this should work for you.
Note: due to the committed
composer.lock
file, Psalm will not automatically upgrade when newer versions are available.Refs:
Utils::pregSplit: limit is not nullable
Correctly flagged by Psalm:
The
$limit
argument of the PHP nativepreg_split()
function is not nullable.Ref: https://www.php.net/manual/en/function.preg-split
Tags::__toString(): remove redundant type casts
Psalm flags these type casts as redundant:
I have verified each and can confirm that these are redundant. They are probably a left-over from the time when the
__construct()
method in these classes did not yet have type declarations.Tags/Return: remove redundant condition
Psalm flags this condition as redundant:
Based on the statement in the line above -
$type = $this->type ? '' . $this->type : 'mixed';
-, Psalm is correct and the$type
variable can never be an empty string.Psalm: suppress two notices
The current version of Psalm flags the following issues:
I'm suggest ignoring this as
list
isn't an officially supported type.After this PR, there is still one issue remaining.
I believe this issue is for the
phpDocumentor\Reflection\Utils
class and expects thepregSplit()
method to apply input validation to the value received for$flags
before passing it off to the PHP nativepreg_split()
function.IMO that's taking things a little too far as PHP will handle this internally without errors. Want me to add it to the list of issues to be ignored ?
See: https://3v4l.org/NdDRK
Side-note: the weird thing is that this issue does show up in CI, but with the same PHP + Psalm Phar version, I cannot reproduce this locally.