Rule to enforce that certain controller methods must "used"

.github/workflows/ci.yml

@@ -19,7 +19,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        php-version: ['8.1', '8.2', '8.3']
+        php-version: ['8.1', '8.4']
         dependencies: ['highest']
         include:
           - php-version: '8.1'
@@ -68,7 +68,7 @@ jobs:
         uses: ramsey/composer-install@v2
 
       - name: Run phpcs
-        run: vendor/bin/phpcs --report=checkstyle src/ tests/ | cs2pr
+        run: vendor/bin/phpcs --report=checkstyle | cs2pr
 
       - name: Run phpstan
         if: always()

README.md

@@ -148,13 +148,48 @@ This rule check if the options (args) passed to Table::find and SelectQuery are
 ### TableGetMatchOptionsTypesRule
 This rule check if the options (args) passed to Table::get are valid find options types.
 
-To enable this rule update your phpstan.neon with:
+### ControllerMethodMustBeUsedRule
+This rule enforces that controller methods like `render()` and `redirect()` must be used (returned or assigned) to prevent unreachable code. These methods should not be called in void context - use them with a `return` statement or assign them to a variable to make the control flow explicit.
 
+<details>
+      <summary>Examples:</summary>
+
+```php
+// Bad - code after render() is unreachable
+public function myAction()
+{
+    $this->render('edit');
+    $this->set('data', 'value'); // This will never execute
+}
+
+// Good - explicit return prevents confusion
+public function myAction()
+{
+    return $this->render('edit');
+}
+
+// Also good - assignment is valid
+public function myAction()
+{
+    $response = $this->render('edit');
+
+    return $response;
+}
+
+// Bad - code after redirect() is unreachable
+public function myAction()
+{
+    $this->redirect(['action' => 'index']);
+    $this->Flash->success('Done'); // This will never execute
+}
+
+// Good - explicit return prevents confusion
+public function myAction()
+{
+    return $this->redirect(['action' => 'index']);
+}
 ```
-parameters:
-	cakeDC:
-	 	disallowEntityArrayAccessRule: true
-```
+</details>
 
 ### How to disable a rule
 Each rule has a parameter in cakeDC 'namespace' to enable or disable, it is the same name of the

composer.json

@@ -12,12 +12,12 @@
     ],
     "require": {
         "php": ">=8.1.0",
-        "phpstan/phpstan": "^2.0",
+        "phpstan/phpstan": "^2.1.26",
         "cakephp/cakephp": "^5.0"
     },
     "require-dev": {
         "phpstan/phpstan-phpunit": "^2.0",
-        "phpunit/phpunit": "^10.1",
+        "phpunit/phpunit": "^10.5 || ^11.5 || ^12.1",
         "cakephp/cakephp-codesniffer": "^5.0",
         "phpstan/phpstan-deprecation-rules": "^2.0",
         "phpstan/phpstan-strict-rules": "^2.0"
@@ -42,8 +42,8 @@
         }
     },
     "scripts": {
-        "cs-check": "phpcs -p src/ tests",
-        "cs-fix": "phpcbf -p src/ tests",
+        "cs-check": "phpcs -p",
+        "cs-fix": "phpcbf -p",
         "test": "phpunit --stderr",
         "stan-integration": [
             "phpstan analyse --debug -c phpstan-test-app.neon",

phpcs.xml

@@ -1,8 +1,11 @@
 <?xml version="1.0"?>
-<ruleset name="CakePHP Core">
-  <config name="installed_paths" value="../../cakephp/cakephp-codesniffer" />
+<ruleset name="cakephp-phpstan">
+    <rule ref="CakePHP"/>
 
-  <rule ref="CakePHP" />
+    <arg value="s"/>
 
-  <arg value="s"/>
-</ruleset>
+    <file>src/</file>
+    <file>tests/</file>
+
+    <exclude-pattern>/tests/data/</exclude-pattern>
+</ruleset>

rules.neon

@@ -8,6 +8,7 @@ parameters:
 	 	disallowEntityArrayAccessRule: false
 	 	getMailerExistsClassRule: true
 	 	loadComponentExistsClassRule: true
+	 	controllerMethodMustBeUsedRule: true
 parametersSchema:
 	cakeDC: structure([
 		addAssociationExistsTableClassRule: anyOf(bool(), arrayOf(bool()))
@@ -18,13 +19,16 @@ parametersSchema:
 		disallowEntityArrayAccessRule: anyOf(bool(), arrayOf(bool()))
 		getMailerExistsClassRule: anyOf(bool(), arrayOf(bool()))
 		loadComponentExistsClassRule: anyOf(bool(), arrayOf(bool()))
+		controllerMethodMustBeUsedRule: anyOf(bool(), arrayOf(bool()))
 	])
 
 conditionalTags:
 	CakeDC\PHPStan\Visitor\AddAssociationSetClassNameVisitor:
 		phpstan.parser.richParserNodeVisitor: %cakeDC.addAssociationExistsTableClassRule%
 	CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule:
 		phpstan.rules.rule: %cakeDC.loadComponentExistsClassRule%
+	CakeDC\PHPStan\Rule\Controller\ControllerMethodMustBeUsedRule:
+		phpstan.rules.rule: %cakeDC.controllerMethodMustBeUsedRule%
 	CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule:
 		phpstan.rules.rule: %cakeDC.addAssociationExistsTableClassRule%
 	CakeDC\PHPStan\Rule\Model\AddAssociationMatchOptionsTypesRule:
@@ -45,6 +49,8 @@ services:
 		class: CakeDC\PHPStan\Visitor\AddAssociationSetClassNameVisitor
 	-
 		class: CakeDC\PHPStan\Rule\Controller\LoadComponentExistsClassRule
+	-
+		class: CakeDC\PHPStan\Rule\Controller\ControllerMethodMustBeUsedRule
 	-
 		class: CakeDC\PHPStan\Rule\Model\AddAssociationExistsTableClassRule
 	-

src/Method/AssociationTableMixinClassReflectionExtension.php

@@ -49,11 +49,11 @@ protected function getTableReflection(): ClassReflection
     public function hasMethod(ClassReflection $classReflection, string $methodName): bool
     {
         // magic findBy* method
-        if ($classReflection->isSubclassOf(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
+        if ($classReflection->is(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
             return true;
         }
 
-        if (!$classReflection->isSubclassOf(Association::class)) {
+        if (!$classReflection->is(Association::class)) {
             return false;
         }
 
@@ -68,7 +68,7 @@ public function hasMethod(ClassReflection $classReflection, string $methodName):
     public function getMethod(ClassReflection $classReflection, string $methodName): MethodReflection
     {
         // magic findBy* method
-        if ($classReflection->isSubclassOf(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
+        if ($classReflection->is(Table::class) && preg_match('/^find(?:\w+)?By/', $methodName) > 0) {
             return new TableFindByPropertyMethodReflection($methodName, $classReflection);
         }
 
@@ -82,11 +82,11 @@ public function getMethod(ClassReflection $classReflection, string $methodName):
      */
     public function hasProperty(ClassReflection $classReflection, string $propertyName): bool
     {
-        if (!$classReflection->isSubclassOf(Association::class)) {
+        if (!$classReflection->is(Association::class)) {
             return false;
         }
 
-        return $this->getTableReflection()->hasProperty($propertyName);
+        return $this->getTableReflection()->hasInstanceProperty($propertyName);
     }
 
     /**

src/Method/DummyParameter.php

@@ -48,7 +48,7 @@ public function __construct(
         bool $optional,
         ?PassedByReference $passedByReference,
         bool $variadic,
-        ?Type $defaultValue
+        ?Type $defaultValue,
     ) {
         $this->name = $name;
         $this->type = $type;

src/Method/TableFindByPropertyMethodReflection.php

@@ -45,13 +45,13 @@ public function __construct(string $name, ClassReflection $declaringClass)
         $this->name = $name;
 
         $this->declaringClass = $declaringClass;
-        $params = array_map(fn ($field) => new DummyParameter(
+        $params = array_map(fn($field) => new DummyParameter(
             $field,
             new MixedType(),
             false,
             null,
             false,
-            null
+            null,
         ), $this->getParams($name));
 
         $returnType = new ObjectType(SelectQuery::class);
@@ -62,7 +62,7 @@ public function __construct(string $name, ClassReflection $declaringClass)
                 null,
                 $params,
                 false,
-                $returnType
+                $returnType,
             ),
         ];
     }

src/PhpDoc/TableAssociationTypeNodeResolverExtension.php

@@ -80,7 +80,7 @@ public function resolve(TypeNode $typeNode, NameScope $nameScope): ?Type
         if ($config['table'] !== null && $config['association'] !== null) {
             return new GenericObjectType(
                 $config['association']->getObjectClassNames()[0],
-                [$config['table']]
+                [$config['table']],
             );
         }
 

src/Rule/Controller/ControllerMethodMustBeUsedRule.php

@@ -0,0 +1,107 @@
+<?php
+declare(strict_types=1);
+
+/**
+ * Copyright 2024, Cake Development Corporation (https://www.cakedc.com)
+ *
+ * Licensed under The MIT License
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright Copyright 2024, Cake Development Corporation (https://www.cakedc.com)
+ * @license   MIT License (http://www.opensource.org/licenses/mit-license.php)
+ */
+
+namespace CakeDC\PHPStan\Rule\Controller;
+
+use Cake\Controller\Controller;
+use PhpParser\Node;
+use PhpParser\Node\Expr\MethodCall;
+use PhpParser\Node\Stmt\Expression;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+
+/**
+ * Rule to enforce that certain controller methods must be used
+ * (returned or assigned) to prevent unreachable code.
+ */
+class ControllerMethodMustBeUsedRule implements Rule
+{
+    /**
+     * Methods that must be used (returned or assigned)
+     *
+     * @var array<string>
+     */
+    protected array $methodsRequiringUsage = [
+        'render',
+        'redirect',
+    ];
+
+    /**
+     * @inheritDoc
+     */
+    public function getNodeType(): string
+    {
+        return Expression::class;
+    }
+
+    /**
+     * @param \PhpParser\Node $node
+     * @param \PHPStan\Analyser\Scope $scope
+     * @return list<\PHPStan\Rules\IdentifierRuleError>
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        assert($node instanceof Expression);
+
+        // Check if this is a method call
+        if (!$node->expr instanceof MethodCall) {
+            return [];
+        }
+
+        $methodCall = $node->expr;
+
+        // Check if the method name is one we care about
+        if (!$methodCall->name instanceof Node\Identifier) {
+            return [];
+        }
+
+        $methodName = $methodCall->name->toString();
+        if (!in_array($methodName, $this->methodsRequiringUsage, true)) {
+            return [];
+        }
+
+        // Check if the method is being called on $this
+        $callerType = $scope->getType($methodCall->var);
+        if (!$callerType->isObject()->yes()) {
+            return [];
+        }
+
+        // Check if it's a Controller class
+        $classReflections = $callerType->getObjectClassReflections();
+        $isController = false;
+        foreach ($classReflections as $classReflection) {
+            if ($classReflection->is(Controller::class)) {
+                $isController = true;
+                break;
+            }
+        }
+
+        if (!$isController) {
+            return [];
+        }
+
+        // If we reach here, it means the method call is wrapped in an Expression node
+        // which means it's used as a statement (not returned or assigned)
+        return [
+            RuleErrorBuilder::message(sprintf(
+                'Method `%s()` must be used to prevent unreachable code. ' .
+                'Use `return $this->%s()` or assign it to a variable.',
+                $methodName,
+                $methodName,
+            ))
+            ->identifier('cake.controller.' . $methodName . 'MustBeUsed')
+            ->build(),
+        ];
+    }
+}

src/Rule/LoadObjectExistsCakeClassRule.php

@@ -71,7 +71,7 @@ public function processNode(Node $node, Scope $scope): array
         if ($inputClassName === null) {
             $inputClassName = $this->getInputClassName(
                 $details['alias']->value,
-                $details['options']
+                $details['options'],
             );
         }
         if ($inputClassName === null || $this->getTargetClassName($inputClassName) !== null) {