\DatetimeImmutable::modify signature change (vs \Datetime::modify)

[VincentLanglet]

Currently https://psalm.dev/r/d94dac07ab is not reflecting the 4.27 release.
I'm running psalm on php 7.4 version

(new \DateTimeImmutable('2016-08-31'))->modify('+23 hours 59 minutes 59 seconds');

is reported as DateTimeImmutable&static|false
and I have an error reported when calling getTimestamp on it.
This is a new error related to the changes in #8350 (@Ocramius, @orklah, @AndrolGenhald)

What is weird is the fact that

(new \DateTime('2016-08-31'))->modify('+23 hours 59 minutes 59 seconds');

is reported as DateTime&static|false
but not error is reported when calling getTimestamp on it.

If psalm is using a benevolent union, it seems like the recent DatetimeImmutable stub lost it.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/d94dac07ab

<?php

function foo(\DateTimeImmutable $d): int {
    /** @psalm-trace $a */
    $a = $d->modify('+23 hours 59 minutes 59 seconds');
    
    return $a->getTimestamp();
}

function bar(\DateTime $d): int {
    /** @psalm-trace $a */
    $a = $d->modify('+23 hours 59 minutes 59 seconds');
    
    return $a->getTimestamp();
}

Psalm output (using commit afe85fa):

INFO: Trace - 5:5 - $a: DateTimeImmutable&static

INFO: Trace - 12:5 - $a: DateTime&static|false

[Ocramius]

This is correct: ->modify() can return false.

From PHP 8.2.0-RC1: https://github.com/php/php-src/blob/6a0e11445d0eb7edf3fae196032ffa399137082c/ext/date/php_date.c#L3077-L3091

[VincentLanglet]

I agree, it's correct, but \DateTime::modify can return false too and doesn't work the same way for psalm.

(new \DateTimeImmutable('2016-08-31'))->modify('+23 hours 59 minutes 59 seconds')->getTimestamp(); // PossiblyFalseError
(new \DateTime('2016-08-31'))->modify('+23 hours 59 minutes 59 seconds')->getTimestamp(); // No Error

We should look for consistency.

I was wondering if some Benevolent were used for \DateTime::modify which would have explained that no error were found

Also

(new \DateTimeImmutable('2016-08-31'))->modify('+23 hours 59 minutes 59 seconds')

is not false.

So it might be worth to implement something similar to
https://github.com/phpstan/phpstan-src/blob/f1fd385433480f87bdda1d7d8ff6887e25f003ba/src/Type/Php/DateTimeModifyReturnTypeExtension.php
for psalm.

[Ocramius]

TBH, I didn't touch DateTime because it should no longer be used anyway: https://twitter.com/derickr/status/1551611856007069696

The false scenario depends on the input of a DateTimeImmutable::__construct(), so you cannot really determine upfront if ->modify() will fail, unless you have the instantiation in the same chunk of code.

[VincentLanglet]

The false scenario depends on the input of a DateTimeImmutable::__construct(), so you cannot really determine upfront if ->modify() will fail, unless you have the instantiation in the same chunk of code.

Interesting, do you have an example ?

Phpstan only checks the modify input https://github.com/phpstan/phpstan-src/blob/f1fd385433480f87bdda1d7d8ff6887e25f003ba/conf/config.neon#L1264-L1269
Might be worth to report it.

Currently I have the issue with ->modify('+23 hours 59 minutes 59 seconds') that

• If I don't check for false, psalm is complaining
• If I check for false, PHPStan is complaining because this can't happen

[Ocramius]

https://twitter.com/Ocramius/status/1555495120446922752

[Ocramius]

Btw, all commits from #8350 are detailed on why these choices were made: please read up on them.

[orklah]

Not sure if anything is actionnable here then? It seems that the false case can happen (admittedly in very rare cases) that can't be statically inferred.

So either PHPStan should start adding this in their definitions or there will be a difference between tools on that method.

The only correct thing we could do would be to align DateTime if anyone need to

[VincentLanglet]

Not sure if anything is actionnable here then? It seems that the false case can happen (admittedly in very rare cases) that can't be statically inferred.

So either PHPStan should start adding this in their definitions or there will be a difference between tools on that method.

Phpstan consider that $dateTimeImmutable->modify() can return false.
but there is a dynamicReturnType which consider that $dateTimeImmutable->modify('+23 hours 59 minutes 59 seconds') is never false.

Even with the tweet or the commit messages, I still don't understand when

$dateTimeImmutable->modify('+23 hours 59 minutes 59 seconds')

can return false.

If it's never false, psalm should add some dynamic return type resolver. (For datetime and datetimeimmutable)
If it can be false, it should be reported to phpstan.

Do you have a php example @orklah ?

[Ocramius]

You'd probably need to look in the PHP test suite for one: what has been done here is just reflecting the code paths that I already linked above.

[VincentLanglet]

You'd probably need to look in the PHP test suite for one: what has been done here is just reflecting the code paths that I already linked above.

I'm really sorry but I did look and still don't understand.

You linked https://github.com/php/php-src/blob/6a0e11445d0eb7edf3fae196032ffa399137082c/ext/date/php_date.c#L3077-L3091, and I looked at tests, and the only one I found about it was https://github.com/php/php-src/blob/b46632e9e478f1f8205f8c62b2e0738f36005427/ext/date/tests/DateTimeImmutable_modify_invalid_format.phpt and also looked your commit message 7cd3d49

