Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 28, 2024

@staabm
Copy link
Contributor Author

staabm commented Jul 28, 2024

//cc @Seldaek

@@ -44,7 +44,7 @@ public function resolve(Expr $expr): Type
&& $expr->name instanceof Name
&& $expr->name->toLowerString() === 'preg_quote'
) {
return new ConstantStringType('');
return new ConstantStringType('.*');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of dummy-replace preg_quote($x) with '' we use .* as a placeholder, so we don't produce invalid patterns

Choose a reason for hiding this comment

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

Can you please explain the constant string meaning? AFAICT such assumption here can produce misleading assumptions/phpstan errors.

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 replace a preg_quote with some constant value so we can finally resolve a string concatenation to a pattern at analysis time.

The preg_quoted value cannot return something which creates capturing groups or syntax errors, therefore we simplify it to improve coverage

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd probably help to have some more comments explaining these edge cases / hacks to future archeologists 😅 but PR looks good thanks

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

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit 5c54586 into phpstan:1.11.x Jul 28, 2024
449 of 456 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

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.

5 participants