Specfiy root expression from type specifying extensions #1516
Conversation
|
Oh no, a typo in the title. I blame the wine I had before.. |
herndlm
force-pushed
the
specify-root-expr
branch
5 times, most recently
from
8ca6cb5
to
42ebd1b
Compare
herndlm
force-pushed
the
specify-root-expr
branch
from
42ebd1b
to
0d827f8
Compare
| ), | ||
| new FuncCall(new Name('FAUX_FUNCTION_' . $functionReflection->getName()), $args), | ||
| ) | ||
| : new FuncCall(new Name('FAUX_FUNCTION_' . $functionReflection->getName()), $args); |
There was a problem hiding this comment.
This is a bit weird but needed to avoid false-positives for functions not evaluating to a boolean like strpos.
Maybe it's an indicator that we cannot evaluate root expressions in general and it should be done by the extensions specifying them themselves hmm
There was a problem hiding this comment.
Kept thinking about this. I think this cannot be there. And to get rid of it (and the error without it, see https://github.com/phpstan/phpstan-src/runs/7373184847?check_suite_focus=true) I'd need to check if the real expression (the function/method call) returns a boolean. For other functions that don't, like strpos the root expression should not be further specified. But to do that I'd need to check the return type via reflection I guess and this feels like too much here.
I'm not sure if this PR is the right approach. Maybe root expression specification should only be done where the FAUX_FUNCTION is created (-> the extension).
|
I think a generic approach like this is not the right one, see #1516 (comment) for details. |
This is a follow-up of the introduction of the root expression feature #1254
And it improves the
StrContainingTypeSpecifyingExtension, the only extension where we currently make use of aFAUX_FUNCTION#1068 to specify additional infos we cannot model with the type system (in case ofstr_containsthat e.g. "foo" is inside some string, which we specify a non-empty-string).UPDATE: this was a bit weirder than I thought at first to adapt because the extension supports functions that evaluate to a boolean and also others that don't and cannot be supported by what this PR adds.
The idea behind this PR is that if extensions specify a root expression, that resolves to a
BooleanType(which theImpossibleCheckTypeHelperbasically ignores now to avoid false positives), we can specify that asConstantBooleanType(true). This includes the previously mentionedFAUX_FUNCTIONthat is used to make the TypeSpecifier aware of the additional unknown info. What this effectively does is, whenever the exact same expression, with the exact sameFAUX_FUNCTIONis resolved again, it resolves totrueand produces an impossible type error.That means that we can now detect cases where calls with the exact same "unknown information" were made multiple times.
This also helps me simplify the logic I want to add to the webmozart-assert extension to specify various
non-empty-stringassertions without missing those impossible types :) In case this generic adaption does not make much sense, I guess I'd only specify those types in the webmozart-assert extension which is also fine.