From e76500278e26ab57ea8e7914995c814c869f461d Mon Sep 17 00:00:00 2001 From: bknutson Date: Wed, 15 Jan 2025 15:49:25 -0700 Subject: [PATCH 1/2] [SPN-1447] Start of Service Documentation Guardrail right now this just includes strong typing for params and returns --- src/Checks/ErrorConstants.php | 3 + .../ServiceMethodDocumentationCheck.php | 77 +++++++++++++++++++ src/NodeVisitors/StaticAnalyzer.php | 11 ++- .../TestServiceMethodDocumentationCheck.1.inc | 40 ++++++++++ .../TestServiceMethodDocumentationCheck.2.inc | 42 ++++++++++ .../TestServiceMethodDocumentationCheck.php | 26 +++++++ tests/units/Checks/TestTemplates.php | 8 +- 7 files changed, 202 insertions(+), 5 deletions(-) create mode 100644 src/Checks/ServiceMethodDocumentationCheck.php create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc create mode 100644 tests/units/Checks/TestServiceMethodDocumentationCheck.php diff --git a/src/Checks/ErrorConstants.php b/src/Checks/ErrorConstants.php index 080d2e3..3266740 100644 --- a/src/Checks/ErrorConstants.php +++ b/src/Checks/ErrorConstants.php @@ -40,6 +40,7 @@ class ErrorConstants { const TYPE_INCORRECT_REGEX = 'Standard.Incorrect.Regex'; const TYPE_METRICS_COMPLEXITY = 'Standard.Metrics.Complexity'; const TYPE_METRICS_LINES_OF_CODE = 'Standard.Metrics.Lines'; + const TYPE_METRICS_DEPRECATED_FUNCTIONS = 'Standard.Metrics.Deprecated'; const TYPE_MISSING_BREAK = 'Standard.Switch.Break'; const TYPE_MISSING_CONSTRUCT = 'Standard.Constructor.MissingCall'; const TYPE_NULL_DEREFERENCE = "Standard.Null.Dereference"; @@ -81,6 +82,8 @@ class ErrorConstants { const TYPE_VARIABLE_FUNCTION_NAME = 'Standard.VariableFunctionCall'; const TYPE_VARIABLE_VARIABLE = 'Standard.VariableVariable'; const TYPE_COUNTABLE_EMPTINESS_CHECK = 'Standard.Countable.Emptiness'; + const TYPE_WEB_API_DOCUMENTATION_CHECK = 'Standard.WebApi.Documentation'; + const TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK = 'Standard.ServiceMethod.Documentation'; /** diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php new file mode 100644 index 0000000..9f39cea --- /dev/null +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -0,0 +1,77 @@ +isPublic()) { + foreach ($node->getParams() as $param) { + if ($param->type instanceof Node\UnionType || $param->type instanceof Node\IntersectionType) { + foreach ($param->type->types as $type) { + $this->checkParamTyping($type, $node, $fileName, $inside); + } + } + else { + $this->checkParamTyping($param->type, $node, $fileName, $inside); + } + } + $returnType = $node->getReturnType(); + if ($returnType instanceof Node\UnionType || $returnType instanceof Node\IntersectionType) { + foreach ($returnType->types as $type) { + $this->checkReturnTyping($type->name ?? $type->toString(), $node, $fileName, $inside); + } + } else { + $this->checkReturnTyping($returnType, $node, $fileName, $inside); + } + } + } + + private function checkParamTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { + if (in_array($type?->name ?? $type?->toString() ?? null, [null, 'mixed', 'object'])) { + $this->emitErrorOnLine( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have strong types for all parameters. 'Mixed' and 'object' params are prohibited." + ); + } + } + + private function checkReturnTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { + if (in_array($type, [null, 'mixed', 'object'])) { + $this->emitErrorOnLine( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have a return type. 'Mixed' and 'object' return types are prohibited." + ); + } + } +} \ No newline at end of file diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index 0d69da2..d6da79d 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -33,6 +33,7 @@ use BambooHR\Guardrail\Checks\ReadOnlyPropertyCheck; use BambooHR\Guardrail\Checks\ReturnCheck; use BambooHR\Guardrail\Checks\DependenciesOnVendorCheck; +use BambooHR\Guardrail\Checks\ServiceMethodDocumentationCheck; use BambooHR\Guardrail\Checks\SplatCheck; use BambooHR\Guardrail\Checks\StaticCallCheck; use BambooHR\Guardrail\Checks\StaticPropertyFetchCheck; @@ -43,6 +44,7 @@ use BambooHR\Guardrail\Checks\UnsafeSuperGlobalCheck; use BambooHR\Guardrail\Checks\UnusedPrivateMemberVariableCheck; use BambooHR\Guardrail\Checks\UseStatementCaseCheck; +use BambooHR\Guardrail\Checks\WebApiDocumentationCheck; use BambooHR\Guardrail\Config; use BambooHR\Guardrail\Evaluators as Ev; use BambooHR\Guardrail\Evaluators\ExpressionInterface; @@ -114,9 +116,8 @@ function getTimingsAndCounts() function __construct(SymbolTable $index, OutputInterface $output, MetricOutputInterface $metricOutput, Config $config) { $this->index = $index; - $this->scopeStack = new ScopeStack($output, $config); + $this->scopeStack = new ScopeStack($output, $metricOutput, $config); $this->scopeStack->pushScope(new Scope(true, true, false)); - $this->metricOutput = $metricOutput; /** @var \BambooHR\Guardrail\Checks\BaseCheck[] $checkers */ $checkers = [ @@ -158,6 +159,8 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn new ThrowsCheck($this->index, $output), new CountableEmptinessCheck($this->index, $output), new DependenciesOnVendorCheck($this->index, $output, $metricOutput), + new WebApiDocumentationCheck($this->index, $output, $metricOutput), + new ServiceMethodDocumentationCheck($this->index, $output, $metricOutput), //new ClassStoredAsVariableCheck($this->index, $output) ]; @@ -216,7 +219,9 @@ public function getEvaluator(Node $node): ?Ev\EvaluatorInterface public function setFile($name, Config $config) { $this->file = $name; - $this->scopeStack = new ScopeStack($this->scopeStack->getOutput(), $config); + $this->scopeStack = new ScopeStack( + $this->scopeStack->getOutput(), $this->scopeStack->getMetricOutput(), $config + ); $this->scopeStack->pushScope(new Scope(true, true, false)); $this->scopeStack->setCurrentFile($name); } diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc new file mode 100644 index 0000000..a51e29a --- /dev/null +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc @@ -0,0 +1,40 @@ +assertEquals(5, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } + + /** + * testParametersSetAndUnset + * + * @return void + */ + public function testReturnTypesSetAndUnset() { + $this->assertEquals(4, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } +} \ No newline at end of file diff --git a/tests/units/Checks/TestTemplates.php b/tests/units/Checks/TestTemplates.php index c032037..8717b27 100644 --- a/tests/units/Checks/TestTemplates.php +++ b/tests/units/Checks/TestTemplates.php @@ -9,8 +9,12 @@ class TestTemplates extends TestSuiteSetup { function testValidTemplate() { - $this->assertEquals(0, $this->runAnalyzerOnFile('.1.inc',"Standard.*", ["ignore-errors"=>["Standard.Autoload.Unsafe"]])); - + $this->assertEquals(0, $this->runAnalyzerOnFile('.1.inc', "Standard.*", [ + "ignore-errors" => [ + "Standard.Autoload.Unsafe", ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK, + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + ] + ])); } From 34ba8b1f8fbcb49e3f166e02c7bd40d5f032ddbc Mon Sep 17 00:00:00 2001 From: bknutson Date: Thu, 16 Jan 2025 13:49:15 -0700 Subject: [PATCH 2/2] [SPN-1448] Refactor Service Method Documentation Check and add checks to ensure doc blocks match contract --- .../ServiceMethodDocumentationCheck.php | 175 ++++++++++++++---- src/NodeVisitors/StaticAnalyzer.php | 2 +- .../TestServiceMethodDocumentationCheck.1.inc | 32 +++- .../TestServiceMethodDocumentationCheck.2.inc | 25 ++- .../TestServiceMethodDocumentationCheck.3.inc | 53 ++++++ .../TestServiceMethodDocumentationCheck.4.inc | 62 +++++++ .../TestServiceMethodDocumentationCheck.php | 14 ++ 7 files changed, 321 insertions(+), 42 deletions(-) create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.4.inc diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php index 9f39cea..9de684d 100644 --- a/src/Checks/ServiceMethodDocumentationCheck.php +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -2,17 +2,13 @@ namespace BambooHR\Guardrail\Checks; -use BambooHR\Guardrail\Metrics\MetricOutputInterface; use BambooHR\Guardrail\Scope; use PhpParser\Node; use PhpParser\Node\Stmt\ClassLike; class ServiceMethodDocumentationCheck extends BaseCheck { - function __construct($index, $output, private readonly MetricOutputInterface $metricOutput) { - parent::__construct($index, $output); - } - + private const array BLOCKED_SERVICE_DOCUMENTATION_TYPES = [null, 'mixed', 'object']; /** * getCheckNodeTypes * @@ -32,46 +28,153 @@ function getCheckNodeTypes() { */ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { if ($node instanceof Node\Stmt\ClassMethod && $node->isPublic()) { - foreach ($node->getParams() as $param) { - if ($param->type instanceof Node\UnionType || $param->type instanceof Node\IntersectionType) { - foreach ($param->type->types as $type) { - $this->checkParamTyping($type, $node, $fileName, $inside); - } - } - else { - $this->checkParamTyping($param->type, $node, $fileName, $inside); - } - } - $returnType = $node->getReturnType(); - if ($returnType instanceof Node\UnionType || $returnType instanceof Node\IntersectionType) { - foreach ($returnType->types as $type) { - $this->checkReturnTyping($type->name ?? $type->toString(), $node, $fileName, $inside); - } - } else { - $this->checkReturnTyping($returnType, $node, $fileName, $inside); + $docComment = $node->getDocComment(); + if (empty($docComment)) { + $this->emitMissingDocBlockError($fileName, $node, $inside); + return; } + + $docCommentData = $this->extractDocCommentData($docComment); + $actualParams = array_map(fn($param) => $param->var, $node->getParams()); + $docCommentParams = $docCommentData['params']; + + $this->validateParameters($actualParams, $docCommentParams, $fileName, $node, $inside); + $this->validateReturnType($node, $docCommentData['return'], $fileName, $inside); } } - private function checkParamTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { - if (in_array($type?->name ?? $type?->toString() ?? null, [null, 'mixed', 'object'])) { - $this->emitErrorOnLine( - $fileName, - $node->getLine(), + private function emitMissingDocBlockError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { + $this->emitErrorOnLine($fileName, $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods must have a DocBlock." + ); + } + + private function validateParameters($actualParams, $docCommentParams, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { + if (count($docCommentParams) > count($actualParams)) { + $this->emitErrorOnLine($fileName, $node->getLine(), ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, - "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have strong types for all parameters. 'Mixed' and 'object' params are prohibited." + "Method: {$node->name->name}, Class: {$inside->name->name} - There are extra parameters in your DocBlock that are not present in the method signature." ); } + + foreach ($actualParams as $actualParam) { + $this->validateParameter($actualParam, $docCommentParams, $fileName, $node, $inside); + } } - private function checkReturnTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { - if (in_array($type, [null, 'mixed', 'object'])) { - $this->emitErrorOnLine( - $fileName, - $node->getLine(), - ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, - "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have a return type. 'Mixed' and 'object' return types are prohibited." - ); + private function validateParameter($actualParam, $docCommentParams, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { + $actualParamName = $actualParam->name ?? $actualParam->getString(); + $docCommentParam = $docCommentParams[$actualParamName] ?? null; + if (!$docCommentParam) { + $this->emitParameterMismatchError($fileName, $node, $inside, $actualParamName); + return; + } + + $actualParamAttribute = $actualParam->getAttribute('inferrer-type'); + if ($this->isComplexType($actualParamAttribute)) { + $this->validateComplexType($actualParamAttribute->types, $docCommentParam['type'], $fileName, $node, $inside, $actualParamName); + } else { + $actualParamType = $actualParamAttribute?->name ?? $actualParamAttribute?->toString(); + $this->validateSimpleType($actualParamType, $docCommentParam['type'], $fileName, $node, $inside, $actualParamName); } } + + private function isComplexType($type): bool { + return $type instanceof Node\UnionType || $type instanceof Node\IntersectionType; + } + + private function validateComplexType($actualParamTypes, $docCommentParamType, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $propertyName): void { + if (!is_array($docCommentParamType) || count($actualParamTypes) !== count($docCommentParamType)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + return; + } + + foreach ($actualParamTypes as $typeObject) { + $actualParamType = $typeObject->name ?? $typeObject->toString(); + if (!in_array($actualParamType, $docCommentParamType) || in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + break; + } + } + } + + private function validateSimpleType($actualParamType, $docCommentParamType, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $paramName): void { + if ($actualParamType !== $docCommentParamType || in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $paramName); + } + } + + private function emitParameterMismatchError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $paramName): void { + $this->emitErrorOnLine($fileName, $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - DocBlock does not contain matching parameter: $paramName" + ); + } + + private function emitTypeMismatchError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $propertyName): void { + $this->emitErrorOnLine($fileName, $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name}, Property: $propertyName - DocBlock does not match method signature." + ); + } + + /** + * @param Node\Stmt\ClassMethod $node + * @param $docCommentReturn + * @param string $fileName + * @param ClassLike $inside + * + * @return void + */ + private function validateReturnType(Node\Stmt\ClassMethod $node, $docCommentReturn, string $fileName, Node\Stmt\ClassLike $inside): void { + $propertyName = 'return'; + if (!$docCommentReturn) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + return; + } + + $actualReturn = $node->getReturnType(); + if ($this->isComplexType($actualReturn)) { + $this->validateComplexType($actualReturn->types, $docCommentReturn, $fileName, $node, $inside, $propertyName); + } else { + $this->validateSimpleType($actualReturn?->name ?? $actualReturn?->toString() ?? null, $docCommentReturn, $fileName, $node, $inside, $propertyName); + } + } + + /** + * @param string|null $docComment + * + * @return array + */ + private function extractDocCommentData(?string $docComment) { + $result = [ + 'params' => [], + 'return' => null, + ]; + if (preg_match_all('/@param\s+([^\s$]+(?:\s*[\|&]\s*[^\s$]+)*)?\s*(\$[^\s]+)?/', $docComment, $paramMatches, PREG_SET_ORDER)) { + foreach ($paramMatches as $paramMatch) { + $result['params'][$variableName] = [ + 'type' => $this->extractDocGetVariableType($paramMatch[1] ?? null), + 'variable' => $variableName = ltrim($paramMatch[2] ?? null, '$'), + ]; + } + } + + if (preg_match('/@return\s+([^\s$]+(?:\s*\|\s*[^\s$]+|&[^\s$]+)*)/', $docComment, $returnMatch)) { + $result['return'] = $this->extractDocGetVariableType($returnMatch[1] ?? null); + } + + return $result; + } + + private function extractDocGetVariableType($variableType) { + if (str_contains($variableType, '|')) { + $variableType = array_map('trim', explode('|', $variableType)); + } else if (str_contains($variableType, '&')) { + $variableType = array_map('trim', explode('&', $variableType)); + } + + return $variableType; + } } \ No newline at end of file diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index d6da79d..2df9279 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -160,7 +160,7 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn new CountableEmptinessCheck($this->index, $output), new DependenciesOnVendorCheck($this->index, $output, $metricOutput), new WebApiDocumentationCheck($this->index, $output, $metricOutput), - new ServiceMethodDocumentationCheck($this->index, $output, $metricOutput), + new ServiceMethodDocumentationCheck($this->index, $output), //new ClassStoredAsVariableCheck($this->index, $output) ]; diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc index a51e29a..6d8b51b 100644 --- a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc @@ -3,8 +3,8 @@ class MyService { // This method should emit 2 errors /** - * @param string $a - * @param int $b + * @param int $a + * @param string $b * * @return int */ @@ -13,21 +13,49 @@ class MyService { } // This method should emit 2 errors + + /** + * @param mixed $a + * @param object $b + * + * @return int + */ public function specialParamTypes(mixed $a, object $b): int { return 123; } // This method should emit 1 error + + /** + * @param int|string|object $a + * @param int|float $b + * + * @return int + */ public function unionParamTypes(int | string | object $a, int | float $b): int { return 123; } // This method should emit 0 errors + + /** + * @param MyObj2&MyObj $a + * + * @return int + */ public function intersectionParamTypes(MyObj&MyObj2 $a): int { return 123; } // This method should emit 0 errors + + /** + * @param bool $a + * @param int $b + * @param string $c + * + * @return int + */ public function withNormalTypedParams(bool $a, int $b, string $c): int { return 123; } diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc index e7f5122..ebb7847 100644 --- a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc @@ -10,28 +10,47 @@ class MyService { } // This method should emit 1 error + + /** + * @return mixed + */ public function specialReturnType(): mixed { return 123; } // This method should emit 1 error + + /** + * @return object + */ public function specialReturnType2(): object { return 123; } // This method should emit 1 error - public function unionParamTypes(): int|string|object { + + /** + * @return int|string|object + */ + public function unionParamTypes(): int | string | object { return 123; } // This method should emit 0 errors + + /** + * @return MyObj2&MyObj + */ public function intersectionParamTypes(): MyObj&MyObj2 { - return 123; } // This method should emit 0 errors + + /** + * @return bool + */ public function withNormalTypedParams(): bool { - return 123; + return true; } } diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc new file mode 100644 index 0000000..c3661bd --- /dev/null +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc @@ -0,0 +1,53 @@ +assertEquals(4, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); } + + /** + * @return void + */ + public function testSimpleTypesMatchingAndUnMatchingDocBlocks() { + $this->assertEquals(7, $this->runAnalyzerOnFile('.3.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } + + /** + * @return void + */ + public function testComplexTypesMatchingAndUnMatchingDocBlocks() { + $this->assertEquals(13, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } } \ No newline at end of file