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.

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

Add psalm #324

Closed
vv12131415 opened this issue Mar 22, 2020 · 6 comments
Closed

Add psalm #324

vv12131415 opened this issue Mar 22, 2020 · 6 comments

Comments

@vv12131415
Copy link

Description
since there is phpstan used in this repo. I would like to suggest to use psalm.
It also has baseline like phpstan

@Tobion
Copy link
Member

Tobion commented Mar 23, 2020

What's the point of using psalm and phpstan or psalm over phpstan?

@vv12131415
Copy link
Author

psalm over phpstan
No, it's psalm AND phpstan

It will add more types (type inference) to the code.

@Tobion
Copy link
Member

Tobion commented Mar 26, 2020

@vladyslavstartsev can you give some concrete examples where this would help?
See also phpstan/phpstan#2223

@vv12131415
Copy link
Author

vv12131415 commented Mar 28, 2020

okey, I've finally got some time to explain
let's see what phpstan is telling us about AppendStream

                -
                        message: "#^Property GuzzleHttp\\\\Psr7\\\\AppendStream\\:\\:\\$seekable has no typehint specified\\.$#"
                        count: 1
                        path: src/AppendStream.php

                -
                        message: "#^Property GuzzleHttp\\\\Psr7\\\\AppendStream\\:\\:\\$current has no typehint specified\\.$#"
                        count: 1
                        path: src/AppendStream.php

                -
                        message: "#^Property GuzzleHttp\\\\Psr7\\\\AppendStream\\:\\:\\$pos has no typehint specified\\.$#"
                        count: 1
                        path: src/AppendStream.php

                -
                        message: "#^Method GuzzleHttp\\\\Psr7\\\\AppendStream\\:\\:addStream\\(\\) has no return typehint specified\\.$#"
                        count: 1
                        path: src/AppendStream.php

                -
                        message: "#^Method GuzzleHttp\\\\Psr7\\\\AppendStream\\:\\:detach\\(\\) should return resource\\|null but return statement is missing\\.$#"
                        count: 1
                        path: src/AppendStream.php

                -
                        message: "#^Method GuzzleHttp\\\\Psr7\\\\AppendStream\\:\\:rewind\\(\\) has no return typehint specified\\.$#"
                        count: 1
                        path: src/AppendStream.php

                -
                        message: "#^Method GuzzleHttp\\\\Psr7\\\\AppendStream\\:\\:seek\\(\\) has no return typehint specified\\.$#"
                        count: 1
                        path: src/AppendStream.php

when we run the same analysis in psalm, here is our output


ERROR: MissingPropertyType - src/AppendStream.php:19:13 - Property GuzzleHttp\Psr7\AppendStream::$seekable does not have a declared type - consider bool (see https://psalm.dev/045)
    private $seekable = true;


ERROR: MissingPropertyType - src/AppendStream.php:20:13 - Property GuzzleHttp\Psr7\AppendStream::$current does not have a declared type - consider int (see https://psalm.dev/045)
    private $current = 0;


ERROR: MissingPropertyType - src/AppendStream.php:21:13 - Property GuzzleHttp\Psr7\AppendStream::$pos does not have a declared type - consider int (see https://psalm.dev/045)
    private $pos = 0;


ERROR: MissingReturnType - src/AppendStream.php:55:21 - Method GuzzleHttp\Psr7\AppendStream::addStream does not have a return type, expecting void (see https://psalm.dev/050)
    public function addStream(StreamInterface $stream)


ERROR: MixedInferredReturnType - src/AppendStream.php:110:21 - Could not verify return type 'int' for GuzzleHttp\Psr7\AppendStream::tell (see https://psalm.dev/047)
    public function tell()


ERROR: MixedArrayOffset - src/AppendStream.php:142:14 - Cannot access value on variable $this->streams using mixed offset (see https://psalm.dev/031)
             $this->streams[$this->current]->eof());


ERROR: MissingReturnType - src/AppendStream.php:145:21 - Method GuzzleHttp\Psr7\AppendStream::rewind does not have a return type, expecting void (see https://psalm.dev/050)
    public function rewind()


ERROR: MissingReturnType - src/AppendStream.php:155:21 - Method GuzzleHttp\Psr7\AppendStream::seek does not have a return type, expecting void (see https://psalm.dev/050)
    public function seek($offset, $whence = SEEK_SET)


ERROR: MixedArrayOffset - src/AppendStream.php:199:36 - Cannot access value on variable $this->streams using mixed offset (see https://psalm.dev/031)
            if ($progressToNext || $this->streams[$this->current]->eof()) {


ERROR: MixedOperand - src/AppendStream.php:204:17 - Left operand cannot be mixed (see https://psalm.dev/059)
                $this->current++;


ERROR: MixedArrayOffset - src/AppendStream.php:207:23 - Cannot access value on variable $this->streams using mixed offset (see https://psalm.dev/031)
            $result = $this->streams[$this->current]->read($remaining);


ERROR: MixedOperand - src/AppendStream.php:219:9 - Left operand cannot be mixed (see https://psalm.dev/059)
        $this->pos += strlen($buffer);


ERROR: MixedInferredReturnType - src/AppendStream.php:234:21 - Could not verify return type 'bool' for GuzzleHttp\Psr7\AppendStream::isSeekable (see https://psalm.dev/047)
    public function isSeekable()


if we remove same issues that was found by phpstan. Here are extra issue found


ERROR: MixedInferredReturnType - src/AppendStream.php:110:21 - Could not verify return type 'int' for GuzzleHttp\Psr7\AppendStream::tell (see https://psalm.dev/047)
    public function tell()

ERROR: MixedArrayOffset - src/AppendStream.php:142:14 - Cannot access value on variable $this->streams using mixed offset (see https://psalm.dev/031)
             $this->streams[$this->current]->eof());

ERROR: MixedArrayOffset - src/AppendStream.php:199:36 - Cannot access value on variable $this->streams using mixed offset (see https://psalm.dev/031)
            if ($progressToNext || $this->streams[$this->current]->eof()) {


ERROR: MixedOperand - src/AppendStream.php:204:17 - Left operand cannot be mixed (see https://psalm.dev/059)
                $this->current++;


ERROR: MixedArrayOffset - src/AppendStream.php:207:23 - Cannot access value on variable $this->streams using mixed offset (see https://psalm.dev/031)
            $result = $this->streams[$this->current]->read($remaining);


ERROR: MixedOperand - src/AppendStream.php:219:9 - Left operand cannot be mixed (see https://psalm.dev/059)
        $this->pos += strlen($buffer);


ERROR: MixedInferredReturnType - src/AppendStream.php:234:21 - Could not verify return type 'bool' for GuzzleHttp\Psr7\AppendStream::isSeekable (see https://psalm.dev/047)
    public function isSeekable()


I've used current master (3472035ddb363a8452bc6999eeb92a92985879d7), so the lines may change

So with psalm we will have extra type errors found.


The link you gave. phpstan and psalm can read each other tool specific annotations (@phpstan-param and @psalm-param, also psalm can read PhpStorm annotations see https://psalm.dev/docs/running_psalm/configuration/#allowphpstormgenerics) so this is not an issue anymore.
But there is still a problem in writing it (annotations) 2 times. The 1st one is for SA tool, and the second one is for IDE (PhpStorm upvote for the support https://youtrack.jetbrains.com/issues/WI?q=sort%20by:%20votes%20State:%20-Fixed%20State:%20-Obsolete%20%20State:%20-Verified%20State:%20-Declined%20%20State:%20-Backlog%20State:%20-Workaround%20State:%20-Duplicate%20sort%20by:%20updated%20). BUT most of the times (80%) one time is enough.

@vv12131415
Copy link
Author

@Tobion what's your opinion?

@Tobion
Copy link
Member

Tobion commented May 14, 2020

The psalm errors you listed are already reported by phpstan, just with a different wording.
For example

ERROR: MixedOperand - src/AppendStream.php:219:9 - Left operand cannot be mixed (see >https://psalm.dev/059)
$this->pos += strlen($buffer);

in phpstan is reported that there is no type specified for AppendStream::$pos.

To me there is no point in using phpstan and psalm at the same time. If you find concrete examples where it would help please open a PR.
For the time being it's more important to fix the existing phpstan warnings, which I started in #333. Feel free to also help in this regard. Thanks.

@Tobion Tobion closed this as completed May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants