Conditionally verify that array offsets exist (#2147)

* Check array offsets idea

* Clean up some issues

* Add a few light fixes

* Add docs

config.xsd

@@ -55,6 +55,7 @@
         <xs:attribute name="resolveFromConfigFile" type="xs:string" />
         <xs:attribute name="includePhpVersionsInErrorBaseline" type="xs:string" />
         <xs:attribute name="loadXdebugStub" type="xs:string" />
+        <xs:attribute name="ensureArrayStringOffsetsExist" type="xs:string" />
     </xs:complexType>
 
     <xs:complexType name="ProjectFilesType">

docs/running_psalm/configuration.md

@@ -213,6 +213,14 @@ If not present, Psalm will only load the Xdebug stub if psalm has unloaded the e
 When `true`, Psalm will load the Xdebug extension stub (as the extension is unloaded when Psalm runs).
 Setting to `false` prevents the stub from loading.
 
+#### ensureArrayStringOffsetsExist
+```xml
+<psalm
+  ensureArrayStringOffsetsExist="[bool]"
+>
+```
+When `true`, Psalm will complain when referencing an explicit string offset on an array e.g. `$arr['foo']` without a user first asserting that it exists (either via an `isset` check or via an object-like array). Defaults to `false`.
+
 ### Running Psalm
 
 #### autoloader

psalm.xml.dist

@@ -10,6 +10,7 @@
     checkForThrowsDocblock="false"
     throwExceptionOnError="0"
     findUnusedCode="true"
+    ensureArrayStringOffsetsExist="false"
     resolveFromConfigFile="true"
     xsi:schemaLocation="https://getpsalm.org/schema/config config.xsd"
 >
@@ -132,5 +133,11 @@
                 <directory name="tests"/>
             </errorLevel>
         </InternalMethod>
+
+        <PossiblyUndefinedArrayOffset>
+            <errorLevel type="suppress">
+                <directory name="tests"/>
+            </errorLevel>
+        </PossiblyUndefinedArrayOffset>
     </issueHandlers>
 </psalm>

src/Psalm/Config.php

@@ -321,6 +321,11 @@ class Config
      */
     public $infer_property_types_from_constructor = true;
 
+    /**
+     * @var bool
+     */
+    public $ensure_array_string_offsets_exist = false;
+
     /**
      * @var array<string, bool>
      */
@@ -691,6 +696,7 @@ class Config
             'ignoreInternalFunctionNullReturn' => 'ignore_internal_nullable_issues',
             'includePhpVersionsInErrorBaseline' => 'include_php_versions_in_error_baseline',
             'loadXdebugStub' => 'load_xdebug_stub',
+            'ensureArrayStringOffsetsExist' => 'ensure_array_string_offsets_exist',
         ];
 
         foreach ($booleanAttributes as $xmlName => $internalName) {

src/Psalm/ErrorBaseline.php

@@ -35,8 +35,11 @@ class ErrorBaseline
         foreach ($existingIssues as $existingIssue) {
             $totalIssues += array_reduce(
                 $existingIssue,
+                /**
+                 * @param array{o:int, s:array<int, string>} $existingIssue
+                 */
                 function (int $carry, array $existingIssue): int {
-                    return $carry + (int)$existingIssue['o'];
+                    return $carry + $existingIssue['o'];
                 },
                 0
             );

src/Psalm/Internal/Analyzer/ClassAnalyzer.php

@@ -1099,6 +1099,7 @@ class ClassAnalyzer extends ClassLikeAnalyzer
         if (!$storage->abstract
             && !$constructor_analyzer
             && isset($storage->declaring_method_ids['__construct'])
+            && isset($storage->appearing_method_ids['__construct'])
             && $class->extends
         ) {
             list($constructor_declaring_fqcln) = explode('::', $storage->declaring_method_ids['__construct']);

src/Psalm/Internal/Analyzer/FileAnalyzer.php

@@ -352,6 +352,10 @@ class FileAnalyzer extends SourceAnalyzer implements StatementsSource
             }
         }
 
+        if (!isset($this_context->vars_in_scope['$this'])) {
+            throw new \UnexpectedValueException('Should exist');
+        }
+
         $call_context->vars_in_scope['$this'] = $this_context->vars_in_scope['$this'];
 
         $class_analyzer_to_examine->getMethodMutations($method_name, $call_context);

src/Psalm/Internal/Analyzer/FunctionAnalyzer.php

@@ -302,6 +302,7 @@ class FunctionAnalyzer extends FunctionLikeAnalyzer
 
                         if (isset($first_arg->inferredType)) {
                             if ($first_arg->inferredType->hasArray()) {
+                                /** @psalm-suppress PossiblyUndefinedArrayOffset */
                                 $array_type = $first_arg->inferredType->getTypes()['array'];
                                 if ($array_type instanceof Type\Atomic\ObjectLike) {
                                     return $array_type->getGenericValueType();

src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php

@@ -557,7 +557,10 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements
                         ))
                 ) {
                     if ($this->function->inferredType) {
-                        /** @var Type\Atomic\TFn */
+                        /**
+                         * @psalm-suppress PossiblyUndefinedArrayOffset
+                         * @var Type\Atomic\TFn
+                         */
                         $closure_atomic = $this->function->inferredType->getTypes()['Closure'];
                         $closure_atomic->return_type = $closure_return_type;
                     }

src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php

@@ -509,42 +509,70 @@ class ArrayFetchAnalyzer
                                     $has_valid_offset = true;
                                 }
                             }
