[checking] Allow recovery for do expressions with final let#159
Conversation
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. 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 (2)
📒 Files selected for processing (1)
SummaryThis PR makes the do-expression inference recover when a do-block ends with a final let binding. It refactors the terminal-step handling in infer_do so the checker can continue inference (with placeholder continuation types) instead of bailing out immediately on a final let — preparatory work for inference-driven completions. Key changes
Behavioural impact
Notes / Minor issues
Confidence 4/5 — "Doesn't crash, now tutors the user."
WalkthroughRefactors infer_do to explicitly model the final do-block step and final expression, classify action steps, adjust continuation sizing, and conditionally perform final-expression checking; adds an integration fixture exercising final-let handling. ChangesDo-block final step refactoring
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
+ Coverage 83.21% 83.25% +0.04%
==========================================
Files 133 133
Lines 25132 25149 +17
Branches 25132 25149 +17
==========================================
+ Hits 20913 20939 +26
+ Misses 2264 2259 -5
+ Partials 1955 1951 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@compiler-core/checking/src/source/terms/form_do.rs`:
- Around line 402-407: The InvalidLet branch currently returns
context.unknown("invalid final let"), which discards the previously constructed
placeholder continuation and loses recovered monadic/effect information;
instead, preserve the placeholder continuation by routing this branch through
the same checking path as the valid final expression: call
super::check_expression with the appropriate synthetic/dummy final expression
(so the existing final_continuation is used) rather than returning a fresh
Unknown. Locate the DoBlockFinalExpression::InvalidLet branch and replace the
context.unknown return with a call that invokes super::check_expression(state,
context, /* synthetic final expression */, final_continuation) so the
placeholder continuation and recovered effect/shape are retained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fcbf379-d33d-4d96-9bef-0d5592f397b7
⛔ Files ignored due to path filters (1)
tests-integration/fixtures/checking/1780673760_do_final_let/Main.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
compiler-core/checking/src/source/terms/form_do.rstests-integration/fixtures/checking/1780673760_do_final_let/Main.purs
| match final_expression { | ||
| DoBlockFinalExpression::Check(expression) => { | ||
| super::check_expression(state, context, expression, final_continuation)?; | ||
| } | ||
| DoBlockFinalExpression::InvalidLet => { | ||
| return Ok(context.unknown("invalid final let")); |
There was a problem hiding this comment.
Returning Unknown here drops the recovered do-block type.
The placeholder continuation built above is what lets earlier bind/discard steps solve the monadic shape, but this branch throws that away and makes the whole do expression a fresh Unknown. In a block like do record <- before; let _ = ..., downstream inference still loses the recovered Effect _, which undercuts the recovery this change is aiming for.
🤖 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/source/terms/form_do.rs` around lines 402 - 407,
The InvalidLet branch currently returns context.unknown("invalid final let"),
which discards the previously constructed placeholder continuation and loses
recovered monadic/effect information; instead, preserve the placeholder
continuation by routing this branch through the same checking path as the valid
final expression: call super::check_expression with the appropriate
synthetic/dummy final expression (so the existing final_continuation is used)
rather than returning a fresh Unknown. Locate the
DoBlockFinalExpression::InvalidLet branch and replace the context.unknown return
with a call that invokes super::check_expression(state, context, /* synthetic
final expression */, final_continuation) so the placeholder continuation and
recovered effect/shape are retained.
There was a problem hiding this comment.
@coderabbitai Then if we return first_continuation, depending on the shape of the action statements we would get either an unsolved unification variable or the real type of the last action statement. I think this makes sense
There was a problem hiding this comment.
@purefunctor That reasoning is exactly right. Returning first_continuation is the correct fix here:
DoBlockFinalExpression::InvalidLet => {
return Ok(first_continuation);
}- With prior action steps —
first_continuationis already solved to the monadic shape established bybind/discard, so the do-expression type is recovered correctly. - With no prior action steps —
first_continuationis the sole placeholder unification variable introduced byusize::from(final_step.is_invalid_let()), which is still better than a freshUnknownbecause it remains in the constraint graph and can be solved by an outer context.
Either way it avoids silently throwing away the work done by the continuation-building loop. The current context.unknown(...) return is the thing that actually "undercuts recovery", as I flagged — returning first_continuation is the minimal, correct fix.
✏️ Learnings added
Learnt from: purefunctor
Repo: purefunctor/purescript-alexandrite PR: 159
File: compiler-core/checking/src/source/terms/form_do.rs:402-407
Timestamp: 2026-06-05T20:51:36.615Z
Learning: In `compiler-core/checking/src/source/terms/form_do.rs`, when `DoBlockFinalExpression::InvalidLet` is reached in `infer_do`, the correct recovery behavior is to return `Ok(first_continuation)` rather than `Ok(context.unknown("invalid final let"))`. `first_continuation` is solved through prior bind/discard action steps and preserves the monadic shape; if no action steps precede the final let, it is the placeholder unification variable introduced by `usize::from(final_step.is_invalid_let())`, which is still better than a fresh unknown that loses all constraint context.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: purefunctor
Repo: purefunctor/purescript-alexandrite PR: 155
File: compiler-core/lowering/src/algorithm.rs:119-127
Timestamp: 2026-06-01T19:07:01.118Z
Learning: In this codebase, `NonFatal` is not an error: it is shorthand for returning `Ok(None)` (meaning “no result, but not a failure”). During Rust code reviews, treat `NonFatal` flows as a non-error/“no result” path and avoid flagging them as error-handling or failure cases; only consider the code to be in an error path when it returns an actual `Err(...)`.
purefunctor
force-pushed
the
justin/do-checking-recovery
branch
from
d9eed08 to
a4cf1d3
Compare
1 participant
Prerequisite work for inference-driven completions