Merge pull request #91 from BambooHR/jgardiner/readonly-improvements

Improve support for readonly properties.

src/Abstractions/ClassAbstraction.php

@@ -63,6 +63,10 @@ public function isDeclaredAbstract() {
 		}
 	}
 
+	public function isReadOnly(): bool {
+		return ($this->class instanceof Class_ ? $this->class->isReadonly() : false);
+	}
+
 	/**
 	 * getMethodNames
 	 *
@@ -199,27 +203,31 @@ public function getPropertyNames() {
 	 * @return Property
 	 */
 	public function getProperty($name) {
-		$properties = Grabber::filterByType($this->class->stmts, \PhpParser\Node\Stmt\Property::class);
-		foreach ($properties as $prop) {
-			/** @var \PhpParser\Node\Stmt\Property $prop */
-			foreach ($prop->props as $propertyProperty) {
-				/** @var PropertyProperty $propertyProperty */
-				if ($propertyProperty->name == $name) {
-					if ($prop->isPrivate()) {
-						$access = "private";
-					} else if ($prop->isProtected()) {
-						$access = "protected";
-					} else {
-						$access = "public";
+		foreach ($this->class->stmts as $prop) {
+			if ($prop instanceof \PhpParser\Node\Stmt\Property) {
+				/** @var \PhpParser\Node\Stmt\Property $prop */
+				foreach ($prop->props as $propertyProperty) {
+					/** @var PropertyProperty $propertyProperty */
+					if ($propertyProperty->name == $name) {
+						if ($prop->isPrivate()) {
+							$access = "private";
+						} else {
+							if ($prop->isProtected()) {
+								$access = "protected";
+							} else {
+								$access = "public";
+							}
+						}
+						$type = $prop->type;
+						//if (Config::shouldUseDocBlockForProperties() && empty($type)) {
+						//	$type = Scope::nameFromName($propertyProperty->namespacedType);
+						//}
+						return new Property($this,$propertyProperty->name, $type, $access, $prop->isStatic(), $prop->isReadOnly());
 					}
-					$type = $prop->type;
-					//if (Config::shouldUseDocBlockForProperties() && empty($type)) {
-					//	$type = Scope::nameFromName($propertyProperty->namespacedType);
-					//}
-					return new Property($propertyProperty->name, $type, $access, $prop->isStatic());
 				}
 			}
 		}
+		return null;
 	}
 
 	public function isEnum(): bool {

src/Abstractions/ClassInterface.php

@@ -94,4 +94,6 @@ public function getConstantExpr($name):null|Expr|Name;
 	public function isInterface();
 
 	public function isEnum():bool;
+
+	public function isReadOnly():bool;
 }

src/Abstractions/Property.php

@@ -27,21 +27,22 @@ class Property {
 	 */
 	private $access;
 
-	/**
-	 * @var bool
-	 */
-	private $static;
+
+	private bool $isStatic;
+
+	private bool $isReadOnly;
 
 	/**
 	 * Property constructor.
 	 *
 
 	 */
-	public function __construct(string $name,?\PhpParser\Node $type, string $access, string $isStatic) {
+	public function __construct(private ClassInterface $cls, string $name,?\PhpParser\Node $type, string $access, bool $isStatic, bool $isReadOnly) {
 		$this->name = $name;
 		$this->access = $access;
 		$this->type = $type;
-		$this->static = $isStatic;
+		$this->isStatic = $isStatic;
+		$this->isReadOnly = $isReadOnly;
 	}
 
 	/**
@@ -53,6 +54,10 @@ public function getName() {
 		return $this->name;
 	}
 
+	public function getClass():ClassInterface {
+		return $this->cls;
+	}
+
 	/**
 	 * getAccess
 	 *
@@ -62,6 +67,10 @@ public function getAccess() {
 		return $this->access;
 	}
 
+	public function isReadOnly() {
+		return $this->isReadOnly;
+	}
+
 	/**
 	 * getType
 	 *
@@ -77,6 +86,6 @@ public function getType() {
 	 * @return bool
 	 */
 	public function isStatic() {
-		return $this->static;
+		return $this->isStatic;
 	}
 }

src/Abstractions/ReflectedClass.php

@@ -163,10 +163,10 @@ public function getProperty($name) {
 					$access = "public";
 				}
 				$type = Util::reflectionTypeToPhpParserType($prop->getType());
-				return new Property($prop->getName(), $type, $access, $modifiers & \ReflectionProperty::IS_STATIC );
+				return new Property($this, $prop->getName(), $type, $access, $modifiers & \ReflectionProperty::IS_STATIC, $modifiers & \ReflectionProperty::IS_READONLY );
 			}
 			return null;
-		} catch (\ReflectionException $exception) {
+		} catch (\ReflectionException) {
 			return null;
 		}
 	}
@@ -188,4 +188,12 @@ public function getPropertyNames() {
 	public function isEnum(): bool {
 		return $this->refl instanceof \ReflectionEnum;
 	}
+
+	public function isReadOnly(): bool {
+		/*
+		if (method_exists($this->refl,"isReadOnly")) {
+			return $this->refl->isReadOnly();
+		}*/
+		return false;
+	}
 }

src/Checks/CatchCheck.php

@@ -54,6 +54,10 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop
 					}
 				} else if (!$this->symbolTable->isDefinedClass($name)) {
 					$this->emitError($fileName, $node, ErrorConstants::TYPE_UNKNOWN_CLASS, "Attempt to catch unknown type: $name");
+				} else {
+					if (!$this->symbolTable->isParentClassOrInterface("throwable", $name)) {
+						$this->emitError($fileName, $node, ErrorConstants::TYPE_UNKNOWN_CLASS, "Attempt to catch exception $name that doesn't derive from \Throwable");
+					}
 				}
 			}
 		}

