fix(ir-gen): scope IfLet binding to its then-branch via alias table

Reported by @SiyuanSun0736 on PR #19.

Signed-off-by: Cong Wang <cwang@multikernel.io>

Author: congwang-mk

src/ir_generator.ml

@@ -67,6 +67,12 @@ type ir_context = {
   map_origin_variables: (string, (string * ir_value * (ir_value_desc * ir_type))) Hashtbl.t; (* var_name -> (map_name, key, underlying_info) *)
   (* Track inferred variable types for proper lookups *)
   variable_types: (string, ir_type) Hashtbl.t; (* var_name -> ir_type *)
+  (* Active IfLet bindings: source name -> synthetic IR name, for the duration
+     of the then-branch. Reads, simple assignments, and compound assignments
+     of the source name are rewritten to the synthetic name; the synthetic
+     name is what was actually declared in IR, so an outer variable of the
+     same name is never clobbered when the backend hoists declarations. *)
+  iflet_aliases: (string, string) Hashtbl.t;
   mutable current_program_type: program_type option;
 }
 
@@ -92,6 +98,7 @@ let create_context ?(global_variables = []) ?(helper_functions = []) symbol_tabl
                      tbl);
   map_origin_variables = Hashtbl.create 32;
   variable_types = Hashtbl.create 32;
+  iflet_aliases = Hashtbl.create 4;
   current_program_type = None;
   helper_functions = (let tbl = Hashtbl.create 16 in
                      List.iter (fun helper_name -> Hashtbl.add tbl helper_name ()) helper_functions;
@@ -557,13 +564,22 @@ let expand_map_operation ctx map_name operation key_val value_val_opt pos =
 
   | _ -> failwith ("Unknown map operation: " ^ operation)
 
+(** Resolve a source-level identifier to its current IR-level name.
+    Returns the synthetic name if the identifier is currently bound by an
+    enclosing IfLet, otherwise the name unchanged. *)
+let resolve_iflet_alias ctx name =
+  match Hashtbl.find_opt ctx.iflet_aliases name with
+  | Some synth -> synth
+  | None -> name
+
 (** Lower AST expressions to IR values *)
 let rec lower_expression ctx (expr : Ast.expr) =
   match expr.expr_desc with
   | Ast.Literal lit ->
       lower_literal lit expr.expr_pos
 
   | Ast.Identifier name ->
+      let name = resolve_iflet_alias ctx name in
       (* Check if this is a map identifier *)
       if Hashtbl.mem ctx.maps name then
         (* For map identifiers, create a map reference *)
@@ -1450,6 +1466,7 @@ and lower_statement ctx stmt =
            ())
 
   | Ast.Assignment (name, expr) ->
+      let name = resolve_iflet_alias ctx name in
       let value = lower_expression ctx expr in
 
       (* Track if this assignment is from a map access *)
@@ -1501,6 +1518,7 @@ and lower_statement ctx stmt =
         emit_instruction ctx assign_instr
 
   | Ast.CompoundAssignment (name, op, expr) ->
+      let name = resolve_iflet_alias ctx name in
       let value = lower_expression ctx expr in
 
       (* Check if this is a global variable assignment *)
@@ -2042,14 +2060,19 @@ and lower_statement ctx stmt =
 
   | Ast.CompoundFieldIndexAssignment (map_expr, key_expr, field, op, value_expr) ->
       (* Desugar `map[k].field op= rhs` to:
-           if (var __cidx_field_N = map[k]) {
+           var __cidx_field_N = map[k]
+           if (__cidx_field_N != null) {
              __cidx_field_N.field = __cidx_field_N.field op rhs
            }
-         The IfLet lowering (above) handles the presence check; the field-store
-         lowers to a pointer-checked `ptr->field = ...` via IRStructFieldAssignment.
-         We look up the field's AST type from the map's value-struct definition so
-         that the synthesized FieldAccess/BinaryOp get correct expr_type — without
-         this the IR generator defaults to IRU32, mis-sizing wider fields. *)
+         The synthetic name is fresh, so it cannot collide with any user
+         variable — we go straight to a plain Declaration + If rather than
+         routing through `Ast.IfLet` (whose alpha-rename machinery is only
+         needed when the binding name comes from user source). The
+         field-store lowers to a pointer-checked `ptr->field = ...` via
+         IRStructFieldAssignment. We look up the field's AST type from the
+         map's value-struct definition so the synthesized FieldAccess /
+         BinaryOp get the correct expr_type — without this the IR generator
+         defaults to IRU32, mis-sizing wider fields. *)
       let pos = stmt.stmt_pos in
       let synth_name = generate_temp_variable ctx "cidx_field" in
       let map_name = match map_expr.expr_desc with
