Php-parser: Avoid `parenthesis` and `cast` nodes in ast

[alexander-akait]

Example how this do babylon https://astexplorer.net/#/gist/56cf77073a7a8dcc4a785a423c26a1c7/209de623aac9c972645542acfcefdcf67a7bceb1

Why? Example from our code (print return node):

 case "return": {
      const parts = [];

      parts.push("return");

      if (node.expr) {
        const printedExpr = path.call(print, "expr");

        if (node.expr.kind === "bin") {
          parts.push(
            group(
              concat([
                ifBreak(" (", " "),
                indent(concat([softline, printedExpr])),
                softline,
                ifBreak(")")
              ])
            )
          );
        } else {
          parts.push(" ", printedExpr);
        }
      }

      if (hasDanglingComments(node)) {
        parts.push(
          " ",
          comments.printDanglingComments(path, options, /* sameIndent */ true)
        );
      }

      return concat(parts);
}

Look on node.expr.kind === "bin", in some cases people can have parenthesis and our check does not works correctly and you will get bad output (ugly). Adding check on parenthesis and parenthesis.inner can solve this problem, but it is require a lot of code and new checks (in some places it is really a lot of code). Better use logic as in prettier for js. They have ast without parenthesis and print nodes as is and then add parens basic on own logic for parenthesis (https://github.com/prettier/prettier/blob/master/src/language-js/needs-parens.js#L43).

Same for cast node.

All this would allow us to work in a beautiful form with the ast.

[ichiriac]

related to #62 - avoiding inner blocks - I've replaced the parenthesis node with a parenthesizedExpression property available on every expression based node.

This change definitively breaks backward compatibility but it's a great move towards as it's simplify the AST structure and resolves too many parenthesis usage like $a = ((($b)));

[ichiriac]

I've closed the issue too early, did not answer on cast nodes. For the parenthesis part it's released under 3.0.0-prerelease.1 on npm.

For the cast I'm not sure how to handle them - it's not the same case, unlike the parenthesis it's doing an action - whereas the parenthesis role is just to handle precedence.

Any idea or example @evilebottnawi ?

[alexander-akait]

@ichiriac

Problem.
Input:

function foo() {
    $var = $veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar . '0000000';
}

We check right part on bin node and use print logic for bin.

Output:

function foo() {
    $var =
        $veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar .
        "0000000";
}

But when we have cast node:

function foo() {
    $var = (int) $veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongVar . '0000000';
}

We don't have bin node now and our print logic for bin nodes do not work. Check each node on cast node very bad (a lot of lines of code only on checks). It looks a little dirty ast in our case. Also in some cases we can don't print (int) when we have number in right part ($a = (int) 100;). Will be great have cast as property of node.

Example:

$a = (int) 1;

Now we have:

      "right": {
        "kind": "cast",
        "loc": {
          "source": null,
          "start": {
            "line": 3,
            "column": 5,
            "offset": 12
          },
          "end": {
            "line": 3,
            "column": 12,
            "offset": 19
          }
        },
        "type": "int",
        "what": {
          "kind": "number",
          "loc": {
            "source": null,
            "start": {
              "line": 3,
              "column": 11,
              "offset": 18
            },
            "end": {
              "line": 3,
              "column": 12,
              "offset": 19
            }
          },
          "value": "1"
        }
      }

Better:

      "right": {
        "kind": "number",
        "cast": "int",
        "loc": {
          "source": null,
          "start": {
            "line": 3,
            "column": 5,
            "offset": 12
          },
          "end": {
            "line": 3,
            "column": 6,
            "offset": 13
          }
        },
        "value": "1"
      }

It is reduce our checks, ast looks more cleaner and we also save all information to print/transform ast.

[ichiriac]

I understand better the problem now. I have to handle another problem in order to be sure what to do, actually the cast precedence #172 is incorect (not handled at all) and I need to address this problem first.

In both cases, you're right, you only need the cast information as an attribute, so what you can do, is to use the parser with this configuration :

      parser: {
          read_expr_cast: function(cast) {
            const rawCast = this.text();
            const expr = this.next().read_expr();
            expr.cast = cast;
            expr.rawCast = rawCast;
            return expr;
          }
          // + other parser options
        }

This way you can rewrite any parsing function, so I've made a function specialy for cast nodes and added it to snapshots in order to avoid breaking changes on future releases.

With this kind of atomic function you can still diverge from the parser implementation and continue to integrate bugfixes on each release, the catch is to be sure that these functions are atomic enough and don't introduce parsing logic.

[alexander-akait]

@ichiriac thanks!

[ichiriac]

Ok, I can close it now

[alexander-akait]

@ichiriac can we do same for silent node?

[ichiriac]

@evilebottnawi yes, it's a good idea, it's opened here #195