src/Checks/ErrorConstants.php

@@ -31,6 +31,7 @@ class ErrorConstants {
 	const TYPE_GLOBAL_EXPRESSION_ACCESSED = 'Standard.Global.Expression';
 	const TYPE_GLOBAL_STRING_ACCESSED = 'Standard.Global.String';
 	const TYPE_GOTO = 'Standard.Goto';
+	const TYPE_READONLY_DECLARATION = "Standard.Incorrect.ReadOnly";
 	const TYPE_ILLEGAL_ENUM = 'Standard.Illegal.Enum';
 	const TYPE_INCORRECT_DYNAMIC_CALL = 'Standard.Incorrect.Dynamic';
 	const TYPE_INCORRECT_STATIC_CALL = 'Standard.Incorrect.Static';

src/Checks/PropertyFetchCheck.php

@@ -65,12 +65,12 @@ public function run($fileName, Node $node, ClassLike $inside=null, Scope $scope=
 							// SimpleXMLElement has arbitrary properties based on the XML that was parsed.
 							return;
 						}
-						list($property, $declaredIn) = Util::findAbstractedProperty($typeStr, strval($node->name), $this->symbolTable);
+						$property= Util::findAbstractedProperty($typeStr, strval($node->name), $this->symbolTable);
 
 						if (!$property) {
 							$this->handleUndeclaredProperty($fileName, $node, $typeStr);
 						} else {
-							$this->handleDeclaredProperty($fileName, $node, $typeStr, $inside, $property, $declaredIn);
+							$this->handleDeclaredProperty($fileName, $node, $typeStr, $inside, $property, $property->getClass()->getName());
 						}
 					}
 				}

src/Checks/PropertyStoreCheck.php

@@ -59,6 +59,28 @@ public function run($fileName, Node $node, ClassLike $inside=null, Scope $scope=
 
 			$targetType = $node->var->getAttribute(TypeComparer::INFERRED_TYPE_ATTR);
 			$valueType = $node->expr->getAttribute(TypeComparer::INFERRED_TYPE_ATTR);
+			$nodeVarName=strval($node->var->name);
+
+			//echo "Checking safety of assigning ".TypeComparer::typeToString($valueType). " into a  ".TypeComparer::typeToString($targetType)."\n";
+
+			$insideConstructor=$this->insideConstructor($scope);
+
+			TypeComparer::forEachType($targetType, function($individualType) use ($nodeVarName, $fileName, $node, $insideConstructor) {
+				if ($individualType instanceof Node\Identifier || $individualType instanceof Node\Name) {
+					$typeStr = strval($individualType);
+					if ($this->symbolTable->isDefinedClass($typeStr)) {
+						$property = Util::findAbstractedProperty($typeStr, $nodeVarName, $this->symbolTable);
+						if ($property &&
+							!$insideConstructor &&
+							($property->isReadOnly() || $property->getClass()->isReadOnly())
+						) {
+							$this->emitError($fileName, $node, ErrorConstants::TYPE_ACCESS_VIOLATION, "Attempt to set read only variable " . $typeStr . "->" . $nodeVarName);
+						}
+					}
+				}
+			});
+
+
 			if (!$this->typeComparer->isCompatibleWithTarget($targetType, $valueType, $scope)) {
 				if($targetType instanceof Node\Identifier && util::isScalarType(strval($targetType))) {
 					$errorType = ErrorConstants::TYPE_ASSIGN_MISMATCH_SCALAR;
@@ -69,4 +91,17 @@ public function run($fileName, Node $node, ClassLike $inside=null, Scope $scope=
 			}
 		}
 	}
+
+	function insideConstructor(?Scope $scope) {
+		if (!$scope) {
+			return false;
+		}
+		foreach(array_reverse($scope->getParentNodes()) as $parentNode) {
+			if ($parentNode instanceof Node\FunctionLike || $parentNode instanceof Node\Stmt\Class_) {
+				return ($parentNode instanceof Node\Stmt\ClassMethod &&
+					strcasecmp($parentNode->name,"__construct")==0);
+			}
+		}
+		return false;
+	}
 }