-                        } elseif ((!TypeAnalyzer::isContainedBy(
+                        } else {
-                            $codebase,
+                            $offset_type_contained_by_expected = TypeAnalyzer::isContainedBy(
-                            $offset_type,
-                            $expected_offset_type,
-                            true,
-                            $offset_type->ignore_falsable_issues,
-                            $union_comparison_results
-                        ) && !$union_comparison_results->type_coerced_from_scalar)
-                            || $union_comparison_results->to_string_cast
-                        ) {
-                            if ($union_comparison_results->type_coerced_from_mixed
-                                && !$offset_type->isMixed()
-                            ) {
-                                if (IssueBuffer::accepts(
-                                    new MixedArrayTypeCoercion(
-                                        'Coercion from array offset type \'' . $offset_type->getId() . '\' '
-                                            . 'to the expected type \'' . $expected_offset_type->getId() . '\'',
-                                        new CodeLocation($statements_analyzer->getSource(), $stmt)
-                                    ),
-                                    $statements_analyzer->getSuppressedIssues()
-                                )) {
-                                    // fall through
-                                }
-                            } else {
-                                $expected_offset_types[] = $expected_offset_type->getId();
-                            }
-
-                            if (TypeAnalyzer::canExpressionTypesBeIdentical(
                                 $codebase,
                                 $offset_type,
-                                $expected_offset_type
+                                $expected_offset_type,
-                            )) {
+                                true,
+                                $offset_type->ignore_falsable_issues,
+                                $union_comparison_results
+                            );
+
+                            if ($offset_type_contained_by_expected
+                                && $offset_type->hasLiteralString()
+                                && !$expected_offset_type->hasLiteralClassString()
+                                && !$context->inside_isset
+                                && !$context->inside_unset
+                            ) {
+                                if ($codebase->config->ensure_array_string_offsets_exist) {
+                                    if (IssueBuffer::accepts(
+                                        new PossiblyUndefinedArrayOffset(
+                                            'Possibly undefined array offset \''
+                                                . $offset_type->getId() . '\' '
+                                                . 'is risky given expected type \''
+                                                . $expected_offset_type->getId() . '\'.'
+                                                . ' Consider using isset beforehand.',
+                                            new CodeLocation($statements_analyzer->getSource(), $stmt)
+                                        ),
+                                        $statements_analyzer->getSuppressedIssues()
+                                    )) {
+                                        // fall through
+                                    }
+                                }
+                            }
+
+                            if ((!$offset_type_contained_by_expected
+                                    && !$union_comparison_results->type_coerced_from_scalar)
+                                || $union_comparison_results->to_string_cast
+                            ) {
+                                if ($union_comparison_results->type_coerced_from_mixed
+                                    && !$offset_type->isMixed()
+                                ) {
+                                    if (IssueBuffer::accepts(
+                                        new MixedArrayTypeCoercion(
+                                            'Coercion from array offset type \'' . $offset_type->getId() . '\' '
+                                                . 'to the expected type \'' . $expected_offset_type->getId() . '\'',
+                                            new CodeLocation($statements_analyzer->getSource(), $stmt)
+                                        ),
+                                        $statements_analyzer->getSuppressedIssues()
+                                    )) {
+                                        // fall through
+                                    }
+                                } else {
+                                    $expected_offset_types[] = $expected_offset_type->getId();
+                                }
+
+                                if (TypeAnalyzer::canExpressionTypesBeIdentical(
+                                    $codebase,
+                                    $offset_type,
+                                    $expected_offset_type
+                                )) {
+                                    $has_valid_offset = true;
+                                }
+                            } else {
                                 $has_valid_offset = true;
                             }
-                        } else {
-                            $has_valid_offset = true;
                         }
                     }
 

