Skip to content

Commit 8ddaba5

Browse files
jack-wormanjack-worman
committed
Report redundant inline @var tags with reportRedundantVarTag
1 parent 6ebd2d7 commit 8ddaba5

8 files changed

Lines changed: 346 additions & 12 deletions

‎conf/config.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ parameters:
104104
reportStaticMethodSignatures: false
105105
reportWrongPhpDocTypeInVarTag: false
106106
reportAnyTypeWideningInVarTag: false
107+
reportRedundantVarTag: false
107108
reportNonIntStringArrayKey: false
108109
reportUnsafeArrayStringKeyCasting: null
109110
reportPossiblyNonexistentGeneralArrayOffset: false

‎conf/parametersSchema.neon‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ parametersSchema:
108108
reportStaticMethodSignatures: bool()
109109
reportWrongPhpDocTypeInVarTag: bool()
110110
reportAnyTypeWideningInVarTag: bool()
111+
reportRedundantVarTag: bool()
111112
reportNonIntStringArrayKey: bool()
112113
reportUnsafeArrayStringKeyCasting: schema(string(), pattern('detect|prevent'), nullable())
113114
reportPossiblyNonexistentGeneralArrayOffset: bool()

‎src/Rules/PhpDoc/VarTagChangedExpressionTypeRule.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public function getNodeType(): string
2626

2727
public function processNode(Node $node, Scope $scope): array
2828
{
29-
return $this->varTagTypeRuleHelper->checkExprType($scope, $node->getExpr(), $node->getVarTag()->getType());
29+
return $this->varTagTypeRuleHelper->checkExprType($scope, $node->getExpr(), $node->getVarTag()->getType(), true);
3030
}
3131

3232
}

‎src/Rules/PhpDoc/VarTagTypeRuleHelper.php‎

Lines changed: 64 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use PhpParser\Node;
66
use PhpParser\Node\Expr;
7+
use PHPStan\Analyser\MutatingScope;
78
use PHPStan\Analyser\NameScope;
89
use PHPStan\Analyser\Scope;
910
use PHPStan\DependencyInjection\AutowiredParameter;
@@ -46,17 +47,22 @@ public function __construct(
4647
private bool $checkTypeAgainstPhpDocType,
4748
#[AutowiredParameter(ref: '%reportAnyTypeWideningInVarTag%')]
4849
private bool $strictWideningCheck,
50+
#[AutowiredParameter(ref: '%reportRedundantVarTag%')]
51+
private bool $reportRedundantVarTag,
4952
private InitializerExprTypeResolver $initializerExprTypeResolver,
5053
)
5154
{
5255
}
5356

