Skip to content
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

LHS of assignment must be variable #19

Open
nikic opened this issue Jan 18, 2017 · 7 comments
Open

LHS of assignment must be variable #19

nikic opened this issue Jan 18, 2017 · 7 comments
Labels
bug Issue identified by VS Code Team member as probable bug error-handling grammar

Comments

@nikic
Copy link
Contributor

nikic commented Jan 18, 2017

Noticed while looking at example 4:

In PHP an expression like $a == $b = $c is parsed as $a == ($b = $c), because this is the only valid way to parse the code under the constraint that the LHS of an assignment must be a variable. The parser currently treats this as ($a == $b) = $c instead.

@nikic
Copy link
Contributor Author

nikic commented Jan 18, 2017

The same also applies to other expressions that accept only variables on the LHS, for example PHP interprets !$x instanceof Y as !($x instanceof Y), while the parser currently treats it as (!$x) instanceof Y.

@mousetraps
Copy link
Contributor

mousetraps commented Jan 19, 2017

Ahh, makes sense. Thanks for pointing that out! I was wondering why variables were called out separately from expressions in general.

For instanceof, I think I'm missing something because both the spec and yacc grammar seem to specify expression as LHS.

Looking back at my notes, my interpretation of the spec was that unary-expression takes precedence over instanceof-expression, which in turn takes precedence over multiplicative-expression, where both intanceof and multiplicative expressions are defined similarly, which led me to believe they're parsed similarly.

instanceof-expression:
   unary-expression
   instanceof-subject   instanceof   instanceof-type-designator

instanceof-subject:
   expression

multiplicative-expression:
   instanceof-expression
   multiplicative-expression   *   instanceof-expression
   multiplicative-expression   /   instanceof-expression
   multiplicative-expression   %   instanceof-expression

Should instanceof-subject be variable, rather than expression? And if so, how exactly does that get specified in the yacc grammar?

@nikic
Copy link
Contributor Author

nikic commented Jan 19, 2017

You're right about instanceof, I got confused there. The LHS is a normal expression, it's the RHS that is a (restricted) variable.

However, the spec is incorrect about the precedence of instanceof: It should have higher precedence than ! (but lower than ~, so different unary operator are treated differently here).

@mousetraps mousetraps added bug Issue identified by VS Code Team member as probable bug error-handling grammar labels Jan 22, 2017
@Hywan
Copy link

Hywan commented Jan 26, 2017

👍

@mattacosta
Copy link

I also happened to come across the following example that is related to this while looking at an RFC:

!$x = f() should be !($x = f()) instead of (!$x) = f()

TysonAndre added a commit to TysonAndre/tolerant-php-parser that referenced this issue Feb 12, 2018
This does not fix every single edge case.
This only fixes the edge cases for unary operators
on the left hand side of binary operators expecting a variable.

(That probably isn't even every single binary operator or unary operator)
TysonAndre added a commit to TysonAndre/tolerant-php-parser that referenced this issue Feb 12, 2018
This does not fix every single edge case.
This only fixes the edge cases for unary operators
on the left hand side of binary operators expecting a variable.

(That probably isn't even every single binary operator or unary operator)
roblourens added a commit that referenced this issue Mar 18, 2018
For #19: Fix an edge case parsing `=` and `instanceof`
@mattacosta
Copy link

@TysonAndre The affected operators with this restriction are:

  • All assignment operators (it looks like you got all of these)
  • Increment and decrement operators

The instanceof operator is not affected by the restriction. It only appears to be related because the ! operator has the wrong precedence. This means that the PR probably makes instanceof have an incorrect precedence as well, and that part of the change should be reverted.


Also, before anyone goes and thinks about closing this issue, note that the previous PR is just a workaround. It alters the precedence of those operators, but that isn't the cause of the issue. For example, the parser will still parse statements such as 1 = 2 without error.

@TysonAndre
Copy link
Contributor

TysonAndre commented Mar 24, 2018

The instanceof operator is not affected by the restriction. It only appears to be related because the ! operator has the wrong precedence.

That sounds right, after a few checks. I agree that this should be kept open.

I was basing my changes off of the comment from #19 (comment) . But that earlier comment now seems incorrect, according to https://secure.php.net/manual/en/language.operators.precedence.php (Which puts ! as lower precedence than instanceof)

You're right about instanceof, I got confused there. The LHS is a normal expression, it's the RHS that is a (restricted) variable.

I missed/misread the followup comment they made.

  • I had thought that ~$x instanceof T would be parsed as ~($x instanceof T), but it's actually parsed as (~$x) instanceof T. There's no special case. My change probably introduced a bug for ~$x instanceof T.

  • As mattacosta said, the precedence of instanceof (compared to unary operators) should be fixed.

    And more tests should be added.

php > $x = new stdClass();
php > var_export(~$x instanceof stdClass);

Warning: Uncaught Error: Unsupported operand types in php shell code:1

Also, I'd agree that it looks like 1 = 2; is a syntax error (e.g. for getDiagnostics()), not a semantics error. That may be easier to track in a separate ticket.

https://github.com/php/php-langspec/blob/master/spec/10-expressions.md#assignment-operators limits the valid expression types for lhs

php > var_export(ast\parse_code('<?php 2 = 3;', 50));

Parse error: syntax error, unexpected '=' in string code on line 1

TysonAndre added a commit to TysonAndre/tolerant-php-parser that referenced this issue Dec 5, 2018
This fixes the most common cases for microsoft#19
(there may be other edge cases I haven't considered yet)

This also fixes incorrect precedence for `<=>`.
It has the same precedence as `==` -
https://secure.php.net/manual/en/language.operators.precedence.php
has correct information for the `<=>` operator.
TysonAndre added a commit to TysonAndre/tolerant-php-parser that referenced this issue Dec 5, 2018
This fixes the most common cases for microsoft#19
(there may be other edge cases I haven't considered yet)

This also fixes incorrect precedence for `<=>`.
It has the same precedence as `==` -
https://secure.php.net/manual/en/language.operators.precedence.php
has correct information for the `<=>` operator.
TysonAndre added a commit to TysonAndre/tolerant-php-parser that referenced this issue Dec 5, 2018
This fixes the most common cases for microsoft#19
(there may be other edge cases I haven't considered yet)

This also fixes incorrect precedence for `<=>`.
It has the same precedence as `==` -
https://secure.php.net/manual/en/language.operators.precedence.php
has correct information for the `<=>` operator.
TysonAndre added a commit to TysonAndre/tolerant-php-parser that referenced this issue Feb 23, 2019
Fixes one edge case in microsoft#19

Also, fix parsing of `@$x instanceof $y`.
My earlier PR fixed `!$x instanceof $y` but broke that.
TysonAndre added a commit to TysonAndre/tolerant-php-parser that referenced this issue Mar 9, 2019
Fixes one edge case in microsoft#19

Also, fix parsing of `@$x instanceof $y`.
My earlier PR fixed `!$x instanceof $y` but broke that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug error-handling grammar
Projects
None yet
Development

No branches or pull requests

5 participants