Allow cyclic unions to discriminate on nested paths

[shaungrady]

Fix cyclic union discrimination

Fixes #1547

Problem

Discriminated unions containing circular references were completely excluded from discrimination, causing validation to check all branches and produce confusing error messages referencing irrelevant branches.

Solution

Allow cyclic unions to use property-based discrimination (e.g., a type discriminator field) while blocking structural discrimination that would attempt to describe the entire recursive structure.

The fix filters discrimination candidates to only include those with path.length > 0, ensuring cyclic unions discriminate on specific properties rather than overall object structure.

Changes

• Remove early bailout for cyclic unions in discriminate()
• Filter candidates to exclude structural discrimination (path.length === 0) for cyclic unions
• Add test validating discrimination works and error messages only reference the relevant branch

Example

const s = scope({
  AChild: { type: "'AChild'", children: "(AParent)[] > 0" },
  AParent: { type: "'AParent'", children: "(AChild)[] > 0" },
  BChild: { type: "'BChild'", children: "unknown[]" },
  BParent: { type: "'BParent'", layout: "number[]", children: "(BChild)[] > 0" }
})

const Thing = s.type("AParent | BParent")

Thing({ type: "BParent", layout: "", children: [...] })
// Before: "children[1].type must be 'AChild' (was 'BChild') or layout must be an array (was string)"
// After:  "layout must be an array (was string)"

• Code is up-to-date with the main branch
• You've successfully run pnpm prChecks locally
• There are new or updated unit tests validating the change

[shaungrady]

Any feedback on this @ssalbdivad? (Happy holidays, no pressure! 🦃)

[shaungrady]

@ssalbdivad I'm eager to get this in. Any notes I can take action on to get this PR ready?

[ssalbdivad]

Hey @shaungrady very sorry for the delay getting back and thanks for working on this!

I'm going to try and take a look this weekend. Honestly I've been avoiding this a bit because of the complexity of some of the issues to do with recursive discriminated unions in the past which is why I disabled them for cyclic types.

I'll try to give this another once over myself, but could you take a look at closed issues that mention bugs with recursive discriminated unions and make sure they are all reflected in our unit tests and still function correctly with this change?

If so, I'm very grateful for your work in such a complex area of the code base and am excited to do a final review and ship it

[shaungrady]

No sweat, I'll search through closed issues to see what I can see.

[ssalbdivad]

Great, thanks so much! I'm guessing many of those cases are already reflected in the tests so no need to duplicate if you can search the code base and find them, but any that aren't would be a great addition.

[shaungrady]

Full disclosure, I used Claude Opus 4.5 to help do quick analysis of these issues as it relates to this change. Test coverage is all there for the found issues, and everything still passes. The change is relatively straightforward but I can appreciate the hesitancy and care you want to devote to it.

I rebased/pushed. Let me know if there's anything else you need after you get a chance to review it more closely.

Analysis

Relevant

Issue	Description	Conclusion
#1033	Boolean + recursive discriminated union	Safe; discriminates on ["type"], which this change allows
#1100	null in union with object	Safe; not cyclic, filter does not apply
#1164	Pruned discriminant keys + onUndeclaredKey	Safe; not cyclic
#1263	Overlapping morphs silently dropping fields	Safe; validation happens in reduceBranches, not discriminate
#1304	Cyclic XML element types	Potentially improved; this change enables nested ["tagName"] discrimination that was previously blocked
#1440	Bounded array vs tuple discrimination	Safe; not cyclic
#1441	Array + tuple union rejects empty array	Safe; not cyclic

Unrelated

Issue	Description	Why Unrelated
#1209	Cyclic record/literal/array union	Array vs object domain discrimination issue, not cyclic candidate filtering
#1476	OOM with mutually-recursive Truthy/Falsy	Memory issue during scope construction, not discrimination
#1487	Cyclic type inference display	TypeScript type-level display issue, not runtime discrimination

[ssalbdivad]

So it looks like for whatever reason unrelated to the change disallowing shallow discrimination for cyclic types, something I've done since #1425 when cyclic discriminated unions were broken actually fixed them.

I'm still not totally sure what, but all the issues cited there as a reason to disable them (#1284, #1209, #1367, #1188) have now been added to tests and are working. I've adjusted this PR to add tests for these which are now passing, and removed the constraint around shallow discrimination which seems to not be needed. There are some cases where undiscriminated unions actually give a clearer error message, but that should be handled more generally as part of some of the other issues in the backlog.

Thanks for taking a look at this and I'll be merging this PR as soon as CI passes ❤️

[shaungrady]

Appreciate your help with this @ssalbdivad!

[shaungrady]

Would you be willing to cut a release for this soon?