feat: format new without parenthesis in PHP 8.4

[claytonrcarter]

This adds support for formatting things like (new Foo())->bar as new Foo()->bar. Support for this syntax was added in PHP 8.4 and very recently merged and released in php-parser.

Note that php-parser 3.2.2 introduced a parser bug that affects how we format specifically placed comments in class constant assignments. My opinion is that this is an extreme edge case issue and it's worth the temporary regression in order to gain support for one of the highlighted features of php 8.4. (There is already a pending PR to fix this in php-parser, and we can rollout an update to fix it here when that lands. edit: this has been fixed

• bumps php-parser to the newly released v3.2.3
• breaking: removes (temporarily) support for some comments in class constant assigments
• adds a new phpVersion option for 8.4
• adds support for the above syntax, with tests
• misc light refactoring to support these changes

Verification

1. I used this to format all text fixture files and confirmed that now (relevant) issues were introduced.
2. I used this to format a small Laravel project of mine (300+ files, 27k lines of code) and confirmed that the tests still passed.

Try it out

<?php

class Foo 
{
    public $bar = 123;
}

echo (new Foo)->bar;

This should be reformatted to

-echo (new Foo)->bar;
+echo new Foo()->bar;

And php foo.php should print 123 in both cases (though the latter will fail in php <= 8.3).

[claytonrcarter]

Marking as draft for now because I want to do some additional testing, and it looks like there are CI failures.

[czosel]

Looks really good! 💯 I'm hopeful that we can land glayzzle/php-parser#1152 in the coming days - let's merge this afterwards, without the regression 🙂

[cseufert]

@czosel I have rebased this to fix the package.json/yarn.lock conflict, tests run fine locally, but CI fails, and I cant reproduce that locally. Might need to recreate the PR?

[claytonrcarter]

Thank you for nudging me on this (I had been away). I've updated the PR to use php-parser 3.2.4 and dropped the feature regression related to glayzzle/php-parser#1152. I am also seeing CI failures, but I recall from when I first started this that some of those may be related to parsing the AST. I'll try to dig in and share what I find.

[claytonrcarter]

I can reproduce the CI failure locally when running AST_COMPARE=1 yarn test:standalone (which is what CI does).

I'm seeing 2 failures:

1. there is a diff in the AST between the old and new files
2. the new files are failing to parse

