checking: allow recursive Reflectable user instances

[bneiswander]

checking: allow recursive Reflectable user instances

Base branch: main

Review Order

This PR is independent and can be reviewed against main at any time. It does not depend on the row-fundep/givens PR or the stacked instance/operator PRs.

Summary

• Allow user-defined recursive Reflectable instances that build type-level lists through ordinary instance resolution.
• Keep the primitive Reflectable implementation from rejecting valid recursive list evidence too early.
• Add an integration fixture covering recursive user instances for reflected type-level structures.

Motivation

This fixes a false positive found while checking real project code that encodes values through recursive type-level lists. PureScript accepts the recursive user instances, but the analyzer rejected them during primitive Reflectable solving.

The primitive solver should cooperate with user-defined recursive evidence rather than treating the recursive structure as unsolvable. This lets ordinary instance resolution build the reflected value one list cell at a time.

Motivating Example

The checker should allow Reflectable evidence to be assembled recursively through user instances:

data Nil
data Cons head tail

class ReflectList xs where
  reflectList :: Proxy xs -> Array String

instance ReflectList Nil where
  reflectList _ = []

instance (Reflectable head String, ReflectList tail) => ReflectList (Cons head tail) where
  reflectList _ = reflectType (Proxy :: Proxy head) : reflectList (Proxy :: Proxy tail)

Before this change, recursive evidence like the Cons case could be rejected too early even though PureScript can solve it.

Implementation Notes

• Narrows the primitive Reflectable rejection path so recursive user instance chains can produce the required reflected value.
• Leaves the change isolated to prim_reflectable; it does not alter row solving or instance-chain backtracking.
• Adds a focused fixture instead of broad snapshot churn.

Tests

Added fixture:

• 1778704560_reflectable_recursive_user_instances

Validation run locally:

cargo check -p checking --tests

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c1936cf-5cde-4d3e-81cf-669450c9e924

📥 Commits

Reviewing files that changed from the base of the PR and between 9dea7c2 and d70787e.

⛔ Files ignored due to path filters (1)

