Fix multiple issues with @internal and @psalm-internal (#3841)

* Add passing tests for property fetch on an @internal class I'm trying to work out why the equivilent InvalidCodeParse test is failing for PsalmInternal * Treat all properties of a psalm-internal class as psalm-internal * Remove all $internal properties from storage - use psalm_internal instead @internal can be represented as internal to the namespace root, avoiding the need to check for both properties in storage later. * Raise InternalClass issue when an internal class is used with e.g. instanceOf * fix docs and tests * Add return type declartion to code example in doc * Don't allow class psalm-internal to overide a tighter method psalm-internal * Break up long line * Code style - move && from EOL to SOL * Restore misplaced && * Fix code style * Fix namespace fetching so it works Co-authored-by: Matthew Brown <github@muglug.com>
vimeo · Jul 22, 2020 · 3bc91b9 · 3bc91b9
1 parent 9430a9b
commit 3bc91b9
Show file tree

Hide file tree

Showing 18 changed files with 210 additions and 147 deletions.
diff --git a/docs/running_psalm/issues/InternalClass.md b/docs/running_psalm/issues/InternalClass.md
@@ -15,7 +15,7 @@ namespace A {
 
 namespace B {
     class Bat {
-        public function batBat() {
+        public function batBat(): void {
             $a = new \A\Foo();
         }
     }

diff --git a/docs/running_psalm/issues/InternalMethod.md b/docs/running_psalm/issues/InternalMethod.md
@@ -17,7 +17,7 @@ namespace A {
 }
 namespace B {
     class Bat {
-        public function batBat() {
+        public function batBat(): void {
             \A\Foo::barBar();
         }
     }

diff --git a/src/Psalm/Internal/Analyzer/ClassAnalyzer.php b/src/Psalm/Internal/Analyzer/ClassAnalyzer.php
@@ -302,27 +302,6 @@ public function analyze(
                     }
                 }
 
-                if ($parent_class_storage->internal) {
-                    $code_location = new CodeLocation(
-                        $this,
-                        $class->extends,
-                        $class_context ? $class_context->include_location : null,
-                        true
-                    );
-                    if (! NamespaceAnalyzer::nameSpaceRootsMatch($fq_class_name, $parent_fq_class_name)) {
-                        if (IssueBuffer::accepts(
-                            new InternalClass(
-                                $parent_fq_class_name . ' is marked internal',
-                                $code_location,
-                                $parent_fq_class_name
-                            ),
-                            $storage->suppressed_issues + $this->getSuppressedIssues()
-                        )) {
-                            // fall through
-                        }
-                    }
-                }
-
                 if ($parent_class_storage->psalm_internal &&
                     ! NamespaceAnalyzer::isWithin($fq_class_name, $parent_class_storage->psalm_internal)
                 ) {

diff --git a/src/Psalm/Internal/Analyzer/ClassLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/ClassLikeAnalyzer.php
@@ -8,8 +8,10 @@
 use Psalm\Context;
 use Psalm\Internal\FileManipulation\FileManipulationBuffer;
 use Psalm\Issue\InaccessibleProperty;
+use Psalm\Issue\InternalClass;
 use Psalm\Issue\InvalidClass;
 use Psalm\Issue\MissingDependency;
+use Psalm\Issue\PsalmInternalError;
 use Psalm\Issue\ReservedWord;
 use Psalm\Issue\UndefinedClass;
 use Psalm\Issue\UndefinedDocblockClass;
@@ -399,6 +401,27 @@ public static function checkFullyQualifiedClassLikeName(
             }
         }
 
+        if (!$inferred) {
+            if ($class_storage->psalm_internal) {
+                $sourceNamespace = $statements_source->getNamespace();
+                if (!$sourceNamespace
+                    || ! NamespaceAnalyzer::isWithin($sourceNamespace, $class_storage->psalm_internal)
+                ) {
+                    if (IssueBuffer::accepts(
+                        new InternalClass(
+                            $class_storage->name . ' is internal to ' . $class_storage->psalm_internal,
+                            $code_location,
+                            $class_storage->name
+                        ),
+                        $suppressed_issues
+                    )
+                    ) {
+                        // fall through
+                    }
+                }
+            }
+        }
+
         return true;
     }
 

diff --git a/src/Psalm/Internal/Analyzer/NamespaceAnalyzer.php b/src/Psalm/Internal/Analyzer/NamespaceAnalyzer.php
@@ -184,16 +184,11 @@ public static function isWithin(string $className, string $namespace): bool
         return $className === $namespace || strpos($className, $namespace) === 0;
     }
 
-    public static function nameSpaceRootsMatch(string $fqcnA, string $fqcnB): bool
-    {
-        return strtolower(self::getNameSpaceRoot($fqcnA)) === strtolower(self::getNameSpaceRoot($fqcnB));
-    }
-
     /**
      * @param string $fullyQualifiedClassName, e.g. '\Psalm\Internal\Analyzer\NamespaceAnalyzer'
      * @return string , e.g. 'Psalm'
      */
-    private static function getNameSpaceRoot(string $fullyQualifiedClassName): string
+    public static function getNameSpaceRoot(string $fullyQualifiedClassName): string
     {
         return preg_replace('/^([^\\\]+).*/', '$1', $fullyQualifiedClassName);
     }

diff --git a/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/...Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php
@@ -690,21 +690,6 @@ public static function analyze(
                         }
                     }
 
-                    if ($property_storage->internal && $context->self) {
-                        if (! NamespaceAnalyzer::nameSpaceRootsMatch($context->self, $declaring_property_class)) {
-                            if (IssueBuffer::accepts(
-                                new InternalProperty(
-                                    $property_id . ' is marked internal',
-                                    new CodeLocation($statements_analyzer->getSource(), $stmt),
-                                    $property_id
-                                ),
-                                $statements_analyzer->getSuppressedIssues()
-                            )) {
-                                // fall through
-                            }
-                        }
-                    }
-
                     // prevents writing to readonly properties
                     if ($property_storage->readonly) {
                         $appearing_property_class = $codebase->properties->getAppearingClassForProperty(

diff --git a/...alm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallProhibitionAnalyzer.php b/...alm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallProhibitionAnalyzer.php
@@ -67,28 +67,6 @@ public static function analyze(
                 }
             }
         }
-
-        if ($storage->internal
-            && $context->self
-            && !$context->collect_initializations
-            && !$context->collect_mutations
-        ) {
-            $declaring_class = $method_id->fq_class_name;
-            if (! NamespaceAnalyzer::nameSpaceRootsMatch($context->self, $declaring_class)) {
-                if (IssueBuffer::accepts(
-                    new InternalMethod(
-                        'The method ' . $codebase_methods->getCasedMethodId($method_id) .
-                            ' has been marked as internal',
-                        $code_location,
-                        (string) $method_id
-                    ),
-                    $suppressed_issues
-                )) {
-                    // fall through
-                }
-            }
-        }
-
         return null;
     }
 }
diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php
@@ -425,21 +425,6 @@ public static function analyze(
                     }
                 }
 
-                if ($storage->internal && $context->self) {
-                    if (! NamespaceAnalyzer::nameSpaceRootsMatch($context->self, $fq_class_name)) {
-                        if (IssueBuffer::accepts(
-                            new InternalClass(
-                                $fq_class_name . ' is marked internal',
-                                new CodeLocation($statements_analyzer->getSource(), $stmt),
-                                $fq_class_name
-                            ),
-                            $statements_analyzer->getSuppressedIssues()
-                        )) {
-                            // fall through
-                        }
-                    }
-                }
-
                 $method_id = new \Psalm\Internal\MethodIdentifier($fq_class_name, '__construct');
 
                 if ($codebase->methods->methodExists(

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php
@@ -779,25 +779,6 @@ function (PhpParser\Node\Arg $arg) {
                     }
                 }
 
-                if ($class_storage->internal
-                    && $context->self
-                    && !$context->collect_initializations
-                    && !$context->collect_mutations
-                ) {
-                    if (! NamespaceAnalyzer::nameSpaceRootsMatch($context->self, $fq_class_name)) {
-                        if (IssueBuffer::accepts(
-                            new InternalClass(
-                                $fq_class_name . ' is marked internal',
-                                new CodeLocation($statements_analyzer->getSource(), $stmt),
-                                $fq_class_name
-                            ),
-                            $statements_analyzer->getSuppressedIssues()
-                        )) {
-                            // fall through
-                        }
-                    }
-                }
-
                 if (Method\MethodVisibilityAnalyzer::analyze(
                     $method_id,
                     $context,

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/InstancePropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/InstancePropertyFetchAnalyzer.php
@@ -28,6 +28,7 @@
 use Psalm\Issue\UndefinedThisPropertyFetch;
 use Psalm\Issue\UninitializedProperty;
 use Psalm\IssueBuffer;
+use Psalm\Storage\PropertyStorage;
 use Psalm\Type;
 use Psalm\Storage\ClassLikeStorage;
 use Psalm\Type\Atomic\TGenericObject;
@@ -834,21 +835,6 @@ public static function analyze(
                         }
                     }
                 }
-
-                if ($property_storage->internal && $context->self) {
-                    if (! NamespaceAnalyzer::nameSpaceRootsMatch($context->self, $declaring_property_class)) {
-                        if (IssueBuffer::accepts(
-                            new InternalProperty(
-                                $property_id . ' is marked internal',
-                                new CodeLocation($statements_analyzer->getSource(), $stmt),
-                                $property_id
-                            ),
-                            $statements_analyzer->getSuppressedIssues()
-                        )) {
-                            // fall through
-                        }
-                    }
-                }
             }
 
             $class_property_type = $codebase->properties->getPropertyType(

diff --git a/src/Psalm/Internal/Codebase/Populator.php b/src/Psalm/Internal/Codebase/Populator.php
@@ -20,6 +20,7 @@
 use function reset;
 use function strpos;
 use function strtolower;
+use function strlen;
 
 /**
  * @internal
@@ -249,16 +250,25 @@ private function populateClassLikeStorage(ClassLikeStorage $storage, array $depe
             }
         }
 
-        if ($storage->internal
+        if ($storage->psalm_internal
             && !$storage->is_interface
             && !$storage->is_trait
         ) {
             foreach ($storage->methods as $method) {
-                $method->internal = true;
+                if (null === $method->psalm_internal ||
+                    strlen($storage->psalm_internal) > strlen($method->psalm_internal)
+                ) {
+                    $method->psalm_internal = $storage->psalm_internal;
+                }
             }
 
+
             foreach ($storage->properties as $property) {
-                $property->internal = true;
+                if (null === $property->psalm_internal ||
+                    strlen($storage->psalm_internal) > strlen($property->psalm_internal)
+                ) {
+                    $property->psalm_internal = $storage->psalm_internal;
+                }
             }
         }
 

diff --git a/src/Psalm/Internal/PhpVisitor/ReflectorVisitor.php b/src/Psalm/Internal/PhpVisitor/ReflectorVisitor.php
@@ -1,6 +1,7 @@
 <?php
 namespace Psalm\Internal\PhpVisitor;
 
+use Psalm\Internal\Analyzer\NamespaceAnalyzer;
 use function array_filter;
 use function array_merge;
 use function array_pop;
@@ -1308,8 +1309,16 @@ function (array $l, array $r) : int {
                 }
 
                 $storage->deprecated = $docblock_info->deprecated;
-                $storage->internal = $docblock_info->internal;
-                $storage->psalm_internal = $docblock_info->psalm_internal;
+
+                if ($docblock_info->internal
+                    && !$docblock_info->psalm_internal
+                    && $this->aliases->namespace
+                ) {
+                    $storage->psalm_internal = explode('\\', $this->aliases->namespace)[0];
+                } else {
+                    $storage->psalm_internal = $docblock_info->psalm_internal;
+                }
+
                 $storage->final = $storage->final || $docblock_info->final;
 
                 if ($docblock_info->mixin) {
@@ -2122,8 +2131,13 @@ private function registerFunctionLike(PhpParser\Node\FunctionLike $stmt, $fake_m
         $doc_comment = $stmt->getDocComment();
 
 
-        if ($class_storage && ! $class_storage->is_trait) {
-            $storage->internal = $class_storage->internal;
+        if ($class_storage
+            && !$class_storage->is_trait
+            && $class_storage->psalm_internal
+            && (!$storage->psalm_internal
+                || strlen($class_storage->psalm_internal) > strlen($storage->psalm_internal)
+            )
+        ) {
             $storage->psalm_internal = $class_storage->psalm_internal;
         }
 
@@ -2180,14 +2194,15 @@ private function registerFunctionLike(PhpParser\Node\FunctionLike $stmt, $fake_m
             $storage->deprecated = true;
         }
 
-        if ($docblock_info->internal) {
-            $storage->internal = true;
-        }
-
-        if (null === $class_storage ||
-            null === $class_storage->psalm_internal ||
-            (null !== $docblock_info->psalm_internal &&
-                strlen($docblock_info->psalm_internal) > strlen($class_storage->psalm_internal)
+        if ($docblock_info->internal
+            && !$docblock_info->psalm_internal
+            && $this->aliases->namespace
+        ) {
+            $storage->psalm_internal = explode('\\', $this->aliases->namespace)[0];
+        } elseif (!$class_storage
+            || !$class_storage->psalm_internal
+            || ($docblock_info->psalm_internal
+                && strlen($docblock_info->psalm_internal) > strlen($class_storage->psalm_internal)
             )
         ) {
             $storage->psalm_internal = $docblock_info->psalm_internal;
@@ -3403,8 +3418,10 @@ private function visitPropertyDeclaration(
             $property_storage->stmt_location = new CodeLocation($this->file_scanner, $stmt);
             $property_storage->has_default = $property->default ? true : false;
             $property_storage->deprecated = $var_comment ? $var_comment->deprecated : false;
-            $property_storage->internal = $var_comment ? $var_comment->internal : false;
             $property_storage->psalm_internal = $var_comment ? $var_comment->psalm_internal : null;
+            if (! $property_storage->psalm_internal && $var_comment && $var_comment->internal) {
+                $property_storage->psalm_internal = NamespaceAnalyzer::getNameSpaceRoot($fq_classlike_name);
+            }
             $property_storage->readonly = $var_comment ? $var_comment->readonly : false;
             $property_storage->allow_private_mutation = $var_comment ? $var_comment->allow_private_mutation : false;
 

diff --git a/src/Psalm/Storage/ClassLikeStorage.php b/src/Psalm/Storage/ClassLikeStorage.php
@@ -87,11 +87,6 @@ class ClassLikeStorage
      */
     public $deprecated = false;
 
-    /**
-     * @var bool
-     */
-    public $internal = false;
-
     /**
      * @var null|string
      */

diff --git a/src/Psalm/Storage/FunctionLikeStorage.php b/src/Psalm/Storage/FunctionLikeStorage.php
@@ -66,11 +66,6 @@ abstract class FunctionLikeStorage
      */
     public $deprecated;
 
-    /**
-     * @var ?bool
-     */
-    public $internal;
-
     /**
      * @var null|string
      */

diff --git a/src/Psalm/Storage/PropertyStorage.php b/src/Psalm/Storage/PropertyStorage.php
@@ -64,11 +64,6 @@ class PropertyStorage
      */
     public $deprecated = false;
 
-    /**
-     * @var bool
-     */
-    public $internal = false;
-
     /**
      * @var bool
      */

diff --git a/tests/Config/PluginTest.php b/tests/Config/PluginTest.php
@@ -185,6 +185,8 @@ public function testStringAnalyzerPluginWithClassConstantConcat()
         $this->addFile(
             $file_path,
             '<?php
+                namespace Psalm;
+
                 class A {
                     const C = [
                         "foo" => \Psalm\Internal\Analyzer\ProjectAnalyzer::class . "::foo",