Skip to content

Commit f34a29b

Browse files
committed
Report null coalesce errors on the last line of the left operand
- `??` and `??=` errors used the start line of the whole expression, which is the first line of the left operand. When the left operand spans several lines, the error landed above the operator. - The AST does not keep the operator token, so the error now uses the last line of the left operand: the operator is on that line or below it, so a line that was already correct stays. - When `??` starts its own line, the error is still one line above the operator.
1 parent ec0d4b6 commit f34a29b

3 files changed

Lines changed: 104 additions & 0 deletions

File tree

‎src/Rules/Variables/NullCoalesceRule.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ static function (Type $type): ?string {
6868
return [];
6969
}
7070

71+
$error = RuleErrorBuilder::message($error->getMessage())
72+
->identifier($error->getIdentifier())
73+
->line($this->getOperatorLine($node))
74+
->build();
75+
7176
if ($scope->isInTrait()) {
7277
// The error messages already distinguish the possible outcomes,
7378
// so the contexts only need to be told apart by error/no error.
@@ -78,6 +83,24 @@ static function (Type $type): ?string {
7883
return [$error];
7984
}
8085

86+
/**
87+
* The AST does not keep the operator token. The left side ends on the operator's
88+
* line or above it, so its last line is the closest one that never overshoots.
89+
*/
90+
private function getOperatorLine(CoalesceExpressionNode $node): int
91+
{
92+
$originalExpr = $node->getOriginalExpr();
93+
if ($originalExpr instanceof Node\Expr\BinaryOp\Coalesce) {
94+
return $originalExpr->left->getEndLine();
95+
}
96+
97+
if ($originalExpr instanceof Node\Expr\AssignOp\Coalesce) {
98+
return $originalExpr->var->getEndLine();
99+
}
100+
101+
return $node->getStartLine();
102+
}
103+
81104
private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scope $scope): ?IdentifierRuleError
82105
{
83106
if (!$this->unnecessaryNullCoalesce) {

‎tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -711,6 +711,36 @@ public function testBug12780(): void
711711
$this->analyse([__DIR__ . '/data/bug-12780.php'], []);
712712
}
713713

714+
public function testBug10714(): void
715+
{
716+
$this->analyse([__DIR__ . '/data/bug-10714.php'], [
717+
[
718+
'Expression on left side of ?? is not nullable.',
719+
12,
720+
],
721+
[
722+
'Variable $a on left side of ?? always exists and is not nullable.',
723+
18,
724+
],
725+
[
726+
'Variable $a on left side of ?? always exists and is not nullable.',
727+
24,
728+
],
729+
[
730+
'Variable $a on left side of ?? always exists and is not nullable.',
731+
30,
732+
],
733+
[
734+
'Variable $a on left side of ??= always exists and is not nullable.',
735+
35,
736+
],
737+
[
738+
'Offset \'k\' on array{k: string} on left side of ??= always exists and is not nullable.',
739+
48,
740+
],
741+
]);
742+
}
743+
714744
#[RequiresPhp('>= 8.2.0')]
715745
public function testReadonlyPropertyOnNullableReceiver(): void
716746
{
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug10714;
4+
5+
/**
6+
* @param array<string, string> $info
7+
*/
8+
function issueLayout(array $info, string $other): string
9+
{
10+
return strtolower(
11+
($info['test'] ?? 'default') . '-' .
12+
$other ?? 'other'
13+
);
14+
}
15+
16+
function operatorStartsTheLine(string $a): string
17+
{
18+
return $a
19+
?? 'b';
20+
}
21+
22+
function operatorEndsTheLine(string $a): string
23+
{
24+
return $a ??
25+
'b';
26+
}
27+
28+
function singleLine(string $a): string
29+
{
30+
return $a ?? 'b';
31+
}
32+
33+
function assignOperatorStartsTheLine(string $a): string
34+
{
35+
$a
36+
??= 'b';
37+
38+
return $a;
39+
}
40+
41+
/**
42+
* @param array{k: string} $s
43+
*/
44+
function assignOperatorAfterMultiLineLeftSide(array $s): string
45+
{
46+
$s[
47+
'k'
48+
] ??= 'b';
49+
50+
return $s['k'];
51+
}

0 commit comments

Comments
 (0)