From c195e8fd21a197a783f2d2bdb310ce6e03fd49f0 Mon Sep 17 00:00:00 2001 From: Brown Date: Wed, 30 Jan 2019 15:40:37 -0500 Subject: [PATCH] Add more nuanced analysis of array access fetch --- .../Assignment/ArrayAssignmentAnalyzer.php | 1 + .../Expression/Fetch/ArrayFetchAnalyzer.php | 96 ++++++++++++++----- src/Psalm/Internal/Codebase/ClassLikes.php | 4 - tests/ArrayAccessTest.php | 12 ++- tests/MethodSignatureTest.php | 2 +- 5 files changed, 83 insertions(+), 32 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php index c8d2a94e0..41253ff30 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php @@ -200,6 +200,7 @@ class ArrayAssignmentAnalyzer isset($child_stmt->dim->inferredType) ? $child_stmt->dim->inferredType : Type::getInt(), true, $array_var_id, + $context, $child_stmts ? null : $assignment_type ); diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php index 2f23336ac..ccae1351f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php @@ -147,8 +147,8 @@ class ArrayFetchAnalyzer $used_key_type, false, $array_var_id, - null, - $context->inside_isset + $context, + null ); if ($context->inside_isset @@ -241,7 +241,6 @@ class ArrayFetchAnalyzer * @param Type\Union $offset_type * @param bool $in_assignment * @param null|string $array_var_id - * @param bool $inside_isset * * @return Type\Union */ @@ -252,8 +251,8 @@ class ArrayFetchAnalyzer Type\Union $offset_type, $in_assignment, $array_var_id, - Type\Union $replacement_type = null, - $inside_isset = false + Context $context, + Type\Union $replacement_type = null ) { $codebase = $statements_analyzer->getCodebase(); @@ -305,7 +304,7 @@ class ArrayFetchAnalyzer return Type::getMixed(); } - if ($offset_type->isNullable() && !$offset_type->ignore_nullable_issues && !$inside_isset) { + if ($offset_type->isNullable() && !$offset_type->ignore_nullable_issues && !$context->inside_isset) { if (IssueBuffer::accepts( new PossiblyNullArrayOffset( 'Cannot access value on variable ' . $array_var_id @@ -346,7 +345,7 @@ class ArrayFetchAnalyzer $array_access_type = new Type\Union([new TEmpty]); } } else { - if (!$inside_isset) { + if (!$context->inside_isset) { if (IssueBuffer::accepts( new PossiblyNullArrayAccess( 'Cannot access array value on possibly null variable ' . $array_var_id . @@ -459,7 +458,7 @@ class ArrayFetchAnalyzer if ($array_access_type->isEmpty() && !$array_type->hasMixed() && !$in_assignment - && !$inside_isset + && !$context->inside_isset ) { if (IssueBuffer::accepts( new EmptyArrayAccess( @@ -513,7 +512,7 @@ class ArrayFetchAnalyzer ); } } else { - if (!$inside_isset || $type->sealed) { + if (!$context->inside_isset || $type->sealed) { $object_like_keys = array_keys($type->properties); if (count($object_like_keys) === 1) { @@ -547,7 +546,7 @@ class ArrayFetchAnalyzer $type_coerced_from_scalar ); - if ($inside_isset && !$is_contained) { + if ($context->inside_isset && !$is_contained) { $is_contained = TypeAnalyzer::canBeContainedBy( $codebase, $offset_type, @@ -611,7 +610,7 @@ class ArrayFetchAnalyzer $has_valid_offset = true; } else { - if (!$inside_isset || $type->sealed) { + if (!$context->inside_isset || $type->sealed) { $expected_offset_types[] = (string)$generic_key_type->getId(); } @@ -683,7 +682,7 @@ class ArrayFetchAnalyzer if ($type instanceof TMixed || $type instanceof TGenericParam || $type instanceof TEmpty) { $codebase->analyzer->incrementMixedCount($statements_analyzer->getFilePath()); - if (!$inside_isset) { + if (!$context->inside_isset) { if ($in_assignment) { if (IssueBuffer::accepts( new MixedArrayAssignment( @@ -718,18 +717,71 @@ class ArrayFetchAnalyzer continue; } - if ($type instanceof TNamedObject) { - if (strtolower($type->value) !== 'simplexmlelement' - && strtolower($type->value) !== 'arrayaccess' - && (($codebase->classExists($type->value) - && !$codebase->classImplements($type->value, 'ArrayAccess')) - || ($codebase->interfaceExists($type->value) - && !$codebase->interfaceExtends($type->value, 'ArrayAccess')) + if ($type instanceof TNamedObject && $stmt->dim) { + if (strtolower($type->value) === 'simplexmlelement') { + $array_access_type = Type::getMixed(); + } elseif (strtolower($type->value) === 'domnodelist') { + $fake_method_call = new PhpParser\Node\Expr\MethodCall( + $stmt->var, + new PhpParser\Node\Identifier('item', $stmt->var->getAttributes()), + [ + new PhpParser\Node\Arg($stmt->dim) + ] + ); + + $suppressed_issues = $statements_analyzer->getSuppressedIssues(); + + if (!in_array('PossiblyInvalidMethodCall', $suppressed_issues, true)) { + $statements_analyzer->addSuppressedIssues(['PossiblyInvalidMethodCall']); + } + + \Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer::analyze( + $statements_analyzer, + $fake_method_call, + $context + ); + + if (!in_array('PossiblyInvalidMethodCall', $suppressed_issues, true)) { + $statements_analyzer->removeSuppressedIssues(['PossiblyInvalidMethodCall']); + } + + $iterator_class_type = $fake_method_call->inferredType ?? null; + $array_access_type = $iterator_class_type ?: Type::getMixed(); + } elseif (strtolower($type->value) === 'arrayaccess' + || (($codebase->classExists($type->value) + && $codebase->classImplements($type->value, 'ArrayAccess')) + || ($codebase->interfaceExists($type->value) + && $codebase->interfaceExtends($type->value, 'ArrayAccess')) ) ) { - $non_array_types[] = (string)$type; + $fake_method_call = new PhpParser\Node\Expr\MethodCall( + $stmt->var, + new PhpParser\Node\Identifier('offsetGet', $stmt->var->getAttributes()), + [ + new PhpParser\Node\Arg($stmt->dim) + ] + ); + + $suppressed_issues = $statements_analyzer->getSuppressedIssues(); + + if (!in_array('PossiblyInvalidMethodCall', $suppressed_issues, true)) { + $statements_analyzer->addSuppressedIssues(['PossiblyInvalidMethodCall']); + } + + \Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer::analyze( + $statements_analyzer, + $fake_method_call, + $context + ); + + if (!in_array('PossiblyInvalidMethodCall', $suppressed_issues, true)) { + $statements_analyzer->removeSuppressedIssues(['PossiblyInvalidMethodCall']); + } + + $iterator_class_type = $fake_method_call->inferredType ?? null; + $array_access_type = $iterator_class_type ?: Type::getMixed(); } else { - $array_access_type = Type::getMixed(); + $non_array_types[] = (string)$type; } } else { $non_array_types[] = (string)$type; @@ -817,7 +869,7 @@ class ArrayFetchAnalyzer . (is_int($key_value) ? $key_value : '\'' . $key_value . '\''); } - if ($has_valid_offset && $inside_isset) { + if ($has_valid_offset && $context->inside_isset) { // do nothing } elseif ($has_valid_offset) { if (IssueBuffer::accepts( diff --git a/src/Psalm/Internal/Codebase/ClassLikes.php b/src/Psalm/Internal/Codebase/ClassLikes.php index 7faa4bd34..92c62b80a 100644 --- a/src/Psalm/Internal/Codebase/ClassLikes.php +++ b/src/Psalm/Internal/Codebase/ClassLikes.php @@ -453,10 +453,6 @@ class ClassLikes return true; } - if ($interface_id === 'arrayaccess' && $fq_class_name === 'domnodelist') { - return true; - } - if (isset(ClassLikeAnalyzer::SPECIAL_TYPES[$interface_id]) || isset(ClassLikeAnalyzer::SPECIAL_TYPES[$fq_class_name]) ) { diff --git a/tests/ArrayAccessTest.php b/tests/ArrayAccessTest.php index 43243b346..aa0802ac8 100644 --- a/tests/ArrayAccessTest.php +++ b/tests/ArrayAccessTest.php @@ -148,15 +148,17 @@ class ArrayAccessTest extends TestCase 'loadXML(""); - $doc->getElementsByTagName("node")[0];' + $e = $doc->getElementsByTagName("node")[0];', + [ + '$e' => 'null|DOMElement', + ] ], 'getOnArrayAcccess' => [ ' $a */ + function foo(ArrayAccess $a) : string { + return $a[0]; }', - 'assertions' => [], - 'error_levels' => ['MixedArgument'], ], 'mixedKeyMixedOffset' => [ 'id,