fix for https://github.com/navicore/patch-seq/issues/65

[navicore]

⏺ All 20 tests pass including our new test.

⏺ Summary of the fix for Issue #65:

1. Root cause: The unify_types function in unification.rs didn't have a case for Type::Union. When unifying two union types (even identical ones like Union("Sexpr") with Union("Sexpr")), they fell through to the default _ catch-all case which always returns an error.
2. The bug was NOT related to includes - we reproduced it with a single-file test case. As you suspected, includes just flatten everything into a single code stream.
3. The fix: Added a simple pattern match case for union types: (Type::Union(name1), Type::Union(name2)) => { if name1 == name2 { Ok(Subst::empty()) } else { Err(format!("Type mismatch: cannot unify Union({}) with Union({})", name1, name2)) } }
4. Test added: tests/integration/src/test-union-same-type-fields.seq which creates a variant with two fields of the same union type.

[claude]

PR Review: Fix for Union Type Unification (Issue #65)

Summary

This PR fixes a critical bug where union types couldn't be unified with themselves, causing compilation failures when variants had multiple fields of the same union type. The fix is clean, correct, and well-tested.

Code Quality ✅

Strengths:

• Minimal, targeted fix: Adds exactly the missing case without over-engineering
• Follows existing patterns: Matches the structure of other concrete type unification cases (Int, Bool, String, Float)
• Clear code placement: Logically positioned after primitive types and before type variables
• Good error messages: The error case provides helpful diagnostics with both union names
• Consistent style: Formatting and commenting align with the existing codebase

The implementation at crates/compiler/src/unification.rs:142-152 is exactly right for nominal typing semantics where union types are identified by name.

Test Coverage ✅

Integration test is excellent:

• Self-contained: Single-file test that doesn't rely on includes (validates the fix addresses the root cause)
• Comprehensive: Tests the exact scenario from the bug report (two fields of same union type)
• Clear documentation: Comments explain the purpose and context
• Expected output: Proper expected result file included

Suggestion for improvement (minor):
Consider adding a unit test in the tests module of unification.rs to directly test Union type unification for faster feedback during development.

Potential Issues 🔍

No bugs identified, but here are architectural considerations:

1. Occurs check consistency (already correct): The occurs_in_type function at line 106 already handles Type::Union correctly, which is consistent with the new unification code.

2. Structural vs Nominal typing: This implementation uses nominal typing (unions are equal if names match). This is the right choice for most type systems.

Performance Considerations ✅

• Optimal: String comparison is O(n) in string length, which is unavoidable for nominal typing
• No allocations on success: Returns Subst::empty() without creating new data structures
• Early exit: Checks equality immediately before creating error messages

Security Concerns ✅

No security issues identified. The implementation uses safe Rust patterns and properly handles all cases.

Documentation 📝

The code is well-commented and the PR description provides excellent context about root cause analysis, why includes weren't the issue, the fix approach, and test validation.

Architecture Consistency ✅

The fix integrates perfectly with the existing Hindley-Milner unification algorithm and follows the same patterns as other concrete types.

Recommendation: APPROVE ✓

This is a high-quality fix that:

1. ✅ Solves the reported issue correctly
2. ✅ Includes comprehensive testing
3. ✅ Follows existing code patterns
4. ✅ Has no performance or security concerns
5. ✅ Is well-documented

Optional enhancement: Add unit tests for Union type unification to strengthen the test suite, but this is not a blocker for merging.

Great work isolating this bug and providing a clean, minimal fix! 🎉