Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add exceptional case for DateInterval::format return type inference #4442

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

Open
adamturcsan wants to merge 1 commit into phpstan:2.1.x
base: 2.1.x
Choose a base branch
Loading
from adamturcsan:bugfix/13693-dateinterval-format-fix

Conversation

@adamturcsan
Copy link

@adamturcsan adamturcsan commented Oct 15, 2025

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

// Could be lowercase-string&non-falsy-string&numeric-string&uppercase-string
assertType('lowercase-string&non-falsy-string', $dateInterval->format('%a'));
assertType(
"'(unknown)'&lowercase-string&non-empty-string&numeric-string&uppercase-string",
Copy link
Contributor

@VincentLanglet VincentLanglet Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ok.

I think you're expecting

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

adamturcsan reacted with thumbs up emoji
Copy link
Author

@adamturcsan adamturcsan Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@adamturcsan adamturcsan force-pushed the bugfix/13693-dateinterval-format-fix branch from 8740766 to 75008af Compare October 17, 2025 14:52
Copy link
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

]);
$possibleReturnTypes[] = $diffInDaysType === null
? $intersectionType
: new UnionType([$diffInDaysType, $intersectionType]);
Copy link
Contributor

@VincentLanglet VincentLanglet Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Author

@adamturcsan adamturcsan Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@VincentLanglet VincentLanglet Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@adamturcsan adamturcsan Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@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.

Copy link
Author

adamturcsan commented Oct 17, 2025
edited
Loading

@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?

Copy link
Contributor

VincentLanglet commented Oct 17, 2025
edited
Loading

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')

Difference in days might behave differently when the DateInterval is created from scratch or from a diff.
Filter returned type information for DateInterval::format. It can only be non-falsy for sure
if it's not '%a'.
@adamturcsan adamturcsan force-pushed the bugfix/13693-dateinterval-format-fix branch from 75008af to 4b35722 Compare October 18, 2025 07:28
Copy link
Author

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!

VincentLanglet reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@VincentLanglet VincentLanglet VincentLanglet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrectly inferred type - DateInterval::format('%a') can be falsy

AltStyle によって変換されたページ (->オリジナル) /