Fix phpstan/phpstan#14429: Treat PHPDocTypes as certain:false being ignored#5394
Fix phpstan/phpstan#14429: Treat PHPDocTypes as certain:false being ignored#5394phpstan-bot wants to merge 8 commits intophpstan:2.1.xfrom
Conversation
- ObjectType::equals(GenericObjectType) incorrectly returned true when both had the same class name, because it didn't check whether generic type arguments differed - This caused MutatingScope::mergeWith() native type optimization to replace native types (e.g. ArrayObject) with PHPDoc-enriched types (e.g. ArrayObject<string, string>), contaminating native type tracking after any branch merge - With treatPhpDocTypesAsCertain: false, this led to false "will always evaluate to true" errors for is_string/is_int checks on foreach values from generic objects - Added get_class equality check in ObjectType::equals() which also subsumes the old instanceof EnumCaseObjectType check - Removed now-unneeded baseline entry for the EnumCaseObjectType instanceof check - New regression test in tests/PHPStan/Rules/Comparison/data/bug-14429.php
|
@staabm I don't understand why I cannot reproduce the nextras/orm failure I tried this one https://github.com/nextras/orm/blob/274ef5b74d596f01528747deee26589706531c82/src/Relationships/HasOne.php#L269 Am I missing something to you ? |
|
I can reproduce it in isolation with <?php declare(strict_types = 1);
namespace Nextras\Orm\Relationships;
/**
* @template E of IEntity
* @implements IRelationshipContainer<E>
*/
abstract class HasOne implements IRelationshipContainer
{
/** @var E|null */
protected ?IEntity $parent = null;
/** @var IRepository<E>|null */
protected ?IRepository $targetRepository = null;
/**
* @return IRepository<E>
*/
protected function getTargetRepository(): IRepository
{
if ($this->targetRepository === null) {
/** @var IRepository<E> $targetRepository */
$targetRepository = $this->getParentEntity()
->getRepository()
->getModel()
->getRepository($this->metadataRelationship->repository);
$this->targetRepository = $targetRepository;
}
return $this->targetRepository;
}
/**
* @return E
*/
protected function getParentEntity(): IEntity
{
return $this->parent ?? throw new \RuntimeException('Relationship is not attached to a parent entity.');
}
}
interface IEntity {
/**
* @return IRepository<IEntity>
*/
public function getRepository(): IRepository;
}
interface IProperty {}
interface IPropertyContainer extends IProperty {}
/**
* @template E of IEntity
*/
interface IEntityAwareProperty extends IProperty {}
/**
* @template E of IEntity
* @extends IEntityAwareProperty<E>
*/
interface IRelationshipContainer extends IPropertyContainer, IEntityAwareProperty {}
interface IModel {
/**
* @template E of IEntity
* @template T of IRepository<E>
* @param class-string<T> $className
* @return T
*/
public function getRepository(string $className): IRepository;
}
/**
* @template E of IEntity
*/
interface IRepository {
public function getModel(): IModel;
}but was not yet able to reproduce it in a test |
VincentLanglet
left a comment
There was a problem hiding this comment.
This fix introduce a regression (see failing test), because
Bug14429\IRepository<Bug14429\IEntity>
is not a superTypeOf
Bug14429\IRepository<E of Bug14429\IEntity (class Bug14429\HasOne, parameter)>
anymore.
This is because
E of Bug14429\IEntity (class Bug14429\HasOne, parameter)
and
Bug14429\IEntity
are not equals anymore.
WDYT ?
The previous fix using `get_class($type) !== static::class` was too strict - it prevented TemplateObjectType from being considered equal to ObjectType, breaking invariant generic type variance checks. Narrow the check to only reject GenericObjectType and EnumCaseObjectType when the concrete classes differ, preserving template type compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The fix narrows the
|
VincentLanglet
left a comment
There was a problem hiding this comment.
Do you have a solution to keep
ObjectType->(TemplateObjectType) false
but IRepository remains a valid subtype of IRepository
…t variance for template types Instead of narrowly rejecting only EnumCaseObjectType and GenericObjectType in ObjectType::equals(), use a strict get_class() comparison for all ObjectType subclasses. This makes equals() properly symmetric and prevents any ObjectType subclass from being considered equal to a plain ObjectType. To preserve the correct behavior where IRepository<E of IEntity> remains a valid subtype of IRepository<IEntity>, add a template type bound check in TemplateTypeVariance::isValidVariance() for invariant positions: when the actual type argument ($b) is a TemplateType whose bound equals the expected type argument ($a), treat them as equal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The background task (full |
VincentLanglet
left a comment
There was a problem hiding this comment.
I start to think the isSuperTypeOf was wrong too cause if you look at
/**
* @return IRepository<E>
*/
protected function getTargetRepository(): IRepository
{
if ($this->targetRepository === null) {
/** @var IRepository<E> $targetRepository */
$targetRepository = $this->getParentEntity()
->getRepository()
->getModel()
->getRepository($this->metadataRelationship->repository);
\PHPStan\dumpType($targetRepository);
$this->test($targetRepository);
\PHPStan\dumpType($targetRepository);
$this->targetRepository = $targetRepository;
}
return $this->targetRepository;
}
/** @param IRepository<IEntity> $i */
public function test(IRepository $i): void {}
The IEntity is not covariant so test method might modify the IRepository and therefor the call $this->test($targetRepository); should be reported.
So maybe the issue is somewhere else, (in the VarTagTypeRuleHelper maybe ?)
We should have the same behavior with
/** @var IRepository<E> $targetRepository */
or
class Foo implements IEntity {}
/** @var IRepository<Foo> $targetRepository */
so no varTag.subType error
…leHelper Revert the TemplateTypeVariance hack that treated template types as equal to their bounds for invariant checks - this was conceptually wrong since invariant means exact type equality. Instead, fix VarTagTypeRuleHelper::checkType() to compare generic object type arguments individually (covariantly) when both types are the same generic class. This correctly allows @var tags like `@var IRepository<E>` or `@var IRepository<Foo>` when the expression type is `IRepository<IEntity>` and E/Foo are subtypes of IEntity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's what I changed to address VincentLanglet's review:
All tests pass (11723 tests, 78964 assertions) and |
VincentLanglet
left a comment
There was a problem hiding this comment.
Instead of modifying the VarTagTypeRule, maybe consider playing with the $acceptsContext boolean ?
…ypeVariance Instead of special-casing GenericObjectType in VarTagTypeRuleHelper, handle the template type equivalence at the type system level: in the invariant variance check, when the other type ($b) is a TemplateType whose bound equals $a, consider them equivalent. This approach uses the existing $acceptsContext infrastructure direction rather than adding rule-level workarounds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pushed. Here's what I changed to address VincentLanglet's review: Removed the Added a template type bound check in The check is intentionally one-directional (
All tests pass (11723 tests, 78964 assertions) and |
Summary
When
treatPhpDocTypesAsCertain: falseis set, PHPStan incorrectly reported "will always evaluate to true" errors for type-checking functions likeis_string()andis_int()on variables from foreach loops over generic objects (e.g.ArrayObject<string, string>). The native types were being contaminated with PHPDoc generic type information after any branch merge (if/else, loops, etc.).Changes
ObjectType::equals()insrc/Type/ObjectType.phpto check that both types are the same concrete class usingget_class(), preventingObjectTypefrom being considered equal toGenericObjectTypewith the same class nameinstanceof EnumCaseObjectTypecheck (subsumed by theget_classcheck)phpstan-baseline.neontests/PHPStan/Rules/Comparison/data/bug-14429.phpRoot cause
ObjectType::equals()used$type instanceof selfto verify type compatibility, but sinceGenericObjectTypeextendsObjectType, a plainObjectType('ArrayObject')was considered equal toGenericObjectType('ArrayObject', [StringType, StringType]).This equality bug was triggered by an optimization in
MutatingScope::mergeWith()(lines 3392-3414) that shortcuts native type merging: when "our native type equals our expression type" AND "their native type equals their expression type", it uses the merged expression type as the merged native type. Due to the brokenequals(), nativeArrayObject|nullwas considered equal to PHPDocArrayObject<string, string>|null, causing the optimization to replace the native type with the PHPDoc-enriched merged expression type.After any branch merge (if/else, foreach, while, etc.), native types of generic object variables got contaminated with PHPDoc generic information. With
treatPhpDocTypesAsCertain: false, theImpossibleCheckTypeHelperuses native types to determine if type checks are always true/false, so the contaminated native types led to false positive "always true" errors.Test
Added a rule test that verifies no false positives are reported when
treatPhpDocTypesAsCertain: falseforis_string()andis_int()calls on foreach values fromArrayObjectparameters with PHPDoc generic types.Fixes phpstan/phpstan#14429