Merge pull request #8165 from AndrolGenhald/stop-using-issuebuffer-add-😡

Improve @psalm-internal and prevent usage of IssueBuffer::add().
vimeo · Jun 25, 2022 · a4ab664 · a4ab664
2 parents d626d24 + b671117
commit a4ab664
Show file tree

Hide file tree

Showing 35 changed files with 347 additions and 138 deletions.
diff --git a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php
@@ -135,6 +135,7 @@ public function __construct(PhpParser\Node\Stmt $class, SourceAnalyzer $source,
         }
     }
 
+    /** @return non-empty-string */
     public static function getAnonymousClassName(PhpParser\Node\Stmt\Class_ $class, string $file_path): string
     {
         return preg_replace('/[^A-Za-z0-9]/', '_', $file_path)
@@ -251,7 +252,7 @@ public function analyze(
         }
 
         foreach ($storage->docblock_issues as $docblock_issue) {
-            IssueBuffer::add($docblock_issue);
+            IssueBuffer::maybeAdd($docblock_issue);
         }
 
         $classlike_storage_provider = $codebase->classlike_storage_provider;
@@ -1647,7 +1648,7 @@ private function analyzeClassMethod(
         $config = Config::getInstance();
 
         if ($stmt->stmts === null && !$stmt->isAbstract()) {
-            IssueBuffer::add(
+            IssueBuffer::maybeAdd(
                 new ParseError(
                     'Non-abstract class method must have statements',
                     new CodeLocation($this, $stmt)
@@ -1660,7 +1661,7 @@ private function analyzeClassMethod(
         try {
             $method_analyzer = new MethodAnalyzer($stmt, $source);
         } catch (UnexpectedValueException $e) {
-            IssueBuffer::add(
+            IssueBuffer::maybeAdd(
                 new ParseError(
                     'Problem loading method: ' . $e->getMessage(),
                     new CodeLocation($this, $stmt)
@@ -2506,11 +2507,12 @@ private function checkParentClass(
                 );
             }
 
-            if (!NamespaceAnalyzer::isWithin($fq_class_name, $parent_class_storage->internal)) {
+            if (!NamespaceAnalyzer::isWithinAny($fq_class_name, $parent_class_storage->internal)) {
                 IssueBuffer::maybeAdd(
                     new InternalClass(
-                        $parent_fq_class_name . ' is internal to ' . $parent_class_storage->internal
-                        . ' but called from ' . $fq_class_name,
+                        $parent_fq_class_name . ' is internal to '
+                            . InternalClass::listToPhrase($parent_class_storage->internal)
+                            . ' but called from ' . $fq_class_name,
                         $code_location,
                         $parent_fq_class_name
                     ),

diff --git a/src/Psalm/Internal/Analyzer/CommentAnalyzer.php b/src/Psalm/Internal/Analyzer/CommentAnalyzer.php
@@ -9,6 +9,7 @@
 use Psalm\Exception\IncorrectDocblockException;
 use Psalm\Exception\TypeParseTreeException;
 use Psalm\FileSource;
+use Psalm\Internal\Scanner\DocblockParser;
 use Psalm\Internal\Scanner\ParsedDocblock;
 use Psalm\Internal\Scanner\VarDocblockComment;
 use Psalm\Internal\Type\TypeAlias;
@@ -21,7 +22,6 @@
 use function preg_match;
 use function preg_replace;
 use function preg_split;
-use function reset;
 use function rtrim;
 use function str_replace;
 use function strlen;
@@ -236,14 +236,7 @@ private static function decorateVarDocblockComment(
             }
         }
 
-        if (isset($parsed_docblock->tags['psalm-internal'])) {
-            $psalm_internal = trim(reset($parsed_docblock->tags['psalm-internal']));
-
-            if (!$psalm_internal) {
-                throw new DocblockParseException('psalm-internal annotation used without specifying namespace');
-            }
-
-            $var_comment->psalm_internal = $psalm_internal;
+        if (count($var_comment->psalm_internal = DocblockParser::handlePsalmInternal($parsed_docblock)) !== 0) {
             $var_comment->internal = true;
         }
 

diff --git a/src/Psalm/Internal/Analyzer/FileAnalyzer.php b/src/Psalm/Internal/Analyzer/FileAnalyzer.php
@@ -196,7 +196,7 @@ public function analyze(
         $statements_analyzer = new StatementsAnalyzer($this, $this->node_data);
 
         foreach ($file_storage->docblock_issues as $docblock_issue) {
-            IssueBuffer::add($docblock_issue);
+            IssueBuffer::maybeAdd($docblock_issue);
         }
 
         // if there are any leftover statements, evaluate them,

diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
@@ -209,7 +209,7 @@ public function analyze(
         }
 
         foreach ($storage->docblock_issues as $docblock_issue) {
-            IssueBuffer::add($docblock_issue);
+            IssueBuffer::maybeAdd($docblock_issue);
         }
 
         $function_information = $this->getFunctionInformation(

diff --git a/src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php b/src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php
@@ -133,7 +133,7 @@ public function analyze(): void
                     );
                 }
             } elseif ($stmt instanceof PhpParser\Node\Stmt\Property) {
-                IssueBuffer::add(
+                IssueBuffer::maybeAdd(
                     new ParseError(
                         'Interfaces cannot have properties',
                         new CodeLocation($this, $stmt)

diff --git a/src/Psalm/Internal/Analyzer/NamespaceAnalyzer.php b/src/Psalm/Internal/Analyzer/NamespaceAnalyzer.php
@@ -12,11 +12,13 @@
 use ReflectionProperty;
 use UnexpectedValueException;
 
+use function assert;
+use function count;
 use function implode;
 use function preg_replace;
 use function strpos;
 use function strtolower;
-use function trim;
+use function substr;
 
 /**
  * @internal
@@ -152,33 +154,127 @@ public function getFileAnalyzer(): FileAnalyzer
     }
 
     /**
-     * Returns true if $className is the same as, or starts with $namespace, in a case-insensitive comparison.
+     * Returns true if $calling_identifier is the same as, or is within with $identifier, in a
+     * case-insensitive comparison. Identifiers can be namespaces, classlikes, functions, or methods.
      *
+     * @psalm-pure
+     *
+     * @throws InvalidArgumentException if $identifier is not a valid identifier
+     */
+    public static function isWithin(string $calling_identifier, string $identifier): bool
+    {
+        $normalized_calling_ident = self::normalizeIdentifier($calling_identifier);
+        $normalized_ident = self::normalizeIdentifier($identifier);
+
+        if ($normalized_calling_ident === $normalized_ident) {
+            return true;
+        }
+
+        $normalized_calling_ident_parts = self::getIdentifierParts($normalized_calling_ident);
+        $normalized_ident_parts = self::getIdentifierParts($normalized_ident);
+
+        if (count($normalized_calling_ident_parts) < count($normalized_ident_parts)) {
+            return false;
+        }
+
+        for ($i = 0; $i < count($normalized_ident_parts); ++$i) {
+            if ($normalized_ident_parts[$i] !== $normalized_calling_ident_parts[$i]) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    /**
+     * Returns true if $calling_identifier is the same as or is within any identifier
+     * in $identifiers in a case-insensitive comparison, or if $identifiers is empty.
+     * Identifiers can be namespaces, classlikes, functions, or methods.
      *
      * @psalm-pure
+     *
+     * @psalm-assert-if-false !empty $identifiers
+     *
+     * @param list<string> $identifiers
      */
-    public static function isWithin(string $calling_namespace, string $namespace): bool
+    public static function isWithinAny(string $calling_identifier, array $identifiers): bool
     {
-        if ($namespace === '') {
-            return true; // required to prevent a warning from strpos with empty needle in PHP < 8
+        if (count($identifiers) === 0) {
+            return true;
         }
 
-        $calling_namespace = strtolower(trim($calling_namespace, '\\') . '\\');
-        $namespace = strtolower(trim($namespace, '\\') . '\\');
+        foreach ($identifiers as $identifier) {
+            if (self::isWithin($calling_identifier, $identifier)) {
+                return true;
+            }
+        }
 
-        return $calling_namespace === $namespace
-            || strpos($calling_namespace, $namespace) === 0;
+        return false;
     }
 
     /**
-     * @param string $fullyQualifiedClassName, e.g. '\Psalm\Internal\Analyzer\NamespaceAnalyzer'
+     * @param non-empty-string $fullyQualifiedClassName, e.g. '\Psalm\Internal\Analyzer\NamespaceAnalyzer'
      *
-     * @return string , e.g. 'Psalm'
+     * @return non-empty-string , e.g. 'Psalm'
      *
      * @psalm-pure
      */
     public static function getNameSpaceRoot(string $fullyQualifiedClassName): string
     {
-        return preg_replace('/^([^\\\]+).*/', '$1', $fullyQualifiedClassName);
+        $root_namespace = preg_replace('/^([^\\\]+).*/', '$1', $fullyQualifiedClassName);
+        if ($root_namespace === "") {
+            throw new InvalidArgumentException("Invalid classname \"$fullyQualifiedClassName\"");
+        }
+        return $root_namespace;
+    }
+
+    /**
+     * @return ($lowercase is true ? lowercase-string : string)
+     *
+     * @psalm-pure
+     */
+    public static function normalizeIdentifier(string $identifier, bool $lowercase = true): string
+    {
+        if ($identifier === "") {
+            return "";
+        }
+
+        $identifier = $identifier[0] === "\\" ? substr($identifier, 1) : $identifier;
+        return $lowercase ? strtolower($identifier) : $identifier;
+    }
+
+    /**
+     * Splits an identifier into parts, eg `Foo\Bar::baz` becomes ["Foo", "\\", "Bar", "::", "baz"].
+     *
+     * @return list<non-empty-string>
+     *
+     * @psalm-pure
+     */
+    public static function getIdentifierParts(string $identifier): array
+    {
+        $parts = [];
+        while (($pos = strpos($identifier, "\\")) !== false) {
+            if ($pos > 0) {
+                $part = substr($identifier, 0, $pos);
+                assert($part !== "");
+                $parts[] = $part;
+            }
+            $parts[] = "\\";
+            $identifier = substr($identifier, $pos + 1);
+        }
+        if (($pos = strpos($identifier, "::")) !== false) {
+            if ($pos > 0) {
+                $part = substr($identifier, 0, $pos);
+                assert($part !== "");
+                $parts[] = $part;
+            }
+            $parts[] = "::";
+            $identifier = substr($identifier, $pos + 2);
+        }
+        if ($identifier !== "") {
+            $parts[] = $identifier;
+        }
+
+        return $parts;
     }
 }
diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php
@@ -74,7 +74,7 @@ public static function analyze(
 
         foreach ($stmt->items as $item) {
             if ($item === null) {
-                IssueBuffer::add(
+                IssueBuffer::maybeAdd(
                     new ParseError(
                         'Array element cannot be empty',
                         new CodeLocation($statements_analyzer, $stmt)

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php
@@ -959,7 +959,7 @@ protected static function processCustomAssertion(
                     }
                 } elseif ($assertion->var_id === '$this') {
                     if (!$expr instanceof PhpParser\Node\Expr\MethodCall) {
-                        IssueBuffer::add(
+                        IssueBuffer::maybeAdd(
                             new InvalidDocblock(
                                 'Assertion of $this can be done only on method of a class',
                                 new CodeLocation($source, $expr)

diff --git a/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php
@@ -37,6 +37,7 @@
 use Psalm\Issue\ImplicitToStringCast;
 use Psalm\Issue\ImpurePropertyAssignment;
 use Psalm\Issue\InaccessibleProperty;
+use Psalm\Issue\InternalClass;
 use Psalm\Issue\InternalProperty;
 use Psalm\Issue\InvalidPropertyAssignment;
 use Psalm\Issue\InvalidPropertyAssignmentValue;
@@ -420,7 +421,7 @@ public static function analyzeStatement(
         foreach ($stmt->props as $prop) {
             if ($prop->default) {
                 if ($stmt->isReadonly()) {
-                    IssueBuffer::add(
+                    IssueBuffer::maybeAdd(
                         new InvalidPropertyAssignment(
                             'Readonly property ' . $context->self . '::$' . $prop->name->name
                                 . ' cannot have a default',
@@ -1258,11 +1259,11 @@ private static function analyzeAtomicAssignment(
                 );
             }
 
-            if ($context->self && !NamespaceAnalyzer::isWithin($context->self, $property_storage->internal)) {
+            if ($context->self && !NamespaceAnalyzer::isWithinAny($context->self, $property_storage->internal)) {
                 IssueBuffer::maybeAdd(
                     new InternalProperty(
-                        $property_id . ' is internal to ' . $property_storage->internal
-                        . ' but called from ' . $context->self,
+                        $property_id . ' is internal to ' . InternalClass::listToPhrase($property_storage->internal)
+                            . ' but called from ' . $context->self,
                         new CodeLocation($statements_analyzer->getSource(), $stmt),
                         $property_id
                     ),

diff --git a/.../Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php b/.../Internal/Analyzer/Statements/Expression/Call/Method/ExistingAtomicMethodCallAnalyzer.php
@@ -271,7 +271,7 @@ public static function analyze(
                 $codebase,
                 $context,
                 $method_id,
-                $statements_analyzer->getNamespace(),
+                $statements_analyzer->getFullyQualifiedFunctionMethodOrNamespaceName(),
                 $name_code_location,
                 $statements_analyzer->getSuppressedIssues()
             );

diff --git a/...alm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallProhibitionAnalyzer.php b/...alm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallProhibitionAnalyzer.php
@@ -8,6 +8,7 @@
 use Psalm\Internal\Analyzer\NamespaceAnalyzer;
 use Psalm\Internal\MethodIdentifier;
 use Psalm\Issue\DeprecatedMethod;
+use Psalm\Issue\InternalClass;
 use Psalm\Issue\InternalMethod;
 use Psalm\IssueBuffer;
 
@@ -22,7 +23,7 @@ public static function analyze(
         Codebase $codebase,
         Context $context,
         MethodIdentifier $method_id,
-        ?string $namespace,
+        ?string $caller_identifier,
         CodeLocation $code_location,
         array $suppressed_issues
     ): ?bool {
@@ -51,12 +52,12 @@ public static function analyze(
         if (!$context->collect_initializations
             && !$context->collect_mutations
         ) {
-            if (!NamespaceAnalyzer::isWithin($namespace ?: '', $storage->internal)) {
+            if (!NamespaceAnalyzer::isWithinAny($caller_identifier ?? "", $storage->internal)) {
                 IssueBuffer::maybeAdd(
                     new InternalMethod(
                         'The method ' . $codebase_methods->getCasedMethodId($method_id)
-                            . ' is internal to ' . $storage->internal
-                            . ' but called from ' . ($context->self ?: 'root namespace'),
+                            . ' is internal to ' . InternalClass::listToPhrase($storage->internal)
+                            . ' but called from ' . ($caller_identifier ?: 'root namespace'),
                         $code_location,
                         (string) $method_id
                     ),

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php
@@ -349,12 +349,12 @@ private static function analyzeNamedConstructor(
         if ($context->self
             && !$context->collect_initializations
             && !$context->collect_mutations
-            && !NamespaceAnalyzer::isWithin($context->self, $storage->internal)
+            && !NamespaceAnalyzer::isWithinAny($context->self, $storage->internal)
         ) {
             IssueBuffer::maybeAdd(
                 new InternalClass(
-                    $fq_class_name . ' is internal to ' . $storage->internal
-                    . ' but called from ' . $context->self,
+                    $fq_class_name . ' is internal to ' . InternalClass::listToPhrase($storage->internal)
+                        . ' but called from ' . $context->self,
                     new CodeLocation($statements_analyzer->getSource(), $stmt),
                     $fq_class_name
                 ),
@@ -411,16 +411,13 @@ private static function analyzeNamedConstructor(
             if ($declaring_method_id) {
                 $method_storage = $codebase->methods->getStorage($declaring_method_id);
 
-                $namespace = $statements_analyzer->getNamespace() ?: '';
-                if (!NamespaceAnalyzer::isWithin(
-                    $namespace,
-                    $method_storage->internal
-                )) {
+                $caller_identifier = $statements_analyzer->getFullyQualifiedFunctionMethodOrNamespaceName() ?: '';
+                if (!NamespaceAnalyzer::isWithinAny($caller_identifier, $method_storage->internal)) {
                     IssueBuffer::maybeAdd(
                         new InternalMethod(
                             'Constructor ' . $codebase->methods->getCasedMethodId($declaring_method_id)
-                            . ' is internal to ' . $method_storage->internal
-                            . ' but called from ' . ($namespace ?: 'root namespace'),
+                                . ' is internal to ' . InternalClass::listToPhrase($method_storage->internal)
+                                . ' but called from ' . ($caller_identifier ?: 'root namespace'),
                             new CodeLocation($statements_analyzer, $stmt),
                             (string) $method_id
                         ),