fix(engine): skip impossible targeted return after sacrificed token ceases to exist#4628
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #4571 by ensuring that zone moves for objects that no longer exist (such as tokens removed by state-based actions) resolve as a no-op instead of causing a panic. A regression test simulating 'It That Betrays' and 'All Is Dust' with a token sacrifice has been added to verify this behavior. The feedback suggests optimizing the zone move check by retrieving the object once using state.objects.get(&obj_id) to avoid redundant map lookups in a hot path, which also simplifies the subsequent emblem check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // CR 111.7 + CR 400.7: a targeted return effect may still carry the | ||
| // snapshotted ObjectId of a token (or any other object that no longer | ||
| // exists) after state-based actions removed it from the game. Once the | ||
| // object is absent from `state.objects`, the zone move is impossible and | ||
| // must resolve as a no-op rather than calling `move_to_zone` on a missing | ||
| // object (issue #4571). | ||
| if !state.objects.contains_key(&obj_id) { | ||
| return ZoneMoveResult::Done; | ||
| } | ||
|
|
||
| // CR 114.5: Emblems cannot be moved between zones. | ||
| if state.objects.get(&obj_id).is_some_and(|o| o.is_emblem) { | ||
| return ZoneMoveResult::Done; |
There was a problem hiding this comment.
[MEDIUM] Redundant map lookups on state.objects.
Why it matters: Performing contains_key followed by get results in multiple redundant lookups on state.objects in a hot path.
Suggested fix: Retrieve the object once using state.objects.get(&obj_id) and bind it, which also simplifies the subsequent emblem check.
| // CR 111.7 + CR 400.7: a targeted return effect may still carry the | |
| // snapshotted ObjectId of a token (or any other object that no longer | |
| // exists) after state-based actions removed it from the game. Once the | |
| // object is absent from `state.objects`, the zone move is impossible and | |
| // must resolve as a no-op rather than calling `move_to_zone` on a missing | |
| // object (issue #4571). | |
| if !state.objects.contains_key(&obj_id) { | |
| return ZoneMoveResult::Done; | |
| } | |
| // CR 114.5: Emblems cannot be moved between zones. | |
| if state.objects.get(&obj_id).is_some_and(|o| o.is_emblem) { | |
| return ZoneMoveResult::Done; | |
| // CR 111.7 + CR 400.7: a targeted return effect may still carry the | |
| // snapshotted ObjectId of a token (or any other object that no longer | |
| // exists) after state-based actions removed it from the game. Once the | |
| // object is absent from state.objects, the zone move is impossible and | |
| // must resolve as a no-op rather than calling move_to_zone on a missing | |
| // object (issue #4571). | |
| let Some(obj) = state.objects.get(&obj_id) else { | |
| return ZoneMoveResult::Done; | |
| }; | |
| // CR 114.5: Emblems cannot be moved between zones. | |
| if obj.is_emblem { | |
| return ZoneMoveResult::Done; | |
| } |
References
- Idiomatic Rust uses the type system, ownership model, and standard library idioms to their fullest, including avoiding redundant map lookups by leveraging Option-returning methods like get. (link)
|
Addressed the redundant |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The PR's own regression test fails, so the claimed production behavior is not fixed. Evidence: crates/engine/tests/issue_4571_it_that_betrays_all_is_dust.rs:87 drives the real cast pipeline, then crates/engine/tests/issue_4571_it_that_betrays_all_is_dust.rs:89 asserts the sacrificed nontoken permanent returned; GitHub Actions shard 1 fails at that assertion on this head. Why it matters: the crash guard may avoid one missing-object panic path, but the It That Betrays + All Is Dust scenario still does not satisfy the rule behavior the PR claims to cover. Suggested fix: fix the trigger/continuation path so the nontoken permanent returns under P0 while the sacrificed token remains gone, then keep this regression test green.
# Conflicts: # crates/engine/src/game/effects/change_zone.rs
Parse changes introduced by this PR · 32 card(s), 16 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The PR's own regression still fails at the exact head. Evidence: crates/engine/tests/issue_4571_it_that_betrays_all_is_dust.rs:87 drives the cast pipeline and :89 asserts the nontoken permanent returned; GitHub check runs for f31b3938 show Rust tests (shard 1/2) failing. Why it matters: the missing-object no-op may avoid one panic, but the claimed It That Betrays / All Is Dust behavior is still not fixed. Suggested fix: fix the trigger/continuation path so the nontoken permanent returns under P0 while the sacrificed token remains gone, then keep this regression green.
Reviewed current head f31b3938060c5c89d841d99be6f7db880d1434e7.
2 participants
Summary
ChangeZonemoves when the referenced object no longer existsIt That Betrays+All Is Dustwhen the sacrificed set includes a tokenobject existspanic path from issue#4571Change Type
Behavior Proof
process_one_zone_movenow returnsDoneimmediately if the snapshottedObjectIdis no longer present instate.objectsCR 400.7/CR 111.7behavior: once the object is gone, the zone move is impossible and should no-op rather than panicIt That Betraysreanimation shape with a sacrificed colored token and asserts only the nontoken permanent returnsValidation
cargo fmt --allcargo test -p engine all_is_dust_with_token_sacrifice_does_not_crash_it_that_betrays_return -- --nocaptureCloses #4571.