diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index b4c370ed4..43f5a7387 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -442,6 +442,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **A null check written through `?? null` narrows the value.** `if (null !== ($row['name'] ?? null))` makes `$row['name']` non-null the way `isset()` does, and so does getting past `if (($row['name'] ?? null) === null)`. Contributed by @phcorp. - **PHPUnit's `assertSame()` narrows to the expected value.** After `$this->assertSame(4, $width)`, `$width` is `4` instead of `object`, so passing it on to an `int` parameter is no longer reported. Contributed by @phcorp. Closes #473. - **Assertions whose type is a template.** A `@phpstan-assert` naming a `@template` now narrows to the type of the argument it is bound from, so after `$this->assertSame(4, $width)`, `$width` is `4` instead of `object`, and passing it on to an `int` parameter is no longer reported. Contributed by @phcorp. +- **A variable a local closure writes by reference holds what was written once the closure is called.** After `$write = function () use (&$out) { $out = 'written'; };` and `$write();`, `$out` was still reported as possibly holding the value it had before, so passing it on raised a false argument type error. Calling the variable now runs the closure there and then, the same as calling it inline does. A closure that is only called on some paths, handed to another function, or replaced before the call, and a write the closure only makes on some paths, still leave both values possible. Contributed by @phcorp. +- **Calling a closure stored in a variable applies its by-reference writes.** After `$write = function () use (&$out) { $out = 'written'; };` and `$write();`, `$out` holds `'written'` instead of possibly still its old value, so passing it on no longer raises a false argument type error. Contributed by @phcorp. Closes #474. ### Internal diff --git a/src/type_engine/variable/forward_walk/assignment.rs b/src/type_engine/variable/forward_walk/assignment.rs index 8e4d33540..d364eee40 100644 --- a/src/type_engine/variable/forward_walk/assignment.rs +++ b/src/type_engine/variable/forward_walk/assignment.rs @@ -338,6 +338,13 @@ pub(crate) fn process_assignment_expr<'b>( let condition_proofs = condition_implications(&lhs_name, assignment.rhs, &rhs_types, scope, ctx); let ternary_proofs = ternary_arm_implications(&lhs_name, assignment.rhs, scope, ctx); + // `$write = function () use (&$out) { … };` — remember which + // closure `$write` holds, so `$write();` can run its body. Read + // before the write: `use ($write)` copies the old value. + let held_closure = match crate::parser::unwrap_parens(assignment.rhs) { + Expression::Closure(closure) => held_closure_of(closure, scope), + _ => None, + }; // Reassigning the variable replaces its object identity, so any // property/array-access key rooted at it (seeded by an earlier // assignment or condition narrowing) is now stale. Drop them @@ -379,6 +386,9 @@ pub(crate) fn process_assignment_expr<'b>( // entry is the plain unknown a join treats as top. scope.set_untyped(&lhs_name); } + if let Some(held) = held_closure { + scope.set_held_closure(&lhs_name, held); + } // `$isHtml = $raw instanceof HtmlString` makes `$isHtml` stand // for the check, so testing it later narrows `$raw`. record_assertion_variable(&lhs_name, assignment.rhs, scope, ctx); diff --git a/src/type_engine/variable/forward_walk/by_ref.rs b/src/type_engine/variable/forward_walk/by_ref.rs index d3bf0b7c1..ca9ee31d2 100644 --- a/src/type_engine/variable/forward_walk/by_ref.rs +++ b/src/type_engine/variable/forward_walk/by_ref.rs @@ -10,6 +10,7 @@ use std::sync::Arc; use mago_span::HasSpan; use mago_syntax::cst::argument::Argument; +use mago_syntax::walker::Walker; use crate::atom::{atom, bytes_to_str}; use crate::php_type::{PhpType, TypeKind}; @@ -60,6 +61,16 @@ pub(crate) fn process_by_ref_closure_captures<'b>( process_by_ref_closure_captures(arg_expr, scope, ctx); } } + + // `$write()` on a variable still holding the closure literal it + // was assigned runs that body here, after the arguments, just + // as the immediately invoked closure above does. + if let Call::Function(fc) = call + && let Expression::Variable(Variable::Direct(var)) = + crate::parser::unwrap_parens(fc.function) + { + invoke_closure_literal_held_by(bytes_to_str(var.name), scope, ctx); + } } // A closure that is defined but not provably invoked (stored in a // variable, passed somewhere opaque) may still run any time later, @@ -98,6 +109,113 @@ pub(crate) fn process_by_ref_closure_captures<'b>( } } +/// What a variable assigned `closure` holds, for [`ScopeState::held_closure`], +/// read off `scope` as it stands before the assignment. `None` for a +/// closure that captures nothing by reference, since calling it cannot +/// change a local. +pub(crate) fn held_closure_of(closure: &Closure<'_>, scope: &ScopeState) -> Option { + let use_clause = closure.use_clause.as_ref()?; + if !use_clause + .variables + .iter() + .any(|use_var| use_var.ampersand.is_some()) + { + return None; + } + let mut by_value = Vec::new(); + for use_var in use_clause.variables.iter() { + if use_var.ampersand.is_some() { + continue; + } + let name = bytes_to_str(use_var.variable.name); + let bare = atom(name); + if let Some(types) = scope.locals.get(&bare) { + by_value.push((bare, types.clone())); + } + for key in scope.locals.compound_keys() { + if crate::type_engine::type_ops::condition_facts::key_reads_variable(key.as_str(), name) + && let Some(types) = scope.locals.get(key) + { + by_value.push((*key, types.clone())); + } + } + } + Some(HeldClosure { + offset: closure.span().start.offset, + by_value, + }) +} + +/// Run the by-reference capturing closure literal `var_name` holds, if +/// [`ScopeState::held_closure`] says it holds one. +/// +/// The scope only keeps the closure's offset, since it outlives the +/// syntax tree being walked, so the closure is found again in the parsed +/// file. Not finding it leaves the captures as the definition widened +/// them. +fn invoke_closure_literal_held_by( + var_name: &str, + scope: &mut ScopeState, + ctx: &ForwardWalkCtx<'_>, +) { + let Some(held) = scope.held_closure(var_name) else { + return; + }; + let offset = held.offset; + crate::parser::with_parsed_program(ctx.file.content, "invoke_closure_literal", |program, _| { + let mut found = None; + let containing = program.statements.iter().find(|stmt| { + let span = stmt.span(); + span.start.offset <= offset && offset < span.end.offset + }); + if let Some(stmt) = containing { + Walker::walk_statement(&ClosureAtOffset(offset), stmt, &mut found); + } + let Some(closure) = found else { + return; + }; + let captured = by_ref_captured_names(closure); + let mut closure_scope = ScopeState::new(); + seed_closure_captures(&mut closure_scope, scope, closure.use_clause.as_ref()); + // The by-value captures were copied where the closure was written, + // not here. + if let Some(use_clause) = closure.use_clause.as_ref() { + for use_var in use_clause.variables.iter() { + if use_var.ampersand.is_some() { + continue; + } + let name = bytes_to_str(use_var.variable.name); + closure_scope.locals.remove(&atom(name)); + closure_scope.locals.retain_compound(|key| { + !crate::type_engine::type_ops::condition_facts::key_reads_variable( + key.as_str(), + name, + ) + }); + } + } + for (key, types) in &held.by_value { + closure_scope.locals.insert(*key, types.clone()); + } + run_by_ref_closure(closure, captured, closure_scope, scope, ctx, true, true); + }); +} + +/// Finds the closure literal starting at an offset. +struct ClosureAtOffset(u32); + +impl<'ast, 'arena> Walker<'ast, 'arena, Option<&'ast Closure<'arena>>> for ClosureAtOffset { + fn walk_in_closure( + &self, + closure: &'ast Closure<'arena>, + found: &mut Option<&'ast Closure<'arena>>, + ) { + if closure.span().start.offset == self.0 { + *found = Some(closure); + } + } +} + /// Whether a call provably invokes its callable argument before /// returning, so a by-ref capture's final state can *replace* the outer /// variable rather than widen it. @@ -427,13 +545,33 @@ pub(crate) fn process_by_ref_closure_capture<'b>( if captured.is_empty() { return; } + let mut closure_scope = ScopeState::new(); + seed_closure_captures(&mut closure_scope, scope, closure.use_clause.as_ref()); + run_by_ref_closure( + closure, + captured, + closure_scope, + scope, + ctx, + invoked_immediately, + runs_once, + ); +} +/// [`process_by_ref_closure_capture`] once the closure's scope has been +/// entered with its captures. +fn run_by_ref_closure<'b>( + closure: &'b Closure<'b>, + captured: Vec, + mut closure_scope: ScopeState, + scope: &mut ScopeState, + ctx: &ForwardWalkCtx<'_>, + invoked_immediately: bool, + runs_once: bool, +) { let full_ctx = ctx .with_cursor_offset(u32::MAX) .with_in_loop(ctx.in_loop || !runs_once); - let mut closure_scope = ScopeState::new(); - - seed_closure_captures(&mut closure_scope, scope, closure.use_clause.as_ref()); seed_closure_params( &mut closure_scope, @@ -496,6 +634,21 @@ pub(crate) fn process_pass_by_ref<'b>( if !passes_a_variable(expr) && !is_extract_call(expr) { return; } + + // A callee taking the closure variable by reference may put something + // else in it without the passes below noticing (an untyped `&$cb` + // leaves a typed variable alone), so passing it on is enough to stop + // trusting that a later call runs the closure it was assigned. + if !scope.closure_literals.is_empty() + && let Some(arg_list) = call_argument_list(expr) + { + for arg in arg_list.arguments.iter() { + if let Expression::Variable(Variable::Direct(dv)) = arg.value() { + scope.forget_held_closure(bytes_to_str(dv.name)); + } + } + } + if !super::array_assignment::process_array_push_call(expr, scope, ctx) && !super::array_assignment::process_array_cursor_call(expr, scope, ctx) && !super::array_assignment::process_array_sort_call(expr, scope) @@ -625,19 +778,22 @@ fn method_call_on<'b>( /// (`$matches` in `preg_match($pattern, $subject, $matches)`), the only /// kind of argument a by-reference parameter can write through. fn passes_a_variable(expr: &Expression<'_>) -> bool { - let arg_list = match expr { - Expression::Call(Call::Function(fc)) => &fc.argument_list, - Expression::Call(Call::Method(mc)) => &mc.argument_list, - Expression::Call(Call::NullSafeMethod(mc)) => &mc.argument_list, - Expression::Call(Call::StaticMethod(sc)) => &sc.argument_list, - Expression::Instantiation(inst) => match &inst.argument_list { - Some(al) => al, - None => return false, - }, - _ => return false, - }; - arg_list - .arguments - .iter() - .any(|arg| matches!(arg.value(), Expression::Variable(Variable::Direct(_)))) + call_argument_list(expr).is_some_and(|arg_list| { + arg_list + .arguments + .iter() + .any(|arg| matches!(arg.value(), Expression::Variable(Variable::Direct(_)))) + }) +} + +/// The arguments the call or instantiation `expr` passes, if it is one. +fn call_argument_list<'a, 'b>(expr: &'a Expression<'b>) -> Option<&'a ArgumentList<'b>> { + match expr { + Expression::Call(Call::Function(fc)) => Some(&fc.argument_list), + Expression::Call(Call::Method(mc)) => Some(&mc.argument_list), + Expression::Call(Call::NullSafeMethod(mc)) => Some(&mc.argument_list), + Expression::Call(Call::StaticMethod(sc)) => Some(&sc.argument_list), + Expression::Instantiation(inst) => inst.argument_list.as_ref(), + _ => None, + } } diff --git a/src/type_engine/variable/forward_walk/scope_state/merge.rs b/src/type_engine/variable/forward_walk/scope_state/merge.rs index ab274a661..151d953ab 100644 --- a/src/type_engine/variable/forward_walk/scope_state/merge.rs +++ b/src/type_engine/variable/forward_walk/scope_state/merge.rs @@ -30,6 +30,7 @@ impl ScopeState { || self.non_null_implications != other.non_null_implications || self.preg_outcomes != other.preg_outcomes || self.closure_captures != other.closure_captures + || self.closure_literals != other.closure_literals || self.key_facts != other.key_facts || !same_implied_narrowings(&self.implied_narrowings, &other.implied_narrowings) { @@ -168,6 +169,10 @@ impl ScopeState { for key in disagreeing(&self.closure_captures, &other.closure_captures) { self.closure_captures.remove(&key); } + if !self.closure_literals.is_empty() { + self.closure_literals + .retain(|name, held| other.closure_literals.get(name) == Some(held)); + } // A key fact holds past the join only where every path left it // standing. diff --git a/src/type_engine/variable/forward_walk/scope_state/mod.rs b/src/type_engine/variable/forward_walk/scope_state/mod.rs index c4564239c..7416ab2b8 100644 --- a/src/type_engine/variable/forward_walk/scope_state/mod.rs +++ b/src/type_engine/variable/forward_walk/scope_state/mod.rs @@ -15,7 +15,7 @@ pub(crate) use proof_map::ProofMap; pub(crate) use proofs::*; pub(crate) use trie::AtomTrie; -use crate::atom::{Atom, atom}; +use crate::atom::{Atom, AtomMap, atom}; use crate::php_type::PhpType; use crate::types::ResolvedType; @@ -97,6 +97,31 @@ pub(crate) struct ClosureCaptureEffect { pub method: Option, } +/// A by-reference capturing closure literal a variable holds, so calling +/// the variable can run its body. See [`ScopeState::held_closure`]. +#[derive(Clone, Debug)] +pub(crate) struct HeldClosure { + /// Where the closure literal starts in the file. + pub offset: u32, + /// The scope entries for the by-value captures, and the paths read + /// through them, as they stood where the closure was written. A + /// by-value capture is copied at the definition, so a call made after + /// the variable was reassigned still sees the old value. + pub by_value: Vec<(Atom, Vec)>, +} + +impl PartialEq for HeldClosure { + fn eq(&self, other: &Self) -> bool { + self.offset == other.offset + && self.by_value.len() == other.by_value.len() + && self + .by_value + .iter() + .zip(&other.by_value) + .all(|((a, at), (b, bt))| a == b && merge::same_types(at, bt)) + } +} + /// What has to be shown about a proof's holder before the proof applies. #[derive(Clone, Debug)] pub(crate) enum ProofTrigger { @@ -321,6 +346,16 @@ pub(crate) struct ScopeState { /// a variable's identity. pub closure_captures: AtomTrie>, + /// Variable name → the closure literal it was last assigned, for a + /// closure that captures by reference, so calling the variable + /// (`$write()`) can run that closure's body right there. + /// + /// Dropped by every write to the variable, not just an assignment: a + /// stale entry would make a call run a closure the variable no longer + /// holds. Shared behind an `Arc` because scopes are cloned far more + /// often than closures are assigned. + pub closure_literals: AtomMap>, + /// What this path knows about which keys arrays hold; see /// [`KeyFacts`]. Boxed and `None` while there is nothing to say, which /// is almost always: scopes are cloned and cached by the thousand. @@ -359,6 +394,7 @@ impl ScopeState { unresolved: AtomTrie::default(), ruled_out: ProofMap::default(), closure_captures: AtomTrie::default(), + closure_literals: AtomMap::default(), key_facts: None, unreachable: false, } @@ -388,6 +424,32 @@ impl ScopeState { self.closure_captures.insert(atom(var_name), effects); } + /// The by-reference capturing closure literal `var_name` holds, if it + /// holds one. + pub fn held_closure(&self, var_name: &str) -> Option> { + self.closure_literals.get(&atom(var_name)).cloned() + } + + /// Record that `var_name` now holds `closure`. Must follow the write + /// that put it there, which drops whatever the variable held before. + pub fn set_held_closure(&mut self, var_name: &str, closure: HeldClosure) { + self.closure_literals + .insert(atom(var_name), std::sync::Arc::new(closure)); + } + + /// Stop treating `var_name` as holding the closure it was assigned, + /// for a write the scope does not otherwise see (a callee taking it by + /// reference). + pub fn forget_held_closure(&mut self, var_name: &str) { + self.forget_closure_literal(&atom(var_name)); + } + + fn forget_closure_literal(&mut self, key: &Atom) { + if !self.closure_literals.is_empty() { + self.closure_literals.remove(key); + } + } + /// Borrow the proofs this scope holds that are not variable types. pub fn proofs(&self) -> ScopeProofs<'_> { ScopeProofs { @@ -428,6 +490,7 @@ impl ScopeState { } let key = atom(var_name); self.unresolved.remove(&key); + self.forget_closure_literal(&key); self.locals.insert(key, types); } @@ -449,6 +512,7 @@ impl ScopeState { /// it from erasing the other paths' types at the next join. pub fn set_unknown(&mut self, var_name: &str) { let key = atom(var_name); + self.forget_closure_literal(&key); self.locals.insert(key, Vec::new()); self.unresolved.insert(key, ()); } @@ -465,6 +529,7 @@ impl ScopeState { /// way an unresolved entry does. pub fn set_untyped(&mut self, var_name: &str) { let key = atom(var_name); + self.forget_closure_literal(&key); self.locals.insert(key, Vec::new()); self.unresolved.remove(&key); } @@ -476,6 +541,7 @@ impl ScopeState { } let key = atom(var_name); self.unresolved.remove(&key); + self.forget_closure_literal(&key); self.locals.insert(key, types); } @@ -572,6 +638,7 @@ impl ScopeState { pub fn invalidate_proofs(&mut self, var_name: &str) { let key = atom(var_name); self.closure_captures.remove(&key); + self.forget_closure_literal(&key); let stale = |subject: &Atom| { *subject == key || crate::type_engine::type_ops::condition_facts::key_reads_variable( diff --git a/src/type_engine/variable/resolution_tests.rs b/src/type_engine/variable/resolution_tests.rs index 1375eb7bc..280bcb08a 100644 --- a/src/type_engine/variable/resolution_tests.rs +++ b/src/type_engine/variable/resolution_tests.rs @@ -2118,15 +2118,14 @@ $foo; #[test] fn by_ref_closure_capture_widens_when_closure_stored_in_variable() { - // A closure assigned to a variable may run later (PHPStan widens at - // the definition, not the call), so the outer type becomes a union. + // A closure assigned to a variable may run later, so the outer type + // becomes a union at the definition. let content = r#" Vec { + crate::common::slow_diagnostic_messages( + &create_test_backend_with_full_stubs(), + "file:///ByRefInvoked.php", + php, + "type_mismatch_argument", + ) +} + +/// Calling the closure a local holds runs its body there and then, so a +/// capture it always writes holds what was written once the call returns. +#[test] +fn by_ref_capture_written_by_an_invoked_local_closure_is_definite() { + let errors = argument_type_errors( + r#" $others */ + public function rebound(array $others): int + { + $out = null; + $write = static function () use (&$out): void { + $out = 'written'; + }; + foreach ($others as $write) { + } + $write(); + return \strlen($out); + } +} +"#, + ); + assert_eq!(errors.len(), 1, "got: {errors:?}"); +} + +/// A by-value capture holds what the variable held where the closure was +/// written, not what it holds where the closure is called. +#[test] +fn by_value_capture_of_an_invoked_closure_keeps_its_definition_value() { + let errors = argument_type_errors( + r#" 0) { + $write(); + } + }; + $write(); + return \strlen($out); + } +} +"#, + ); +} + +/// A loop may run its body no times, so a call inside it only maybe ran. +#[test] +fn by_ref_capture_of_a_closure_invoked_in_a_loop_stays_a_union() { + let errors = argument_type_errors( + r#" $items */ + public function looped(array $items): int + { + $out = null; + $write = static function () use (&$out): void { + $out = 'written'; + }; + foreach ($items as $item) { + $write(); + } + return \strlen($out); + } +} +"#, + ); + assert_eq!(errors.len(), 1, "got: {errors:?}"); +} diff --git a/tests/phpstan_nsrt/bug-2760.php b/tests/phpstan_nsrt/bug-2760.php index 72e2e6a51..f984b323b 100644 --- a/tests/phpstan_nsrt/bug-2760.php +++ b/tests/phpstan_nsrt/bug-2760.php @@ -20,5 +20,6 @@ function (): void { $fn(); - assertType('bool', $boolean); + // PHPantom is more precise than PHPStan here: calling `$fn` runs the closure, whose loop over `[0]` always sets the capture before returning. + assertType('true', $boolean); }; diff --git a/tests/phpstan_nsrt/bug-4657.php b/tests/phpstan_nsrt/bug-4657.php index db175d8a6..1cc30faf2 100644 --- a/tests/phpstan_nsrt/bug-4657.php +++ b/tests/phpstan_nsrt/bug-4657.php @@ -14,7 +14,8 @@ function (): void { }; $callback(); - assertType('DateTime|null', $value); + // PHPantom is more precise than PHPStan here: calling `$callback` runs the closure, which always assigns the capture. + assertType('DateTime', $value); assertNativeType('DateTime|null', $value); assertType('null', $other);