Skip to content

Commit

Permalink
TooWideMethodReturnTypehintRule - always report for final methods in …
Browse files Browse the repository at this point in the history 
…bleeding edge
  • Loading branch information
@ondrejmirtes
ondrejmirtes committed Dec 7, 2023
1 parent 942afbf commit c30e9a4
Show file tree
Hide file tree
Showing 9 changed files with 226 additions and 8 deletions.
1 change: 1 addition & 0 deletions conf/bleedingEdge.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ parameters:
missingMagicSerializationRule: true
nullContextForVoidReturningFunctions: true
unescapeStrings: true
alwaysCheckTooWideReturnTypeFinalMethods: true
duplicateStubs: true
invarianceComposition: true
alwaysTrueAlwaysReported: true
Expand Down
1 change: 1 addition & 0 deletions conf/config.level4.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ services:
class: PHPStan\Rules\TooWideTypehints\TooWideMethodReturnTypehintRule
arguments:
checkProtectedAndPublicMethods: %checkTooWideReturnTypesInProtectedAndPublicMethods%
alwaysCheckFinal: %featureToggles.alwaysCheckTooWideReturnTypeFinalMethods%
tags:
- phpstan.rules.rule

Expand Down
1 change: 1 addition & 0 deletions conf/config.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ parameters:
missingMagicSerializationRule: false
nullContextForVoidReturningFunctions: false
unescapeStrings: false
alwaysCheckTooWideReturnTypeFinalMethods: false
duplicateStubs: false
invarianceComposition: false
alwaysTrueAlwaysReported: false
Expand Down
1 change: 1 addition & 0 deletions conf/parametersSchema.neon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ parametersSchema:
missingMagicSerializationRule: bool()
nullContextForVoidReturningFunctions: bool()
unescapeStrings: bool()
alwaysCheckTooWideReturnTypeFinalMethods: bool()
duplicateStubs: bool()
invarianceComposition: bool()
alwaysTrueAlwaysReported: bool()
Expand Down
17 changes: 13 additions & 4 deletions src/Rules/TooWideTypehints/TooWideMethodReturnTypehintRule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
class TooWideMethodReturnTypehintRule implements Rule
{

public function __construct(private bool $checkProtectedAndPublicMethods)
public function __construct(private bool $checkProtectedAndPublicMethods, private bool $alwaysCheckFinal)
{
}

Expand All @@ -35,10 +35,19 @@ public function processNode(Node $node, Scope $scope): array
$method = $node->getMethodReflection();
$isFirstDeclaration = $method->getPrototype()->getDeclaringClass() === $method->getDeclaringClass();
if (!$method->isPrivate()) {
if (!$this->checkProtectedAndPublicMethods) {
if ($this->alwaysCheckFinal) {
if (!$method->getDeclaringClass()->isFinal() && !$method->isFinal()->yes()) {

This comment has been minimized.

Sign in to view

Copy link
@Warxcell

Warxcell Dec 13, 2023 •
edited
Loading

shouldn't be checked only if method is not implementation of interface?

https://phpstan.org/r/c83e6d0b-98d3-489a-a31f-268017c49474

This comment has been minimized.

Sign in to view

Copy link
@ondrejmirtes

ondrejmirtes via email Dec 13, 2023

Author Member

This comment has been minimized.

Sign in to view

Copy link
@Warxcell

Warxcell Dec 13, 2023 •
edited
Loading

if you have class that implements that interface
first phpstan tells you that you should remove string because its never returned - ok, removed
then phpstan tells you that return type has no value type specified in iterable type array. https://phpstan.org/r/9cb218fa-f084-4eed-b107-ff85cd19c13c

then you have to copy-paste the docblock from interface and remove unneeded types, once again - https://phpstan.org/r/3a6a667e-eea8-47ca-8adb-e3818d0380b1

its annoying.
if (!$this->checkProtectedAndPublicMethods) {
return [];
}

if ($isFirstDeclaration) {
return [];
}
}
} elseif (!$this->checkProtectedAndPublicMethods) {
return [];
}
if ($isFirstDeclaration && !$method->getDeclaringClass()->isFinal() && !$method->isFinal()->yes()) {
} elseif ($isFirstDeclaration && !$method->getDeclaringClass()->isFinal() && !$method->isFinal()->yes()) {
return [];
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Rules/Whitespace/FileWhitespaceRule.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public function processNode(Node $node, Scope $scope): array
private array $lastNodes = [];

/**
* @return int|Node|null
* @return int|null
*/
public function enterNode(Node $node)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Type/Php/FilterFunctionReturnTypeHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public function __construct(private ReflectionProvider $reflectionProvider, priv
$this->flagsString = new ConstantStringType('flags');
}

public function getOffsetValueType(Type $inputType, Type $offsetType, ?Type $filterType, ?Type $flagsType): ?Type
public function getOffsetValueType(Type $inputType, Type $offsetType, ?Type $filterType, ?Type $flagsType): Type
{
$inexistentOffsetType = $this->hasFlag($this->getConstant('FILTER_NULL_ON_FAILURE'), $flagsType)
? new ConstantBooleanType(false)
Expand All @@ -73,7 +73,7 @@ public function getOffsetValueType(Type $inputType, Type $offsetType, ?Type $fil
: $filteredType;
}

public function getInputType(Type $typeType, Type $varNameType, ?Type $filterType, ?Type $flagsType): ?Type
public function getInputType(Type $typeType, Type $varNameType, ?Type $filterType, ?Type $flagsType): Type
{
$this->supportedFilterInputTypes ??= TypeCombinator::union(
$this->reflectionProvider->getConstant(new Node\Name('INPUT_GET'), null)->getValueType(),
Expand Down
144 changes: 143 additions & 1 deletion tests/PHPStan/Rules/TooWideTypehints/TooWideMethodReturnTypehintRuleTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@
class TooWideMethodReturnTypehintRuleTest extends RuleTestCase
{

private bool $checkProtectedAndPublicMethods = true;

private bool $alwaysCheckFinal = false;

protected function getRule(): Rule
{
return new TooWideMethodReturnTypehintRule(true);
return new TooWideMethodReturnTypehintRule($this->checkProtectedAndPublicMethods, $this->alwaysCheckFinal);
}

public function testPrivate(): void
Expand Down Expand Up @@ -102,4 +106,142 @@ public function testBug6175(): void
$this->analyse([__DIR__ . '/data/bug-6175.php'], []);
}

public function dataAlwaysCheckFinal(): iterable
{
yield [
false,
false,
[
[
'Method MethodTooWideReturnAlwaysCheckFinal\Foo::test() never returns null so it can be removed from the return type.',
8,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test() never returns null so it can be removed from the return type.',
28,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test() never returns null so it can be removed from the return type.',
48,
],
],
];

yield [
true,
false,
[
[
'Method MethodTooWideReturnAlwaysCheckFinal\Foo::test() never returns null so it can be removed from the return type.',
8,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test() never returns null so it can be removed from the return type.',
28,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test2() never returns null so it can be removed from the return type.',
33,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test3() never returns null so it can be removed from the return type.',
38,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test() never returns null so it can be removed from the return type.',
48,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test2() never returns null so it can be removed from the return type.',
53,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test3() never returns null so it can be removed from the return type.',
58,
],
],
];

yield [
false,
true,
[
[
'Method MethodTooWideReturnAlwaysCheckFinal\Foo::test() never returns null so it can be removed from the return type.',
8,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test() never returns null so it can be removed from the return type.',
28,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test2() never returns null so it can be removed from the return type.',
33,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test3() never returns null so it can be removed from the return type.',
38,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test() never returns null so it can be removed from the return type.',
48,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test2() never returns null so it can be removed from the return type.',
53,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test3() never returns null so it can be removed from the return type.',
58,
],
],
];

yield [
true,
true,
[
[
'Method MethodTooWideReturnAlwaysCheckFinal\Foo::test() never returns null so it can be removed from the return type.',
8,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test() never returns null so it can be removed from the return type.',
28,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test2() never returns null so it can be removed from the return type.',
33,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FinalFoo::test3() never returns null so it can be removed from the return type.',
38,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test() never returns null so it can be removed from the return type.',
48,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test2() never returns null so it can be removed from the return type.',
53,
],
[
'Method MethodTooWideReturnAlwaysCheckFinal\FooFinalMethods::test3() never returns null so it can be removed from the return type.',
58,
],
],
];
}

/**
* @dataProvider dataAlwaysCheckFinal
* @param list<array{0: string, 1: int, 2?: string|null}> $expectedErrors
*/
public function testAlwaysCheckFinal(bool $checkProtectedAndPublicMethods, bool $alwaysCheckFinal, array $expectedErrors): void
{
$this->checkProtectedAndPublicMethods = $checkProtectedAndPublicMethods;
$this->alwaysCheckFinal = $alwaysCheckFinal;
$this->analyse([__DIR__ . '/data/method-too-wide-return-always-check-final.php'], $expectedErrors);
}

}
63 changes: 63 additions & 0 deletions tests/PHPStan/Rules/TooWideTypehints/data/method-too-wide-return-always-check-final.php
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

namespace MethodTooWideReturnAlwaysCheckFinal;

class Foo
{

private function test(): ?int
{
return 1;
}

protected function test2(): ?int
{
return 1;
}

public function test3(): ?int
{
return 1;
}

}

final class FinalFoo
{

private function test(): ?int
{
return 1;
}

protected function test2(): ?int
{
return 1;
}

public function test3(): ?int
{
return 1;
}

}

class FooFinalMethods
{

private function test(): ?int
{
return 1;
}

final protected function test2(): ?int
{
return 1;
}

final public function test3(): ?int
{
return 1;
}

}

0 comments on commit c30e9a4

Please sign in to comment.