Add exceptional case for DateInterval::format return type inference

[adamturcsan]

Resolves phpstan/phpstan#13693

Difference in days might behave differently when the DateInterval is created from scratch or from a diff.

Extend returned type information for DateInterval::format method

[VincentLanglet]

This is not ok.

I think you're expecting

'(unknown)'|lowercase-string&non-empty-string&numeric-string&uppercase-string

[adamturcsan]

Thanks for the review! I've pushed a change to fix this.

[VincentLanglet]

Thinking about it again, I think the approach is not correct cause you handle %a to add new ConstantStringType('(unknown)') but it could occur with eveyr other format containing %a.

[VincentLanglet]

You don't have to add $diffInDayType in every string you could do

$possibleReturnTypes = [];
if ($formatString === '%a') {
     $possibleReturnTypes[] = new ConstantStringType('(unknown)');
}

[adamturcsan]

If the pattern is not %a without any additional character, it won't only return (unknown) and so it would only be a lowercase-string&non-falsy-string.

[VincentLanglet]

Yes, but it could be considered weird/unnecessary to have the (unknown)|foo precision for %a and not for foo %a

[adamturcsan]

The "weirdness" of it I'm gonna let you decide :) I'd say the more precise the better, but I'm okay if we don't go with this.

[VincentLanglet]

@adamturcsan Wouldn't be enough to just change

if ($value !== '0' && $value !== '') {
     $accessories[] = new AccessoryNonFalsyStringType();
} elseif ($value !== '') {
     $accessories[] = new AccessoryNonEmptyStringType();
}

into

if ($value !== '0' && $value !== '' && $format !== '%a') {
     $accessories[] = new AccessoryNonFalsyStringType();
} elseif ($value !== '') {
     $accessories[] = new AccessoryNonEmptyStringType();
}

Then the dumped type will be lowercase-string&non-empty-string which seems correct.

[adamturcsan]

@adamturcsan Wouldn't be enough to just change

if ($value !== '0' && $value !== '') {
     $accessories[] = new AccessoryNonFalsyStringType();
} elseif ($value !== '') {
     $accessories[] = new AccessoryNonEmptyStringType();
}

into

if ($value !== '0' && $value !== '' && $format !== '%a') {
     $accessories[] = new AccessoryNonFalsyStringType();
} elseif ($value !== '') {
     $accessories[] = new AccessoryNonEmptyStringType();
}

Then the dumped type will be lowercase-string&non-empty-string which seems correct.

[UPDATED with second if]

I'd argue a non-falsy-string to be a better type for (unknown) but not for a number that might contain '0'. Also with your suggestion the dumped type for a %a format would be lowercase-string&non-empty-string&numeric-string&uppercase-string which is still not correct as (unknown) is not numeric. If we'd also add this condition to other ifs, like

if (is_numeric($value))

and

if (strtoupper($value) === $value)

into

if (is_numeric($value) && $format !== '%a')

and

if (strtoupper($value) === $value && $formatString !== '%a')

it becomes correct and results in lowercase-string&non-empty-string. Should I go with this?

[VincentLanglet]

it becomes correct and results in lowercase-string&non-empty-string&uppercase-string. Should I go with this?

Since (unknown) is possible it also means that it's not uppercase-string.

I feel like you should keep the $dateInterval = new DateInterval('P0D'); rather than
(new DateTimeImmutable('@0'))->diff((new DateTimeImmutable('@0')));.

then it should just be

        // The worst case scenario for the non-falsy-string check is that every number is 0.
        // `%a` format will gives `(unknown)` which allows to remove numeric and uppercase
        // accessory but then we'll have to manually check for the non-falsy one.
		$dateInterval = new DateInterval('P0D');

		$possibleReturnTypes = [];
		foreach ($constantStrings as $string) {
			$value = $dateInterval->format($string->getValue());

			$accessories = [];
			if (is_numeric($value)) {
				$accessories[] = new AccessoryNumericStringType();
			}
			if ($value !== '0' && $value !== '' && $format !== '%a') { // The only change
				$accessories[] = new AccessoryNonFalsyStringType();
			} elseif ($value !== '') {
				$accessories[] = new AccessoryNonEmptyStringType();
			}
			if (strtolower($value) === $value) {
				$accessories[] = new AccessoryLowercaseStringType();
			}
			if (strtoupper($value) === $value) {
				$accessories[] = new AccessoryUppercaseStringType();
			}

			if (count($accessories) === 0) {
				return null;
			}

			$possibleReturnTypes[] = new IntersectionType([new StringType(), ...$accessories]);

if (is_numeric($value) && $format !== '%a')

and

if (strtoupper($value) === $value && $formatString !== '%a')

it becomes correct and results in lowercase-string&non-empty-string. Should I go with this?

you'll need str_contains instead of checking exactly %a.
That's why I feel like you can avoid this by using new DateInterval('P0D')

[adamturcsan]

Alright, this is much-much simpler so I really can get behind of not providing "too precise" results and it also still solves my own problem, so I pushed the relevant changes. Thanks so much for your collaboration!