Skip to content

Commit 2902a04

Browse files
CayanCayan
committed
Set the null coalesce error line where the error is built
- IssetCheck::check() takes an optional line and passes it to every error it builds, so NullCoalesceRule no longer copies an error to change its line and nothing else is lost. isset() and empty() pass no line and keep reporting where they did. - The unnecessary `?? null` error gets the same line. - Tests cover an unnecessary `?? null` and a property fetch whose left side spans lines.
1 parent f34a29b commit 2902a04

4 files changed

Lines changed: 76 additions & 28 deletions

File tree

‎src/Rules/IssetCheck.php‎

Lines changed: 43 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -42,20 +42,21 @@ public function __construct(
4242
/**
4343
* @param ErrorIdentifier $identifier
4444
* @param callable(Type): ?string $typeMessageCallback
45+
* @param int|null $line null reports the error on the line of the node the error is attached to
4546
*/
46-
public function check(ExpressionResult $exprResult, Scope $scope, string $operatorDescription, string $identifier, callable $typeMessageCallback): ?IdentifierRuleError
47+
public function check(ExpressionResult $exprResult, Scope $scope, string $operatorDescription, string $identifier, callable $typeMessageCallback, ?int $line = null): ?IdentifierRuleError
4748
{
4849
$walkScope = $scope->toWalkScope();
4950
$resolution = $exprResult->getIssetabilityResolution($walkScope, !$this->treatPhpDocTypesAsCertain, true);
5051

51-
return $this->doCheck($resolution, $walkScope, $operatorDescription, $identifier, $typeMessageCallback, null);
52+
return $this->doCheck($resolution, $walkScope, $operatorDescription, $identifier, $typeMessageCallback, null, $line);
5253
}
5354

5455
/**
5556
* @param ErrorIdentifier $identifier
5657
* @param callable(Type): ?string $typeMessageCallback
5758
*/
58-
private function doCheck(IssetabilityResolution $resolution, MutatingScope $scope, string $operatorDescription, string $identifier, callable $typeMessageCallback, ?IdentifierRuleError $error): ?IdentifierRuleError
59+
private function doCheck(IssetabilityResolution $resolution, MutatingScope $scope, string $operatorDescription, string $identifier, callable $typeMessageCallback, ?IdentifierRuleError $error, ?int $line): ?IdentifierRuleError
5960
{
6061
$link = $resolution->getLink();
6162
$inner = $resolution->getInner();
@@ -80,11 +81,12 @@ private function doCheck(IssetabilityResolution $resolution, MutatingScope $scop
8081
$typeMessageCallback,
8182
$identifier,
8283
'variable',
84+
$line,
8385
);
8486
}
8587
}
8688

87-
return RuleErrorBuilder::message(sprintf('Variable $%s %s is never defined.', $link->getVariableName(), $operatorDescription))
89+
return $this->errorBuilder(sprintf('Variable $%s %s is never defined.', $link->getVariableName(), $operatorDescription), $line)
8890
->identifier(sprintf('%s.variable', $identifier))
8991
->build();
9092
}
@@ -95,7 +97,7 @@ private function doCheck(IssetabilityResolution $resolution, MutatingScope $scop
9597
if ($link->isOffset()) {
9698
$type = $link->getVarType();
9799
if (!$link->getIsOffsetAccessible()->yes()) {
98-
return $error ?? $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier);
100+
return $error ?? $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier, $line);
99101
}
100102

101103
$dimType = $link->getDimType();
@@ -105,13 +107,14 @@ private function doCheck(IssetabilityResolution $resolution, MutatingScope $scop
105107
return null;
106108
}
107109

108-
return RuleErrorBuilder::message(
110+
return $this->errorBuilder(
109111
sprintf(
110112
'Offset %s on %s %s does not exist.',
111113
$dimType->describe(VerbosityLevel::value()),
112114
$type->describe(VerbosityLevel::value()),
113115
$operatorDescription,
114116
),
117+
$line,
115118
)->identifier(sprintf('%s.offset', $identifier))->build();
116119
}
117120

@@ -127,10 +130,10 @@ private function doCheck(IssetabilityResolution $resolution, MutatingScope $scop
127130
$dimType->describe(VerbosityLevel::value()),
128131
$type->describe(VerbosityLevel::value()),
129132
$operatorDescription,
130-
), $typeMessageCallback, $identifier, 'offset');
133+
), $typeMessageCallback, $identifier, 'offset', $line);
131134

