Skip to content

Commit

Permalink
DiagnoseExtension for mixed calls (#107)
Browse files Browse the repository at this point in the history
  • Loading branch information
janedbal authored Nov 4, 2024
1 parent 482392a commit 9636567
Show file tree
Hide file tree
Showing 7 changed files with 138 additions and 22 deletions.
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,14 @@ parameters:
trackCallsOnMixed: false
```

- If you want to check how many of those cases are present in your codebase, you can run PHPStan analysis with `-vvv` and you will see some diagnostics:

```
Found 2 methods called over unknown type:
• setCountry, for example in App\Entity\User::updateAddress
• setStreet, for example in App\Entity\User::updateAddress
```

## Comparison with tomasvotruba/unused-public
- You can see [detailed comparison PR](https://github.com/shipmonk-rnd/dead-code-detector/pull/53)
- Basically, their analysis is less precise and less flexible. Mainly:
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
],
"require": {
"php": "^7.4 || ^8.0",
"phpstan/phpstan": "^1.11.5"
"phpstan/phpstan": "^1.11.7"
},
"require-dev": {
"doctrine/orm": "^2.19 || ^3.0",
Expand Down
2 changes: 1 addition & 1 deletion composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions rules.neon
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ services:
class: ShipMonk\PHPStan\DeadCode\Rule\DeadMethodRule
tags:
- phpstan.rules.rule
- phpstan.diagnoseExtension
arguments:
reportTransitivelyDeadMethodAsSeparateError: %shipmonkDeadCode.reportTransitivelyDeadMethodAsSeparateError%

Expand Down
51 changes: 50 additions & 1 deletion src/Rule/DeadMethodRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use LogicException;
use PhpParser\Node;
use PHPStan\Analyser\Scope;
use PHPStan\Command\Output;
use PHPStan\Diagnose\DiagnoseExtension;
use PHPStan\Node\CollectedDataNode;
use PHPStan\Rules\IdentifierRuleError;
use PHPStan\Rules\Rule;
Expand All @@ -21,14 +23,17 @@
use function array_keys;
use function array_merge;
use function array_merge_recursive;
use function array_slice;
use function count;
use function explode;
use function in_array;
use function sprintf;
use function strpos;

/**
* @implements Rule<CollectedDataNode>
*/
class DeadMethodRule implements Rule
class DeadMethodRule implements Rule, DiagnoseExtension
{

public const ERROR_IDENTIFIER = 'shipmonk.deadMethod';
Expand Down Expand Up @@ -551,4 +556,48 @@ private function isNeverReportedAsDead(string $methodKey): bool
return false;
}

public function print(Output $output): void
{
if ($this->mixedCalls === [] || !$output->isDebug()) {
return;
}

$maxExamplesToShow = 20;
$output->writeLineFormatted(sprintf('<fg=red>Found %d methods called over unknown type</>:', count($this->mixedCalls)));

foreach (array_slice($this->mixedCalls, 0, $maxExamplesToShow) as $methodName => $calls) {
$output->writeFormatted(sprintf(' • <fg=white>%s</>', $methodName));

$exampleCaller = $this->getExampleCaller($calls);

if ($exampleCaller !== null) {
$output->writeFormatted(sprintf(', for example in <fg=white>%s</>', $exampleCaller));
}

$output->writeLineFormatted('');
}

if (count($this->mixedCalls) > $maxExamplesToShow) {
$output->writeLineFormatted(sprintf('... and %d more', count($this->mixedCalls) - $maxExamplesToShow));
}

$output->writeLineFormatted('');
$output->writeLineFormatted('Thus, any method named the same is considered used, no matter its declaring class!');
$output->writeLineFormatted('');
}

/**
* @param list<Call> $calls
*/
private function getExampleCaller(array $calls): ?string
{
foreach ($calls as $call) {
if ($call->caller !== null) {
return $call->caller->toString();
}
}

return null;
}

}
57 changes: 53 additions & 4 deletions tests/Rule/DeadMethodRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,18 @@ class DeadMethodRuleTest extends RuleTestCase

private bool $unwrapGroupedErrors = true;

private ?DeadMethodRule $rule = null;

protected function getRule(): DeadMethodRule
{
return new DeadMethodRule(
new ClassHierarchy(),
!$this->emitErrorsInGroups,
);
if ($this->rule === null) {
$this->rule = new DeadMethodRule(
new ClassHierarchy(),
!$this->emitErrorsInGroups,
);
}

return $this->rule;
}

/**
Expand Down Expand Up @@ -108,6 +114,49 @@ public function testMixedCallsNotTracked(): void
$this->analyseFiles([__DIR__ . '/data/DeadMethodRule/mixed/untracked.php']);
}

public function testDiagnoseMixedCalls(): void
{
$this->analyseFiles([__DIR__ . '/data/DeadMethodRule/mixed/tracked.php']);
$rule = $this->getRule();

$actualOutput = '';
$output = $this->createMock(Output::class);
$output->expects(self::once())
->method('isDebug')
->willReturn(true);
$output->expects(self::atLeastOnce())
->method('writeFormatted')
->willReturnCallback(
static function (string $message) use (&$actualOutput): void {
$actualOutput .= $message;
},
);
$output->expects(self::atLeastOnce())
->method('writeLineFormatted')
->willReturnCallback(
static function (string $message) use (&$actualOutput): void {
$actualOutput .= $message . "\n";
},
);

$rule->print($output);

$ec = ''; // hack editorconfig checker to ignore wrong indentation
$expectedOutput = <<<"OUTPUT"
<fg=red>Found 4 methods called over unknown type</>:
$ec • <fg=white>getter1</>, for example in <fg=white>DeadMixed1\Tester::__construct</>
$ec • <fg=white>getter2</>, for example in <fg=white>DeadMixed1\Tester::__construct</>
$ec • <fg=white>getter3</>, for example in <fg=white>DeadMixed1\Tester::__construct</>
$ec • <fg=white>staticMethod</>, for example in <fg=white>DeadMixed1\Tester::__construct</>
Thus, any method named the same is considered used, no matter its declaring class!
OUTPUT;

self::assertSame($expectedOutput, $actualOutput);
}

/**
* @dataProvider provideAutoRemoveFiles
*/
Expand Down
39 changes: 24 additions & 15 deletions tests/Rule/data/DeadMethodRule/mixed/tracked.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,34 @@ public function getter6() {}

}

function testIt($mixed, string $notClass, object $object, IFace $iface, int|Clazz $maybeClass) {
$mixed->getter1(); // may be valid call
class Tester
{
function __construct($mixed, string $notClass, object $object, IFace $iface, int|Clazz $maybeClass)
{
$mixed->getter1(); // may be valid call

if (!$mixed instanceof Clazz) {
$mixed->getter2(); // ideally, should mark only IFace, not Clazz (not implemented)
}
if (!$mixed instanceof Clazz) {
$mixed->getter2(); // ideally, should mark only IFace, not Clazz (not implemented)
}

$object->getter3();
$iface->getter4();
$maybeClass->getter5(); // mark only Clazz, not IFace
$object->getter3();
$iface->getter4();
$maybeClass->getter5(); // mark only Clazz, not IFace

$notClass->nonStaticMethod(); // fatal error, does not count
$notClass::staticMethod(); // may be valid call
}
$notClass->nonStaticMethod(); // fatal error, does not count
$notClass::staticMethod(); // may be valid call

function testMethodExists(Iface $iface) {
if (method_exists($iface, 'someMethod')) {
$iface->someMethod(); // does not not mark Clazz
$this->testMethodExists();
}

$iface->getter6(); // not defined on Iface, but should mark used on its implementations but not on unrelated Clazz
function testMethodExists(Iface $iface)
{
if (method_exists($iface, 'someMethod')) {
$iface->someMethod(); // does not not mark Clazz
}

$iface->getter6(); // not defined on Iface, but should mark used on its implementations but not on unrelated Clazz
}
}

new Tester();

0 comments on commit 9636567

Please sign in to comment.