array_filter keeps what its callback proves about the keys

Author: AJenbo

docs/CHANGELOG.md

@@ -121,6 +121,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **`array_filter` keeps what its callback proves about the keys.** In the two modes that hand the callback the key (`ARRAY_FILTER_USE_KEY` and `ARRAY_FILTER_USE_BOTH`) the result was reported with the key type it went in with, so `array_filter($data, fn ($k) => is_string($k), ARRAY_FILTER_USE_KEY)` still claimed integer keys the call exists to remove, and passing it on to a parameter declared `array<string, …>` was reported as a mismatch. The keys that survive are now read off the callback the same way an `if (is_string($k))` body is read, whether it is written inline or named (`'is_string'`), and a callback that admits every key it could receive leaves the type alone.
 - **An element write refines what the array already holds.** A write into a variable whose keys were already tracked was thrown away rather than recorded: `$row[] = $pen` on a `array{name: string}` left the shape exactly as it was, and so did `$row[$key] = 1`, so the value that had just been written was not there to read back. Both are now applied. An append takes the next free integer key beside the keys already tracked, an append one level down extends what that key holds instead of leaving it at the value it was initialised with, and a write through a key only known at runtime widens the shape to the keys and values it and the existing entries describe together, since a runtime key may land on any of them. The reverse mistake is gone too: writing a literal key into an array declared by key and value type (`array<string, int>`) rebuilt it as a shape holding that one key, discarding every other key it was known to hold, and appending to a string-keyed array called the result a `list`. Both now keep the array's key and value types and fold the written pair into them.
 - **`+=` no longer forgets what the right side contributes.** Where the left side had no tracked type, `$config += ['slot' => $default]` produced a bare `array`, so the key it had just added was not there to complete or hover on. `+` only accepts arrays, so whatever the left side held was one, and the result now carries what the right side contributes. Two positional arrays union index by index as PHP does, rather than standing down because their entries are written without keys.
 - **An array shape answers the `list` and `non-empty-array` promises from its own entries.** A shape was compared against those types by name alone, so `array{}` satisfied a `non-empty-array` parameter and `array{name: string}` satisfied a `list`, while a real list of values written as `array{string, int}` did not satisfy `list`. A shape is now non-empty when it names a key that is always there, and a list when its keys run `0, 1, 2, …` in order with any optional entry at the end.

docs/todo.md

