Skip to content

strtr returns non-empty-string#1963

Merged
ondrejmirtes merged 1 commit intophpstan:1.9.xfrom
staabm:non-es-strtr
Nov 4, 2022
Merged

strtr returns non-empty-string#1963
ondrejmirtes merged 1 commit intophpstan:1.9.xfrom
staabm:non-es-strtr

Conversation

@staabm
Copy link
Copy Markdown
Contributor

@staabm staabm commented Nov 4, 2022

No description provided.

staabm
staabm commented Nov 4, 2022
View reviewed changes
tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php Outdated
Copy link
Copy Markdown
Contributor Author

@staabm staabm Nov 4, 2022

Choose a reason for hiding this comment

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

tbh, I don't know why this test got more precise with this change :)

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Nov 4, 2022

Choose a reason for hiding this comment

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

Because preg_replace is impacted by your changes. By using selectFromArgs, conditional return type is resolved.

staabm
staabm commented Nov 4, 2022
View reviewed changes
tests/PHPStan/Analyser/data/strtr.php Outdated
Copy link
Copy Markdown
Contributor Author

@staabm staabm Nov 4, 2022

Choose a reason for hiding this comment

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

results before this PR:
https://phpstan.org/r/8833f01d-4202-4163-bf72-982f9729c3c9

@staabm staabm marked this pull request as ready for review November 4, 2022 08:57
@phpstan-bot
Copy link
Copy Markdown
Collaborator

phpstan-bot commented Nov 4, 2022

This pull request has been marked as ready for review.

staabm
staabm commented Nov 4, 2022
View reviewed changes
src/Type/Php/ReplaceFunctionsDynamicReturnTypeExtension.php Outdated
Copy link
Copy Markdown
Contributor Author

@staabm staabm Nov 4, 2022

Choose a reason for hiding this comment

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

since strtr has multiple signatures, I had to use selectFromArgs() here.

Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes Nov 4, 2022

Choose a reason for hiding this comment

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

Because preg_replace is impacted by your changes. By using selectFromArgs, conditional return type is resolved.

ondrejmirtes
ondrejmirtes requested changes Nov 4, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Looking at the extension, could we replace it with stubs with conditional return types instead?

@staabm
Copy link
Copy Markdown
Contributor Author

staabm commented Nov 4, 2022 •
edited
Loading

could we replace it with stubs with conditional return types instead?

I think we could, but it would be a rather complicated conditional, which needs to cover non-empty-string, non-falsey-string and array types, and even combine those logical across parameters subject and replace.

for strtr we would also need to incorporate the 2 function variants.. I don't feel this would lead to a readable stub-file.

@ondrejmirtes ondrejmirtes merged commit d91411b into phpstan:1.9.x Nov 4, 2022
@ondrejmirtes
Copy link
Copy Markdown
Member

ondrejmirtes commented Nov 4, 2022

Thank you.

@staabm staabm deleted the non-es-strtr branch November 4, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ondrejmirtes ondrejmirtes ondrejmirtes requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@staabm @phpstan-bot @ondrejmirtes