vimeo / psalm

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

ArgumentTypeCoercion passing string attribute to DateTimeZone contructor #11144

Open flaviovs opened 1 month ago

flaviovs commented 1 month ago

https://psalm.dev/r/19a5e603dc

psalm-github-bot[bot] commented 1 month ago

I found these snippets:

https://psalm.dev/r/19a5e603dc ```php tz1); new \DateTimeZone($obj->tz2); ``` ``` Psalm output (using commit 03ee02c): ERROR: ArgumentTypeCoercion - 13:19 - Argument 1 of DateTimeZone::__construct expects non-empty-string, but parent type string provided ERROR: ArgumentTypeCoercion - 14:19 - Argument 1 of DateTimeZone::__construct expects non-empty-string, but parent type string provided ```
adamkoppede commented 4 weeks ago

psalm does not let one pass "" (a value of type string and type empty-string) because it is neither a supported timezone name, offset value nor timezone abbreviation and thus results in an \DateInvalidTimeZoneException being thrown. Unfortunately, the error message shown when not specifying the type of the properties isn't helpful, as it only suggests the string type. But the more concrete type non-empty-string is required, which can be specified using PHPDoc @var.

The string type of the properties cannot be silently upgraded to non-empty-string, because changing the value of the properties to "" (a string value that is also empty-string but not non-empty-string) would also invalidate the call further down. It is unreasonable for a call to be invalid when only a value but not its type annotation is changed.

Upgrading a value from type string to non-empty-string can be done safely by ensuring that it isn't equals to the "" value of type empty-string: https://psalm.dev/r/ef46919ad4

psalm-github-bot[bot] commented 4 weeks ago

I found these snippets:

https://psalm.dev/r/0b1e619b63 ```php tz1); new \DateTimeZone($obj->tz2); ``` ``` Psalm output (using commit 03ee02c): INFO: MissingPropertyType - 12:19 - Property Foo::$tz1 does not have a declared type INFO: MixedArgument - 12:19 - Argument 1 of DateTimeZone::__construct cannot be mixed, expecting string INFO: MissingPropertyType - 13:19 - Property Foo::$tz2 does not have a declared type INFO: MixedArgument - 13:19 - Argument 1 of DateTimeZone::__construct cannot be mixed, expecting string INFO: MissingPropertyType - 5:12 - Property Foo::$tz1 does not have a declared type - consider string INFO: MissingPropertyType - 7:12 - Property Foo::$tz2 does not have a declared type - consider string ```
https://psalm.dev/r/2c64a273ec ```php tz1); new \DateTimeZone($obj->tz2); ``` ``` Psalm output (using commit 03ee02c): No issues! ```
https://psalm.dev/r/ef46919ad4 ```php tz1) !== "" && ($tz2 = $obj->tz2) !== "") { new \DateTimeZone($tz1); new \DateTimeZone($tz2); } ``` ``` Psalm output (using commit 03ee02c): No issues! ```
flaviovs commented 4 weeks ago

First and foremost: conceptually, from a programming language perspective, "empty string" is not a type, but a runtime condition. String is the type.

But I see it.

Problem is: most properly typed libraries know nothing about empty/non-empty string "types" and just use string, which would require suppressing ArgumentTypeCoercion everywhere, which is like throwing the baby out with the bathwater.

Even PHP calls can be problematic: https://psalm.dev/r/4a2419e7f2

(Of course, one could also test for an empty string, but this could be redundant in many cases — as in your and my example — and of course added coded complexity, not to say a waste of CPU cycles.)

Anyways, I appreciate your comments.

psalm-github-bot[bot] commented 4 weeks ago

I found these snippets:

https://psalm.dev/r/4a2419e7f2 ```php = 8.3 -- apparently psalm.dev are not on 8.3 yet. die('Invalid timezone in bar.txt'); } echo $tz->getName(); ``` ``` Psalm output (using commit 03ee02c): ERROR: ArgumentTypeCoercion - 6:26 - Argument 1 of DateTimeZone::__construct expects non-empty-string, but parent type false|string provided ```