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

Reference assignment operator =& gets broken up #7303

Closed
nikic opened this issue Sep 17, 2023 · 16 comments · Fixed by #7388
Closed

Reference assignment operator =& gets broken up #7303

nikic opened this issue Sep 17, 2023 · 16 comments · Fixed by #7388
Labels

Comments

@nikic
Copy link

nikic commented Sep 17, 2023

Bug report

Description

The binary_operator_spaces fixer using at_least_single_space (part of PSR12 ruleset since recently) will break up the reference assignment operator =& by inserting a space between the two parts:

<?php
$a =& $b;
// becomes
$a = & $b;

I think this should either be left as $a =& $b (preferred) or become $a = &$b (misleading), but the chosen form of $a = & $b is not a style I have ever seen.

Runtime version

PHP CS Fixer 3.26.1 Crank Cake by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.1.2-1ubuntu2.14

Command

php-cs-fixer fix test.php --rules=@PSR12
@nikic nikic added the kind/bug label Sep 17, 2023
@Wirone
Copy link
Member

Wirone commented Sep 17, 2023

Hi @nikic, nice to see you here 🙂. It was already discussed in #7232 and conclusion was that's expected behavior. But maybe assumption was wrong and =& is an actual operator?

@nikic
Copy link
Author

nikic commented Sep 17, 2023

It's a bit of a tricky question. & is not a standalone operator and cannot appear in arbitrary expressions -- $a + &$b is not a valid expression. It is only allowed in a few specific places, one of those being the combination of = and &. In that sense, =& should be seen as a single operator, rather than the combination of a binary and (non-existent) unary operator. At the same time, PHP does not have a dedicated token for =& and instead handles this as a combination of the = and & tokens, which is what allows placing whitespace between = and & in the first place (e.g. one couldn't write & =, but one can write = &). In that sense, these are separate operators.

I guess the expected formatting of reference assignment is something that PER may want to clarify. I think there is a problem on the PHP-CS-Fixer side either way though, as it should produce one of $a =& $b or $a = &$b, but not $a = & $b.

@Wirone
Copy link
Member

Wirone commented Sep 17, 2023

Thank you for the explanation 🙂. We probably need to make sure that it is just left as-is, regardless if it's =& $foo or = &$foo and then probably introduce assign_by_reference fixer to standardise it?

@nikic
Copy link
Author

nikic commented Sep 17, 2023

Thank you for the explanation 🙂. We probably need to make sure that it is just left as-is, regardless if it's =& $foo or = &$foo and then probably introduce assign_by_reference fixer to standardise it?

Yeah, I think that would be the ideal thing to do.

@greew
Copy link

greew commented Oct 23, 2023

@Wirone Would you see the assign_by_reference fixer as an option to the binary_operator_spaces fixer or a separate fixer?

@Wirone
Copy link
Member

Wirone commented Oct 23, 2023

@greew what about universal reference_spaces (or something like that), that would ensure proper whitespace around & for any reference usage (assignment, function signature, anonymous function's use block etc)?

@greew
Copy link

greew commented Oct 23, 2023

That might actually be better, yeah! And also allow for only handling e.g. assignments, if only that is the desired case.

I might give it a try, but since it's my first time working with PHP-CS-Fixer core, the PR will probably need some work and comments! ;)

@keradus
Copy link
Member

keradus commented Oct 24, 2023

I am not convinced it's good idea @Wirone .

We do not want to create a dedicated rule for each symbol, like ~, !, ...

If we have a rule that handle unary operators well, but it handles one extra case too much (=&), let's fix that rule and that's it.

I'm not sure about $a =& $b over $a = &$b. For me the 2nd variant was more intuitive tbh, PER also only mention & glued on right side (eg &...$parts, &$param) - but indeed, there is no explicit example around = & combination.

@keradus
Copy link
Member

keradus commented Oct 24, 2023

if we would go with & being sticky on right side, tbh I think we have almost good setup - binary_operator_spaces adds space after =, and unary_operator_spaces removes space after &,
ie

ker@d ~/github/FriendsOfPHP/PHP-CS-Fixer λ ./php-cs-fixer fix x.php --dry-run --diff -vvv --rules=@PSR12,unary_operator_spaces
PHP CS Fixer 3.35.2-DEV Freezy Vrooom by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.2.10
Loaded config default from "/Users/d.ruminski/github/FriendsOfPHP/PHP-CS-Fixer/.php-cs-fixer.dist.php".
Paths from configuration file have been overridden by paths provided as command arguments.
F                                                                                                                                                               1 / 1 (100%)
Legend: .-no changes, F-fixed, S-skipped (cached or empty file), I-invalid file syntax (file ignored), E-error
   1) x.php (unary_operator_spaces, binary_operator_spaces) 
-$a =& $b;
+$a = &$b;

but we miss the unary_operator_config to be part of @PSR12


yep, PHP docs also use this styling $var = &$othervar
ref https://www.php.net/manual/en/language.operators.assignment.php#language.operators.assignment.reference

@greew
Copy link

greew commented Oct 25, 2023

@Wirone
Copy link
Member

Wirone commented Oct 25, 2023

We do not want to create a dedicated rule for each symbol, like ~, !, ... (...)
but we miss the unary_operator_config to be part of @PSR12

@keradus I wasn't aware that unary_operator_spaces covers that, but after looking at the test cases it seems like it's enough 🤷‍♂️. Now I'm wondering if we can just add this fixer to @PSR12 🤔?

@greew
Copy link

greew commented Oct 25, 2023

I have just tested using unary_operator_spaces, and this fixes the issue for me! :)

@keradus
Copy link
Member

keradus commented Oct 25, 2023

#7388

@wimski
Copy link

wimski commented Nov 14, 2023

@keradus

The PSR-12 documentation only makes two explicit remarks about unary operators. So anything else shouldn't be 'fixed' when using the PSR-12 ruleset.

6.1. Unary operators

The increment/decrement operators MUST NOT have any space between the operator and operand.

$i++;
++$j;

Type casting operators MUST NOT have any space within the parentheses:

$intValue = (int) $input;

https://www.php-fig.org/psr/psr-12/#61-unary-operators

.

With the new forced (and non-configurable) unary_operator_spaces rule being part of the PSR-12 ruleset, things like this are changed:

--- if (! $someBoolean) {
+++ if (!$someBoolean) {

Even-though that's more part of the logical operators than unary operators in general. And PHP uses a space after it in their reference:

Example Name Result
! $a Not true if $a is not true.

https://www.php.net/manual/en/language.operators.logical.php

@keradus
Copy link
Member

keradus commented Nov 14, 2023

logical operators can be unary (eg !) or binary (eg or)

also from PHP docs: https://www.php.net/manual/en/language.operators.php

Unary operators take only one value, for example ! (the logical not operator) or ++ (the increment operator).

with that, PSR 12 saying to have no space for unary operator, making no space when using !.

If you prefer to follow limited set of PSR set, you can adjust your project config to disable that rule.

@wimski
Copy link

wimski commented Nov 14, 2023

logical operators can be unary (eg !) or binary (eg or)

Good point 👍 I've scratched that argument from my original comment.

PSR 12 saying to have no space for unary operator

Ah, but the fact is that it doesn't say that. It only cites two specific rules in the section 'unary operators' without mentioning a general rule applying to all unary operators.

stevegrunwell added a commit to stevegrunwell/time-constants that referenced this issue Dec 17, 2023
This is a change for me, but PHP-CS-Fixer/PHP-CS-Fixer#7303 makes a good point for `if (!defined())` being a better match with PSR-12 than `if(! defined())`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants