Skip to content

Commit 86062c7

Browse files
committed
Fix multi-namespace type resolution and foreach narrowing bugs
1 parent b2a2070 commit 86062c7

9 files changed

Lines changed: 381 additions & 57 deletions

File tree

docs/CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- **Multi-namespace function return type resolution.** In files with multiple `namespace { }` blocks, function return types were resolved against the first namespace instead of the function's own namespace. This caused incorrect type inference for variables assigned from function calls in later namespace blocks.
13+
- **Short class name resolution in type hints.** When resolving an unqualified class name from a property or return type annotation, the resolver now prefers the class in the same namespace as the owning type before falling back to first-match.
14+
- **Foreach narrowing with break in else.** When a foreach body contained an `if` with an `else { break; }` branch, the variable state from the break path was not included in the post-loop type, causing the merged type to be too narrow.
15+
- **Foreach element type from untyped arrays.** Variables bound in a `foreach` over an untyped `array` (e.g. from a function returning bare `array`) now resolve to `mixed` instead of empty, so assignments from the loop variable propagate correctly.
16+
- **Foreach target type after non-empty literal array.** When iterating a non-empty literal array such as `["a", "b", "c"]`, the pre-loop sentinel value of the target variable (e.g. `null` from `$tag = null`) no longer survives as a possible post-loop type.
17+
1018
### Changed
1119

1220
- **Updated embedded phpstorm-stubs.** Generic annotations for `ArrayIterator`, `ArrayObject`, `SplDoublyLinkedList`, `SplQueue`, `SplStack`, `SplPriorityQueue`, `SplFixedArray`, `SplObjectStorage`, and `array_reduce` are now provided by upstream stubs directly, removing the need for runtime patches.

docs/todo/bugs.md

Lines changed: 5 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,18 @@ case-insensitive method lookup, and function-level `@template`
2121
inference through generic wrapper params are now fixed.
2222
Remaining gaps:
2323

24-
- **Function name resolution in multi-namespace files**: bare
25-
function names in namespaced code are not resolved to their FQN,
26-
so function-level template inference fails in single-file tests
27-
with multiple namespaces. Works correctly in real projects.
24+
- **Multi-namespace class shadowing in `IteratorAggregate` resolution**:
25+
When a multi-namespace file defines a class like `SomeIterator` in
26+
an earlier namespace, resolving `getIterator()` return type
27+
`ArrayIterator<K, V>` may pick up the wrong class.
2828
- **Method-level `@template` with `key-of<T>` bound and `T[K]` return**:
2929
`key-of<T>`, `value-of<T>`, and `T[K]` now evaluate correctly after
3030
class-level template substitution. However, inferring a method-level
3131
template parameter `K` from a string literal argument (to resolve
3232
`T[K]` at a specific call site) is not yet supported.
3333

3434
**Tests:** SKIPs in `tests/psalm_assertions/template_class_template_extends.php`
35-
(lines 177, 227, 427, 500, 682, 843 (namespace)).
35+
(lines 427, 500, 682, 980, 981).
3636

3737

3838
## B14. Template/generic resolution in namespace-level and complex scenarios
@@ -65,37 +65,6 @@ only by this infrastructure limitation.
6565
640, 667, 701, 710, 788, 800).
6666

6767

68-
## B15. Loop exit type narrowing
69-
70-
**Discovered:** SKIP audit of `tests/psalm_assertions/loop_while.php`,
71-
`loop_foreach.php`.
72-
73-
Loop exit truthiness narrowing (e.g. `while ($a) { ... }` narrowing
74-
`$a` to `null` after the loop) works for do-while loops but not for
75-
while loops when the variable is reassigned inside the loop body.
76-
Remaining issues:
77-
78-
- While loop exit narrowing not applied to variables reassigned
79-
inside the loop body (e.g. `$a = $a->parent` in `while ($a)`
80-
should yield `null` after exit)
81-
- `instanceof` loop exit narrowing does not strip the matched type
82-
(e.g. `while ($a instanceof A)` should narrow to `B` for `A|B`)
83-
- Property access on loop-narrowed variable returns no type
84-
- `foreach` loop variable not narrowed away from initial `null`
85-
after non-empty iteration (would require tracking iterable
86-
emptiness)
87-
- Break-in-else branch type not merged with loop variable
88-
- Assignment from untyped array value inside null check not
89-
widened to `mixed`
90-
91-
Related to T20 (type narrowing reconciliation engine) and T29
92-
(definite vs possible variable existence tracking).
93-
94-
**Tests:** SKIPs in `loop_while.php` (lines 39, 67, 91, 115, 152),
95-
`loop_foreach.php` (lines 81, 128, 156, 188, 208).
96-
97-
98-
9968

10069
## B16. PDOStatement fetch mode-dependent return types
10170

src/completion/types/resolution.rs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -298,9 +298,25 @@ fn resolve_named_type(
298298
};
299299

300300
// ── Class lookup ───────────────────────────────────────────────
301-
let found = find_class_by_name(all_classes, name)
302-
.map(Arc::clone)
303-
.or_else(|| class_loader(name));
301+
// When `name` is a short name (no backslash), prefer a class in the
302+
// same namespace as the owning class. This prevents cross-namespace
303+
// contamination in multi-namespace files where multiple namespaces
304+
// define a class with the same short name.
305+
let found = if !name.contains('\\') && owning_class_name.contains('\\') {
306+
let owning_ns = owning_class_name.rsplit_once('\\').map(|(ns, _)| ns);
307+
// Try same-namespace first.
308+
let same_ns = all_classes
309+
.iter()
310+
.find(|c| c.name == name && c.file_namespace.as_deref() == owning_ns)
311+
.map(Arc::clone);
312+
same_ns
313+
.or_else(|| find_class_by_name(all_classes, name).map(Arc::clone))
314+
.or_else(|| class_loader(name))
315+
} else {
316+
find_class_by_name(all_classes, name)
317+
.map(Arc::clone)
318+
.or_else(|| class_loader(name))
319+
};
304320

