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 value holds a float zero, either as the value itself or inside a constant array ($a = [-0.0]; $a = [0.0];). The check runs on the offset's value type, so it also covers offset writes such as $a['k'] = 0.0.

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. I regenerated turbo-ext/src/generated/AssignHandler.h for the new private method, and 5be8ab0 is the make bump-turbo follow-up. If this is squash-merged, EXPECTED_EXTENSION_VERSION has to be bumped again after the merge, since the squashed commit gets a new SHA.

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 ten zero cases reported (including the https://3v4l.org/GiSbY reproducer), and passes with it. A zero two arrays deep ([[-0.0]] then [[0.0]]) makes the test fail if containsFloatZero() stops recursing. 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 (after the version bump, with 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 from 86ec4b8 to b8573de Compare September 25, 2026 00:40
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.
@zonuexe
zonuexe force-pushed the assign-redundant-signed-zero branch 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.

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".
@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.

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.

2 participants