• tests-integration/fixtures/checking/1778704620_reflectable_symbol_exact_instance/Main.snap is excluded by !**/*.snap

📒 Files selected for processing (1)

• compiler-core/checking/src/core/constraint.rs

---

Summary

This PR adjusts the primitive Reflectable handling so user-defined recursive Reflectable instances that build type-level lists via ordinary instance resolution are accepted. The change narrows the primitive Reflectable rejection path: when the first Reflectable solver argument is not a primitive literal or a unification variable, the primitive solver now yields None (no compiler solver applies) instead of Apart (permanent rejection). That allows the normal declared-instance resolution to attempt user-defined Reflectable instances and assemble recursive list evidence one cell at a time. The change is isolated to prim_reflectable and does not alter row solving or instance-chain backtracking.

Technical details

• Behavioural change: prim_reflectable’s terminal path (match_reflectable) returns Ok(None) for non-primitive/non-variable first arguments, permitting compiler::match_compiler_instance to fall through to instances::collect_instance_chains and matching::match_declared.
• Related solver reordering: solve_constraints was adjusted to prefer declared-instance candidate-chain search or compiler-instance matching depending on context (based on whether the constraint’s canonical (file_id, type_id) equals context.known_reflectable.reflectable). A helper handle_compiler_match centralises the compiler-instance handling and blocking bookkeeping.
• Scope: change is local to compiler-core/checking constraint solving (prim_reflectable and related solver flow). No changes to unification, row solving, or instance backtracking logic.

Tests and fixtures

• A focused integration fixture was added: 1778704560_reflectable_recursive_user_instances (to exercise recursive user instances).
• Existing primitive fixture 1772442600_prim_reflectable remains relevant for literal cases.
• Local validation performed with: cargo check -p checking --tests.

Code review observations

Strengths

• Small, surgical change that aligns the analyser with PureScript’s instance resolution for recursive user instances.
• Preserves existing primitive Reflectable handling for literals and variables.
• Change avoids permanently discarding user-defined candidates whose subgoals are also user-defined Reflectable constraints.

Risks / considerations

• Constraint-solver control flow is critical; ensure declared-instance search is reliably invoked when prim_reflectable returns None.
• Error diagnostics for failing recursive instance chains rely on correct propagation through the declared-instance search; verify messages remain clear.
• Potential performance implications from deferring non-primitive Reflectable resolution to declared-instance search should be checked on representative codebases.
• The solver reordering (compiler vs declared-instance attempts) must be validated across a suite of real-world constraints to rule out regressions.

Merge confidence: 4/5

Rationale:

• Confidence is raised by the focused, minimal scope of the change, the addition of a targeted fixture, and local cargo check validation.
• Reduced from maximum because this touches a central solver path; full test-suite/integration runs and verification that diagnostics and performance remain acceptable would increase confidence to 5/5.

Walkthrough

The solver reorders compiler-instance matching based on a per-constraint prefer_declared_instances decision and adds a shared handle_compiler_match helper that updates the work list or records blocked/skolem states.

Changes

Constraint Solver Instance-Matching Order

Layer / File(s)	Summary
Helper: handle_compiler_match compiler-core/checking/src/core/constraint.rs	Adds a helper that interprets compiler::match_compiler_instance results, extends WorkList on Match, or records blocked/blocked_on_skolem on Stuck/Skolem.
Early compiler-instance attempt when not preferring declared compiler-core/checking/src/core/constraint.rs	When the constraint does not match context.known_reflectable.reflectable, performs an early compiler-instance match via the helper and restarts the main loop on success.
Deferred compiler-instance attempt when preferring declared compiler-core/checking/src/core/constraint.rs	When the constraint matches the known reflectable, runs declared-instance candidate-chain search first, then invokes the helper for a deferred compiler-instance match and continues the loop on success.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The pull request description clearly articulates the changes: allowing recursive Reflectable user instances by narrowing the primitive Reflectable rejection path, with specific motivation, implementation notes, and test additions.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[purefunctor]

Nice! Verified that this checks in the original compiler.

Could you split this into two commits? The first one to add the failing test case and have the second commit be the fix?

[coderabbitai]

🧹 Nitpick comments (1)

compiler-core/checking/src/core/constraint.rs (1)

246-260: 💤 Low value

Consider extracting duplicated compiler-instance matching logic.

Lines 247–259 duplicate the match handling from lines 210–222. A helper closure or local function could reduce repetition and ensure both paths stay in sync during future maintenance.

♻️ Possible refactor using a closure

+        let try_compiler_instance =
+            |work: &mut WorkList,
+             blocked: &mut FxHashSet<u32>,
+             blocked_on_skolem: &mut bool|
+             -> QueryResult<bool> {
+                match compiler::match_compiler_instance(state, context, wanted, &given)? {
+                    Some(matching::MatchInstance::Match { unifications, constraints }) => {
+                        work.extend_from_parts(unifications, constraints);
+                        Ok(true)
+                    }
+                    Some(matching::MatchInstance::Stuck { stuck }) => {
+                        blocked.extend(stuck);
+                        Ok(false)
+                    }
+                    Some(matching::MatchInstance::Skolem) => {
+                        *blocked_on_skolem = true;
+                        Ok(false)
+                    }
+                    Some(matching::MatchInstance::Apart) | None => Ok(false),
+                }
+            };
+
         if !prefer_declared_instances {
-            match compiler::match_compiler_instance(state, context, wanted, &given)? {
-                Some(matching::MatchInstance::Match { unifications, constraints }) => {
-                    work.extend_from_parts(unifications, constraints);
-                    continue 'work;
-                }
-                Some(matching::MatchInstance::Stuck { stuck }) => {
-                    blocked.extend(stuck);
-                }
-                Some(matching::MatchInstance::Skolem) => {
-                    blocked_on_skolem = true;
-                }
-                Some(matching::MatchInstance::Apart) | None => (),
+            if try_compiler_instance(&mut work, &mut blocked, &mut blocked_on_skolem)? {
+                continue 'work;
             }
         }

Apply analogous change to the deferred block at lines 246–260.

🤖 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.rs` around lines 246 - 260, The
match handling of compiler::match_compiler_instance is duplicated; extract the
logic into a small helper (closure or local fn) that takes the MatchInstance
result and performs the actions now repeated (calling work.extend_from_parts
with unifications/constraints, extending blocked with stuck, setting
blocked_on_skolem, and no-op for Apart/None), then replace both match blocks
(the one guarded by prefer_declared_instances and the earlier one) to call this
helper with the result of compiler::match_compiler_instance(state, context,
wanted, &given) so behavior stays in sync for symbols:
prefer_declared_instances, compiler::match_compiler_instance,
matching::MatchInstance, work.extend_from_parts, blocked, blocked_on_skolem.

🤖 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.rs`:
- Around line 246-260: The match handling of compiler::match_compiler_instance
is duplicated; extract the logic into a small helper (closure or local fn) that
takes the MatchInstance result and performs the actions now repeated (calling
work.extend_from_parts with unifications/constraints, extending blocked with
stuck, setting blocked_on_skolem, and no-op for Apart/None), then replace both
match blocks (the one guarded by prefer_declared_instances and the earlier one)
to call this helper with the result of compiler::match_compiler_instance(state,
context, wanted, &given) so behavior stays in sync for symbols:
prefer_declared_instances, compiler::match_compiler_instance,
matching::MatchInstance, work.extend_from_parts, blocked, blocked_on_skolem.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8b890595-261d-44fe-a131-df7a9e33b9de

📥 Commits

Reviewing files that changed from the base of the PR and between 7d7b5c9 and 9dea7c2.

📒 Files selected for processing (1)

• compiler-core/checking/src/core/constraint.rs

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.10%. Comparing base (15405d0) to head (d70787e).

Files with missing lines	Patch %	Lines
compiler-core/checking/src/core/constraint.rs	85.71%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #123   +/-   ##
=======================================
  Coverage   83.09%   83.10%           
=======================================
  Files         130      130           
  Lines       23730    23757   +27     
  Branches    23730    23757   +27     
=======================================
+ Hits        19719    19743   +24     
- Misses       2237     2238    +1     
- Partials     1774     1776    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.