Allow single line braces for empty function body

[tuqqu]

Closes #5475.

This option allows placing braces on a single line for empty function bodies:

class X 
{
     public function __construct(
              public int $x,
              //...
     ) {}
}

Since the introduction of property promotion this kind of constructors are quite widespread. This option also enables this behaviour for other empty functions as well, e.g.

public function x(): void {}

[julienfalque]

As proposed in #5475, it might be better to deprecate the allow_single_line_anonymous_class_with_empty_body option to replace it with a new one that would apply to any braces instead of some specific cases. For example, we could go with allow_single_line_empty_body. What do you think?

[tuqqu]

@julienfalque I think it's a good suggestion. I adjusted the PR. Let me know if there is anything that still needs to be fixed.

[tuqqu]

@julienfalque Not sure what to do with tests complaining about self-deprecations. I can't remove them, since the option being deprecated had a default value, therefore must still be present (I guess). Any ideas?

[julienfalque]

@tuqqu You probably need to update rulesets to stop using the deprecated option. Tests that still use it purposely should be tagged with @group legacy and may use $this->expectDeprecation().

[coveralls]

Coverage increased (+0.0001%) to 92.164% when pulling a44c1d90897a8c781388758b5fa485846f1340ff on tuqqu:allow_single_line_empty_function_body into 73b6c0a5ee26e741e176f4d38e799f5279e8d3ec on FriendsOfPHP:master.

[julienfalque]

By the way, @tuqqu please avoid amending/squashing commits, this forces reviewers to read the whole changes on every push. Commits will be squashed before merging anyway.

[tuqqu]

@julienfalque thanks for the review. I made changes to the code. I tested on my machine and all tests are passing now.

[tuqqu]

@julienfalque I'm not sure if any of the recent test fails relate to this PR in any way. What should we do?

[julienfalque]

@tuqqu Indeed, the issue also happens on master branch. We should fix that so you can rebase and have a green build :)

[julienfalque]

@tuqqu Build fixed on master, please rebase.

[tuqqu]

@julienfalque, the tests are passing now, the only issue is with fabbot.io that you suggested to ignore.

is there anything else that ought to be done with this PR?

[julienfalque]

We didn't make a decision about https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/5757#discussion_r646499681.

[tuqqu]

@julienfalque Hello. Is there any estimation of how long it will take to make a decision?

[tuqqu]

@keradus Maybe you could help with that question? Just want to make sure this PR is not forgotten

[GrahamCampbell]

👎 on the options here. We should instead add a separate option specifically for saying if this should be allowed for constructors only, since they are special methods that support property promotion. Maybe we should be even stricter, and only allow this if property promotion is actually present!

[chewbakartik]

-1 on the options here. We should instead add a separate option specifically for saying if this should be allowed for constructors only, since they are special methods that support property promotion. Maybe we should be even stricter, and only allow this if property promotion is actually present!

This doesn't really need to be overcomplicated. If someone has an empty body of a method, and the braces are on the same line, it doesn't force a line break. That's particularly useful for property promotion in constructors. Having said that, if someone already has a line break in there, it's not forcing the braces to be on the same line.

It doesn't change anything functionally, so there's not really a risk if it's not a constructor and has an empty body. That's not something that can be "fixed" even if php-cs-fixer was to put a line break in those instances. The line break isn't going to make code that has a function without a body any better, it's still going to be an empty body.

I feel like we're splitting hairs on a pretty vanilla change. It's just skipping adding a line break to code that has an empty body. Without this PR, in my code where I've started using property promotion, the extra line break is getting pretty annoying.

[ralphjsmit]

Any idea when this will become an option in PHP CS Fixer?

[gnutix]

Very eager to see this one coming out too !

[ralphjsmit]

@julienfalque @SpacePossum Could you shed some light on the release date/timeline of this rule? Thanks!🙌

[winkbrace]

My team would much appreciate this option too. :)

And I know this is out of scope, but FYI another use case for empty body I sometimes have is

class CustomException extends \Exception 
{}

[SpacePossum]

just a FYI as it is braces related I won't be the one reviewing it, there are however two unresolved comments from reviewers here.

[gnutix]

Any chance this PR could get some attention ? Last review was almost 6 months ago.

[julienfalque]

@tuqqu The handling of braces position has been extracted from braces into curly_braces_position, can you rebase and move your changes to this new rule?

[nicholasruunu]

@julienfalque Seems like his branch is deleted, could work on it but not sure how.

[tuqqu]

@julienfalque it's been a while, I deleted the branch since. Whoever wants to take on the issue is welcome

[Crell]

Just a note that the next version of PER-CS is going to encourage (but not require) the style shown here.

[Wirone]

@tuqqu would you mind to rebase your branch and refresh this PR? I think it's worth finishing, since it's second most liked issue on the backlog 🙂.

[nickygerritsen]

I tried rebasing this myself but I got quite stuck with the logic in the new curly braceposition fixer. I tried setting the $allowSingleLineIfEmpty by default to the new config instead of false, but that makes it do this:

function test() {}

becomes

function test()
{}

I do hope we can get this fixed, since I also think the empty body case is a good one.

[Wirone]

@tuqqu's fork was removed, but it could be picked up, BUT I believe this should be closed since it provides changes for braces fixer which was deprecated and is not supported anymore. We need new implementation based on smaller, existing fixers or new ones. @keradus @kubawerlos what do you think?

[Wirone]

Closing this one, we'll continue in #6933 🙂