Merge pull request #9177 from Nicelocal/duplicate_keys

Detect duplicate keys in array shapes
vimeo · Jan 25, 2023 · aec0edc · aec0edc
2 parents 0682adf + 40a05d5
commit aec0edc
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 14 deletions.
diff --git a/src/Psalm/Internal/Analyzer/CommentAnalyzer.php b/src/Psalm/Internal/Analyzer/CommentAnalyzer.php
@@ -162,7 +162,7 @@ public static function arrayToDocblocks(
                     throw new DocblockParseException(
                         $line_parts[0] .
                         ' is not a valid type' .
-                        ' (from ' .
+                        ' ('.$e->getMessage().' in ' .
                         $source->getFilePath() .
                         ':' .
                         $comment->getStartLine() .

diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php b/src/Psalm/Internal/PhpVisitor/Reflector/ClassLikeNodeScanner.php
@@ -1895,7 +1895,7 @@ private static function getTypeAliasesFromCommentLines(
                     $self_fqcln,
                 );
             } catch (TypeParseTreeException $e) {
-                throw new DocblockParseException($type_string . ' is not a valid type');
+                throw new DocblockParseException($type_string . ' is not a valid type: '.$e->getMessage());
             }
 
             $type_alias_tokens[$type_alias] = new InlineTypeAlias($type_tokens);

diff --git a/src/Psalm/Internal/Type/TypeParser.php b/src/Psalm/Internal/Type/TypeParser.php
@@ -1478,14 +1478,18 @@ private static function getTypeFromKeyedArrayTree(
                 $had_optional = true;
             }
 
+            if (isset($properties[$property_key])) {
+                throw new TypeParseTreeException("Duplicate key $property_key detected");
+            }
+
             $properties[$property_key] = $property_type;
             if ($class_string) {
                 $class_strings[$property_key] = true;
             }
         }
 
         if ($had_explicit && $had_implicit) {
-            throw new TypeParseTreeException('Cannot mix explicit and implicit keys!');
+            throw new TypeParseTreeException('Cannot mix explicit and implicit keys');
         }
 
         if ($type === 'object') {
@@ -1500,15 +1504,15 @@ private static function getTypeFromKeyedArrayTree(
         }
 
         if ($callable && !$properties) {
-            throw new TypeParseTreeException('A callable array cannot be empty!');
+            throw new TypeParseTreeException('A callable array cannot be empty');
         }
 
         if ($type !== 'array' && $type !== 'list') {
             throw new TypeParseTreeException('Unexpected brace character');
         }
 
         if ($type === 'list' && !$is_list) {
-            throw new TypeParseTreeException('A list shape cannot describe a non-list!');
+            throw new TypeParseTreeException('A list shape cannot describe a non-list');
         }
 
         if (!$properties) {
@@ -1520,7 +1524,7 @@ private static function getTypeFromKeyedArrayTree(
             $class_strings,
             $sealed
                 ? null
-                : [$is_list ? Type::getInt() : Type::getArrayKey(), Type::getMixed()],
+                : [$is_list ? Type::getListKey() : Type::getArrayKey(), Type::getMixed()],
             $is_list,
             $from_docblock,
         );

diff --git a/tests/TypeParseTest.php b/tests/TypeParseTest.php
@@ -473,54 +473,60 @@ public function testTKeyedCallableArrayNonList(): void
 
     public function testTKeyedListNonList(): void
     {
-        $this->expectExceptionMessage('A list shape cannot describe a non-list!');
+        $this->expectExceptionMessage('A list shape cannot describe a non-list');
         Type::parseString('list{a: 0, b: 1, c: 2}');
     }
 
 
     public function testTKeyedListNonListOptional(): void
     {
-        $this->expectExceptionMessage('A list shape cannot describe a non-list!');
+        $this->expectExceptionMessage('A list shape cannot describe a non-list');
         Type::parseString('list{a: 0, b?: 1, c?: 2}');
     }
 
     public function testTKeyedListNonListOptionalWrongOrder1(): void
     {
-        $this->expectExceptionMessage('A list shape cannot describe a non-list!');
+        $this->expectExceptionMessage('A list shape cannot describe a non-list');
         Type::parseString('list{0?: 0, 1: 1, 2: 2}');
     }
 
     public function testTKeyedListNonListOptionalWrongOrder2(): void
     {
-        $this->expectExceptionMessage('A list shape cannot describe a non-list!');
+        $this->expectExceptionMessage('A list shape cannot describe a non-list');
         Type::parseString('list{0: 0, 1?: 1, 2: 2}');
     }
 
 
     public function testTKeyedListWrongOrder(): void
     {
-        $this->expectExceptionMessage('A list shape cannot describe a non-list!');
+        $this->expectExceptionMessage('A list shape cannot describe a non-list');
         Type::parseString('list{1: 1, 0: 0}');
     }
 
     public function testTKeyedListNonListKeys(): void
     {
-        $this->expectExceptionMessage('A list shape cannot describe a non-list!');
+        $this->expectExceptionMessage('A list shape cannot describe a non-list');
         Type::parseString('list{1: 1, 2: 2}');
     }
 
     public function testTKeyedListNoExplicitAndImplicitKeys(): void
     {
-        $this->expectExceptionMessage('Cannot mix explicit and implicit keys!');
+        $this->expectExceptionMessage('Cannot mix explicit and implicit keys');
         Type::parseString('list{0: 0, 1}');
     }
 
     public function testTKeyedArrayNoExplicitAndImplicitKeys(): void
     {
-        $this->expectExceptionMessage('Cannot mix explicit and implicit keys!');
+        $this->expectExceptionMessage('Cannot mix explicit and implicit keys');
         Type::parseString('array{0, test: 1}');
     }
 
+    public function testTKeyedArrayNoDuplicateKeys(): void
+    {
+        $this->expectExceptionMessage('Duplicate key a detected');
+        Type::parseString('array{a: int, a: int}');
+    }
+
     public function testSimpleCallable(): void
     {
         $this->assertSame(