Declare more precise phpdoc types #993

staabm · 2024-04-12T20:39:50Z

goal: ease working in projects which are static analysis covered.

we are doing the same thing in PHPStan with phpstan/phpstan-src#3013 (review)

staabm · 2024-04-12T20:41:08Z

lib/PhpParser/Node/Name.php

@@ -8,6 +8,7 @@ class Name extends NodeAbstract
 {
    /**
     * @var string[] Parts of the name
+     * @psalm-var non-empty-array<string>
     * @deprecated Use getParts() instead
     */
    public $parts;


we could even declare * @param non-empty-string|non-empty-array<string>|self $name in __construct but since more strict param types could be considered a BC break, I did not add this change.

staabm · 2024-04-12T21:19:48Z

in the same turn I think about making the $type in Identifier a non-empty-string.. do you think this would work?

nikic · 2024-04-13T00:02:04Z

Can you please target this at master instead?

in the same turn I think about making the $type in Identifier a non-empty-string.. do you think this would work?

I think that's fine. Empty string identifiers need to be written as something like {''}, in which case they're not Identifiers anymore.

lib/PhpParser/Node/Identifier.php

staabm · 2024-04-13T06:48:36Z

the CI build on php-parser@v5 looks pretty broken. it seems there is some chicken-egg problem regarding deps and v4.
I don't think its related to this PR

staabm · 2024-04-13T07:25:04Z

after a deep dive into the CI problem I found a solution (its still not caused by this PR though).
My guess is, that this errors only occur in pull requests from forks.

the last remaining question is #993 (comment)

Thank you.

ondrejmirtes · 2024-04-13T08:25:35Z

I’m not a huge fan of this. More precise parameter types will break static analysis in many projects, forcing people to write more precise PHPDoc types.

These types of changes should be done only in a major version IMHO.

staabm · 2024-04-13T08:29:34Z

I’m not a huge fan of this. More precise parameter types will break static analysis in many projects, forcing people to write more precise PHPDoc types.

Yeah thats why I asked. I am fine with return type improvements only

ondrejmirtes · 2024-04-13T08:36:37Z

Yeah, return types should be fine, although it's a slight BC break too.

Imagine you're extending a class and typehinting string return type.

If the parent narrows the return type, suddenly the child return type is no longer covariant.

nikic · 2024-04-19T11:46:31Z

.github/workflows/main.yml

@@ -20,7 +20,7 @@ jobs:
      - name: "Install dependencies"
        run: |
          composer require php-coveralls/php-coveralls:^2.2 --dev --no-update
-          composer update --no-progress --prefer-dist
+          COMPOSER_ROOT_VERSION=dev-master composer update --no-progress --prefer-dist


What does this do / why is it necessary?

without this fix, CI errors in PRs of forks. see #993 (comment)

errors beeing fixed can be seen here: https://github.com/nikic/PHP-Parser/actions/runs/8671744331/job/23781260607?pr=993

Loading composer repositories with package information Updating dependencies Your requirements could not be resolved to an installable set of packages. Problem 1 - phpunit/phpunit[9.6.12, ..., 9.6.19] require phpunit/php-code-coverage ^9.2.28 -> satisfiable by phpunit/php-code-coverage[9.2.28, 9.2.29, 9.2.30, 9.2.31]. - phpunit/phpunit[9.5.16, ..., 9.6.11] require phpunit/php-code-coverage ^9.2.13 -> satisfiable by phpunit/php-code-coverage[9.2.13, ..., 9.2.31]. - phpunit/phpunit[9.5.10, ..., 9.5.14] require phpunit/php-code-coverage ^9.2.7 -> satisfiable by phpunit/php-code-coverage[9.2.7, ..., 9.2.31]. - phpunit/phpunit[9.5.0, ..., 9.5.9] require phpunit/php-code-coverage ^9.2.3 -> satisfiable by phpunit/php-code-coverage[9.2.3, ..., 9.2.31]. - phpunit/phpunit[9.4.0, ..., 9.4.4] require phpunit/php-code-coverage ^9.2 -> satisfiable by phpunit/php-code-coverage[9.2.0, ..., 9.2.31]. - phpunit/phpunit 9.3.11 requires phpunit/php-code-coverage ^9.1.11 -> satisfiable by phpunit/php-code-coverage[9.1.11, ..., 9.2.31]. - phpunit/phpunit[9.3.8, ..., 9.3.10] require phpunit/php-code-coverage ^9.1.5 -> satisfiable by phpunit/php-code-coverage[9.1.5, ..., 9.2.31]. - phpunit/phpunit[9.3.4, ..., 9.3.7] require phpunit/php-code-coverage ^9.1.1 -> satisfiable by phpunit/php-code-coverage[9.1.1, ..., 9.2.31]. - phpunit/phpunit[9.3.0, ..., 9.3.3] require phpunit/php-code-coverage ^9.0 -> satisfiable by phpunit/php-code-coverage[9.0.0, ..., 9.2.31]. - phpunit/phpunit[9.0.0, ..., 9.2.6] require php ^7.3 -> your php version (8.3.6) does not satisfy that requirement. - phpunit/php-code-coverage[9.0.0, ..., 9.1.2] require nikic/php-parser ^4.7 -> satisfiable by nikic/php-parser[v4.7.0, ..., 4.x-dev] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - phpunit/php-code-coverage[9.1.3, ..., 9.2.2] require nikic/php-parser ^4.8 -> satisfiable by nikic/php-parser[v4.8.0, ..., 4.x-dev] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - phpunit/php-code-coverage[9.2.3, ..., 9.2.6] require nikic/php-parser ^4.10.2 -> satisfiable by nikic/php-parser[v4.10.2, ..., 4.x-dev] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - phpunit/php-code-coverage 9.2.7 requires nikic/php-parser ^4.12.0 -> satisfiable by nikic/php-parser[v4.12.0, ..., 4.x-dev] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - phpunit/php-code-coverage[9.2.8, ..., 9.2.15] require nikic/php-parser ^4.13.0 -> satisfiable by nikic/php-parser[v4.13.0, ..., 4.x-dev] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - phpunit/php-code-coverage[9.2.16, ..., 9.2.24] require nikic/php-parser ^4.14 -> satisfiable by nikic/php-parser[v4.14.0, ..., 4.x-dev] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - phpunit/php-code-coverage[9.2.25, ..., 9.2.29] require nikic/php-parser ^4.15 -> satisfiable by nikic/php-parser[v4.15.0, ..., 4.x-dev] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - phpunit/php-code-coverage[9.2.30, ..., 9.2.31] require nikic/php-parser ^4.18 || ^5.0 -> satisfiable by nikic/php-parser[dev-master, v4.18.0, v4.19.0, v4.19.1, 4.x-dev, v5.0.0alpha1, ..., 5.0.x-dev (alias of dev-master)] from composer repo (https://repo.packagist.org/) but nikic/php-parser is the root package and cannot be modified. See https://getcomposer.org/dep-on-root for details and assistance. - Root composer.json requires phpunit/phpunit ^9.0 -> satisfiable by phpunit/phpunit[9.0.0, ..., 9.6.19]. Error: Your requirements could not be resolved to an installable set of packages.

without thix fix composer does not realize what the version of the package of the checked-out project is

Interesting. I guess I caused this with b43758e and previously this probably just resolved to PHPUnit 8 instead...

nikic · 2024-04-19T11:47:12Z

lib/PhpParser/Node/Identifier.php

@@ -25,6 +28,10 @@ class Identifier extends NodeAbstract {
     * @param array<string, mixed> $attributes Additional attributes
     */
    public function __construct(string $name, array $attributes = []) {
+        if ($name === '') {
+            throw new \InvalidArgumentException('Identifier name cannot be empty');
+        }


Check this in https://github.com/nikic/PHP-Parser/blob/master/test/PhpParser/Node/IdentifierTest.php?

staabm commented Apr 12, 2024

View reviewed changes

staabm marked this pull request as ready for review April 12, 2024 20:41

staabm mentioned this pull request Apr 12, 2024

Fix fatal error on constant('') phpstan/phpstan-src#3013

Open

staabm changed the base branch from 4.x to master April 13, 2024 06:25

Declare more precise phpdoc types

999135f

staabm force-pushed the types branch from dce0f17 to 999135f Compare April 13, 2024 06:33

non-empty-string in Identifier

e244286

staabm force-pushed the types branch from 3e606e6 to 3203270 Compare April 13, 2024 06:39

staabm commented Apr 13, 2024

View reviewed changes

lib/PhpParser/Node/Identifier.php Outdated Show resolved Hide resolved

fix typo

42a34bd

staabm force-pushed the types branch from 3203270 to 42a34bd Compare April 13, 2024 06:41

staabm added 3 commits April 13, 2024 09:16

try fix CI

911ba18

try fix more CI

3182297

try fix coverage job

e408e0b

Revert stricter param type which would be a BC break.

ca150d9

nikic reviewed Apr 19, 2024

View reviewed changes

added test

f917163

nikic merged commit c5ee33d into nikic:master Apr 19, 2024
9 checks passed

staabm deleted the types branch April 19, 2024 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declare more precise phpdoc types #993

Declare more precise phpdoc types #993

staabm commented Apr 12, 2024 •

edited

staabm Apr 12, 2024 •

edited

staabm commented Apr 12, 2024

nikic commented Apr 13, 2024

staabm commented Apr 13, 2024 •

edited

staabm commented Apr 13, 2024 •

edited

ondrejmirtes commented Apr 13, 2024

staabm commented Apr 13, 2024

ondrejmirtes commented Apr 13, 2024

nikic Apr 19, 2024

staabm Apr 19, 2024 •

edited

nikic Apr 19, 2024

nikic Apr 19, 2024

Declare more precise phpdoc types #993

Declare more precise phpdoc types #993

Conversation

staabm commented Apr 12, 2024 • edited

staabm Apr 12, 2024 • edited

Choose a reason for hiding this comment

staabm commented Apr 12, 2024

nikic commented Apr 13, 2024

staabm commented Apr 13, 2024 • edited

staabm commented Apr 13, 2024 • edited

ondrejmirtes commented Apr 13, 2024

staabm commented Apr 13, 2024

ondrejmirtes commented Apr 13, 2024

nikic Apr 19, 2024

Choose a reason for hiding this comment

staabm Apr 19, 2024 • edited

Choose a reason for hiding this comment

nikic Apr 19, 2024

Choose a reason for hiding this comment

nikic Apr 19, 2024

Choose a reason for hiding this comment

staabm commented Apr 12, 2024 •

edited

staabm Apr 12, 2024 •

edited

staabm commented Apr 13, 2024 •

edited

staabm commented Apr 13, 2024 •

edited

staabm Apr 19, 2024 •

edited