src/Checks/ReadOnlyPropertyCheck.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace BambooHR\Guardrail\Checks;
+
+use BambooHR\Guardrail\Scope;
+use PhpParser\Node;
+use PhpParser\Node\Stmt\ClassLike;
+use PhpParser\Node\Stmt\PropertyProperty;
+
+class ReadOnlyPropertyCheck extends BaseCheck {
+	function getCheckNodeTypes() {
+		return [PropertyProperty::class];
+	}
+
+	public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) {
+		if ($node instanceof PropertyProperty) {
+			$parentNodes = $scope->getParentNodes();
+
+			/** @var Node\Stmt\Property $prop */
+			$prop = end($parentNodes);
+			$isReadOnlyClass = $inside instanceof Node\Stmt\Class_ && $inside->isReadonly();
+			if ($prop->isReadonly() || $isReadOnlyClass) {
+				if ($node->default !== null) {
+					$this->emitError($fileName, $node, ErrorConstants::TYPE_READONLY_DECLARATION, "Readonly properties can't have a default value");
+				}
+				if ($prop->type === null) {
+					$this->emitError($fileName, $node, ErrorConstants::TYPE_READONLY_DECLARATION, "Readonly properties must have a declared type");
+				}
+			}
+		}
+	}
+}

src/Checks/StaticPropertyFetchCheck.php

