From e07acc020815038803bee30c7ab904a9733dc78b Mon Sep 17 00:00:00 2001 From: Michael Schimmel Date: Tue, 24 Feb 2026 12:53:06 +0100 Subject: [PATCH] Refactor optimizer to track upvalue usage The `UsageInfo` struct has been updated to include tracking for `used_upvalues` and `assigned_upvalues`. The `collect_usage` function has been modified to handle these new fields when encountering `Address::Upvalue`. The `map_params_to_args` function now takes `body_usage` as an argument to ensure that parameters assigned to or used in the body are correctly substituted. A new helper function, `collect_parameter_slots_set`, has been added to gather all parameter slots within a pattern. The inlining logic in `Optimizer::inline_call` has been enhanced to prevent inlining if a parameter is used or assigned in the function body but cannot be substituted. This addresses a bug where aggressive inlining could lead to incorrect code generation when parameters were reassigned. A new integration test, `test_closure_reassignment_optimization_bug`, has been added to specifically target and verify the fix for this inlining issue. --- examples/pattern-rules.myc | 20 ------ gemini.md | 2 + src/ast/compiler/optimizer.rs | 127 +++++++++++++++++++++++++++++----- src/integration_test.rs | 11 +++ 4 files changed, 121 insertions(+), 39 deletions(-) delete mode 100644 examples/pattern-rules.myc diff --git a/examples/pattern-rules.myc b/examples/pattern-rules.myc deleted file mode 100644 index 4b5c5b8..0000000 --- a/examples/pattern-rules.myc +++ /dev/null @@ -1,20 +0,0 @@ -(do -(def val1 4) ; korrekt -(def val2 [4 5]) ; korrekt -(def [val3] 5) ; falsch -(def [val4] [5]) ; korrekt - -(def [[x1 [y1 y3]] z1] [[1 [2 3]] 4]) ; korrekt -(def [[x2 [y2 y4]] z2] [1 2 3 4]) ; falsch -;(def [[x [y1 y2]] z] [1] [2 3] 4) ; falsch -;(def [[x [y1 y2]] z] [1 [2 3]] 4) ; falsch - -(def f (fn [[x [y1 y2]] z] (+ x y1 y2 z))) - -(f [1 [2 3]] 4) ; korrekt -(f [1 2 3 4]) ; falsch -(f [1 2 [3] 4]) ; falsch -(f 1 2 3 4) ; falsch -(f 1 [2 [3]] 4) ; falsch -(f [[1 2] [3 4]]) ; falsch -) diff --git a/gemini.md b/gemini.md index f3441a6..e116a6c 100644 --- a/gemini.md +++ b/gemini.md @@ -31,6 +31,7 @@ Wichtig: zentraler Einstiegspunkt ist Delphi/Myc.Ast.Environment.pasund die dort * Wir wollen die Sprachfeatures von Rust nutzen. * Der Code soll aber so gut wie möglich nach Rust-Style-Guidelines geschrieben werden. * Dokumentation nach Rust-Regeln. +* Im Code und den Kommentaren muss alles auf englisch sein. ## Interaktion @@ -38,6 +39,7 @@ Wichtig: zentraler Einstiegspunkt ist Delphi/Myc.Ast.Environment.pasund die dort * Wenn clippy etwas vorschlägt, von dem du glaubst, dass es nicht stabil ist, dann überprüfe die aktuelle Version. Wahrscheinlich ist das Feature mittlerweile im trunk. Wir gehen immer davon aus, dass clippy recht hat. * Keine Interaktion mit GIT. Kein Commit, oder ähnliches. Schlage auch nichts dergleichen vor. * Gehe immer schrittweise vor. Zeige mir, was du vor hast, bevor du Code erzeugst. +* Sprich im Chat deutsch mit mir. ## Testing & Debugging diff --git a/src/ast/compiler/optimizer.rs b/src/ast/compiler/optimizer.rs index b070ef7..f7b8279 100644 --- a/src/ast/compiler/optimizer.rs +++ b/src/ast/compiler/optimizer.rs @@ -46,9 +46,11 @@ impl PathTracker { struct UsageInfo { used_locals: HashSet, used_globals: HashSet, + used_upvalues: HashSet, used_identities: HashSet, assigned_locals: HashSet, assigned_globals: HashSet, + assigned_upvalues: HashSet, } impl Optimizer { @@ -110,7 +112,9 @@ impl Optimizer { Address::Global(idx) => { info.assigned_globals.insert(*idx); } - _ => {} + Address::Upvalue(idx) => { + info.assigned_upvalues.insert(*idx); + } }, BoundKind::Tuple { elements } => { for el in elements { @@ -759,15 +763,34 @@ impl Optimizer { sub: &mut SubstitutionMap, path: &mut PathTracker, ) -> Option { + let mut body_usage = UsageInfo::default(); + self.collect_usage(&body, &mut body_usage); + let mut arg_vals = Vec::new(); self.flatten_tuple(args.clone(), &mut arg_vals); let mut slot_index = 0; - self.map_params_to_args(params, &arg_vals, &mut slot_index, sub); + self.map_params_to_args(params, &arg_vals, &mut slot_index, sub, &body_usage); if slot_index != arg_vals.len() { return None; } + + // Verify that all used/assigned parameters have been substituted. + // If a parameter is used in the body but we couldn't substitute it (e.g. because it's assigned), + // we cannot inline the function because the parameter slot would become invalid. + let mut param_slots = HashSet::new(); + self.collect_parameter_slots_set(params, &mut param_slots); + + for slot in param_slots { + if (body_usage.used_locals.contains(&slot) || body_usage.assigned_locals.contains(&slot)) + && !sub.locals.contains_key(&slot) + && !sub.ast_locals.contains_key(&slot) + { + return None; + } + } + if sub.locals.is_empty() && sub.ast_locals.is_empty() && sub.upvalues.is_empty() @@ -779,6 +802,20 @@ impl Optimizer { Some(self.visit_node(body, sub, path)) } + fn collect_parameter_slots_set(&self, node: &AnalyzedNode, slots: &mut HashSet) { + match &node.kind { + BoundKind::Parameter { slot, .. } => { + slots.insert(*slot); + } + BoundKind::Tuple { elements } => { + for el in elements { + self.collect_parameter_slots_set(el, slots); + } + } + _ => {} + } + } + fn flatten_tuple(&self, node: AnalyzedNode, into: &mut Vec) { match node.kind { BoundKind::Tuple { elements } => { @@ -802,22 +839,25 @@ impl Optimizer { args: &[AnalyzedNode], offset: &mut usize, sub: &mut SubstitutionMap, + body_usage: &UsageInfo, ) { match &pattern.kind { BoundKind::Parameter { slot, .. } => { if let Some(arg) = args.get(*offset) { - if let BoundKind::Constant(val) = &arg.kind { - sub.add_local(*slot, val.clone()); - } else if let BoundKind::Lambda { upvalues, .. } = &arg.kind { - if upvalues.is_empty() { + if !body_usage.assigned_locals.contains(slot) { + if let BoundKind::Constant(val) = &arg.kind { + sub.add_local(*slot, val.clone()); + } else if let BoundKind::Lambda { upvalues, .. } = &arg.kind { + if upvalues.is_empty() { + sub.add_ast_local(*slot, arg.clone()); + } + } else if let BoundKind::Get { + addr: Address::Global(_), + .. + } = &arg.kind + { sub.add_ast_local(*slot, arg.clone()); } - } else if let BoundKind::Get { - addr: Address::Global(_), - .. - } = &arg.kind - { - sub.add_ast_local(*slot, arg.clone()); } } sub.slot_mapping.insert(*slot, *slot); @@ -841,7 +881,7 @@ impl Optimizer { // Match inner elements against the flattened compound argument let mut sub_offset = 0; for el in elements { - self.map_params_to_args(el, &sub_args, &mut sub_offset, sub); + self.map_params_to_args(el, &sub_args, &mut sub_offset, sub, body_usage); } *offset += 1; return; @@ -850,7 +890,7 @@ impl Optimizer { // Fallback: Continue flat matching (original behavior) for el in elements { - self.map_params_to_args(el, args, offset, sub); + self.map_params_to_args(el, args, offset, sub, body_usage); } } _ => {} @@ -879,7 +919,9 @@ impl Optimizer { Address::Global(idx) => { info.used_globals.insert(*idx); } - _ => {} + Address::Upvalue(idx) => { + info.used_upvalues.insert(*idx); + } }, BoundKind::Set { addr, value } => { match addr { @@ -889,14 +931,61 @@ impl Optimizer { Address::Global(idx) => { info.assigned_globals.insert(*idx); } - _ => {} + Address::Upvalue(idx) => { + info.assigned_upvalues.insert(*idx); + } } self.collect_usage(value, info); } - BoundKind::Lambda { params, body, .. } => { + BoundKind::Lambda { + params, + body, + upvalues, + .. + } => { info.used_identities.insert(node.identity.clone()); - self.collect_usage(params, info); - self.collect_usage(body, info); + + let mut inner_info = UsageInfo::default(); + self.collect_usage(params, &mut inner_info); + self.collect_usage(body, &mut inner_info); + + // Propagate globals and identities + info.used_globals.extend(inner_info.used_globals); + info.assigned_globals.extend(inner_info.assigned_globals); + info.used_identities.extend(inner_info.used_identities); + + // Map used upvalues to parent scope + for idx in inner_info.used_upvalues { + if let Some(addr) = upvalues.get(idx as usize) { + match addr { + Address::Local(slot) => { + info.used_locals.insert(*slot); + } + Address::Global(idx) => { + info.used_globals.insert(*idx); + } + Address::Upvalue(idx) => { + info.used_upvalues.insert(*idx); + } + } + } + } + // Map assigned upvalues to parent scope + for idx in inner_info.assigned_upvalues { + if let Some(addr) = upvalues.get(idx as usize) { + match addr { + Address::Local(slot) => { + info.assigned_locals.insert(*slot); + } + Address::Global(idx) => { + info.assigned_globals.insert(*idx); + } + Address::Upvalue(idx) => { + info.assigned_upvalues.insert(*idx); + } + } + } + } } BoundKind::Block { exprs } => { for e in exprs { diff --git a/src/integration_test.rs b/src/integration_test.rs index a817561..41149aa 100644 --- a/src/integration_test.rs +++ b/src/integration_test.rs @@ -390,4 +390,15 @@ mod tests { "[\"Symbol:\" \"btc\" \"field:\" :close \"id:\" \"cls\"]" ); } + + #[test] + fn test_closure_reassignment_optimization_bug() { + let env = Environment::new(); + // This test case reproduces a bug where the optimizer aggressively inlined a function + // ('f') even though its parameters ('x') were being assigned to in the body (by inner lambda). + // The fix ensures that such functions are NOT inlined. + let source = "(do (def f (fn [[x y]] (fn [] (assign x (+ x y))))) ((f [1 2])))"; + let res = env.run_script(source); + assert_eq!(format!("{}", res.unwrap()), "3"); + } }