mirror of
https://github.com/danog/psalm.git
synced 2024-11-26 20:34:47 +01:00
Throw error if magic getter or setter called for undefined property or invalid type specified with annotations (#500)
* Fix path to psalm * If a magic getter or setter is used to access a property on a class that is not defined but a `@property` annotation for the property exists, throw an error. If no `@property` annotation exists, it's not an error because you're allowed to make magic getters and setters do crazy things. Fixes #480 * Move logic to a better place to avoid duplicate checks * Move logic into function * Remove some nesting * Check psalm-seal-properties and property type correctly
This commit is contained in:
Nicky Robinson
Matthew Brown
parent
55c12cd01c
commit
8e77ff1ce9
@ -38,7 +38,7 @@
|
||||
"php-coveralls/php-coveralls": "^2.0"
|
||||
},
|
||||
"scripts": {
|
||||
"psalm": "./bin/psalm",
|
||||
"psalm": "./psalm",
|
||||
"standards": "php ./vendor/friendsofphp/php-cs-fixer/php-cs-fixer fix --verbose --allow-risky=yes",
|
||||
"tests": [
|
||||
"php ./vendor/squizlabs/php_codesniffer/bin/phpcs",
|
||||
|
||||
@ -7,11 +7,13 @@ use Psalm\Checker\FunctionLikeChecker;
|
||||
use Psalm\Checker\MethodChecker;
|
||||
use Psalm\Checker\Statements\ExpressionChecker;
|
||||
use Psalm\Checker\StatementsChecker;
|
||||
use Psalm\Checker\TypeChecker;
|
||||
use Psalm\Codebase\CallMap;
|
||||
use Psalm\CodeLocation;
|
||||
use Psalm\Config;
|
||||
use Psalm\Context;
|
||||
use Psalm\Issue\InvalidMethodCall;
|
||||
use Psalm\Issue\InvalidPropertyAssignmentValue;
|
||||
use Psalm\Issue\InvalidScope;
|
||||
use Psalm\Issue\MixedMethodCall;
|
||||
use Psalm\Issue\NullReference;
|
||||
@ -20,6 +22,8 @@ use Psalm\Issue\PossiblyInvalidMethodCall;
|
||||
use Psalm\Issue\PossiblyNullReference;
|
||||
use Psalm\Issue\PossiblyUndefinedMethod;
|
||||
use Psalm\Issue\UndefinedMethod;
|
||||
use Psalm\Issue\UndefinedThisPropertyAssignment;
|
||||
use Psalm\Issue\UndefinedThisPropertyFetch;
|
||||
use Psalm\IssueBuffer;
|
||||
use Psalm\Type;
|
||||
use Psalm\Type\Atomic\TGenericObject;
|
||||
@ -377,6 +381,15 @@ class MethodCallChecker extends \Psalm\Checker\Statements\Expression\CallChecker
|
||||
return false;
|
||||
}
|
||||
|
||||
if (!self::checkMagicGetterOrSetterProperty(
|
||||
$statements_checker,
|
||||
$project_checker,
|
||||
$stmt,
|
||||
$fq_class_name
|
||||
)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
$self_fq_class_name = $fq_class_name;
|
||||
|
||||
$return_type_candidate = $codebase->methods->getMethodReturnType(
|
||||
@ -544,4 +557,113 @@ class MethodCallChecker extends \Psalm\Checker\Statements\Expression\CallChecker
|
||||
$context->vars_in_scope[$var_id] = $class_type;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Check properties accessed with magic getters and setters.
|
||||
* If `@psalm-seal-properties` is set, they must be defined.
|
||||
* If an `@property` annotation is specified, the setter must set something with the correct
|
||||
* type.
|
||||
*
|
||||
* @param StatementsChecker $statements_checker
|
||||
* @param \Psalm\Checker\ProjectChecker $project_checker
|
||||
* @param PhpParser\Node\Expr\MethodCall $stmt
|
||||
* @param string $fq_class_name
|
||||
*
|
||||
* @return bool
|
||||
*/
|
||||
private static function checkMagicGetterOrSetterProperty(
|
||||
StatementsChecker $statements_checker,
|
||||
\Psalm\Checker\ProjectChecker $project_checker,
|
||||
PhpParser\Node\Expr\MethodCall $stmt,
|
||||
$fq_class_name
|
||||
) {
|
||||
if (!is_string($stmt->name)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
$method_name = strtolower($stmt->name);
|
||||
if (!in_array($method_name, ['__get', '__set'], true)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
$first_arg_value = $stmt->args[0]->value;
|
||||
if (!$first_arg_value instanceof PhpParser\Node\Scalar\String_) {
|
||||
return true;
|
||||
}
|
||||
|
||||
$prop_name = $first_arg_value->value;
|
||||
$property_id = $fq_class_name . '::$' . $prop_name;
|
||||
$class_storage = $project_checker->classlike_storage_provider->get($fq_class_name);
|
||||
|
||||
// if the property exists on the object, everything is good
|
||||
if ($project_checker->codebase->properties->propertyExists($property_id)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
switch ($method_name) {
|
||||
case '__set':
|
||||
// If `@psalm-seal-properties` is set, the property must be defined with
|
||||
// a `@property` annotation
|
||||
if ($class_storage->sealed_properties
|
||||
&& !isset($class_storage->pseudo_property_set_types['$' . $prop_name])
|
||||
&& IssueBuffer::accepts(
|
||||
new UndefinedThisPropertyAssignment(
|
||||
'Instance property ' . $property_id . ' is not defined',
|
||||
new CodeLocation($statements_checker->getSource(), $stmt)
|
||||
),
|
||||
$statements_checker->getSuppressedIssues()
|
||||
)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
|
||||
// If a `@property` annotation is set, the type of the value passed to the
|
||||
// magic setter must match the annotation.
|
||||
$second_arg_type = $stmt->args[1]->value->inferredType;
|
||||
if (isset($class_storage->pseudo_property_set_types['$' . $prop_name])
|
||||
&& isset($second_arg_type)
|
||||
&& !TypeChecker::isContainedBy(
|
||||
$project_checker->codebase,
|
||||
$second_arg_type,
|
||||
ExpressionChecker::fleshOutType(
|
||||
$project_checker,
|
||||
$class_storage->pseudo_property_set_types['$' . $prop_name],
|
||||
$fq_class_name,
|
||||
$fq_class_name
|
||||
)
|
||||
)
|
||||
&& IssueBuffer::accepts(
|
||||
new InvalidPropertyAssignmentValue(
|
||||
$prop_name . ' with declared type \''
|
||||
. $class_storage->pseudo_property_set_types['$' . $prop_name]
|
||||
. '\' cannot be assigned type \'' . $second_arg_type . '\'',
|
||||
new CodeLocation($statements_checker->getSource(), $stmt)
|
||||
),
|
||||
$statements_checker->getSuppressedIssues()
|
||||
)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
break;
|
||||
|
||||
case '__get':
|
||||
// If `@psalm-seal-properties` is set, the property must be defined with
|
||||
// a `@property` annotation
|
||||
if ($class_storage->sealed_properties
|
||||
&& !isset($class_storage->pseudo_property_get_types['$' . $prop_name])
|
||||
&& IssueBuffer::accepts(
|
||||
new UndefinedThisPropertyFetch(
|
||||
'Instance property ' . $property_id . ' is not defined',
|
||||
new CodeLocation($statements_checker->getSource(), $stmt)
|
||||
),
|
||||
$statements_checker->getSuppressedIssues()
|
||||
)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
break;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
@ -410,6 +410,77 @@ class AnnotationTest extends TestCase
|
||||
'$b' => 'null|stdClass',
|
||||
],
|
||||
],
|
||||
/**
|
||||
* With a magic setter and no annotations specifying properties or types, we can
|
||||
* set anything we want on any variable name. The magic setter is trusted to figure
|
||||
* it out.
|
||||
*/
|
||||
'magicSetterUndefinedPropertyNoAnnotation' => [
|
||||
'<?php
|
||||
class A {
|
||||
public function __get(string $name): ?string {
|
||||
if ($name === "foo") {
|
||||
return "hello";
|
||||
}
|
||||
}
|
||||
|
||||
/** @param mixed $value */
|
||||
public function __set(string $name, $value): void {
|
||||
}
|
||||
|
||||
public function goodSet(): void {
|
||||
$this->__set("foo", new stdClass());
|
||||
}
|
||||
}',
|
||||
],
|
||||
/**
|
||||
* With a magic getter and no annotations specifying properties or types, we can
|
||||
* get anything we want with any variable name. The magic getter is trusted to figure
|
||||
* it out.
|
||||
*/
|
||||
'magicGetterUndefinedPropertyNoAnnotation' => [
|
||||
'<?php
|
||||
class A {
|
||||
public function __get(string $name): ?string {
|
||||
if ($name === "foo") {
|
||||
return "hello";
|
||||
}
|
||||
}
|
||||
|
||||
/** @param mixed $value */
|
||||
public function __set(string $name, $value): void {
|
||||
}
|
||||
|
||||
public function goodGet(): void {
|
||||
echo $this->__get("foo");
|
||||
}
|
||||
}',
|
||||
],
|
||||
/**
|
||||
* The property $foo is defined as a string with the `@property` annotation. We
|
||||
* use the magic setter to set it to a string, so everything is cool.
|
||||
*/
|
||||
'magicSetterValidAssignmentType' => [
|
||||
'<?php
|
||||
/**
|
||||
* @property string $foo
|
||||
*/
|
||||
class A {
|
||||
public function __get(string $name): ?string {
|
||||
if ($name === "foo") {
|
||||
return "hello";
|
||||
}
|
||||
}
|
||||
|
||||
/** @param mixed $value */
|
||||
public function __set(string $name, $value): void {
|
||||
}
|
||||
|
||||
public function goodSet(): void {
|
||||
$this->__set("foo", "value");
|
||||
}
|
||||
}',
|
||||
],
|
||||
];
|
||||
}
|
||||
|
||||
@ -912,6 +983,84 @@ class AnnotationTest extends TestCase
|
||||
}',
|
||||
'error_message' => 'PossiblyInvalidMethodCall',
|
||||
],
|
||||
/**
|
||||
* The property $foo is not defined on the object, but accessed with the magic setter.
|
||||
* This is an error because `@psalm-seal-properties` is specified on the class block.
|
||||
*/
|
||||
'magicSetterUndefinedProperty' => [
|
||||
'<?php
|
||||
/**
|
||||
* @psalm-seal-properties
|
||||
*/
|
||||
class A {
|
||||
public function __get(string $name): ?string {
|
||||
if ($name === "foo") {
|
||||
return "hello";
|
||||
}
|
||||
}
|
||||
|
||||
/** @param mixed $value */
|
||||
public function __set(string $name, $value): void {
|
||||
}
|
||||
|
||||
public function badSet(): void {
|
||||
$this->__set("foo", "value");
|
||||
}
|
||||
}',
|
||||
'error_message' => 'UndefinedThisPropertyAssignment',
|
||||
],
|
||||
/**
|
||||
* The property $foo is not defined on the object, but accessed with the magic getter.
|
||||
* This is an error because `@psalm-seal-properties` is specified on the class block.
|
||||
*/
|
||||
'magicGetterUndefinedProperty' => [
|
||||
'<?php
|
||||
/**
|
||||
* @psalm-seal-properties
|
||||
*/
|
||||
class A {
|
||||
public function __get(string $name): ?string {
|
||||
if ($name === "foo") {
|
||||
return "hello";
|
||||
}
|
||||
}
|
||||
|
||||
/** @param mixed $value */
|
||||
public function __set(string $name, $value): void {
|
||||
}
|
||||
|
||||
public function badGet(): void {
|
||||
$this->__get("foo");
|
||||
}
|
||||
}',
|
||||
'error_message' => 'UndefinedThisPropertyFetch',
|
||||
],
|
||||
/**
|
||||
* The property $foo is defined as a string with the `@property` annotation, but
|
||||
* the magic setter is used to set it to an object.
|
||||
*/
|
||||
'magicSetterInvalidAssignmentType' => [
|
||||
'<?php
|
||||
/**
|
||||
* @property string $foo
|
||||
*/
|
||||
class A {
|
||||
public function __get(string $name): ?string {
|
||||
if ($name === "foo") {
|
||||
return "hello";
|
||||
}
|
||||
}
|
||||
|
||||
/** @param mixed $value */
|
||||
public function __set(string $name, $value): void {
|
||||
}
|
||||
|
||||
public function badSet(): void {
|
||||
$this->__set("foo", new stdClass());
|
||||
}
|
||||
}',
|
||||
'error_message' => 'InvalidPropertyAssignmentValue',
|
||||
],
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user