From f3733087bcef7be5b02ed5c4695a3386b0ea916d Mon Sep 17 00:00:00 2001 From: jasisz Date: Fri, 25 Sep 2026 09:58:29 +0200 Subject: [PATCH 1/2] Derive perf-shared-update from the field moves generated code makes The check decided whether a record still held a collection with its own reading of the AST, which followed the VM's field takes. It disagreed with what generated Rust does in both directions: - done = toppedUp(s.pool, s.book, s.nextKey)? followed by S.update(s, pool = ..., book = ..., nextKey = ...) warned, although the Book moves out of s: nothing else reads it and the update replaces it. - match (s.pool, s.book) with (pool, book) -> f(s, g(book)) did not warn, although s is still used whole, so the match copies the Book and g copies it again when it inserts. The check now lowers the module to MIR the way it compiles, against the program's symbol table, and asks field_moves, the analysis the Rust backend moves fields by. A value handed to an update is reported when it is a field read that does not move, or a local bound to one by a let or a match and handed on at its last use; a record a loop hands on unchanged never gives up a field. Which callees update in place is still read from the source, followed into dependencies. field_moves now also understands the base of an update of a field chain (T.update(s.window, created = ...)): when nothing outside the update reads that part of s again, the base moves what the update keeps and the replaced field may move out before it. Generated Rust emits Window { created: ..., ..setting.window } instead of cloning both, so the nested update the VM already did in place no longer copies the Map in Rust, and the check agrees with both. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 2 + docs/diagnostics-slugs.md | 2 +- src/checker/mod.rs | 2 +- src/checker/shared_update.rs | 564 ++++++++++-------- src/diagnostics/analyze.rs | 9 +- src/ir/mir/field_moves.rs | 163 ++++- src/types/checker/mod.rs | 14 + .../fixtures/rust_nested_update_moves/main.av | 49 ++ tests/rust_work_spec.rs | 39 ++ tests/shared_update_spec.rs | 40 +- 10 files changed, 605 insertions(+), 279 deletions(-) create mode 100644 tests/fixtures/rust_nested_update_moves/main.av diff --git a/CHANGELOG.md b/CHANGELOG.md index 979cebba7..8808c2bb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,8 @@ The generated loop is now written from the program's source alone, and the manif ### Fixed +- **`perf-shared-update` reports the copies generated code makes, and only those.** The check now reads the same field moves the Rust backend makes, from the module lowered as it compiles. `done = toppedUp(s.pool, s.book, s.nextKey)?` followed by `S.update(s, pool = done.pool, book = done.book, nextKey = done.nextKey)` moves the Book out of `s` and no longer warns. `match (s.window.created, s.height)` with `(created, _) -> f(s, absorbed(created, k))` copies the Map, since `s` still holds it, and now warns: "`absorbed` updates `created`, read from `s.window.created`, a Map that is still held by `s`". A field of a record a loop hands on unchanged is reported too. +- **Generated Rust moves the rest of a nested record into its update.** `Setting.update(setting, window = Window.update(setting.window, created = Map.set(setting.window.created, k, v)), height = setting.height + 1)` used to clone `setting.window` and `setting.window.created`, so every `Map.set` copied the Map. When nothing reads that part of `setting` again, the Map now moves into `Map.set` and the other fields of `setting.window` move into the new `Window`. - **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/docs/diagnostics-slugs.md b/docs/diagnostics-slugs.md index 4158ed6e5..6274880c3 100644 --- a/docs/diagnostics-slugs.md +++ b/docs/diagnostics-slugs.md @@ -57,7 +57,7 @@ When a real signature shape is deliberately uninhabited, keep the suppression ex | `perf-list-len` | warning | `List.len` called inside recursion, which adds up to O(n²). | Compute the length once, outside the recursion. | | `perf-string-concat` | warning | String concatenation inside recursion. | Accumulate in a list and join once. | | `perf-nested-match` | warning | Nested `match` on the same subject. | Combine into one `match`. | -| `perf-shared-update` | warning | A Map or Vector read out of a record (`setting.window.created`) is updated in place while the record still holds it, in a function that runs again and again (recursive, reached from a recursive function of its module, or part of an answer module). The update is `Map.set`, `Map.remove` or `Vector.set` on the field, or a call that hands the field to one of them and returns the updated collection (followed into the dependencies the call names). The record still holds it when it is read again after that call, or when it was already passed whole to the call or record the update is an argument of. Each such update copies the whole collection. A record update or literal that reads the field it replaces once, reads nothing else of the record but other fields, and is the record's last use is not reported: the VM takes the field out first. The same goes for a field further down (`setting.window.created`) read once in such a literal or update, inside updates of `setting.window` that write it. Not seen: a caller that keeps the record it passed, an alias made through a binding, one collection in two records. | Take the field out of the record before updating it (bind the parts with a `match` and carry on with a record that no longer holds them, or give it an empty collection in their place), or read it at the record's last use. | +| `perf-shared-update` | warning | A Map or Vector read out of a record (`setting.window.created`, or a local bound to such a field by a `let` or a `match`) is updated in place while the record still holds it, in a function that runs again and again (recursive, reached from a recursive function of its module, or part of an answer module). The update is `Map.set`, `Map.remove` or `Vector.set` on the value, or a call that hands it to one of them and returns the updated collection (followed into the dependencies the call names). The record still holds the field whenever the read does not move it, which is decided by the same field-move analysis generated code uses: a field read moves when every other read of the record runs in another branch, finished earlier, or reads a disjoint part (such as the rest of the record handed to the update that replaces the field), and nothing reads the record after. Each update of a field that does not move copies the whole collection. A record a loop hands on unchanged never gives up a field. Not seen: a caller that keeps the record it passed, a callee that borrows the value and copies it itself, an alias made through a binding, one collection in two records. | Read the field where nothing reads that part of the record again, for example in the update of the record that replaces it (`S.update(s, book = g(s.book))`, or `b = s.book` followed by `S.update(s, book = g(b))`), so it moves out of the record. Binding it with a `match` does not help while the record is still used whole afterwards. | | `perf-loop-invariant` | warning | An expression is recomputed on every recursive call but does not depend on the recursion. | Hoist it outside the recursion. | | `cse-match` | warning | Subexpression computed in both the match condition and an arm body. | Bind it once above the match. | | `cse-duplicate` | warning | Expression computed more than once in one function. | Bind it and reuse it. | diff --git a/src/checker/mod.rs b/src/checker/mod.rs index e646692a2..4821c6f51 100644 --- a/src/checker/mod.rs +++ b/src/checker/mod.rs @@ -323,6 +323,6 @@ pub use module_effects::{collect_module_effects_warnings, collect_module_effects pub use naming::{collect_naming_warnings, collect_naming_warnings_in}; pub use perf::{collect_perf_warnings, collect_perf_warnings_in}; pub use serve_path::{collect_serve_path_warnings, collect_serve_path_warnings_in}; -pub use shared_update::{ModuleSource, collect_shared_update_warnings}; +pub use shared_update::{ModuleSource, ProgramSymbols, collect_shared_update_warnings}; pub use traversal::collect_traversal_warnings_in; pub use verify::{expr_to_str, merge_verify_blocks, verify_block_label}; diff --git a/src/checker/shared_update.rs b/src/checker/shared_update.rs index 2d8a4deba..f618ec755 100644 --- a/src/checker/shared_update.rs +++ b/src/checker/shared_update.rs @@ -16,34 +16,34 @@ //! `Map.remove` or `Vector.set`, or at a parameter position of a function //! that hands that parameter, directly or through further calls, to one of //! them (followed into the dependencies the call names). -//! - **A value the compiler can see is shared.** Either it is read out of a -//! record — `setting.window.created` — while the record stays reachable -//! (the local is read again later, or it was already passed whole to the -//! call or record the update is an argument of), or it is a local -//! collection that is read again after the update. +//! - **A copy the backends make.** The value is a field read out of a record +//! local — `setting.window.created`, or a local bound to one by a `let` or +//! a `match` and handed on at its last use — and the read does not move +//! the field out of the record. The module is lowered the way it compiles +//! and the answer is `field_moves`, the analysis generated Rust moves +//! fields by: a field read moves when every other read of the record runs +//! in another branch, finished earlier, or reads a disjoint part (the rest +//! of the record handed on to the update that replaces the field), and the +//! record is not used after. Any other field read is a copy, and the record +//! keeps the original. A record a loop hands on unchanged keeps every +//! field. //! - **Repeated.** The function doing it is recursive, is reached from a //! recursive function of its module, or belongs to an answer module, whose //! functions run once per request. //! -//! One shape is left alone on purpose: a record literal or `T.update(x, …)` -//! that reads the field it replaces once and otherwise only other fields of -//! `x`, with `x` dead afterwards. The VM takes that field out of the record -//! before the update, so the collection is not shared when the update runs. -//! The same goes for a field further down (`x.window.created`) read once in -//! such a literal or update, inside updates of `x.window` that write it. -//! //! What it misses: a caller that keeps a record it passed to the function -//! doing the update (the function cannot see its callers), aliases made -//! through a binding (`kept = state`), a collection shared by two records, and -//! calls through function values. Each of those still copies; none of them is -//! visible in one function's body without the whole-program ownership facts. +//! doing the update (the function cannot see its callers), a callee that +//! borrows the value and copies it itself, aliases made through a binding +//! (`kept = state`), a collection shared by two records, and calls through +//! function values. use std::collections::{HashMap, HashSet, VecDeque}; use crate::ast::{Expr, FnDef, Spanned, Stmt, StrPart, TopLevel}; use super::CheckFinding; -use crate::ir::field_take::ReadLog; +use crate::ir::mir::expr::walk_children; +use crate::ir::mir::{LocalId, MirCallee, MirExpr, MirFn, MirPattern, MirProgram}; /// The builtins that update their first argument in place when they own it. const UPDATES: [(&str, Kind); 3] = [ @@ -367,198 +367,256 @@ fn repeated_fns(items: &[TopLevel]) -> HashSet { repeated } -/// A record literal or update the VM may take a field out of: the paths of -/// each local it may take. The VM's own planning decides -/// (`vm::compiler::field_take`), fed the same reads from the checked AST: -/// every read of the local in the values is a path of fields, no other read -/// goes through the path, the local dies there, and every update of the local -/// or a shorter path around the read writes the field the path goes on -/// through. -#[derive(Default)] -struct Scope { - takes: HashMap>>, +/// Where a value handed to an update comes from: a field of a record local, +/// read either at the update itself or earlier into the local handed on. +struct Origin<'m> { + /// The field read, `s.window.created`: the node the backends move or + /// copy. + read: &'m MirExpr, + /// The record local's name and the path read out of it. + record: String, + path: Vec, +} + +/// One function's lowered body, with what the backends decide about it. +struct Body<'m> { + /// Field reads that move their field out of the record + /// (`field_moves::movable_projections`); every other field read copies + /// it, and the record keeps the original. + movable: HashSet, + /// Params a self tail call hands on unchanged: the loop keeps them whole, + /// so no field of one ever moves. + carried: HashSet, + /// Locals bound to a field read, by a `let` or by a `match` on it. + bound: HashMap>, } -/// `local.f1.….fn` as `(slot, last_use, [f1, …, fn])`; a bare local has no -/// fields. -fn resolved_path(expr: &Expr) -> Option<(u16, bool, Vec)> { +/// `local.f1.….fn` as `(local, name, [f1, …, fn])`. +fn projection_path(expr: &MirExpr) -> Option<(LocalId, String, Vec)> { match expr { - Expr::Resolved { slot, last_use, .. } => Some((*slot, last_use.0, Vec::new())), - Expr::Attr(base, field) => { - let (slot, last_use, mut fields) = resolved_path(&base.node)?; - fields.push(field.clone()); - Some((slot, last_use, fields)) + MirExpr::Local(local) if !local.node.name.is_empty() => { + Some((local.node.slot, local.node.name.clone(), Vec::new())) + } + MirExpr::Project(project) => { + let (slot, name, mut path) = projection_path(&project.node.base.node)?; + path.push(project.node.field.clone()); + Some((slot, name, path)) } _ => None, } } -fn collect_reads(expr: &Spanned, log: &mut ReadLog) { - if let Some((slot, last_use, fields)) = resolved_path(&expr.node) { - log.read(u32::from(slot), last_use, fields); - return; - } - match &expr.node { - Expr::RecordUpdate { base, updates, .. } if resolved_path(&base.node).is_some() => { - let (slot, last_use, fields) = resolved_path(&base.node).expect("checked above"); - let written = updates.iter().map(|(name, _)| name.clone()).collect(); - log.enter_base(u32::from(slot), last_use, fields, written); - for (_, value) in updates { - collect_reads(value, log); +fn field_read(expr: &MirExpr) -> Option> { + let (_, record, path) = projection_path(expr)?; + (!path.is_empty()).then_some(Origin { + read: expr, + record, + path, + }) +} + +/// Every local a pattern binds, with the part of the subject it binds when +/// that part is a field read. +fn bind_pattern<'m>( + pattern: &MirPattern, + subject: &'m MirExpr, + bound: &mut HashMap>, +) { + match pattern { + MirPattern::Bind(slot, _) => { + if let Some(origin) = field_read(subject) { + bound.insert(*slot, origin); } - log.leave_base(); } - other => children(other, &mut |child| collect_reads(child, log)), + MirPattern::Tuple(items) => { + if let MirExpr::Tuple(parts) = subject { + for (item, part) in items.iter().zip(parts) { + bind_pattern(item, &part.node, bound); + } + } else { + for item in items { + bind_pattern(item, subject, bound); + } + } + } + // A payload or a list element is part of the field it was matched + // out of, and shares its backing with it. + MirPattern::Ctor { bindings, .. } => { + for slot in bindings { + if let Some(origin) = field_read(subject) { + bound.insert(*slot, origin); + } + } + } + MirPattern::Cons { head, tail, .. } => { + for slot in [head, tail] { + if let Some(origin) = field_read(subject) { + bound.insert(*slot, origin); + } + } + } + MirPattern::Wildcard | MirPattern::Literal(_) | MirPattern::EmptyList => {} } } -fn scope_of<'e>( - base: Option<(u16, Vec)>, - written: &HashSet<&str>, - values: impl Iterator>, -) -> Scope { - let mut log = ReadLog::default(); - if let Some((slot, fields)) = base { - let written = written.iter().map(|field| field.to_string()).collect(); - log.enter_base(u32::from(slot), false, fields, written); - } - for value in values { - collect_reads(value, &mut log); - } - let mut scope = Scope::default(); - for plan in log.plans() { - let Ok(slot) = u16::try_from(plan.slot) else { - continue; - }; - scope.takes.insert( - slot, - plan.paths.into_iter().map(|path| path.fields).collect(), - ); +fn collect_bound<'m>(expr: &'m MirExpr, bound: &mut HashMap>) { + match expr { + MirExpr::Let(binding) => { + let value = &binding.node.value.node; + if let Some(origin) = field_read(value) { + bound.insert(binding.node.binding, origin); + } + } + MirExpr::Match(matched) => { + for arm in &matched.node.arms { + bind_pattern(&arm.pattern, &matched.node.subject.node, bound); + } + } + _ => {} } - scope + walk_children(expr, &mut |child| collect_bound(child, bound)); } -/// Whether the last read of local `slot` is inside `args`. -fn last_read_within(slot: u16, args: &[Spanned]) -> bool { - fn visit(expr: &Spanned, slot: u16, found: &mut bool) { - if let Expr::Resolved { - slot: read, - last_use, - .. - } = &expr.node - && *read == slot - && last_use.0 +/// The params every self tail call of `f` hands on as they are. +fn carried_params(f: &MirFn) -> HashSet { + fn visit(expr: &MirExpr, f: &MirFn, kept: &mut Vec, calls: &mut usize) { + if let MirExpr::TailCall(call) = expr + && call.node.target == f.fn_id { - *found = true; + *calls += 1; + for (index, param) in f.params.iter().enumerate() { + let same = call.node.args.get(index).is_some_and(|arg| { + matches!(&arg.node, MirExpr::Local(local) if local.node.slot == param.local) + }); + kept[index] &= same; + } } - children(&expr.node, &mut |child| visit(child, slot, found)); + walk_children(expr, &mut |child| visit(child, f, kept, calls)); } - let mut found = false; - for arg in args { - visit(arg, slot, &mut found); - } - found -} - -/// The locals an evaluated value holds whole: a bare read, or one inside a -/// tuple, list, constructor or record built right there. -fn held_locals(expr: &Expr, out: &mut Vec<(u16, String)>) { - match expr { - Expr::Resolved { slot, name, .. } => out.push((*slot, name.clone())), - Expr::Tuple(items) | Expr::List(items) => { - items.iter().for_each(|item| held_locals(&item.node, out)) - } - Expr::Constructor(_, Some(inner)) => held_locals(&inner.node, out), - Expr::RecordCreate { fields, .. } => fields - .iter() - .for_each(|(_, value)| held_locals(&value.node, out)), - Expr::RecordUpdate { base, updates, .. } => { - held_locals(&base.node, out); - updates - .iter() - .for_each(|(_, value)| held_locals(&value.node, out)); - } - _ => {} + let mut kept = vec![true; f.params.len()]; + let mut calls = 0; + visit(&f.body.node, f, &mut kept, &mut calls); + if calls == 0 { + return HashSet::new(); } + f.params + .iter() + .zip(kept) + .filter_map(|(param, kept)| kept.then_some(param.local)) + .collect() } -struct Walk<'s, 'd> { +struct Walk<'s, 'd, 'm> { own: &'s Summary, deps: &'s mut Dependencies<'d>, + program: &'m MirProgram, + symbols: &'s crate::ir::SymbolTable, + body: &'s Body<'m>, fn_name: String, module: Option, - /// Locals whose whole value an enclosing call or constructor has already - /// evaluated and still holds, innermost last. The flag marks the base of - /// an enclosing record update. - pending: Vec<(u16, String, bool)>, - scopes: Vec, findings: Vec, } -impl Walk<'_, '_> { - /// Whether the VM takes the field at the end of `path` out of local `slot` - /// before the update: an enclosing literal or update plans it. - fn taken_first(&self, slot: u16, path: &[&str]) -> bool { - self.scopes - .iter() - .find(|scope| scope.takes.contains_key(&slot)) - .is_some_and(|scope| { - scope.takes[&slot] - .iter() - .any(|taken| taken.iter().map(String::as_str).eq(path.iter().copied())) - }) +impl Walk<'_, '_, '_> { + /// The name a call spells its callee by, as the summaries key it: the + /// builtin's name, a function of this module by its own name, one of a + /// dependency as `Module.fn`. + fn callee_name(&self, callee: &MirCallee) -> Option { + match callee { + MirCallee::Builtin(id) => Some(self.program.builtin_name(*id).to_string()), + MirCallee::Fn(id) => self.fn_name_of(*id), + _ => None, + } } - fn check_site(&mut self, callee: &str, index: usize, args: &[Spanned]) { - let arg = &args[index]; + fn fn_name_of(&self, id: crate::ir::FnId) -> Option { + let key = &self.symbols.fn_entry(id).key; + Some(match &key.scope { + Some(scope) if Some(scope.as_str()) != self.module.as_deref() => { + format!("{scope}.{}", key.name) + } + _ => key.name.clone(), + }) + } + + /// Whether the backends copy the field `origin` reads, leaving the + /// original in the record: the read does not move it, or the record is + /// a param the loop carries whole. + fn copies(&self, origin: &Origin<'_>) -> bool { + let moved = self + .body + .movable + .contains(&(origin.read as *const MirExpr as usize)); + let carried = projection_path(origin.read) + .is_some_and(|(slot, _, _)| self.body.carried.contains(&slot)); + !moved || carried + } + + fn check_site(&mut self, callee: &str, index: usize, arg: &Spanned) { let Some(kind) = update_at(callee, index, self.own, self.deps) else { return; }; - // The record the value is read out of, and its first field. - let mut fields: Vec<&str> = Vec::new(); - let mut root = &arg.node; - while let Expr::Attr(base, field) = root { - fields.push(field); - root = &base.node; - } - let Expr::Resolved { slot, name, .. } = root else { - return; + let body = self.body; + // The value is a field read here, or a local bound to one earlier + // and read here for the last time. A local read again later is left + // alone: the caller keeping both versions is usually the point (a + // scope, an undo). A record field is different: the record is the + // only reason the old one lives. + let found; + let (origin, via) = match &arg.node { + MirExpr::Local(local) => { + let Some(origin) = body.bound.get(&local.node.slot) else { + return; + }; + if !local.node.last_use { + return; + } + (origin, Some(local.node.name.clone())) + } + _ => { + let Some(origin) = field_read(&arg.node) else { + return; + }; + found = origin; + (&found, None) + } }; - // A bare local read again later is left alone: the caller keeping - // both versions is usually the point (a scope, an undo). A record - // field is different: the record is the only reason the old one lives. - if fields.is_empty() { - return; + if self.copies(origin) { + self.report(callee, index, kind, origin, via, arg.line); } - fields.reverse(); - let exempt = self.taken_first(*slot, &fields); - let held_by_pending = self - .pending - .iter() - .any(|(pending, _, is_base)| pending == slot && !(*is_base && exempt)); - // The update runs once every argument of its call is evaluated, so - // a read in a later argument is over by then; only a read after the - // whole call keeps the local holding the value. - let read_later = !last_read_within(*slot, args) && !exempt; - if !(held_by_pending || read_later) { - return; - } - let shown = format!("{name}.{}", fields.join(".")); + } + + fn report( + &mut self, + callee: &str, + index: usize, + kind: Kind, + origin: &Origin<'_>, + via: Option, + line: usize, + ) { + let name = &origin.record; + let read = format!("{name}.{}", origin.path.join(".")); let what = kind.name(); let repair = format!( - "take it out of `{name}` first (bind the parts with a match and carry on with a `{name}` that no longer holds them), or read it at the last use of `{name}`" + "read it where nothing reads that part of `{name}` again, for example in the update of `{name}` that replaces it, so the {what} moves out of `{name}` instead of being shared with it" ); - let message = if builtin_update(callee, index).is_some() { - format!( - "`{callee}` on `{shown}` updates a {what} that is still held by `{name}`; each update copies the whole {what} — {repair}" - ) - } else { - format!( - "`{callee}` updates `{shown}`, a {what} that is still held by `{name}`; each call copies the whole {what} — {repair}" - ) + let message = match (builtin_update(callee, index).is_some(), &via) { + (true, None) => format!( + "`{callee}` on `{read}` updates a {what} that is still held by `{name}`; each update copies the whole {what} — {repair}" + ), + (true, Some(local)) => format!( + "`{callee}` on `{local}`, read from `{read}`, updates a {what} that is still held by `{name}`; each update copies the whole {what} — {repair}" + ), + (false, None) => format!( + "`{callee}` updates `{read}`, a {what} that is still held by `{name}`; each call copies the whole {what} — {repair}" + ), + (false, Some(local)) => format!( + "`{callee}` updates `{local}`, read from `{read}`, a {what} that is still held by `{name}`; each call copies the whole {what} — {repair}" + ), }; self.findings.push(CheckFinding { - line: arg.line, + line, module: self.module.clone(), file: None, fn_name: Some(self.fn_name.clone()), @@ -567,93 +625,72 @@ impl Walk<'_, '_> { }); } - /// Walk the arguments of one call: each sees the whole values the earlier - /// ones hold, and each is checked as an update site. - fn walk_call(&mut self, callee: Option<&str>, args: &[Spanned]) { - let mark = self.pending.len(); - for (index, item) in args.iter().enumerate() { - if let Some(callee) = callee { - self.check_site(callee, index, args); + fn walk(&mut self, expr: &MirExpr) { + match expr { + MirExpr::Call(call) => { + if let Some(name) = self.callee_name(&call.node.callee) { + for (index, arg) in call.node.args.iter().enumerate() { + self.check_site(&name, index, arg); + } + } + } + MirExpr::TailCall(call) => { + if let Some(name) = self.fn_name_of(call.node.target) { + for (index, arg) in call.node.args.iter().enumerate() { + self.check_site(&name, index, arg); + } + } } - self.walk(item); - let mut held = Vec::new(); - held_locals(&item.node, &mut held); - self.pending - .extend(held.into_iter().map(|(slot, name)| (slot, name, false))); + _ => {} } - self.pending.truncate(mark); + walk_children(expr, &mut |child| self.walk(child)); } +} - /// Walk the items of one aggregate in evaluation order: each sees the - /// whole values the earlier ones hold. - fn walk_in_order<'e>(&mut self, items: impl Iterator>) { - let mark = self.pending.len(); - for item in items { - self.walk(item); - let mut held = Vec::new(); - held_locals(&item.node, &mut held); - self.pending - .extend(held.into_iter().map(|(slot, name)| (slot, name, false))); +/// Whether some repeated function of `items` hands anything to an update at +/// all. Lowering the module is only worth it then. +fn has_update_site( + items: &[TopLevel], + repeated: &HashSet, + own: &Summary, + deps: &mut Dependencies<'_>, +) -> bool { + let mut found = false; + for fd in fn_defs(items) + .into_iter() + .filter(|fd| repeated.contains(&fd.name)) + { + for expr in body_exprs(fd) { + each_call(expr, &mut |callee, args| { + found = found + || (0..args.len()).any(|index| update_at(callee, index, own, deps).is_some()); + }); } - self.pending.truncate(mark); } + found +} - fn walk(&mut self, expr: &Spanned) { - match &expr.node { - Expr::FnCall(callee, args) => { - let name = dotted(&callee.node); - self.walk_call(name.as_deref(), args); - } - Expr::TailCall(tail) => { - let target = tail.target.clone(); - self.walk_call(Some(&target), &tail.args); - } - Expr::Tuple(items) | Expr::List(items) | Expr::IndependentProduct(items, _) => { - self.walk_in_order(items.iter()) - } - Expr::RecordCreate { fields, .. } => { - let written: HashSet<&str> = fields.iter().map(|(n, _)| n.as_str()).collect(); - self.scopes.push(scope_of( - None, - &written, - fields.iter().map(|(_, value)| value), - )); - self.walk_in_order(fields.iter().map(|(_, value)| value)); - self.scopes.pop(); - } - Expr::RecordUpdate { base, updates, .. } => { - self.walk(base); - let base_slot = match &base.node { - Expr::Resolved { slot, .. } => Some(*slot), - _ => None, - }; - let written: HashSet<&str> = updates.iter().map(|(n, _)| n.as_str()).collect(); - self.scopes.push(scope_of( - resolved_path(&base.node).map(|(slot, _, fields)| (slot, fields)), - &written, - updates.iter().map(|(_, value)| value), - )); - let mark = self.pending.len(); - let mut held = Vec::new(); - held_locals(&base.node, &mut held); - self.pending.extend( - held.into_iter() - .map(|(slot, name)| (slot, name, Some(slot) == base_slot)), - ); - self.walk_in_order(updates.iter().map(|(_, value)| value)); - self.pending.truncate(mark); - self.scopes.pop(); - } - other => children(other, &mut |child| self.walk(child)), - } - } +/// The module lowered the way every backend lowers it, so the check reads +/// the same field moves they make. +fn lowered(items: &[TopLevel], symbols: &crate::ir::SymbolTable) -> MirProgram { + let mut items = items.to_vec(); + crate::resolver::resolve_program(&mut items); + crate::ir::last_use::annotate_program_last_use(&mut items); + let resolved = crate::ir::hir::resolve_program(symbols, &items); + crate::ir::mir::optimize(crate::ir::mir::lower_program(&resolved)) } +/// The symbol table of the program a module is checked in: the module and +/// the dependencies it loads. +pub type ProgramSymbols<'a> = dyn Fn(&[TopLevel]) -> crate::ir::SymbolTable + 'a; + /// Warnings for one module. `items` is the module as the checker saw it; -/// `source` finds the dependencies its calls name. +/// `source` finds the dependencies its calls name and `symbols` builds the +/// program's symbol table, so the module lowers the way it compiles. pub fn collect_shared_update_warnings( items: &[TopLevel], source: &ModuleSource<'_>, + symbols: &ProgramSymbols<'_>, ) -> Vec { let repeated = repeated_fns(items); if repeated.is_empty() { @@ -661,41 +698,42 @@ pub fn collect_shared_update_warnings( } let mut deps = Dependencies::new(source); let own = summarize(items, &mut deps); + if !has_update_site(items, &repeated, &own, &mut deps) { + return Vec::new(); + } let module = super::module_name_for_items(items); - - // Last use needs slots; resolve a copy of the repeated functions only. - let mut resolved: Vec = items - .iter() - .filter(|item| match item { - TopLevel::FnDef(fd) => repeated.contains(&fd.name), - TopLevel::TypeDef(_) => true, - _ => false, - }) - .cloned() - .collect(); - crate::resolver::resolve_program(&mut resolved); - crate::ir::last_use::annotate_program_last_use(&mut resolved); + let symbols = symbols(items); + let program = lowered(items, &symbols); let mut findings = Vec::new(); + let mut fns: Vec<&MirFn> = program.iter().map(|(_, f)| f).collect(); + fns.sort_by_key(|f| f.fn_id.0); // Functions the compiler generated (the loop's `__…` helpers) are not the // author's to change, so they are not reported at the author's file. - for fd in fn_defs(&resolved) + for f in fns .into_iter() - .filter(|fd| !fd.name.starts_with("__")) + .filter(|f| repeated.contains(&f.name) && !f.name.starts_with("__")) { + let mut bound = HashMap::new(); + collect_bound(&f.body.node, &mut bound); + let body = Body { + movable: crate::ir::mir::field_moves::movable_projections(&f.body.node), + carried: carried_params(f), + bound, + }; let mut walk = Walk { own: &own, deps: &mut deps, - fn_name: fd.name.clone(), + program: &program, + symbols: &symbols, + body: &body, + fn_name: f.name.clone(), module: module.clone(), - pending: Vec::new(), - scopes: Vec::new(), findings: Vec::new(), }; - for expr in body_exprs(fd) { - walk.walk(expr); - } + walk.walk(&f.body.node); findings.extend(walk.findings); } + findings.sort_by_key(|finding| finding.line); findings } diff --git a/src/diagnostics/analyze.rs b/src/diagnostics/analyze.rs index bab1aed54..29cf8b672 100644 --- a/src/diagnostics/analyze.rs +++ b/src/diagnostics/analyze.rs @@ -488,7 +488,14 @@ fn analyze_prechecked_items_impl( let text = std::fs::read_to_string(path).ok()?; crate::source::parse_source(&text).ok() }; - for w in collect_shared_update_warnings(transformed, &source) { + let symbols = |items: &[TopLevel]| { + crate::types::checker::program_symbols( + items, + options.loaded_modules.as_deref(), + options.module_base_dir.as_deref(), + ) + }; + for w in collect_shared_update_warnings(transformed, &source, &symbols) { diagnostics.push(from_check_finding_with_index( Severity::Warning, &w, diff --git a/src/ir/mir/field_moves.rs b/src/ir/mir/field_moves.rs index aebe77d67..28ccd7cf0 100644 --- a/src/ir/mir/field_moves.rs +++ b/src/ir/mir/field_moves.rs @@ -35,18 +35,28 @@ use super::program::LocalId; enum Part { /// `s` (empty path) or `s.a.b`. Path(Vec), - /// The base of `T.update(s, a = ...)`: every field of `s` except the - /// replaced ones. - AllExcept(Vec), + /// The base of `T.update(s, a = ...)` (empty prefix) or of + /// `T.update(s.window, a = ...)` (prefix `window`): every field under + /// the prefix except the replaced ones. + AllExcept { + prefix: Vec, + replaced: Vec, + }, } #[derive(Debug)] struct Read { part: Part, + /// For the base of an update: the update node's address, so the reads + /// inside its own field values can be told apart. + update: Option, /// Address of the read's outermost node (the projection chain or the /// local itself). addr: usize, last_use: bool, + /// Whether the local read under this part is the local's last use (for + /// a field chain base, `last_use` is whether the base may move). + root_last_use: bool, in_product: bool, /// Each ancestor on the way down from the body: its address and the /// index of the child the read sits under. @@ -65,11 +75,35 @@ enum Branching { /// Addresses (`&MirExpr as *const _ as usize`) of the projection nodes in /// `body` that may move their field out of their root local. +/// +/// The base of an update of a field chain (`T.update(s.window, a = ...)`) +/// is in the set too when it may move what the update keeps: the rest of +/// `s.window` then moves into the new record instead of being cloned, and +/// the fields it replaces may move out before it. pub fn movable_projections(body: &MirExpr) -> HashSet { let mut reads: HashMap> = HashMap::new(); let mut trail = Vec::new(); collect(body, &mut trail, false, &mut reads); let mut out = HashSet::new(); + // A chain base is final once nothing outside its own update reads what + // it keeps; a base further in may depend on one further out, so this + // runs until nothing changes. + loop { + let mut changed = false; + for group in reads.values_mut() { + for bi in 0..group.len() { + if group[bi].last_use || !base_is_final(group, bi) { + continue; + } + group[bi].last_use = true; + out.insert(group[bi].addr); + changed = true; + } + } + if !changed { + break; + } + } for group in reads.values() { for (qi, q) in group.iter().enumerate() { let Part::Path(path) = &q.part else { @@ -98,6 +132,36 @@ pub fn movable_projections(body: &MirExpr) -> HashSet { out } +/// Whether the base `group[bi]` of an update of a field chain may move what +/// the update keeps: every other read of the local either sits inside the +/// update's own field values (they run first), cannot run together with +/// it, or reads a disjoint part; and one of the reads that can run with it +/// is the local's last use. +fn base_is_final(group: &[Read], bi: usize) -> bool { + let b = &group[bi]; + let (Part::AllExcept { prefix, .. }, Some(update)) = (&b.part, b.update) else { + return false; + }; + if prefix.is_empty() || b.in_product { + return false; + } + let mut ends_here = b.root_last_use; + for (oi, o) in group.iter().enumerate() { + if oi == bi || apart(o, b) { + continue; + } + let inside = o + .trail + .iter() + .any(|(addr, child, _)| *addr == update && *child >= 1); + if !inside && !disjoint(&o.part, o.last_use, prefix) { + return false; + } + ends_here |= o.last_use || o.root_last_use; + } + ends_here +} + /// The locals under `body` that give up a field through one of the /// `movable` projections. Such a local may be partially moved by the time /// it is read again, so it can only be read by fields it still holds. @@ -149,8 +213,10 @@ fn collect( MirExpr::Local(local) => { reads.entry(local.node.slot).or_default().push(Read { part: Part::Path(Vec::new()), + update: None, addr, last_use: local.node.last_use, + root_last_use: local.node.last_use, in_product, trail: trail.clone(), }); @@ -160,8 +226,10 @@ fn collect( if let Some((slot, last_use, path)) = projection_root(expr) { reads.entry(slot).or_default().push(Read { part: Part::Path(path), + update: None, addr, last_use, + root_last_use: last_use, in_product, trail: trail.clone(), }); @@ -169,17 +237,22 @@ fn collect( } } MirExpr::RecordUpdate(update) => { - if let MirExpr::Local(local) = &update.node.base.node { + if let Some((slot, root_last_use, prefix)) = projection_root(&update.node.base.node) { let replaced = update .node .updates .iter() .map(|field| field.name.clone()) .collect(); - reads.entry(local.node.slot).or_default().push(Read { - part: Part::AllExcept(replaced), + // A local base is final by its last-use flags; a field chain + // base is decided once every read is known. + let last_use = prefix.is_empty() && update_base_is_final(&update.node); + reads.entry(slot).or_default().push(Read { + part: Part::AllExcept { prefix, replaced }, + update: Some(addr), addr: &update.node.base.node as *const MirExpr as usize, - last_use: update_base_is_final(&update.node), + last_use, + root_last_use, in_product, trail: { let mut t = trail.clone(); @@ -354,15 +427,23 @@ fn apart(o: &Read, q: &Read) -> bool { } /// Whether a read of `part` leaves `path` untouched. The base of an update -/// that is not the local's last use is cloned whole, so it overlaps -/// everything. +/// that may not move is cloned whole, so it overlaps everything under its +/// prefix. fn disjoint(part: &Part, last_use: bool, path: &[String]) -> bool { match part { Part::Path(other) => { let common = other.len().min(path.len()); other[..common] != path[..common] } - Part::AllExcept(replaced) => last_use && replaced.iter().any(|field| *field == path[0]), + Part::AllExcept { prefix, replaced } => { + let common = prefix.len().min(path.len()); + if prefix[..common] != path[..common] { + return true; + } + last_use + && path.len() > prefix.len() + && replaced.iter().any(|field| *field == path[prefix.len()]) + } } } @@ -449,6 +530,68 @@ mod tests { assert!(movable.contains(&addr(&args(&chain.node.body)[0]))); } + #[test] + fn a_field_chain_base_moves_what_its_update_keeps() { + // Setting.update(s, window = Window.update(s.window, + // created = f(s.window.created)), height = g(s.height)) + let inner_base = project(local(0, false), "window"); + let inner = sp(MirExpr::RecordUpdate(Spanned::bare(MirRecordUpdate { + type_id: Some(TypeId(1)), + type_name: "Window".to_string(), + base: Box::new(inner_base), + updates: vec![MirRecordField { + name: "created".to_string(), + value: call(vec![project(project(local(0, false), "window"), "created")]), + }], + }))); + let body = sp(MirExpr::RecordUpdate(Spanned::bare(MirRecordUpdate { + type_id: Some(TypeId(0)), + type_name: "Setting".to_string(), + base: Box::new(local(0, false)), + updates: vec![ + MirRecordField { + name: "window".to_string(), + value: inner, + }, + MirRecordField { + name: "height".to_string(), + value: call(vec![project(local(0, true), "height")]), + }, + ], + }))); + let movable = movable_projections(&body.node); + let MirExpr::RecordUpdate(outer) = &body.node else { + unreachable!() + }; + let MirExpr::RecordUpdate(inner) = &outer.node.updates[0].value.node else { + unreachable!() + }; + assert!(movable.contains(&addr(&inner.node.base))); + assert!(movable.contains(&addr(&args(&inner.node.updates[0].value)[0]))); + } + + #[test] + fn a_field_chain_base_read_again_after_its_update_is_cloned() { + // f(Window.update(s.window, created = g(s.window.created)), s.window) + let update = sp(MirExpr::RecordUpdate(Spanned::bare(MirRecordUpdate { + type_id: Some(TypeId(1)), + type_name: "Window".to_string(), + base: Box::new(project(local(0, false), "window")), + updates: vec![MirRecordField { + name: "created".to_string(), + value: call(vec![project(project(local(0, false), "window"), "created")]), + }], + }))); + let body = call(vec![update, project(local(0, true), "window")]); + assert!(movable_projections(&body.node).len() <= 1); + let MirExpr::RecordUpdate(update) = &args(&body)[0].node else { + unreachable!() + }; + let movable = movable_projections(&body.node); + assert!(!movable.contains(&addr(&update.node.base))); + assert!(!movable.contains(&addr(&args(&update.node.updates[0].value)[0]))); + } + #[test] fn a_field_moves_beside_the_base_of_its_own_update() { let body = sp(MirExpr::RecordUpdate(Spanned::bare(MirRecordUpdate { diff --git a/src/types/checker/mod.rs b/src/types/checker/mod.rs index 5fc6772f2..3390f5ae5 100644 --- a/src/types/checker/mod.rs +++ b/src/types/checker/mod.rs @@ -136,6 +136,20 @@ fn build_symbols_for_items(items: &[TopLevel], base_dir: Option<&str>) -> Symbol SymbolTable::build(items, &dep_modules) } +/// The symbol table a check of `items` resolves against: the module and the +/// dependencies it loads, from `loaded` when the caller has them, else from +/// `base_dir`. Checks that lower the module the way it compiles read it. +pub(crate) fn program_symbols( + items: &[TopLevel], + loaded: Option<&[crate::source::LoadedModule]>, + base_dir: Option<&str>, +) -> SymbolTable { + match loaded { + Some(loaded) => build_symbols_with_loaded(items, loaded), + None => build_symbols_for_items(items, base_dir), + } +} + /// Pre-loaded variant of [`build_symbols_for_items`] for the /// `WithLoaded` typecheck driver (playground virtual FS). fn build_symbols_with_loaded( diff --git a/tests/fixtures/rust_nested_update_moves/main.av b/tests/fixtures/rust_nested_update_moves/main.av new file mode 100644 index 000000000..aae133fce --- /dev/null +++ b/tests/fixtures/rust_nested_update_moves/main.av @@ -0,0 +1,49 @@ +module Main + intent = "An update of a record inside an update of the record that holds it." + effects [Args.get, Console.print] + +record Window + created: Map + held: Int + +record Setting + window: Window + height: Int + +fn step(setting: Setting, left: Int) -> Setting + ? "One key per step, set through an update of the window inside an update of the setting." + match left <= 0 + true -> setting + false -> step(Setting.update(setting, window = Window.update(setting.window, created = Map.set(setting.window.created, left, left)), height = setting.height + 1), left - 1) + +verify step + step(Setting(window = Window(created = {}, held = 7), height = 0), 2) => Setting(window = Window(created = {1 => 1, 2 => 2}, held = 7), height = 2) + +fn kept(setting: Setting, left: Int) -> Int + ? "The same update with the old window read after it: the window is copied." + match left <= 0 + true -> setting.height + false -> keptOn(Window.update(setting.window, created = Map.set(setting.window.created, left, left)), setting.window, left) + +verify kept + kept(Setting(window = Window(created = {}, held = 7), height = 3), 0) => 3 + +fn keptOn(grown: Window, old: Window, left: Int) -> Int + ? "How many keys the grown window has over the old one." + Map.len(grown.created) - Map.len(old.created) + left + +verify keptOn + keptOn(Window(created = {1 => 1}, held = 0), Window(created = {}, held = 0), 1) => 2 + +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) + done = step(Setting(window = Window(created = {}, held = 7), height = 0), size) + Console.print("{Map.len(done.window.created)} {done.window.held} {done.height} {kept(done, 5)}") diff --git a/tests/rust_work_spec.rs b/tests/rust_work_spec.rs index 3ccdb4801..481843ddf 100644 --- a/tests/rust_work_spec.rs +++ b/tests/rust_work_spec.rs @@ -517,6 +517,45 @@ fn a_record_gives_up_its_fields_at_its_last_use() { result.unwrap_or_else(|error| panic!("{error}")); } +/// An update of `setting.window` inside the update of `setting` that replaces +/// it moves what it keeps: the Map moves into `Map.set` and the rest of the +/// window into the new `Window`, so no insert copies the Map. With the old +/// window read after the update, both are cloned instead. +#[test] +fn a_nested_update_moves_the_rest_of_the_inner_record() { + let name = "rust_nested_update_moves"; + let ws = temp_dir(name); + let project = ws.join("project"); + fs::create_dir_all(&project).expect("create project dir"); + let args = ["3000"]; + 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 expected in [ + "Setting { window: Window { created: setting.window.created.insert_owned(left.clone(), left.clone()), ..setting.window }, height: setting.height.add(", + "Window { created: setting.window.created.clone().insert_owned(left.clone(), left.clone()), ..setting.window.clone() }, &setting.window, left)", + ] { + if !entry.contains(expected) { + return Err(format!( + "{name}: missing `{expected}` in the generated entry module:\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(); diff --git a/tests/shared_update_spec.rs b/tests/shared_update_spec.rs index 0eaf398e7..8640e79b3 100644 --- a/tests/shared_update_spec.rs +++ b/tests/shared_update_spec.rs @@ -107,9 +107,10 @@ fn stepAgain(setting: Setting, left: Int) -> Setting ))); } -/// The parts are moved out of the record first (the btc fix): nothing warns. +/// A part bound by a `match` is a copy while the record is still used whole +/// afterwards: the record keeps its own, so the update copies the Map. #[test] -fn parts_moved_out_of_the_record_do_not_warn() { +fn a_part_bound_by_a_match_while_the_record_is_passed_on_warns() { let found = warnings(&program( r#" fn emptied(setting: Setting) -> Setting @@ -118,10 +119,43 @@ fn emptied(setting: Setting) -> Setting fn step(setting: Setting, left: Int) -> Setting ? "One Block per step." + match left <= 0 + true -> setting + false -> match (setting.window.created, setting.height) + (created, _) -> step(absorbedInto(emptied(setting), absorbed(created, left)), left - 1) +"#, + )); + assert_eq!(found.len(), 1, "{found:?}"); + assert!( + found[0].contains("`absorbed` updates `created`, read from `setting.window.created`, a Map that is still held by `setting`"), + "{}", + found[0] + ); +} + +/// A field read into a `let` and replaced by the update that uses the record +/// next moves out: nothing else reads it, so nothing warns. +#[test] +fn a_field_read_before_the_update_that_replaces_it_does_not_warn() { + let found = warnings(&program( + r#" +fn step(setting: Setting, left: Int) -> Setting + ? "One Block per step." + match left <= 0 + true -> setting + false -> stepOn(setting, left) + +fn stepOn(setting: Setting, left: Int) -> Setting + ? "The window grown, then set back." + grown = absorbed(setting.window.created, left) + step(Setting.update(setting, window = Window(created = grown, held = 0), height = setting.height + 1), left - 1) + +fn bound(setting: Setting, left: Int) -> Setting + ? "The same with the Map bound by a match." match left <= 0 true -> setting false -> match setting.window.created - created -> step(absorbedInto(emptied(setting), absorbed(created, left)), left - 1) + created -> bound(Setting.update(setting, window = Window(created = absorbed(created, left), held = 0)), left - 1) "#, )); assert!(found.is_empty(), "{found:?}"); From 7f387ea460a522f31d439e05203d834328f048ce Mon Sep 17 00:00:00 2001 From: jasisz Date: Fri, 25 Sep 2026 10:24:14 +0200 Subject: [PATCH 2/2] Treat a read inside a call in a match subject as over before the arms field_moves counted a read in a match subject or if condition as running together with the arms, so State.update(state, jobs = Map.set(state.jobs, task, job)) under match Map.get(state.jobs, task) did not move the Map. Generated Rust moved it anyway through its record-successor fast path, and the check derived from field_moves then warned about a copy that does not happen (the run guide example). A read inside a call in the subject is over once the call returns: the call's value holds nothing of it. Such a read is now apart from the arms, so the field read in the arm moves. A subject that is the field read itself, bound by the arm's pattern, still overlaps the arms. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/ir/mir/field_moves.rs | 70 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/src/ir/mir/field_moves.rs b/src/ir/mir/field_moves.rs index 28ccd7cf0..eca7ccb1d 100644 --- a/src/ir/mir/field_moves.rs +++ b/src/ir/mir/field_moves.rs @@ -70,6 +70,8 @@ enum Branching { Alternatives, /// `let`: child 0 is the value, child 1 the body. Sequence, + /// A call: its value holds nothing of what its arguments read. + Call, Other, } @@ -273,6 +275,7 @@ fn collect( let branching = match expr { MirExpr::Match(_) | MirExpr::IfThenElse(_) => Branching::Alternatives, MirExpr::Let(_) => Branching::Sequence, + MirExpr::Call(_) => Branching::Call, _ => Branching::Other, }; let in_product = in_product || matches!(expr, MirExpr::IndependentProduct(_)); @@ -411,16 +414,26 @@ fn final_reads( /// Whether `o` never runs together with `q`, or finishes before `q` starts /// without holding a borrow of the local. fn apart(o: &Read, q: &Read) -> bool { - for (a, b) in o.trail.iter().zip(q.trail.iter()) { + for (depth, (a, b)) in o.trail.iter().zip(q.trail.iter()).enumerate() { if a == b { continue; } let (_, o_child, branching) = *a; let q_child = b.1; return match branching { - Branching::Alternatives => o_child >= 1 && q_child >= 1, + // Two arms never run together. A read inside a call in the + // subject or condition is over once that call returns, before + // any arm runs: the call's value holds nothing of it. + Branching::Alternatives => { + (o_child >= 1 && q_child >= 1) + || (o_child == 0 + && q_child >= 1 + && o.trail[depth + 1..] + .iter() + .any(|(_, _, below)| *below == Branching::Call)) + } Branching::Sequence => o_child == 0 && q_child == 1, - Branching::Other => false, + Branching::Call | Branching::Other => false, }; } false @@ -592,6 +605,57 @@ mod tests { assert!(!movable.contains(&addr(&args(&update.node.updates[0].value)[0]))); } + #[test] + fn a_read_inside_a_call_in_the_subject_is_over_before_the_arms() { + use crate::ir::mir::expr::{MirMatch, MirMatchArm, MirPattern}; + // match f(s.jobs) + // _ -> T.update(s, jobs = g(s.jobs)) + let update = sp(MirExpr::RecordUpdate(Spanned::bare(MirRecordUpdate { + type_id: Some(TypeId(0)), + type_name: "State".to_string(), + base: Box::new(local(0, false)), + updates: vec![MirRecordField { + name: "jobs".to_string(), + value: call(vec![project(local(0, true), "jobs")]), + }], + }))); + let arm_read = addr( + &args(match &update.node { + MirExpr::RecordUpdate(update) => &update.node.updates[0].value, + _ => unreachable!(), + })[0], + ); + let subject = call(vec![project(local(0, false), "jobs")]); + let subject_read = addr(&args(&subject)[0]); + let body = sp(MirExpr::Match(Spanned::bare(MirMatch { + subject: Box::new(subject), + arms: vec![MirMatchArm { + pattern: MirPattern::Wildcard, + body: update, + }], + }))); + let movable = movable_projections(&body.node); + assert!(movable.contains(&arm_read)); + assert!(!movable.contains(&subject_read)); + } + + #[test] + fn a_subject_bound_whole_is_not_over_before_the_arms() { + use crate::ir::mir::expr::{MirMatch, MirMatchArm, MirPattern}; + // match s.jobs + // _ -> g(s.jobs) + let arm = call(vec![project(local(0, true), "jobs")]); + let arm_read = addr(&args(&arm)[0]); + let body = sp(MirExpr::Match(Spanned::bare(MirMatch { + subject: Box::new(project(local(0, false), "jobs")), + arms: vec![MirMatchArm { + pattern: MirPattern::Wildcard, + body: arm, + }], + }))); + assert!(!movable_projections(&body.node).contains(&arm_read)); + } + #[test] fn a_field_moves_beside_the_base_of_its_own_update() { let body = sp(MirExpr::RecordUpdate(Spanned::bare(MirRecordUpdate {