132135
if ($error !== null) {
133-
return $inner !== null ? $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error) : $error;
136+
return $inner !== null ? $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error, $line) : $error;
134137
}
135138
}
136139

@@ -143,7 +146,7 @@ private function doCheck(IssetabilityResolution $resolution, MutatingScope $scop
143146
$propertyFetch = $link->getPropertyFetch();
144147

145148
if ($reflection === null || !$link->isReflectionNative()) {
146-
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier);
149+
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier, $line);
147150
}
148151

149152
if ($link->hasNativeType() && !$link->isVirtual()->yes()) {
@@ -165,6 +168,7 @@ static function (Type $type) use ($typeMessageCallback): ?string {
165168
},
166169
$identifier,
167170
'initializedProperty',
171+
$line,
168172
);
169173
}
170174

@@ -182,11 +186,11 @@ static function (Type $type) use ($typeMessageCallback): ?string {
182186
$propertyType = $reflection->getWritableType();
183187
if ($error !== null) {
184188
return $inner !== null
185-
? $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error)
189+
? $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error, $line)
186190
: $error;
187191
}
188192
if (!$this->checkAdvancedIsset) {
189-
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier);
193+
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier, $line);
190194
}
191195

192196
$error = $this->generateError(
@@ -195,10 +199,11 @@ static function (Type $type) use ($typeMessageCallback): ?string {
195199
$typeMessageCallback,
196200
$identifier,
197201
'property',
202+
$line,
198203
);
199204

200205
if ($error !== null && $inner !== null) {
201-
return $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error);
206+
return $this->doCheck($inner, $scope, $operatorDescription, $identifier, $typeMessageCallback, $error, $line);
202207
}
203208

204209
return $error;
@@ -219,6 +224,7 @@ static function (Type $type) use ($typeMessageCallback): ?string {
219224
$typeMessageCallback,
220225
$identifier,
221226
'expr',
227+
$line,
222228
);
223229
if ($error !== null) {
224230
return $error;
@@ -227,12 +233,12 @@ static function (Type $type) use ($typeMessageCallback): ?string {
227233
if ($link->leafIsNullsafePropertyFetch()) {
228234
$leafExpr = $link->getLeafExpr();
229235
if ($leafExpr instanceof NullsafePropertyFetch && $leafExpr->name instanceof Identifier) {
230-
return RuleErrorBuilder::message(sprintf('Using nullsafe property access "?->%s" %s is unnecessary. Use -> instead.', $leafExpr->name->name, $operatorDescription))
236+
return $this->errorBuilder(sprintf('Using nullsafe property access "?->%s" %s is unnecessary. Use -> instead.', $leafExpr->name->name, $operatorDescription), $line)
231237
->identifier('nullsafe.neverNull')
232238
->build();
233239
}
234240

235-
return RuleErrorBuilder::message(sprintf('Using nullsafe property access "?->(Expression)" %s is unnecessary. Use -> instead.', $operatorDescription))
241+
return $this->errorBuilder(sprintf('Using nullsafe property access "?->(Expression)" %s is unnecessary. Use -> instead.', $operatorDescription), $line)
236242
->identifier('nullsafe.neverNull')
237243
->build();
238244
}
@@ -243,7 +249,7 @@ static function (Type $type) use ($typeMessageCallback): ?string {
243249
/**
244250
* @param ErrorIdentifier $identifier
245251
*/
246-
private function checkUndefinedInner(?IssetabilityResolution $resolution, MutatingScope $scope, string $operatorDescription, string $identifier): ?IdentifierRuleError
252+
private function checkUndefinedInner(?IssetabilityResolution $resolution, MutatingScope $scope, string $operatorDescription, string $identifier, ?int $line): ?IdentifierRuleError
247253
{
248254
if ($resolution === null) {
249255
return null;
@@ -257,32 +263,33 @@ private function checkUndefinedInner(?IssetabilityResolution $resolution, Mutati
257263
return null;
258264
}
259265

260-
return RuleErrorBuilder::message(sprintf('Variable $%s %s is never defined.', $link->getVariableName(), $operatorDescription))
266+
return $this->errorBuilder(sprintf('Variable $%s %s is never defined.', $link->getVariableName(), $operatorDescription), $line)
261267
->identifier(sprintf('%s.variable', $identifier))
262268
->build();
263269
}
264270

265271
if ($link->isOffset()) {
266272
if (!$link->getIsOffsetAccessible()->yes()) {
267-
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier);
273+
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier, $line);
268274
}
269275

270276
if (!$link->getHasOffsetValue()->no()) {
271-
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier);
277+
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier, $line);
272278
}
273279

274-
return RuleErrorBuilder::message(
280+
return $this->errorBuilder(
275281
sprintf(
276282
'Offset %s on %s %s does not exist.',
277283
$link->getDimType()->describe(VerbosityLevel::value()),
278284
$link->getVarType()->describe(VerbosityLevel::value()),
279285
$operatorDescription,
280286
),
287+
$line,
281288
)->identifier(sprintf('%s.offset', $identifier))->build();
282289
}
283290

284291
if ($link->isProperty()) {
285-
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier);
292+
return $this->checkUndefinedInner($inner, $scope, $operatorDescription, $identifier, $line);
286293
}
287294

