Repository navigation
Conversation
187cb9b to
86ec4b8
Compare
b8573de to
5be8ab0
Compare
|
Please show proof on 3v4l.org that such an assignment is not redundant. |
|
@ondrejmirtes https://3v4l.org/NdkoE: https://3v4l.org/GiSbY shows the same on one screen: |
11b8235 to
09830b7
Compare
| } | ||
| foreach ($type->getConstantArrays() as $array) { | ||
| foreach ($array->getValueTypes() as $valueType) { | ||
| if (self::containsFloatZero($valueType)) { |
There was a problem hiding this comment.
This isn't great. Why would you ever mix up 0.0 and nested array without limit such as [0.0]? Even if we wanted this, it certainly misses some use cases like object shape with 0.0, or also general array with 0.0.
There was a problem hiding this comment.
Dropped the recursion in 121f182; only the assigned value itself is checked now. A zero inside a constant array ($a = [-0.0]; $a = [0.0];, or if ($a === [0.0]) { $a = [0.0]; }) is reported again, which seems an acceptable trade-off for such code.
Object shapes and general arrays never reached this check: their getFiniteTypes() is [], so redundant() returns early.
0.0 and -0.0 are identical, so ConstantFloatType::equals() does not tell them apart, but the sign stays observable: (string) -0.0 is "-0" and fdiv(1, -0.0) is -INF. `$f === 0.0` also narrows $f to 0.0 while it may still hold -0.0, so a sign-aware comparison would not help either. AssignHandler::redundant() now gives up when the value holds a float zero, directly or inside a constant array; the native twin does the same.
…nt test A zero two arrays deep pins the recursion of containsFloatZero(), and `$x = 0.0; $x = 0.0;` pins that a repeated zero is no longer reported: the type cannot tell a literal 0.0 from a narrowed zero of unknown sign.
https://3v4l.org/GiSbY: `$f === 0.0` holds for -0.0, yet `$f = 0.0` in that branch changes what `echo $f` prints from "-0" to "0".
Constant arrays holding a zero are no longer searched, so
`$a = [-0.0]; $a = [0.0];` and `if ($a === [0.0]) { $a = [0.0]; }` are
reported again. Object shapes and general arrays never have a single
finite type and do not reach this check.
09830b7 to
121f182
Compare
|
This pull request has been marked as ready for review. |
UnusedVariableRulegained anassign.redundantcheck in b1c223b (2.3.x, enabled byfeatureToggles.unusedVariablein bleedingEdge, unreleased). It reports an assignment of0.0or-0.0to a variable that already holds a zero, but that assignment can change the value.Output on level 9 with bleedingEdge:
ConstantFloatType::equals()compares with===, and-0.0 === 0.0, so it treats the two zeros as one value. PHP tells them apart:(string) -0.0andjson_encode(-0.0)give"-0",var_export(-0.0, true)gives'-0.0', andfdiv(1, -0.0)gives-INF. Line 4 turns-0.0into0.0. Delete it as the message suggests andf(-0.0)returns-0.0again. The two reports ing()also contradict each other. The report on line 9 is right, since line 10 overwrites that-0.0before anything reads it, and that makes line 10 the assignment that gives$xits value.The check has not shipped in a release yet, so this fix lands before any user sees the false positive.
The fix
A sign-aware comparison would still miss
f().$f === 0.0narrows$fto0.0while the runtime value can be-0.0, and=== -0.0,== 0andin_array($f, [0.0], true)behave the same way.AssignHandler::redundant()now returnsnullwhen the assigned value itself is a float zero. The check runs on the offset's value type, so it also covers offset writes such as$a['k'] = 0.0. A zero inside a constant array is not checked, so$a = [-0.0]; $a = [0.0];andif ($a === [0.0]) { $a = [0.0]; }are still (wrongly) reported, while the offset write$a[0] = 0.0is not. Object shapes and general arrays never have a single finite type, so they do not reach this check at all.This gives up one true positive:
$x = 0.0; $x = 0.0;is no longer reported. The type of$xcannot tell a literal0.0from a narrowed zero of unknown sign, andConstantFloatType::toString()andFiniteTypeSet::key()already treat a zero that way:toString()returns'0'|'-0'for a zero, andFiniteTypeSet::key()excludes floats becauseequals()does not agree with value identity. Redundant assignments of any other value are still reported. The test data keeps a1.5case as a control and pins the repeated0.0, and with this PR the reproducer above reports only line 9.turbo-ext/src/AssignHandler.cppgets the same check.Other types
I looked for other finite types whose
equals()is coarser than value identity and found none. Constant int/string/bool compare values with===,nullhas one value, enum cases compare class and case name, and constant arrays compare keys and values by position, which keeps key order significant.ConstantFloatType::getFiniteTypes()returns[]forNAN, soNANnever reaches this check.Left for a separate PR: on a
string,$s == '1'narrows$sto'1', while'01',' 1'and'1e0'also compare equal to'1'. The rule therefore still reports$s = '1'inside that branch as redundant.switch ($s) { case '1': ... }and non-strictin_array($s, ['1'])narrow the same way, and'0'is affected as well;matchcompares with===, so the rule reports it correctly. The cause lies in how loose comparisons narrow string types.Verification
UnusedVariableRuleTest::testRedundantAssignmentOfSignedZerofails on 2.3.x without the fix, with all eight zero cases reported (including the https://3v4l.org/GiSbY reproducer), and passes with it. It also passes with the PHP change alone (extension off) and with the C++ change alone (extension on).smoke.php,signature-parity.php,side-by-side.php, andwalk-trace.phpon the default corpus and ontests/PHPStan/Rules/DeadCode/datapass. Analysis output for theunused-variable*test data matches with the extension on and off.make lint-turbo, because clang-tidy 21 is not installed here.