Do not let method_exists() check fall through to general logic when object type contains template types#5605
Conversation
… object type contains template types - When the specific `method_exists` handling in `ImpossibleCheckTypeHelper` cannot determine the result (no concrete class names, no GenericClassStringType), it falls through to the general type specifier logic - The general logic uses `HasMethodType::isSuperTypeOf()` which calls `hasMethod()` on the argument type; for `object&T` where T is a TemplateMixedType, this returns Yes (inherited from MixedType), causing a false positive "will always evaluate to true" - Add early `return null` when `$objectType->hasTemplateOrLateResolvableType()` is true, preventing the general logic from running on types where method existence is uncertain - Verified analogous cases: `property_exists` is not affected (its type specifying extension returns empty SpecifiedTypes for non-native properties); `is_callable` is not affected; `class-string<T>` template bounds are correctly handled by the existing GenericClassStringType block
VincentLanglet
left a comment
There was a problem hiding this comment.
If the issue seems related to TemplateMixedType, why it doesn't currently occur for MixedType ?
Demonstrates that the false positive only occurs with template types (object&T), not with plain mixed or object. For mixed, ObjectWithoutClassType::isSuperTypeOf(MixedType) returns Maybe (mixed is not necessarily an object), preventing the false positive. For plain object, ObjectWithoutClassType::hasMethod() returns Maybe (via ObjectTypeTrait), also preventing it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the answer to the reviewer's question: Why doesn't The false positive requires two conditions to align in the general type specifier logic:
For plain For plain For I added test cases for the |
VincentLanglet
left a comment
There was a problem hiding this comment.
Should/could the fix be done in the general specifier ?
…o return Maybe A template type T represents a specific-but-unknown type. MixedType::hasMethod() returns Yes (mixed encompasses all types including those with any method), but for a template T we don't know whether the specific type has a given method. This fixes the root cause instead of working around it in ImpossibleCheckTypeHelper. The previous workaround checked hasTemplateOrLateResolvableType() in the method_exists-specific handling to bail out early. With this fix, the general type specifier logic works correctly because HasMethodType::isSuperTypeOf() now gets Maybe (not Yes) from TemplateMixedType, so it no longer concludes that method_exists() on object&T "will always evaluate to true." Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The fix now addresses the reviewer's suggestion: instead of working around the issue in Why this is the right fix: All 11,995 tests pass and |
VincentLanglet
left a comment
There was a problem hiding this comment.
List other possible fixes cause this one is not ok.
MixedType:: and TemplateMixedType methods shouldn't have different result.
…lper Revert the hasMethod() override on TemplateMixedType so that it inherits MixedType::hasMethod() (returning Yes) as before — the two should not diverge. Instead, prevent the method_exists specific handling in ImpossibleCheckTypeHelper from falling through to the general type specifier logic when the object type contains template or late-resolvable types. The general logic incorrectly concludes "always true" because MixedType::hasMethod() returns Yes, which dominates the IntersectionType result via lazyMaxMin. Alternatives considered: - HasMethodType::isSuperTypeOf(): could check for template types, but would incorrectly return Maybe for bounded templates like T of SomeClass where the method genuinely exists on the bound. - IntersectionType::hasMethod(): changing from lazyMaxMin to lazyMin would be a large behavioral change affecting all intersections. - General result processing: capping results at Maybe for template types is equivalent to this fix but less targeted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. The fix now addresses the reviewer's concern: What changed: Reverted the Why the general logic gives the wrong answer for Alternatives considered:
|
VincentLanglet
left a comment
There was a problem hiding this comment.
I feel like the fix should be inside HasMethodType::isSuperTypeOf() instead.
For instance HasMethodType::isSuperTypeOf(Mixed) should be Maybe only (this would be consistent with MixedType::isSubtypeOf(HasMethod) which is maybe)
…ompoundTypes HasMethodType::isSuperTypeOf(MixedType) was returning Yes (via MixedType::hasMethod() which always returns Yes), inconsistent with MixedType::isSubTypeOf(HasMethodType) which returns Maybe. This caused false positives for method_exists() on template types like object&T, where TemplateMixedType (inheriting MixedType::hasMethod()) dominated IntersectionType::hasMethod() via lazyMaxMin. The fix delegates to $type->isSubTypeOf($this) for CompoundType instances (the standard pattern used by other accessory types like AccessoryNonEmptyStringType). When isSubTypeOf returns No, we fall back to hasMethod() — this prevents AccessoryTypes in intersections (e.g. AccessoryLiteralStringType in class-string&literal-string) from being overly strict, since their isSubTypeOf checks orthogonal properties that can incorrectly poison the lazyMaxMin result. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I processed this review but have nothing to report. |
|
why don't we have the same bug for |
|
Indeed, I'll ask the bot |
VincentLanglet
left a comment
There was a problem hiding this comment.
why don't we have the same bug for property_exists?
https://phpstan.org/r/01359ac6-3f8e-4ea2-b6f3-82b1e2de8b3f
… CompoundTypes Same pattern as the HasMethodType fix: delegate to $type->isSubTypeOf($this) for CompoundTypes so that MixedType correctly returns Maybe instead of Yes. property_exists() doesn't currently trigger the same false positive as method_exists() because PropertyExistsTypeSpecifyingExtension bails out early for non-native properties (which MixedType provides). But the underlying type inconsistency is the same: HasPropertyType::isSuperTypeOf(MixedType) was returning Yes when it should return Maybe. Added property_exists test cases to bug-8217 regression test to cover the template, mixed, and object scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the answer to the reviewer's question: Why The false positive requires the general type specifier logic to conclude "always true." For For But the underlying type inconsistency was the same: I also investigated |
VincentLanglet
left a comment
There was a problem hiding this comment.
Just use
if ($type instanceof CompoundType) {
return $type->isSubTypeOf($this);
}
to fix this issue and explains why it impacts class-string&literal-string.
…delegation Use the standard pattern `return $type->isSubTypeOf($this)` instead of falling back to hasMethod()/hasProperty() when isSubTypeOf() returns No. This is consistent with how other AccessoryType classes handle CompoundTypes. The simplified delegation changes behavior for `class-string&literal-string`: since IntersectionType::isSubTypeOf(HasMethodType) uses lazyMaxMin over individual members, and neither ClassStringType nor AccessoryLiteralStringType has methods, the result becomes No instead of the previous Maybe. To prevent this from causing false "always false" reports for method_exists() on class-string types (where the actual class is unknown), add explicit return null in ImpossibleCheckTypeHelper for both generic class-string types (non-final TypeWithClassName) and plain class-string types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the summary: Changes made:
Why this impacts With the simplified delegation, The dedicated |
VincentLanglet
left a comment
There was a problem hiding this comment.
I don't understand your explanation
With the simplified delegation, HasMethodType::isSuperTypeOf(class-string&literal-string) returns No instead of the previous Maybe. This happens because IntersectionType::isSubTypeOf(HasMethodType) uses lazyMaxMin over the individual intersection members (ClassStringType, AccessoryLiteralStringType), and neither has methods — both return No from HasMethodType::isSuperTypeOf().
Because both ClassStringType::hasMethod and AccessoryLiteralStringType::hasMethod are supposed to return MAYBE
…() returns No AccessoryLiteralStringType implements CompoundType, so HasMethodType::isSuperTypeOf(AccessoryLiteralStringType) was delegating to AccessoryLiteralStringType::isSubTypeOf(HasMethodType), which returns No (because HasMethodType::isLiteralString() is No). This made HasMethodType::isSuperTypeOf(class-string&literal-string) return No instead of Maybe, since lazyMaxMin propagated the No from the accessory type even though ClassStringType::hasMethod() correctly returned Maybe. The fix: when the CompoundType delegation returns No, fall back to the hasMethod()/hasProperty() check which gives the correct result. This removes the need for the class-string guard in ImpossibleCheckTypeHelper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's the explanation for the reviewer: Why the The reviewer is correct that both The actual issue: The fix: When the CompoundType delegation returns |
VincentLanglet
left a comment
There was a problem hiding this comment.
Looking at code like
$objectType = $scope->getType($args[0]->value);
if ($objectType->isString()->yes()) {
if ($objectType->isClassString()->yes()) {
return $this->typeSpecifier->create(
$args[0]->value,
new IntersectionType([
$objectType,
new HasMethodType($methodNameType->getValue()),
]),
$context,
$scope,
);
}
return new SpecifiedTypes([], []);
}
I feel like the issue is that HasMethod::isString (and similar method) return No because the ObjectTrait is used while it should return Maybe WDYT ?
Summary
When using
method_exists()on a template type narrowed toobject&T(e.g., after anis_object()check), PHPStan incorrectly reports "will always evaluate to true". This is a false positive because an arbitraryobject&Tdoes not necessarily have any given method.Changes
return nullinsrc/Rules/Comparison/ImpossibleCheckTypeHelper.phpat the end of themethod_existsspecific handling block, when the object type contains template or late-resolvable typestests/PHPStan/Rules/Comparison/data/bug-8217.phpreproducing the exact scenario from the issuetestBug8217()intests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.phpRoot cause
The
method_existsspecific handling inImpossibleCheckTypeHelperhas checks for:getObjectClassNames() !== []) — checkshasMethod()directlyGenericClassStringType— checks on the generic typeFor
object&Twhere T is a bare template type, neither check applies (no class names, no GenericClassStringType), so the code falls through to the general type specifier logic. The general logic usesHasMethodType::isSuperTypeOf(), which calls$type->hasMethod()on the argument. ForIntersectionType([ObjectWithoutClassType, TemplateMixedType]),hasMethod()returnsYesbecauseTemplateMixedTypeinheritsMixedType::hasMethod()which always returnsYes. This causes the general logic to conclude the check "will always evaluate to true".The fix adds an explicit
return nullwhen the object type contains template or late-resolvable types, correctly indicating that method existence is uncertain.Analogous cases investigated
property_exists: Not affected —PropertyExistsTypeSpecifyingExtensionreturns emptySpecifiedTypeswhen finding a non-native property (which MixedType provides), so the general logic has nothing to checkis_callable: Not affected — tested withis_callable($object)onobject&T, no false positiveclass-string<T>templates: Not affected — handled correctly by the existingGenericClassStringTypeblock in the specificmethod_existshandlingT of objectbound: Not affected —TemplateObjectWithoutClassType::hasMethod()correctly returnsMaybe(viaObjectTypeTrait), notYesTest
Regression test reproduces the exact scenario from the issue: a function with
@template Tparameter, narrowed viais_object($object), then checked withmethod_exists($object, 'method'). The test expects no errors (previously reported a false positive).Fixes phpstan/phpstan#8217