Repository: spaze/phpstan-disallowed-calls (php)

Issue:
disallowedAttributes together with allowInMethodsWithAttributes does not work as expected
Consider the following config:
```php
<?php

declare(strict_types=1);

use TicketSwap\Shared\Infrastructure\Messaging\MessageBus\Attribute\AsCommandHandler;

return [
    'parameters' => [
        'disallowedAttributes' => [
            [
                'attribute' => TicketSwap\Shared\Infrastructure\Messaging\MessageBus\Attribute\IgnoreException::class,
                'errorTip' => sprintf(
                    '%s only works inside a command handler. Catch the exception and return instead.',
                    TicketSwap\Shared\Infrastructure\Messaging\MessageBus\Attribute\IgnoreException::class,
                ),
                'allowInMethodsWithAttributes' => [
                    AsCommandHandler::class,
                ],
            ],
        ],
    ],
];

```

The following code produces an error:
```php
<?php

declare(strict_types=1);

namespace App\Handler;

use TicketSwap\Shared\Infrastructure\Messaging\MessageBus\Attribute\AsCommandHandler;
use TicketSwap\Shared\Infrastructure\Messaging\MessageBus\Attribute\IgnoreException;

final readonly class SomeHandler
{
    #[AsCommandHandler]
    #[IgnoreException(SomeException::class)]
    public function __invoke(SomeCommand $command, Actor $actor, EventRecorder $eventRecorder) : void
    {
    }
}
```

> Class EventRecorder is forbidden

Pull request description:
Check also attributes on methods when checking whether an attribute should be allowed in methods
A disallowed attribute used on a method (or on a function) is technically not in the method, so `$scope->getFunction()` returns null and thus no attributes could be returned. Now the original node is passed around and used to get the reflection to get the attributes.

Close #303

Patch:
diff --git a/docs/allow-in-methods-with-attributes.md b/docs/allow-in-methods-with-attributes.md
index 0d449db..5740a68 100644
--- a/docs/allow-in-methods-with-attributes.md
+++ b/docs/allow-in-methods-with-attributes.md
@@ -25,3 +25,32 @@ parameters:
 ```
 
 The attribute names support [fnmatch()](https://www.php.net/function.fnmatch) patterns. If you specify multiple attributes, the method or the function in which the item should be allowed or disallowed, needs to have just one of them.
+
+### Attributes on methods
+
+In case of disallowing attributes and then re-allowing them in methods with attributes, the disallowed attributes can be both _inside_ the method, and _on_ the method.
+This means that the following code with the following configuration is also valid, even though `Attribute1` is technically not used _inside_ the method.
+
+```php
+class Foo
+{
+
+    #[Attribute1]
+    #[Attribute2]
+    public function bar()
+    {
+    }
+
+}
+```
+
+```neon
+parameters:
+    disallowedAttributes:
+        -
+            attribute: Attribute1
+            allowInMethodsWithAttributes:
+                - Attribute2
+```
+
+No error would be reported for `Attribute1` as it is used "in" a method with `Attribute2`.
diff --git a/src/Allowed/Allowed.php b/src/Allowed/Allowed.php
index fdcffdc..3f1c245 100644
--- a/src/Allowed/Allowed.php
+++ b/src/Allowed/Allowed.php
@@ -3,7 +3,10 @@ declare(strict_types = 1);
 
 namespace Spaze\PHPStan\Rules\Disallowed\Allowed;
 
+use PhpParser\Node;
 use PhpParser\Node\Arg;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\Node\Stmt\Function_;
 use PHPStan\Analyser\Scope;
 use PHPStan\BetterReflection\Reflection\Adapter\FakeReflectionAttribute;
 use PHPStan\BetterReflection\Reflection\Adapter\ReflectionAttribute;
@@ -46,12 +49,13 @@ class Allowed
 
 
 	/**
+	 * @param Node|null $node
 	 * @param Scope $scope
 	 * @param array<Arg>|null $args
 	 * @param Disallowed|DisallowedWithParams $disallowed
 	 * @return bool
 	 */
-	public function isAllowed(Scope $scope, ?array $args, Disallowed $disallowed): bool
+	public function isAllowed(?Node $node, Scope $scope, ?array $args, Disallowed $disallowed): bool
 	{
 		$hasParams = $disallowed instanceof DisallowedWithParams;
 		foreach ($disallowed->getAllowInCalls() as $call) {
@@ -90,10 +94,10 @@ class Allowed
 			return !$this->hasAllowedAttribute($this->getAttributes($scope), $disallowed->getAllowExceptInClassWithAttributes());
 		}
 		if ($disallowed->getAllowInCallsWithAttributes()) {
-			return $this->hasAllowedAttribute($this->getCallAttributes($scope), $disallowed->getAllowInCallsWithAttributes());
+			return $this->hasAllowedAttribute($this->getCallAttributes($node, $scope), $disallowed->getAllowInCallsWithAttributes());
 		}
 		if ($disallowed->getAllowExceptInCallsWithAttributes()) {
-			return !$this->hasAllowedAttribute($this->getCallAttributes($scope), $disallowed->getAllowExceptInCallsWithAttributes());
+			return !$this->hasAllowedAttribute($this->getCallAttributes($node, $scope), $disallowed->getAllowExceptInCallsWithAttributes());
 		}
 		if ($disallowed->getAllowInClassWithMethodAttributes()) {
 			return $this->hasAllowedAttribute($this->getAllMethodAttributes($scope), $disallowed->getAllowInClassWithMethodAttributes());
@@ -226,19 +230,25 @@ class Allowed
 
 
 	/**
+	 * @param Node|null $node
 	 * @param Scope $scope
 	 * @return list<FakeReflectionAttribute|ReflectionAttribute|BetterReflectionAttribute>
 	 */
-	private function getCallAttributes(Scope $scope): array
+	private function getCallAttributes(?Node $node, Scope $scope): array
 	{
 		$function = $scope->getFunction();
 		if ($function instanceof MethodReflection) {
 			return $scope->isInClass() ? $scope->getClassReflection()->getNativeReflection()->getMethod($function->getName())->getAttributes() : [];
 		} elseif ($function instanceof FunctionReflection) {
 			return $this->reflector->reflectFunction($function->getName())->getAttributes();
-		} else {
-			return [];
+		} elseif ($function === null) {
+			if ($node instanceof ClassMethod && $scope->isInClass()) {
+				return $scope->getClassReflection()->getNativeReflection()->getMethod($node->name->name)->getAttributes();
+			} elseif ($node instanceof Function_) {
+				return $this->reflector->reflectFunction($node->name->name)->getAttributes();
+			}
 		}
+		return [];
 	}
 
 
diff --git a/src/RuleErrors/DisallowedAttributeRuleErrors.php b/src/RuleErrors/DisallowedAttributeRuleErrors.php
index fb53eeb..50ac676 100644
--- a/src/RuleErrors/DisallowedAttributeRuleErrors.php
+++ b/src/RuleErrors/DisallowedAttributeRuleErrors.php
@@ -3,6 +3,7 @@ declare(strict_types = 1);
 
 namespace Spaze\PHPStan\Rules\Disallowed\RuleErrors;
 
+use PhpParser\Node;
 use PhpParser\Node\Attribute;
 use PHPStan\Analyser\Scope;
 use PHPStan\Rules\IdentifierRuleError;
@@ -31,19 +32,19 @@ class DisallowedAttributeRuleErrors
 
 
 	/**
-	 * @param Attribute $attribute
+	 * @param Node $node
 	 * @param Scope $scope
 	 * @param list<DisallowedAttribute> $disallowedAttributes
 	 * @return list<IdentifierRuleError>
 	 */
-	public function get(Attribute $attribute, Scope $scope, array $disallowedAttributes): array
+	public function get(Node $node, Attribute $attribute, Scope $scope, array $disallowedAttributes): array
 	{
 		foreach ($disallowedAttributes as $disallowedAttribute) {
 			$attributeName = $attribute->name->toString();
 			if (!$this->identifier->matches($disallowedAttribute->getAttribute(), $attributeName, $disallowedAttribute->getExcludes())) {
 				continue;
 			}
-			if ($this->allowed->isAllowed($scope, $attribute->args, $disallowedAttribute)) {
+			if ($this->allowed->isAllowed($node, $scope, $attribute->args, $disallowedAttribute)) {
 				continue;
 			}
 
diff --git a/src/RuleErrors/DisallowedCallsRuleErrors.php b/src/RuleErrors/DisallowedCallsRuleErrors.php
index 5c73c15..f7de554 100644
--- a/src/RuleErrors/DisallowedCallsRuleErrors.php
+++ b/src/RuleErrors/DisallowedCallsRuleErrors.php
@@ -53,7 +53,7 @@ class DisallowedCallsRuleErrors
 			if (
 				$this->identifier->matches($disallowedCall->getCall(), $name, $disallowedCall->getExcludes())
 				&& $this->definedInMatches($disallowedCall, $definedIn)
-				&& !$this->allowed->isAllowed($scope, isset($node) ? $node->getArgs() : null, $disallowedCall)
+				&& !$this->allowed->isAllowed($node, $scope, isset($node) ? $node->getArgs() : null, $disallowedCall)
 			) {
 				$errorBuilder = RuleErrorBuilder::message(sprintf(
 					$message ?? 'Calling %s is forbidden%s%s',
diff --git a/src/RuleErrors/DisallowedNamespaceRuleErrors.php b/src/RuleErrors/DisallowedNamespaceRuleErrors.php
index 901ae68..073a308 100644
--- a/src/RuleErrors/DisallowedNamespaceRuleErrors.php
+++ b/src/RuleErrors/DisallowedNamespaceRuleErrors.php
@@ -3,6 +3,7 @@ declare(strict_types = 1);
 
 namespace Spaze\PHPStan\Rules\Disallowed\RuleErrors;
 
+use PhpParser\Node;
 use PHPStan\Analyser\Scope;
 use PHPStan\Rules\IdentifierRuleError;
 use PHPStan\Rules\RuleErrorBuilder;
@@ -31,6 +32,7 @@ class DisallowedNamespaceRuleErrors
 
 
 	/**
+	 * @param Node $node
 	 * @param NamespaceUsage $namespaceUsage
 	 * @param string $description
 	 * @param Scope $scope
@@ -38,12 +40,12 @@ class DisallowedNamespaceRuleErrors
 	 * @param string $identifier
 	 * @return list<IdentifierRuleError>
 	 */
-	public function getDisallowedMessage(NamespaceUsage $namespaceUsage, string $description, Scope $scope, array $disallowedNamespaces, string $identifier): array
+	public function getDisallowedMessage(Node $node, NamespaceUsage $namespaceUsage, string $description, Scope $scope, array $disallowedNamespaces, string $identifier): array
 	{
 		foreach ($disallowedNamespaces as $disallowedNamespace) {
 			if (
 				!$this->identifier->matches($disallowedNamespace->getNamespace(), $namespaceUsage->getNamespace(), $disallowedNamespace->getExcludes())
-				|| $this->allowed->isAllowed($scope, null, $disallowedNamespace)
+				|| $this->allowed->isAllowed($node, $scope, null, $disallowedNamespace)
 				|| ($disallowedNamespace->isAllowInUse() && $namespaceUsage->isUseItem())
 			) {
 				continue;
diff --git a/src/Usages/AttributeUsages.php b/src/Usages/AttributeUsages.php
index 7773c1f..4029512 100644
--- a/src/Usages/AttributeUsages.php
+++ b/src/Usages/AttributeUsages.php
@@ -89,7 +89,7 @@ class AttributeUsages implements Rule
 
 		$errors = [];
 		foreach ($this->attributes as $attribute) {
-			$ruleErrors = $this->disallowedAttributeRuleErrors->get($attribute, $scope, $this->disallowedAttributes);
+			$ruleErrors = $this->disallowedAttributeRuleErrors->get($node, $attribute, $scope, $this->disallowedAttributes);
 			if ($ruleErrors) {
 				$errors = array_merge($errors, $ruleErrors);
 			}
diff --git a/src/Usages/NamespaceUsages.php b/src/Usages/NamespaceUsages.php
index eed14bd..f736fc8 100644
--- a/src/Usages/NamespaceUsages.php
+++ b/src/Usages/NamespaceUsages.php
@@ -122,6 +122,7 @@ class NamespaceUsages implements Rule
 		$errors = [];
 		foreach ($namespaces as $namespaceUsage) {
 			$ruleErrors = $this->disallowedNamespaceRuleErrors->getDisallowedMessage(
+				$node,
 				$namespaceUsage,
 				$description ?? 'Namespace',
 				$scope,


Test patch:
diff --git a/tests/Usages/AttributeUsagesAllowInClassWithAttributesTest.php b/tests/Usages/AttributeUsagesAllowInClassWithAttributesTest.php
index fb0177e..3c37f03 100644
--- a/tests/Usages/AttributeUsagesAllowInClassWithAttributesTest.php
+++ b/tests/Usages/AttributeUsagesAllowInClassWithAttributesTest.php
@@ -94,6 +94,10 @@ class AttributeUsagesAllowInClassWithAttributesTest extends RuleTestCase
 				'Attribute Attribute10 is forbidden.',
 				123,
 			],
+			[
+				'Attribute Attribute13 is forbidden.',
+				150,
+			],
 			[
 				'Attribute Attribute12 is forbidden.',
 				157,
@@ -106,6 +110,17 @@ class AttributeUsagesAllowInClassWithAttributesTest extends RuleTestCase
 	}
 
 
+	public function testRuleInFunctions(): void
+	{
+		$this->analyse([__DIR__ . '/../src/AttributeFunctions.php'], [
+			[
+				'Attribute Attribute13 is forbidden.',
+				26,
+			],
+		]);
+	}
+
+
 	public static function getAdditionalConfigFiles(): array
 	{
 		return [
diff --git a/tests/src/AttributeClass.php b/tests/src/AttributeClass.php
index bdc7eba..8025e4e 100644
--- a/tests/src/AttributeClass.php
+++ b/tests/src/AttributeClass.php
@@ -133,7 +133,7 @@ use PhpOption\Some;
 class CallsWithAttributes
 {
 
-	#[\Attribute10]
+	#[\Attribute10, \Attribute12]
 	public function method1(): void
 	{
 		strtolower('');
@@ -147,7 +147,7 @@ class CallsWithAttributes
 	}
 
 
-	#[\Attribute11]
+	#[\Attribute11, \Attribute13]
 	public function method2(): void
 	{
 		strtolower('');
diff --git a/tests/src/AttributeFunctions.php b/tests/src/AttributeFunctions.php
index 32f0329..4f448aa 100644
--- a/tests/src/AttributeFunctions.php
+++ b/tests/src/AttributeFunctions.php
@@ -15,3 +15,15 @@ function withAttribute11(): void
 	strtolower('');
 	strtoupper('');
 }
+
+
+#[\Attribute10, \Attribute12]
+function withAttribute10And12(): void
+{
+}
+
+
+#[\Attribute11, \Attribute13]
+function withAttribute11And13(): void
+{
+}