array_filter() reports the type the filter leaves behind

Author: AJenbo

docs/CHANGELOG.md

@@ -107,6 +107,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **`array_filter()` reports the type the filter leaves behind.** A callback that tests the value it is handed proves something about every entry that survives, but the result kept whatever element type went in, so `array_filter($values, fn ($v) => $v !== null)` still looked like it could hold `null` and returning it from a function declared `int[]` was reported as a type error. The surviving values are now narrowed the way the body of an `if` narrows the variable it guards, whether the test is written as an `is_…()` call, a comparison against `null`, an `instanceof` check, or a callable string such as `'is_int'`. The keys were already narrowed this way in the modes that hand the callback a key, and both halves narrow together under `ARRAY_FILTER_USE_BOTH`. Closes #376.
 - **A deprecation warning belongs to the variable it is written on.** The deprecated-usage check typed its subject from a cache keyed by variable name and class, so two methods of the same class that reuse a parameter name shared one entry and whichever type was bound first won. A Laravel controller with a `Request $request` method above a `PendingRequest $request` method reported `Illuminate\Http\Request::get is deprecated` on the HTTP client call, where `get()` is the ordinary way to make a request, and renaming either parameter made the warning vanish. Every subject is now typed in the scope it is written in, so each method, closure, and `instanceof` branch gets its own answer, and a genuine deprecation is still reported no matter which order the methods appear in.
 - **A generic argument left out takes the default its `@template` declares.** `@template TAsync of bool = false` says that a use of the class without a generic argument means `false`, but the parameter was widened to its upper bound instead, so a conditional return keyed on it always picked the else branch. Laravel's HTTP client is where this shows up: `PendingRequest` declares synchronous mode as its default, so an ordinary `Http::get()` looked like it returned a promise rather than a `Response`, and `json()` along with the rest of the response API appeared to be missing on it. A plain request now resolves to `Response` whether it is made through `PendingRequest`, the client factory, or the `Http` facade, and `async()` still resolves to `PromiseInterface` through all three. Contributed by @shuvroroy (#377).
 - **A conditional assertion narrows the value the call was written on.** `if (filled($search))` left a `?string` nullable inside the branch, so passing it on to something that expects a `string` was reported as an error. Two things stood in the way, and Laravel's `filled()` and `blank()` hit both. An asserted type written as a union (`!=null|''`, which is how the pair is annotated) matched no type guard at all and narrowed nothing; ruling one out now rules out each of its members on its own. And a tag written in the equality form was being inverted into the branch it does not name, which typed every filled value as `numeric|bool`. Those tags promise something in one direction only, so they are now left out of the opposite branch, while the subtype form (`!null`) keeps narrowing both. Closes #375.

docs/todo.md