305321
match found {
306322
Some(cls) => {

src/completion/variable/forward_walk.rs

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5523,13 +5523,24 @@ fn process_if_statement_body<'b>(
55235523
if !then_exits || then_exits_via_loop {
55245524
surviving_scopes.push(&then_scope);
55255525
}
5526-
for (ei_scope, ei_exits) in &elseif_scopes {
5527-
if !ei_exits {
5526+
for (idx, (ei_scope, ei_exits)) in elseif_scopes.iter().enumerate() {
5527+
if !ei_exits
5528+
|| body
5529+
.else_if_clauses
5530+
.iter()
5531+
.nth(idx)
5532+
.is_some_and(|ei| exits_via_loop_control(ei.statement))
5533+
{
55285534
surviving_scopes.push(ei_scope);
55295535
}
55305536
}
55315537
if let Some(ref es) = else_scope {
5532-
if !else_exits {
5538+
if !else_exits
5539+
|| body
5540+
.else_clause
5541+
.as_ref()
5542+
.is_some_and(|ec| exits_via_loop_control(ec.statement))
5543+
{
55335544
surviving_scopes.push(es);
55345545
}
55355546
} else {
@@ -6074,6 +6085,7 @@ fn process_foreach<'b>(foreach: &'b Foreach<'b>, scope: &mut ScopeState, ctx: &F
60746085
let foreach_offset = foreach.foreach.span().start.offset as usize;
60756086
let before = &ctx.content[..foreach_offset.min(ctx.content.len())];
60766087
let trimmed = before.trim_end();
6088+
let mut docblock_matched = false;
60776089
if trimmed.ends_with("*/")
60786090
&& let Some(doc_start) = trimmed.rfind("/**")
60796091
{
@@ -6088,8 +6100,16 @@ fn process_foreach<'b>(foreach: &'b Foreach<'b>, scope: &mut ScopeState, ctx: &F
60886100
{
60896101
let resolved = resolve_type_to_resolved_types(&php_type, ctx);
60906102
scope.set(vn, resolved);
6103+
docblock_matched = true;
60916104
}
60926105
}
6106+
// When the iterable is a bare `array` (no generic parameters)
6107+
// and no @var docblock provided a concrete type, the element
6108+
// type is `mixed`. Seed it so that assignments from the loop
6109+
// variable propagate `mixed` correctly through the body.
6110+
if !docblock_matched && iter_type.as_ref().is_some_and(|it| it.is_bare_array()) {
6111+
scope.set(vn, vec![ResolvedType::from_type_string(PhpType::mixed())]);
6112+
}
60936113
}
60946114

60956115
// ── Assignment-depth-bounded loop iteration ─────────────────
@@ -6168,6 +6188,24 @@ fn process_foreach<'b>(foreach: &'b Foreach<'b>, scope: &mut ScopeState, ctx: &F
61686188
*scope = pre_loop_scope;
61696189
scope.merge_branch(&post_loop);
61706190

6191+
// When the iterable is a non-empty literal array (e.g. `["a", "b",
6192+
// "c"]`), the loop body is guaranteed to execute at least once.
6193+
// The pre-loop sentinel value (e.g. `null` from `$tag = null`) must
6194+
// not survive as a possible post-loop type for the foreach target
6195+
// variable — override it with the post-loop value from the body walk.
6196+
if is_non_empty_array_literal(foreach.expression) {
6197+
let target_var = match &foreach.target {
6198+
ForeachTarget::Value(val) => extract_foreach_var_name(val.value),
6199+
ForeachTarget::KeyValue(kv) => extract_foreach_var_name(kv.value),
6200+
};
6201+
if let Some(ref vn) = target_var
6202+
&& let Some(post_val) = post_loop.locals.get(&ustr::ustr(vn.as_str()))
6203+
&& !post_val.is_empty()
6204+
{
6205+
scope.set(vn, post_val.clone());
6206+
}
6207+
}
6208+
61716209
leave_loop(loop_depth);
61726210
}
61736211

@@ -6545,6 +6583,20 @@ fn bind_foreach_value<'b>(
65456583
}
65466584
}
65476585

6586+
/// Returns `true` when `expr` is a non-empty array literal such as
6587+
/// `["a", "b", "c"]` or `array(1, 2, 3)`.
6588+
///
6589+
/// Used by `process_foreach` to detect iterables that are guaranteed to
6590+
/// have at least one element, so that the pre-loop type of the target
6591+
/// variable does not survive into the post-loop scope.
6592+
fn is_non_empty_array_literal(expr: &Expression<'_>) -> bool {
6593+
match expr {
6594+
Expression::Array(arr) => !arr.elements.is_empty(),
6595+
Expression::LegacyArray(arr) => !arr.elements.is_empty(),
6596+
_ => false,
6597+
}
6598+
}
6599+
65486600
/// Extract the variable name from a foreach value expression, unwrapping
65496601
/// a leading `&` (by-reference) if present.
65506602
fn extract_foreach_var_name(expr: &Expression<'_>) -> Option<String> {

src/parser/ast_update.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,11 @@ impl Backend {
248248
for func in &mut functions {
249249
let skip_names: Vec<String> =
250250
func.template_params.iter().map(|a| a.to_string()).collect();
251-
let resolver = Self::build_type_resolver(&use_map, &namespace, &skip_names);
251+
// Use the function's own namespace (not the file-level one)
252+
// so that multi-namespace files resolve return types
253+
// against the correct namespace block.
254+
let func_ns = func.namespace.clone().or_else(|| namespace.clone());
255+
let resolver = Self::build_type_resolver(&use_map, &func_ns, &skip_names);
252256

253257
if let Some(ref ret) = func.return_type {
254258
let resolved = ret.resolve_names(&resolver);

0 commit comments

Comments
 (0)