Re: [RFC] Shorter Attribute Syntax Change RFC 0.2

On Sun, Aug 16, 2020 at 4:47 PM tyson andre <tysonandre775@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