Skip to content

Commit 9ea5bf8

Browse files
azjezzazjezz
committed
fix(analyzer): narrow parent array types when !isset() confirms key absence
closes #569 Signed-off-by: azjezz <[email protected]>
1 parent cdc68cc commit 9ea5bf8

File tree

5 files changed

+121
-28
lines changed

5 files changed

+121
-28
lines changed

‎crates/analyzer/src/expression/array_access.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ impl<'ast, 'arena> Analyzable<'ast, 'arena> for ArrayAccess<'arena> {
9797
mod tests {
9898
use indoc::indoc;
9999

100+
use crate::code::IssueCode;
100101
use crate::test_analysis;
101102

102103
test_analysis! {
@@ -122,4 +123,27 @@ mod tests {
122123
}
123124
"},
124125
}
126+
127+
test_analysis! {
128+
name = negated_isset_narrows_parent_array_type,
129+
code = indoc! {r#"
130+
<?php
131+
132+
/**
133+
* @return array{foo?: array{bar?: string}}
134+
*/
135+
function y(): array { return []; }
136+
137+
$y = y();
138+
139+
if (isset($y['foo']) && !isset($y['foo']['bar'])) {
140+
echo $y['foo']['bar'];
141+
echo $y['foo']['baz'];
142+
}
143+
"#},
144+
issues = [
145+
IssueCode::UndefinedStringArrayIndex, // $y['foo']['bar']
146+
IssueCode::UndefinedStringArrayIndex, // $y['foo']['baz']
147+
],
148+
}
125149
}

‎crates/analyzer/src/reconciler/mod.rs‎

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,20 +184,31 @@ pub fn reconcile_keyed_types<'ctx>(
184184

185185
let result_type = result_type.unwrap_or_else(get_never);
186186

187+
let key_parts = break_up_path_into_parts(key_str);
188+
187189
if !did_type_exist && result_type.is_never() {
190+
// Even when the type doesn't exist and result is never, we still need to
191+
// update parent array types for negated isset/key_exists to remove the key
192+
if key_str.ends_with(']') && (has_inverted_isset || has_inverted_key_exists) {
193+
adjust_array_type_remove_key(key_parts.clone(), block_context, changed_var_ids);
194+
}
195+
188196
continue;
189197
}
190198

191199
let type_changed =
192200
if let Some(before_adjustment) = &before_adjustment { &result_type != before_adjustment } else { true };
193201

194-
let key_parts = break_up_path_into_parts(key_str);
195202
if type_changed {
196203
changed_var_ids.insert(*key);
197204

198205
if key_str.ends_with(']') && !has_inverted_isset && !has_inverted_key_exists && !has_empty && !is_equality {
199206
adjust_array_type(key_parts.clone(), block_context, changed_var_ids, &result_type);
200-
} else if key_str != "$this" {
207+
} else if key_str.ends_with(']') && (has_inverted_isset || has_inverted_key_exists) {
208+
adjust_array_type_remove_key(key_parts.clone(), block_context, changed_var_ids);
209+
}
210+
211+
if key_str != "$this" {
201212
let mut removable_keys: Vec<Atom> = Vec::new();
202213
for new_key in block_context.locals.keys() {
203214
if new_key == key {
@@ -349,6 +360,79 @@ fn adjust_array_type(
349360
context.locals.insert(base_key_atom, Rc::new(existing_type));
350361
}
351362

363+
fn adjust_array_type_remove_key(
364+
mut key_parts: Vec<String>,
365+
context: &mut BlockContext<'_>,
366+
changed_var_ids: &mut AtomSet,
367+
) {
368+
key_parts.pop();
369+
let Some(array_key) = key_parts.pop() else {
370+
return;
371+
};
372+
373+
key_parts.pop();
374+
375+
if array_key.starts_with('$') {
376+
return;
377+
}
378+
379+
let mut has_string_offset = false;
380+
381+
let arraykey_offset = if array_key.starts_with('\'') || array_key.starts_with('\"') {
382+
has_string_offset = true;
383+
array_key[1..(array_key.len() - 1)].to_string()
384+
} else {
385+
array_key.clone()
386+
};
387+
388+
let base_key = key_parts.join("");
389+
let base_key_atom = atom(&base_key);
390+
391+
let mut existing_type = if let Some(existing_type) = context.locals.get(&base_key_atom) {
392+
(**existing_type).clone()
393+
} else {
394+
return;
395+
};
396+
397+
for base_atomic_type in existing_type.types.to_mut() {
398+
match base_atomic_type {
399+
TAtomic::Array(TArray::Keyed(TKeyedArray { known_items, .. })) => {
400+
let dictkey = if has_string_offset {
401+
ArrayKey::String(atom(&arraykey_offset))
402+
} else if let Ok(arraykey_value) = arraykey_offset.parse::<i64>() {
403+
ArrayKey::Integer(arraykey_value)
404+
} else {
405+
continue;
406+
};
407+
408+
if let Some(known_items) = known_items {
409+
known_items.remove(&dictkey);
410+
}
411+
}
412+
TAtomic::Array(TArray::List(TList { known_elements, .. })) => {
413+
if let Ok(arraykey_offset) = arraykey_offset.parse::<usize>()
414+
&& let Some(known_elements) = known_elements
415+
{
416+
known_elements.remove(&arraykey_offset);
417+
}
418+
}
419+
_ => {
420+
continue;
421+
}
422+
}
423+
424+
changed_var_ids.insert(concat_atom!(&base_key, "[", &array_key, "]"));
425+
426+
if let Some(last_part) = key_parts.last()
427+
&& last_part == "]"
428+
{
429+
adjust_array_type(key_parts.clone(), context, changed_var_ids, &wrap_atomic(base_atomic_type.clone()));
430+
}
431+
}
432+
433+
context.locals.insert(base_key_atom, Rc::new(existing_type));
434+
}
435+
352436
fn refine_array_key(key_type: &TUnion) -> TUnion {
353437
fn refine_array_key_inner(key_type: &TUnion) -> Option<TUnion> {
354438
let mut refined = false;

‎crates/analyzer/tests/cases/issue_557_part_1.php‎

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,18 @@ function expectsArray(array $value): void
2323
$x = [];
2424

2525
// Test 1: Direct negated isset check
26-
// When !isset($x['foo']) is true, $x['foo'] should be null
26+
// When !isset($x['foo']) is true, we know 'foo' definitely doesn't exist
2727
if (!isset($x['foo'])) {
28-
/**
29-
* @mago-expect analysis:possibly-undefined-string-array-index
30-
* @mago-expect analysis:possibly-invalid-argument
31-
*/
28+
// @mago-expect analysis:undefined-string-array-index
3229
expectsNull($x['foo']);
3330
}
3431

3532
// Test 2: Negated isset check via else branch
36-
// The else branch uses negation internally as well
33+
// The else branch means isset returned false, so key doesn't exist
3734
if (isset($x['foo'])) {
3835
expectsString($x['foo']);
3936
} else {
40-
/**
41-
* @mago-expect analysis:possibly-undefined-string-array-index
42-
* @mago-expect analysis:possibly-invalid-argument
43-
*/
37+
// @mago-expect analysis:undefined-string-array-index
4438
expectsNull($x['foo']);
4539
}
4640

@@ -49,12 +43,8 @@ function expectsArray(array $value): void
4943
$y = [];
5044

5145
if (!isset($y['foo']['bar'])) {
52-
/**
53-
* @mago-expect analysis:possibly-undefined-string-array-index
54-
* @mago-expect analysis:possibly-undefined-string-array-index
55-
* @mago-expect analysis:possibly-null-array-access
56-
* @mago-expect analysis:possibly-invalid-argument
57-
*/
46+
// After !isset($y['foo']['bar']), we know 'bar' doesn't exist in $y['foo']
47+
// @mago-expect analysis:undefined-string-array-index
5848
expectsNull($y['foo']['bar']);
5949
} else {
6050
expectsArray($y['foo']);
@@ -63,12 +53,7 @@ function expectsArray(array $value): void
6353
if (isset($y['foo']['bar'])) {
6454
expectsString($y['foo']['bar']);
6555
} else {
66-
/**
67-
* @mago-expect analysis:possibly-undefined-string-array-index
68-
* @mago-expect analysis:possibly-undefined-string-array-index
69-
* @mago-expect analysis:possibly-null-array-access
70-
* @mago-expect analysis:possibly-invalid-argument
71-
*/
56+
// @mago-expect analysis:undefined-string-array-index
7257
expectsNull($y['foo']['bar']);
7358
}
7459

‎crates/analyzer/tests/cases/issue_557_part_2.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,14 @@
44
$x = [];
55

66
if (!isset($x['foo'])) {
7-
// @mago-expect analysis:possibly-undefined-string-array-index
7+
// @mago-expect analysis:undefined-string-array-index
88
echo $x['foo'];
99
}
1010

1111
if (isset($x['foo'])) {
1212
// This is correctly diagnosed as always being a string
1313
echo $x['foo'];
1414
} else {
15-
// @mago-expect analysis:possibly-undefined-string-array-index
15+
// @mago-expect analysis:undefined-string-array-index
1616
echo $x['foo'];
1717
}

‎crates/analyzer/tests/cases/issue_570.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
function x1(array $y): void
99
{
1010
if (isset($y['foo']) && !isset($y['foo']['bar'])) {
11-
echo $y['foo']['bar']; // @mago-expect analysis:possibly-undefined-string-array-index
11+
echo $y['foo']['bar']; // @mago-expect analysis:undefined-string-array-index
1212
}
1313
}
1414

@@ -18,6 +18,6 @@ function x1(array $y): void
1818
function x2(array $y): void
1919
{
2020
if (!isset($y['foo']['bar']) && isset($y['foo'])) {
21-
echo $y['foo']['bar']; // @mago-expect analysis:possibly-undefined-string-array-index
21+
echo $y['foo']['bar']; // @mago-expect analysis:undefined-string-array-index
2222
}
2323
}

0 commit comments

Comments
 (0)