feat(compiler): add named and default arguments#81
Merged
Conversation
Complete the call surface with default parameter values and the D-113 named-argument calling convention — positional arguments first, named arguments after, only defaulted parameters may be named. The type checker now type-checks default expressions against their parameter at the declaration site, and binds calls in three phases (positionals, then named into defaulted parameters, then defaults fill the rest) before running arity and type checks on the bound set. The compiler reorders named arguments into parameter declaration order and materialises omitted defaults, so the unchanged Call opcode receives a fully-bound positional list. No new opcodes, no VM dispatch change. Register the four call-site diagnostics as dedicated codes per D-318: E0008 named-before-positional, E0009 naming a required parameter, E0010 duplicate named argument, E0011 unknown parameter name. Each has an ErrorCatalog descriptor (D-308) and a gold-master example pair; the canonical error-code total moves 103 -> 107 in lockstep with the count anchor (D-316). E0003 and E0004 still fire on the bound argument set. Tests: checker binding cases for all four codes plus defaults, compiler reorder-and-fill bytecode shape, a hand-built VM chunk, and an end-to-end integration script. Coverage on the affected files stays above 90% (98.5%/95.2%/95.0%). Implements D-113 (named parameter calling convention) and D-318 (dedicated call-site diagnostic codes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughImplements the D-113 named-argument calling convention in the Grob compiler. Adds four new error descriptors (E0008–E0011) to ChangesNamed-Argument Calling Convention (E0008–E0011)
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
🤖 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/Compiler.Expressions.cs`:
- Around line 556-558: The issue is that default parameter expressions (from
fn.Parameters[i].DefaultValue) are being compiled at the call site in the
caller's scope rather than the callee's scope. When a default references another
parameter (like b: int = a + 1), the identifier lookup happens against the wrong
scope. Fix this by either moving the evaluation of omitted defaults to occur
within the callee frame using the appropriate scope context, or alternatively
add validation in the type-checking phase to reject default expressions that are
not self-contained and reference other parameters, using a diagnostic to report
this as an error to the user.
In `@src/Grob.Compiler/TypeChecker.Expressions.cs`:
- Around line 285-290: In the CheckCall method where the E0008 error is emitted
for argument ordering violations, replace the direct return statement with
setting bindingError = true and using continue to move to the next argument.
This allows the method to collect all binding errors in the same call instead of
terminating early, and the existing if (bindingError) return statement will
properly gate the downstream logic after all errors have been collected.
🪄 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: f3f5838a-fbec-4a50-9cb9-3fd689d9b12b
📒 Files selected for processing (19)
docs/design/grob-error-codes.mddocs/errors/examples/README.mddocs/errors/examples/duplicate-named-argument/duplicate-named-argument.expected.txtdocs/errors/examples/duplicate-named-argument/duplicate-named-argument.grobdocs/errors/examples/named-argument-before-positional/named-argument-before-positional.expected.txtdocs/errors/examples/named-argument-before-positional/named-argument-before-positional.grobdocs/errors/examples/named-argument-names-required-parameter/named-argument-names-required-parameter.expected.txtdocs/errors/examples/named-argument-names-required-parameter/named-argument-names-required-parameter.grobdocs/errors/examples/unknown-parameter-name/unknown-parameter-name.expected.txtdocs/errors/examples/unknown-parameter-name/unknown-parameter-name.grobsrc/Grob.Compiler/Compiler.Expressions.cssrc/Grob.Compiler/TypeChecker.Declarations.cssrc/Grob.Compiler/TypeChecker.Expressions.cssrc/Grob.Core/ErrorCatalog.cstests/Grob.Compiler.Tests/CompilerNamedArgumentTests.cstests/Grob.Compiler.Tests/TypeCheckerNamedArgumentTests.cstests/Grob.Consistency.Tests/ErrorCodeCountTests.cstests/Grob.Integration.Tests/Sprint5IncrementBTests.cstests/Grob.Vm.Tests/VirtualMachineNamedArgumentTests.cs
Addresses PR #81 review (CodeRabbit + SonarCloud). Check parameter defaults in the function's enclosing scope rather than the parameter scope. Defaults materialise at the call site (D-113), so a default that references a sibling parameter is incoherent: it bound to the parameter at check time but compiled against caller scope, a silent miscompile (a same-named global was read instead of the argument). It is now a compile-time E1001 — defaults resolve only against identifiers visible at the call site. (CodeRabbit; resolution chosen by Chris.) Collect all call-site binding errors instead of returning after the first E0008. A positional-after-named violation now sets the error flag and continues, so independent later binding errors (E0009/E0010/E0011) still surface; the existing gate still suppresses the arity/type cascade. Honours the two-mode "one diagnostic per root cause, no cap" rule. (CodeRabbit.) Refactor CheckCall into BindArguments, TryBindNamed and EmitArityError to bring cognitive complexity under the threshold (SonarCloud S3776, 29 -> within 15) and extract the nested ternary in the arity message into a Plural helper (S3358). Remove the unused 'line' parameter from EmitReorderedArguments (S1172). No behaviour change from the refactor. Tests: a default referencing a sibling parameter raises E1001; a default referencing an enclosing const is clean; two independent binding errors in one call are both reported. Compiler suite 733 green; full solution green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
Review triage — committed locally as
|
| # | Source | Finding | Disposition |
|---|---|---|---|
| 1 | CodeRabbit | Defaults compiled in caller scope → sibling-param defaults miscompile | Fixed — defaults now checked in the enclosing scope; sibling-param reference is a compile-time E1001. Confirmed a silent-wrong-value path first (101 vs 6). Resolution chosen by the maintainer. |
| 2 | CodeRabbit | return after first E0008 stops error collection |
Fixed — collect all binding errors, then gate the cascade. |
| 3 | SonarCloud S1172 | Unused line parameter in EmitReorderedArguments |
Fixed — parameter removed. |
| 4 | SonarCloud S3776 | CheckCall cognitive complexity 29 > 15 |
Fixed — extracted BindArguments / TryBindNamed / EmitArityError; compile-time code, not a hot path, so helpers are the right call. |
| 5 | SonarCloud S3358 | Nested ternary in the arity message | Fixed — extracted a Plural helper. |
All three SonarCloud new issues resolved (the bar is zero new issues, not just a passing gate).
Tests added: sibling-param default → E1001; enclosing-const default → clean; two independent binding errors both reported. Build: 0 warnings. Tests: full solution green (Compiler 733).
Note: the push and the re-run of CodeRabbit/SonarCloud are the maintainer's — a fresh bot pass will follow the next push.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Grob.Compiler.Tests/TypeCheckerNamedArgumentTests.cs (1)
310-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse reference-based sentinel assertion for
GrobType.Error.At Line 312,
Assert.NotEqualchecks value equality, but D-311 requires sentinel identity semantics. UseAssert.NotSame(GrobType.Error, id.ResolvedType)here.Suggested patch
foreach (IdentifierExpr id in collector.Identifiers) { Assert.NotNull(id.Declaration); - Assert.NotEqual(GrobType.Error, id.ResolvedType); + Assert.NotSame(GrobType.Error, id.ResolvedType); }As per coding guidelines, sentinel checks for
GrobType.Error/UnresolvedDecl.Instancemust use reference semantics (Assert.Same/identity-based assertions), not value comparisons.🤖 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 `@tests/Grob.Compiler.Tests/TypeCheckerNamedArgumentTests.cs` around lines 310 - 313, In the foreach loop that iterates through collector.Identifiers, the assertion checking id.ResolvedType uses Assert.NotEqual for the GrobType.Error sentinel comparison. Replace this with Assert.NotSame to enforce reference-based identity semantics instead of value equality, as required by the coding guidelines for sentinel values like GrobType.Error.Source: Coding guidelines
🤖 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/TypeCheckerNamedArgumentTests.cs`:
- Around line 231-245: The test method MultipleBindingErrors_AllReported
currently only validates the error count and error codes, but does not assert
the exact 1-based line and column positions for each diagnostic. Enhance the
assertions to verify the full diagnostic contract by adding assertions that
check the exact Line and Column properties of each error. For the E0008 error
(positional after named), verify it points to the correct location of the '2'
argument on line 4, and for the E0011 error (unknown name), verify it points to
the correct location of 'x: 3' on line 4. This ensures diagnostic range accuracy
and prevents regressions.
---
Outside diff comments:
In `@tests/Grob.Compiler.Tests/TypeCheckerNamedArgumentTests.cs`:
- Around line 310-313: In the foreach loop that iterates through
collector.Identifiers, the assertion checking id.ResolvedType uses
Assert.NotEqual for the GrobType.Error sentinel comparison. Replace this with
Assert.NotSame to enforce reference-based identity semantics instead of value
equality, as required by the coding guidelines for sentinel values like
GrobType.Error.
🪄 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: e4d226ee-cbaa-45bd-953f-76297a6f9fa5
📒 Files selected for processing (4)
src/Grob.Compiler/Compiler.Expressions.cssrc/Grob.Compiler/TypeChecker.Declarations.cssrc/Grob.Compiler/TypeChecker.Expressions.cstests/Grob.Compiler.Tests/TypeCheckerNamedArgumentTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Grob.Compiler/TypeChecker.Declarations.cs
- src/Grob.Compiler/TypeChecker.Expressions.cs
- src/Grob.Compiler/Compiler.Expressions.cs
Collaborator
Author
Follow-up review triage — committed locally as
|
| Source | Finding | Disposition |
|---|---|---|
CodeRabbit (test :245) |
MultipleBindingErrors_AllReported asserts only count + codes |
Fixed — added exact line/column for both E0008 (4:9) and E0011 (4:12) |
github-code-quality (Declarations.cs:57) |
Use .Where() instead of a continue guard |
Pushed back — the is null guard gives flow-narrowing so Visit(p.DefaultValue) needs no !; .Where would force a null-forgiving operator the project style avoids. Behaviour-identical, no gain. |
| CodeRabbit (replies on the two resolved threads) | Acknowledgements of the 61122c4 fixes |
No action |
| SonarCloud (re-run) | 0 new issues, gate passed | Confirmed clean — the three prior findings (S1172/S3776/S3358) are resolved |
Build: 0 warnings. Tests: Compiler 733 green.
The push and the next bot re-run are the maintainer's.
kwakker35
added a commit
that referenced
this pull request
Jun 22, 2026
…g test Addresses PR #81 follow-up review (CodeRabbit). MultipleBindingErrors_AllReported asserted only the error count and codes. Per the test conventions, a test exercising error output must assert the full diagnostic contract — exact code and 1-based line and column, by equality — so range regressions cannot slip through. Adds line/column assertions for both the E0008 (out-of-order positional, 4:9) and E0011 (unknown name, 4:12) diagnostics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Complete the call surface with default parameter values and the D-113 named-argument calling convention — positional arguments first, named arguments after, only defaulted parameters may be named.
The type checker now type-checks default expressions against their parameter at the declaration site, and binds calls in three phases (positionals, then named into defaulted parameters, then defaults fill the rest) before running arity and type checks on the bound set. The compiler reorders named arguments into parameter declaration order and materialises omitted defaults, so the unchanged Call opcode receives a fully-bound positional list. No new opcodes, no VM dispatch change.
Register the four call-site diagnostics as dedicated codes per D-318: E0008 named-before-positional, E0009 naming a required parameter, E0010 duplicate named argument, E0011 unknown parameter name. Each has an ErrorCatalog descriptor (D-308) and a gold-master example pair; the canonical error-code total moves 103 -> 107 in lockstep with the count anchor (D-316). E0003 and E0004 still fire on the bound argument set.
Tests: checker binding cases for all four codes plus defaults, compiler reorder-and-fill bytecode shape, a hand-built VM chunk, and an end-to-end integration script. Coverage on the affected files stays above 90% (98.5%/95.2%/95.0%).
Implements D-113 (named parameter calling convention) and D-318 (dedicated call-site diagnostic codes).
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes
Tests