coding-standard icon indicating copy to clipboard operation
coding-standard copied to clipboard

Enhancement: Add option to only report semantically useless parentheses

Open jlherren opened this issue 3 years ago • 1 comments

Currently, UselessParentheses reports the parenthesis on $a + ($b + $c). In many cases such parentheses can be omitted, since addition is associative, thus writing $a + $b + $c produces the same result. However, this is not strictly true for floating point math, where it may cause a different result. Example:

$a = 7;
$b = 1e20;
$c = -1e20;
printf("%f\n", $a + $b + $c); // Case 1: Prints 0.000000
printf("%f\n", ($a + $b) + $c); // Case 2: Prints 0.000000
printf("%f\n", $a + ($b + $c)); // Case 3: Prints 7.000000

https://3v4l.org/sCil3

Case 1 and case 2 are semantically equivalent due to addition being left-associative, thus $a and $b are summed first in both cases. However, case 3 causes $b and $c to be summed first, which may intentionally be written that way to achieve better numerical stability. For writing code related to math, it would be nice to have an option to not report such situations.

jlherren avatar Feb 24 '22 16:02 jlherren

I second that In general it would be nice to have a bit more control over what is deemed unnecessary. Usually, the computer - apart from the example above - does not care about any of those parentheses. But for human readability (and to prevent mistakes when refactoring) we usually add them in some less clear examples (or where people could misread them maybe). The more experienced, maybe you dont need them. But for a good and well written code we usually want some in place in cases like these:

- if ($limit && $count > ($limit + $offset)) {
+ if ($limit && $count > $limit + $offset) {
- $x = ($this->someMethod($args, $moreArgs) > $myKey) ? $foo : $y;
+ $x = $this->someMethod($args, $moreArgs) > $myKey ? $foo : $y;
- $percentage = $total ? ($approved / $total) : 0;
+ $percentage = $total ? $approved / $total : 0;

Not sure if this one might change results:

- $multiplier = (strlen($matches[1]) / 2) * 4;
+ $multiplier = strlen($matches[1]) / 2 * 4;

This could also change behavior:

-       return ((string)$rangeFrom === '' || version_compare($version, $rangeFrom, '>='))
+        return (string)$rangeFrom === '' || version_compare($version, $rangeFrom, '>=')
            && ((string)$rangeTo === '' || version_compare($version, $rangeTo, '<='));

All of these cases the sniffer removes them and code wise probably not relevant, sure. But for human interpretation and to read the code better they should not be removed IMO.

A setting here would be maybe some threshold on surrounding complexity in general (operator wise).

        <property name="ignoreAroundOperators" value="true"/>

dereuromark avatar Mar 22 '22 22:03 dereuromark