PHP 8.3: "yield /*comment*/ from" is no longer a parse error ?

[jrfnl]

Description

The following code:

<?php
function generator()
{
    yield from gen2();

    yield /* comment */ from gen2();

    yield
    from
    gen2();

    yield // comment
	from gen2();

    yield
    /* comment */
    from
    gen2();
}

https://3v4l.org/2SI2Q#veol

Resulted in this output:

_no output_

But I expected this output instead:

Parse error: syntax error, unexpected identifier "gen2" in /in/2SI2Q on line 7

For a full analysis of the differences in tokenization, see: PHPCSStandards/PHP_CodeSniffer#529 (comment)

While I'm not necessarily challenging this change, I'd like to know if this was a deliberate/intentional change.

I have not been able to find anything in the PHP 8.3 CHANGELOG about this change, nor in the NEWS file. I can't even seem to find the commit which caused this change.

If this change was intentional, this is probably a documentation issue and the change should be annotated in the PHP 8.3 CHANGELOG.

If this change was unintentional, I believe it may be prudent to revert the change (or at least revert the side-effects of the commit which incidentally caused this change).

I'd like to get some clarity about this as it will inform how the PHP_CodeSniffer issue linked above should be fixed.

PHP Version

PHP 8.3.8

Operating System

Not relevant

[damianwadley]

This was changed by @iluuu1994 during f291d37 in response to #10083...

Tip: for grammar questions, one of the first places to check for answers is zend_language_scanner.

[nielsdos]

Damnit, beat me on commenting literally by seconds 😂
Indeed, this is a side effect of the bugfix and imo not a bug.

[jrfnl]

I do kind of consider it a bug as it is an undocumented change in behaviour - the changelog entry really doesn't cover the potential/actual impact of the change.

Knowing where this is coming from, I'm surprised I haven't seen more breakage due to this change in PHPCS. Then again, that would require the PHPCS tests to hit the changed tokenization behaviour and as this kind of thing was previously a parse error, I can understand why this is not covered by the existing tests.

If this change stays, in my opinion, the changelog entry needs to be updated to more comprehensively state what has actually changed and what the impact of this. I also think it should be mentioned in the CHANGELOG, not just in NEWS.

Additionally, I'd recommend for tests to be added with the above code sample for yield from to prevent a regression in the future. Preferably tests which would not just check that the above code sample is not regarded as a parse error, but tests which also check the tokenization is consistent.

[jrfnl]

P.S.: Thanks @damianwadley and @nielsdos for your fast response to my query!

[cmb69]

Since this affects PHP 8.3, I'm not sure whether updating CHANGELOG makes much sense now, but it's certainly worth documenting it in the migration guide of the PHP documentation.

[iluuu1994]

@jrfnl I indeed didn't think about linters, this should have gotten an UPGRADING entry. Sorry about that.

[mvorisek]

IMO it is not a bug, as comments are (should be) allowed between any code token in general.

This is probably already documented or at least it is "generally expected". So f291d37 is a fix.

[iluuu1994]

@mvorisek What Juliette references is the patch changing the output of PhpToken::tokenize(), with the tokenizer treating yield /* comment */ from as a singular token. This is the only feasible solution without switching to a GLR parser.

[mvorisek]

That is indeed not good and such token should be broken into separate tokens further.

repro: https://3v4l.org/7VmX1

[jrfnl]

Forgive me if I don't use the correct terminology for some things, I'm hoping that even if the terminology is not 100% correct for C, you can still understand what I'm trying to say.

IMO it is not a bug, as comments are (should be) allowed between any code token in general.

... which is exactly the reason why I was testing the Tokenizer behaviour with comments between the words.

Having said that, it's also not true....

In nearly all cases, tokens consist of a "single entity", so no comments can exist within a token.
This includes "compound operators", like ??, where no whitespace or comment is allowed between the first and the second ?.

A typical case where the tokenization changed from multiple tokens to single token is the PHP 8.0 "namespaced names" change.
However, that change also explicitly removed the ability to have whitespace or comments within a namespaced name.

Another one which comes to mind are cast tokens, like (bool). These tokens can contain whitespace, i.e. ( bool ), but they cannot contain comments. See: https://3v4l.org/A6Sgj and https://3v4l.org/nE9H8

So, in practice, yield from is (AFAICS) the only "multi-entity" token which can contain comments... (since PHP 8.3).

Retrospectively, it would have been better if yield from would have been introduced as yield_from (note the underscore). This would be in line with other multi-word keywords, like include_once or as yieldfrom, which would be in line with keywords like endif, but considering it has been in the language for such a long time now, changing this now would be a huge BC-break.

I consider the change in f291d37 a bug as it is an undocumented (breaking) change in behaviour for the Tokenizer for projects consuming the token output.
That bug could be fixed by either fixing/updating the documentation or by reverting the change.

So, what remains then is the discussion about how yield from should be tokenized.

I'm not sure about this and you can also see some thoughts about this in the linked PHPCS ticket (as PHPCS needs to ensure the token stream sniffs receive is the same on all PHP versions, so I need to make a choice on how to get round the changed tokenization).

On the one hand I agree that comments between the keywords should be ignored - as they mostly are elsewhere -, on the other hand, comments cause a parse error in other "multi entity" tokens, so this change introduces an inconsistency.

[iluuu1994]

In nearly all cases, tokens consist of a "single entity", so no comments can exist within a token. This includes "compound operators", like ??, where no whitespace or comment is allowed between the first and the second ?.

That's true, but what qualifies as a token is usually very intuitive. yield from is one of the few exceptions, and it's primary rationale for being a single token to avoid making from itself a keyword.

So, in practice, yield from is (AFAICS) the only "multi-entity" token which can contain comments... (since PHP 8.3).

I think that's correct. The other cases only use lookahead for comments.

So, what remains then is the discussion about how yield from should be tokenized.

Unfortunately, for yield from to not cause parser ambiguitites, it must be a single token. Hence, when a comment does appear in that spot, it is implicitly part of the token. Since 8.3 has been out for a while, I don't think removing it now is feasible, and if it is removed from 8.4, then that still leaves 8.3 as an issue for you.

It might be possible for you to artificially split yield /* comment */ from into multiple tokens. It's not great to push that responsibility onto users, but I don't see a great alternative at the moment. If you have any other ideas that might help with your situation, please let me know.

[jrfnl]

Unfortunately, for yield from to not cause parser ambiguitites, it must be a single token. Hence, when a comment does appear in that spot, it is implicitly part of the token.

And that's what I don't necessarily agree with - and nor did PHP 7.0 - 8.2 in which this was a parse error.

Since 8.3 has been out for a while, I don't think removing it now is feasible

And again, I don't agree with this. I only discovered this bug now as the change wasn't documented. That doesn't make it any less of a bug/inconsistency with every single other token, as none of them allow for comments within the token.

If it had been included in the changelog, I would probably have questioned the change/introduction of this inconsistency before the release was out.

Note: I'm only challenging the change to the yield from tokenization. Not the other changes in the same commit.