Here's an example of # 1:

        Object {
        "expression": Object {
            "kind": "assign",
            "left": Object {
            "curly": false,
            "kind": "variable",
            "name": "class",
            },
            "operator": "=",
            "right": Object {
+           "kind": "bin",
+           "left": Object {
                "arguments": Array [],
-           "kind": "call",
+             "kind": "new",
                "what": Object {
-             "kind": "propertylookup",
-             "offset": Object {
-               "kind": "identifier",
-               "name": "veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongMethod",
+               "kind": "name",
+               "name": "Foo",
+               "resolution": "uqn",
+             },
            },
-             "what": Object {
+           "right": Object {
                "arguments": Array [],
-               "kind": "new",
+             "kind": "call",
                "what": Object {
                "kind": "name",
-                 "name": "Foo",
+               "name": "veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongMethod",
                "resolution": "uqn",
                },
            },
-           },
+           "type": "->",
            },
        },

I don't understand all of the details in that, but it looks like (new Foo)->method() is treated as a binary expression with a call expression, while new Foo()->method() is treated as just a call expression. It seems to me that this is the intended behavior with this change, but I don't know how to work around that in CI. Perhaps we need to update the AST tests to ... what? Allow specific ART diff regressions for specific files and/or versions? Or we could snapshot the the intended diff and use that to see if a diff is "known"? I'm stabbing in the dark on this; and input is welcome!

As for # 2, an example error is

/Users/crcarter/src/Other/plugin-php/tests/parens/new.php parse
    SyntaxError: Parse Error : syntax error, unexpected '[', expecting ';' on line 34
      32 | $var = new $foo()->bar;
      33 | $var = new $bar->y()->x;
    > 34 | $var = new foo()[0];
         |                ^
      35 | $var = new foo()[0]["string"];
      36 | $var = new $a->b();
      37 | $var = new $a->b();

In this case, the input snippet $var = (new foo)[0]; has been reformatted to $var = new foo()[0]; (which is valid in PHP 8.4). My hunch is that the parser is trying to parse the code fixtures as <8.4, but I am attempting to pass the correct version to the parser already.

I'll keep tinkering with this; but if someone can confirm that I'm on the right track (or not!) that would be really helpful. Thank you!

[czosel]

@claytonrcarter thanks for looking into this! Concerning the differences in AST, you'll have to "clean those up": see

plugin-php/src/clean.mjs

Line 26 in 14f802a

function clean(node, newObj) {

I'll take a look at the parser error and get back to you.

[czosel]

I could reproduce the issue in the php-parser repository by adapting a test case. I created an issue for this: glayzzle/php-parser#1160

[claytonrcarter]

OK, it sounds like we may have to wait for glayzzle/php-parser#1160. In the meantime, I'm looking at cleaning up the AST and I have some questions:

1. clean() doesn't appear to let me compare a before/after of a node, so I can only examine the "after" node, figure out if it is a node that needs adjusting, and then adjust it to match the "before" node. Correct?
2. If so, I understand that clean() is only used for testing, correct? So it's OK if I couple/hard-code some values based on specific test failures in clean()? Otherwise it seems like it may be difficult to accurately determine that node that is parsed as a bin node after formatting corresponds to a node that is parsed as an call node before formatting.
3. Is it correct that I should be using clean() to fundamentally alter the AST nodes just to get CI to pass? For example, in one case, the "before" node is a call propertylookup on new while the "after" is parsed as binary -> left: new, right: call name. I'm working on this, but it smells odd that I'm rewriting the AST node so significantly.

Again, just want to confirm that I'm on the right track. Thanks.

Update here's an example of what I'm talking about:

Expand to show example

The code snippet

$class = (new Foo)->veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongMethod();

is reformatted in 8.4 to

$class = new Foo()->veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongMethod();

But these are parsed differently by the parser. The RHS of the assign in the former is

    {
      arguments: [],
      kind: "call",
      what: {
        kind: "propertylookup",
        offset: {
          kind: "identifier",
          name: "veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongMethod",
        },
        what: {
          arguments: [],
          kind: "new",
          what: {
            kind: "name",
            name: "Foo",
            resolution: "uqn",
          },
        },
      },
    }

while for the latter it's more like this (approx)

    {
      kind: "bin",
      type: "->",
      left: {
        arguments: [],
        kind: "new",
        what: {
          kind: "name",
          name: "Foo",
          resolution: "uqn",        
      },
      right: {
        arguments: [],
        kind: "call",
        what: {
          kind: "name",
          name: "veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongMethod",
            resolution: "uqn",
        },
      },
    }

So they are parsed as substantially different nodes, and – to get them to match – I'm currently just returning a hard-coded JS object of the former. This works, but it feels wrong. Am I misunderstanding something about this approach?

Note that there seem to be only 4 cases that fail like this, so it's not many, but they will all need this sort of fussy hard-coded replacement. Again, unless I'm misunderstanding how I should be doing this.

[czosel]

Generally, I think you're on the right track. The fact that new Foo->bar() is parsed as a binary expression (bin) surprises me a little though. I would have expected a call, because that's also what you get when you leave new out: https://astexplorer.net/#/gist/d91ad587337680134cada16dd251935a/3491aa3bdfa356b66b171084bb67826507d625dd

When parsing new Foo()->some_method(); with https://github.com/nikic/php-ast under PHP 8.4, one also gets a METHOD_CALL (and no binary expression). I think this might also need a change in the parser.

cc @cseufert @genintho

object(ast\Node)#1 (4) {                                                                                                                                                                
  ["kind"]=>                                                                                                                                                                            
  int(132)   // <--- STMT_LIST                                                                                                                                                                             
  ["flags"]=>                                                                                                                                                                           
  int(0)                                                                                                                                                                                
  ["lineno"]=>                                                                                                                                                                          
  int(1)                                                                                                                                                                                
  ["children"]=>                                                                                                                                                                        
  array(1) {                                                                                                                                                                            
    [0]=>                                                                                                                                                                               
    object(ast\Node)#2 (4) {                                                                                                                                                            
      ["kind"]=>                                                                                                                                                                        
      int(768) // <--- METHOD_CALL                                                                                                                                                                         
      ["flags"]=>                                                                                                                                                                       
      int(0)                                                                                                                                                                            
      ["lineno"]=>                                                                                                                                                                      
      int(2)                                                                                                                                                                            
      ["children"]=>                                                                                                                                                                    
      array(3) {                                                                                                                                                                        
        ["expr"]=>                                                                                                                                                                      
        object(ast\Node)#3 (4) {                                                                                                                                                        
          ["kind"]=>
          int(527) // <--- NEW                                                                                                                                                                         
          ["flags"]=>
          int(0)
          ["lineno"]=>
          int(2)
          ["children"]=>
          array(2) {
            ["class"]=>
            object(ast\Node)#4 (4) {
              ["kind"]=>
              int(2048)
              ["flags"]=>
              int(1)
              ["lineno"]=>
              int(2)
              ["children"]=>
              array(1) {
                ["name"]=>
                string(3) "Foo"
              }
            }
            ["args"]=>
            object(ast\Node)#5 (4) {
              ["kind"]=>
              int(128)
              ["flags"]=>
              int(0)
              ["lineno"]=>
              int(2)
              ["children"]=>
              array(0) {
              }
            }
          }
        }
        ["method"]=>
        string(11) "some_method"
        ["args"]=>
        object(ast\Node)#6 (4) {
          ["kind"]=>
          int(128)
          ["flags"]=>
          int(0)
          ["lineno"]=>
          int(2)
          ["children"]=>
          array(0) {
          }
        }
      }
    }
  }
}

