Do not treat method_exists() as always true for @method-annotated methods#5664
Do not treat method_exists() as always true for @method-annotated methods#5664phpstan-bot wants to merge 9 commits into
method_exists() as always true for @method-annotated methods#5664Conversation
… methods - In `ImpossibleCheckTypeHelper::findSpecifiedType()`, the `method_exists()` check used `hasMethod()` which includes `@method` PHPDoc annotations. Since `@method` methods are virtual (handled by `__call()`), they don't guarantee `method_exists()` returns true at runtime. - Added `hasNativeMethod()` check for object types to only report always-true when the method is actually declared in PHP code, not just via `@method`. - Applied the same fix for the generic class-string code path. - Verified that `property_exists()` already handles `@property` correctly via `PropertyExistsTypeSpecifyingExtension::isNative()` check.
VincentLanglet
left a comment
There was a problem hiding this comment.
The rule should still report a method_exists already specified to true.
// @method should not make method_exists always true
if (\method_exists($hell, 'isTrue')) {
if (\method_exists($hell, 'isTrue')) { // This one should be reported
}
}
Also if we finally check native type ; is it still needed to check hasMethod($methodType->getValue())->yes() before ?
When inside an `if (method_exists(...))` block, a repeated method_exists() check for the same object and method is redundant and should be reported as always-true, even for @method-annotated methods. To detect this, the MethodExistsTypeSpecifyingExtension now also tracks the method_exists() FuncCall expression as ConstantBooleanType(true) in the scope (in addition to narrowing the argument type). This allows ImpossibleCheckTypeHelper to detect the nested case via Scope::hasExpressionType() before any type-specific logic runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Here's a summary of the changes made to address the review: Review point 1: Nested Two coordinated changes:
Review point 2: "Is Yes, it's still needed. The |
VincentLanglet
left a comment
There was a problem hiding this comment.
Does the issue exists for property_exists ; if not, why ? Can't the method_exist behave the same way ?
…ropertyExistsTypeSpecifyingExtension Move the @method annotation check into MethodExistsTypeSpecifyingExtension to match how PropertyExistsTypeSpecifyingExtension handles @Property annotations. When the method only exists via @method PHPDoc, return empty SpecifiedTypes so the type is not narrowed. Simplify ImpossibleCheckTypeHelper accordingly and remove the nested funcCallSpec approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
property_exists is not statisfying cause it reports
if (property_exists($task, $a)) {
if (property_exists($task, $a)) {
}
}
but not
if (property_exists($task, '$a')) {
if (property_exists($task, '$a')) {
}
}
VincentLanglet
left a comment
There was a problem hiding this comment.
property_exists is not statisfying cause it reports
if (property_exists($task, $a)) {
if (property_exists($task, $a)) {
}
}
but not
if (property_exists($task, '$a')) {
if (property_exists($task, '$a')) {
}
}
…ethod/@Property When method_exists() or property_exists() checks a method/property that only exists via @method/@Property PHPDoc annotation, the type specifying extensions now store the FuncCall expression as ConstantBooleanType(true) in scope instead of returning empty SpecifiedTypes. This enables ImpossibleCheckTypeHelper to detect nested (redundant) checks as always-true while still not reporting the outer check. An early check in findSpecifiedType() detects when a FuncCall expression already has a stored constant boolean type in scope and returns it directly, bypassing the special-case logic that would otherwise return null for @method-only methods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's a summary of the changes: What the reviewer asked for: Nested Three coordinated changes:
|
| if ($propertyReflection !== null) { | ||
| if (!$propertyReflection->isNative()) { | ||
| return new SpecifiedTypes([], []); | ||
| return $this->typeSpecifier->create( |
There was a problem hiding this comment.
This is the same code than line 61, this can be refactored ; maybe a private method ?
There was a problem hiding this comment.
Pushed. Here's a summary of the changes:
Two refactoring changes requested by the reviewer:
-
MethodExistsTypeSpecifyingExtension: Extracted duplicated FuncCall-as-true spec creation into acreateFuncCallSpec()private method. This removes the eager$funcCallSpecvariable and defers creation until actually needed. All three call sites (non-constant method name, constant class-string with@method, object type with@method) now use the private method. Also fixed a remaining$funcCallSpecreference in thegetClassStringObjectType()path that was added by a concurrent remote change. -
PropertyExistsTypeSpecifyingExtension: Same extraction — duplicated code at lines 61 and 88 now callscreateFuncCallSpec().
Regarding the previous review question about ImpossibleCheckTypeHelper line 257: The hasNativeMethod() check in the generic class-string path is still needed. I verified by removing it — the test for method_exists($classString, 'test') where $classString: class-string<Foo> fails because the general type specifier path can't determine always-true for class-string types (since HasMethodType::isSuperTypeOf(class-string<Foo>) returns maybe, not yes). The fast path is the only way to correctly report native methods as always-true for generic class-strings.
…Extension The hasNativeMethod() check in ImpossibleCheckTypeHelper for generic class-strings (class-string<Foo>) is still needed because the helper runs its own hasMethod() check before the TypeSpecifier flow. Without the guard, method_exists() with @method-only methods on generic class-strings would be incorrectly reported as always-true. This commit also adds the same hasNativeMethod() guard to MethodExistsTypeSpecifyingExtension for generic class-strings, matching the existing handling for constant strings and object types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pec creation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… funcCallSpec creation The early $scope->hasExpressionType() check in findSpecifiedType() was unnecessary — nested method_exists()/property_exists() for @method/@Property annotations are already caught by IfConstantConditionRule through the funcCallSpec stored in scope. This matches how nested checks with variable method names are already handled. Extract funcCallSpec creation into private createFuncCallSpec() methods in both MethodExistsTypeSpecifyingExtension and PropertyExistsTypeSpecifyingExtension to avoid duplication and eager creation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| // Nested method_exists with @method should report the inner as always-true | ||
| if (\method_exists($hell, 'isTrue')) { | ||
| if (\method_exists($hell, 'isTrue')) { // should be reported |
There was a problem hiding this comment.
This should be reported ; fix it.
There was a problem hiding this comment.
Pushed. The fix adds an early scope check in the method_exists fast path of ImpossibleCheckTypeHelper: before the hasNativeMethod guard, it checks whether the scope already has the method_exists() FuncCall stored as a constant boolean (from an outer if (method_exists(...)) block). This correctly reports the inner nested call as always-true, even for @method-only methods. The check uses isTrue()/isFalse() to examine the actual stored type value, addressing the previous review concern about blindly returning true.
1da62a7 to
7838b26
Compare
Summary
When an interface or class has a
@methodPHPDoc annotation, PHPStan incorrectly reportsmethod_exists()checks as always evaluating to true. Since@method-annotated methods are virtual (typically handled by__call()) and don't exist as actual PHP methods,method_exists()may return false at runtime.Changes
src/Rules/Comparison/ImpossibleCheckTypeHelper.php:hasMethod()returns yes, additionally verify viaClassReflection::hasNativeMethod()that the method is actually declared in PHP code, not just via@methodannotation. Returnnull(indeterminate) instead oftruewhen only a@methodannotation exists.hasNativeMethod()guard before reporting always-true.tests/PHPStan/Rules/Comparison/data/bug-6211.phpcovering:@methodon an interface, checked via object instance and class string@methodon a class directly@propertywith__get()correctly handled (already working viaPropertyExistsTypeSpecifyingExtension)Root cause
ImpossibleCheckTypeHelper::findSpecifiedType()used$objectType->hasMethod()to determine ifmethod_exists()would always return true.hasMethod()delegates toClassReflection::hasMethod(), which includes methods fromAnnotationsMethodsClassReflectionExtension(i.e.,@methodPHPDoc annotations). Since these annotations describe methods that may be handled by__call()but don't create real PHP methods,method_exists()should not be considered always-true for them.The fix uses
ClassReflection::hasNativeMethod()to verify the method is an actual PHP method before reporting the check as always-true.Analogous cases probed
property_exists()with@property: Already correctly handled —PropertyExistsTypeSpecifyingExtensionchecks$propertyReflection->isNative()at line 87 and returns empty specifiers for non-native properties.method_exists()with class-string: Fixed in the same PR — the generic class-string code path inImpossibleCheckTypeHelperhad the same issue.Test
Regression test
testBug6211inImpossibleCheckTypeFunctionCallRuleTestwith test data covering@methodon interfaces and classes, both via object instances and class strings, alongside native methods that should still be reported as always-true.Fixes phpstan/phpstan#6211