Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Add NoRedundantInlineAnnotationsRule #12

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP] Add NoRedundantInlineAnnotationsRule #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions lib/NoUselessInlineAnnotationsRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

declare(strict_types=1);

namespace SlamPhpStan;

use PhpParser\Node;
use PhpParser\Node\Expr\Assign;
use PHPStan\Analyser\Scope;
use PHPStan\Broker\Broker;
use PHPStan\Rules\Rule;

final class NoUselessInlineAnnotationsRule implements Rule
{
/**
* @var Broker
*/
private $broker;

public function __construct(Broker $broker)
{
$this->broker = $broker;
}

public function getNodeType(): string
{
return Assign::class;
}

/**
* @param \PhpParser\Node\Expr\Assign $node
* @param \PHPStan\Analyser\Scope $scope
*
* @return string[] errors
*/
public function processNode(Node $node, Scope $scope): array
{
r($node);
}
}
45 changes: 45 additions & 0 deletions tests/NoUselessInlineAnnotationsRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

declare(strict_types=1);

namespace SlamPhpStan\Tests;

use PHPStan\Rules\Rule;
use PHPStan\Testing\RuleTestCase;
use SlamPhpStan\NoUselessInlineAnnotationsRule;

/**
* @covers \SlamPhpStan\NoUselessInlineAnnotationsRule
*/
final class NoUselessInlineAnnotationsRuleTest extends RuleTestCase
{
protected function getRule(): Rule
{
$broker = $this->createBroker();

return new NoUselessInlineAnnotationsRule($broker);
}

public function testClassConstant()
{
$this->analyse(
[
__DIR__ . '/TestAsset/NoUselessInlineAnnotationsRule/fixture.php',
],
[
[
'Function mkdir is unsafe to use, rely on Symfony component Filesystem::mkdir instead.',
9,
],
[
'Function readlink is unsafe to use, rely on Symfony component Filesystem::readlink instead.',
12,
],
[
'Function touch is unsafe to use, rely on Symfony component Filesystem::touch instead.',
15,
],
]
);
}
}
77 changes: 77 additions & 0 deletions tests/TestAsset/NoUselessInlineAnnotationsRule/fixture.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?php

namespace SlamPhpStan\Tests\TestAsset\NoUselessInlineAnnotationsRule;

class Foo {
public function tester()
{
/** @var Bar[] $foo1 */
$foo1 = $this->bar1();

/** @var Bar $foo2 */
$foo2 = $this->bar2();
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ondrejmirtes after using more and more @phpstan I now trust it much more than developers annotations: now I'm scared that in the future this kind of redundant annotation (::bar2 already returns Bar) will hide bugs if the related API change.

Is such a rule, NoRedundantInlineAnnotationsRule, feasable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, first of all, make sure to run PHPStan with conf/bleedingEdge.neon (include it in phpstan.neon), it contains some additional rules around @var.

But the main problem is that you need to consider these things:

  1. @var is sometimes used for more precise casting - I get object and I know it's Foo.
  2. @var is sometimes used for fixing wrong 3rd party phpDoc - I get string and I know it's int. This point makes any type verification impossible.
  3. Sometimes @var is used because PHPStan understands the type, but IDE doesn't.

These points make any general rule impossible. But some more proprietary variant is possible to implement, very easily. I'd recommend you to look how those bleeding edge rules are implemented in 0.11.9 (https://github.com/phpstan/phpstan/releases/tag/0.11.9) - commits are linked in the release notes.


/** @var Bar $foo2bis */
$foo2bis = $this->bar2bis();

/** @var Bar[] $foo3 */
$foo3 = $this->bar3();

/** @var Bar $foo4 */
$foo4 = $this->bar4();

$no = $this->bar1();
$no = $this->bar2();
$no = $this->bar2bis();
$no = $this->bar3();
$no = $this->bar4();

/** @var Bar[] $xyz */
$foo1 = $this->bar1();

/** @var Bar $xyz */
$foo2 = $this->bar2();

/** @var Bar $xyz */
$foo2bis = $this->bar2bis();

/** @var Bar[] $xyz */
$foo3 = $this->bar3();

/** @var Bar $xyz */
$foo4 = $this->bar4();
}

/**
* @return Bar[]
*/
public function bar1(): array
{
return [new Bar()];
}

public function bar2(): Bar
{
return new Bar();
}

/**
* @return Bar
*/
public function bar2bis()
{
return new Bar();
}

public function bar3(): array
{
return [new Bar()];
}

public function bar4()
{
return new Bar();
}
}

class Bar {}