[czosel]

@claytonrcarter After spending a few more minutes thinking about this, I'm relatively convinced that the AST representation of

• (new Foo)->bar(); and
• new Foo->bar();

should actually be the same, namely a call: https://astexplorer.net/#/gist/d91ad587337680134cada16dd251935a/27dad8f7fd43d10305d94f264323455a27640143

Which would make the AST cleanup obsolete 👍

[hackel]

@claytonrcarter Thank you so much for working on this!

I just ran your branch on my codebase and observed the following:
It's inlining the first chained method call for some reason. e.g.

new A()
    ->b()
    ->c()
    ->d();

becomes

new A()->b()
    ->c()
    ->d();

I'm not sure if this is covered by what you're already working to solve above, or warrants a new test. It also really likes to move assignments to the next line like this:

    $asdf =
        new A()->b()
            ->c();

This causes yet another level of indentation which I don't think is wanted. Those are the only issues I had in our fairly large codebase, though (~1700 files). Really looking forward to seeing this merged!

[claytonrcarter]

This now includes the fix in glayzzle/php-parser#1162, and ... CI is passing! No AST massaging needed! Thank you all for your help and guidance; truly a team effort!

@hackel Ryan, I added a some code to the snapshots based on your examples and they seem to formatting as you would expect. Can you can try this out again on your code base and see if it's any better?

[claytonrcarter]

CI is passing

Well ... almost passing. Codecov seems to be flagging me on changes to util.mjs, even though it looks like my only change to that file is covered. I may not be reading the report correctly, though. Suggestions?

[cseufert]

CI is passing

Well ... almost passing. Codecov seems to be flagging me on changes to util.mjs, even though it looks like my only change to that file is covered. I may not be reading the report correctly, though. Suggestions?

I dont think codecov knows what is going on there. Here is the same thing happening on changing the default parser version:
https://app.codecov.io/gh/prettier/plugin-php/pull/2434/indirect-changes

[czosel]

I dont think codecov knows what is going on there. Here is the same thing happening on changing the default parser version: https://app.codecov.io/gh/prettier/plugin-php/pull/2434/indirect-changes

I've stopped caring about what codecov is reporting ... 🙈

[czosel]

Apart from the last minor question, this is ready to be merged from my point of view 💯
cc @cseufert

[hackel]

@claytonrcarter I ran the new code and it all looks great on my end. Great job!

[czosel]

This took some iterating, but was well worth it 👍 thanks @claytonrcarter and @cseufert 💯

[czosel]

Released in v0.23.0 🎉