288295
return null;
@@ -293,16 +300,30 @@ private function checkUndefinedInner(?IssetabilityResolution $resolution, Mutati
293300
* @param ErrorIdentifier $identifier
294301
* @param 'variable'|'offset'|'property'|'expr'|'initializedProperty' $identifierSecondPart
295302
*/
296-
private function generateError(Type $type, string $message, callable $typeMessageCallback, string $identifier, string $identifierSecondPart): ?IdentifierRuleError
303+
private function generateError(Type $type, string $message, callable $typeMessageCallback, string $identifier, string $identifierSecondPart, ?int $line): ?IdentifierRuleError
297304
{
298305
$typeMessage = $typeMessageCallback($type);
299306
if ($typeMessage === null) {
300307
return null;
301308
}
302309

303-
return RuleErrorBuilder::message(
310+
return $this->errorBuilder(
304311
sprintf('%s %s.', $message, $typeMessage),
312+
$line,
305313
)->identifier(sprintf('%s.%s', $identifier, $identifierSecondPart))->build();
306314
}
307315

316+
/**
317+
* @return RuleErrorBuilder<RuleError>
318+
*/
319+
private function errorBuilder(string $message, ?int $line): RuleErrorBuilder
320+
{
321+
$builder = RuleErrorBuilder::message($message);
322+
if ($line === null) {
323+
return $builder;
324+
}
325+
326+
return $builder->line($line);
327+
}
328+
308329
}

‎src/Rules/Variables/NullCoalesceRule.php‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -61,18 +61,14 @@ static function (Type $type): ?string {
6161

6262
return 'is not nullable';
6363
},
64+
$this->getOperatorLine($node),
6465
) ?? $this->checkUnnecessaryNullCoalesce($node, $scope);
6566

6667
if ($error === null) {
6768
$this->constantConditionInTraitHelper->emitNoError(self::class, $scope, $subjectResult->getExpr());
6869
return [];
6970
}
7071

71-
$error = RuleErrorBuilder::message($error->getMessage())
72-
->identifier($error->getIdentifier())
73-
->line($this->getOperatorLine($node))
74-
->build();
75-
7672
if ($scope->isInTrait()) {
7773
// The error messages already distinguish the possible outcomes,
7874
// so the contexts only need to be told apart by error/no error.
@@ -146,7 +142,7 @@ private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scop
146142

147143
return RuleErrorBuilder::message(
148144
sprintf('Coalesce operator %s is unnecessary because the left side is always set and the right side is null.', $operator),
149-
)->identifier('nullCoalesce.unnecessary')->build();
145+
)->identifier('nullCoalesce.unnecessary')->line($this->getOperatorLine($node))->build();
150146
}
151147

152148
/**

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,14 @@ public function testBug10714(): void
738738
'Offset \'k\' on array{k: string} on left side of ??= always exists and is not nullable.',
739739
48,
740740
],
741+
[
742+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
743+
60,
744+
],
745+
[
746+
'Property Bug10714\\PropertyOnMultiLineLeftSide::$p (string) on left side of ?? is not nullable.',
747+
71,
748+
],
741749
]);
742750
}
743751

‎tests/PHPStan/Rules/Variables/data/bug-10714.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,26 @@ function assignOperatorAfterMultiLineLeftSide(array $s): string
4949

5050
return $s['k'];
5151
}
52+
53+
/**
54+
* @param array{k: string|null} $s
55+
*/
56+
function unnecessaryCoalesceAfterMultiLineLeftSide(array $s): ?string
57+
{
58+
return $s[
59+
'k'
60+
] ?? null;
61+
}
62+
63+
class PropertyOnMultiLineLeftSide
64+
{
65+
66+
private string $p = '';
67+
68+
public function get(): string
69+
{
70+
return $this
71+
->p ?? 'x';
72+
}
73+
74+
}

0 commit comments

Comments
 (0)