Skip to content

RegexArrayShapeMatcher - Fix shape of single top level alternations #3299

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

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 6, 2024

closes phpstan/phpstan#11462

additionally in more cases we get more precise tagged unions 🥳

@staabm
Copy link
Contributor Author

staabm commented Aug 6, 2024

@mvorisek please double check the correctness of the test expectations


function (string $s): void {
if (preg_match('~a|(\d)|(\s)~', $s, $matches) === 1) {
assertType("array{0: string, 1?: numeric-string}|array{string, '', non-empty-string}", $matches);
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, thank you for fixing this!

@staabm staabm marked this pull request as ready for review August 7, 2024 10:13
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.


function (string $s): void {
if (preg_match('~a|((u)x)|((v)y)~', $s, $matches, PREG_UNMATCHED_AS_NULL) === 1) {
assertType("array{string, 'ux'|null, 'u'|null, 'vy'|null, 'v'|null}", $matches);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really like this tagged unions

$combiType = TypeCombinator::union(
new ConstantArrayType([new ConstantIntegerType(0)], [$this->createSubjectValueType($flags, $matchesAll)], [0], [], true),
$combiType,
);
}

$onlyOptionalTopLevelGroup->clearOverrides();
Copy link
Member

Choose a reason for hiding this comment

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

This smells of too much statefulness and mutability. Is this pattern in more places in Regex? I'd much prefer immutable stateless value objects.

Copy link
Contributor Author

@staabm staabm Aug 8, 2024

Choose a reason for hiding this comment

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

totally agree.

I tried doing it in a immutable way, but my attempts don't work because the objects beeing changed here, are referenced by other objects via parent property and we need the objects referenced by $onlyOptionalTopLevelGroup to change, no matter which object in the $groupList directly or nested references it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this pattern in more places in Regex?

we use this pattern for the 2 tagged union special cases in this method here. nowhere else.

@staabm staabm force-pushed the fix5 branch 2 times, most recently from d307f86 to 55422b2 Compare August 8, 2024 18:48
@ondrejmirtes
Copy link
Member

Conflict here

@ondrejmirtes ondrejmirtes merged commit 0132833 into phpstan:1.11.x Aug 8, 2024
228 of 230 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the fix5 branch August 8, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants