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

Fix issues found by static analysis #394

Merged
merged 11 commits into from
Feb 4, 2018
Merged

Fix issues found by static analysis #394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ba1ed0c
Fix issues found by Psalm
Jan 5, 2018
cc14790
Expand nullable type
Jan 5, 2018
3fac4b9
Convert assertions to docblocks
Jan 5, 2018
b09c165
Remove ternary to empty array
Jan 29, 2018
5ed9c17
Fix coding standard violations
Jan 30, 2018
b6a4914
Revert changes to void return types
Jan 31, 2018
633de24
Revert generic iterator change
Jan 31, 2018
60ac692
Revert changes to exports that will always throw exceptions
muglug Feb 2, 2018
c33fedf
Revert ReflectionClass::getProperty return type change
muglug Feb 3, 2018
688934c
Add asserts to testAdapterMethods
muglug Feb 3, 2018
b554734
Add back docblock annotation
muglug Feb 3, 2018
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
2 changes: 2 additions & 0 deletions src/NodeCompiler/CompileNodeToValue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ private function compileConstFetch(Node\Expr\ConstFetch $constNode)
*/
private function compileClassConstFetch(Node\Expr\ClassConstFetch $node, CompilerContext $context)
{
/** @var Node\Name $node->class */
Copy link
Member

Choose a reason for hiding this comment

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

This syntax is not really compatible with most tooling out there - what should we do about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it breaks anything I'm happy to remove it, or replace with an assert.

My hope is that the PSR-5 PHPDoc proposal is approved, and the people who make similar tools add support for this. It's a common-sense annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also that could be fixed with a Psalm config, as it relates to behaviour of PhpParser that will never occur given how it's used in BetterReflection. vimeo/psalm#482 would fix that internally.

Anyway, using an assert also fixes a PHPStan error at the same location so maybe that's the way to go here.

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep them for now 👍

$className = $node->class->toString();

if ($node->name === 'class') {
Expand All @@ -148,6 +149,7 @@ private function compileClassConstFetch(Node\Expr\ClassConstFetch $node, Compile
$classInfo = $context->getReflector()->reflect($className);
}

/** @var string $node->name */
Copy link
Member

Choose a reason for hiding this comment

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

Same question about compatibility: what to do here?

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this for now

$reflectionConstant = $classInfo->getReflectionConstant($node->name);

return $this->__invoke(
Expand Down
2 changes: 1 addition & 1 deletion src/Reflection/Adapter/ReflectionMethod.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ public function isDestructor()
public function getClosure($object = null)
{
try {
$this->betterReflectionMethod->getClosure($object);
return $this->betterReflectionMethod->getClosure($object);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we never tested this - that's needed here

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this needs a test addition too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test added in 74ba592

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

} catch (NoObjectProvided | NotAnObject $e) {
return null;
} catch (Throwable $e) {
Expand Down
2 changes: 1 addition & 1 deletion src/Reflection/ReflectionClass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ function (ReflectionMethod $method) use ($filter) {
*/
public function getImmediateMethods(?int $filter = null) : array
{
/** @var \ReflectionMethod[] $methods */
Copy link
Member

Choose a reason for hiding this comment

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

why removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it can be inferred easily

Copy link
Member

Choose a reason for hiding this comment

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

array_map()'s documented return value is array - unless we have generics, this isn't inferred by tooling that doesn't assume implementation details of array_map, so the change should be reverted.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, errata - it can be inferred, it's just that most tooling won't do recursive inferring of types, so this can't really be removed unless the ecosystem evolves

Copy link
Contributor Author

Choose a reason for hiding this comment

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

evolution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this was also inaccurate, which is why I made the change in the first place (it shouldn't have the leading slash)

Copy link
Member

Choose a reason for hiding this comment

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

I like the pic :D

I know the tool you built is doing it, but the majority still needs to catch up ;-)

/** @var ReflectionMethod[] $methods */
$methods = array_map(
function (ClassMethod $methodNode) : ReflectionMethod {
return ReflectionMethod::createFromNode(
Expand Down
3 changes: 0 additions & 3 deletions src/Reflection/ReflectionClassConstant.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,6 @@ public function __toString() : string
return ReflectionClassConstantStringCast::toString($this);
}

/**
* {@inheritDoc}
*/
public static function export() : void
{
throw new Exception('Unable to export statically');
Expand Down
2 changes: 2 additions & 0 deletions src/Reflection/ReflectionFunction.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ public function getClosure() : Closure
$this->assertFunctionExist($functionName);

return function (...$args) use ($functionName) {
/** @var callable $functionName */
return $functionName(...$args);
};
}
Expand Down Expand Up @@ -140,6 +141,7 @@ public function invokeArgs(array $args = [])

$this->assertFunctionExist($functionName);

/** @var callable $functionName */
return $functionName(...$args);
}

Expand Down
4 changes: 2 additions & 2 deletions src/Reflection/ReflectionFunctionAbstract.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ abstract class ReflectionFunctionAbstract implements CoreReflector
public const CLOSURE_NAME = '{closure}';

/**
* @var NamespaceNode
* @var NamespaceNode|null
*/
private $declaringNamespace;

Expand Down Expand Up @@ -75,7 +75,7 @@ public static function export() : void
/**
* Populate the common elements of the function abstract.
*
* @param Node\Stmt\ClassMethod|Node\FunctionLike|Node\Stmt|Node $node Node has to be processed by the PhpParser\NodeVisitor\NameResolver
* @param Node\Stmt\ClassMethod|Node\Stmt\Function_|Node\Expr\Closure $node Node has to be processed by the PhpParser\NodeVisitor\NameResolver
*
* @throws \Roave\BetterReflection\Reflection\Exception\InvalidAbstractFunctionNodeType
*/
Expand Down
2 changes: 0 additions & 2 deletions src/Reflection/ReflectionObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ public static function export($instance = null) : string
*
* @param object $object
*
* @return self
*
* @throws \ReflectionException
* @throws \InvalidArgumentException
* @throws \Roave\BetterReflection\Reflector\Exception\IdentifierNotFound
Expand Down
5 changes: 4 additions & 1 deletion src/Reflection/ReflectionParameter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,16 @@ private function parseDefaultValueNode() : void
$defaultValueNode = $this->node->default;

if ($defaultValueNode instanceof Node\Expr\ClassConstFetch) {
/** @var Node\Name $defaultValueNode->class */
$className = $defaultValueNode->class->toString();

if ($className === 'self' || $className === 'static') {
/** @var string $defaultValueNode->name */
$className = $this->findParentClassDeclaringConstant($defaultValueNode->name);
}

$this->isDefaultValueConstant = true;
$this->isDefaultValueConstant = true;
/** @var string $defaultValueNode->name */
$this->defaultValueConstantName = $className . '::' . $defaultValueNode->name;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Reflection/ReflectionType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class ReflectionType
];

/**
* @var $type
* @var string
Copy link
Member

Choose a reason for hiding this comment

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

👍

*/
private $type;

Expand Down
26 changes: 16 additions & 10 deletions src/SourceLocator/Reflection/SourceStubber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,20 @@ public function __invoke(CoreReflectionClass $classReflection) : string
{
$classNode = $this->createClass($classReflection);

$this->addClassModifiers($classNode, $classReflection);
if ($classNode instanceof Class_) {
$this->addClassModifiers($classNode, $classReflection);
Copy link
Member

Choose a reason for hiding this comment

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

Interface_ and Trait_ should be passed here too

Copy link
Contributor Author

@muglug muglug Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How can we have abstract/final interfaces or traits?

Copy link
Member

Choose a reason for hiding this comment

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

No fix needed

}

if ($classNode instanceof Class_ || $classNode instanceof Interface_) {
$this->addExtendsAndImplements($classNode, $classReflection);
}

if ($classNode instanceof Class_ || $classNode instanceof Trait_) {
$this->addProperties($classNode, $classReflection);
$this->addTraitUse($classNode, $classReflection);
}

$this->addDocComment($classNode, $classReflection);
$this->addExtendsAndImplements($classNode, $classReflection);
$this->addTraitUse($classNode, $classReflection);
$this->addProperties($classNode, $classReflection);
$this->addConstants($classNode, $classReflection);
$this->addMethods($classNode, $classReflection);

Expand Down Expand Up @@ -106,10 +115,7 @@ private function addDocComment(BuilderAbstract $node, CoreReflector $reflection)
}
}

/**
* @param Class_|Interface_|Trait_ $classNode
*/
private function addClassModifiers(Declaration $classNode, CoreReflectionClass $classReflection) : void
private function addClassModifiers(Class_ $classNode, CoreReflectionClass $classReflection) : void
Copy link
Member

Choose a reason for hiding this comment

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

Is ClassLike not more appropriate here? As this would no longer accept Interface_ or Trait_ I think, which I believe should be accepted so we can make them abstract (unless I'm mistaken)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Traits/Interfaces can't be abstract though? Or final?

Copy link
Member

Choose a reason for hiding this comment

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

No fix needed

{
if (! $classReflection->isInterface() && $classReflection->isAbstract()) {
// Interface \Iterator is interface and abstract
Expand All @@ -122,7 +128,7 @@ private function addClassModifiers(Declaration $classNode, CoreReflectionClass $
}

/**
* @param Class_|Interface_|Trait_ $classNode
* @param Class_|Interface_ $classNode
*/
private function addExtendsAndImplements(Declaration $classNode, CoreReflectionClass $classReflection) : void
{
Expand All @@ -139,7 +145,7 @@ private function addExtendsAndImplements(Declaration $classNode, CoreReflectionC
}

foreach ($interfaces as $interfaceName) {
if ($classReflection->isInterface()) {
if ($classNode instanceof Interface_) {
$classNode->extend(new FullyQualified($interfaceName));
} else {
$classNode->implement(new FullyQualified($interfaceName));
Expand Down
4 changes: 2 additions & 2 deletions src/SourceLocator/Type/ClosureSourceLocator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ private function getReflectionFunction(Reflector $reflector, IdentifierType $ide
private $startLine;

/**
* @var Node[][]
* @var (Node|null)[][]
*/
private $closureNodes = [];

Expand Down Expand Up @@ -139,7 +139,7 @@ public function leaveNode(Node $node) : void
*/
public function getClosureNodes() : ?array
{
/** @var Node[][] $closureNodesDataOnSameLine */
/** @var (Node|null)[][] $closureNodesDataOnSameLine */
$closureNodesDataOnSameLine = array_values(array_filter($this->closureNodes, function (array $nodes) : bool {
return $nodes[0]->getLine() === $this->startLine;
}));
Expand Down
31 changes: 21 additions & 10 deletions src/TypesFinder/PhpDocumentor/NamespaceNodeToReflectionTypeContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,28 @@ public function __invoke(?Namespace_ $namespace) : Context
private function aliasesToFullyQualifiedNames(Namespace_ $namespace) : array
{
// flatten(flatten(map(stuff)))
return array_merge([], ...array_merge([], ...array_map(function ($use) : array {
/** @var $use Use_|GroupUse */
return array_merge(
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what was changed here, since the alignment was changed. The operation was simply turning array[][] into array, but I don't see the diff nodes affected here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing /** @var $use Use_|GroupUse */ to /** @param Use_|GroupUse $use */

Copy link
Member

Choose a reason for hiding this comment

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

Seems related to https://github.com/Roave/BetterReflection/pull/394/files#r164988430 - type is being inferred over implementation of array_map(). LGTM.

Copy link
Contributor Author

@muglug muglug Feb 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not totally related. Psalm has a MissingDocblockType that triggers for @var $var Type annotations. When I had a look at this, it made sense to not only fix the order but change it to a @param.

Psalm doesn't project types into closures, so it's not able to infer this, but it does check the boundaries – e.g. it sees the error in

class A {}
class B {}

$as = [new A(), new B()];

$classes = array_map(
  function(B $a) : string {
    return "hello";
  },
  $as
);

(I don't have link generation set up, but you can always paste into getpsalm.org to verify)

Copy link
Member

Choose a reason for hiding this comment

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

👍

[],
...array_merge(
[],
...array_map(
/** @param Use_|GroupUse $use */
function ($use) : array {
return array_map(
function (UseUse $useUse) use ($use) : array {
if ($use instanceof GroupUse) {
return [$useUse->alias => $use->prefix->toString() . '\\' . $useUse->name->toString()];
}

return array_map(function (UseUse $useUse) use ($use) : array {
if ($use instanceof GroupUse) {
return [$useUse->alias => $use->prefix->toString() . '\\' . $useUse->name->toString()];
}

return [$useUse->alias => $useUse->name->toString()];
}, $use->uses);
}, $this->classAlikeUses($namespace))));
return [$useUse->alias => $useUse->name->toString()];
},
$use->uses
);
},
$this->classAlikeUses($namespace)
)
)
);
}

/**
Expand Down
25 changes: 22 additions & 3 deletions test/unit/Reflection/Adapter/ReflectionMethodTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
use Roave\BetterReflection\Reflection\Adapter\Exception\NotImplemented;
use Roave\BetterReflection\Reflection\Adapter\ReflectionClass as ReflectionClassAdapter;
use Roave\BetterReflection\Reflection\Adapter\ReflectionMethod as ReflectionMethodAdapter;
use Roave\BetterReflection\Reflection\Adapter\ReflectionParameter as ReflectionParameterAdapter;
use Roave\BetterReflection\Reflection\Adapter\ReflectionType as ReflectionTypeAdapter;
use Roave\BetterReflection\Reflection\Exception\NoObjectProvided;
use Roave\BetterReflection\Reflection\Exception\NotAnObject;
use Roave\BetterReflection\Reflection\Exception\ObjectNotInstanceOfClass;
Expand Down Expand Up @@ -67,7 +69,7 @@ public function methodExpectationProvider() : array
['isUserDefined', null, true, []],
['getClosureThis', NotImplemented::class, null, []],
['getClosureScopeClass', NotImplemented::class, null, []],
['getDocComment', null, '', []],
['getDocComment', null, false, []],
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch here 👍

['getStartLine', null, 123, []],
['getEndLine', null, 123, []],
['getExtension', NotImplemented::class, null, []],
Expand Down Expand Up @@ -121,8 +123,25 @@ public function testAdapterMethods(string $methodName, ?string $expectedExceptio
$this->expectException($expectedException);
}

$adapter = new ReflectionMethodAdapter($reflectionStub);
$adapter->{$methodName}(...$args);
$adapter = new ReflectionMethodAdapter($reflectionStub);
$adapterReturnValue = $adapter->{$methodName}(...$args);

switch ($methodName) {
case 'getParameters':
$this->assertContainsOnly(ReflectionParameterAdapter::class, $adapterReturnValue);
break;

case 'getReturnType':
$this->assertInstanceOf(ReflectionTypeAdapter::class, $adapterReturnValue);
break;

case 'getPrototype':
$this->assertInstanceOf(ReflectionMethodAdapter::class, $adapterReturnValue);
break;

default:
$this->assertEquals($returnValue, $adapterReturnValue);
}
}

public function testExport() : void
Expand Down