The original php method preg_split might return `string[][]` in some
situations, however this should never be the case for this library. By
adding an extra assert, we should be more safe.
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
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.
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.
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.
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
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
* Fix the name and description to prevent confusion between the project ruleset and the organisation ruleset.
* Set the minimum PHP version for the PHPCompatibility standard.
* Don't require property type declarations.
* Ensure special characters used as literals in an exclude pattern are escaped.
PHPStan flags the code within the `Source::__toString()` method:
```
------ -------------------------------------------------------------------------------------------
Line DocBlock\Tags\Source.php
------ -------------------------------------------------------------------------------------------
111 Result of || is always true.
114 Result of || is always true.
114 Result of || is always true.
------ -------------------------------------------------------------------------------------------
```
I have investigated this and can confirm that these flags are correct.
1. `$this->startingLine` is cast to an integer in the `__construct()` method (line 45) and subsequently cast to a string in `__toString()` (line 105).
This means that it can only ever be a non-empty ("truthy") string or the string '0', so the `$startingLine || $startingLine === '0'` condition used in two places is redundant.
2. `$this->lineCount` is either an integer or `null` after the `__construct()` method (line 46).
In the `__toString()` method, if the `lineCount` is an integer, it is effectively cast to a string by the concatenation with an empty string on line 107, while if the `lineCount` was `null`, it is turned into an empty string.
By changing the concatenation from concatenating with an empty string to concatenating with a one-space string, we can remove the ternary in the `return` statement checking for `$lineCount` being empty.
The existing unit tests already cover this code and still pass after this change.
... or rather add the missing `@coversNothing` as this is an integration test.
This prevents the test from being marked as "risky" due to the missing tag on PHPUnit 9.x:
```
There was 1 risky test:
1) phpDocumentor\Reflection\DocblockSeeTagResolvingTest::testResolvesSeeFQSENOfInlineTags
This test does not have a @covers annotation but is expected to have one
```
... to prevent it from being considered "risky" and being listed as "risky" below each test run.
```
There was 1 risky test:
1) phpDocumentor\Reflection\ModifyBackTraceSafeTest::testBackTraceModificationDoesNotImpactFunctionArguments
This test did not perform any assertions
/home/runner/work/ReflectionDocBlock/ReflectionDocBlock/tests/integration/ModifyBackTraceSafeTest.php:15
```
Jaap van Otterdijk
Fabien Villepinte
dependabot[bot]
jrfnl