How distinct method with and without public visibility ?

[llaville]

Hello,

I've a problem [1] on my project php-compatinfo that used php-reflect that itself used PHP-Parser 1.0 (stable)

My question is : how can you distinct parsing nodes from code 1 to code 2

code 1

<?php
class Foo
{
    function bar() {}
}

code 2

<?php
class Foo
{
    public function bar() {}
}

Both gave me the same nodes dump

==> Node dump:
array(
    0: Stmt_Class(
        type: 0
        name: Foo
        extends: null
        implements: array(
        )
        stmts: array(
            0: Stmt_ClassMethod(
                type: 1
                byRef: false
                name: bar
                params: array(
                )
                stmts: array(
                )
            )
        )
    )
)

The PhpParser\Node\Stmt\ClassMethod::isPublic return me always TRUE with code1 or code2 (public visibility declared or not)

Thanks in advance for answer.
Laurent

[1] llaville/php-compatinfo#129

[cmb69]

The PhpParser\Node\Stmt\ClassMethod::isPublic return me always TRUE with code1 or code2 (public visibility declared or not)

That is caused by PhpParser\Parser, which defaults to MODIFIER_PUBLIC when no visibility modifier is given. phpparser/grammar/zend_language_parser.php line 481:

variable_modifiers:
      non_empty_member_modifiers                            { $$ = $1; }
    | T_VAR                                                 { $$ = Stmt\Class_::MODIFIER_PUBLIC; }
;

method_modifiers:
      /* empty */                                           { $$ = Stmt\Class_::MODIFIER_PUBLIC; }
    | non_empty_member_modifiers                            { $$ = $1; }
;

ISTM that it is not possible to distinguish an implicit from an explicit public modifier as it is now.

I'll try to come up with a patch; however, I won't be able to make a PR, due to no experience with Git (still stuck with SVN).

[nikic]

Yes, the parser does not distinguish between explicit and implicit visibility, because both are equivalent as far as PHP is concerned. It's a formatting difference and the syntax tree does not retain formatting.

The only way to distinguish this currently is by inspecting the tokens. Using the token offset lexer:

<?php

function isImplicitlyPublicProperty(array $tokens, Stmt\Property $prop) {
    $i = $prop->getAttribute('startOffset');
    return isset($tokens[$i]) && $tokens[$i][0] === T_VAR;
}

function isImplicitlyPublicFunction(array $tokens, Stmt\ClassMethod $method) {
    $i = $method->getAttribute('startOffset');
    for ($c = count($tokens); $i < $c; ++$i) {
        $t = $tokens[$i];
        if ($t[0] === T_PUBLIC || $t[0] === T_PROTECTED || $t[0] === T_PRIVATE) {
            return false;
        }
        if ($t[0] === T_FUNCTION) {
            break;
        }
    }
    return true;
}

If you like, I can add support for getting this information directly to the parser, but that would be a temporary solution.

[llaville]

@niki I beg your pardon, but I don't see how to use / implement it in real condition.

Could you explain me how you'll connect these two functions ?

[cmb69]

I have set up a Gist to show how isImplicitlyPublicProperty() can be used to distinguish implicit and explicit public properties. However, this uses a terrible hack to get the token list. @nikic: Is there any sane way to get the token list from the lexer or parser? Maybe adding LexerWithTokenOffsets::getTokens()?

[nikic]

@cmb69 Yep, that pretty much matches what I had in mind. And yes, getting the tokens using a getTokens() method on the lexer sounds like a fine approach (and is even necessary if you extend the emulative lexer - which has different output than token_get_all). So something like https://gist.github.com/nikic/04fce01e69ae5b7b44f8.

[llaville]

@cmb69 and @nikic Thanks to you. I understand much more now the approach, and finally I could do something on my php-reflect project that implement the php-parser.

[llaville]

As I'm currently implemented the solution, I'll noticed that there is an error in isImplicitlyPublicFunction function provided by Nikita (probably a copy/paste from isImplicitlyPublicProperty).

So replace line $i = $prop->getAttribute('startOffset'); by $i = $method->getAttribute('startOffset');

[nikic]

@llaville Yeah, that was a copy&paste error. I've fixed the code in the comment.

[llaville]

@nikic I was pretty glad of the solution after first unit test. But, there is a but, I can't solve, and I need your point of view.

To use the LexerWithTokenOffsets, we need tokens, that are initialized line 55 after the parser line 54. This is what I called the standard process.

Now, the BUT. If you don't parsed again your code, and used a cached version of the AST, like I did in php-reflect ( See lines 115 to 126 ), it does not work anymore.

Any idea to solve a "cache" problem implementation ?
Hope my explain are enough.

[llaville]

If you want to see result of my implementation. Have a look on the TokenOffsets branch.

See changes at commit llaville/php-reflect@4daccd5

Be carefull, this implementation run fine only if you don't add the cache plugin. (In my previous comment lines 115 to 126).

[nikic]

@llaville Doesn't that just mean that you need to cache both the tokens and the AST?

[llaville]

@niki Thanks for your answer. I was away this week-end, so I reply only now.

Finally I've adopted another solution. To avoid lot of impacts and problem, with cached AST results, I read online the tokens list.

And I got expected results.

Your solution works as expected, so I'll close this issue.

[cmb69]

@llaville

Finally I've adopted another solution.

As @nikic has explained, using token_get_all() will not necessarily work when the emulative lexer (\PhpParser\Lexer\Emulative) is used:

and is even necessary if you extend the emulative lexer - which has different output than token_get_all

[llaville]

@cmb69 Have you tested the previous solution without the commit ?

On first run it works, but on second run if you've the cachePlugin it fails !

NOTE: the TokenOffesets lexer used extends the Emulative lexer

[cmb69]

@llaville

Have you tested the previous solution without the commit ?

No. Unfortunately, I was not able to install the dev version of PHP_Reflect due to a dependency mismatch with PHP_CompatInfo reported by Composer.

On first run it works, but on second run if you've the cachePlugin it fails !

That's most likely due to the incompatibility of token_get_all() and Lexer\Emulative.

Can't you add another field "tokens" to the event object (besides "ast") on the first run, and retrieve and use this information in subsequent runs?

Anyway, these details might better be discussed in the respective PHP_Reflect issue.