Skip to content
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

Added better stubs for DateTimeImmutable, highlighting how the constructor is **NOT** immutable #8350

Merged
merged 22 commits into from
Aug 7, 2022

Conversation

Ocramius
Copy link
Contributor

DateTimeImmutable is almost immutable: DateTimeImmutable::__construct() is in fact not a pure
method, since new DateTimeImmutable('now') produces a different value at each instantiation (by design).

This change makes sure that DateTimeImmutable loses its @psalm-immutable class-level marker,
preventing downstream misuse of the constructor inside otherwise referentially transparent code.

Note: only pure methods are stubbed here: all other methods declared by DateTimeImmutable (parent interface)
are NOT present here, and are either inferred from runtime reflection, or CallMap*.php definitions.

Methods are sorted in the order defined by reflection on PHP 8.1.8, at the time of writing this ( https://3v4l.org/3TGg8 ).

Following simplistic snippet was used to infer the current signature:

<?php

$c = new \ReflectionClass(\DateTimeImmutable::class);

$methods = array_map(function ($m) {
    return $m->getName()
        . '(' . implode(',', array_map(function ($p) {
            return $p->getType()
                . ' $' . $p->getName()
                . ($p->isOptional() ? ' = ' . var_export($p->getDefaultValue(), true) : '');
        }, $m->getParameters())) . ')' . ($m->getReturnType() ? (': ' . $m->getReturnType()) : '');
}, $c->getMethods());

$properties = array_map(function ($m) {
    return $m->getName();
}, $c->getProperties());

var_dump($methods, $properties);

…tructor is **NOT** immutable

`DateTimeImmutable` is **almost** immutable: `DateTimeImmutable::__construct()` is in fact not a pure
method, since `new DateTimeImmutable('now')` produces a different value at each instantiation (by design).

This change makes sure that `DateTimeImmutable` loses its `@psalm-immutable` class-level marker,
preventing downstream misuse of the constructor inside otherwise referentially transparent code.

Note: only pure methods are stubbed here: all other methods declared by `DateTimeImmutable` (parent interface)
are NOT present here, and are either inferred from runtime reflection, or `CallMap*.php` definitions.

Methods are sorted in the order defined by reflection on PHP 8.1.8, at the time of writing this ( https://3v4l.org/3TGg8 ).

Following simplistic snippet was used to infer the current signature:

```php
<?php

$c = new \ReflectionClass(\DateTimeImmutable::class);

$methods = array_map(function ($m) {
    return $m->getName()
        . '(' . implode(',', array_map(function ($p) {
            return $p->getType()
                . ' $' . $p->getName()
                . ($p->isOptional() ? ' = ' . var_export($p->getDefaultValue(), true) : '');
        }, $m->getParameters())) . ')' . ($m->getReturnType() ? (': ' . $m->getReturnType()) : '');
}, $c->getMethods());

$properties = array_map(function ($m) {
    return $m->getName();
}, $c->getProperties());

var_dump($methods, $properties);
```
@Ocramius
Copy link
Contributor Author

Note: this sparked from initial conversations on slack: I don't really know if I should change the existing CallMap files, after adjusting this stub.

/cc @guidobonuzzi, since you may be interested.

…mutable`

`DateTimeImmutable` is not really immutable, therefore this marker was wrong upfront
tests/MethodCallTest.php Outdated Show resolved Hide resolved
@AndrolGenhald AndrolGenhald added the release:fix The PR will be included in 'Fixes' section of the release notes label Jul 31, 2022
…to use

Ref: https://github.com/vimeo/psalm/pull/8350/files/c205d652d1e9afd9510db59e72c3fd0a4a093b3d#r934032422

The idea is that `@psalm-pure` disallows `$this` usage in child classes,
which is not wanted, while `@psalm-mutation-free` allows it.

By using `@psalm-mutation-free`, we don't completely destroy inheritance
use-cases based on internal (immutable) state.
… by stub

Note: some conditional return type magic was required here.

See: vimeo#8350 (comment)
…vered by stub

Note: also verified that a `DateTimeImmutable#getTimezone()` always returns
a default timezone (initialized internally), and therefore restricted the
type a bit.
…overed by stub

This also simplifies the return type from `int|false` to always `int`,
since a timestamp can always be produced.

Ref: https://github.com/php/php-src/blob/eff9aed1592f59cddb12d36a55dec0ccc3bbbfd6/ext/date/php_date.stub.php#L496-L500
… by stub

Also expanded the return type from `static` to `static|false`, since the
operation can fail (with a warning too), such as in following example:

https://3v4l.org/Xrjlc

```php
<?php

var_dump(
    (new DateTimeImmutable())
        ->modify('potato')
);
```

Produces

```
Warning: DateTimeImmutable::modify(): Failed to parse time string (potato) at position 0 (p): The timezone could not be found in the database in /in/Xrjlc on line 6
bool(false)
```

Ref: https://github.com/php/php-src/blob/534127d3b22b193ffb9511c4447584f0d2bd4e24/ext/date/php_date.stub.php#L508-L509
…vered by stub

Also simplified the return type from `static|false` to `static`, since
the method throws at all times, on failure.

On PHP 7.x, it could only fail if an invalid type was passed in, which is
not really valid anyway, from a type perspective.

Ref (PHP 8.2.x): https://github.com/php/php-src/blob/534127d3b22b193ffb9511c4447584f0d2bd4e24/ext/date/php_date.c#L3291-L3307
Ref (PHP 8.2.x): https://github.com/php/php-src/blob/534127d3b22b193ffb9511c4447584f0d2bd4e24/ext/date/php_date.stub.php#L517-L518
Ref (PHP 7.0.33): https://github.com/php/php-src/blob/bf574c2b67a1f786e36cf679f41b758b973a82c4/ext/date/php_date.c#L3363-L3379
…d by stub

Also simplified the return type from `static|false` to `static`, since
the method throws at all times, on failure.

On PHP 7.x, it could only fail if an invalid type was passed in, which is
not really valid anyway, from a type perspective.

Ref (PHP 8.1.x): https://github.com/php/php-src/blob/32d55f74229e7913db0d59ef874a401744479b6a/ext/date/php_date.c#L3212-L3228
Ref (PHP 7.0.33): https://github.com/php/php-src/blob/bf574c2b67a1f786e36cf679f41b758b973a82c4/ext/date/php_date.c#L3447-L3463
…d by stub

Also simplified the return type from `static|false` to `static`, since
the method throws at all times, on failure.

On PHP 7.x, it could only fail if an invalid type was passed in, which is
not really valid anyway, from a type perspective.

Ref (PHP 8.1.x): https://github.com/php/php-src/blob/32d55f74229e7913db0d59ef874a401744479b6a/ext/date/php_date.c#L3258-L3274
Ref (PHP 7.0.33): https://github.com/php/php-src/blob/bf574c2b67a1f786e36cf679f41b758b973a82c4/ext/date/php_date.c#L3496-L3512
…ered by stub

Also simplified the return type from `static|false` to `static`, since
the method throws at all times, on failure.

On PHP 7.x, it could only fail if an invalid type was passed in, which is
not really valid anyway, from a type perspective.

Ref (PHP 8.1.x): https://github.com/php/php-src/blob/32d55f74229e7913db0d59ef874a401744479b6a/ext/date/php_date.c#L3308-L3324
Ref (PHP 7.0.33): https://github.com/php/php-src/blob/bf574c2b67a1f786e36cf679f41b758b973a82c4/ext/date/php_date.c#L3549-L3565
…overed by stub

Also simplified the return type from `static|false` to `static`, since
the method throws at all times, on failure.

On PHP 7.x, it could only fail if an invalid type was passed in, which is
not really valid anyway, from a type perspective.

Ref (PHP 8.1.x): https://github.com/php/php-src/blob/32d55f74229e7913db0d59ef874a401744479b6a/ext/date/php_date.c#L3353-L3369
Ref (PHP 7.0.33): https://github.com/php/php-src/blob/bf574c2b67a1f786e36cf679f41b758b973a82c4/ext/date/php_date.c#L3596-L3612
Also:

 * a non-empty format string will always lead to `non-empty-string`
 * it seems that you can throw **everything** at `DateTimeInterface#format()`, even null bytes,
   and it will always produce a `string`
@Ocramius
Copy link
Contributor Author

Ocramius commented Aug 5, 2022

Interesting finding: DateTimeImmutable#format() always produces a string, no matter what you pass to it.

I took the occasion to refine that non-empty-string input will lead to non-empty-string output :-)

Ref: 68ffae0

While there is value in declaring `DateTimeImmutable::createFromInterface()` as mutation-free in
a stub, this method was introduced in PHP 8.0, so we cannot really declare it in a stub.

For now, we drop it, as the value of its stub declaration is much lower than the problems it
introduces through its conditional existence.
@Ocramius
Copy link
Contributor Author

Ocramius commented Aug 5, 2022

@AndrolGenhald @orklah this is now ready, IMO: thanks for all the support! 👍

@orklah
Copy link
Collaborator

orklah commented Aug 7, 2022

Could you please keep the content of callmaps please? I'm not entirely sure we can drop it, and even if we could, there's no harm to have a comprehensive list if needed some day

@Ocramius
Copy link
Contributor Author

Ocramius commented Aug 7, 2022

I think it's duplicated at this point: shouldn't be kept, if what you said about priority is true.

What value does the added info provide, considering that each removal contains documentation/references?

@orklah orklah merged commit 57fcc39 into vimeo:4.x Aug 7, 2022
@orklah
Copy link
Collaborator

orklah commented Aug 7, 2022

Fine then :p Thanks!

@Ocramius Ocramius deleted the fix/datetime-constructor-is-not-immutable branch August 7, 2022 20:58
@Ocramius
Copy link
Contributor Author

Ocramius commented Aug 7, 2022

Thanks @orklah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants