RegexArrayShapeMatcher - Support preg_quote()'d patterns #3233
Conversation
|
You've opened the pull request against the latest branch 1.12.x. If your code is relevant on 1.11.x and you want it to be released sooner, please rebase your pull request and change its target to 1.11.x. |
|
This pull request has been marked as ready for review. |
|
//cc @Seldaek |
| private function getPatternType(Expr $patternExpr, Scope $scope): Type | ||
| { | ||
| if ($patternExpr instanceof Expr\BinaryOp\Concat) { | ||
| return $this->resolvePatternConcat($patternExpr, $scope); |
There was a problem hiding this comment.
I don't get it, what's happening here? Shouldn't this be done instead by improving MutatingScope::resolveType? By introducing dynamic return type extension for preg_quote or something?
There was a problem hiding this comment.
I'll try to explain as I feel this is my fault :D
I think you can't reasonably treat preg quote's return value as a constant string (empty or "x") in any other context than here when trying to get a constant pattern string so that you have a regex to parse.
As such it feels appropriate to me to hack this in here but maybe there are better ways 🤷🏻♂️
There was a problem hiding this comment.
Its pretty unlikely that we know the args to preg_quote() at static analysis time in a real world use-case.
Please also look at the added test-cases. These wouldn't work with a return type extension, as the extension couldn't produce a constant-string for these cases.
So while adding a preg_quote return type extension could make sense in the future, it would not help with some of the examples at hand. The ArrayShapeMatcher just can ignore the preg_quote() parts and still analyse the shape, as long as all other parts of a concatenation are constant.
This assumption only works for the ArrayShapeMatcher therefore its not implemented for the common case in the Scope
There was a problem hiding this comment.
Alright, in that case the code needs some comments. I did not understand at all why you are returning an empty string for a preg_quote call.
| && $concat->right->name instanceof Name | ||
| && $concat->right->name->toLowerString() === 'preg_quote' | ||
| ) { | ||
| $right = new ConstantStringType(''); |
There was a problem hiding this comment.
Another thing.. If you do smth like https://3v4l.org/1J7nFF you see the first call triggers a warning but this code will assume it's fine even tho it is a detectable mistake. If you're evaluating the pattern anyway, you could check the first character and try to run preq_quote on it. If it doesn't come out escaped then any preg quote call that is inline in the concatenated string forming the pattern should have that character passed in as second argument to ensure the regex cannot be broken.
I hope I'm making sense @staabm .. Could be another PR tho for sure as it's smth for a new rule I guess. It sucks a bit that we can't warn from here tho because that would warn also for free in composer/pcre and others integrating with it.
There was a problem hiding this comment.
I agree this sounds useful. please create a new issue with a reproducer
|
Thank you. |
closes phpstan/phpstan#11332 - implements the idea described in the issue
since the
preg_quotedetection requires access to theExprof the pattern, I had to add a new public api method, which should be used instead of the previousmatchTypemethod.I marked the "old" method as
@deprecatedand suggest the newly added method instead.