Php-parser: invalid ast for offsetlookup and encapsed

Created on 22 Jan 2019  路  14Comments  路  Source: glayzzle/php-parser

Input:

$var = "string ${juices['FOO']} string";

Encapsedpart node contains variable node, but should contains offsetlookup node.

PHP parser output:

array(
    0: Stmt_Expression(
        expr: Expr_Assign(
            var: Expr_Variable(
                name: var
            )
            expr: Scalar_Encapsed(
                parts: array(
                    0: Scalar_EncapsedStringPart(
                        value: string 
                    )
                    1: Expr_ArrayDimFetch(
                        var: Expr_Variable(
                            name: juices
                        )
                        dim: Scalar_String(
                            value: FOO
                        )
                    )
                    2: Scalar_EncapsedStringPart(
                        value:  string
                    )
                )
            )
        )
    )
)

Expr_ArrayDimFetch is same as offsetlookup

Maybe will be great add tests for propertylookup and staticlookup to ensure ast correct for lookup nodes.

bug AST RELEASE-READY high-pri
Source
picture alexander-akait

All 14 comments

related to https://github.com/glayzzle/php-parser/issues/248#issuecomment-456596682

This is implemented with the fix on the issue, take a look at the implementation rules.

Here the output :

Program {
  "children": Array [
    ExpressionStatement {
      "expression": Assign {
        "kind": "assign",
        "left": Variable {
          "byref": false,
          "curly": false,
          "kind": "variable",
          "name": "var",
        },
        "operator": "=",
        "right": Encapsed {
          "kind": "encapsed",
          "raw": "\\"string \${juices['FOO']} string\\"",
          "type": "string",
          "value": Array [
            EncapsedPart {
              "curly": false,
              "expression": String {
                "isDoubleQuote": false,
                "kind": "string",
                "raw": "string ",
                "unicode": false,
                "value": "string ",
              },
              "kind": "encapsedpart",
            },
            EncapsedPart {
              "curly": false,
              "expression": Variable {
                "byref": false,
                "curly": true,
                "kind": "variable",
                "name": OffsetLookup {
                  "kind": "offsetlookup",
                  "offset": String {
                    "isDoubleQuote": false,
                    "kind": "string",
                    "raw": "'FOO'",
                    "unicode": false,
                    "value": "FOO",
                  },
                  "what": Variable {
                    "byref": false,
                    "curly": false,
                    "kind": "variable",
                    "name": "juices",
                  },
                },
              },
              "kind": "encapsedpart",
            },
            EncapsedPart {
              "curly": false,
              "expression": String {
                "isDoubleQuote": false,
                "kind": "string",
                "raw": " string",
                "unicode": false,
                "value": " string",
              },
              "kind": "encapsedpart",
            },
          ],
        },
      },
      "kind": "expressionstatement",
    },
  ],
  "errors": Array [],
  "kind": "program",
}

The curly property on the EncapsedNode is no more usefull, maybe we could avoid using this tag ? If you have some time to clean it would be cool, in order to avoid depending on extra tags

ichiriac picture ichiriac on 23 Jan 2019

@ichiriac problem was not fixed, we have invalid ast here, as i describe above EncapsedPart should contains OffsetLookup node, not Variable.
This is right ast:

Program {
  "children": Array [
    ExpressionStatement {
      "expression": Assign {
        "kind": "assign",
        "left": Variable {
          "byref": false,
          "curly": false,
          "kind": "variable",
          "name": "var",
        },
        "operator": "=",
        "right": Encapsed {
          "kind": "encapsed",
          "raw": "\\"string \${juices['FOO']} string\\"",
          "type": "string",
          "value": Array [
            EncapsedPart {
              "curly": false,
              "expression": String {
                "isDoubleQuote": false,
                "kind": "string",
                "raw": "string ",
                "unicode": false,
                "value": "string ",
              },
              "kind": "encapsedpart",
            },
            EncapsedPart {
              "curly": false,
              "expression": OffsetLookup {
                "kind": "offsetlookup",
                "offset": String {
                    "isDoubleQuote": false,
                    "kind": "string",
                    "raw": "'FOO'",
                    "unicode": false,
                    "value": "FOO",
                },
                "what": Variable {
                    "byref": false,
                    "curly": false,
                    "kind": "variable",
                    "name": "juices",
                },
              },
              "kind": "encapsedpart",
            },
            EncapsedPart {
              "curly": false,
              "expression": String {
                "isDoubleQuote": false,
                "kind": "string",
                "raw": " string",
                "unicode": false,
                "value": " string",
              },
              "kind": "encapsedpart",
            },
          ],
        },
      },
      "kind": "expressionstatement",
    },
  ],
  "errors": Array [],
  "kind": "program",
}

The curly property on the EncapsedNode is no more usefull, maybe we could avoid using this tag ? If you have some time to clean it would be cool, in order to avoid depending on extra tags

We still need curly, example:

$var = "$this->target"; // simple syntax
$var = "{$this->target}"; // complex syntax

curly mean complex syntax

alexander-akait picture alexander-akait on 23 Jan 2019
👍1

Hi @evilebottnawi, agree with you it's still not clear :smile: - I will focus on this.

$var = "$this->target";
$var = "{$this->target}";
$var = "${$this->target}";

Results into :

Program {
  kind: 'program',
  children:
   [ ExpressionStatement {
       kind: 'expressionstatement',
       expression:
        Assign {
          kind: 'assign',
          operator: '=',
          left: Variable { kind: 'variable', name: 'var', byref: false, curly: false },
          right:
           Encapsed {
             kind: 'encapsed',
             value:
              [ EncapsedPart {
                  kind: 'encapsedpart',
                  expression:
                   PropertyLookup {
                     kind: 'propertylookup',
                     what: Variable { kind: 'variable', name: 'this', byref: false, curly: false },
                     offset: Identifier { kind: 'identifier', name: 'target' } },
                  curly: false } ],
             raw: '"$this->target"',
             type: 'string' } } },
     ExpressionStatement {
       kind: 'expressionstatement',
       expression:
        Assign {
          kind: 'assign',
          operator: '=',
          left: Variable { kind: 'variable', name: 'var', byref: false, curly: false },
          right:
           Encapsed {
             kind: 'encapsed',
             value:
              [ EncapsedPart {
                  kind: 'encapsedpart',
                  expression:
                   PropertyLookup {
                     kind: 'propertylookup',
                     what: Variable { kind: 'variable', name: 'this', byref: false, curly: false },
                     offset: Identifier { kind: 'identifier', name: 'target' } },
                  curly: true } ],
             raw: '"{$this->target}"',
             type: 'string' } } },
     ExpressionStatement {
       kind: 'expressionstatement',
       expression:
        Assign {
          kind: 'assign',
          operator: '=',
          left: Variable { kind: 'variable', name: 'var', byref: false, curly: false },
          right:
           Encapsed {
             kind: 'encapsed',
             value:
              [ EncapsedPart {
                  kind: 'encapsedpart',
                  expression:
                   Variable {
                     kind: 'variable',
                     name:
                      PropertyLookup {
                        kind: 'propertylookup',
                        what: Variable { kind: 'variable', name: 'this', byref: false, curly: false },
                        offset: Identifier { kind: 'identifier', name: 'target' } },
                     byref: false,
                     curly: true },
                  curly: false } ],
             raw: '"${$this->target}"',
             type: 'string' } } } ],
  errors: [],
  comments: [] }

So here the rules (not sure they are quite good) :

$var = "$this->target";
// EncapsedPart with curly = false / expression PropertyLookup
$var = "{$this->target}";
// EncapsedPart with curly = true / expression PropertyLookup
$var = "${$this->target}";
// EncapsedPart with curly = false / expression Variable with curly = true / name as PropertyLookup

