-
Notifications
You must be signed in to change notification settings - Fork 509
implemented str_contains
FunctionTypeSpecifyingExtension
#1068
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
Conversation
It's not ideal but this feature has to wait because it can lead to these kinds of errors:
|
What do you think about enabling it via bleedingEdge?
didn't work... not sure why yet |
did some debugging and found out, that the
just added a test and a fix for that |
if (strstr($s, ':') === 'hallo') { | ||
assertType('string', $s); // could be non-empty-string | ||
} | ||
assertType('string', $s); | ||
if (strstr($s, ':', true) === 'hallo') { | ||
assertType('string', $s); // could be non-empty-string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimal would be a non-empty-string here.. but the extension as-is cannot deliver it atm.
I have no idea yet, how a type-specifying extension could evaluate a comparision like if (strstr($s, ':', true) === 'hallo') {
.
atm I am only verifying a plain string
type, to make sure the new extension does not scramble the existing type in this scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparison based inference can be implemented like df27a12 but ===
should be preferred over ==
the PR now contains various str-containing functions, no longer a |
The problem here is still this: #1068 (comment) Unfortunately |
6fd372d
to
c0dcd57
Compare
Looks like my idea is going to work :) /cc @herndlm |
Thank you! |
$args[$needleArg]->value, | ||
new String_(''), | ||
), | ||
new FuncCall(new Name('FAUX_FUNCTION'), [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx for getting this over the finishing line <3.
could you elaborate a bit, what this FAUX_FUNCTION thing is about?
is this a kind-of workaround, which should be encapsulated in a helper somehow, so the code gets readable for non-phpstan-hard-core users :-)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's kind of the soft force flag xD but I agree, it is hard to understand and workaround-ish, maybe it should end up in some kind of helper or smth like that hmm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the other hand this is veeery special and the expression is different for each case maybe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing that's problematic for str_contains()
type-specifying extensions and similar is that PHPStan thinks the function is is_string_non_empty()
because it narrows down the type to non-empty-string
. But there are other properties of the narrowed string than that - in this case that it contains another string which can't be expressed by the PHPStan typesystem.
So when you call str_contains($nonEmptyString)
PHPStan thinks it's is_string_non_empty($nonEmptyString)
and that it has to be always true. But in fact it should be is_string_non_empty($nonEmptyString) && string_contains_string(...)
. So that's what I'm simulating here.
I don't think we need a helper here.
I have not quite decided yet if I really like that modified expression thing. Potentially we then have x different ways of working around that. I also found phpstan/phpstan-webmozart-assert#134 (double executing code would not report, but that's maybe ok to live with) |
first str-family-function to infer
non-empty-string
type.Initially had the idea of putting everything into a single StrFamily* extension, but then figured that the semantics between substr, substring, strpos, stripos, str_contains etc are too different, and it would be more readable to put each of them into a separate extension.
therefore this is the first PR. if merged, I will work on similar semantics for the remaining functions
refs phpstan/phpstan#6792