On Sun, Aug 16, 2020 at 4:47 PM tyson andre <tysonandre...@hotmail.com> wrote:
> Hi Benjamin, > > > We are looking for further feedback from the community. > > Thanks, the updated RFC looks much better. > Some more feedback on why the edge cases are a concern to me, > and why the lack of an ending delimiter is similar to parsing problems > already faced. > > I'd agree that restarting a two-week discussion period seems excessive, > and a shorter one is fine with me. > Thank you for the feedback, the section on #[] forward compatibility is something I want to improve a bit more today, your feedback helps a lot. > > ---- > > The references section should probably also link to the other 2 RFC > discussion threads > https://externals.io/message/111416 and the RFC discussion thread > https://externals.io/message/111218 > > ---- > > Developers who are new to PHP (e.g. learning by example) or those who > spend most of their time programming in other languages such as Rust or > Java or Golang > might forget/not know that `#` is a supported line comment syntax, or that > attributes were introduced in 8.0 (not an earlier release). > They might see that code using these edge cases passes syntax checks in > PHP 7.4 and 8.0, and assume it'd work the same way in both versions, > leading to published code incorrectly marked as compatible with PHP 7.4. > > - PHP doesn't warn about calling a user-defined function too many > parameters, making this harder to detect. > > ---- > > The `#[]` syntax makes it possible and convenient to use attributes syntax, > but causes various problems for end users of code using attributes syntax. > I'd expect the drawbacks of bugs caused by edge cases such > as those in > https://wiki.php.net/rfc/shorter_attribute_syntax_change#discussion_of_forwards_compatibility_procons > to outweigh the benefits of being able to release attributes immediately. > > Putting attributes on the same line as parameters or closures or anonymous > classes seems natural to do to me, > and I'd expect releases would do it eventually, whether it be immediately > in 8.0, years from now, etc. > > Suppose that package `A/A` supports PHP `7.4|^8.0`. > It depends on a package `B/B` 2.0 which supports PHP 7.4. > `B/B` depends on `C/C` `^4.0|^5.0`, where C/C 4.x supports PHP 7 > and C/C 5.0 drops support for PHP 7. > C/C 5.x uses attribute syntax on the same line as other code, > including several edge cases with one-line attributes that parse > differently in php 7.0. > > ``` > public function doDbMaintenance( > #[MyUnused] bool $log = false, // unused in db backend > bool $doPotentiallyDestructiveUpdate = false, > mixed $extraFlags = null, > ) {...} > > // B/B calls doDbMaintenance($log = true) > ``` > > Then suppose that a maintainer or third-party packager (e.g. for a Linux > distro) of `A/A` > runs composer install with PHP 8.0 before building a package such as a > phar, rpm, or tarball > (or `vendor/` checked into git) from `A/A` 1.0. > They would pull in B/B 2.0 and C/C 5.0, creating a release of A/A that was > buggy > but did not emit any parse, compilation, or runtime warnings if end users > ran that release with php 7.4. > > Then the maintainers of `A/A` and `C/C` 5.0 would get bug reports for PHP > 7.4 for mysterious behaviors with no warnings, > which would be hard to reproduce and debug, despite all involved packages > correctly specifying their dependencies. > End users would also be inconvenienced by bugs that had no obvious > indication or subtler symptoms that may take a long time to get reported. > > This could be worked around by > > 1. Setting `{"config": {"platform": {"php": "7.4.9"}}}` in composer.json > of A/A, > or by documenting that `composer install` should always be run with PHP > 7.4, > but I wouldn't expect new composer users to be aware of the ability to > do that > until they run into issues, and that doesn't help if A/A is a > dependency of another package. > > 2. C/C 5.0 could exit() when bootstrapping when it's run in php 7, > but that seems excessive and not currently done by libraries in my > experience. > > ---- > > If a developer needs to backport code or patches for php 8.0+ to php 7.4 > or older > (e.g. to support legacy applications or OSes that make updating php > impractical), > the lack of a syntax error would make this backporting error prone > if the developer didn't remember this incompatibility > or learn about tools created to catch issues. > (or didn't know about *up-to-date, bug-free* tools to downgrade php syntax) > > ---- > > For > https://wiki.php.net/rfc/shorter_attribute_syntax_change#machine_scanning_for_end_of_attribute_declaration > I'd agree that the lack of an ending delimiter makes token-based > extraction more complicated but still practical, > but it is a complexity that projects such as phpcs **would already face in > similar syntaxes.** > The problems with whitespace and comments and doc comments between tokens > can be fixed by filtering out whitespace and comments before/while looking > to the end of a token. > Projects such as nikic/php-parser or microsoft/tolerant-php-parser would > be what I'd recommend for new code/scripts, > but this is impractical for large existing codebases with third party > plugins such as phpcs. > > A stack-based approach is used for `@[` or `#[`. > A stack-based approach would also be used for `@@Attribute /*comment */ (` > by skipping T_WHITESPACE, T_COMMENT, and T_DOC_COMMENT and ending parsing > of that attribute if the next token isn't `(`. > Skipping whitespace and comments that way already needs to be done to > **correctly** find the end of the function call `myGlobalFunction /* > comment */ (args);` (or `new MyClass /* comment */ (args)`). > For both function calls and attributes, I'd assume style guides would > forbid whitespace and comments before `(`. > > Thanks, > - Tyson
