check: derive perf-shared-update from the field moves generated code makes - #1440
Merged
Merged
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Owner
Author
|
Pushed 7f387ea: the run guide example ( |
…-update-from-field-moves # Conflicts: # CHANGELOG.md # tests/rust_work_spec.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Porting btc-listener found
perf-shared-update(#1432) disagreeing with generated Rust in both directions.Root cause
The check decided "the record still holds the field" with its own AST walk that mirrored the VM's field takes (
ir::field_take), not the facts the Rust backend moves fields by (ir::mir::field_moves):done = toppedUp(s.pool, s.book, s.nextKey)?thenS.update(s, pool = done.pool, book = done.book, nextKey = done.nextKey): the check sawsread after the call and warned; Rust moves the Book out (nothing else reads it, the update replaces it).match (s.pool, s.book)/(pool, book) -> f(s, g(book)): the update's argument is a bare local, which the check never looked at; Rust cloness.bookinto the match (becausesis used whole afterwards), sogcopies the Map. btcinfra/chainstate.avabsorbedFromandinfra/follow.avtoppedUpat a835677 have this shape.Fix
lower_program,optimize) and usesfield_moves::movable_projectionsper function. A value handed to an update is reported when it is a field read that does not move, or a local bound to a field read by aletor amatch(tuple, constructor and list patterns included) and handed on at its last use. A param that every self tail call hands on unchanged never gives up a field (Rust's loop-carried params). Which callees update in place is still read from the source summaries and followed into dependencies. The MIR is built only when some repeated function has an update site at all, so a check without one does no extra work; on btc (debug build)aver checktakes 100 s against 98 s before.types::checker::program_symbolsexposes the symbol table the typechecker builds (from the loaded modules or the module root), so the lowering resolves dependency calls.field_movesnow understands the base of an update of a field chain,T.update(s.window, created = ...): when nothing outside the update reads that part ofsagain, the base moves what the update keeps (a fixpoint, since an inner base may depend on an outer one), and the replaced field may move out before it. Without this the check would have had to warn on the nested update that the VM already does in place, because Rust clonedsetting.windowandsetting.window.created. Rust now emitsWindow { created: setting.window.created.insert_owned(..), ..setting.window }.match, which is the false-negative shape; it says to read the field where nothing reads that part of the record again, for example in the update that replaces it.docs/diagnostics-slugs.mdis updated.Mutual tail-call arms only get field moves with #1438, so the let form inside a mutual group (the btc
tendedshape) moves in Rust once #1438 is in; this check already treats it as moving.Precision
examples/: 0 warnings (life.av's one warning is gone:editorLoop(st.grid, …)reads the grid in aletthat the update replacing it follows, sofield_movesmoves it; the copy that remains there comes fromeditorLoopbeing a mutual tail-call member whose wrapper borrows and clones, which the check does not see).self_hosted/: 0.pool.*warnings stay; thewithStandingones (one.standingandpopped.standinginorPump,greetingRead,greetingPopped,poppedFor) go, because those fields do move; the two false negatives above are now reported.Tests
shared_update_spec: the oldparts_moved_out_of_the_record_do_not_warnencoded the false negative and is nowa_part_bound_by_a_match_while_the_record_is_passed_on_warns; newa_field_read_before_the_update_that_replaces_it_does_not_warn(the btc false positive, in let and match form). The nested-update test keeps expecting no warning, now true for Rust too.field_movesunit tests: a chain base that moves, and one read again afterwards that does not.rust_work_spec::a_nested_update_moves_the_rest_of_the_inner_record(fixturerust_nested_update_moves): checks the emitted moves and clones, builds, compares with the VM.examples/,tests/fixtures,self_hosted/with amaincompiled to Rust andcargo checked: 151 pass; the same 6 as on main fail for unrelated reasons (E0277/E0308). btc HEAD passescargo check. Self-host regenerated with no change.rust_work_spec,own_param_*,rust_codegen_regression,compile_spec,verify_handle_params_spec, fmt, clippy.