Skip to content

Add DateIntervalFormatDynamicReturnTypeExtension #4165

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: 2.1.x
Choose a base branch
from
Open

Add DateIntervalFormatDynamicReturnTypeExtension #4165

wants to merge 6 commits into from
+141 −0

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Jul 26, 2025 •
edited
Loading

Related to phpstan/phpstan#1452

but does not close it unfortunately since the %a is complicated.

staabm
staabm reviewed Jul 26, 2025
View reviewed changes
@VincentLanglet VincentLanglet force-pushed the fix/1452 branch 2 times, most recently from 34d5241 to ffb94a4 Compare July 26, 2025 14:32
@VincentLanglet VincentLanglet requested a review from staabm July 26, 2025 14:52
ondrejmirtes
ondrejmirtes requested changes Sep 7, 2025
View reviewed changes
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Please add regression test for https://phpstan.org/r/66bd52bc-2c1c-4b6e-ac27-57c2a7261982.

I'd like both a test in InvalidBinaryOperationRule, and assertType for %a.

@VincentLanglet VincentLanglet marked this pull request as draft September 10, 2025 10:54
@VincentLanglet
Copy link
Contributor Author

Please add regression test for phpstan.org/r/66bd52bc-2c1c-4b6e-ac27-57c2a7261982.

I'd like both a test in InvalidBinaryOperationRule, and assertType for %a.

This doesn't solve the InvalidBinaryOperationRule issue indeed, I just added a nsrt test to show why.

%a can return either a numeric string or (unknown), and we cannot know the DateTimeInterval is coming from a DateTime::diff call or was just instantiated with something like new DateInterval(...).

So this PR is still an improvement but doesn't solve the issue.

@VincentLanglet VincentLanglet marked this pull request as ready for review September 10, 2025 11:07
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

staabm
staabm reviewed Sep 10, 2025
View reviewed changes
src/Type/Php/DateIntervalFormatDynamicReturnTypeExtension.php
$constantStrings = $arg->getConstantStrings();
if (count($constantStrings) === 0) {
if ($arg->isNonEmptyString()->yes()) {
return new IntersectionType([new StringType(), new AccessoryNonEmptyStringType()]);
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeCombinator::intersect() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We generally use directly IntersectionType for string type + accessories because it doesn't require any extra manipulation while intersect does lot of different things useless in this case

staabm
staabm suggested changes Sep 10, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ondrejmirtes ondrejmirtes Awaiting requested review from ondrejmirtes

1 more reviewer

@staabm staabm staabm requested 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.
4 participants
@VincentLanglet @phpstan-bot @ondrejmirtes @staabm