add psalm types #531

orklah · 2020-01-14T21:17:43Z

This is a draft PR with the changes I made for #525 to see the rest of the CI and to discuss about some changes

The increase in type coverage is disappointing because a lot of mixed types still comes from outside.

Still, the changes prepare the way for when
nikic/PHP-Parser#647
and
vimeo/psalm#2612

will be merged.

orklah · 2020-01-14T21:19:58Z

src/Reflection/ReflectionProperty.php

@@ -448,6 +450,8 @@ private function assertClassExist(string $className) : void
     * @throws NoObjectProvided
     * @throws NotAnObject
     * @throws ObjectNotInstanceOfClass
+     * 
+     * @psalm-assert object $object


I think this is a good addition, but it adds an error on psalm because it complains that when this method is called, the variable can only be an object and this is redundant.

I think we should suppress the error and keep the assert.

I think the assertion could be refined further to @psalm-assert T, where T is the type for this ReflectionProperty's owner, but that's for another time/discussion.

Indeed, psalm assertions are annoying when put in test scope, because they disallow obvious/dumb scenarios :D

orklah · 2020-01-14T21:20:46Z

src/Reflection/StringCast/ReflectionClassConstantStringCast.php

@@ -16,13 +19,14 @@ final class ReflectionClassConstantStringCast
    public static function toString(ReflectionClassConstant $constantReflection) : string
    {
        $value = $constantReflection->getValue();
+        assert($value === null || is_scalar($value));


A constant can technically return an array. This makes sure that it doesn't happen

orklah · 2020-01-14T21:22:23Z

src/Reflection/ReflectionClass.php

@@ -874,7 +874,7 @@ public function isInterface() : bool
     * Get the traits used, if any are defined. If this class does not have any
     * defined traits, this will return an empty array.
     *
-     * @return ReflectionClass[]
+     * @return list<ReflectionClass>


This is discussed here: #530 It raises an error with Psalm that I think is legitimate and I don't know how to handle.

orklah · 2020-01-14T21:24:35Z

src/Reflection/ReflectionClass.php

@@ -519,7 +519,7 @@ static function (ReflectionClassConstant $constant) : string {
     * Get an associative array of the defined constants in this class,
     * with keys as constant names and values as {@see ReflectionClassConstant} objects.
     *
-     * @return ReflectionClassConstant[] indexed by name
+     * @return array<string, ReflectionClassConstant> indexed by name


This raise a new error on Psalm too because their internal stubs expects this to be an array<int, ReflectionClassConstant>. Not sure who's right on this

I think the stub may need updating here

https://3v4l.org/t05Dv I took this example directly from php.net. The index of array is numerical.
I'll add the case to #530 as it is roughly the same issue

Yeh, the question is whether it is a list or not: I think it is a list in core too

Yeh, the question is whether it is a list or not: I think it is a list in core too

I think you got me wrong. The library returns array<string, ReflectionClassConstant> right now but native Object return array<int, ReflectionClassConstant> (and more probably a list<ReflectionClassConstant> but this is not the issue).

I'll silence those errors in psalm to allow the merge, but I think those are legitimate issues.

Oh, careful! We have adapters and then there is this stuff: the adapters are supposed to be compliant with internal definitions, but these classes don't need to be.

Oh, careful! We have adapters and then there is this stuff: the adapters are supposed to be compliant with internal definitions, but these classes don't need to be.

oh, I get it. But Psalm sees that this class implements Reflection and warn about the type of the interface being different than declared

Ocramius

Overall excellent work, but we need (unavoidable, sorry :-( ) to use @psalm--prefixed annotations for any types that aren't understood by the wider ecosystem, sorry :S

See for example in ProxyManager: https://github.com/Ocramius/ProxyManager/blob/20cd371e97e73d04711f21caf3079d783f9ab936/src/ProxyManager/Proxy/GhostObjectInterface.php#L12-L23

src/NodeCompiler/CompileNodeToValue.php

src/Reflection/ReflectionClass.php

src/SourceLocator/Ast/FindReflectionsInTree.php

src/SourceLocator/Ast/Locator.php

orklah · 2020-01-14T21:39:15Z

Overall excellent work, but we need (unavoidable, sorry :-( ) to use @psalm--prefixed annotations for any types that aren't understood by the wider ecosystem, sorry :S

Will do 👍

Can you look at my own review? I highlighted some important changes

Thanks!

Ocramius · 2020-01-14T23:13:58Z

Read through review comments.

Note that the list type was added to psalm recently

orklah · 2020-01-15T17:01:55Z

Note that the list type was added to psalm recently

Yeah but PHPStorm won't read it, so I added @psalm- where possible while keeping @return

I just removed the draft tag. I suppressed or fixed the remaining psalm errors

Ocramius

LGTM 👍

Let's ship this as-is for now, it is a whole lot of added detail that is indeed immediate added value for the lib 🚢

Ocramius · 2020-01-15T17:17:11Z

Note that the list type was added to psalm recently

Yeah but PHPStorm won't read it, so I added @psalm- where possible while keeping @return

I just removed the draft tag. I suppressed or fixed the remaining psalm errors

This was to note that the stubs may not yet use list, since it is a recent addition ;-)

Ocramius · 2020-01-15T17:32:10Z

Thanks @orklah!

orklah · 2020-01-15T17:57:08Z

@Ocramius Thanks for merging :)

I must have broke Psalm on CI though. I added a windows path on xml file. It worked fine locally but it breaks the CI. Can you fix it or should I create a PR?

Ocramius · 2020-01-15T18:46:14Z

Please do send a separate patch when you have time: I'm not actively working on the package these weeks, besides following contributions

add psalm types

0cc04e0

orklah commented Jan 14, 2020

View reviewed changes

Ocramius requested changes Jan 14, 2020

View reviewed changes

duplicate annotations when using psalm specific notation + fix CS

5bc45be

Fix, silence or suppress remaining errors

70f2fd5

orklah marked this pull request as ready for review January 15, 2020 17:01

Merge branch 'master' into types6

184a244

Ocramius approved these changes Jan 15, 2020

View reviewed changes

Ocramius added this to the 4.0.0 milestone Jan 15, 2020

Ocramius added enhancement reflection compatibility labels Jan 15, 2020

Ocramius self-assigned this Jan 15, 2020

Ocramius merged commit e276075 into Roave:master Jan 15, 2020

orklah deleted the types6 branch January 15, 2020 17:32

This was referenced Jan 15, 2020

fix windows path to linux + phpcbf #532

Merged

Fix issues detected by psalm on 2.7 doctrine/orm#7989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add psalm types #531

add psalm types #531

orklah commented Jan 14, 2020

orklah Jan 14, 2020

Ocramius Jan 15, 2020 •

edited

Loading

orklah Jan 14, 2020

orklah Jan 14, 2020

orklah Jan 14, 2020 •

edited

Loading

Ocramius Jan 14, 2020

orklah Jan 15, 2020

Ocramius Jan 15, 2020

orklah Jan 15, 2020

Ocramius Jan 15, 2020

orklah Jan 15, 2020

Ocramius left a comment

orklah commented Jan 14, 2020 •

edited

Loading

Ocramius commented Jan 14, 2020

orklah commented Jan 15, 2020

Ocramius left a comment

Ocramius commented Jan 15, 2020

Ocramius commented Jan 15, 2020

orklah commented Jan 15, 2020

Ocramius commented Jan 15, 2020

add psalm types #531

add psalm types #531

Conversation

orklah commented Jan 14, 2020

Choose a reason for hiding this comment

Ocramius Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orklah Jan 14, 2020 • 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

Ocramius left a comment

Choose a reason for hiding this comment

orklah commented Jan 14, 2020 • edited Loading

Ocramius commented Jan 14, 2020

orklah commented Jan 15, 2020

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Jan 15, 2020

Ocramius commented Jan 15, 2020

orklah commented Jan 15, 2020

Ocramius commented Jan 15, 2020

Ocramius Jan 15, 2020 •

edited

Loading

orklah Jan 14, 2020 •

edited

Loading

orklah commented Jan 14, 2020 •

edited

Loading