Preserve heredoc/nowdoc formatting

[theofidry]

Whilst upgrading PHP-Scoper to PHP-Parser 5, I've run into the issue of https://github.com/sebastianbergmann/phpunit/issues/5855.

This is caused by this change:

For PHP >= 7.3 (default), heredoc/nowdoc strings are now indented just like regular code. This was allowed with the introduction of flexible heredoc/nowdoc strings.

Previously the parser was preserving the formatting while possible, this does not seem to be the case anymore. I assume this was a deliberate change in the printer to simplify it and keep the old behaviour with Printer::printFormatPreserving().

With PHP-Scoper however, I very easily run into the issue of the printer bailing out (if preserving format), which to be honest is too surprising considering the changes to the AST are not small (you can find plenty of examples here, but the gist of it is, changing names, adding namespaces and class_alias() statements or function_exists() statements.).

Is printFormatPreserving() the way to go in which case more fixing needs to be done to the printer or is there another way?

[nikic]

I don't think this is related to formatting preservation at all (it is never preserved by the default printer) -- it sounds like you are printing for the default version of PHP 7.4, which assumes flexible doc strings are supported. If you want to generate PHP 7.2 compatible code, you can do so by passing a PhpVersion::fromComponents(7, 2) object to the pretty printer.

[theofidry]

I don't think this is related to formatting preservation at all (it is never preserved by the default printer)

Maybe this was not tested (on my end) in PHP Parser v4 but wasn't it the case before? I would expect to have run into this issue much sooner otherwise as PHP-Scoper has never been compatible with PHP 7.2 code and the PHP version used is the host version

[theofidry]

Ok I'm comparing my PHP-Parser v4 code:

$parser = (new ParserFactory())->create(
    ParserFactory::PREFER_PHP7,
    new Lexer(),
);

$printer = new StandardPrinter(
    new Standard(),
);

The current code is:

$phpVersion = PhpVersion::getHostVersion();

$lexer = $version->isHostVersion() ? new Lexer() : new Emulative($version);

$parser =$version->id >= 80_000
    ? new Php8($lexer, $version)
    : new Php7($lexer, $version);

$printer = new StandardPrinter(
    new Standard([
        'phpVersion' => $phpVersion,
    ]),
);

I can confirm that for the PHP-Parser v4 the following code:

<?php

function foo()
{
    $x = <<<'PHP'
$foo

PHP;
}

is NOT transformed into:

<?php

namespace PhpScoperPrefix;

function foo()
{
    $x = <<<'PHP'
$foo

PHP;
}

but instead preserved:

<?php

namespace PhpScoperPrefix;

function foo()
{
    $x = <<<'PHP'
    $foo

    PHP;
}

If you want to generate PHP 7.2 compatible code, you can do so by passing a PhpVersion::fromComponents(7, 2) object to the pretty printer.

I'll add the feature to be able to forcefully set a PHP version, but otherwise currently PHP-Scoper just picks the host version and it worked fine because things like heredocs were not reformatted unless necessary.

[nikic]

I don't think this is related to formatting preservation at all (it is never preserved by the default printer)

Maybe this was not tested (on my end) in PHP Parser v4 but wasn't it the case before? I would expect to have run into this issue much sooner otherwise as PHP-Scoper has never been compatible with PHP 7.2 code and the PHP version used is the host version

PHP-Parser 4 did not support specifying the PHP version for the pretty printer -- the produced code was always the "most compatible", with an effective target version of PHP 5.3. You can go back to the old behavior by explicitly specifying that as the version. Possibly that's appropriate for PHP-Scoper, as the code is not intended to be read by anyone anyway.

[theofidry]

You can go back to the old behavior by explicitly specifying that as the version

But wouldn't that result in the opposite, i.e. making indented heredoc into non-indented heredocs? And work strangely for code that is 8.3 and for 8.3?

[nikic]

You can go back to the old behavior by explicitly specifying that as the version

But wouldn't that result in the opposite, i.e. making indented heredoc into non-indented heredocs? And work strangely for code that is 8.3 and for 8.3?

It would turn indented into non-indented heredoc, yes -- that's exactly what PHP-Parser 4 did. The non-indented version works on all PHP versions. (Well, all PHP versions that have heredoc.)

[theofidry]

It is not ideal as I would prefer to minimise the diff between the source and scoped code, but I guess if I do want that the printFormatPreserving() would be more appropriate?

I have about 1/3 of the test suites that fail with this mode, so is it just that I'm breaking too much the existing code, am I doing something wrong or is it bugs I should report?

[nikic]

It is not ideal as I would prefer to minimise the diff between the source and scoped code, but I guess if I do want that the printFormatPreserving() would be more appropriate?

Yes.

I have about 1/3 of the test suites that fail with this mode, so is it just that I'm breaking too much the existing code, am I doing something wrong or is it bugs I should report?

What does "fail" mean here? It generates incorrect code? Output is different because your test expectations used different formatting?

[theofidry]

What does "fail" mean here? It generates incorrect code? Output is different because your test expectations used different formatting?

Hmm I misread some the output. Ok no most of the errors are due to the <?php tag not being added: https://github.com/humbug/php-scoper/pull/1051/files#diff-97fed5ab6603b5e3a8db515c6b3aa438821ccaa44f41e80a9a0d961d61f7f577

Which easily explains all the e2e test failures

[nikic]

Hm, that's weird. printFormatPreserving is supposed to include the opening PHP tag.

[theofidry]

ok, if thats unexpected ill debug it to see what's going on

[nikic]

Closing this as I think the part about heredoc/nowdoc has been answered. Please open a new issue for the <?php tag issue if it's still relevant...