@@ -102,6 +102,7 @@ unlikely to move the needle for most users.
 | T10 | [Ternary expression as RHS of list destructuring](todo/type-inference.md#t10-ternary-expression-as-rhs-of-list-destructuring)                                               | Low         | Medium      |
 | T11 | [Nested list destructuring](todo/type-inference.md#t11-nested-list-destructuring)                                                                                           | Low         | Medium      |
 |     | **[Bugs](todo/bugs.md)**                                                                                                                                                    |             |             |
+| B181 | [`array_filter()` reports a `list` the filter cannot preserve](todo/bugs.md#b181-array_filter-reports-a-list-the-filter-cannot-preserve)                                    | Low-Medium  | Medium      |
 |     | **[Diagnostics](todo/diagnostics.md)**                                                                                                                                      |             |             |
 | D6  | [Unreachable code diagnostic](todo/diagnostics.md#d6-unreachable-code-diagnostic)                                                                                           | Low-Medium  | Medium      |
 | D16 | [`unreachable_match_arm` ignores literal subject types](todo/diagnostics.md#d16-unreachable_match_arm-ignores-literal-subject-types)                                        | Low-Medium  | Medium      |

docs/todo/bugs.md

@@ -37,7 +37,31 @@ No outstanding items.
 
 ## Array types
 
-No outstanding items.
+### B181. `array_filter()` reports a `list` the filter cannot preserve
+
+**Impact: Low-Medium · Complexity: Medium**
+
+`array_filter()` keeps the key of every entry it keeps, so filtering a
+`list` leaves gaps in the numbering and the result is `array<int, T>`
+rather than `list<T>`. PHPantom hands back the container it was given:
+
+```php
+/** @param list<int> $values */
+function probe(array $values): void {
+    $kept = array_filter($values, fn ($v) => $v > 3);  // reported as list<int>
+    $kept[0];  // [3, 4] filtered this way starts at key 1, so this is unset
+}
+```
+
+The over-claim runs both ways: reading `$kept[0]` looks safe when it is
+not, and a function declared `@return list<int>` that hands back an
+unwrapped `array_filter()` result is accepted where PHPStan reports it.
+The rule that rebuilds the container for the preserving family lives in
+`type_engine/variable/array_func_rules.rs`; `array_filter` needs to drop
+to `array<int, T>` there while the renumbering functions around it
+(`array_values`, `array_merge`) keep answering `list<T>`, and the demo
+files and tests that currently assert `list<…>` for a filtered list need
+updating with it.
 
 ## Docblock handling
 

examples/php/completion.php

@@ -3403,6 +3403,18 @@ public function keysAndValues(Scaffolding\ScaffoldingArrayFunc $src): void
         $stringKeyed = array_filter($src->mixedKeys(), fn($key) => is_string($key), ARRAY_FILTER_USE_KEY);
         strtoupper(array_keys($stringKeyed)[0]);   // list<string>: the int key is gone
 
+        // The default mode hands the callback the value instead, so a
+        // callback that tests it says as much about the entries that
+        // survive as the truthiness test above does.
+        $named = array_filter($src->optionalLabels(), fn($label) => $label !== null);
+        strtoupper($named['ink']);        // array<string, string>: the null half is gone
+
+        // An instanceof check filters for a type, and the result carries it.
+        // array_filter keeps the original keys, so array_values renumbers
+        // them before the first entry is read back.
+        $pens = array_values(array_filter($src->mixedWriters(), fn($writer) => $writer instanceof Scaffolding\Pen));
+        $pens[0]->write();                // list<Scaffolding\Pen>, so Pen's members are here
+
         // An all-int array cannot sum to a float.
         $total = array_sum($src->weights());
         intdiv($total, 1);                // int

examples/php/scaffolding/assertions.php

@@ -251,6 +251,10 @@ function runDemoAssertions(): void
     assert(array_key_first((new Scaffolding\ScaffoldingArrayFunc())->byName()) === 'blue', 'array_key_first() over a string-keyed array yields a string');
     $stringKeyed = array_filter((new Scaffolding\ScaffoldingArrayFunc())->mixedKeys(), fn($key) => is_string($key), ARRAY_FILTER_USE_KEY);
     assert(array_keys($stringKeyed) === ['ink'], 'array_filter() with ARRAY_FILTER_USE_KEY keeps only the keys its callback approves of');
+    $named = array_filter((new Scaffolding\ScaffoldingArrayFunc())->optionalLabels(), fn($label) => $label !== null);
+    assert($named === ['ink' => 'ink'], 'array_filter() with a value callback keeps only the entries it approves of, so no null survives');
+    $writers = array_values(array_filter((new Scaffolding\ScaffoldingArrayFunc())->mixedWriters(), fn($writer) => $writer instanceof Scaffolding\Pen));
+    assert(count($writers) === 2 && $writers[0] instanceof Scaffolding\Pen, 'array_filter() with an instanceof callback keeps only that class');
 
     // ── array<T>|false keeps its element type after a false check ────────
     $pens = Scaffolding\loadPensOrFail();

examples/php/scaffolding/scaffolding.php

@@ -768,6 +768,9 @@ public function optionalLabels(): array { return ['ink' => 'ink', 'gel' => null]
     /** @return array<string|int, string> */
     public function mixedKeys(): array { return ['ink' => 'gel', 7 => 'nib']; }
 
+    /** @return list<Pen|Pencil> */
+    public function mixedWriters(): array { return [new Pen('blue'), new Pencil(), new Pen('red')]; }
+
     /** @return list<int> */
     public function weights(): array { return [2, 3, 4]; }
 }

src/type_engine/types/narrowing/guards.rs

@@ -878,10 +878,71 @@ fn narrow_by_condition_inner(
         _ => {}
     }
 
+    // `$v instanceof Foo` is the type test a filter callback is most often
+    // written with, and `$v !== null` the one `is_null()` is spelled as
+    // when nobody reaches for a function call.
+    if let Some(extraction) = super::try_extract_instanceof_with_negation(condition, var_name) {
+        return narrow_type_by_instanceof(ty, &extraction, class_loader);
+    }
+    if let Some(expects_null) = try_extract_null_comparison(condition, var_name) {
+        return filter_type_by_guard(ty, TypeGuardKind::Null, expects_null, class_loader);
+    }
+
     let (kind, negated) = try_extract_type_guard(condition, var_name)?;
     filter_type_by_guard(ty, kind, !negated, class_loader)
 }
 
+/// Whether `expr` compares `var_name` against `null`, and whether passing
+/// it proves the value *is* null.
+///
+/// Only the strict operators prove a value is null: `$v == null` is also
+/// true for `''`, `0` and `[]`, so reporting `null` for it would claim
+/// more than the comparison shows. Proving a value is *not* null needs no
+/// such care, since anything that survives either `!==` or `!=` is
+/// non-null.
+fn try_extract_null_comparison(expr: &Expression<'_>, var_name: &str) -> Option<bool> {
+    match expr {
+        Expression::Parenthesized(inner) => try_extract_null_comparison(inner.expression, var_name),
+        Expression::UnaryPrefix(prefix) if prefix.operator.is_not() => {
+            // `!($v !== null)` proves the value is null, which only the
+            // strict operator it negates is allowed to say.
+            match try_extract_null_comparison(prefix.operand, var_name)? {
+                true => None,
+                false => Some(true),
+            }
+        }
+        Expression::Binary(bin) => {
+            let expects_null = match bin.operator {
+                BinaryOperator::Identical(_) => true,
+                BinaryOperator::NotIdentical(_) | BinaryOperator::NotEqual(_) => false,
+                _ => return None,
+            };
+            let (subject, literal) = if is_null_literal(bin.rhs) {
+                (bin.lhs, bin.rhs)
+            } else {
+                (bin.rhs, bin.lhs)
+            };
+            if !is_null_literal(literal) || expr_to_subject_key(subject)? != var_name {
+                return None;
+            }
+            Some(expects_null)
+        }
+        _ => None,
+    }
+}
+
+/// Whether `expr` is the `null` keyword.
+fn is_null_literal(expr: &Expression<'_>) -> bool {
+    match expr {
+        Expression::Parenthesized(inner) => is_null_literal(inner.expression),
+        Expression::Literal(Literal::Null(_)) => true,
+        Expression::ConstantAccess(access) => bytes_to_str(access.name.value())
+            .trim_start_matches('\\')
+            .eq_ignore_ascii_case("null"),
+        _ => false,
+    }
+}
+
 /// Try to extract a type-guard function call on a variable.
 ///
 /// Matches `is_array($var)`, `is_string($var)`, etc. (with optional

src/type_engine/types/narrowing/instanceof.rs

@@ -5,7 +5,7 @@
 use std::sync::Arc;
 
 use crate::atom::{atom, bytes_to_str, literal_bytes_to_str};
-use crate::php_type::PhpType;
+use crate::php_type::{PhpType, TypeKind};
 use crate::types::ClassInfo;
 
 use mago_syntax::cst::*;
@@ -1002,3 +1002,95 @@ fn collect_negated_and_instanceof_classes<'b>(
         }
     }
 }
+
+/// Narrow `ty` to the values an `instanceof`-style check keeps (or, when
+/// the check is negated, the ones it rejects).
+///
+/// Each member of the union answers on its own: one that already names a
+/// subtype of the checked class is the more specific of the two and
+/// stays as it is, one the checked class is a subtype of narrows to the
+/// class, and one that is unrelated (or not an object at all) cannot
+/// pass and is dropped.
+///
+/// Returns `None` when nothing can be said — no class loader, a class
+/// name that does not resolve, a `self`/`static`/`parent` reference the
+/// enclosing scope alone could resolve, or a check every member already
+/// satisfies.
+pub(in crate::type_engine) fn narrow_type_by_instanceof(
+    ty: &PhpType,
+    extraction: &InstanceofExtraction,
+    class_loader: GuardClassLoader<'_>,
+) -> Option<PhpType> {
+    let loader = class_loader?;
+    let class_name = extraction.class_type.class_name()?;
+    if extraction.class_type.is_self_like() || class_name.eq_ignore_ascii_case("parent") {
+        return None;
+    }
+    loader(class_name)?;
+
+    let members = instanceof_union_members(ty);
+    let kept: Vec<PhpType> = members
+        .iter()
+        .filter_map(|member| {
+            if extraction.negated {
+                // Ruling the class out leaves every member that is not one
+                // of its instances exactly as it was.
+                (!crate::class_lookup::is_subtype_of_named(member, class_name, loader))
+                    .then(|| member.clone())
+            } else {
+                instanceof_member(member, extraction, class_name, loader)
+            }
+        })
+        .collect();
+
+    if kept.is_empty() || kept == members {
+        return None;
+    }
+    // A lone survivor is that type, not a union of one: `PhpType::union`
+    // only collapses a single member when it deduplicated to get there.
+    match kept.len() {
+        1 => kept.into_iter().next(),
+        _ => Some(PhpType::union(kept)),
+    }
+}
+
+/// The alternatives a value of `ty` can take, with `?T` spelled out as
+/// the `T|null` it stands for so each half is judged separately.
+fn instanceof_union_members(ty: &PhpType) -> Vec<PhpType> {
+    match ty.kind() {
+        TypeKind::Nullable(inner) => vec![inner.clone(), PhpType::null()],
+        TypeKind::Union(members) => members.to_vec(),
+        _ => vec![ty.clone()],
+    }
+}
+
+/// What a positive `instanceof` check leaves of a single union member,
+/// or `None` when the member cannot pass it.
+fn instanceof_member(
+    member: &PhpType,
+    extraction: &InstanceofExtraction,
+    class_name: &str,
+    loader: &dyn Fn(&str) -> Option<Arc<ClassInfo>>,
+) -> Option<PhpType> {
+    if !member.is_object_like() {
+        // `null`, a scalar and an array all fail `instanceof` outright.
+        return None;
+    }
+    // A member that names no class of its own (`object`, `mixed`) is
+    // whatever the check proves.
+    let Some(member_name) = member.class_name().filter(|n| loader(n).is_some()) else {
+        return Some(extraction.class_type.clone());
+    };
+    // An exact identity check (`get_class($v) === Foo::class`) admits the
+    // class itself and nothing below it, so a member naming a subclass
+    // cannot pass.
+    if extraction.exact {
+        return crate::class_lookup::is_subtype_of_names(class_name, member_name, loader)
+            .then(|| extraction.class_type.clone());
+    }
+    if crate::class_lookup::is_subtype_of_named(member, class_name, loader) {
+        return Some(member.clone());
+    }
+    crate::class_lookup::is_subtype_of_names(class_name, member_name, loader)
+        .then(|| extraction.class_type.clone())
+}

src/type_engine/variable/array_func_rules.rs

@@ -111,11 +111,10 @@ pub(in crate::type_engine) fn array_func_raw_type(
                 if !args.has_arg(1) {
                     return Some(filter_element_type(&raw).unwrap_or(raw));
                 }
-                // A callback handed the key decides which keys survive, so
-                // what it asserts about them describes the result.
-                if let Some(narrowed) = filter_key_type(&raw, args) {
-                    return Some(narrowed);
-                }
+                // A callback decides which entries survive, so what it
+                // asserts about the value or the key it is handed
+                // describes the result.
+                return Some(filter_callback_type(&raw, args).unwrap_or(raw));
             }
             return Some(raw);
         }
