feat: Implement positional function calls and call frame management#79
Merged
Conversation
Implements Sprint 5 Increment A: fn declarations compile to their own bytecode, calls and returns run over a call-frame stack, and recursion works within a bounded depth. Core gains BytecodeFunction (a GrobFunction carrying its own Chunk); GrobFunction becomes abstract, so the Core tests that constructed it directly now build a BytecodeFunction. The VM gains a CallFrame array and frame-relative local addressing: GetLocal/SetLocal (and the increment/decrement arms) resolve slots against the active frame's stack base. The script runs at base zero, so existing top-level locals are unaffected. Call saves the caller's resume context and switches into the callee's chunk; Return discards the callee value, its arguments and locals in one step, then pushes the result. The frames array holds call frames only (the script is not a frame), so a 257th nested call overflows a 256-entry array and raises E5901 as a clean RuntimeError rather than a host CLR stack overflow. Implements D-180 (call-stack overflow), D-166 (forward references via pass-1 registration) and the positional path of D-113. The type checker validates fn bodies and positional call sites: argument count (E0003), argument types (E0004) and return-type mismatch (E0005), plus top-level return (E2203). All reuse existing ErrorCatalog descriptors; no new codes. A call resolves to the function's declared return type so surrounding operators select the right typed opcode. All-paths-return is not a v1 rule and no error code is assigned, so the compiler emits an implicit Nil + Return safety net at the end of every body (control fall-off returns nil) rather than inventing a check. No new opcodes, no parser or AST edits. Tests: VM call/recursion/E5901, type-checker diagnostics and the section 3.1.1 invariant, compiler emission shape, and end-to-end recursion (factorial, fibonacci). Affected-file line coverage ~96%; the one new uncovered line is a defensive parameter-slot-overflow guard, matching the existing local-slot-overflow guard convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Integrates the D-318 drop-in into grob-decisions-log.md in
four-location lockstep: summary-index row, full entry, and footer
changelog line. D-318 assigns the four named-argument call-site
errors from D-113 dedicated codes E0008-E0011 (named-before-
positional, naming a required parameter, duplicate named argument,
unknown parameter name) rather than folding them into E0003, since
none is an arity error and each carries a distinct fix.
No error codes are registered by this entry — Sprint 5 Increment B
adds the E0008-E0011 descriptors to the catalog. D-318 must exist
before that increment runs, which is why it lands now.
Removes docs/design/_pending/D-318-drop-in.md: it was an explicit
transient staging block ("NOT an integrated file") whose content is
now in the canonical log, so keeping it would duplicate the D-318
prose. Consistency gate (D-316 lockstep) green.
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughSprint 5 Increment A implements end-to-end user-defined function support: ChangesSprint 5A: Function declaration, call, and return
Sequence Diagram(s)sequenceDiagram
participant Source as Source text
participant TypeChecker as TypeChecker
participant Compiler as Compiler
participant VM as VirtualMachine
Source->>TypeChecker: VisitFnDecl — push return type, visit body, pop
TypeChecker->>TypeChecker: VisitCall — CheckPositionalCall (E0003), CheckArgumentType (E0004)
TypeChecker->>TypeChecker: VisitReturn — E2203 at script level, E0005 on mismatch
Source->>Compiler: VisitFnDecl → CompileFunction → BytecodeFunction constant → DefineGlobal
Source->>Compiler: VisitCall → callee + arguments + OpCode.Call(argCount)
Source->>Compiler: VisitReturn → value or Nil + OpCode.Return
Compiler->>VM: Chunk with BytecodeFunction constants and Call/Return opcodes
VM->>VM: OpCode.Call — save CallFrame, update stackBase, swap chunk
VM->>VM: GetLocal/SetLocal at stackBase+slot
VM->>VM: OpCode.Return — TrimToCount(calleeBase), push result, restore CallFrame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: 2
🧹 Nitpick comments (1)
src/Grob.Compiler/TypeChecker.Expressions.cs (1)
243-275: 💤 Low valueConsider extracting argument-type validation to reduce cognitive complexity.
SonarCloud flags cognitive complexity at 17 (threshold 15). The logic is clear and linear, but if this method grows further (e.g., named arguments in Increment B), extracting the argument-count check and the per-argument type check into private helpers would keep it maintainable.
Not blocking for this increment — the current structure is readable and not on a hot path.
🤖 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.Compiler/TypeChecker.Expressions.cs` around lines 243 - 275, The VisitCall method has high cognitive complexity (17, exceeding the threshold of 15) due to nested validation logic. Extract the argument-count validation (the if statement comparing node.Arguments.Count to expected) into a private helper method, and extract the per-argument type checking logic (the for loop that validates each argument against the parameter type using TypesAreAssignable) into another private helper method. These helpers should encapsulate the EmitError calls and return early from VisitCall if validation fails, reducing the nesting depth and overall complexity within VisitCall while keeping the type checking logic organized and reusable.Source: Linters/SAST tools
🤖 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 `@src/Grob.Compiler/TypeChecker.Statements.cs`:
- Around line 57-68: The type checker currently only validates return statements
that have a value (node.Value is not null) but skips validation for bare return
statements (node.Value is null). Add an else clause after the existing if block
to handle bare returns: when node.Value is null, check if the expected return
type (from _functionReturnTypes.Peek()) is not Unknown and not Error, and if so,
emit an error using EmitError with ErrorCatalog.E0005 indicating that a bare
return is not allowed in a function that declares a non-void return type. Use
node.Range for the error location.
In `@tests/Grob.Vm.Tests/VirtualMachineCallTests.cs`:
- Around line 203-205: The test assertion for the E5901 error path is incomplete
and only verifies the error code through ex.Code. You need to add additional
Assert.Equal() statements to verify the exact line and column coordinates of the
GrobRuntimeException by asserting ex.Line and ex.Column with their expected
values using equality assertions to catch any off-by-one regressions in
diagnostic coordinate reporting.
---
Nitpick comments:
In `@src/Grob.Compiler/TypeChecker.Expressions.cs`:
- Around line 243-275: The VisitCall method has high cognitive complexity (17,
exceeding the threshold of 15) due to nested validation logic. Extract the
argument-count validation (the if statement comparing node.Arguments.Count to
expected) into a private helper method, and extract the per-argument type
checking logic (the for loop that validates each argument against the parameter
type using TypesAreAssignable) into another private helper method. These helpers
should encapsulate the EmitError calls and return early from VisitCall if
validation fails, reducing the nesting depth and overall complexity within
VisitCall while keeping the type checking logic organized and reusable.
🪄 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: 911fed68-7c6d-48e4-acc9-ad7952ac39db
📒 Files selected for processing (22)
.claude/commands/sprint-5-a.mddocs/design/_pending/D-318-drop-in.mddocs/design/grob-decisions-log.mdprompts/archive/sprint-5/sprint-5-a.mdsrc/Grob.Compiler/Compiler.Expressions.cssrc/Grob.Compiler/Compiler.Statements.cssrc/Grob.Compiler/Compiler.cssrc/Grob.Compiler/TypeChecker.Declarations.cssrc/Grob.Compiler/TypeChecker.Expressions.cssrc/Grob.Compiler/TypeChecker.Statements.cssrc/Grob.Compiler/TypeChecker.cssrc/Grob.Core/BytecodeFunction.cssrc/Grob.Core/GrobFunction.cssrc/Grob.Vm/CallFrame.cssrc/Grob.Vm/ValueStack.cssrc/Grob.Vm/VirtualMachine.cstests/Grob.Compiler.Tests/CompilerFunctionTests.cstests/Grob.Compiler.Tests/TypeCheckerFunctionTests.cstests/Grob.Core.Tests/GrobValueTests.cstests/Grob.Core.Tests/RuntimeTypesTests.cstests/Grob.Integration.Tests/Sprint5IncrementATests.cstests/Grob.Vm.Tests/VirtualMachineCallTests.cs
💤 Files with no reviewable changes (1)
- docs/design/_pending/D-318-drop-in.md
Addresses PR #79 review (CodeRabbit + SonarCloud). A bare `return` yields nil. Since `void` is not a user-declarable return type in Grob (only print() is void), a bare return must still satisfy the declared type: it now reports E0005 against a non-nullable return type and is accepted only by a nullable or nil one. Previously a value-less return skipped the check, so an int function could silently return nil. Uses the existing E0005 — no new code. Adds type-checker tests for both directions. Extracts the positional-call validation in VisitCall into CheckPositionalCall and CheckArgumentType helpers, dropping the method's cognitive complexity below the SonarCloud threshold of 15 (was 17 — the one new Sonar issue on this PR). No behaviour change. Asserts the full diagnostic contract (Line and Column, equality) on the E5901 stack-overflow VM test, per the test guidelines. Updates the bare-return compiler emission test to a nullable return type so its source type-checks under the tightened rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Addressed all review feedback in b2f236a: CodeRabbit — bare CodeRabbit — E5901 test diagnostic coordinates (major): Fixed. The stack-overflow test now asserts SonarCloud 1 new issue / CodeRabbit nitpick — VisitCall cognitive complexity 17 > 15: Fixed. Extracted Full suite green (Core 262, Compiler 712, Vm 216, Integration 158, Consistency 30); build clean with zero warnings. |
|
This was referenced Jun 21, 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.
This pull request finalizes the design decision for dedicated error codes for named-argument call-site diagnostics, integrates the new decision into the authoritative log, and updates the compiler to support function declarations, calls, and recursion as specified for Sprint 5 Increment A. It also clarifies the runtime callee resolution mechanism and mutual recursion handling in both the design documentation and planning prompts.
Design and Documentation Updates:
grob-decisions-log.md, assigning dedicated error codes (E0008–E0011) for the four named-argument call-site errors, with detailed rationale, summary index, and changelog entry. [1] [2] [3]D-318-drop-in.md), as the decision is now fully integrated into the canonical log.Compiler Implementation (Sprint 5 Increment A):
Compiler.Statements.cs, including the creation ofBytecodeFunction, parameter slot assignment, and global binding.Compiler.Expressions.cs, emitting the callee, arguments, and theCallopcode with argument count.Planning and Spec Clarifications:
These changes ensure that the language's function call and recursion semantics match the documented design, and that error diagnostics for named-argument calls are precise and actionable.
Summary by CodeRabbit
Release Notes
returnis type-checked against the declared return type.