I never said this method was not returning false. But I thought we could statically guess the return type, like phpstan does.
When a known-constant-string value is passed to modify, phpstan is doing

$value = (new \DateTime())->modify($theConstantString)

to know if the return value will be false or a DateTime/DateTimeImmutable. Psalm could do the same.

Even re-reading multiple times the discussion I still don't understand how the modify return type is related to the value passed when instantiating the DateTime/DateTimeImmutable. You said

The false scenario depends on the input of a DateTimeImmutable::__construct()
But never found an example which is what I'm looking for, a value $foo and $bar where

(new \DateTimeImmutable($foo))->modify($bar);

is false but

(new \DateTimeImmutable())->modify($bar);

is a \DateTimeImmutable

You also linked https://github.com/php/php-src/blob/534127d3b22b193ffb9511c4447584f0d2bd4e24/ext/date/php_date.c#L3157-L3160 but I don't understand how the code in the sub method is related to the modify one and how it give me an example.

[Ocramius]

and how it give me an example.

It doesn't: I asked the author of ext-date about it on twitter, on purpose, because I was confused too :P

The code path returning false exists

(new \DateTimeImmutable())->modify($bar);

The problem is when you don't have BOTH the DateTimeImmutable::__construct() and the DateTimeImmutable#modify() as constant expressions: according to what I got told, false can be produced depending on __construct()'s input.

That said, if you are this passionate about this issue, my suggestion is that you raise an RFC for PHP 8.3, to remove the edge case completely, and remove the last few false from DateTimeImmutable. Over here, this is just documenting the current state of the situation: I can't improve it 🤷

If you can demonstrate that the date/time functions NEVER return false, then adjust this stub in upstream: https://github.com/php/php-src/blob/eff9aed1592f59cddb12d36a55dec0ccc3bbbfd6/ext/date/php_date.stub.php#L183

Once that's done, it should be possible to do it here too, if that is verifiable.

[VincentLanglet]

It doesn't: I asked the author of ext-date about it on twitter, on purpose, because I was confused too :P

So we need to wait an example from him then.

The problem is when you don't have BOTH the DateTimeImmutable::__construct() and the DateTimeImmutable#modify() as constant expressions: according to what I got told, false can be produced depending on __construct()'s input.

But that's what you got told without an example.
Looking at the implementation of modify method I dont see how the possible false return value is related to the value passed to the constructor.
And looking at the return type from phpstan. @ondrejmirtes is considering that's not the case.

So we can consider implementing the same here, unless someone has a counter example.

That said, if you are this passionate about this issue, my suggestion is that you raise an RFC for PHP 8.3, to remove the edge case completely, and remove the last few false from DateTimeImmutable. Over here, this is just documenting the current state of the situation: I can't improve it 🤷

If you can demonstrate that the date/time functions NEVER return false, then adjust this stub in upstream: https://github.com/php/php-src/blob/eff9aed1592f59cddb12d36a55dec0ccc3bbbfd6/ext/date/php_date.stub.php#L183

Once that's done, it should be possible to do it here too, if that is verifiable.

Again, that's another topic. The question is not weither the false return type exists or not (or should exist).
It's weither with some inputs we can be sure it's never false.

But I feel like I'm loosing your time since we dont succeed to understand each other.

[AndrolGenhald]

@Ocramius I see a return 0 here and here, but as far as I can tell neither of those are dependent of the DateTimeImmutable object itself (as long as it's properly initialized by the constructor, which I'd say is a given). Are you sure ->modify()'s false return is dependent on the constructor, or is it only ->sub() (and can ->sub() even return false? It looks to me like it just gives a warning and returns a clone in case of failure)?

[Ocramius]

Ooooh, interesting, so in theory it should not be possible to ever land in a false state?

Perhaps I took the advice too eagerly?

Happy to adjust accordingly here, if anyone else can check @AndrolGenhald and confirm (I'm too sleep deprived, but I skimmed the linked code, and it may really be that false never really occurs!)

[AndrolGenhald]

I'm a bit confused as to how we got to talking about ->sub(), it's already documented as not returning false. ->modify() definitely can return false, but I thought the issue you had with it was that it might return false dependent on the object as well as the argument:

The false scenario depends on the input of a DateTimeImmutable::__construct(), so you cannot really determine upfront if ->modify() will fail, unless you have the instantiation in the same chunk of code.

But as far as I can tell, the false return from ->modify() depends only on the $modifier passed to it (though I would be happy to have someone confirm this, technically it looks like it might also depend on DATE_TIMEZONEDB and php_date_parse_tzfile_wrapper, whatever those are).

[VincentLanglet]

But as far as I can tell, the false return from ->modify() depends only on the $modifier passed to it

If this is confirmed I opened #8462

[AndrolGenhald]

@Ocramius The ->sub() issue is separate from this, this issue is only for ->modify(), but I do believe ->sub() was incorrect in your PR, here's an example for that special case you found: https://3v4l.org/TIWRB

It indeed gives a warning and returns a clone, so there's no false return.

Also, go get some sleep 😛