Function parameter defaults are incorrect for const-like values

[tmdk]

When determining the default value of an Argument, the result is wrong when the default value is referencing either a constant or class constant.

An example:

<?php

require __DIR__ . '/vendor/autoload.php';

use Acme\Plugin;

function foo( $output = Plugin::class ) {}

function bar( $output = OBJECT ) {}

$projectFactory = \phpDocumentor\Reflection\Php\ProjectFactory::createInstance();
$projectFiles   = [ new \phpDocumentor\Reflection\File\LocalFile( __FILE__ ) ];
$project        = $projectFactory->create( 'My Project', $projectFiles );
$file           = $project->getFiles()[ __FILE__ ];

// output: \Plugin::class
echo $file->getFunctions()['\\foo()']->getArguments()[0]->getDefault( false )->render() . PHP_EOL;

// output: object
echo $file->getFunctions()['\\bar()']->getArguments()[0]->getDefault( false )->render() . PHP_EOL;

Analysis:

\phpDocumentor\Reflection\Php\Factory\Reducer\Parameter::determineDefault() uses ExpressionPrinter to render the default value, which in turn uses TypeResolver to render Name nodes.

In case of the OBJECT const default value, the type resolver returns the type Object_ without FQSEN (which is a result, I guess, of the phpstan type parser not having enough context to properly deduce the type). In absence of a FQSEN, casting the Object_ to string returns 'object'

In case of the Plugin::class class const default value, the type resolver returns the type Object_ with FQSEN '\Plugin'. I'm guessing in this case, the problem is that the file does not have a namespace? It seems no Context is created that contains the namespace aliases / use statements, so the type resolver runs with an empty Context, leading to the incorrect result. If I add a namespace to the file, the FQSEN does resolve correctly.

When debugging this issue, I also noticed that the default values were not properly resolved by php-parser. This is due to ElementNameResolver stopping traversal of Function and Method nodes, which means the NameResolver visitor never sees the function/method parameters. I can see that you would stop traversal out of performance considerations, but is it really worth all the effort in other places to fix up the missing name resolutions?

[jaapio]

Thanks for reporting this, I started to debug this as well and found the issue. It was not at the level I expected it to be. See #717

You can see the new results, please let me know if this resolves the issue.

To answer your question about the name resolving, when we started this project php-parser was not able to do this name resolving. At some point it was added, but we never looked back.

Back then we also thought it would be wise to stop traversing the whole node tree as we did not support collecting information from the method internals. This has been changed in the year and this library does allow you now to collect information from the method body. I did add a benchmark at some point to make sure we did not introduce to many performance penalties. At this point I do not see the need to change the strategy.

[tmdk]

Thanks for looking into this, @jaapio

In case of the class const default value, the result matches the expectation now. Fixing that involved a lot more than I would have imagined!

In case of the const default value, nothing has changed. Since the underlying node is a ConstFetch node, I would expect the rendered default value to be simply the stringified name of the node (OBJECT). The added testcase for this has me confused because it seems to imply that the type should be an Object_ without FQSEN. Just as an example, the php-parser NameResolver resolves the ConstFetch node name to this:

Expr_ConstFetch(
    name: Name_FullyQualified(
        name: OBJECT
    )
)

That makes a lot more sense to me! But maybe I'm missing something

[jaapio]

I realise this issue goes a bit deeper, I tried to stop traversing the children of methods and functions but that does not only make the process a bit slower, but also triggers issues in cases that are now not handled by this library.

For example:

    public function test() {
        function internal() {
            return "yep";
        }

        return internal();
    }

The internal() function does trigger errors in the ElementNameResolver, I could consider to get rid of it this class like you suggested in your opening post. I need to figure this out.

[tmdk]

FWIW, getting this right while traversing the AST just once requires a very specific way of building the reflection objects. You're forced to do it postorder (e.g. on leaving a node) to ensure the NameResolver has done its job, but at the same time it feels way more natural to build the reflection objects top-down.

I ended up doing two traversal passes to fix this contradiction, but that comes at a cost of course.