Add Role Reversal (share a permanent type) - #4839
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing 'permanent type' and 'permanent types' in parse_shared_quality, mapping them to SharedQuality::CardType in accordance with CR 110.4 and CR 205.2a. It also adds corresponding unit tests to verify the parsing and mapping behavior. There are no review comments, and I have no feedback to provide.
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.
matthewevans
left a comment
There was a problem hiding this comment.
This needs a narrower model before it can merge. Mapping "permanent type" to SharedQuality::CardType is over-broad: CR 110.4 defines only artifact, battle, creature, enchantment, land, and planeswalker as permanent types, while CR 205.2a includes other card types such as kindred. Two permanents that share only kindred would incorrectly satisfy Role Reversal's "share a permanent type" constraint under this mapping.
Please model this as a permanent-type-only shared quality (or otherwise restrict the compared card types to the CR 110.4 set) and add a regression that would fail for two permanents sharing only kindred but no permanent type.
`parse_shared_quality` recognized "card type" / "creature type" / "land
type" but not "permanent type", so the two-target exchange-control clause
in Role Reversal ("Exchange control of two target permanents that share a
permanent type.") fell through to Effect::Unimplemented, and Cloudstone
Curio silently dropped its "that shares a permanent type with it" return
constraint.
CR 110.4 defines the six permanent types (artifact, battle, creature,
enchantment, land, planeswalker); CR 205.2a lists the card types. The
permanent types are a subset of the card types, so for permanents "share
a permanent type" is equivalent to "share a card type" and maps to the
same SharedQuality::CardType. Add the singular/plural "permanent type[s]"
tags next to the "card type[s]" arms.
Pure parse-only vocabulary variant: no new Effect, filter, or resolver
behavior. Adds unit tests for the recognizer and an end-to-end Role
Reversal test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…shared quality Addresses review feedback on phase-rs#4839: mapping "permanent type" to SharedQuality::CardType was over-broad. Per CR 110.4 the permanent types are only artifact, battle, creature, enchantment, land, and planeswalker, whereas CR 205.2a card types also include non-permanent types such as kindred. Two permanents sharing only kindred would have wrongly satisfied Role Reversal's "share a permanent type". Add a narrower SharedQuality::PermanentType variant whose value extractor filters an object's core types by CoreType::is_permanent_type() (which already excludes kindred/tribal/dungeon/instant/sorcery), wired through the filter evaluator, the search-distinct resolver, and the coverage label. Map the "permanent type[s]" parser arms to it. Adds a regression (shared_permanent_type_excludes_kindred): two permanents sharing only kindred are disjoint under PermanentType, while they would overlap under the old CardType mapping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4aed8f3 to
3d6bfdd
Compare
|
Thanks — good catch, addressed in the latest push. Introduced a narrower Added a regression Green: rustfmt, parser-combinator gate, |
Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: current head addresses the permanent-type modeling blocker; parse-diff is current, the implementation uses a narrowed SharedQuality::PermanentType seam, and the Kindred regression test discriminates the old over-broad CardType behavior.
Summary
Role Reversal (WAR) — "Exchange control of two target permanents that share a permanent type." — parsed to
Effect::Unimplemented. The shared-quality recognizerparse_shared_quality()already handled "share a card type" (Shifting Loyalties, Burglar's Plot) but not "share a permanent type", so Role Reversal's exchange-control clause fell through.Change
Per CR 110.4, the six permanent types (artifact, battle, creature, enchantment, land, planeswalker) are a subset of the card types (CR 205.2a), so for permanents "share a permanent type" is equivalent to "share a card type". Add the singular/plural
permanent type[s]tags toparse_shared_quality(), mapping to the sameSharedQuality::CardType. Role Reversal then lowers to the existingEffect::ExchangeControlpath — no new effect, filter, or resolver behavior.Bonus fix: Cloudstone Curio shares this phrasing ("...another permanent you control that shares a permanent type with it...") and was silently dropping its shared-type constraint; it now captures it as
SharesQuality{CardType}.The two
"permanent type"forms are the only shared-quality uses of the phrase corpus-wide (verified by grep); the other"permanent type"occurrences (choose / count / for-each) route through unrelated parsers and never reachparse_shared_quality.Tests
parse_shared_quality_permanent_type_maps_to_card_type— recognizer unit test (singular + plural).that_shares_permanent_type_with_it_consumed— Cloudstone Curio return clause.effect_exchange_control_two_permanents_share_permanent_type— Role Reversal end-to-end →ExchangeControlwith both slots carryingSharesQuality{CardType}.Verified: rustfmt clean, parser-combinator (Rule Zero) gate pass,
cargo clippy -D warningsclean, full engine suite 14493 passed; 0 failed. Fails-before (Role Reversal →Unimplemented) / passes-after confirmed.