Actual rules are :

// handling encapsed
foreach(encapsed.vlaue as EncapsedPart) {
  if (EncapsedPart.curly) output += "{";
  output += generate(EncapsedPart.expression)
  if (EncapsedPart.curly) output += "}"; 
}
// handling variables
if (Variable) {
  if (Variable.curly) {
    output += "${" + generate(Variable.name) + "}"; // name is the expression
  } else {
    output += "$" + Variable.name; // name is the variable identifier
  }
}
ichiriac picture ichiriac on 17 Feb 2019

Here another sample code in order to test genericity :

$var->foo_{$bar};

And the result :

Program {
  kind: 'program',
  children:
   [ ExpressionStatement {
       kind: 'expressionstatement',
       expression:
        PropertyLookup {
          kind: 'propertylookup',
          what: Variable { kind: 'variable', name: 'var', byref: false, curly: false },
          offset:
           Encapsed {
             kind: 'encapsed',
             value:
              [ Identifier { kind: 'identifier', name: 'foo_' },
                Variable { kind: 'variable', name: 'bar', byref: false, curly: false } ],
             type: 'offset' } } } ],
  errors: [] }

Ouch, herewe SHOULD use EncapsedPart on the value array, so thats here where things breaks. The variable curly is false so not affected by ${...}.

I we provide the same implementation with EncapsedPart over PropertyLookup offsets in principle this algorithm remains generic

@evilebottnawi - do you see other edge cases ?

ichiriac picture ichiriac on 17 Feb 2019

@ichiriac looks good, can you send a PR and i will tests branch on prettier side to avoid any edge cases (we have very many tests and fast catch all problems)

alexander-akait picture alexander-akait on 18 Feb 2019
👍1

hi @evilebottnawi, with this last commit the rule is implemented, encapsed should work en every case. Let me know if I'm missing something

ichiriac picture ichiriac on 18 Aug 2019

Thanks, look on this at this week

alexander-akait picture alexander-akait on 19 Aug 2019
👍1

WIP on this, due in encaped we can have { and ${ and they can contains variable https://github.com/php/php-src/blob/php-7.4.0RC1/Zend/zend_language_parser.y#L1269 and https://github.com/php/php-src/blob/php-7.4.0RC1/Zend/zend_language_parser.y#L1276 we need new property like dollar: true/false for ast

alexander-akait picture alexander-akait on 30 Sep 2019

Example (means same):

<?php
$juice = 'test';
$test = 'foobar';

echo "string ${$juice} string";
echo "string {$$juice} string";
alexander-akait picture alexander-akait on 30 Sep 2019

Also curly for simple syntax is opinionated:

<?php
echo "He drank some juice made of $juice.";
echo "He drank some juice made of ${juice}.";
alexander-akait picture alexander-akait on 30 Sep 2019

https://github.com/php/php-src/blob/php-7.4.0RC1/Zend/zend_language_parser.y#L1262 - create variable
https://github.com/php/php-src/blob/php-7.4.0RC1/Zend/zend_language_parser.y#L1270 - create variable

So we need new property for encapsedpart as i written above

alexander-akait picture alexander-akait on 30 Sep 2019

hm, maybe wen can refactor to one variable: "syntax: curly | dollar | null"

  1. curly - "test {$test}";
  2. dollar - "test ${test}";
  3. null - "test $test";
alexander-akait picture alexander-akait on 30 Sep 2019
👍1

/cc @ichiriac any ideas? I known we always can refactor it in future, just want some feedback

alexander-akait picture alexander-akait on 30 Sep 2019

I'm agree with you

ichiriac picture ichiriac on 1 Oct 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

alexander-akait picture alexander-akait  路  7Comments
Rivendall picture Rivendall  路  6Comments
mgrip picture mgrip  路  3Comments
alexander-akait picture alexander-akait  路  9Comments
alexander-akait picture alexander-akait  路  8Comments