checking: fix row fundep givens and polykinded row constraints#122
checking: fix row fundep givens and polykinded row constraints#122bneiswander wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
SummaryThis PR addresses false-positive diagnostics in the PureScript analyser related to row functional dependencies and polykinded row constraints. The changes are scoped to the checking module's core constraint-solving infrastructure and integration into equation checking, with no direct LSP behavior modifications. Key ChangesNew Function:
Constraint Matching Refactoring (
Row Kind Inference (
Equation Checking Integration (
Test Fixtures (3 new)
Snapshot Update
Code QualityThe implementation demonstrates solid engineering practices:
No new TODO or FIXME markers were introduced. Dependencies & ScopeThe PR is part of a sequence (follow-up branches: Merge Confidence: 4/5Factors increasing confidence:
Factors moderating confidence:
WalkthroughThis PR introduces functional dependency elaboration to the type checker, adding ChangesFunctional Dependency Elaboration and Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
compiler-core/checking/src/core/constraint/elaborate.rs (1)
76-78: 💤 Low valueConsider adding a comment explaining why
solve_constraintsis called with empty given constraints.The probe constraint at line 77 is solved without passing the original given constraints. This appears intentional—to derive fundep improvements only from instance chains, avoiding circular reasoning where a given constraint might improve itself. Adding a brief comment (e.g., "only instance-based improvement, not given-based, to prevent circular fundep derivations") would clarify this design choice for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@compiler-core/checking/src/core/constraint/elaborate.rs` around lines 76 - 78, Add a clarifying comment above the call to super::solve_constraints(state, context, wanted, &[]) explaining that we intentionally pass an empty slice for the "given" constraints so that probe constraint solving only uses instance-based improvements (e.g., fundep derivations from instances) and does not include the original given constraints, preventing circular improvement where a given might improve itself; reference the call to solve_constraints and the involved variables state, context, and wanted in the comment so future maintainers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@compiler-core/checking/src/core/constraint/elaborate.rs`:
- Around line 76-78: Add a clarifying comment above the call to
super::solve_constraints(state, context, wanted, &[]) explaining that we
intentionally pass an empty slice for the "given" constraints so that probe
constraint solving only uses instance-based improvements (e.g., fundep
derivations from instances) and does not include the original given constraints,
preventing circular improvement where a given might improve itself; reference
the call to solve_constraints and the involved variables state, context, and
wanted in the comment so future maintainers understand the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e63ccd19-426b-4349-af67-16b9c846fa03
⛔ Files ignored due to path filters (4)
tests-integration/fixtures/checking/1772440440_prim_row_open/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1778682360_row_fundep_determined_result/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1778685420_polykinded_row_fundep_safe_union/Main.snapis excluded by!**/*.snaptests-integration/fixtures/checking/1778701560_eval_given_visible_type_application/Main.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
compiler-core/checking/src/core/constraint.rscompiler-core/checking/src/core/constraint/compiler/prim_row.rscompiler-core/checking/src/core/constraint/elaborate.rscompiler-core/checking/src/core/constraint/matching.rscompiler-core/checking/src/source/terms/equations.rstests-integration/fixtures/checking/1778682360_row_fundep_determined_result/Main.purstests-integration/fixtures/checking/1778685420_polykinded_row_fundep_safe_union/Main.purstests-integration/fixtures/checking/1778701560_eval_given_visible_type_application/Main.purs
Fixes CannotUnify diagnostics when using polykinded row type classes like SafeUnion (forall k. Row k -> Row k -> Row k -> Constraint) with higher-kinded effect rows (Row (Type -> Type)). Two separate issues were addressed: 1. Functional-dependency positions in canonical constraint elaboration (elaborate_given_substitution) were indexing into canonical argument lists without accounting for implicit kind binders. The canonical representation interleaves kind arguments (from forall k.) before type arguments, so position 0 in a class's type parameters actually maps to canonical argument index 1 when a kind binder is present. Fixed in both elaborate.rs (speculative solving) and matching.rs (can_determine_stuck). 2. The Prim.Row.Union compiler solver created residual open-row tails (the | r part of (a :: A | r)) using a hardcoded Row Type kind. For polykinded classes, this caused the kind to be stuck as Row Type even when the constraint involved Row (Type -> Type). Fixed by inferring the correct row element kind from the other arguments and using that to construct the fresh tail variable. Also adds a regression fixture that reproduces the SafeUnion pattern using Eval/RowList/FromRow/ToRow (mimicking Type.Eval.Row.Util.SafeUnion) and verifies it type-checks correctly.
When functional-dependency elaboration produces a substitution for a forall-bound rigid variable, the improved givens now extend the original givens rather than replacing them. This allows visible type applications against those variables in the function body (e.g. Proxy @x) to still resolve against the explicit constraint while the improved forms are available for argument and result substitution. Regression test: eval_given_visible_type_application
b8aa234 to
eb0eef8
Compare
checking: fix row fundep givens and polykinded row constraints
Base branch:
mainReview Order
This PR should be reviewed first. It introduces checker infrastructure used by the stacked follow-up PRs for instance-chain backtracking and operator expected-result checking.
Follow-up PRs stacked on this branch:
pr/checking-instance-chain-backtrackingpr/checking-operator-expected-resultsSummary
Motivation
These fixes came from comparing analyzer diagnostics against the PureScript compiler on real project code. The analyzer was reporting errors for row-level constraints that PureScript accepts, especially around function dependencies, row unions, and visible type applications carried by givens.
The common theme is that improving constraints must not lose the information needed to elaborate or solve row givens later. In particular, a constraint that becomes determined through a functional dependency should be elaborated with that determined result before the checker decides it is ambiguous.
Motivating Example
The checker should accept code where the result row is determined by the
Unionfunctional dependency, even when the result is not written explicitly by the caller:Before this change, similar real-world constraints could remain ambiguous after improvement even though PureScript could determine the result row.
Implementation Notes
Tests
Added fixtures:
1778682360_row_fundep_determined_result1778685420_polykinded_row_fundep_safe_union1778701560_eval_given_visible_type_applicationUpdated existing snapshot:
1772440440_prim_row_openValidation run locally: