Switch HasMethodType/HasPropertyType from ObjectTypeTrait to MaybeObjectTypeTrait#5605
Conversation
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 ?
|
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 ?
|
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.
|
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)
|
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
|
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.
|
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
|
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 ?
|
Pushed. Here's the summary of what was done: The reviewer was right: What changed:
|
VincentLanglet
left a comment
There was a problem hiding this comment.
HasMethodType and HasPropertyType should rely on MaybeObjectTypeTrait rather than ObjectTypeTrait (and maybe override some property).
Also, a MaybeStringTypeTrait might be useful to deduplicate some methods
|
All 11,997 tests pass and Here's a summary of the changes: Commit 1: Extract
Commit 2: Switch
|
| return TrinaryLogic::createMaybe(); | ||
| } | ||
|
|
||
| public function getCallableParametersAcceptors(ClassMemberAccessAnswerer $scope): array |
There was a problem hiding this comment.
getCallableParametersAcceptors is already in MaybeCallableTypeTrait
There was a problem hiding this comment.
Pushed. Here's a summary of the changes addressing each review comment:
-
Removed redundant
getCallableParametersAcceptors()fromHasMethodType— already provided byMaybeCallableTypeTrait(and removed unusedTrivialParametersAcceptorimports from both files) -
Moved
getConstantStrings()intoMaybeStringTypeTrait— removed the duplicategetConstantStrings()returning[]fromHasMethodType,HasPropertyType,HasOffsetType, andHasOffsetValueType -
getClassStringObjectType()returns$thisinstead ofErrorType— this preserves method/property info when resolving class-string intersections. ReturningObjectWithoutClassType(as initially suggested) loses the method info, which breaks callable detection aftermethod_exists()checks (e.g.,new $factory()aftermethod_exists($factory, '__invoke')) -
getObjectTypeOrClassStringObjectType()keeps returning$this— same rationale.NewHandleruses this method to resolvenew $variable()types, and returningObjectWithoutClassTypeloses theHasMethodTypeinfo needed for callable checks
| return []; | ||
| } | ||
|
|
||
| public function getConstantStrings(): array |
There was a problem hiding this comment.
I think this can be in MaybeStringTrait ? And I dunno if some other method should be added in the MaybeStringTypeTrait.
|
|
||
| public function getClassStringObjectType(): Type | ||
| { | ||
| return new ErrorType(); |
There was a problem hiding this comment.
This should be ObjectWithoutClassType
VincentLanglet
left a comment
There was a problem hiding this comment.
Suggest a better title and description for the PR now
|
The token doesn't have write access to the upstream repo. Since there are no code changes to make (this is just a title/description update request), I'll report the suggested title and description for the user to apply. The bot token doesn't have permission to edit the PR on New title:
New description: Summary
Root cause
Test plan
Closes phpstan/phpstan#8217 The token lacks write access to |
method_exists() check fall through to general logic when object type contains template typese729d86 to
efa69dc
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Fix
FILE: src/Type/Accessory/HasPropertyType.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
8 | ERROR | [x] Type PHPStan\Reflection\ClassMemberAccessAnswerer is not used
| | in this file.
| | (SlevomatCodingStandard.Namespaces.UnusedUses.UnusedUse)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
|
Background tests also completed successfully. Everything's green — the fix is pushed. |
6785bbc to
f0812c1
Compare
f0812c1 to
ceb425f
Compare
Summary
HasMethodTypeandHasPropertyTypefromObjectTypeTraittoMaybeObjectTypeTrait, so string/scalar/object type queries returnMaybeinstead of definitive answers — these accessory types can appear in intersections with both objects and class-stringsisSuperTypeOf()in both types to delegate toCompoundType::isSubTypeOf(), makingHasMethodType::isSuperTypeOf(MixedType)correctly returnMaybeinstead ofYesMaybeStringTypeTraitto deduplicate string-relatedMaybemethods acrossHasMethodType,HasPropertyType,HasOffsetType, andHasOffsetValueTypeRoot cause
HasMethodTypeandHasPropertyTypeusedObjectTypeTrait, which madeisString(),isScalar(), etc. returnNo. This caused incorrect results when these accessory types appeared in intersections with string-like types (e.g.class-string & literal-string & hasMethod(foo)). Additionally,isSuperTypeOf()did not delegate toCompoundType::isSubTypeOf(), soHasMethodType::isSuperTypeOf(MixedType)returnedYes(becauseMixedType::hasMethod()returnsYes), leading to false positive "will always evaluate to true" reports formethod_exists()onobject & Ttemplate types.Test plan
method_exists()onobject & T(bug-8217.php)method_exists/property_existswithmixedandobjecttypesHasMethodType::isSuperTypeOf(MixedType)=MaybeHasPropertyType::isSuperTypeOf(MixedType)=Maybemake tests— all 11,997 tests passmake phpstan— no errorsCloses phpstan/phpstan#8217