@@ -61,7 +61,7 @@ public function run($fileName, Node $node, ClassLike $inside=null, Scope $scope=
 			}
 
 			if ($class instanceof Name && is_string($node->name)) {
-				list($property,$declaredIn) = Util::findAbstractedProperty($class, $node->name, $this->symbolTable);
+				$property = Util::findAbstractedProperty($class, $node->name, $this->symbolTable);
 				if (!$property) {
 					$method = Util::findAbstractedMethod($class, $node->name, $this->symbolTable);
 					if ($method) {
@@ -77,9 +77,9 @@ public function run($fileName, Node $node, ClassLike $inside=null, Scope $scope=
 					if (!$property->isStatic()) {
 						$this->emitError($fileName, $node, ErrorConstants::TYPE_INCORRECT_STATIC_CALL, "Attempt to fetch a dynamic variable statically $class::" . $node->name);
 					}
-					if ($property->getAccess() == "private" && (!$inside || !isset($inside->namespacedName) || strcasecmp($inside->namespacedName, $declaredIn) != 0)) {
+					if ($property->getAccess() == "private" && (!$inside || !isset($inside->namespacedName) || strcasecmp($inside->namespacedName, $property->getClass()->getName()) != 0)) {
 						$this->emitError($fileName, $node, ErrorConstants::TYPE_ACCESS_VIOLATION, "Attempt to fetch private property " . $node->name);
-					} else if ($property->getAccess() == "protected" && (!$inside || !isset($inside->namespacedName) || !$this->symbolTable->isParentClassOrInterface($declaredIn, $inside->namespacedName))) {
+					} else if ($property->getAccess() == "protected" && (!$inside || !isset($inside->namespacedName) || !$this->symbolTable->isParentClassOrInterface($property->getClass()->getName(), $inside->namespacedName))) {
 						$this->emitError($fileName, $node, ErrorConstants::TYPE_ACCESS_VIOLATION, "Attempt to fetch protected property " . $node->name);
 					}
 				}

src/EnumCodeAugmenter.php

@@ -0,0 +1,29 @@
+<?php
+
+namespace BambooHR\Guardrail;
+
+use PhpParser\Node;
+use PhpParser\Builder\Property;
+use PhpParser\Builder\Param;
+
+class EnumCodeAugmenter {
+	static public function addEnumPropsAndMethods(Node\Stmt\Enum_ $enum) {
+		$isBacked = !is_null($enum->scalarType);
+		$property = new Property("name");
+		$property->setType(new Node\Identifier("string"));
+		$property->makeReadonly();
+		$enum->stmts[]= $property->getNode();
+		if ($isBacked) {
+			$enum->stmts[] = new Node\Stmt\ClassMethod("values",["returnType"=>"array"]);
+			$property = new Property("value");
+			$property->makeReadonly();
+			$property->setType( $enum->scalarType );
+			$enum->stmts[]=$property->getNode();
+
+			$enumName = $enum->namespacedName->toString();
+			$param = (new Param("fromValue"))->setType($enum->scalarType);
+			$enum->stmts[]=new Node\Stmt\ClassMethod("tryFrom",["returnType" => $enumName, "flags"=>Node\Stmt\Class_::MODIFIER_STATIC, 'params'=>[$param->getNode()]]);
+			$enum->stmts[]=new Node\Stmt\ClassMethod("from", ["returnType" => $enumName, "flags"=>Node\Stmt\Class_::MODIFIER_STATIC, 'params'=>[$param->getNode()]]);
+		}
+	}
+}

src/Evaluators/Expression/PropertyFetch.php

@@ -70,7 +70,7 @@ public function getProperty($class, Node\Identifier $name,SymbolTable $table) {
 				function ($class) use ($propName, &$types, &$unknown, $table) {
 					$classDef = $table->getAbstractedClass($class);
 					if ($classDef) {
-						list($prop) = Util::findAbstractedProperty($class, $propName, $table);
+						$prop= Util::findAbstractedProperty($class, $propName, $table);
 						if ($prop) {
 							$types[] = $prop->getType();
 						} else {

src/NodeVisitors/Grabber.php

@@ -5,6 +5,7 @@
  * Apache 2.0 License
  */
 
+use BambooHR\Guardrail\EnumCodeAugmenter;
 use PhpParser\Error;
 use PhpParser\Node;
 use PhpParser\Node\Stmt\Class_;
@@ -181,6 +182,7 @@ static public function getClassFromFile( SymbolTable $table, $fileName, $classNa
 					$traverser = new NodeTraverser;
 					$traverser->addVisitor(new TraitImportingVisitor($table));
 					$stmts = $traverser->traverse($stmts);
+
 				} catch (UnknownTraitException $exception) {
 					echo "[$className] Unknown trait! " . $exception->getMessage() . "\n";
 					// Ignore these for now.
@@ -192,7 +194,11 @@ static public function getClassFromFile( SymbolTable $table, $fileName, $classNa
 		}
 
 		if ($stmts) {
-			return self::getClassFromStmts($table, $stmts, $className, $classType);
+			$cls = self::getClassFromStmts($table, $stmts, $className, $classType);
+			if ($cls instanceof Node\Stmt\Enum_) {
+				EnumCodeAugmenter::addEnumPropsAndMethods($cls);
+			}
+			return $cls;
 		}
 		return null;
 	}

src/NodeVisitors/PromotedPropertyVisitor.php

@@ -30,7 +30,7 @@ public function enterNode(Node $node) {
 				$newStmts=[];
 				$newProps=[];
 				foreach ($constructor->getParams() as $param) {
-					if ($param->flags & (Class_::MODIFIER_PRIVATE | Class_::MODIFIER_PROTECTED | Class_::MODIFIER_PUBLIC)) {
+					if ($param->flags & (Class_::MODIFIER_READONLY | Class_::MODIFIER_PRIVATE | Class_::MODIFIER_PROTECTED | Class_::MODIFIER_PUBLIC)) {
 						$newProps[] = $this->buildProp($param);
 						$newStmts[] =  $this->buildAssign($param);
 					}
@@ -64,6 +64,9 @@ public function buildProp(Node\Param $param) {
 		if($param->flags & Class_::MODIFIER_PROTECTED) {
 			$prop->makeProtected();
 		}
+		if($param->flags & Class_::MODIFIER_READONLY) {
+			$prop->makeReadonly();
+		}
 		if($param->type) {
 			$prop->setType( $param->type );
 		}

src/NodeVisitors/StaticAnalyzer.php

@@ -28,6 +28,7 @@
 use BambooHR\Guardrail\Checks\PropertyFetchCheck;
 use BambooHR\Guardrail\Checks\PropertyStoreCheck;
 use BambooHR\Guardrail\Checks\Psr4Check;
+use BambooHR\Guardrail\Checks\ReadOnlyPropertyCheck;
 use BambooHR\Guardrail\Checks\ReturnCheck;
 use BambooHR\Guardrail\Checks\SplatCheck;
 use BambooHR\Guardrail\Checks\StaticCallCheck;
@@ -147,6 +148,7 @@ function __construct($basePath, $index, OutputInterface $output, $config)
 			new ImagickCheck($this->index, $output),
 			new UnsafeSuperGlobalCheck($this->index, $output),
 			new UseStatementCaseCheck($this->index, $output),
+			new ReadOnlyPropertyCheck($this->index, $output),
 			//new ClassStoredAsVariableCheck($this->index, $output)
 		];