@@ -99,7 +99,6 @@ unlikely to move the needle for most users.
 | D17 | [`docblock_native_mismatch` only judges nullability](todo/diagnostics.md#d17-docblock_native_mismatch-only-judges-nullability)                                              | Low         | Medium      |
 |     | **[Bug Fixes](todo/bugs.md)**                                                                                                                                               |             |             |
 | B151 | [`?T` and `T\|null` are judged by different rules](todo/bugs.md#b151-t-and-tnull-are-judged-by-different-rules)                                                            | High        | Low-Medium  |
-| B152 | [`array_filter` with `ARRAY_FILTER_USE_KEY` does not narrow the key type](todo/bugs.md#b152-array_filter-with-array_filter_use_key-does-not-narrow-the-key-type)           | Low-Medium  | Medium      |
 |     | **[Code Actions](todo/actions.md)**                                                                                                                                         |             |             |
 | A40 | [Generate method from call](todo/actions.md#a40-generate-method-from-call)                                                                                                  | Medium-High | Medium      |
 | A41 | [Create class from non-existing name](todo/actions.md#a41-create-class-from-non-existing-name)                                                                              | Medium      | Medium      |

docs/todo/bugs.md

@@ -76,22 +76,4 @@ have been guarded out) rather than removing it outright.
 
 ## Array types
 
-### B152. `array_filter` with `ARRAY_FILTER_USE_KEY` does not narrow the key type
-
-**Impact: Low-Medium · Effort: Medium**
-
-```php
-/** @return array<string> $data */
-$data = array_filter($data, fn (string|int $k): bool => is_string($k), ARRAY_FILTER_USE_KEY);
-$data = $this->viewData($view) + $data;    // reported: array<string|int, string>
-```
-
-`array_filter` preserves its input type verbatim, so the callback's
-proof about the keys is dropped. That is invisible on its own, but the
-key type surfaces the moment the result is merged with `+` or passed to
-a parameter declared `array<string, …>` (2 sites in Bladestan). The
-`ARRAY_FILTER_USE_BOTH` mode has the same gap for the key half.
-
-**Fix:** read the callback's assertions about its key parameter (the
-same reconciliation an `if (is_string($k))` body already gets) and
-rebuild the result's key type from what survives.
+No outstanding items.

examples/php/completion.php

@@ -3163,6 +3163,11 @@ public function keysAndValues(Scaffolding\ScaffoldingArrayFunc $src): void
         $present = array_filter($src->optionalLabels());
         strtoupper($present['ink']);      // array<string, string>
 
+        // ARRAY_FILTER_USE_KEY hands the callback the key, so what the
+        // callback proves about it describes the keys that survive.
+        $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
+
         // An all-int array cannot sum to a float.
         $total = array_sum($src->weights());
         intdiv($total, 1);                // int

examples/php/scaffolding/assertions.php

@@ -209,6 +209,8 @@ function runDemoAssertions(): void
     assert(array_keys((new Scaffolding\ScaffoldingArrayFunc())->labels()) === [0, 1], 'array_keys() over a list yields int keys');
     assert(array_search('gel', (new Scaffolding\ScaffoldingArrayFunc())->labels()) === 1, 'array_search() over a list yields an int key');
     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');
 
     // ── array<T>|false keeps its element type after a false check ────────
     $pens = Scaffolding\loadPensOrFail();

examples/php/scaffolding/scaffolding.php

@@ -765,6 +765,9 @@ public function labels(): array { return ['ink', 'gel']; }
     /** @return array<string, string|null> */
     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<int> */
     public function weights(): array { return [2, 3, 4]; }
 }

src/type_engine/call_resolution/arg_type_resolution.rs

@@ -124,6 +124,28 @@ impl ArrayFuncArgs for TextArrayFuncArgs<'_, '_> {
     fn callback_inferred_return_type(&self, index: usize, param_type: &PhpType) -> Option<PhpType> {
         Backend::infer_closure_return_type_from_body(self.arg_text(index)?, param_type, self.ctx)
     }
+
+    fn arg_atom_text(&self, index: usize) -> Option<String> {
+        let text = self.arg_text(index)?.trim();
+        let atom = crate::util::strip_fqn_prefix(text);
+        let is_atom =
+            !atom.is_empty() && atom.chars().all(|c| c.is_ascii_alphanumeric() || c == '_');
+        is_atom.then(|| atom.to_string())
+    }
+
+    fn callback_param_narrowing(
+        &self,
+        index: usize,
+        param_index: usize,
+        subject: &PhpType,
+    ) -> Option<PhpType> {
+        crate::type_engine::variable::callback_narrowing::narrow_callback_param_text(
+            self.arg_text(index)?,
+            param_index,
+            subject,
+            Some(&self.ctx.class_loader),
+        )
+    }
 }
 
 impl Backend {

src/type_engine/types/narrowing/guards.rs

@@ -768,6 +768,112 @@ pub(crate) fn guard_kind_to_narrowed_type(kind: TypeGuardKind) -> PhpType {
 /// The interface every non-array iterable implements.
 const TRAVERSABLE_FQN: &str = "Traversable";
 
+/// The domain a `is_*()` builtin tests its argument against.
+///
+/// Returns `None` for any other function name.
+pub(crate) fn type_guard_kind_from_name(name: &str) -> Option<TypeGuardKind> {
+    Some(match name.trim_start_matches('\\') {
+        "is_array" => TypeGuardKind::Array,
+        "is_string" => TypeGuardKind::String,
+        "is_int" | "is_integer" | "is_long" => TypeGuardKind::Int,
+        "is_float" | "is_double" | "is_real" => TypeGuardKind::Float,
+        "is_bool" => TypeGuardKind::Bool,
+        "is_object" => TypeGuardKind::Object,
+        "is_numeric" => TypeGuardKind::Numeric,
+        "is_callable" => TypeGuardKind::Callable,
+        "is_null" => TypeGuardKind::Null,
+        "is_scalar" => TypeGuardKind::Scalar,
+        "is_resource" => TypeGuardKind::Resource,
+        "is_iterable" => TypeGuardKind::Iterable,
+        _ => return None,
+    })
+}
+
+/// Narrow `ty` to the values the `is_*()` builtin named by `name`
+/// accepts.
+///
+/// Returns `None` when `name` is not a type guard, when every value of
+/// `ty` already passes it, and when none can.
+pub(crate) fn narrow_type_by_guard_name(
+    name: &str,
+    ty: &PhpType,
+    class_loader: GuardClassLoader<'_>,
+) -> Option<PhpType> {
+    let kind = type_guard_kind_from_name(name)?;
+    let narrowed = filter_type_by_guard(ty, kind, true, class_loader)?;
+    (!narrowed.is_empty_sentinel()).then_some(narrowed)
+}
+
+/// Narrow `ty` to the values of `var_name` that can make `condition`
+/// truthy.
+///
+/// `&&` chains narrow through each operand in turn, `||` chains join what
+/// each branch admits, and a negated guard narrows by exclusion. Returns
+/// `None` when the condition says nothing about `var_name` (so the caller
+/// keeps the type it has) and when it admits no value at all, since an
+/// empty type is never a useful answer for a caller that only knows the
+/// condition held.
+pub(crate) fn narrow_type_by_condition(
+    condition: &Expression<'_>,
+    var_name: &str,
+    ty: &PhpType,
+    class_loader: GuardClassLoader<'_>,
+) -> Option<PhpType> {
+    let narrowed = narrow_by_condition_inner(condition, var_name, ty, class_loader)?;
+    (!narrowed.is_empty_sentinel()).then_some(narrowed)
+}
+
+fn narrow_by_condition_inner(
+    condition: &Expression<'_>,
+    var_name: &str,
+    ty: &PhpType,
+    class_loader: GuardClassLoader<'_>,
+) -> Option<PhpType> {
+    match condition {
+        Expression::Parenthesized(inner) => {
+            return narrow_by_condition_inner(inner.expression, var_name, ty, class_loader);
+        }
+        Expression::Binary(bin)
+            if matches!(
+                bin.operator,
+                BinaryOperator::And(_) | BinaryOperator::LowAnd(_)
+            ) =>
+        {
+            // Both operands hold, so the right-hand one narrows whatever
+            // the left-hand one left behind.
+            let lhs = narrow_by_condition_inner(bin.lhs, var_name, ty, class_loader);
+            let base = lhs.as_ref().unwrap_or(ty);
+            return narrow_by_condition_inner(bin.rhs, var_name, base, class_loader).or(lhs);
+        }
+        Expression::Binary(bin)
+            if matches!(
+                bin.operator,
+                BinaryOperator::Or(_) | BinaryOperator::LowOr(_)
+            ) =>
+        {
+            // Either operand may be what let the value through, so the
+            // answer is what they admit between them. An operand that says
+            // nothing about `var_name` admits everything, which makes the
+            // whole condition uninformative.
+            let lhs = narrow_by_condition_inner(bin.lhs, var_name, ty, class_loader)?;
+            let rhs = narrow_by_condition_inner(bin.rhs, var_name, ty, class_loader)?;
+            let members: Vec<PhpType> = [lhs, rhs]
+                .into_iter()
+                .filter(|m| !m.is_empty_sentinel())
+                .collect();
+            return match members.len() {
+                0 => Some(PhpType::empty_sentinel()),
+                1 => members.into_iter().next(),
+                _ => Some(PhpType::join_runtime_value_types(members)),
+            };
+        }
+        _ => {}
+    }
+
+    let (kind, negated) = try_extract_type_guard(condition, var_name)?;
+    filter_type_by_guard(ty, kind, !negated, class_loader)
+}
+
 /// Try to extract a type-guard function call on a variable.
 ///
 /// Matches `is_array($var)`, `is_string($var)`, etc. (with optional
@@ -791,21 +897,7 @@ pub(crate) fn try_extract_type_guard(
                 }
                 _ => return None,
             };
-            let kind = match func_name {
-                "is_array" => TypeGuardKind::Array,
-                "is_string" => TypeGuardKind::String,
-                "is_int" | "is_integer" | "is_long" => TypeGuardKind::Int,
-                "is_float" | "is_double" | "is_real" => TypeGuardKind::Float,
-                "is_bool" => TypeGuardKind::Bool,
-                "is_object" => TypeGuardKind::Object,
-                "is_numeric" => TypeGuardKind::Numeric,
-                "is_callable" => TypeGuardKind::Callable,
-                "is_null" => TypeGuardKind::Null,
-                "is_scalar" => TypeGuardKind::Scalar,
-                "is_resource" => TypeGuardKind::Resource,
-                "is_iterable" => TypeGuardKind::Iterable,
-                _ => return None,
-            };
+            let kind = type_guard_kind_from_name(func_name)?;
             let args = &fc.argument_list.arguments;
             if args.len() != 1 {
                 return None;

src/type_engine/variable/array_func_rules.rs

@@ -17,7 +17,7 @@
 /// the handful of questions the rules ask about an argument, and the
 /// rules stay in one place so a fix to `array_map`'s element type
 /// reaches every consumer.
-use crate::php_type::{PhpType, TypeKind};
+use crate::php_type::{PhpType, TypeKind, is_array_like_name};
 
 use super::{ARRAY_ELEMENT_FUNCS, ARRAY_PRESERVING_FUNCS};
 
@@ -47,6 +47,21 @@ pub(in crate::type_engine) trait ArrayFuncArgs {
     /// function at `index`, with its first parameter seeded to
     /// `param_type`.
     fn callback_inferred_return_type(&self, index: usize, param_type: &PhpType) -> Option<PhpType>;
+
+    /// The argument at `index` written as a bare constant name or integer
+    /// literal (`ARRAY_FILTER_USE_KEY`, `2`), with any namespace prefix
+    /// stripped.  `None` for any other expression.
+    fn arg_atom_text(&self, index: usize) -> Option<String>;
+
+    /// `subject` narrowed to the values that make the closure or arrow
+    /// function at `index` accept them through its `param_index`th
+    /// parameter.  `None` when the callback asserts nothing about it.
+    fn callback_param_narrowing(
+        &self,
+        index: usize,
+        param_index: usize,
+        subject: &PhpType,
+    ) -> Option<PhpType>;
 }
 
 /// For known array-producing functions, resolve the **raw output type**
@@ -83,8 +98,15 @@ pub(in crate::type_engine) fn array_func_raw_type(
             // drops `null`, `false`, `0`, `''` and friends. With a
             // callback the kept members are whatever it approves of, which
             // says nothing about their type.
-            if func_name.eq_ignore_ascii_case("array_filter") && !args.has_arg(1) {
-                return Some(filter_element_type(&raw).unwrap_or(raw));
+            if func_name.eq_ignore_ascii_case("array_filter") {
+                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);
+                }
             }
             return Some(raw);
         }
@@ -210,6 +232,50 @@ fn filter_element_type(raw: &PhpType) -> Option<PhpType> {
     }
 }
 
+/// Rebuild an `array_filter` result with its key type narrowed to what
+/// the callback asserts about the key it was handed.
+///
+/// Returns `None` unless the call runs in one of the two modes that pass
+/// the key (`ARRAY_FILTER_USE_KEY`, `ARRAY_FILTER_USE_BOTH`), the callback
+/// proves something about it, and the input carries a key type the proof
+/// can narrow.
+fn filter_key_type(raw: &PhpType, args: &dyn ArrayFuncArgs) -> Option<PhpType> {
+    let param_index = filter_key_param_index(args)?;
+    let key = raw.iterable_key_type()?;
+    let narrowed = args.callback_param_narrowing(1, param_index, &key)?;
+    // A callback that admits every key it could receive (`is_int($k) ||
+    // is_string($k)`) leaves nothing to say, and answering with the
+    // rebuilt union would only reorder its members.
+    if key.is_subtype_of(&narrowed) {
+        return None;
+    }
+    let value = raw.extract_value_type(false)?.clone();
+    // A filter can drop every entry, so the result is a plain `array`
+    // whatever refinement (`non-empty-array`, `list`) the input carried.
+    match raw.kind() {
+        TypeKind::Array(_) => Some(PhpType::generic_array(narrowed, value)),
+        TypeKind::Generic(g) if is_array_like_name(g.name.as_str()) => {
+            Some(PhpType::generic_array(narrowed, value))
+        }
+        _ => None,
+    }
+}
+
+/// Which of the callback's parameters receives the key, from
+/// `array_filter`'s mode argument.
+///
+/// The default mode passes only the value, so the callback says nothing
+/// about the keys and this returns `None`.
+fn filter_key_param_index(args: &dyn ArrayFuncArgs) -> Option<usize> {
+    match args.arg_atom_text(2)?.as_str() {
+        "ARRAY_FILTER_USE_KEY" | "2" => Some(0),
+        // `ARRAY_FILTER_USE_BOTH` passes the value first and the key
+        // second.
+        "ARRAY_FILTER_USE_BOTH" | "1" => Some(1),
+        _ => None,
+    }
+}
+
 /// Extract the output element type for `array_map($callback, $array)`.
 ///
 /// Strategy: