vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.58k stars 664 forks source link

Incorrect property type detection after using psalm-suppress PossiblyInvalidPropertyAssignmentValue. #8704

Open mj4444ru opened 2 years ago

mj4444ru commented 2 years ago

https://psalm.dev/r/151f1f9ca8

The type for $this->response is hardcoded at the php language level. However, psalm thinks that there may be something else, which is completely wrong.

Maybe at the same time someone will tell you how to tell psalm that string|null value is placed in $this->response?

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/151f1f9ca8 ```php response = curl_exec($ch) ?: null; $this->test2($this->response); } private function test2(?string $response): void { echo $response ?? ''; } } ``` ``` Psalm output (using commit 56bc854): ERROR: InvalidScalarArgument - 15:22 - Argument 1 of Test::test2 expects null|string, but non-falsy-string|null|true provided ```
weirdan commented 2 years ago

Do you mean to override Psalm inference? Use @var for that: https://psalm.dev/r/c80db53215

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/c80db53215 ```php response = curl_exec($ch) ?: null; $this->test2($this->response); } private function test2(?string $response): void { echo $response ?? ''; } } ``` ``` Psalm output (using commit 56bc854): No issues! ```
orklah commented 2 years ago

I think the issue is with CURLOPT_RETURNTRANSFER here.

curl_exec is special because it either return true or false in default mode but if CURLOPT_RETURNTRANSFER is set through curl_setopt, the return type becoms string|false.

Psalm is handling that by documenting the return is bool|string. When you had a ?: null, it becomes true|non-empty-string|null

There's a small discussion about that here: https://github.com/vimeo/psalm/issues/3570#issuecomment-766675960 but on a closed issue

EDIT: oh, I get what you meant, the property being typed, it could help narrow the type. I'm not sure what I feel about that, It would probably mean making a difference between typed and non-typed parameters...

weirdan commented 2 years ago

EDIT: oh, I get what you meant, the property being typed, it could help narrow the type. I'm not sure what I feel about that, It would probably mean making a difference between typed and non-typed parameters...

Typed and untyped properties behave differently in PHP, so it would be useful to have that distinction.

orklah commented 2 years ago

Ok, why not. It may not be simple though. This will have to support the intersection between the type of the property and the type of the assignment (both could be templates and the property may be located in a parent or in a trait)

mj4444ru commented 2 years ago

If a property of an object is strongly typed, you shouldn't override its type no matter what you assign to it. It seems to me that it is necessary to distinguish strongly typed properties from the rest and not perform type substitution when assigning for strongly typed properties.

orklah commented 2 years ago

If a property of an object is strongly typed, you shouldn't override its type no matter what you assign to it.

We're in a closed scope, if you assign a bool in a bool|string property, people will expect Psalm to know the property can't contain a string. You have to perform an intersection between the type of the property and the type assigned into it, not just take the type of the property itself