@@ -2079,31 +2102,58 @@ and lower_statement ctx stmt =
       let tmp_id = mk_expr (Ast.Identifier synth_name) in
       let cur_field = mk_expr ?ty:field_ast_type (Ast.FieldAccess (tmp_id, field)) in
       let bin = mk_expr ?ty:field_ast_type (Ast.BinaryOp (cur_field, op, value_expr)) in
-      let store = { Ast.stmt_desc = Ast.FieldAssignment (tmp_id, field, bin); stmt_pos = pos } in
-      let iflet = { Ast.stmt_desc = Ast.IfLet (synth_name, access, [store], None); stmt_pos = pos } in
-      lower_statement ctx iflet
+      let store =
+        { Ast.stmt_desc = Ast.FieldAssignment (tmp_id, field, bin); stmt_pos = pos } in
+      let cond =
+        mk_expr ~ty:Ast.Bool
+          (Ast.BinaryOp (tmp_id, Ast.Ne, mk_expr (Ast.Literal Ast.NullLit))) in
+      lower_statement ctx
+        { Ast.stmt_desc = Ast.Declaration (synth_name, None, Some access);
+          stmt_pos = pos };
+      lower_statement ctx
+        { Ast.stmt_desc = Ast.If (cond, [store], None); stmt_pos = pos }
 
   | Ast.IfLet (name, expr, then_stmts, else_opt) ->
       (* Desugar `if (var name = expr) { T } else { E }` into:
-           var name = expr
-           if (name != null) { T } else { E }
-         The eBPF/userspace codegen rule for `IRMapAccess <op> NullLit` (and
-         the symmetric form for raw pointers) emits a pointer presence
-         check, so this lowers correctly without an extra dereference. *)
+           var __iflet_<name>_<N> = expr
+           if (__iflet_<name>_<N> != null) { T } else { E }
+         The synthetic name is what the IR actually declares, so an outer
+         variable of the same name is never clobbered when the backend
+         hoists declarations to function scope. References to `name` inside
+         `T` are redirected to the synthetic name through
+         `ctx.iflet_aliases`, which is set up only around the lowering of
+         the then-branch — the else-branch sees the un-aliased name. The
+         codegen rule for `IRMapAccess <op> NullLit` (and the symmetric
+         form for raw pointers) emits a pointer presence check, so this
+         lowers correctly without an extra dereference. *)
       let pos = stmt.stmt_pos in
-      let decl_stmt = { Ast.stmt_desc = Ast.Declaration (name, None, Some expr); stmt_pos = pos } in
-      let name_ident = { Ast.expr_desc = Ast.Identifier name; expr_type = expr.Ast.expr_type;
-                         expr_pos = pos; type_checked = false; program_context = None;
-                         map_scope = None } in
-      let null_lit = { Ast.expr_desc = Ast.Literal Ast.NullLit; expr_type = None;
-                       expr_pos = pos; type_checked = false; program_context = None;
-                       map_scope = None } in
-      let cond = { Ast.expr_desc = Ast.BinaryOp (name_ident, Ast.Ne, null_lit);
-                   expr_type = Some Ast.Bool; expr_pos = pos; type_checked = false;
-                   program_context = None; map_scope = None } in
-      let if_stmt = { Ast.stmt_desc = Ast.If (cond, then_stmts, else_opt); stmt_pos = pos } in
-      lower_statement ctx decl_stmt;
-      lower_statement ctx if_stmt
+      let synth = generate_temp_variable ctx ("iflet_" ^ name) in
+      let mk_expr ?ty d =
+        { Ast.expr_desc = d; expr_pos = pos; expr_type = ty;
+          type_checked = false; program_context = None; map_scope = None } in
+      lower_statement ctx
+        { Ast.stmt_desc = Ast.Declaration (synth, None, Some expr); stmt_pos = pos };
+      let cond_val = lower_expression ctx (mk_expr ~ty:Ast.Bool
+        (Ast.BinaryOp
+           (mk_expr ?ty:expr.Ast.expr_type (Ast.Identifier synth),
+            Ast.Ne,
+            mk_expr (Ast.Literal Ast.NullLit)))) in
+      let collect_block stmts =
+        let saved = ctx.current_block in
+        ctx.current_block <- [];
+        List.iter (lower_statement ctx) stmts;
+        let instrs = List.rev ctx.current_block in
+        ctx.current_block <- saved;
+        instrs in
+      let prev_alias = Hashtbl.find_opt ctx.iflet_aliases name in
+      Hashtbl.replace ctx.iflet_aliases name synth;
+      let then_instrs = collect_block then_stmts in
+      (match prev_alias with
+       | Some p -> Hashtbl.replace ctx.iflet_aliases name p
+       | None -> Hashtbl.remove ctx.iflet_aliases name);
+      let else_instrs_opt = Option.map collect_block else_opt in
+      emit_instruction ctx
+        (make_ir_instruction (IRIf (cond_val, then_instrs, else_instrs_opt)) pos)
 
   | Ast.For (var, start_expr, end_expr, body_stmts) ->
       (* Analyze the loop to determine if it's bounded or unbounded *)

tests/test_compound_index_assignment.ml

@@ -465,7 +465,12 @@ var stats : hash<u32, Stats>(1024)
   let contains s =
     try let _ = Str.search_forward (Str.regexp_string s) c 0 in true
     with Not_found -> false in
-  (* (a) pointer-typed synthetic binding initialised from the lookup pointer *)
+  (* (a) pointer-typed synthetic binding initialised from the lookup pointer.
+     The Phase 2 desugaring emits a plain `var __cidx_field_<N> = m[k]`
+     (the synthetic name is fresh by construction, so the IfLet alpha-
+     rename machinery is not needed and is bypassed). The codegen then
+     produces `struct Stats* __cidx_field_<N> = __map_lookup_<M>` via the
+     pointer-from-map-access path in IRVariableDecl. *)
   check bool "synthetic binding declared as a struct pointer" true
     (contains "struct Stats* __cidx_field_");
   let bad_value_init = contains

tests/test_iflet.ml

@@ -219,8 +219,13 @@ var counters : hash<u32, u64>(1024)
 }
 |} in
   let c = codegen_ebpf source in
+  (* The IfLet binding is alpha-renamed to a fresh synthetic name during IR
+     lowering (see `subst_ident_stmts` in ir_generator.ml) so that an outer
+     variable of the same name is not silently clobbered when the backend
+     hoists declarations to function scope. The synthetic name has the
+     form `__iflet_<orig>_<N>`. *)
   check bool "scalar binding declared as value, not pointer" true
-    (contains_substr c "__u64 c =");
+    (contains_substr c "__u64 __iflet_c_");
   check bool "binding init uses the dereffed value statement-expression" true
     (contains_substr c "__val = *(");
   check bool "presence check on the underlying lookup pointer" true
@@ -259,13 +264,55 @@ var stats : hash<u32, Stats>(1024)
 }
 |} in
   let c = codegen_ebpf source in
+  (* Binding is alpha-renamed to `__iflet_s_<N>` — see the comment on
+     `test_codegen_scalar_value_binding` for why. *)
   check bool "binding declared with value type, not pointer" true
-    (contains_substr c "struct Stats s =");
+    (contains_substr c "struct Stats __iflet_s_");
   check bool "value-typed binding uses deref-load init" true
     (contains_substr c "struct Stats __val");
   check bool "field write goes through the underlying lookup pointer" true
     (contains_substr c "->count =")
 
+(** 11. Codegen (shadowing): an outer binding of the same name as the IfLet
+       binding must survive both branches and remain referenceable after the
+       if. The branch-local invariant the frontend enforces (binding visible
+       only inside the then-branch) has to be preserved end-to-end through
+       IR lowering — i.e., the inner binding cannot collapse onto the outer
+       name in the generated C. *)
+let test_codegen_shadow_outer_binding () =
+  let source = {|
+var counters : hash<u32, u64>(1024)
+
+@xdp fn probe(ctx: *xdp_md) -> xdp_action {
+  var c : u64 = 100
+  if (var c = counters[1]) {
+    return XDP_DROP
+  }
+  if (c == 100) {
+    return XDP_PASS
+  }
+  return XDP_DROP
+}
+|} in
+  let c = codegen_ebpf source in
+  (* The outer `c = 100` declaration must remain literally — the inner binding
+     must not reuse the name. *)
+  check bool "outer c declared with literal value" true
+    (contains_substr c "__u64 c = 100");
+  (* The outer `c` must NOT be reassigned by the IfLet's lowering. The bug
+     symptom was a statement-expression assignment `c = ({ ... });` that
+     clobbered the outer binding with the lookup result (or zero on miss).
+     A bare `c = ({` (no `__u64` prefix) is the giveaway. *)
+  let outer_clobber =
+    try let _ = Str.search_forward
+                  (Str.regexp "[^_a-zA-Z0-9]c = ({") c 0 in true
+    with Not_found -> false in
+  check bool "outer c not clobbered by iflet init" false outer_clobber;
+  (* The post-if comparison `c == 100` must reference the outer `c`, not be
+     rewritten into another fresh map deref. *)
+  check bool "post-if uses outer c by name" true
+    (contains_substr c "(c == 100)")
+
 let suite = [
   "parse_iflet_no_else",              `Quick, test_parse_iflet_no_else;
   "parse_iflet_with_else",            `Quick, test_parse_iflet_with_else;
@@ -277,6 +324,7 @@ let suite = [
   "codegen_struct_in_place",          `Quick, test_codegen_struct_in_place;
   "codegen_scalar_value_binding",     `Quick, test_codegen_scalar_value_binding;
   "codegen_struct_value_binding_shape", `Quick, test_codegen_struct_value_binding_shape;
+  "codegen_shadow_outer_binding",     `Quick, test_codegen_shadow_outer_binding;
 ]
 
 let () =