From 8e77ff1ce9f5e79c556706cd2d6463061e39d1bb Mon Sep 17 00:00:00 2001 From: Nicky Robinson Date: Fri, 9 Feb 2018 19:37:09 -0500 Subject: [PATCH] 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 --- composer.json | 2 +- .../Expression/Call/MethodCallChecker.php | 122 ++++++++++++++ tests/AnnotationTest.php | 149 ++++++++++++++++++ 3 files changed, 272 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 698480aaa..476fee89b 100644 --- a/composer.json +++ b/composer.json @@ -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", diff --git a/src/Psalm/Checker/Statements/Expression/Call/MethodCallChecker.php b/src/Psalm/Checker/Statements/Expression/Call/MethodCallChecker.php index 63077e6f2..ff03af95a 100644 --- a/src/Psalm/Checker/Statements/Expression/Call/MethodCallChecker.php +++ b/src/Psalm/Checker/Statements/Expression/Call/MethodCallChecker.php @@ -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; + } } diff --git a/tests/AnnotationTest.php b/tests/AnnotationTest.php index e7a09c37b..ea6a11d25 100644 --- a/tests/AnnotationTest.php +++ b/tests/AnnotationTest.php @@ -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' => [ + '__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' => [ + '__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' => [ + '__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' => [ + '__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' => [ + '__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' => [ + '__set("foo", new stdClass()); + } + }', + 'error_message' => 'InvalidPropertyAssignmentValue', + ], ]; } }