PhpStan based tag parsing

[jaapio]

In this PR I'm working on an attempt to replace the regex-based parsing in each tag with a more battle tested parser that allows us to support all modern types like tools as phpstan and psalm are defining.

• implement param tag
• implement method tag
• implement return tag
• implement var tag
• implement property tag
• introduce callable params
• introduce callable return type
• document new features
• document new architecture
• deprecate replaced tag factories
• implement constant types

BC promise

This library has existed for many years and is used in many projects to get information from docblocks. We can do this fast, and reliably. The new implementation should be introduced without any BC breaks. Only changed and improved tags.

The current approach allows external consumers to create tags based on instanciated factories. Not just the static factory methods as we knew them before. This will enable us to introduce more complex logic in the factories and add extra dependencies. Which as not fully possible in the past.

[jaapio]

@voku and @williamdes what are your thoughts about this?

[williamdes]

is the version drop required ? could we drop less of them ?

[jaapio]

Why would you like to support end-of-life versions of PHP? I think for people using old versions of PHP the older library versions will work.

Tools using this library can easily run on newer versions while the applications being processed can be on old PHP versions.

But I'm open to see if we can support PHP 7.2 as well. however phpstan requires > 7.4

[williamdes]

Because that would mean that I also need to drop PHP versions. I am not saying this is bad but if it can be avoided because x.y is supported by the lib you add then let's not drop them ?

[williamdes]

@voku and @williamdes what are your thoughts about this?

That would mean a lot for me as a maintainer of Doctum
It would bring support for more cool syntax and remove some buggy behaviors this lib has

[williamdes]

@voku and @williamdes what are your thoughts about this?

That would mean a lot for me as a maintainer of Doctum It would bring support for more cool syntax and remove some buggy behaviors this lib has

Ref: https://twitter.com/wdesportes/status/1535565834340417536

[jaapio]

I started to cleanup the code in this PR, Please check phpDocumentor/TypeResolver#175

[jaapio]

I will add the missing documentation in a new PR. This should be ready to merge.

I have decided that dropping end-of-life PHP versions is required. We have to move forward and other dependencies of this library are dropping PHP 7.2 as well. As this library is mainly used for static analysis where the executed code is not the analyzed code. There are good options to make it work for legacy projects. This will allow us to start optimizing the code for php 7.4+ and be more strict in a number of locations.

There are no BC breaks regarding the new implementation, so it should be possible to support the earlier released version together with this update.

[williamdes]

I am testing your changes onto Doctum, here is my PR: code-lts/doctum#51

[williamdes]

With your work this changed to InvalidTag

class phpDocumentor\Reflection\DocBlock\Tags\InvalidTag#962 (3) {
  private $name =>
  string(33) "void setInteger(integer $integer)"
  private $body =>
  string(7) "@method"
  private $throwable =>
  NULL
}

* @method void setInteger(integer $integer)

https://github.com/code-lts/doctum/blob/b789b4e76fea8e71c319999cfc40eb6b70eeb147/tests/Parser/DocBlockParserTest.php#L369

It is invalid ?

[williamdes]

And https://github.com/code-lts/doctum/blob/b789b4e76fea8e71c319999cfc40eb6b70eeb147/tests/Parser/NodeVisitorTest.php#L395 is failing because

because only param1 and param4 are found as @param tags

            '/**' . "\n"
            . '* @param type1 $param1 Description' . "\n"
            . '* @param $param8 Description 4' . "\n"
            . '* @param $param9' . "\n"
            . '* @param foo' . "\n"
            . '* @param type1 $param4 Description 4' . "\n"
            . '**/' . "\n",

1) Doctum\Tests\Parser\NodeVisitorTest::testUpdateMethodParametersFromTags
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
     0 => 'The "param2" parameter of the method "fun1" is missing a @param tag'
-    1 => 'The method "fun1" has "5" @param tags but only "2" where expected.'
 )

[williamdes]

And finally @param array[\Illuminate\Notifications\Channels\Notification] $notification is now mixed

'/**' . "\n"
            . '* @param array[\Illuminate\Notifications\Channels\Notification]  $notification' . "\n"
            . '**/' . "\n",

class phpDocumentor\Reflection\DocBlock\Tags\Param#1025 (6) {
  protected $name =>
  string(5) "param"
  protected $description =>
  class phpDocumentor\Reflection\DocBlock\Description#1019 (2) {
    private $bodyTemplate =>
    string(0) ""
    private $tags =>
    array(0) {
    }
  }
  protected $type =>
  class phpDocumentor\Reflection\Types\Mixed_#1020 (0) {
  }
  private $variableName =>
  string(12) "notification"
  private $isVariadic =>
  bool(false)
  private $isReference =>
  bool(false)
}

Failing my invalid tag detection and error reporting

1) Doctum\Tests\Parser\NodeVisitorTest::testUpdateMethodParametersFromInvalidTagsReport
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 (
-    0 => 'The "notification" parameter of the method "fun1" is missing a @param tag'
-    1 => 'The method "fun1" has "1" invalid @param tags.'
-    2 => 'Invalid @param tag on "fun1": "array[\Illuminate\Notifications\Channels\Notification]  $notification"'
-)
+Array &0 ()

And yes using array<\Illuminate\Notifications\Channels\Notification> gives a correct array type

[jaapio]

Thanks @williamdes for reporting this. I will have a look how to fix this. If you are able to detect it yourself, pr's are welcome.

It looks like we are missing some testcoverage somewhere. :-(

[williamdes]

If you are able to detect it yourself, pr's are welcome.

For now I will not have enough time to contribute on this repo and learn it's internals. But you can ping me on PRs

[jaapio]

@method void setInteger(integer $integer)

Just fixed this one... I did overlook a factory registration. Added tests to prevent future regressions.

[williamdes]

@method void setInteger(integer $integer)

Just fixed this one... I did overlook a factory registration. Added tests to prevent future regressions.

Thank you for 29e383a!

[xabbuh]

In the Symfony test suite we have a fixtures with a property like this:

/**
 * @var ?string|int
 */
public $i;

We can update the docblock to this which would make things work again, but there may be others affected by the behaviour change as well:

/**
 * @var string|int|null
 */
public $i;

Should I open a dedicated issue for this?

[voku]

@jaapio I also see many warnings in my tests: https://github.com/voku/Simple-PHP-Code-Parser/actions/runs/3560700677/jobs/5980930057

---

• there are also some missing "description" (I think PHPStan will not fetch them, or?)

e.g.

        /**
         * This is a test-text [...] öäü !"§?.
         *
         * @param \phpDocumentor\Reflection\DocBlock\Tags\BaseTag $parsedParamTag
         *                                                                        <p>this is a test-text [...] öäü !"§?</p>
         *
         * @return array
         *
         * @psalm-return array{parsedParamTagStr: string, variableName: null[]|string}
         */
        public function withComplexReturnArray(\phpDocumentor\Reflection\DocBlock\Tags\BaseTag $parsedParamTag)
        {
            return [
                'parsedParamTagStr' => 'foo',
                'variableName'      => [null],
            ];
        }

• it will not report missing phpdocs anymore (Unexpected token "$parsedParamTag", expected type)

e.g.

  /**
     * @param $parsedParamTag
     *
     * @return array
     *
     * @psalm-return array{parsedParamTagStr: string, variableName: null[]|string}
     */
    public static function withEmptyParamTypePhpDoc($parsedParamTag)
    {
        return [
            'parsedParamTagStr' => 'foo',
            'variableName'      => [null],
        ];
    }

• something also happened to "{@inheritdoc}", but currently I do not see the error