Skip to content
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

implemented str_contains FunctionTypeSpecifyingExtension #1068

Merged
merged 2 commits into from Apr 28, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 13, 2022

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

@ondrejmirtes
Copy link
Member

It's not ideal but this feature has to wait because it can lead to these kinds of errors:

Call to function str_starts_with() with non-empty-string and 'I' will always evaluate to true.

@staabm
Copy link
Contributor Author

staabm commented Mar 13, 2022

What do you think about enabling it via bleedingEdge?

alternatively could type-validation be prevented by passing $overwrite=true to the type specifier

didn't work... not sure why yet

@staabm
Copy link
Contributor Author

staabm commented Mar 15, 2022

did some debugging and found out, that the $override flag in this case atm does not work as I initially thought, because the override flags value is lost when SpecifiedTypes->unionWith(...) is invoked.

return new self($sureTypeUnion, $sureNotTypeUnion);

just added a test and a fix for that

Comment on lines +98 to +108
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
Copy link
Contributor Author

@staabm staabm Mar 19, 2022

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

Copy link
Contributor Author

@staabm staabm Mar 26, 2022

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 ==

@staabm
Copy link
Contributor Author

staabm commented Mar 19, 2022

the PR now contains various str-containing functions, no longer a str_contains only case as the PR title suggests.
(I did not change the PR title, as I know it would lead in some extra work for ondrey)

@ondrejmirtes
Copy link
Member

The problem here is still this: #1068 (comment)

Unfortunately $overwrite=true is not a solution, just a workarodun, and it'd lead to more bugs.

@ondrejmirtes ondrejmirtes changed the base branch from 1.5.x to 1.6.x April 28, 2022 06:53
@ondrejmirtes
Copy link
Member

Looks like my idea is going to work :) /cc @herndlm

@ondrejmirtes ondrejmirtes marked this pull request as ready for review April 28, 2022 06:53
@ondrejmirtes ondrejmirtes merged commit 49b8b26 into phpstan:1.6.x Apr 28, 2022
@ondrejmirtes
Copy link
Member

Thank you!

$args[$needleArg]->value,
new String_(''),
),
new FuncCall(new Name('FAUX_FUNCTION'), [
Copy link
Contributor Author

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 :-)?

Copy link
Contributor

@herndlm herndlm Apr 28, 2022

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

Copy link
Contributor

@herndlm herndlm Apr 28, 2022

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

Copy link
Member

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.

@herndlm
Copy link
Contributor

herndlm commented Apr 28, 2022

Looks like my idea is going to work :) /cc @herndlm

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants