Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion phpstan-baseline.neon
Original file line number Diff line number Diff line change
Expand Up @@ -1644,7 +1644,7 @@ parameters:
-
rawMessage: 'Doing instanceof PHPStan\Type\Constant\ConstantStringType is error-prone and deprecated. Use Type::getConstantStrings() instead.'
identifier: phpstanApi.instanceofType
count: 2
count: 1
path: src/Type/Php/PropertyExistsTypeSpecifyingExtension.php

-
Expand Down
19 changes: 19 additions & 0 deletions src/Rules/Properties/AccessStaticPropertiesCheck.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,14 @@
namespace PHPStan\Rules\Properties;

use PhpParser\Node;
use PhpParser\Node\Arg;
use PhpParser\Node\Expr\ClassConstFetch;
use PhpParser\Node\Expr\FuncCall;
use PhpParser\Node\Expr\StaticPropertyFetch;
use PhpParser\Node\Identifier;
use PhpParser\Node\Name;
use PhpParser\Node\Name\FullyQualified;
use PhpParser\Node\Scalar\String_;
use PHPStan\Analyser\NullsafeOperatorHelper;
use PHPStan\Analyser\Scope;
use PHPStan\DependencyInjection\AutowiredParameter;
Expand Down Expand Up @@ -256,6 +262,19 @@
]);
}

if ($node->class instanceof Name) {
$classExpr = new ClassConstFetch($node->class, new Identifier('class'));
} else {
$classExpr = $node->class;
}
$propertyExistsCall = new FuncCall(new FullyQualified('property_exists'), [
new Arg($classExpr),
new Arg(new String_($name)),
]);
if ($scope->getType($propertyExistsCall)->isTrue()->yes()) {

Check warning on line 274 in src/Rules/Properties/AccessStaticPropertiesCheck.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.4, ubuntu-latest)

Escaped Mutant for Mutator "PHPStan\Infection\LooseBooleanMutator": @@ @@ new Arg($classExpr), new Arg(new String_($name)), ]); - if ($scope->getType($propertyExistsCall)->isTrue()->yes()) { + if ($scope->getType($propertyExistsCall)->toBoolean()->isTrue()->yes()) { return []; }

Check warning on line 274 in src/Rules/Properties/AccessStaticPropertiesCheck.php

View workflow job for this annotation

GitHub Actions / Mutation Testing (8.3, ubuntu-latest)

Escaped Mutant for Mutator "PHPStan\Infection\LooseBooleanMutator": @@ @@ new Arg($classExpr), new Arg(new String_($name)), ]); - if ($scope->getType($propertyExistsCall)->isTrue()->yes()) { + if ($scope->getType($propertyExistsCall)->toBoolean()->isTrue()->yes()) { return []; }
return [];
}

return array_merge($messages, [
RuleErrorBuilder::message(sprintf(
'Access to an undefined static property %s::$%s.',
Expand Down
34 changes: 27 additions & 7 deletions src/Type/Php/PropertyExistsTypeSpecifyingExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
use PHPStan\Reflection\FunctionReflection;
use PHPStan\Rules\Properties\PropertyReflectionFinder;
use PHPStan\Type\Accessory\HasPropertyType;
use PHPStan\Type\ClassStringType;
use PHPStan\Type\Constant\ConstantBooleanType;
use PHPStan\Type\Constant\ConstantStringType;
use PHPStan\Type\FunctionTypeSpecifyingExtension;
use PHPStan\Type\IntersectionType;
use PHPStan\Type\ObjectWithoutClassType;
use PHPStan\Type\UnionType;
use function count;

#[AutowiredService]
Expand Down Expand Up @@ -66,17 +68,35 @@ public function specifyTypes(
}

$objectType = $scope->getType($args[0]->value);
if ($objectType instanceof ConstantStringType) {
return new SpecifiedTypes([], []);
} elseif ($objectType->isObject()->yes()) {
$propertyNode = new PropertyFetch(
if ($objectType->isString()->yes()) {
return $this->typeSpecifier->create(
new FuncCall(new FullyQualified('property_exists'), $node->getRawArgs()),
new ConstantBooleanType(true),
$context,
$scope,
);
}

if (!$objectType->isObject()->yes()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this if-statement is not covered by a test

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed successfully. Here's a summary of what was done:

Addressing staabm's review comments:

  1. Non-static property fetch tests — The remote already had concurrent changes adding InstancePropertyAccess class tests (with $this and $obj patterns) to both bug-2861.php and bug-2861-assign.php. I resolved the rebase conflicts keeping those additions.

  2. Uncovered !$objectType->isObject()->yes() branch — Added tests/PHPStan/Analyser/nsrt/bug-2861.php with assertType tests that exercise the object|string narrowing path in PropertyExistsTypeSpecifyingExtension. This covers the branch where the first argument to property_exists() is neither purely string nor purely object, verifying it narrows to class-string|(object&hasProperty(foo)).

return $this->typeSpecifier->create(
$args[0]->value,
new Identifier($propertyNameType->getValue()),
new UnionType([
new IntersectionType([
new ObjectWithoutClassType(),
new HasPropertyType($propertyNameType->getValue()),
]),
new ClassStringType(),
]),
$context,
$scope,
);
} else {
return new SpecifiedTypes([], []);
}

$propertyNode = new PropertyFetch(
$args[0]->value,
new Identifier($propertyNameType->getValue()),
);

$propertyReflection = $this->propertyReflectionFinder->findPropertyReflectionFromNode($propertyNode, $scope);
if ($propertyReflection !== null) {
if (!$propertyReflection->isNative()) {
Expand Down
23 changes: 23 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-2861.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php declare(strict_types = 1);

namespace Bug2861Nsrt;

use function PHPStan\Testing\assertType;

/**
* @param object|string $objectOrClass
*/
function testObjectOrString($objectOrClass): void {
if (property_exists($objectOrClass, 'foo')) {
assertType('class-string|(object&hasProperty(foo))', $objectOrClass);
}
}

/**
* @param object|class-string $objectOrClass
*/
function testObjectOrClassString($objectOrClass): void {
if (property_exists($objectOrClass, 'bar')) {
assertType('class-string|(object&hasProperty(bar))', $objectOrClass);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -253,4 +253,9 @@ public function testCloneWith(): void
]);
}

public function testBug2861(): void
{
$this->analyse([__DIR__ . '/data/bug-2861-assign.php'], []);
}

}
8 changes: 8 additions & 0 deletions tests/PHPStan/Rules/Properties/AccessPropertiesRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1294,4 +1294,12 @@ public function testBug13539(): void
]);
}

public function testBug2861(): void
{
$this->checkThisOnly = false;
$this->checkUnionTypes = true;
$this->checkDynamicProperties = true;
$this->analyse([__DIR__ . '/data/bug-2861.php'], []);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ public function testRuleExpressionNames(): void
]);
}

public function testBug2861(): void
{
$this->analyse([__DIR__ . '/data/bug-2861-assign.php'], []);
}

#[RequiresPhp('>= 8.5.0')]
public function testAsymmetricVisibility(): void
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,4 +353,9 @@ public function testBug8668Bis(): void
]);
}

