feat: add lambdas, native functions and the VM call-back bridge#85
Merged
Conversation
Sprint 5 Increment C. Lambdas become first-class values and flow through the native-function machinery that consumes them. Lambdas compile to a BytecodeFunction via a sub-compiler that shares the root const cache. Free identifiers resolve by D-296 categories 1-3 only: top-level const inlined, readonly/mutable read or written as globals. Block-body lambdas return their implicit last expression with return as a lambda-local early exit (D-276). Upvalue capture (category 4) is deferred to Increment D — no upvalue opcodes emitted. NativeFunction and the VmInvoker delegate live in Grob.Core so both the type checker and the VM can see them. The VM Call arm dispatches NativeFunction and BytecodeFunction transparently. The load-bearing piece is the re-entrant call-back bridge: ip/activeChunk/stackBase are promoted to VM instance fields and InvokeCallable drives a shared RunDispatch so a native can invoke a lambda argument back through the VM, holding the Increment-A frame discipline across the boundary. The four array higher-order methods land as natives bound at GetProperty time: filter, select, sort (stable, key-selector, descending) and each. sort orders int/float/string/bool keys. Folds in the D-319 cooperative-cancellation seam: the run entry takes a CancellationToken (default None = unlimited) and the dispatch loop checks it on a masked interval. The step counter is a VM-instance field, so the budget is continuous across the bridge — a runaway lambda invoked by a native is caught, not only a top-level loop. Cancellation surfaces as OperationCanceledException, outside the GrobError hierarchy and so uncatchable by a Grob catch. Corrects the section 6 acceptance prose from "filter, map, sort" to "filter, select, sort" (D-280 removed map). Three faults in this increment's own (uncommitted) code, found by the new integration tests, are fixed here: the RunDispatch Return arm terminated the whole script when a top-level call returned to frame 0 (top-level vs re-entrant dispatch are now distinguished); the sort comparer passed line 0 to GrobRuntimeException, which requires line >= 1; and a comparer fault wrapped by the sort helper in InvalidOperationException is now unwrapped to the GrobRuntimeException. No new opcodes, no parser or AST edits. Coverage stays above 90% on the affected projects (GrobValueComparer 100%, ArrayNatives 98%, VirtualMachine 94.6%, modified compiler files 92-98%). Implements D-115, D-276, D-280, D-281, D-296, D-319. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughImplements Sprint 5 Increment C for the Grob language: lambda expression compilation into nested ChangesSprint 5C: Lambdas, Array HOFs, Native Bridge & Cancellation
Sequence Diagram(s)sequenceDiagram
participant GrobSource
participant TypeChecker
participant Compiler
participant VirtualMachine
participant NativeFunction
participant InvokeCallable
GrobSource->>TypeChecker: arr.filter(x => x > 0)
TypeChecker->>TypeChecker: VisitLambda — register params, infer return type → _lambdaReturnTypes
TypeChecker->>TypeChecker: ValidateArrayMethodCall — check predicate return is Bool
TypeChecker-->>Compiler: typed AST
Compiler->>Compiler: VisitLambda — nested Compiler, emit BytecodeFunction constant
Compiler-->>VirtualMachine: Chunk with BytecodeFunction + GetProperty("filter") + Call
VirtualMachine->>VirtualMachine: OpCode.GetProperty("filter") — bind NativeFunction capturing _cancellationToken
VirtualMachine->>NativeFunction: OpCode.Call — extract args, thread VmInvoker
loop for each element
NativeFunction->>InvokeCallable: VmInvoker(lambdaFn, [element])
InvokeCallable->>VirtualMachine: RunDispatch(isReentrant=true)
VirtualMachine-->>InvokeCallable: predicate GrobValue result
InvokeCallable-->>NativeFunction: result
end
NativeFunction-->>VirtualMachine: filtered GrobValue array
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/Grob.Vm/ArrayNatives.cs (1)
48-48: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRemove unnecessary collection-expression spread.
Line 48 spreads
resultinto a collection expression ([.. result]), which creates an intermediate copy, then theGrobArrayconstructor spreads it again into its internal list (context snippet 2, line 11). Pass theList<GrobValue>directly asIEnumerable<GrobValue>to avoid the intermediate allocation.♻️ Proposed fix to eliminate double-copy
- return GrobValue.FromArray(new GrobArray([.. result])); + return GrobValue.FromArray(new GrobArray(result));🤖 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 `@src/Grob.Vm/ArrayNatives.cs` at line 48, In the return statement with GrobValue.FromArray and GrobArray constructor, remove the collection-expression spread operator ([..]) when passing the result list. Instead of new GrobArray([.. result]), pass result directly as new GrobArray(result) to avoid creating an intermediate copy, since the GrobArray constructor already handles converting the IEnumerable into its internal list representation.
🤖 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 `@tests/Grob.Compiler.Tests/CompilerLambdaTests.cs`:
- Around line 283-295: The test method EmptyBlockBodyLambda_EmitsNilAndReturn
claims to test a lambda with an empty block body according to its comment, but
the actual source code being compiled contains a non-empty block with the
statement `x > 0`. Remove this statement from inside the lambda block in the
CompileSource call to make it truly empty, changing the lambda from `x => { x >
0 }` to `x => {}` so the test actually exercises the empty-block-body code path
as intended.
In `@tests/Grob.Compiler.Tests/TypeCheckerLambdaTests.cs`:
- Around line 99-115: The test method
Lambda_BlockBody_AllIdentifiers_SatisfySection311Invariant can pass without
actually validating the block-body invariant because the bag variable is never
checked and the identifiers collection could be empty, causing the foreach loop
to never execute. Add an Assert.False call immediately after the
CollectIdentifiers call to verify that bag.HasErrors is false, then add an
Assert.NotEmpty or Assert.True(identifiers.Count > 0) assertion to ensure the
identifiers collection is not empty before the foreach loop, so the test
actually validates the intended invariant.
- Around line 168-175: The Sort_WithNonBoolDescendingFlag_EmitsE0004 test method
only asserts the error code but must also verify the exact 1-based line and
column numbers as part of the full diagnostic contract. Update the
Assert.Contains expression to check not only that d.Code equals "E0004" but also
verify d.Line and d.Column match the expected position where the non-bool
argument (42) appears in the test code. Apply the same fix to the other E0004
test referenced in lines 207-219 so both tests validate the complete diagnostic
specification.
- Around line 63-69: The test in the foreach loop over identifiers only
validates that Declaration is not null, but the §3.1.1 invariant requires both
Declaration and ResolvedType to be non-null after type-check. The misleading
comment suggests ResolvedType validation is optional for lambda parameters, but
GrobType.Unknown is still a valid non-null type value. Add an assertion to
verify that id.ResolvedType is not null alongside the existing Declaration
assertion, remove the comment that incorrectly exempts ResolvedType from
validation, and apply this same fix to the similar assertion blocks referenced
in lines 89-95, 112-114, and 275-276.
---
Nitpick comments:
In `@src/Grob.Vm/ArrayNatives.cs`:
- Line 48: In the return statement with GrobValue.FromArray and GrobArray
constructor, remove the collection-expression spread operator ([..]) when
passing the result list. Instead of new GrobArray([.. result]), pass result
directly as new GrobArray(result) to avoid creating an intermediate copy, since
the GrobArray constructor already handles converting the IEnumerable into its
internal list representation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8795a03f-e070-4e9b-be82-8f6821bb5641
📒 Files selected for processing (15)
docs/design/grob-v1-requirements.mdsrc/Grob.Compiler/Compiler.Expressions.cssrc/Grob.Compiler/TypeChecker.Expressions.cssrc/Grob.Compiler/TypeChecker.Statements.cssrc/Grob.Compiler/TypeChecker.cssrc/Grob.Core/NativeFunction.cssrc/Grob.Core/VmInvoker.cssrc/Grob.Vm/ArrayNatives.cssrc/Grob.Vm/VirtualMachine.cstests/Grob.Compiler.Tests/CompilerLambdaTests.cstests/Grob.Compiler.Tests/TypeCheckerLambdaTests.cstests/Grob.Core.Tests/RuntimeTypesTests.cstests/Grob.Integration.Tests/Sprint5IncrementCTests.cstests/Grob.Vm.Tests/VirtualMachineCancellationTests.cstests/Grob.Vm.Tests/VirtualMachineNativeTests.cs
Triage and fixes from the bot review of the Sprint 5C lambdas/natives
PR. Behaviour is unchanged; the production fixes remove dead code and
a redundant allocation, the rest strengthen tests.
Production:
- Compiler.EmitArithmetic: drop the always-false `rt == Float` sub-
condition in the Unknown-baseType fallback. When baseType is Unknown
both operands are non-float by construction, so the fallback can only
be Int (CodeQL constant-condition; one of the two Sonar new issues).
- ArrayNatives.Filter: pass the result List straight to the GrobArray
ctor (it takes IEnumerable and copies once) instead of a `[.. result]`
spread that added a redundant intermediate array.
Tests:
- TypeCheckerLambdaTests: assert the type side of the §3.1.1 invariant
too, via Assert.NotEqual(GrobType.Error, id.ResolvedType). ResolvedType
is a non-nullable enum, so CodeRabbit's literal Assert.NotNull would be
an xUnit2002 error; the Error-sentinel check is the correct value-type
form, and a lambda parameter's inferred Unknown is a valid resolved
value, not the error sentinel.
- TypeCheckerLambdaTests: the block-body invariant test no longer passes
vacuously — assert bag.HasErrors is false and the identifier set is
non-empty before iterating (also retires the unused-bag CodeQL note).
- TypeCheckerLambdaTests: both E0004 tests now assert the full diagnostic
contract (code plus exact 1-based line and column), per tests/CLAUDE.md.
- CompilerLambdaTests: the empty-block-body test now compiles a genuinely
empty block (x => {}) so it exercises the safety-net Nil + Return path
it claims to; asserts the exact [Nil, Return] shape.
- Remove an unused OperationCanceledException local in the cancellation
type-hierarchy test; convert four foreach-with-map/filter loops in test
builders to Where/Select (CodeQL quality queries).
Addresses PR #85 review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
Review triage — committed locally as
|
| # | Finding | Source | Disposition |
|---|---|---|---|
| 1 | EmitArithmetic constant condition (always-false rt == Float) |
CodeQL / Sonar | Fixed — simplified to Unknown ? Int : baseType |
| 2 | ArrayNatives.Filter [.. result] double-copy |
CodeRabbit nitpick | Fixed — pass the List to the GrobArray(IEnumerable) ctor directly |
| 3 | §3.1.1 type side unchecked (4 loops) | CodeRabbit | Fixed + pushed back on literal — Assert.NotNull(ResolvedType) is xUnit2002 (non-nullable enum); used Assert.NotEqual(GrobType.Error, …), the correct value-type form |
| 4 | Block-body invariant test passes vacuously | CodeRabbit + CodeQL | Fixed — assert !bag.HasErrors + NotEmpty(identifiers) |
| 5 | E0004 tests assert code only | CodeRabbit | Fixed — full contract (code + exact 1-based line + column) |
| 6 | Empty-block test isn't empty (x > 0 present) |
CodeRabbit | Fixed — genuinely empty x => {}, asserts [Nil, Return] |
| 7 | Unused cancelled local |
CodeQL | Fixed — removed |
| 8 | Unused bag local |
CodeQL | Fixed — now asserted (same test as #4) |
| 9 | 4× foreach→Where/Select in test builders | CodeQL | Fixed — converted (off hot path, behaviour preserved) |
One push-back, on the record: CodeRabbit's suggested Assert.NotNull(id.ResolvedType) cannot compile here — ResolvedType is a non-nullable GrobType enum, so xUnit2002 fires and the build fails under TreatWarningsAsErrors. The §3.1.1 type-side invariant is instead asserted as Assert.NotEqual(GrobType.Error, id.ResolvedType); a lambda parameter's inferred Unknown is a valid resolved value (concrete inference is Increment D), not an unset default.
The two SonarCloud new issues are covered by #1 (constant condition) and the dead-store fixes (#7/#8). Bots re-run on push, so a fresh pass will follow.
Follow-up to the first review pass, addressing the findings the bots
raised after it ran. Behaviour unchanged.
- Compiler.CompileLambdaBlock: drop the unused `endLine` parameter
(Sonar S1172). The last-statement Return already uses the statement's
own line; `endLine` was dead. This was the one remaining Sonar new
issue after the first pass.
- CompilerLambdaTests.FirstLambdaConstant and the const-inlining test:
the first pass's foreach→Where conversions left a map in the loop body,
which CodeQL then re-flagged ("missed opportunity to use Select"). Both
are now terminal LINQ pipelines (OfType+FirstOrDefault, and Any) that
end the cascade rather than shifting it one statement further.
The third CodeQL "use Select" note (Each test) is a push-back, not a
fix: that loop's body calls the side-effecting `invoker` (it runs the
lambda through the VM and records the result), so projecting it into a
Select would be a side-effecting projection — an anti-pattern. The
explicit foreach is correct there.
Addresses PR #85 review (SonarCloud + CodeQL follow-up).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
Review triage (follow-up pass) — committed locally as
|
| Finding | Source | Disposition |
|---|---|---|
S1172 unused endLine param in CompileLambdaBlock |
SonarCloud (the 1 remaining new issue) | Fixed — removed; the Return already uses the statement's own line |
FirstLambdaConstant foreach maps in body |
CodeQL | Fixed — terminal LINQ (OfType<BytecodeFunction>().FirstOrDefault() ?? throw), ends the cascade |
| const-inlining test foreach maps in body | CodeQL | Fixed — collapsed to terminal .Any(...) |
Each test foreach maps to invoker(...) result |
CodeQL | Push-back — invoker is side-effecting; a .Select projection would be a side-effecting anti-pattern |
Note on the CodeQL chain: the two fixed ones were a cascade my own first-pass .Where() conversions triggered — extracting the filter left a map in the loop body, which CodeQL then re-flagged. Switching to terminal LINQ operators (FirstOrDefault/Any) ends the cascade instead of shifting it one statement on.
The four CodeRabbit follow-up replies on the first-pass threads were acknowledgements (all marked resolved) — no further action.
After this lands, SonarCloud new issues should reach 0 and the two fixed CodeQL notes clear; the Each push-back will remain as a deliberate, on-record decision.
|
This was referenced Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sprint 5 Increment C. Lambdas become first-class values and flow through the native-function machinery that consumes them.
Lambdas compile to a BytecodeFunction via a sub-compiler that shares the root const cache. Free identifiers resolve by D-296 categories 1-3 only: top-level const inlined, readonly/mutable read or written as globals. Block-body lambdas return their implicit last expression with return as a lambda-local early exit (D-276). Upvalue capture (category 4) is deferred to Increment D — no upvalue opcodes emitted.
NativeFunction and the VmInvoker delegate live in Grob.Core so both the type checker and the VM can see them. The VM Call arm dispatches NativeFunction and BytecodeFunction transparently. The load-bearing piece is the re-entrant call-back bridge: ip/activeChunk/stackBase are promoted to VM instance fields and InvokeCallable drives a shared RunDispatch so a native can invoke a lambda argument back through the VM, holding the Increment-A frame discipline across the boundary.
The four array higher-order methods land as natives bound at GetProperty time: filter, select, sort (stable, key-selector, descending) and each. sort orders int/float/string/bool keys.
Folds in the D-319 cooperative-cancellation seam: the run entry takes a CancellationToken (default None = unlimited) and the dispatch loop checks it on a masked interval. The step counter is a VM-instance field, so the budget is continuous across the bridge — a runaway lambda invoked by a native is caught, not only a top-level loop. Cancellation surfaces as OperationCanceledException, outside the GrobError hierarchy and so uncatchable by a Grob catch.
Corrects the section 6 acceptance prose from "filter, map, sort" to "filter, select, sort" (D-280 removed map).
Three faults in this increment's own (uncommitted) code, found by the new integration tests, are fixed here: the RunDispatch Return arm terminated the whole script when a top-level call returned to frame 0 (top-level vs re-entrant dispatch are now distinguished); the sort comparer passed line 0 to GrobRuntimeException, which requires line
No new opcodes, no parser or AST edits. Coverage stays above 90% on the affected projects (GrobValueComparer 100%, ArrayNatives 98%, VirtualMachine 94.6%, modified compiler files 92-98%).
Implements D-115, D-276, D-280, D-281, D-296, D-319.
Summary by CodeRabbit
Release Notes
New Features
filter,select,sort, andeach(including correct handling of block-bodied lambdas).Bug Fixes
Unknownhandling in type-checking for unary arithmetic, lambda/predicate validation, and assignments.sort/comparison error surfacing for key-type issues.Documentation