Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Short Closures spec to remove space after fn keyword #53

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

theodorejb
Copy link
Contributor

@theodorejb theodorejb commented Nov 10, 2022

Per the accepted Arrow Functions RFC and per its author, short closures were intended to be written without a space after the fn keyword. This is also the default formatting enforced by PhpStorm.

PHP-CS-Fixer was an outlier which incorrectly enforced spacing after the fn keyword using the same setting as for the function keyword. This was a bug and has been corrected with the addition of a separate closure_fn_spacing setting (though this setting still defaults to one for backwards compatibility - the plan was to change it to enforce no space in the next major 4.0 version). But the current PER coding style spec may get in the way of this plan.

Since the primary goal of arrow functions is to avoid the verbosity of anonymous functions, requiring a space after the fn keyword is a step backwards, and also doesn't align with general usage promoted by PhpStorm as well as the accepted PHP RFC and documentation.

The section on Short Closures was not added until the 17th of July by #17, which was after the 1.0.0 spec was approved/released on June 9, so I'm hoping it's not too late to correct it.

Per the accepted [Arrow Functions RFC](https://wiki.php.net/rfc/arrow_functions_v2) and [per its author](https://www.reddit.com/r/PHP/comments/fctb18/comment/fjd9u6a/), short closures were intended to be written without a space after the `fn` keyword. This is also the default formatting enforced by PhpStorm.

PHP-CS-Fixer was an outlier which incorrectly enforced spacing after the `fn` keyword using the same setting as for the `function` keyword. This was a bug and has been corrected with the addition of a separate `closure_fn_spacing` setting (though this setting still defaults to `one` for backwards compatibility - the plan was to [change it to enforce no space](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/b577444c38b61f2fdc32cd0a4685bd595be391b7/src/Fixer/FunctionNotation/FunctionDeclarationFixer.php#L235) in the next major 4.0 version). But the current PER coding style spec may get in the way of this plan.

Since the primary goal of arrow functions is to avoid the verbosity of anonymous functions, requiring a space after the `fn` keyword is a step backwards, and also doesn't align with general usage promoted by PhpStorm as well as the accepted PHP RFC and documentation.

The section on Short Closures was not added until the 17th of July by php-fig#17, which was after the 1.0.0 spec was approved/released on June 9, so I'm hoping it's not too late to correct it.
@Crell
Copy link
Collaborator

Crell commented Nov 11, 2022

I quite hate the extra space, so if we can change the rule to not have it (the currently published version doesn't mention it at all, so nothing is finalized yet) I am fully on board. Hell, I'd get rid of the space on long-closures too, if we could. 😄

@jrfnl
Copy link
Contributor

jrfnl commented Nov 11, 2022

Independently of personal tastes, this introduces an inconsistency in the standard with long closures. As this is supposed to be a coding style standard, consistency is key, so I'm not sure this is a good idea....

@theodorejb
Copy link
Contributor Author

@jrfnl Arrow functions have a unique syntax from full closures, aimed at conciseness, so I don't think it's inconsistent for them to have their own spacing rule.

@KorvinSzanto
Copy link
Contributor

KorvinSzanto commented Nov 21, 2022 via email

@theodorejb
Copy link
Contributor Author

Note: I also checked the PHP Prettier plugin and found that it, like PhpStorm, formats arrow functions without a space after the fn keyword. Example.

@KorvinSzanto
Copy link
Contributor

Looks good to me, one thing that pops out at me is the following is incorrect:

The fn keyword MUST be preceded by a space

for example consider something like https://3v4l.org/SRSVo:

foo(fn(string $baz) => "Input: $baz");
// vs
foo( fn(string $baz) => "Input: $baz");

but this can be sorted out in a separate PR

@jrmajor
Copy link

jrmajor commented Nov 27, 2022

Can we actually make some research on how arrow functions are used in the wild? PSR-12 team conducted a survey, gathering votes from 142 people including 17 project representatives. For this PR, I can see 4 people, including one being against this change.

requiring a space after the fn keyword (...) doesn't align with general usage

Sure, except for completely niche projects like Symfony, Laravel, PHPUnit, Composer, Magento, Doctrine, PHPStan, Flysystem, Laminas, Phalcon, PHP-CS-Fixer, Rector, Spatie, phpDocumentor... Also, I did a quick code search on GitHub:

screenshot-2022-11-28T00 13 25@2x

requiring a space after the fn keyword (...) doesn't align with documentation

You mean PHP documentation? It doesn't seem to care about any coding style in the slightest (screenshot from https://www.php.net/manual/en/functions.anonymous.php).

screenshot-2022-11-27T22 03 45@2x

PHP-CS-Fixer was an outlier which incorrectly enforced spacing after the fn keyword

This is also the default formatting enforced by PhpStorm.

I also checked the PHP Prettier plugin and found that it, like PhpStorm, formats arrow functions without a space after the fn keyword.

So defaults in PHP-CS-Fixer, which doesn't match your taste, are just incorrect, but bringing up defaults in PhpStorm and Prettier is a valid point in discussion?

Per the accepted Arrow Functions RFC and per its author, short closures were intended to be written without a space after the fn keyword.

That's actually the only argument that makes sense to me. However, the RFC author did allow a space after the fn, I guess in order to let the users decide how they would like to use it, and it seems to me that many of them chose to go with the additional space.

@theodorejb
Copy link
Contributor Author

Sure, except for completely niche projects like Symfony, Laravel, PHPUnit, Composer, Magento, Doctrine, PHPStan, Flysystem, Laminas, Phalcon, PHP-CS-Fixer, Rector, Spatie, phpDocumentor...

Out of that list, Symfony, PHPUnit, Composer Packagist, Magento, Flysystem, Phalcon, PHP-CS-Fixer, and Spatie are all using PHP-CS-Fixer, which until the latest release (v3.13) didn't allow differentiating between function and fn for formatting. So it seems unlikely that these projects chose to use an extra space; they simply didn't have the option to choose something different for fn with PHP-CS-Fixer.

Another project in your list (Laminas) does not appear to enforce an arrow function spacing rule - some fn keywords are succeeded by a space, but the majority are not.

Other well-known projects such as Psalm enforce having no space after the fn keyword.

You mean PHP documentation? It doesn't seem to care about any coding style in the slightest (screenshot from https://www.php.net/manual/en/functions.anonymous.php).

That screenshot isn't related to arrow functions (I did submit a PR to fix that inconsistency, though). The official Arrow Functions documentation examples do use consistent formatting which matches the accepted RFC.

So defaults in PHP-CS-Fixer, which doesn't match your taste, are just incorrect, but bringing up defaults in PhpStorm and Prettier is a valid point in discussion?

My point was that PHP-CS-Fixer's defaults didn't match other popular tools. Now that PHP-CS-Fixer allows configuring spacing for fn separately, you can already find projects that use it starting to opt into enforcing no space after fn.

Since there hasn't yet been a released PSR standard for fn spacing, it's natural that various projects have been using different styles. I have tried to avoid making an argument for one style or another based on my personal taste.

To summarize, the following arguments could be used to favor a space after fn:

  1. A bunch of large open-source projects use this formatting, since PHP-CS-Fixer defaulted to it and didn't use to allow formatting arrow functions separately from full closures.
  2. Some may view it as more consistent with the existing PSR standard for closure spacing.

On the other hand, the following points favor no space after fn:

  1. The primary goal of arrow functions is to avoid the verbosity of full closures. Not having a space after fn supports this goal.
  2. Arrow functions were intended to be written without a space after fn. This is the formatting consistently used in the accepted RFC, as well as the official PHP documentation for arrow functions.
  3. Other popular PHP formatters such the one built into PhpStorm, and Prettier's PHP plugin default to no space after fn. I don't have access to direct numbers, but my hunch is that PhpStorm is used with far more projects than PHP-CS-Fixer is.

@jrmajor
Copy link

jrmajor commented Nov 28, 2022

A bunch of large open-source projects use this formatting, since PHP-CS-Fixer defaulted to it and didn't use to allow formatting arrow functions separately from full closures.

it seems unlikely that these projects chose to use an extra space; they simply didn't have the option to choose something different for fn with PHP-CS-Fixer.

They did have the option, it's as simple as 'function_declaration' => false. They aren't forced to use neither this rule nor the PHP-CS-Fixer itself, yet they've chosen to do so.

Now that PHP-CS-Fixer allows configuring spacing for fn separately, you can already find projects that use it starting to opt into enforcing no space after fn.

This option was released almost a month ago, and none of the big projects I brought up changed it so far.

Why do you think that all of these projects have been forced to use this spacing by PHP-CS-Fixer? What makes you sure that any of these projects wants to change that? All of them were free to format their code however they want. They've chosen. They can change it whenever they want, yet they don't.

You mean PHP documentation? It doesn't seem to care about any coding style in the slightest

That screenshot isn't related to arrow functions

It wasn't supposed to be related to arrow functions, it was supposed to show my point: PHP documentation doesn't care about any coding style, so bringing it up in a discussion about coding style makes no sense.

My point was that PHP-CS-Fixer's defaults didn't match other popular tools.

Other popular PHP formatters such the one built into PhpStorm, and Prettier's PHP plugin default to no space after fn. I don't have access to direct numbers, but my hunch is that PhpStorm is used with far more projects than PHP-CS-Fixer is.

PhpStorm may be used in far more projects, but not as a formatter. PhpStorm can't even be used to enforce consistent formatting, as you can't use it in your CI. And for Prettier we have numbers: 1.5k stars for Prettier, 11.5k stars for PHP-CS-Fixer. So my question remains: why do you call defaults in PHP-CS-Fixer “incorrect”, but bring up defaults in PhpStorm and Prettier is a valid point in discussion?

The primary goal of arrow functions is to avoid the verbosity of full closures. Not having a space after fn supports this goal.

verbose: containing more words than necessary (Merriam-Webster)
verbose: using or containing more words than are necessary (Cambridge)

Sorry for taking apart your words, but I think there's a point there: having more whitespace is something different than having more words, and it makes code easier to read for some people. You don't need to add any spaces around binary operators, after the type colon, or between function arguments, yet you do for some reason. You could probably even format most of your PHP files to be one line to reduce “verbosity”.

Arrow functions were intended to be written without a space after fn. This is the formatting consistently used in the accepted RFC, as well as the official PHP documentation for arrow functions.

I agree with the point about the RFC. The documentation argument doesn't make any sense though, for the reason stated above. There is anything but coding style consistency in PHP documentation.

Another project in your list (Laminas) does not appear to enforce an arrow function spacing rule
Other well-known projects such as Psalm enforce having no space after the fn keyword.

You're right, my bad with Laminas. Also, I'm not saying that nobody prefers this style, Nette seems to use it too.

My point with bringing up these projects is that it's just false that fn( is a “general usage”. Many major projects do use fn (, and code search on GitHub reveals that it's even more popular than fn(.

  • Two most popular PHP frameworks (Symfony, Laravel) use fn (
  • Two most popular testing frameworks (PHPUnit, Pest) use fn (
  • The most popular static analysis tool (PHPStan) uses fn (
  • The most popular package manager (Composer) uses fn (
  • There are more occurrences of fn ( than fn( found with a simple GitHub search

@Crell
Copy link
Collaborator

Crell commented Nov 28, 2022

This option was released almost a month ago, and none of the big projects I brought up changed it so far.

You vastly under-estimate the effort involved in changing coding standards on a large project. IME, it can take months just to agree to do it, to say nothing of actually making the change.

@jrmajor is correct that we should be informed by research, however. Which he has, graciously, done for us. If we take the screenshot from GitHub at face value, it tells us that while fn ( is more common, it's not by much. It's around 53% fn (, 47% fn(. I'd call that close enough to an even split. That means no matter what we do, "around half" of existing code will not already be in compliance with PER-CS 1.1.

It's also a valid observation that it seems very likely that many/most of those projects didn't make an active, deliberate decision. They just turned on php-cs-fixer, or Prettier, or whatever tool and left it at its defaults, or made only one or two changes. That's true for both styles, probably.

So if we make a statement at all, it's going to disrupt about the same amount of code either way. And odds are, those people will upgrade their formatting tool at some point in the future, it will say "now using PER-CS 1.1", and flag a bunch of things. Users will roll their eyes, push "yes", and move on with their lives.

So our decision should be based on internal consistency within the spec, and on what is most ergonomic (which is admittedly hard to get any objective data about in this case). The popularity argument is basically a wash either way.

@jrmajor
Copy link

jrmajor commented Nov 30, 2022

It's also a valid observation that it seems very likely that many/most of those projects didn't make an active, deliberate decision. They just turned on php-cs-fixer, or Prettier, or whatever tool and left it at its defaults, or made only one or two changes. That's true for both styles, probably.

You vastly under-estimate the effort involved in changing coding standards on a large project. IME, it can take months just to agree to do it, to say nothing of actually making the change.

Even if it wasn't as deliberate decision, which I doubt, there is nothing suggesting that these projects want to change that. And there's high chance they won't, for the exact reason you provided.

If we take the screenshot from GitHub at face value, it tells us that while fn ( is more common, it's not by much. It's around 53% fn (, 47% fn(. I'd call that close enough to an even split. That means no matter what we do, "around half" of existing code will not already be in compliance with PER-CS 1.1.

So if we make a statement at all, it's going to disrupt about the same amount of code either way.

The popularity argument is basically a wash either way.

I find the way you rounded these percentages unfair. Anyway, I've taken this screenshot only to show that even the simplest research one can do reveals that the claim about the “general usage” is false.

For more accurate results, I've analysed 1000 most popular Composer packages using https://github.com/nikic/popular-package-analysis and this script. Here are the results:

total files analysed:  109516
total with a space:    1619, 72%
total without a space: 631, 28%

Which is in line with the fact that most popular tools in all categories I could think of also use fn (.

In the end, we can all live with whatever ends up being included in PER CS. My main complaint isn't as much about the fn(, as it is about the process that led to this PR being merged. I believe that most arguments brought up made no sense, as justified in previous comments. The only argument related to the real usage, which is most important IMO, was just calling it “the general usage” with no research provided to support this claim, and the more research I do, the more false it turns out to be.

jrfnl added a commit to WordPress/WordPress-Coding-Standards that referenced this pull request Aug 11, 2023
As things were, the `WordPress.WhiteSpace.ControlStructureSpacing` sniff was being _abused_ to also check the formatting of function declaration statements, while it really isn't suitable for this.

There have been numerous issues regarding this and various "plaster on the wound" fixes have been made previously.

This PR is intended to fix the problem once and for all in a more proper fashion.

The checks which were previously done by the `WordPress.WhiteSpace.ControlStructureSpacing` sniff consisted of the following:
* Exactly one space after the `function` keyword (configurable for closures).
* No space between the function _name_ and the open parenthesis.
* Exactly one space after the open parenthesis.
* Exactly one space before the close parenthesis.
* Exactly one space before the open brace/after the close parenthesis.

The above didn't take return type declarations into account properly, would run into problems with function declarations which didn't have an open brace, such as `abstract` functions or interface functions (also see 2204), and handling multi-line function declarations was undefined territory.

It also led to:
* Duplicate messages - the `ControlStructureSpacing` sniff reporting things also reported by the `Squiz.Functions.FunctionDeclarationArgumentSpacing` sniff or by the `Generic.WhiteSpace.LanguageConstructSpacing` sniff.
* Unclear messages [1] - messages were often thrown on the `function` keyword, while the problem was further down the line.
* Unclear messages [2] - messages would refer to control structures instead of functions and would otherwise not always be clear.

This commit:
* Removes all code related to checking function declarations from the `WordPress.WhiteSpace.ControlStructureSpacing` sniff.
    Includes removing the tests which were specific to function declarations.
    This effectively reverts the function related changes in PRs 417, 432, 440, 463, 1319, 1837 in so far as those touched the `ControlStructureSpacing` sniff.
* Adds the `Squiz.Functions.MultiLineFunctionDeclaration` sniff to the `Core` ruleset to do those checks instead.

The change as currently proposed has been extensively researched and tested and should provide a smooth switch over.

Notes about the ruleset changes:
* The `Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingBeforeClose` code was previously silenced to prevent duplicate messages, but can now be re-enabled as we'll defer to that sniff for the spacing on the inside of the parentheses.
* The `Squiz.Functions.MultiLineFunctionDeclaration` sniff, by default, expects the opening brace for named functions on the line _after_ the declaration and for closures, it expects the opening brace on the _same_ line as the closure declaration.
    WPCS expects the opening brace on the _same_ line as the function declaration in both cases and enforces this via the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff (which is also used by the `Squiz.Functions.MultiLineFunctionDeclaration` sniff for the closure check).
    The ruleset now contains three error code exclusions to bypass the "brace on new line for named functions" checks.
* As the opening brace for closures is enforced to be on the same line, we can let the new Squiz sniff handle this.
    To that end the `checkClosures` property for the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff is set to `false` to prevent duplicate messages for the same.
* To prevent duplicate messages about code after the open brace for named functions, the `Generic.Functions.OpeningFunctionBraceKernighanRitchie.ContentAfterBrace` code has to be silenced.
    We cannot silence the same error code for the Squiz sniff as that would also silence the notice for closures, so silencing it for the `OpeningFunctionBraceKernighanRitchie` sniff, which now only handles named functions, is the only option, but should prevent the duplicate messages.
* Additionally, the `Squiz.Functions.MultiLineFunctionDeclaration.SpaceAfterUse` error code is excluded as it overlaps with the `Generic.WhiteSpace.LanguageConstructSpacing` sniff, which also checks this.

Notes about changes in behaviour before/after this change:
* If the open & close parentheses are not on the same line, the space on the inside of the parentheses is no longer checked (as it is seen as a multi-line function declaration for which different rules may apply).
* While not commonly used in WP, for multi-line function declarations, the sniff will now enforce one parameter per line with the first parameter on the line _after_ the open parenthesis and the close parenthesis on the line after the last parameter and indented the same as the start of the function declaration statement.
    A function is considered multi-line when the open and close parentheses are not on the same line, which means that for closures with `use` statements, these rules will apply to both the function parameter list as well as the use parameter list.
* It is no longer possible to choose the spacing between the `function` keyword for closures and the open parenthesis.
    This will no always be enforced to be a single space, which is in line with PSR12.
    A check of WP Core showed that there was no consistency at all regarding this spacing, with approximately 50/50 of closures having no space vs 1 space, so defering to PSR12 and enforcing consistency in this, seems like a reasonable choice.

Other notes:
* The `WordPress.WhiteSpace.ControlStructureSpacing` sniff did not support formatting checks for PHP 7.4 arrow functions.
    At this time, the `Squiz.Functions.MultiLineFunctionDeclaration` sniff does not do so either.
    There is a PR open upstream squizlabs/php_codesniffer 3661 to add support for arrow functions, though that PR still needs an update for the (very inconsistent) rule chosen by PSR-PER to [enforce _no_ space between the `fn` keyword and the open parenthesis for arrow functions](php-fig/per-coding-style#53).
* There is also a PR open upstream - squizlabs/php_codesniffer 3739 - to fix two fixer issues. At this time, these do not pose a problem for WP Core as (aside from the closure keyword spacing), WP Core is "clean", but would be nice if that PR got merged to prevent future issues.
* While doing the research for this PR, I discovered an issue with the `Generic.Functions.OpeningFunctionBraceKernighanRitchie` sniff. It doesn't check the spacing before the open brace for empty functions.
    PR squizlabs/php_codesniffer 3869 has been opened to fix that.
    As empty functions are the exception, not the rule, I'm hoping it will get merged before any issues about this are reported.

Fixes 1101

Note: check 8 identified in issue 1101 is still open, but there is a separate issue open to discuss that in more depth - 1474 - and that issue remains open (for now).
@andrey-helldar
Copy link

Now the function began to look like everything was thrown into one pile...

fn(string $foo) => $foo;
fn(string $bar) => $bar;

It's ugly and inconvenient. Also difficult to read.

The option with a space looks much better and more elegant:

fn (string $foo) => $foo;
fn (string $bar) => $bar;

But, since we removed the spaces from the arrow functions, why not remove the spaces in other places as well?
For example:

(string)$foo
(int)$bar

function(string $foo) {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants