Fix: Add a check for usage of the empty() construct (#188)

Unleash · Nov 27, 2023 · 62eeea7 · 62eeea7
1 parent 6ac1699
commit 62eeea7
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 4 deletions.
diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php
@@ -3,6 +3,7 @@
 $finder = (new PhpCsFixer\Finder())
     ->in(__DIR__ . '/src')
     ->in(__DIR__ . '/tests')
+    ->in(__DIR__ . '/phpstan')
 ;
 
 return (new PhpCsFixer\Config())

diff --git a/composer.json b/composer.json
@@ -25,7 +25,8 @@
     },
     "autoload": {
         "psr-4": {
-            "Unleash\\Client\\": "src/"
+            "Unleash\\Client\\": "src/",
+            "Unleash\\Client\\PhpstanRules\\": "phpstan/"
         }
     },
     "require-dev": {

diff --git a/phpstan.neon.dist b/phpstan.neon.dist
@@ -10,3 +10,5 @@ parameters:
       path: src/Helper/DefaultImplementationLocator.php
       reportUnmatched: false
   treatPhpDocTypesAsCertain: false
+rules:
+  - Unleash\Client\PhpstanRules\NoEmptyFunctionRule
diff --git a/phpstan/NoEmptyFunctionRule.php b/phpstan/NoEmptyFunctionRule.php
@@ -0,0 +1,56 @@
+<?php
+
+namespace Unleash\Client\PhpstanRules;
+
+use PhpParser\Node;
+use PHPStan\Analyser\Scope;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+use PHPStan\Type\ResourceType;
+
+final readonly class NoEmptyFunctionRule implements Rule
+{
+    public function getNodeType(): string
+    {
+        return Node\Expr\Empty_::class;
+    }
+
+    public function processNode(Node $node, Scope $scope): array
+    {
+        assert($node instanceof Node\Expr\Empty_);
+
+        $hint = '';
+        $expression = $node->expr;
+        if ($expression instanceof Node\Expr\Variable) {
+            if (is_string($expression->name)) {
+                $type = $scope->getVariableType($expression->name);
+                if ($type->isArray()->yes()) {
+                    $hint = "You're probably looking for `!count(\${$expression->name})`.";
+                } elseif ($type->isBoolean()->yes()) {
+                    $hint = "You're probably looking for `!\${$expression->name}`.";
+                } elseif ($type->isFloat()->yes()) {
+                    $hint = "You're probably looking for `\${$expression->name} === 0.0 || \${$expression->name} === -0.0` or simply `!\${$expression->name}`.";
+                } elseif ($type->isInteger()->yes()) {
+                    $hint = "You're probably looking for `\${$expression->name} === 0` or simply `!\${$expression->name}`.";
+                } elseif ($type->isString()->yes()) {
+                    $hint = "You're probably looking for `\${$expression->name} === '' || \${$expression->name} === '0'` or simply `!\${$expression->name}`.";
+                } elseif ($type->isNull()->yes()) {
+                    $hint = "You're probably looking for '\${$expression->name} === null` or simply `!\${$expression->name}`.";
+                } elseif ($type->isObject()->yes()) {
+                    $hint = "Checking for empty on an object always returns false (except for some internal objects, but you shouldn't rely on this behavior).";
+                } elseif ($type instanceof ResourceType) {
+                    $hint = 'Checking for empty on a resource always returns false.';
+                }
+            }
+        }
+
+        $message = 'Never use empty(), always check specifically for what you want.';
+        if ($hint) {
+            $message .= " {$hint}";
+        }
+
+        return [
+            RuleErrorBuilder::message($message)->build(),
+        ];
+    }
+}
diff --git a/src/DefaultUnleash.php b/src/DefaultUnleash.php
@@ -70,12 +70,15 @@ public function getVariant(string $featureName, ?Context $context = null, ?Varia
         $feature = $this->findFeature($featureName, $context);
         $enabledResult = $this->isFeatureEnabled($feature, $context);
         $strategyVariants = $enabledResult->getStrategy()?->getVariants() ?? [];
-        if ($feature === null || $enabledResult->isEnabled() === false ||
-            (!count($feature->getVariants()) && empty($strategyVariants))) {
+        if (
+            $feature === null
+            || $enabledResult->isEnabled() === false
+            || (!count($feature->getVariants()) && !count($strategyVariants))
+        ) {
             return $fallbackVariant;
         }
 
-        if (empty($strategyVariants)) {
+        if (!count($strategyVariants)) {
             $variant = $this->variantHandler->selectVariant($feature->getVariants(), $featureName, $context);
         } else {
             $variant = $this->variantHandler->selectVariant($strategyVariants, $enabledResult->getStrategy()?->getParameters()['groupId'] ?? '', $context);