5457
/**
58+
* $canBeRedundant is false where an equal type does not make the tag
59+
* redundant, e.g. the default of a static variable.
60+
*
5561
* @param VarTag[] $varTags
5662
* @param string[] $assignedVariables
5763
* @return list<IdentifierRuleError>
5864
*/
59-
public function checkVarType(Scope $scope, Node\Expr $var, Node\Expr $expr, array $varTags, array $assignedVariables): array
65+
public function checkVarType(Scope $scope, Node\Expr $var, Node\Expr $expr, array $varTags, array $assignedVariables, bool $canBeRedundant): array
6066
{
6167
$errors = [];
6268

@@ -69,7 +75,7 @@ public function checkVarType(Scope $scope, Node\Expr $var, Node\Expr $expr, arra
6975
return [];
7076
}
7177

72-
return $this->checkExprType($scope, $expr, $varTagType);
78+
return $this->checkExprType($scope, $expr, $varTagType, $canBeRedundant);
7379
} elseif ($var instanceof Expr\List_ || $var instanceof Expr\Array_) {
7480
foreach ($var->items as $i => $arrayItem) {
7581
if ($arrayItem === null) {
@@ -87,7 +93,7 @@ public function checkVarType(Scope $scope, Node\Expr $var, Node\Expr $expr, arra
8793
$itemErrors = $this->checkVarType($scope, $arrayItem->value, new NativeTypeExpr(
8894
$scope->getType($expr)->getOffsetValueType($scope->getType($dimExpr)),
8995
$scope->getNativeType($expr)->getOffsetValueType($scope->getNativeType($dimExpr)),
90-
), $varTags, $assignedVariables);
96+
), $varTags, $assignedVariables, $canBeRedundant);
9197
foreach ($itemErrors as $error) {
9298
$errors[] = $error;
9399
}
@@ -100,7 +106,7 @@ public function checkVarType(Scope $scope, Node\Expr $var, Node\Expr $expr, arra
100106
/**
101107
* @return list<IdentifierRuleError>
102108
*/
103-
public function checkExprType(Scope $scope, Node\Expr $expr, Type $varTagType): array
109+
public function checkExprType(Scope $scope, Node\Expr $expr, Type $varTagType, bool $canBeRedundant): array
104110
{
105111
$errors = [];
106112
// a constant expression (a static variable's `= 1` default, a literal) is
@@ -143,9 +149,63 @@ public function checkExprType(Scope $scope, Node\Expr $expr, Type $varTagType):
143149
}
144150
}
145151

152+
if (
153+
$canBeRedundant
154+
&& $this->reportRedundantVarTag
155+
&& count($errors) === 0
156+
// a trait is analysed once per class using it, and the tag may be
157+
// redundant in only some of them
158+
&& !$scope->isInTrait()
159+
&& !$this->hasResolvedTemplateArguments($scope)
160+
) {
161+
$exprType = $constantType ?? $scope->getScopeType($expr);
162+
if (
163+
$exprType->equals($varTagType)
164+
// equals() ignores whether mixed is explicit, but a tag spelling out
165+
// mixed makes the implicit mixed of the expression explicit
166+
&& !($this->containsMixed($exprType, false) && $this->containsMixed($varTagType, true))
167+
) {
168+
$errors[] = RuleErrorBuilder::message(sprintf(
169+
'PHPDoc tag @var with type %s is redundant, the expression already has this type.',
170+
$varTagType->describe(VerbosityLevel::getRecommendedLevelByType($varTagType)),
171+
))->identifier('varTag.redundant')->build();
172+
}
173+
}
174+
146175
return $errors;
147176
}
148177

178+
/**
179+
* Whether the statement is walked with template arguments resolved from
180+
* their usages. A @var tag is one of those usages, so the expression may
181+
* have the tag's type only because of the tag.
182+
*/
183+
private function hasResolvedTemplateArguments(Scope $scope): bool
184+
{
185+
if (!$scope instanceof MutatingScope) {
186+
return false;
187+
}
188+
189+
$frame = $scope->getCurrentTemplateArgumentFrame();
190+
191+
return $frame !== null && !$frame->isObserving();
192+
}
193+
194+
private function containsMixed(Type $type, bool $explicit): bool
195+
{
196+
$containsMixed = false;
197+
TypeTraverser::map($type, static function (Type $type, callable $traverse) use ($explicit, &$containsMixed): Type {
198+
if ($type instanceof MixedType && $type->isExplicitMixed() === $explicit) {
199+
$containsMixed = true;
200+
return $type;
201+
}
202+
203+
return $traverse($type);
204+
});
205+
206+
return $containsMixed;
207+
}
208+
149209
private function containsPhpStanType(Type $type): bool
150210
{
151211
$classReflections = TypeUtils::toBenevolentUnion($type)->getObjectClassReflections();

‎src/Rules/PhpDoc/WrongVariableNameInVarTagRule.php‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use PhpParser\Comment\Doc;
66
use PhpParser\Node;
77
use PhpParser\Node\Expr;
8+
use PhpParser\NodeFinder;
89
use PHPStan\Analyser\Scope;
910
use PHPStan\DependencyInjection\RegisteredRule;
1011
use PHPStan\Node\Expr\NativeTypeExpr;
@@ -147,7 +148,7 @@ public function processNode(Node $node, Scope $scope): array
147148
* @param VarTag[] $varTags
148149
* @return list<IdentifierRuleError>
149150
*/
150-
private function processAssign(Scope $scope, Node\Expr $var, Node\Expr $expr, array $varTags): array
151+
private function processAssign(Scope $scope, Node\Expr $var, Node\Expr $expr, array $varTags, bool $canBeRedundant): array
151152
{
152153
$errors = [];
153154
$hasMultipleMessage = false;
@@ -191,7 +192,13 @@ private function processAssign(Scope $scope, Node\Expr $var, Node\Expr $expr, ar
191192
}
192193

193194
if (count($errors) === 0) {
194-
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $var, $expr, $varTags, $assignedVariables) as $error) {
195+
// in a loop, the tagged type of a variable the expression reads flows
196+
// back into the expression, so an equal type does not prove the tag redundant
197+
$canBeRedundant = $canBeRedundant && (new NodeFinder())->findFirst(
198+
[$expr],
199+
static fn (Node $node): bool => $node instanceof Expr\Variable && in_array($node->name, $assignedVariables, true),
200+
) === null;
201+
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $var, $expr, $varTags, $assignedVariables, $canBeRedundant) as $error) {
195202
$errors[] = $error;
196203
}
197204
}
@@ -266,23 +273,23 @@ private function processForeach(Scope $scope, Node\Expr $iterateeExpr, ?Node\Exp
266273
))->identifier('varTag.differentVariable')->build();
267274
}
268275

