fix(parser): "target <filter>'s controller/owner <verb>s it" declares its object target#4701
fix(parser): "target <filter>'s controller/owner <verb>s it" declares its object target#4701minion1227 wants to merge 1 commit into
Conversation
… its object target
"{T}: Target artifact creature's controller sacrifices it. …" (Arcum Dagsson,
phase-rs#4678) compiled to `Effect::Sacrifice { target: ParentTarget }` with NO
selectable object-target slot: `parse_subject_application`'s "target " branch
parsed the `Typed([Artifact,Creature])` filter but discarded the "'s controller"
remainder, and the imperative "sacrifices it" lowered to a `ParentTarget` anaphor
disconnected from the subject — dangling for a top-level ability. `build_target_
slots` produced zero slots, so the ability activated without targeting or
sacrificing anything. The whole "target <filter>'s controller/owner <verb>s it"
class was broken (Mercy Killing, Basalt Golem's spell-form siblings, etc.).
Two scoped changes:
- `parse_subject_application` (oracle_effect/subject.rs): when the "target "
subject's remainder is exactly "'s controller"/"'s owner" (nom
`all_consuming(alt(...))`), record the possessive shift as
`affected = ParentTargetController/Owner` while keeping `<filter>` in
`target`. The anaphoric "its controller"/"that creature's controller" subjects
leave `target = None`, so the (affected, target) pair uniquely identifies the
new shift.
- `lower_subject_predicate_ast` imperative arm (oracle_effect/mod.rs): for that
shift, wrap the imperative effect in `Effect::TargetOnly { target: <filter> }`
(the established pattern already used for player-target ChangeZone / Explore).
This declares the object target so `build_target_slots` requires it and the
effect's `ParentTarget` ("it") plus a following `ParentTargetController`
("that player may …") resolve against it. No new engine variant.
CR 608.2c (anaphora) + CR 109.4 (controller/owner) + CR 115.1 (targets).
Runtime test (build_target_slots): Arcum now surfaces exactly one required
target slot; the opponent's artifact creature is legal, a plain creature is not.
fmt + clippy + parser combinator gate clean; 2308 oracle_effect + 125
ability_utils + 78 targeting + 697 casting tests green.
Closes phase-rs#4678
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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 introduces support for parsing and resolving abilities where the target's controller or owner is the active subject of an effect (e.g., "Target 's controller/owner s it"), such as Arcum Dagsson. It updates the subject parser using nom combinators to recognize possessive suffixes and map them to the appropriate target filters, and adds a unit test to verify correct target slot behavior. I have no feedback to provide as the implementation is idiomatic and conforms to the repository's style guide.
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.
Parse changes introduced by this PR · 7 card(s), 11 signature(s) (baseline: main
|
1 participant
Closes #4678.
Bug: "{T}: Target artifact creature's controller sacrifices it. …" (Arcum Dagsson) compiled to
Effect::Sacrifice { target: ParentTarget }with no selectable object-target slot —build_target_slotsproduced zero slots, so the ability activated without targeting or sacrificing anything. Root cause:parse_subject_application's "target " branch parsed theTyped([Artifact,Creature])filter but discarded the "'s controller" remainder, and the imperative "sacrifices it" lowered to a danglingParentTargetanaphor. The whole "target 's controller/owner s it" class was broken (Mercy Killing, etc.).Fix (two scoped changes, no new variant):
parse_subject_application(subject.rs): when the "target " subject's remainder is exactly"'s controller"/"'s owner"(nomall_consuming(alt(...))), record the possessive shift asaffected = ParentTargetController/Ownerwhile keeping<filter>intarget. Anaphoric "its controller"/"that creature's controller" subjects leavetarget = None, so the(affected, target)pair uniquely identifies the new shift.lower_subject_predicate_astimperative arm (mod.rs): for that shift, wrap the imperative effect inEffect::TargetOnly { target: <filter> }— the established pattern already used for player-target ChangeZone / Explore — sobuild_target_slotsrequires the object target and the effect'sParentTarget("it") plus a followingParentTargetController("that player may …") resolve against it.CR 608.2c + CR 109.4 + CR 115.1.
Runtime test (
build_target_slots): Arcum now surfaces exactly one required target slot; the opponent's artifact creature is legal, a plain creature is not.cargo fmt+clippy+ the parser combinator gate are clean; 2308 oracle_effect + 125 ability_utils + 78 targeting + 697 casting tests green.🤖 Generated with Claude Code