Skip to content

An initial attempt at bug #4926 - WIP #5130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
rlerdorf wants to merge 1 commit into
base: v6
Choose a base branch
from
Draft

An initial attempt at bug #4926 - WIP #5130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 50 additions & 5 deletions src/Phan/AST/UnionTypeVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2086,8 +2086,39 @@ private function resolveArrayShapeElementTypes(Node $node, UnionType $union_type
return null;
}
if ($resulting_element_type === false) {
// Fix for issue #4926: Don't emit error if some array shapes in the union don't have the key
// Check if ANY type (including PHPDoc types) could have the key
$could_have_key_phpdoc = false;
$could_have_key_real = self::couldRealTypesHaveKey($union_type->getRealTypeSet(), $dim_value);

// Also check PHPDoc types - resolveArrayShapeElementTypesForOffset returns false
// when it loops through PHPDoc types and finds none with the key, but there might
// be generic arrays in the real types
foreach ($union_type->getTypeSet() as $type) {
if ($type instanceof ArrayShapeType) {
if (isset($type->getFieldTypes()[$dim_value])) {
$could_have_key_phpdoc = true;
break;
}
} elseif ($type instanceof ListType) {
// ListType can only have integer keys >= 0
$filtered = \is_int($dim_value) ? $dim_value : \filter_var($dim_value, \FILTER_VALIDATE_INT);
if (\is_int($filtered) && $filtered >= 0) {
$could_have_key_phpdoc = true;
break;
}
} elseif (!($type instanceof ArrayType) || $type instanceof GenericArrayType) {
// Non-array-shape types (generic arrays, etc.) could have any key
$could_have_key_phpdoc = true;
break;
}
}

$could_have_key = $could_have_key_phpdoc || $could_have_key_real;

// XXX not sure what to do here. For now, just return null and only warn in cases where requested to.
if ($check_invalid_dim) {
if ($check_invalid_dim && !$could_have_key) {
// Only emit error if NO type in the union could possibly have this key
$exception = new IssueException(
Issue::fromType(Issue::TypeInvalidDimOffset)(
$this->context->getFile(),
Expand All @@ -2103,7 +2134,7 @@ private function resolveArrayShapeElementTypes(Node $node, UnionType $union_type
}
// $union_type is exclusively array shape types, but those don't contain the field $dim_value.
// It's undefined (which becomes null)
if (self::couldRealTypesHaveKey($union_type->getRealTypeSet(), $dim_value)) {
if ($could_have_key) {
return NullType::instance(false)->asPHPDocUnionType();
}
return NullType::instance(false)->asRealUnionType();
Expand Down Expand Up @@ -2173,10 +2204,24 @@ public static function resolveArrayShapeElementTypesForOffset(UnionType $union_t
$resulting_element_type = StringType::instance(false)->asPHPDocUnionType();
}
}
} elseif ($type->isArrayLike($code_base) || $type->isObject() || $type instanceof MixedType) {
if ($type instanceof ListType && (!\is_numeric($dim_value) || $dim_value < 0)) {
continue;
} elseif ($type instanceof ListType) {
// Fix for issue #4926: Extract element type from ListType
$filtered = \is_int($dim_value) ? $dim_value : \filter_var($dim_value, \FILTER_VALIDATE_INT);
if (\is_int($filtered) && $filtered >= 0) {
// Valid int index for list - extract element type
// ListType extends GenericArrayType which has genericArrayElementType() method
$element_type = $type->genericArrayElementType();
$element_union_type = $element_type->asPHPDocUnionType();
if (!$element_union_type->isEmpty()) {
if ($resulting_element_type instanceof UnionType) {
$resulting_element_type = $resulting_element_type->withUnionType($element_union_type);
} else {
$resulting_element_type = $element_union_type;
}
}
}
continue;
} elseif ($type->isArrayLike($code_base) || $type->isObject() || $type instanceof MixedType) {
if ($is_computing_real_type_set) {
// Avoid false positives for real type checking.
// TODO: Improve handling for GenericArrayType, strings, etc.
Expand Down
59 changes: 55 additions & 4 deletions src/Phan/Analysis/AssignmentVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,10 +883,61 @@ public function visitDim(Node $node): Context
}

if ($dim_type !== null && !\is_object($dim_value)) {
// TODO: This is probably why Phan has bugs with multi-dimensional assignment adding new union types instead of combining with existing ones.
$right_type = ArrayShapeType::fromFieldTypes([
$dim_value => $this->right_type,
], false)->asRealUnionType();
// Check the base expression's type to decide between ArrayShapeType and generic array
// Fix for issue #4926: Don't create restrictive array shapes for mixed/unknown types
$expr_union_type = UnionTypeVisitor::unionTypeFromNode(
$this->code_base,
$this->context,
$expr_node,
false
);

// Check if we should use array shape or generic array
// Fix for issue #4926: For mixed/unknown base types, use mixed element type
// to avoid false positives when accessing different fields
$has_mixed_type = !$expr_union_type->isEmpty() && $expr_union_type->hasMixedOrNonEmptyMixedType();

// Check if the base comes from an external mixed/generic array source
// vs. being a new structure we're creating (like GLOBALS['a'])
$use_generic_for_mixed = false;
if ($has_mixed_type) {
// Find the root variable to check if it's from an external source
$root_node = $expr_node;
while ($root_node instanceof Node && $root_node->kind === \ast\AST_DIM) {
$root_node = $root_node->children['expr'];
}
if ($root_node instanceof Node && $root_node->kind === \ast\AST_VAR) {
$root_var_name = (new ContextNode($this->code_base, $this->context, $root_node))->getVariableName();
// Hardcoded variables like $GLOBALS should use array shapes
if (!Variable::isHardcodedVariableInScopeWithName($root_var_name, $this->context->isInGlobalScope())) {
$root_type = UnionTypeVisitor::unionTypeFromNode(
$this->code_base,
$this->context,
$root_node,
false
);
// Use generic array if root has generic/mixed array types (external source)
// Use array shape if root is undefined/empty (creating new structure)
$use_generic_for_mixed = !$root_type->isEmpty() &&
($root_type->hasGenericArray() || $root_type->hasMixedOrNonEmptyMixedType());
}
}
}

if (!$has_mixed_type || !$use_generic_for_mixed) {
// Base is not mixed OR we're creating a new structure → create array shape
$right_type = ArrayShapeType::fromFieldTypes([
$dim_value => $this->right_type,
], false)->asRealUnionType();
} else {
// Base is mixed from external source (json_decode, etc.) → use generic array
// This prevents false positives when the same array is accessed with different keys
$key_type_enum = GenericArrayType::keyTypeFromUnionTypeValues($dim_type);
$right_type = GenericArrayType::fromElementType(MixedType::instance(false), false, $key_type_enum)->asRealUnionType();
if (!$right_type->hasRealTypeSet()) {
$right_type = $right_type->withRealTypeSet(UnionType::typeSetFromString('non-empty-array'));
}
}
} else {
// Make the right type a generic (i.e. int -> int[])
if ($dim_node !== null) {
Expand Down
Empty file added tests/files/expected/4926_array_mixed_access.php.expected
View file
Open in desktop
Empty file.
25 changes: 25 additions & 0 deletions tests/files/src/4926_array_mixed_access.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

// Test for issue #4926: False positive when assigning to different fields
// of arrays with mixed type (e.g., from json_decode)

class TestMixedArrayAccess {
public function testForeach(string $json) {
$arr = json_decode($json, true);
foreach ($arr as $row) {
// Both assignments should work without warnings
$row["x"] = strlen($row["x"]);
$row["y"] = strlen($row["y"]);
}
}

public function testWithAnnotation(string $json) {
// With explicit type annotation, should work correctly
$arr = json_decode($json, true);
'@phan-var list<array{x:string,y:string}> $arr';
foreach ($arr as $row) {
$row["x"] = strlen($row["x"]);
$row["y"] = strlen($row["y"]);
}
}
}
Loading