269-
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $iterateeExpr, $iterateeExpr, $varTags, $variableNames) as $error) {
276+
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $iterateeExpr, $iterateeExpr, $varTags, $variableNames, true) as $error) {
270277
$errors[] = $error;
271278
}
272279
if ($keyVar !== null) {
273280
$keyTypeExpr = new NativeTypeExpr(
274281
$scope->getIterableKeyType($scope->getScopeType($iterateeExpr)),
275282
$scope->getIterableKeyType($scope->getScopeNativeType($iterateeExpr)),
276283
);
277-
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $keyVar, $keyTypeExpr, $varTags, $variableNames) as $error) {
284+
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $keyVar, $keyTypeExpr, $varTags, $variableNames, true) as $error) {
278285
$errors[] = $error;
279286
}
280287
}
281288
$valueTypeExpr = new NativeTypeExpr(
282289
$scope->getIterableValueType($scope->getScopeType($iterateeExpr)),
283290
$scope->getIterableValueType($scope->getScopeNativeType($iterateeExpr)),
284291
);
285-
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $valueVar, $valueTypeExpr, $varTags, $variableNames) as $error) {
292+
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $valueVar, $valueTypeExpr, $varTags, $variableNames, true) as $error) {
286293
$errors[] = $error;
287294
}
288295

@@ -300,7 +307,7 @@ private function processExpression(Scope $scope, Expr $expr, array $varTags): ar
300307
|| $expr instanceof Node\Expr\AssignRef
301308
|| $expr instanceof Node\Expr\AssignOp
302309
) {
303-
return $this->processAssign($scope, $expr->var, $expr->expr, $varTags);
310+
return $this->processAssign($scope, $expr->var, $expr->expr, $varTags, !$expr instanceof Node\Expr\AssignOp);
304311
}
305312

306313
return $this->processStmt($scope, $varTags, null);
@@ -350,7 +357,7 @@ private function processStatic(Scope $scope, array $vars, array $varTags): array
350357
if ($var->default === null) {
351358
continue;
352359
}
353-
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $var->var, $var->default, $varTags, $variableNames) as $error) {
360+
foreach ($this->varTagTypeRuleHelper->checkVarType($scope, $var->var, $var->default, $varTags, $variableNames, false) as $error) {
354361
$errors[] = $error;
355362
}
356363
}

