-
Couldn't load subscription status.
- Fork 544
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
Add exceptional case for DateInterval::format return type inference #4442
Conversation
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
8740766 to
75008af
Compare
There was a problem hiding this 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.
There was a problem hiding this comment.
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)');
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@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 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?
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'.
75008af to
4b35722
Compare
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!
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