psalm icon indicating copy to clipboard operation
psalm copied to clipboard

`TDependentListKey` in `MinMaxReturnTypeProvider`

Open nosnickid opened this issue 2 years ago • 3 comments

https://psalm.dev/r/b1d48c0c66

MinMaxReturnTypeProvider, when checking indexes found in a fgetcsv() result, throws "Unexpected type"

https://github.com/vimeo/psalm/blob/36d5a2a83c1d7b3e2aca1cfe9e9dcc916619e962/src/Psalm/Internal/Provider/ReturnTypeProvider/MinMaxReturnTypeProvider.php#L75

This test, after checking for some specific Int cases, should fallback back to instanceof TInt, rather than get_class() === TInt::class, eg:

} elseif ($atomic_type instanceof TInt) {

is desired.

Happy to submit a PR if this change makes sense.

nosnickid avatar Sep 26 '22 11:09 nosnickid

I found these snippets:

https://psalm.dev/r/b1d48c0c66
<?php

$file = fopen("", "r");
$header = fgetcsv($file);

$hA = $hB = null;
foreach($header as $i => $name) {
    if ($name === 'A') $hA = $i;
    if ($name === 'B') $hA = $i;
}

# This check seems needed
if ($hA === null || $hb === null) throw new \Exception();

# $hA and $hB are TDependentListKey
$minCols = max($hA, $hB);

echo $minCols;
Psalm encountered an internal error:

/vendor/vimeo/psalm/src/Psalm/Internal/Provider/ReturnTypeProvider/MinMaxReturnTypeProvider.php: Unexpected type

psalm-github-bot[bot] avatar Sep 26 '22 11:09 psalm-github-bot[bot]

Sounds good to me, the worst case scenario seems to be that we don't have an int range as restricted as it could be, which isn't a huge deal (especially since current behavior is worse).

I would also add a TDependentListKey check to make it int<0, max> instead of just int.

AndrolGenhald avatar Sep 26 '22 14:09 AndrolGenhald

Note that TDependentListKey has a min at 0 and a max at null (can't be negative) contrary to int

EDIT: oops, I didn't saw the response above when replying :p

orklah avatar Sep 26 '22 14:09 orklah