Skip to content

Do not report assigning a float zero as redundant - #6548

Open
zonuexe wants to merge 4 commits into
phpstan:2.3.xfrom
zonuexe:assign-redundant-signed-zero
Open

zonuexe wants to merge 4 commits into
phpstan:2.3.xfrom
zonuexe:assign-redundant-signed-zero

Conversation

@zonuexe

@zonuexe zonuexe commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

UnusedVariableRule gained an assign.redundant check in b1c223b (2.3.x, enabled by featureToggles.unusedVariable in bleedingEdge, unreleased). It reports an assignment of 0.0 or -0.0 to a variable that already holds a zero, but that assignment can change the value.

<?php
function f(float $f): float {
	if ($f === 0.0) {
		$f = 0.0;
	}
	return $f;
}
function g(): float {
	$x = -0.0;
	$x = 0.0;
	return $x;
}

Output on level 9 with bleedingEdge:

4: Variable $f is assigned value 0.0 but it already has that value.
9: Value assigned to variable $x is never read before being overwritten.
10: Variable $x is assigned value 0.0 but it already has that value.

ConstantFloatType::equals() compares with ===, and -0.0 === 0.0, so it treats the two zeros as one value. PHP tells them apart: (string) -0.0 and json_encode(-0.0) give "-0", var_export(-0.0, true) gives '-0.0', and fdiv(1, -0.0) gives -INF. Line 4 turns -0.0 into 0.0. Delete it as the message suggests and f(-0.0) returns -0.0 again. The two reports in g() also contradict each other. The report on line 9 is right, since line 10 overwrites that -0.0 before anything reads it, and that makes line 10 the assignment that gives $x its 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.0 narrows $f to 0.0 while the runtime value can be -0.0, and === -0.0, == 0 and in_array($f, [0.0], true) behave the same way. AssignHandler::redundant() now returns null when 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]; and if ($a === [0.0]) { $a = [0.0]; } are still (wrongly) reported, while the offset write $a[0] = 0.0 is 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 $x cannot tell a literal 0.0 from a narrowed zero of unknown sign, and ConstantFloatType::toString() and FiniteTypeSet::key() already treat a zero that way: toString() returns '0'|'-0' for a zero, and FiniteTypeSet::key() excludes floats because equals() does not agree with value identity. Redundant assignments of any other value are still reported. The test data keeps a 1.5 case as a control and pins the repeated 0.0, and with this PR the reproducer above reports only line 9.

turbo-ext/src/AssignHandler.cpp gets 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 ===, null has 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 [] for NAN, so NAN never reaches this check.

Left for a separate PR: on a string, $s == '1' narrows $s to '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-strict in_array($s, ['1']) narrow the same way, and '0' is affected as well; match compares with ===, so the rule reports it correctly. The cause lies in how loose comparisons narrow string types.

Verification

  • UnusedVariableRuleTest::testRedundantAssignmentOfSignedZero fails 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).
  • turbo: the strict build, smoke.php, signature-parity.php, side-by-side.php, and walk-trace.php on the default corpus and on tests/PHPStan/Rules/DeadCode/data pass. Analysis output for the unused-variable* test data matches with the extension on and off.
  • The full test suite passes with the extension loaded (shadowing confirmed active). Self-analysis with the repository config reports no errors.
  • I did not run make lint-turbo, because clang-tidy 21 is not installed here.

@zonuexe
zonuexe force-pushed the assign-redundant-signed-zero branch from 187cb9b to 86ec4b8 Compare September 23, 2026 09:34
@staabm
staabm requested a review from ondrejmirtes September 24, 2026 07:34
@zonuexe
zonuexe force-pushed the assign-redundant-signed-zero branch 2 times, most recently from b8573de to 5be8ab0 Compare September 30, 2026 16:27
@ondrejmirtes

Copy link
Copy Markdown
Member

Please show proof on 3v4l.org that such an assignment is not redundant.

@zonuexe

zonuexe commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

@ondrejmirtes https://3v4l.org/NdkoE: withAssignment(float $f) and withoutAssignment(float $f) differ only by the reported line, and for -0.0 they return different values: (string) gives 0 vs -0, var_export() 0.0 vs -0.0, json_encode() 0 vs -0, and fdiv(1, $v) INF vs -INF. $f === 0.0 is true for -0.0, so the branch runs, and the reported assignment is what turns -0.0 into 0.0. A -0.0 is easy to get in practice: round(-0.001, 2) returns one, and json_encode() then emits -0.

https://3v4l.org/GiSbY shows the same on one screen: echo $f prints -0 before the assignment and 0 after it. I added this function to the test data as well.

@ondrejmirtes
ondrejmirtes force-pushed the assign-redundant-signed-zero branch from 11b8235 to 09830b7 Compare October 31, 2026 13:30
}
foreach ($type->getConstantArrays() as $array) {
foreach ($array->getValueTypes() as $valueType) {
if (self::containsFloatZero($valueType)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@zonuexe
zonuexe marked this pull request as draft November 1, 2026 20:14
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.
@zonuexe
zonuexe force-pushed the assign-redundant-signed-zero branch from 09830b7 to 121f182 Compare November 1, 2026 20:34
@zonuexe
zonuexe marked this pull request as ready for review November 1, 2026 20:34
@phpstan-bot

Copy link
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

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.

3 participants