diff --git a/CHANGELOG.md b/CHANGELOG.md index 07568de6a..495107e6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ The generated loop is now written from the program's source alone, and the manif ### Fixed +- **Generated Rust builds when a record is updated after one of its fields was read.** `progress = flight.progress` followed by `Flight.update(flight, progress = f(progress))` inside a pair of functions that tail-call each other generated Rust that moved the field out and then moved the whole record, which rustc rejects (E0382). A loop that read a field in a `let` and later returned the whole record failed the same way. Such a field read now moves only when nothing reads that part of the record again and is copied otherwise, and the field read into `f` moves in both spellings (`f(progress)` and `f(flight.progress)`), so a Map `f` updates is not copied. - **wasm-gc: `Map.set` no longer copies the map, and `Map.remove` no longer changes the map it was given.** `set` copied every bucket unless the compiler could prove the map had no other holder, which it cannot for a map held in a record field such as an answer module's state. A 100 000-entry state map served 2000 `Map.set` requests in 11 s under `aver run --wasm-gc` and 1.9 s on Node 26. `remove` wrote into the map it was given, so a caller that still held that map saw the key gone. Both now write into the map's arrays in place and return a new version. The version they were given stays valid, because a record of what the write replaced is kept with it. The same run now takes 0.4 s under `aver run --wasm-gc` and 0.3 s on Node. Reading an older version again costs one step for each write made since. - **A program with the generated loop can key its own waits by a type of its own.** The loop keys its wait by `Int`, and a hand-written `Wait.poll` keyed by a sum beside it used to be refused by the Rust door ("this program keys one wait set by 'Int' and another by 'Watch'") and to fail wasm-gc validation. In a program that answers a capability of its own, each such wait, in the entry or in a dependency, now goes through helpers generated for its key type that carry it through an `Int`-keyed wait. It answers the same keys in the same order. Its recording holds the `Int`-keyed wait. The wasm-gc wait ABI is unchanged, and `--target wasip2` runs such waits too. A program that answers no capability still keys all its waits one way. - **`check` no longer asks a verify block of a function no verify case can call.** A parameter of a capability resource type (`Tcp.Connection`, `Work.Job`, a job kind's handle), or of a tuple, record or sum of the module that always carries one, has no value a case can write, so such a pure branching helper failed `error[missing-verify]` with no way to satisfy it. It is now exempt, the way effectful functions are. A parameter with an empty value (`List`, `Option`, a sum with a resource-free variant) still needs its verify block. diff --git a/src/codegen/rust/from_mir.rs b/src/codegen/rust/from_mir.rs index 2eaab2c54..b9f0c34b4 100644 --- a/src/codegen/rust/from_mir.rs +++ b/src/codegen/rust/from_mir.rs @@ -3669,6 +3669,22 @@ fn emit_mir_tail_value(expr: &Spanned, ctx: &MirEmitCtx<'_>) -> Option< Some(materialize_owned(code, &expr.node, ctx)) } +/// The value of one `let` statement in a loop or trampoline body. A named +/// binding owns its value, so it goes through [`emit_mir_binding_value`] +/// like every other binding: a field read moves only where `field_moves` +/// allows it and clones otherwise, so a record the chain reads a field of +/// stays whole for its later reads and updates. A bare `Int` binding and a +/// discarded value render as they are. +fn emit_mir_statement_value( + let_node: &crate::ir::mir::MirLet, + ctx: &MirEmitCtx<'_>, +) -> Option { + if let_node.binding_name.is_empty() || ctx.bare.is_bare(let_node.binding) { + return emit_mir_expr(&let_node.value, ctx); + } + emit_mir_binding_value(&let_node.value, ctx) +} + /// Emit a `let` binding's value as an OWNED value. /// /// Naming a read of a borrowed param does not change what a function may do @@ -4081,7 +4097,7 @@ fn emit_mir_tco_body( let mut current = body; while let MirExpr::Let(spanned_let) = ¤t.node { let let_node = &spanned_let.node; - let value = emit_mir_expr(&let_node.value, ctx)?; + let value = emit_mir_statement_value(let_node, ctx)?; if let_node.binding_name.is_empty() { lines.push(format!(" {};", value)); } else { @@ -4378,6 +4394,10 @@ pub(super) fn emit_mir_mutual_tco_block( // explicit restricted tags rather than the pre-rewrite whole-function // facts so match subjects and their literals agree on `i64`. policy.apply_rewritten_bare_i64(mir_fn, ctx); + // Each arm binds its params by value, so a field read may move out + // of one exactly as in any other body; the arm's updates then see + // which records gave a field up. + policy.apply_field_moves(mir_fn); let mut arm_ctx = MirEmitCtx::for_fn(ctx, &policy); // Mutual invariants are `rc_wrapped` for owning reads, but unlike // self-TCO's `Arc` representation they are extra `&T` trampoline @@ -4575,7 +4595,7 @@ fn emit_mir_trampoline_body( let mut current = body; while let MirExpr::Let(spanned_let) = ¤t.node { let let_node = &spanned_let.node; - let value = emit_mir_expr(&let_node.value, ctx)?; + let value = emit_mir_statement_value(let_node, ctx)?; if let_node.binding_name.is_empty() { // Discarded intermediate (`Stmt::Expr` / `_ = effect()`) // — bare statement, result dropped. diff --git a/src/ir/mir/optimize/own_param.rs b/src/ir/mir/optimize/own_param.rs index e81988eb5..60323ee2b 100644 --- a/src/ir/mir/optimize/own_param.rs +++ b/src/ir/mir/optimize/own_param.rs @@ -149,6 +149,11 @@ struct RustOwned { /// Map/Vector params each fn updates in place /// (`field_moves::in_place_collection_params`). in_place_params: HashMap>, + /// Locals bound by a `let` whose value is one of the movable field + /// reads above (`progress = flight.progress`). The provenance table + /// holds copies of the binding values, whose addresses the movable set + /// cannot recognise, so these are named here from the body itself. + movable_let_slots: HashMap>, } /// A single visible call edge: `target(args…)` made from `caller`. @@ -386,6 +391,11 @@ fn own_param_refine_for_model(mut program: MirProgram, model: OwnershipModel) -> for (id, f) in program.iter() { let movable = crate::ir::mir::field_moves::movable_projections(&f.body.node); if !movable.is_empty() { + let mut slots = HashSet::new(); + collect_movable_let_slots(&f.body.node, &movable, &mut slots); + if !slots.is_empty() { + rust_owned.movable_let_slots.insert(*id, slots); + } rust_owned.movable_projections.insert(*id, movable); } } @@ -766,6 +776,19 @@ fn slot_owned( { return true; } + // A let-bound field read generated Rust moves out of its record + // (`progress = flight.progress`). It shares the record's backing, which + // is why the alias table flags it, but the move leaves the record + // without it, exactly as a movable field read passed directly. + if !is_param + && model.owned_carriers_are_cow_protected() + && rust_owned + .movable_let_slots + .get(&caller) + .is_some_and(|slots| slots.contains(&slot)) + { + return true; + } // Flagged in the caller's table (RULE 1 param or RULE 2 intra-proc // alias such as a Vector.get handle). if caller_fn @@ -850,6 +873,17 @@ fn collect_pattern_slots(pattern: &MirPattern, out: &mut HashSet) { } /// Collect `slot → binding-RHS` for every `Let` in the body. +/// The `let` locals of a body bound directly to one of its `movable` field +/// reads. +fn collect_movable_let_slots(e: &MirExpr, movable: &HashSet, out: &mut HashSet) { + if let MirExpr::Let(l) = e + && movable.contains(&(&l.node.value.node as *const MirExpr as usize)) + { + out.insert(l.node.binding.0); + } + walk_children(e, &mut |c| collect_movable_let_slots(c, movable, out)); +} + fn collect_let_bindings(e: &MirExpr, out: &mut HashMap>) { if let MirExpr::Let(l) = e { out.entry(l.node.binding.0) diff --git a/src/self_host/aver_generated/domain/eval/core/mod.rs b/src/self_host/aver_generated/domain/eval/core/mod.rs index 382aacfe8..47571affb 100644 --- a/src/self_host/aver_generated/domain/eval/core/mod.rs +++ b/src/self_host/aver_generated/domain/eval/core/mod.rs @@ -814,7 +814,7 @@ fn __mutual_tco_trampoline_1( } __MutualTco1::EvalMatch(mut v @ _, mut arms @ _, mut env @ _) => { crate::cancel_checkpoint(); - aver_list_match!(arms, [] => { return Err(AverStr::from("no matching arm")) }, [arm, rest] => match crate::aver_generated::domain::match_mod::matchPattern(&arm.pattern, &v) { Ok(bindings @ _) => { __MutualTco1::EvalExpr(arm.body.clone(), crate::aver_generated::domain::eval::store::mergeBindings(bindings, env)) }, Err(_) => { __MutualTco1::EvalMatch(v, rest, env) } }) + aver_list_match!(arms, [] => { return Err(AverStr::from("no matching arm")) }, [arm, rest] => match crate::aver_generated::domain::match_mod::matchPattern(&arm.pattern, &v) { Ok(bindings @ _) => { __MutualTco1::EvalExpr(arm.body, crate::aver_generated::domain::eval::store::mergeBindings(bindings, env)) }, Err(_) => { __MutualTco1::EvalMatch(v, rest, env) } }) } }; } @@ -1208,7 +1208,7 @@ fn __mutual_tco_trampoline_2( mut env @ _, ) => { crate::cancel_checkpoint(); - aver_list_match!(arms, [] => { return Err(AverStr::from("no matching arm")) }, [arm, rest] => match crate::aver_generated::domain::match_mod::matchPattern(&arm.pattern, &v) { Ok(bindings @ _) => { __MutualTco2::EvalTailExprSlot(selfId, arm.body.clone(), slotCount, crate::aver_generated::domain::eval::core::mergeBindingsSlot(&bindings, &arm.bindingSlots, &env)) }, Err(_) => { __MutualTco2::EvalTailMatchSlot(selfId, v, rest, slotCount, env) } }) + aver_list_match!(arms, [] => { return Err(AverStr::from("no matching arm")) }, [arm, rest] => match crate::aver_generated::domain::match_mod::matchPattern(&arm.pattern, &v) { Ok(bindings @ _) => { __MutualTco2::EvalTailExprSlot(selfId, arm.body, slotCount, crate::aver_generated::domain::eval::core::mergeBindingsSlot(&bindings, &arm.bindingSlots, &env)) }, Err(_) => { __MutualTco2::EvalTailMatchSlot(selfId, v, rest, slotCount, env) } }) } }; } @@ -1984,7 +1984,7 @@ fn __mutual_tco_trampoline_3( } __MutualTco3::EvalMatchSlot(mut v @ _, mut arms @ _, mut env @ _) => { crate::cancel_checkpoint(); - aver_list_match!(arms, [] => { return Err(AverStr::from("no matching arm")) }, [arm, rest] => match crate::aver_generated::domain::match_mod::matchPattern(&arm.pattern, &v) { Ok(bindings @ _) => { __MutualTco3::EvalExprSlot(arm.body.clone(), crate::aver_generated::domain::eval::core::mergeBindingsSlot(&bindings, &arm.bindingSlots, &env)) }, Err(_) => { __MutualTco3::EvalMatchSlot(v, rest, env) } }) + aver_list_match!(arms, [] => { return Err(AverStr::from("no matching arm")) }, [arm, rest] => match crate::aver_generated::domain::match_mod::matchPattern(&arm.pattern, &v) { Ok(bindings @ _) => { __MutualTco3::EvalExprSlot(arm.body, crate::aver_generated::domain::eval::core::mergeBindingsSlot(&bindings, &arm.bindingSlots, &env)) }, Err(_) => { __MutualTco3::EvalMatchSlot(v, rest, env) } }) } }; } diff --git a/tests/fixtures/rust_update_after_field_move/main.av b/tests/fixtures/rust_update_after_field_move/main.av new file mode 100644 index 000000000..2e85b79eb --- /dev/null +++ b/tests/fixtures/rust_update_after_field_move/main.av @@ -0,0 +1,80 @@ +module Main + intent = "A record update after a field read of the field it replaces, in plain bodies, loops and mutual tail calls." + effects [Args.get, Console.print] + +record Flight + progress: Map + name: String + +fn grow(progress: Map, key: Int) -> Map + ? "One more key." + Map.set(progress, key, key) + +verify grow + grow({}, 1) => {1 => 1} + +fn bumpLet(flight: Flight, key: Int) -> Flight + ? "Reads the field in a let, then replaces it." + progress = flight.progress + Flight.update(flight, progress = grow(progress, key)) + +verify bumpLet + bumpLet(Flight(progress = {}, name = "a"), 1) => Flight(progress = {1 => 1}, name = "a") + +fn bumpInline(flight: Flight, key: Int) -> Flight + ? "Reads the field inside the update that replaces it." + Flight.update(flight, progress = grow(flight.progress, key)) + +verify bumpInline + bumpInline(Flight(progress = {}, name = "a"), 1) => Flight(progress = {1 => 1}, name = "a") + +fn plain(flight: Flight, left: Int) -> Flight + ? "Calls both bumps left times." + match left <= 0 + true -> flight + false -> plain(bumpInline(bumpLet(flight, left), left + 1000), left - 1) + +verify plain + plain(Flight(progress = {}, name = "a"), 0) => Flight(progress = {}, name = "a") + +fn selfKeep(flight: Flight, left: Int) -> Flight + ? "A loop whose let reads a field, then hands on the whole record." + progress = flight.progress + match left <= 0 + true -> flight + false -> selfKeep(Flight.update(flight, name = "n{Map.len(progress)}"), left - 1) + +verify selfKeep + selfKeep(Flight(progress = {}, name = "a"), 1) => Flight(progress = {}, name = "n0") + +fn ping(flight: Flight, left: Int) -> Flight + ? "One half of a mutual tail call: reads the field in a let, then replaces it." + progress = flight.progress + pong(Flight.update(flight, progress = grow(progress, left)), left - 1) + +verify ping + ping(Flight(progress = {}, name = "a"), 0) => Flight(progress = {0 => 0}, name = "a") + +fn pong(flight: Flight, left: Int) -> Flight + ? "The other half: reads the field inside the update that replaces it." + match left <= 0 + true -> flight + false -> ping(Flight.update(flight, progress = grow(flight.progress, left + 1000)), left - 1) + +verify pong + pong(Flight(progress = {}, name = "a"), 0) => Flight(progress = {}, name = "a") + +fn arg(index: Int, fallback: Int) -> Int + ? "The index-th argument as an Int, or the fallback." + ! [Args.get] + match Vector.get(Vector.fromList(Args.get()), index) + Option.Some(text) -> Result.withDefault(Int.fromString(text), fallback) + Option.None -> fallback + +fn main() -> Unit + ! [Args.get, Console.print] + size = arg(0, 1000) + a = plain(Flight(progress = {}, name = "plain"), size) + b = selfKeep(Flight(progress = {1 => 1}, name = "keep"), 3) + c = ping(Flight(progress = {}, name = "mutual"), size) + Console.print("{Map.len(a.progress)} {a.name} {Map.len(b.progress)} {b.name} {Map.len(c.progress)} {c.name}") diff --git a/tests/rust_work_spec.rs b/tests/rust_work_spec.rs index 3ccdb4801..23b7f68a9 100644 --- a/tests/rust_work_spec.rs +++ b/tests/rust_work_spec.rs @@ -517,6 +517,62 @@ fn a_record_gives_up_its_fields_at_its_last_use() { result.unwrap_or_else(|error| panic!("{error}")); } +/// An update that replaces a field read earlier builds, and the read moves. +/// +/// `progress = flight.progress` followed by +/// `Flight.update(flight, progress = grow(progress, key))` moved the field +/// out in the `let` of a mutual tail-call arm and then moved the whole +/// `flight` into the update, which rustc rejects (E0382). The arm now sees +/// the same field-move facts as any other body, so the update keeps the +/// other fields with `..flight`. The field read inside the update +/// (`grow(flight.progress, ...)`) and the one bound by a `let` both move into +/// `grow`, which then takes its Map by value and inserts in place. A loop +/// that reads a field and later hands on the whole record clones the field. +#[test] +fn an_update_after_a_field_read_of_the_replaced_field_builds_and_moves() { + let name = "rust_update_after_field_move"; + let ws = temp_dir(name); + let project = ws.join("project"); + fs::create_dir_all(&project).expect("create project dir"); + let args = ["300"]; + let result = (|| { + compile_rust(name, &project, name, &[])?; + let entry = fs::read_to_string(project.join("src/aver_generated/entry/mod.rs")) + .map_err(|error| format!("read the generated entry module: {error}"))?; + for moved in [ + "pub fn grow(mut progress @ _: aver_rt::AverMap<", + "let progress @ _ = flight.progress;\n", + "__MutualTco1::Pong(Flight { progress: grow(progress, left.clone()), ..flight }", + "__MutualTco1::Ping(Flight { progress: grow(flight.progress, ", + "Flight { progress: grow(progress, key), ..flight }", + "Flight { progress: grow(flight.progress, key), ..flight }", + "let progress @ _ = flight.progress.clone();", + ] { + if !entry.contains(moved) { + return Err(format!( + "{name}: missing `{moved}` in the generated entry module:\n{entry}" + )); + } + } + if entry.contains("let mut __updated = flight; __updated.progress") { + return Err(format!( + "{name}: an update moves a record whose field was already moved out:\n{entry}" + )); + } + let vm = run_vm_with(name, &args)?; + let bin = cargo_build(&project, name)?; + let rust = run_binary_with(&bin, &args)?; + if vm != rust { + return Err(format!( + "{name}: stdout mismatch\n--- VM ---\n{vm}\n--- Rust ---\n{rust}" + )); + } + Ok(()) + })(); + let _ = fs::remove_dir_all(&ws); + result.unwrap_or_else(|error| panic!("{error}")); +} + /// Runs one backend against a loopback peer, on a port nobody else holds. fn with_peer(run: impl FnOnce(&str) -> Result) -> Result { let port = free_port();