@@ -398,21 +397,57 @@ fn filter_element_type(raw: &PhpType) -> Option<PhpType> {
     if truthy == *element {
         return None;
     }
+    with_element_type(raw, truthy)
+}
+
+/// Rebuild an iterable type around a new element type, keeping the
+/// container it already names.
+fn with_element_type(raw: &PhpType, element: PhpType) -> Option<PhpType> {
     match raw.kind() {
-        TypeKind::Array(_) => Some(PhpType::array_of(truthy)),
+        TypeKind::Array(_) => Some(PhpType::array_of(element)),
         TypeKind::Generic(g) if !g.args.is_empty() => {
             let mut args = g.args.clone();
             // Same `<TKey, TValue>` convention `extract_value_type` reads:
             // the value is the second argument when there are two or more,
             // and the lone argument otherwise (`list<V>`).
             let value_idx = if args.len() >= 2 { 1 } else { args.len() - 1 };
-            args[value_idx] = truthy;
+            args[value_idx] = element;
             Some(PhpType::generic_atom(g.name, args))
         }
         _ => None,
     }
 }
 
+/// Rebuild an `array_filter` result with what its callback proves about
+/// the entries it keeps.
+///
+/// The callback is handed the value, the key, or both depending on the
+/// mode argument, and each narrows the half it arrives in. Returns
+/// `None` when neither is narrowed.
+fn filter_callback_type(raw: &PhpType, args: &dyn ArrayFuncArgs) -> Option<PhpType> {
+    let narrowed_value = filter_value_type(raw, args);
+    let base = narrowed_value.as_ref().unwrap_or(raw);
+    filter_key_type(base, args).or(narrowed_value)
+}
+
+/// Rebuild an `array_filter` result with its element type narrowed to
+/// what the callback asserts about the value it was handed.
+///
+/// Returns `None` unless the call runs in one of the two modes that pass
+/// the value (the default and `ARRAY_FILTER_USE_BOTH`) and the callback
+/// proves something the element type does not already say.
+fn filter_value_type(raw: &PhpType, args: &dyn ArrayFuncArgs) -> Option<PhpType> {
+    let param_index = filter_value_param_index(args)?;
+    let element = raw.extract_value_type(false)?;
+    let narrowed = args.callback_param_narrowing(1, param_index, element)?;
+    // A callback that admits every value it could receive says nothing,
+    // and the rebuilt union would only reorder the members.
+    if element.is_subtype_of(&narrowed) {
+        return None;
+    }
+    with_element_type(raw, narrowed)
+}
+
 /// Rebuild an `array_filter` result with its key type narrowed to what
 /// the callback asserts about the key it was handed.
 ///
@@ -450,6 +485,23 @@ fn filter_key_type(raw: &PhpType, args: &dyn ArrayFuncArgs) -> Option<PhpType> {
     }
 }
 
+/// Which of the callback's parameters receives the value, from
+/// `array_filter`'s mode argument.
+///
+/// The default mode passes the value alone, and `ARRAY_FILTER_USE_BOTH`
+/// passes it ahead of the key. `ARRAY_FILTER_USE_KEY` never shows the
+/// callback a value, and a mode written as anything this cannot read
+/// might be that one.
+fn filter_value_param_index(args: &dyn ArrayFuncArgs) -> Option<usize> {
+    if !args.has_arg(2) {
+        return Some(0);
+    }
+    match args.arg_atom_text(2)?.as_str() {
+        "ARRAY_FILTER_USE_BOTH" | "1" | "0" => Some(0),
+        _ => None,
+    }
+}
+
 /// Which of the callback's parameters receives the key, from
 /// `array_filter`'s mode argument.
 ///