‎tests/PHPStan/Rules/PhpDoc/VarTagChangedExpressionTypeRuleTest.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
class VarTagChangedExpressionTypeRuleTest extends RuleTestCase
1515
{
1616

17+
private bool $reportRedundantVarTag = false;
18+
1719
protected function getRule(): Rule
1820
{
1921
$container = self::getContainer();
@@ -23,6 +25,7 @@ protected function getRule(): Rule
2325
self::createReflectionProvider(),
2426
true,
2527
true,
28+
$this->reportRedundantVarTag,
2629
self::getContainer()->getByType(InitializerExprTypeResolver::class),
2730
));
2831
}
@@ -103,4 +106,19 @@ public function testBug12708(): void
103106
]);
104107
}
105108

109+
public function testRedundantVarTag(): void
110+
{
111+
$this->reportRedundantVarTag = true;
112+
$this->analyse([__DIR__ . '/data/redundant-var-tag.php'], [
113+
[
114+
'PHPDoc tag @var with type string is redundant, the expression already has this type.',
115+
130,
116+
],
117+
[
118+
'PHPDoc tag @var with type Exception is redundant, the expression already has this type.',
119+
142,
120+
],
121+
]);
122+
}
123+
106124
}

‎tests/PHPStan/Rules/PhpDoc/WrongVariableNameInVarTagRuleTest.php‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ class WrongVariableNameInVarTagRuleTest extends RuleTestCase
2020

2121
private bool $strictWideningCheck = false;
2222

23+
private bool $reportRedundantVarTag = false;
24+
2325
protected function getRule(): Rule
2426
{
2527
$container = self::getContainer();
@@ -31,6 +33,7 @@ protected function getRule(): Rule
3133
self::createReflectionProvider(),
3234
$this->checkTypeAgainstPhpDocType,
3335
$this->strictWideningCheck,
36+
$this->reportRedundantVarTag,
3437
self::getContainer()->getByType(InitializerExprTypeResolver::class),
3538
),
3639
);
@@ -665,4 +668,55 @@ public function testBug15448ClassNotAnalysed(): void
665668
$this->analyse([__DIR__ . '/data/bug-15448-dependency-usage.php'], []);
666669
}
667670

671+
public function testRedundantVarTag(): void
672+
{
673+
$this->reportRedundantVarTag = true;
674+
$this->analyse([__DIR__ . '/data/redundant-var-tag.php'], [
675+
[
676+
'PHPDoc tag @var with type string is redundant, the expression already has this type.',
677+
39,
678+
],
679+
[
680+
'PHPDoc tag @var with type string is redundant, the expression already has this type.',
681+
42,
682+
],
683+
[
684+
'PHPDoc tag @var with type string is redundant, the expression already has this type.',
685+
45,
686+
],
687+
[
688+
'PHPDoc tag @var with type list<int> is redundant, the expression already has this type.',
689+
57,
690+
],
691+
[
692+
'PHPDoc tag @var with type array<string, mixed> is redundant, the expression already has this type.',
693+
60,
694+
],
695+
[
696+
'PHPDoc tag @var with type mixed is redundant, the expression already has this type.',
697+
63,
698+
],
699+
[
700+
'PHPDoc tag @var with type int is redundant, the expression already has this type.',
701+
72,
702+
],
703+
[
704+
'PHPDoc tag @var with type string is redundant, the expression already has this type.',
705+
75,
706+
],
707+
[
708+
'PHPDoc tag @var with type int is redundant, the expression already has this type.',
709+
83,
710+
],
711+
[
712+
'PHPDoc tag @var with type int<0, max> is redundant, the expression already has this type.',
713+
87,
714+
],
715+
[
716+
'PHPDoc tag @var with type list<int> is redundant, the expression already has this type.',
717+
91,
718+
],
719+
]);
720+
}
721+
668722
}

0 commit comments

Comments
 (0)