src/Psalm/IssueBuffer.php

@@ -354,6 +354,10 @@ class IssueBuffer
         if (self::$issues_data) {
             usort(
                 self::$issues_data,
+                /**
+                 * @param array{file_path: string, line_from: int, column_from: int} $d1
+                 * @param array{file_path: string, line_from: int, column_from: int} $d2
+                 */
                 function (array $d1, array $d2) : int {
                     if ($d1['file_path'] === $d2['file_path']) {
                         if ($d1['line_from'] === $d2['line_from']) {

src/Psalm/Report.php

@@ -68,6 +68,9 @@ abstract class Report
         if (!$report_options->show_info) {
             $this->issues_data = array_filter(
                 $issues_data,
+                /**
+                 * @var array{severity: string}
+                 */
                 function (array $issue_data) : bool {
                     return $issue_data['severity'] !== Config::REPORT_INFO;
                 }

src/Psalm/Type.php

@@ -617,7 +617,10 @@ abstract class Type
 
             $offset_defining_class = array_keys($offset_template_data)[0];
 
-            if (!$offset_defining_class && $offset_template_data[''][0]->isSingle()) {
+            if (!$offset_defining_class
+                && isset($offset_template_data[''])
+                && $offset_template_data[''][0]->isSingle()
+            ) {
                 $offset_template_type = array_values($offset_template_data[''][0]->getTypes())[0];
 
                 if ($offset_template_type instanceof Type\Atomic\TTemplateKeyOf) {

src/Psalm/Type/Union.php

@@ -1675,6 +1675,14 @@ class Union
             || isset($this->types['true']);
     }
 
+    /**
+     * @return bool
+     */
+    public function hasLiteralString()
+    {
+        return count($this->literal_string_types) > 0;
+    }
+
     /**
      * @return bool true if this is a int literal with only one possible value
      */

tests/ArrayAccessTest.php

@@ -8,6 +8,74 @@ class ArrayAccessTest extends TestCase
     use Traits\InvalidCodeAnalysisTestTrait;
     use Traits\ValidCodeAnalysisTestTrait;
 
+    /**
+     * @return void
+     */
+    public function testEnsureArrayOffsetsExist()
+    {
+        $this->expectException(\Psalm\Exception\CodeException::class);
+        $this->expectExceptionMessage('PossiblyUndefinedArrayOffset');
+
+        \Psalm\Config::getInstance()->ensure_array_string_offsets_exist = true;
+
+        $this->addFile(
+            'somefile.php',
+            '<?php
+                function takesString(string $s): void {}
+
+                /** @param array<string, string> $arr */
+                function takesArrayIteratorOfString(array $arr): void {
+                    echo $arr["hello"];
+                }'
+        );
+
+        $this->analyzeFile('somefile.php', new \Psalm\Context());
+    }
+
+    /**
+     * @return void
+     */
+    public function testEnsureArrayOffsetsExistWithIssetCheck()
+    {
+        \Psalm\Config::getInstance()->ensure_array_string_offsets_exist = true;
+
+        $this->addFile(
+            'somefile.php',
+            '<?php
+                function takesString(string $s): void {}
+
+                /** @param array<string, string> $arr */
+                function takesArrayIteratorOfString(array $arr): void {
+                    if (isset($arr["hello"])) {
+                        echo $arr["hello"];
+                    }
+                }'
+        );
+
+        $this->analyzeFile('somefile.php', new \Psalm\Context());
+    }
+
+    /**
+     * @return void
+     */
+    public function testDontEnsureArrayOffsetsExist()
+    {
+        \Psalm\Config::getInstance()->ensure_array_string_offsets_exist = false;
+
+        $this->addFile(
+            'somefile.php',
+            '<?php
+                function takesString(string $s): void {}
+
+                /** @param array<string, string> $arr */
+                function takesArrayIteratorOfString(array $arr): void {
+                    echo $arr["hello"];
+                }'
+        );
+
+        $this->analyzeFile('somefile.php', new \Psalm\Context());
+    }
+
     /**
      * @return iterable<string,array{string,assertions?:array<string,string>,error_levels?:string[]}>
      */