public function testBug2861(): void
{
$this->analyse([__DIR__ . '/data/bug-2861.php'], []);
}

}
33 changes: 33 additions & 0 deletions tests/PHPStan/Rules/Properties/data/bug-2861-assign.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php declare(strict_types = 1);

namespace Bug2861Assign;

class Foo {
public static function test(): void {
if (property_exists(static::class, 'default')) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also test non-static property fetches, like in https://phpstan.org/r/20ab67ef-ef8b-4f24-8783-1ff18cfe7168

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added non-static (instance) property fetch test cases as requested by staabm:

  • bug-2861.php: Added InstancePropertyAccess class with $this->default and $obj->default reads guarded by property_exists()
  • bug-2861-assign.php: Added testInstance() and testInstanceObj() methods with $this->default = 'value' and $obj->default = 'value' writes guarded by property_exists()
  • AccessPropertiesRuleTest: Added testBug2861() to verify instance property reads don't trigger errors
  • AccessPropertiesInAssignRuleTest: Added testBug2861() to verify instance property writes don't trigger errors

All 12131 tests pass and make phpstan reports no errors.

static::$default = 'value';
}
}

/**
* @param class-string $className
*/
public static function testExpr(string $className): void {
if (property_exists($className, 'default')) {
$className::$default = 'value';
}
}

public function testInstance(): void {
if (property_exists($this, 'default')) {
$this->default = 'value';
}
}

/** @param self $obj */
public function testInstanceObj(self $obj): void {
if (property_exists($obj, 'default')) {
$obj->default = 'value';
}
}
}
71 changes: 71 additions & 0 deletions tests/PHPStan/Rules/Properties/data/bug-2861.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php declare(strict_types = 1);

namespace Bug2861;

trait EnumTrait {
/** @var mixed */
protected $value;

/** @param mixed $value */
final public function __construct($value) {
$this->value = $value;
}

/** @return static|null */
public static function getDefault() {
if (property_exists(static::class, 'default') && null !== static::$default) {
$obj = static::$default;
return new static($obj);
}
return null;
}
}

class Foo {
use EnumTrait;
public const BLA = 'bla';
}

class Bar {
use EnumTrait;
public static $default = 'bla';
public const BLA = 'bla';
}

class Baz {
use EnumTrait;

/** @return static|null */
public static function getDefault2() {
if (property_exists(self::class, 'default') && null !== self::$default) {
return new static(self::$default);
}
return null;
}
}

class ExpressionBased {
/**
* @param class-string $className
*/
public static function test(string $className): void {
if (property_exists($className, 'default')) {
echo $className::$default;
}
}
}

class InstancePropertyAccess {
public function test(): void {
if (property_exists($this, 'default')) {
echo $this->default;
}
}

/** @param self $obj */
public function testObj(self $obj): void {
if (property_exists($obj, 'default')) {
echo $obj->default;
}
}
}
Loading