DateInterval::$days false return isn't ignored

[kkmuffme]

https://psalm.dev/r/083ee5e953

But I guess it should be ignored https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Codebase/Reflection.php#L157

I guess it's bc of https://github.com/vimeo/psalm/blob/master/dictionaries/ManualPropertyMap.php#L20 ? (and/or https://github.com/vimeo/psalm/blob/master/dictionaries/PropertyMap.php#L69) where the property map has precedence over things set in code? (which would be bad) @orklah

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/083ee5e953

<?php

$date_time = new DateTime( gmdate( 'Y-m-d' ) );
$date_interval = $date_time->diff( new DateTime( '2023-03-03' ) );
/** @psalm-trace $date_interval */;

$x = $date_interval->days;
/** @psalm-trace $x */;

Psalm output (using commit 8fe1f15):

INFO: Trace - 5:35 - $date_interval: DateInterval

INFO: Trace - 8:23 - $x: false|int

INFO: UnusedVariable - 7:1 - $x is never referenced or the value is not used

[kkmuffme]

Yes, removing it in both files fixes the issue, is it ok to change this? @orklah Not sure what role Reflection class plays here.

[ygottschalk]

One of those files is autogenerated from the other. I think changes must be in the Manual file. There was a comment on the top of the autogenerated file...

[kkmuffme]

Yeah I saw that and ran it, but it's the same thing as just changing this little thing in both manually it seems.

I don't have time to get into how/why the Reflection.php stuff is ignored in this case, I just removed it for now and put a comment. If there's someone who's more familiar with that part of psalm or has some time to check it, would be appreciated.

[orklah]

Reflection is there to get type from PHP core data. So in theory, it should pick up the correct type from what's in PHP but that property is weird enough that we have to make a PropertyMap to correct it's type so it's normal that the map is overriding Reflection.

However, this https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Codebase/Reflection.php#L155 should be put in the PropertyMap parsing too (so, here:

psalm/src/Psalm/Internal/Codebase/PropertyMap.php

Line 32 in dd0d830

self::$property_map = $property_map;

) so that it behave as an ignorable false

[tscni]

Wouldn't that break cases where DateInterval::$days is actually false? (e.g. https://psalm.dev/r/f309122a6a might erroneously start working?)
I don't actually understand why ignore_falsable_issues would be set on it without verifying that it comes from a DateTimeInterface::diff() call. Unless it does and I'm missing something :P
(I suppose I just don't understand ignore_falsable_issues in general)

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/f309122a6a

<?php

function getDays(): int {
    return (new DateInterval('P1D'))->days;
}

Psalm output (using commit 8fe1f15):

ERROR: FalsableReturnStatement - 4:12 - The declared return type 'int' for getDays does not allow false, but the function returns 'false|int'

ERROR: InvalidFalsableReturnType - 3:21 - The declared return type 'int' for getDays does not allow false, but 'false|int' contains false

[kkmuffme]

@orklah thanks for pointing me there. This is actually the wrong place to put that, but I fixed it now at the correct place and removed the initial change.

It's fixed correctly now and the PR can be merged (and this issue closed)

---

@tscni this is a convenience thing to avoid having to create lots of boilerplate code for most users, see https://psalm.dev/docs/running_psalm/configuration/#ignoreinternalfunctionfalsereturn if you don't want to ignore false