php / doc-en

English PHP documentation
508 stars 740 forks source link

Please clarify that asymmetric visibility scope must not be surrounded by spaces #4126

Closed LazyDevStudio closed 2 days ago

LazyDevStudio commented 5 days ago

The documentation never mentions that the scope keyword for asymmetric visibility must not be surrounded by spaces inside its parentheses: https://www.php.net/manual/en/language.oop5.visibility.php#language.oop5.visibility-members-aviz

E.g. private( set ) string $prop with spaces inside the parentheses causes a parse error, while private(set) string $prop withOUT the spaces surrounding "set" works.

This is unexpected behaviour as adding spaces inside grouping parentheses in php is usually fine. Users should be warned.

damianwadley commented 5 days ago

Is this a documentation problem? I can't think of a reason why whitespace shouldn't be permitted there... Feels like more of an oversight in the grammar.

LazyDevStudio commented 5 days ago

I think it's weird too but since all the examples were written without the spaces, it seemed intentional...? Sounds like it would be a good idea to post a bug report for this?

Girgias commented 5 days ago

@iluuu1994 @Crell can you clarify?

Crell commented 4 days ago

Without spaces is correct. Ilija can speak to how the parser handles it currently, but I suspect allowing spaces would be difficult. Assuming it is, I think we just document it. If it's easy to change, I'm open to arguments for doing so but I don't think it's worth what is technically a feature change in mid-release.

iluuu1994 commented 4 days ago

There's an open issue here somewhere where this was discussed. This is expected, I would consider this whole token a singular keyword.

LazyDevStudio commented 4 days ago

I'm sure you've had very good reasons for implementing it like this and mid-release changes are hard to stomach for sure. It makes sense that it's meant to be one keyword, although the entire RFC does not mention anything about spaces. Personally, I would have expected it to be a compound verb ("privateSet", "private-set", or "private set") rather than half of it surrounded by parentheses like a method/function. In any case, documenting that spaces around it are invalid in the docs would be great :-) . Not even Intellij got this and auto-formats the tokens by adding the spaces around the scope causing parsing errors (I filed an issue for this).

iluuu1994 commented 4 days ago

Mentioning in the docs that there must be no spaces is certainly reasonable.

iluuu1994 commented 4 days ago

Also, to clarify: It's not that allowing spaces specifically is hard. This can be achieved in two ways:

There was just a big discussion on a change in 8.3 about how yield from now allows comments between the two words, which is still being parsed as a singular token. By extension, if we allow spaces, it would be reasonable to expect comments to also be allowed, which will just lead to the same issue, which has not yet reached consensus about what the correct behavior is.

As for the former approach, another potential risk is running into future grammar ambiguities for no good reason, which is another reason why I wanted to avoid it. Edit: Oh, and possibly set itself would also need to become a keyword of some sorts, at least.