-
Notifications
You must be signed in to change notification settings - Fork 650
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
Refined explode()
types
#9016
Refined explode()
types
#9016
Commits on Dec 28, 2022
-
Fixes vimeo#5039 This patch removes the need for a custom function return type provider for `explode()`, and instead replaces all that with a single stub for the `explode()` function, which provides types for some of the most common `$limit` input values. With this change, the `$delimiter` is enforced to be a `non-empty-string`, which will lead to downstream consumers having to adjust some code accordingly, but that shouldn't affect the most common scenario of exploding a string based with a constant `literal-string` delimiter, which most PHP devs tend to do. This change didn't come with an accompanying test, since that would be a bit wasteful, but it was verified locally with following script: ```php <?php $possible0 = explode(',', 'hello, world', -100); $possible1 = explode(',', 'hello, world', -1); $possible2 = explode(',', 'hello, world', 0); $possible3 = explode(',', 'hello, world', 1); $possible4 = explode(',', 'hello, world', 2); $possible5 = explode(',', 'hello, world', 3); $possible6 = explode(',', 'hello, world', 4); try { $impossible1 = explode('', '', -1); } catch (Throwable $impossible1) {} $traced = [$possible0, $possible1, $possible2, $possible3, $possible4, $possible5, $possible6, $impossible1]; /** @psalm-trace $traced */ var_dump($traced); return $traced; ``` Running psalm locally, this produces: ``` psalm on feature/vimeo#5039-more-refined-types-for-explode-core-function [?] via 🐘 v8.1.13 via ❄️ impure (nix-shell) ❯ ./psalm --no-cache explode.php Target PHP version: 7.4 (inferred from composer.json) Extensions enabled: dom, simplexml (unsupported extensions: ctype, json, libxml, mbstring, tokenizer) Scanning files... Analyzing files... ░ To whom it may concern: Psalm cannot detect unused classes, methods and properties when analyzing individual files and folders. Run on the full project to enable complete unused code detection. ERROR: InvalidArgument - explode.php:11:28 - Argument 1 of explode expects non-empty-string, but '' provided (see https://psalm.dev/004) $impossible1 = explode('', '', -1); ERROR: PossiblyUndefinedGlobalVariable - explode.php:14:96 - Possibly undefined global variable $impossible1 defined in try block (see https://psalm.dev/126) $traced = [$possible0, $possible1, $possible2, $possible3, $possible4, $possible5, $possible6, $impossible1]; ERROR: ForbiddenCode - explode.php:18:1 - Unsafe var_dump (see https://psalm.dev/002) /** @psalm-trace $traced */ var_dump($traced); ERROR: Trace - explode.php:18:1 - $traced: list{0: array<never, never>, 1: non-empty-list<string>, 2: list{string}, 3: list{string}, 4: array{0: string, 1?: string}, 5: array{0: string, 1?: string, 2?: string}, 6: non-empty-list<string>, 7?: Throwable|non-empty-list<string>} (see https://psalm.dev/224) /** @psalm-trace $traced */ var_dump($traced); ------------------------------ 4 errors found ------------------------------ Checks took 6.31 seconds and used 265.386MB of memory Psalm was unable to infer types in the codebase ``` The actual runtime behavior on PHP 8.x: https://3v4l.org/0NKlW ``` array(8) { [0]=> array(0) { } [1]=> array(1) { [0]=> string(5) "hello" } [2]=> array(1) { [0]=> string(12) "hello, world" } [3]=> array(1) { [0]=> string(12) "hello, world" } [4]=> array(2) { [0]=> string(5) "hello" [1]=> string(6) " world" } [5]=> array(2) { [0]=> string(5) "hello" [1]=> string(6) " world" } [6]=> array(2) { [0]=> string(5) "hello" [1]=> string(6) " world" } [7]=> object(ValueError)vimeo#1 (7) { ["message":protected]=> string(51) "explode(): Argument vimeo#1 ($separator) cannot be empty" ["string":"Error":private]=> string(0) "" ["code":protected]=> int(0) ["file":protected]=> string(9) "/in/0NKlW" ["line":protected]=> int(11) ["trace":"Error":private]=> array(1) { [0]=> array(4) { ["file"]=> string(9) "/in/0NKlW" ["line"]=> int(11) ["function"]=> string(7) "explode" ["args"]=> array(3) { [0]=> string(0) "" [1]=> string(0) "" [2]=> int(-1) } } } ["previous":"Error":private]=> NULL } } ``` On PHP 7: ``` Warning: explode(): Empty delimiter in /in/0NKlW on line 11 array(8) { [0]=> array(0) { } [1]=> array(1) { [0]=> string(5) "hello" } [2]=> array(1) { [0]=> string(12) "hello, world" } [3]=> array(1) { [0]=> string(12) "hello, world" } [4]=> array(2) { [0]=> string(5) "hello" [1]=> string(6) " world" } [5]=> array(2) { [0]=> string(5) "hello" [1]=> string(6) " world" } [6]=> array(2) { [0]=> string(5) "hello" [1]=> string(6) " world" } [7]=> bool(false) } ```
Configuration menu - View commit details
-
Copy full SHA for 04999b1 - Browse repository at this point
Copy the full SHA 04999b1View commit details -
Ensure that
explode($d, lowercase-string)
produces `list<lowercase-……string>` types This specific distinction seems to be very important for Psalm, as `explode()` and `lowercase-string` are used aggressively across the codebase. Also, this change expands the baseline by a few entries, since some of the code locations instide Psalm itself have un-checked list destructuring operations, as well as array access calls on potentially undefined array keys produced by `explode()`, which were previously just `list<string>`, and are now `array{0: string, 1?: string}`, which is a bit more precise.
Configuration menu - View commit details
-
Copy full SHA for bfded43 - Browse repository at this point
Copy the full SHA bfded43View commit details -
Adjusted existing tests to the new signature of
explode()
Note how the signature became `array{0: string, 1?: string, 2?: string, array<int, string>}`, which may be a side-effect of unions between a defined hashmap structure with array keys, and `list<string>`.
Configuration menu - View commit details
-
Copy full SHA for 6341d7f - Browse repository at this point
Copy the full SHA 6341d7fView commit details -
Using
list{0: string, 1?: string}
syntax for more precise array key…… types Thanks to @orklah's feedback, the `explode()` return type is now much more precise too. Ref: vimeo#9016 (comment)
Configuration menu - View commit details
-
Copy full SHA for c0c0116 - Browse repository at this point
Copy the full SHA c0c0116View commit details -
Adjusted
assertDifferentTypeOfArray
test to avoid intersecting inco……mpatible string arrays Getting one interesting failure caused by the `lowercase-string` refinement done before: ``` There was 1 error: 1) Psalm\Tests\AssertAnnotationTest::testValidCode with data set "assertDifferentTypeOfArray" ('<?php\n /*...ts[1];') Psalm\Exception\CodeException: DocblockTypeContradiction - src/somefile.php:21:21 - Cannot resolve types for $parts - docblock-defined type list{0: lowercase-string, 1?: lowercase-string} does not contain list{string, string} ``` Happens on this bit: ```php 'assertDifferentTypeOfArray' => [ 'code' => '<?php /** * @psalm-assert list{string, string} $value * @param mixed $value */ function isStringTuple($value): void { if (!is_array($value) || !isset($value[0]) || !isset($value[1]) || !is_string($value[0]) || !is_string($value[1]) ) { throw new \Exception("bad"); } } $s = ""; $parts = explode(":", $s, 2); isStringTuple($parts); echo $parts[0]; echo $parts[1];', ], ``` If I change this to: ``` @psalm-assert list{lowercase-string, lowercase-string} $value ``` Then everything works: I'm wondering if this error has to do with array intersections, and whether suppressing it suffices. For now, changing the input string, so it is not a `lowercase-string`, is sufficient.
Configuration menu - View commit details
-
Copy full SHA for 45f743f - Browse repository at this point
Copy the full SHA 45f743fView commit details