Fix issues found by static analysis #394

muglug · 2018-01-05T15:29:29Z

This attempts to adhere, as much as possible, to the base Reflection* signatures. In the case of #389, I made the current different return type explicit so that changing will be a clear breaking change.

According to the docs, static ::export methods can return a value, so I allowed that even if your model does not. You may want to add an explicit return null.

Roave\BetterReflection\Reflection\Adapter\ReflectionMethod::getClosure wasn't returning a Closure, now it should.

Ocramius · 2018-01-05T15:31:03Z

src/Reflection/Adapter/ReflectionFunction.php

@@ -26,7 +26,7 @@ public function __construct(BetterReflectionFunction $betterReflectionFunction)
     */
    public static function export($name, $return = null)
    {
-        BetterReflectionFunction::export(...\func_get_args());
+        return BetterReflectionFunction::export(...\func_get_args());


WOW that's bad O_O

Ocramius · 2018-01-05T15:31:23Z

src/Reflection/Adapter/ReflectionClass.php

@@ -180,7 +180,7 @@ public function hasProperty($name)
    /**
     * {@inheritDoc}
     */
-    public function getProperty($name)
+    public function getProperty($name) : ?ReflectionProperty


AFAIK, all adapters didn't really have return types. Wondering if we should change that...

I can put it in the docblock?

PHP allows it...

Ocramius · 2018-01-05T15:31:53Z

src/NodeCompiler/CompileNodeToValue.php

@@ -121,6 +121,10 @@ private function compileConstFetch(Node\Expr\ConstFetch $constNode)
     */
    private function compileClassConstFetch(Node\Expr\ClassConstFetch $node, CompilerContext $context)
    {
+        if ($node->class instanceof Node\Expr || $node->name instanceof Node\Expr\Error) {


This would require also a test to reach this code location. This may as well be an assumption that is hard to code within the types we have available.

$node->class cannot ever be a Node\Expr here, but Node\Expr\ClassConstFetch::$class can be. I suppose the error node may be possible, if you allow errors in PhpParser.

I'll change to a @var docblock

Ocramius · 2018-01-05T15:33:03Z

src/Reflection/ReflectionFunctionAbstract.php

@@ -62,15 +62,15 @@ protected function __construct()
    {
    }

-    public static function export() : void
+    public static function export()


This is indeed supposed to be void

This method inherits from a method that has a non-void return type

http://php.net/manual/en/reflector.export.php

If the parent were user-defined, a void return type would be a fatal error: https://3v4l.org/BXtvR

Ocramius · 2018-01-05T15:33:09Z

src/Reflection/ReflectionFunctionAbstract.php

@@ -34,7 +34,7 @@
    public const CLOSURE_NAME = '{closure}';

    /**
-     * @var NamespaceNode
+     * @var ?NamespaceNode


NamespaceNode|null

Ocramius · 2018-01-05T15:33:47Z

src/Reflection/ReflectionFunctionAbstract.php

@@ -510,7 +510,7 @@ public function setBodyFromClosure(Closure $newBody) : void
     */
    public function setBodyFromString(string $newBody) : void
    {
-        $this->node->stmts = $this->loadStaticParser()->parse('<?php ' . $newBody);
+        $this->node->stmts = $this->loadStaticParser()->parse('<?php ' . $newBody) ?: [];


Hmm... the parser should never yield non-arrays...

In theory it can yield null just based on the return type.

Ocramius · 2018-01-05T15:35:18Z

src/Reflection/ReflectionParameter.php

@@ -184,6 +184,14 @@ private function parseDefaultValueNode() : void
        $defaultValueNode = $this->node->default;

        if ($defaultValueNode instanceof Node\Expr\ClassConstFetch) {
+            if ($defaultValueNode->class instanceof Node\Expr) {


Same scenario as above - this would require a test case

Ocramius · 2018-01-05T15:35:31Z

src/Reflection/ReflectionProperty.php

@@ -67,7 +67,7 @@ private function __construct()
    {
    }

-    public static function export() : void
+    public static function export()


This is indeed void

This method inherits from a method that has a non-void return type

Ocramius · 2018-01-05T15:35:40Z

src/Reflection/ReflectionType.php

@@ -21,7 +21,7 @@ class ReflectionType
    ];

    /**
-     * @var $type
+     * @var string


Ocramius · 2018-01-05T15:35:49Z

src/SourceLocator/Ast/FindReflectionsInTree.php

@@ -87,7 +87,7 @@ public function __construct(
                $this->astConversionStrategy = $astConversionStrategy;
            }

-            public function enterNode(Node $node) : void


These should be kept

This method inherits from a method that has a non-void return type

I've added an @inheritDoc annotation instead

Btw this issue is documented here: phpstan/phpstan#729 (comment)

Yeah, ok - makes sense to drop these at least for now 👍

Will revert this change though, per #394 (comment)

kukulich · 2018-01-05T15:42:29Z

src/SourceLocator/Type/FileIteratorSourceLocator.php

@@ -25,7 +25,7 @@ class FileIteratorSourceLocator implements SourceLocator
    private $aggregateSourceLocator;

    /**
-     * @var \Iterator|\SplFileInfo[]
+     * @var \Iterator<\SplFileInfo>


@muglug Is there an editor that can understand this? I don't think that eg. PHPStorm supports this syntax.

I don't think so? But it's a logical syntax for an iterator that only contains SplFileInfo.

The union type that works in PHPStorm - \Iterator|\SplFileInfo[] - is illogical, since it implies the property can accept arrays.

I agree it's logical but this syntax is not supported in editors and other tools (eg. PHPCS).

https://github.com/phpDocumentor/fig-standards/tree/master/proposed is still only a draft.

Yup, and until people stop using broken docblocks in their code, there'll be less momentum to fix it.

I have a @psalm-var annotation for non-phpdoc-compatible types, I could add it there instead.

Yeah, until this can be fixed "properly", at least we have editor support with the \SplFileInfo[] hint, which is preferable to not figuring out types IMO :/

Syntax change indeed to be reverted here.

Yup will do

asgrim · 2018-01-31T09:03:01Z

src/Reflection/Adapter/ReflectionFunction.php

@@ -27,7 +27,7 @@ public function __construct(BetterReflectionFunction $betterReflectionFunction)
     */
    public static function export($name, $return = null)
    {
-        BetterReflectionFunction::export(...func_get_args());
+        return BetterReflectionFunction::export(...func_get_args());


Hmm, I know there's been a couple of comments on these, but ::export methods really do return nothing:

https://3v4l.org/uLmk8

There's certainly a discrepancy here (phpdocs vs reality), so which do we pick? What the docs/types say, or how it actually behaves? So far we've been obeying the real-world behaviour (which I'd favour, but is of course incompatible with static analysis...)

/cc @Ocramius

Reality would be better overall.

I'm confused. It can return e.g. https://3v4l.org/oolCP

asgrim · 2018-01-31T09:04:11Z

src/Reflection/ReflectionClass.php

@@ -350,7 +350,6 @@ function (ReflectionMethod $method) use ($filter) {
     */
    public function getImmediateMethods(?int $filter = null) : array
    {
-        /** @var \ReflectionMethod[] $methods */


why removing this?

Because it can be inferred easily

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.

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

I'll revert this

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

I like the pic :D

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

asgrim · 2018-01-31T09:05:33Z

src/Reflection/ReflectionFunctionAbstract.php

@@ -515,6 +518,7 @@ public function setBodyFromClosure(Closure $newBody) : void
     */
    public function setBodyFromString(string $newBody) : void
    {
+        /** @var Node[] $this->node->stmts */


I haven't checked, but does PhpStorm (or other IDEs) actually support this syntax? o_O also, can this not be inferred anyway? And finally, why are we documenting the type when it's not actually used anywhere later?

No, this syntax is not supported

I'll remove

also, can this not be inferred anyway

The method can return null here

asgrim · 2018-01-31T09:10:33Z

src/SourceLocator/Reflection/SourceStubber.php

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


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)

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

No fix needed

asgrim · 2018-01-31T09:11:37Z

src/SourceLocator/Reflection/SourceStubber.php

@@ -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);


Interface_ and Trait_ should be passed here too

How can we have abstract/final interfaces or traits?

No fix needed

asgrim · 2018-01-31T09:14:37Z

src/SourceLocator/Type/FileIteratorSourceLocator.php

@@ -25,7 +25,7 @@ class FileIteratorSourceLocator implements SourceLocator
    private $aggregateSourceLocator;

    /**
-     * @var \Iterator|\SplFileInfo[]
+     * @var \Iterator<\SplFileInfo>


Yeah, until this can be fixed "properly", at least we have editor support with the \SplFileInfo[] hint, which is preferable to not figuring out types IMO :/

asgrim · 2018-01-31T09:16:42Z

src/SourceLocator/Ast/FindReflectionsInTree.php

@@ -87,7 +87,7 @@ public function __construct(
                $this->astConversionStrategy = $astConversionStrategy;
            }

-            public function enterNode(Node $node) : void


Yeah, ok - makes sense to drop these at least for now 👍

Ocramius · 2018-01-31T09:33:15Z

src/Reflection/Adapter/ReflectionFunction.php

@@ -27,7 +27,7 @@ public function __construct(BetterReflectionFunction $betterReflectionFunction)
     */
    public static function export($name, $return = null)
    {
-        BetterReflectionFunction::export(...func_get_args());
+        return BetterReflectionFunction::export(...func_get_args());


Reality would be better overall.

Ocramius · 2018-01-31T09:33:35Z

src/Reflection/Adapter/ReflectionMethod.php

@@ -34,7 +34,7 @@ public function __construct(BetterReflectionMethod $betterReflectionMethod)
     */
    public static function export($class, $name, $return = null)
    {
-        BetterReflectionMethod::export(...func_get_args());
+        return BetterReflectionMethod::export(...func_get_args());


Same here: to be reverted

Ocramius · 2018-01-31T09:34:19Z

src/Reflection/Adapter/ReflectionMethod.php

@@ -313,7 +313,7 @@ public function isDestructor()
    public function getClosure($object = null)
    {
        try {
-            $this->betterReflectionMethod->getClosure($object);
+            return $this->betterReflectionMethod->getClosure($object);


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

Yeah, this needs a test addition too

Test added in 74ba592

Ocramius · 2018-01-31T09:34:49Z

src/Reflection/Adapter/ReflectionObject.php

@@ -175,7 +175,9 @@ public function hasProperty($name)
    }

    /**
-     * {@inheritDoc}
+     * @param string $name


We shouldn't change the parent API in the Adapter ns

same as discussion in https://github.com/Roave/BetterReflection/pull/394/files#r164996850

Handled in #391

Ocramius · 2018-01-31T09:35:12Z

src/Reflection/Adapter/ReflectionClass.php

@@ -184,7 +184,9 @@ public function hasProperty($name)
    }

    /**
-     * {@inheritDoc}
+     * @param string $name


We shouldn't change the documented API in the Adapter ns

You already have, though - documented here

Yeah, but that bug is being fixed. This needs revert or we can wait for the other patch to go in before reverting

Ocramius · 2018-01-31T09:39:54Z

src/SourceLocator/Type/AnonymousClassObjectSourceLocator.php

+            /**
+             * {@inheritDoc}
+             */
+            public function enterNode(Node $node)


: void is to be kept - would highlight changes in the parent API.

Ocramius · 2018-01-31T09:40:17Z

src/SourceLocator/Type/ClosureSourceLocator.php

+            /**
+             * {@inheritDoc}
+             */
+            public function enterNode(Node $node)


: void is to be kept - would highlight changes in the parent API.

Ocramius · 2018-01-31T09:40:21Z

src/SourceLocator/Type/ClosureSourceLocator.php

+            /**
+             * {@inheritDoc}
+             */
+            public function leaveNode(Node $node)


: void is to be kept - would highlight changes in the parent API.

Ocramius · 2018-01-31T09:40:47Z

src/SourceLocator/Type/FileIteratorSourceLocator.php

@@ -25,7 +25,7 @@ class FileIteratorSourceLocator implements SourceLocator
    private $aggregateSourceLocator;

    /**
-     * @var \Iterator|\SplFileInfo[]
+     * @var \Iterator<\SplFileInfo>


Syntax change indeed to be reverted here.

Ocramius · 2018-01-31T09:41:48Z

src/TypesFinder/PhpDocumentor/NamespaceNodeToReflectionTypeContext.php

@@ -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(


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.

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

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

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)

muglug · 2018-01-31T14:52:04Z

@Ocramius w/r/t enterNode/leaveNode, do you think a static analysis tool should treat a void return as equal to a null one?

Currently PHPStan doesn’t check return type equality or covariance of docblock return types, allowing you to do whatever you want.

I could add a workaround that allows : void when the upstream return type is only in a docblock (and nullable). That would acknowledge that, as far as PHP is concerned, the function returns null.

Once PHPStan starts using BetterReflection I imagine these issues will pop up there, too.

Ocramius · 2018-01-31T14:53:50Z

@muglug : void is covariant with : null (if we ever had null), so I think that it is correct to allow it.

muglug · 2018-02-01T00:26:18Z

@Ocramius patched here

muglug · 2018-02-02T21:32:50Z

I've removed the unnecessary return statements in export static methods that always threw exceptions anyway.

Ocramius · 2018-02-03T09:09:48Z

src/Reflection/Adapter/ReflectionClass.php

@@ -184,7 +184,9 @@ public function hasProperty($name)
    }

    /**
-     * {@inheritDoc}
+     * @param string $name


Yeah, but that bug is being fixed. This needs revert or we can wait for the other patch to go in before reverting

Ocramius · 2018-02-03T09:10:25Z

src/Reflection/Adapter/ReflectionObject.php

@@ -175,7 +175,9 @@ public function hasProperty($name)
    }

    /**
-     * {@inheritDoc}
+     * @param string $name


same as discussion in https://github.com/Roave/BetterReflection/pull/394/files#r164996850

Ocramius · 2018-02-03T09:11:37Z

src/Reflection/ReflectionClass.php

@@ -350,7 +350,6 @@ function (ReflectionMethod $method) use ($filter) {
     */
    public function getImmediateMethods(?int $filter = null) : array
    {
-        /** @var \ReflectionMethod[] $methods */


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.

Ocramius · 2018-02-03T09:15:25Z

src/TypesFinder/PhpDocumentor/NamespaceNodeToReflectionTypeContext.php

@@ -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(


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

Ocramius · 2018-02-03T09:16:28Z

src/Reflection/Adapter/ReflectionMethod.php

@@ -313,7 +313,7 @@ public function isDestructor()
    public function getClosure($object = null)
    {
        try {
-            $this->betterReflectionMethod->getClosure($object);
+            return $this->betterReflectionMethod->getClosure($object);


Yeah, this needs a test addition too

Ocramius · 2018-02-03T09:17:04Z

src/NodeCompiler/CompileNodeToValue.php

@@ -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 */


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

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.

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.

Let's keep them for now 👍

Ocramius · 2018-02-03T09:18:20Z

src/NodeCompiler/CompileNodeToValue.php

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

+        /** @var string $node->name */


Same question about compatibility: what to do here?

Let's keep this for now

Ocramius · 2018-02-04T12:17:54Z

test/unit/Reflection/Adapter/ReflectionMethodTest.php

@@ -67,7 +69,7 @@ public function methodExpectationProvider() : array
            ['isUserDefined', null, true, []],
            ['getClosureThis', NotImplemented::class, null, []],
            ['getClosureScopeClass', NotImplemented::class, null, []],
-            ['getDocComment', null, '', []],
+            ['getDocComment', null, false, []],


Nice catch here 👍

Ocramius

LGTM 👍

Let's 🚢 this :)

Thanks @muglug for keeping up with our extremely lengthy and annoying review process ^_^

muglug · 2018-02-04T14:36:34Z

@Ocramius totally worth it for the improvements it forced me to make to Psalm. Thanks for taking your time with it!

Ocramius requested changes Jan 5, 2018

View reviewed changes

kukulich reviewed Jan 5, 2018

View reviewed changes

muglug force-pushed the master branch 3 times, most recently from 25556f7 to 9996b89 Compare January 5, 2018 15:56

muglug force-pushed the master branch 2 times, most recently from 627b41c to 497395e Compare January 29, 2018 20:59

asgrim reviewed Jan 31, 2018

View reviewed changes

asgrim assigned Ocramius Jan 31, 2018

asgrim added the enhancement label Jan 31, 2018

asgrim assigned muglug Jan 31, 2018

Ocramius requested changes Jan 31, 2018

View reviewed changes

muglug force-pushed the master branch 2 times, most recently from 3fb0330 to 3d7a27a Compare February 2, 2018 21:21

Ocramius requested changes Feb 3, 2018

View reviewed changes

Matt Brown and others added 8 commits February 3, 2018 09:20

Fix issues found by Psalm

ba1ed0c

Expand nullable type

cc14790

Convert assertions to docblocks

3fac4b9

Remove ternary to empty array

b09c165

Fix coding standard violations

5ed9c17

Revert changes to void return types

b6a4914

Revert generic iterator change

633de24

Revert changes to exports that will always throw exceptions

60ac692

muglug force-pushed the master branch from 3d7a27a to 128ac5e Compare February 3, 2018 14:27

Revert ReflectionClass::getProperty return type change

c33fedf

muglug force-pushed the master branch from fe820df to 1106702 Compare February 3, 2018 15:48

muglug added 2 commits February 3, 2018 11:23

Add asserts to testAdapterMethods

688934c

Add back docblock annotation

b554734

muglug force-pushed the master branch from 1106702 to b554734 Compare February 3, 2018 16:23

Ocramius added this to the 3.0.0 milestone Feb 4, 2018

Ocramius unassigned muglug Feb 4, 2018

Ocramius reviewed Feb 4, 2018

View reviewed changes

Ocramius approved these changes Feb 4, 2018

View reviewed changes

Ocramius merged commit 4cb957e into Roave:master Feb 4, 2018

Ocramius mentioned this pull request Feb 4, 2018

Updated PHPUnit and PHPStan dependencies #400

Merged

Fix issues found by static analysis #394

Fix issues found by static analysis #394

Conversation

muglug commented Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muglug Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muglug Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muglug commented Jan 5, 2018 •

edited

Loading

muglug Jan 5, 2018 •

edited

Loading

muglug Jan 31, 2018 •

edited

Loading