-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP 8.1: Resolve readonly
earlier to fix some bugs
#3584
Conversation
Heads-up: I just applied a minor fix to #3515 as for some silly reason it turned out it had gotten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this yet another step further @kukulich!
I've tested the test cases with and without the proposed change and can confirm the issue of T_BITWISE_OR
not being retokenized to T_TYPE_UNION
when on PHP < 8.1 and that this change fixes that.
If I look at the new implementation for the backfill, however, I see redundancy between the last two conditions of the wrapper if
and the if
/else
inside.
Also note: readonly
can only ever be the readonly
keyword when not followed by an open parenthesis. This includes function calls.
See: php/php-src#7468
There's already a test in place for this, but not one using "wide" spacing, which is where the logic error comes into play.
I'd suggest adding the following extra test which should highlight the problem:
/* testReadonlyUsedAsFunctionCallWithSpaceBetweenKeywordAndParens */
$var = readonly /*comment*/ (); // Should be tokenized as T_STRING.
Regarding the BitwiseOrTest
, just to be on the safe side:
- Should there also be a test with an unconvential (and unsupported) modifier order ? i.e.
readonly protected ...
- Should there be a test with
static readonly
? (also unsupported, but that's not the concern of the tokenizer/File::getMemberProperties()
method) - Should there be a test with
var readonly
? (again unsupported, but again that's not the concern of the tokenizer/File::getMemberProperties()
method)
And
- Would it be an idea to add a test with
readonly
to theNamedFunctionCallArgumentsTest
as well ?
Just confirmed: the test in /* testReservedKeywordReadonly1 */
foobar(readonly: $value, /* testReservedKeywordReadonly2 */ readonly: $value); The above would currently incorrectly tokenize to |
2675692
to
b24eb63
Compare
Rebased.
Thanks, I forgot to remove some lines after refactoring.
Test added but I think the token should be tokenized as
All three tests added.
Test added and fixed. |
Thanks for the updates! Looks good, though I haven't rerun my tests (yet).
Could you explain why you think it should be tokenized as As for my argumentation: even as a parse error, it is definitely not the |
We would have to change the token also for PHP 8.1+: https://3v4l.org/3AfHQ |
Except we probably should anyway: see https://3v4l.org/rFcHe (without the comment) and https://3v4l.org/Wb3mA |
I don’t understand. I think this case is already solved and tested. |
Just catching up on the discussion around With this change the following code tokenizes the function readonly() {}
$var = readonly(); and the following code tokenizes the function readonly /*comment*/ () {}
$var = readonly /*comment*/ (); This is also how PHP 8.1+ tokenizes these strings because the second set of cases are both parse errors, so we can say that the backport is consistent. Time could be spent to tokenize the second set of Please let me know if I'm missing something, otherwise I'll merge this in. |
Yes, my intention was to make the backport compatible with PHP 8.1+.
I think it's not neccessary so we don't need to spend our time on it. I would also need more code (to modify the behaviour on PHP 8.1+). |
I've merged this in now - thanks a lot for the work on this. Any changes to the token structure across all PHP versions can be handled elsewhere, but I thought it was important to get this fix in. |
Fixes some bugs for code like this:
readonly
has to be resolved earlier to resolveT_TYPE_UNION
right.It's based on #3515 to minimize conflicts.