Conversation
Handle else-if parsing and allow int/float/etc as callable types while fixing compound operator parsing and for-loop iterators.
Track immutable bindings and block reassignment while adding %=, &=, |=, ^= handling and expanding for-in behavior.
Add broad evaluator coverage for immutability, compound assignments, for-in behavior, and builtins.
Treat variadic builtins as varargs and tighten call argument validation.
Introduce arena allocation plus return type inference to improve generated C output and result handling.
Document new builtins and operators, refresh language/architecture notes, and update the hello example.
Capture design notes covering ownership semantics, async execution, and interface modeling.
Thread checker type info into codegen via recorded node types and update main wiring.
- Add TypeCategory enum (CopyType, MoveType) to classify types for ownership semantics - Implement Category(), IsCopyType(), IsMoveType() functions in types.go - Add OwnershipState and VarOwnership structs to track variable ownership metadata - Add warnings system to Checker for non-fatal ownership violations - Implement ownership tracking: trackOwnership(), markMoved(), pushOwnership(), popOwnership() - Add 4 ownership tests: copy assignment (no warnings), move assignment (warns on reuse), move arg (warns on reuse), move return (warns on reuse)
… drops
- Replace flat varTypes map with hierarchical cgenScope stack for proper scoping
- Add cgenVar struct to track variable metadata (CType, Mutable, Owned)
- Implement scope management: enterScope(), exitScope(), declareVar(), lookupVar()
- Add preamble buffer pattern for statement expressions (replaces GNU ({ }))
- Implement emitScopeDrops() for automatic cleanup of owned values at scope exit
- Add carv_string struct typedef with ownership tracking (data, len, owned flag)
- Implement string runtime helpers: carv_string_lit(), carv_string_own(), carv_string_clone(), carv_string_move(), carv_string_drop()
- Update carv_strdup to return carv_string instead of char*
- Add codegen tests for ownership semantics
- Print ownership warnings to stderr in run mode (non-fatal for interpreter) - Fail build/emit-c commands if ownership warnings are present (fatal for compilation) - Ensures ownership violations are caught before C code generation
📝 WalkthroughWalkthroughAdds ownership/borrowing, references, and interfaces across parser, AST, type checker, code generator, evaluator, tests, docs, examples, CLI warnings, and a new design document; also updates .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parser
participant Checker
participant CodeGen
participant Runner
User->>Parser: Submit source
Parser->>Parser: Parse AST (Borrow/Deref/Cast/Interface nodes)
Parser-->>Checker: AST
Checker->>Checker: Type-check, track ownership/borrows, record TypeInfo, collect Warnings
Checker-->>CodeGen: AST + TypeInfo (+ Warnings)
CodeGen->>CodeGen: SetTypeInfo, build scopes, emit ownership-aware C (arena, drops, vtables)
CodeGen-->>Runner: Generated C or interpreter-ready representation
Runner->>Runner: Execute or compile/run emitted C
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
Add interface/impl system with full codegen support: - Parser: interface, impl, cast (as) expressions, receiver grammar (&self/&mut self) - Checker: interface registration, impl validation, cast type checking - Codegen: vtable structs, fat pointers, wrapper functions, dynamic dispatch - 12 new tests across parser, checker, and codegen - Updated docs, examples, and README
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/codegen/cgen.go (1)
1319-1322:⚠️ Potential issue | 🟡 MinorRemove unused function
isArrayIdent.The CI pipeline correctly flags this function as unused. It should be removed to keep the codebase clean.
🧹 Proposed fix
-func (g *CGenerator) isArrayIdent(name string) bool { - _, ok := g.arrayLengths[name] - return ok -}
🤖 Fix all issues with AI agents
In `@docs/builtins.md`:
- Around line 249-255: Update the example in the docs for the parse_float
built-in: change the output comment for the call parse_float("2.0") from "// 2"
to "// 2.0" so the example consistently reflects that parse_float returns a
float; edit the example block under the parse_float description to adjust only
that comment.
- Around line 355-368: The docs show inconsistent syntax for the clone API: the
header uses a function form "clone(value) -> value" while the example uses
method form "original.clone()"; update the documentation to be explicit and
consistent by either changing the header to show the method form (e.g.,
"value.clone() -> value") to match the example, or document both forms (add a
brief note that both "clone(value)" and "value.clone()" are supported) and
provide matching examples for each (include both "let copy = original.clone();"
and "let copy = clone(original);" if both exist); ensure the symbol "clone" and
the example "original.clone()" are referenced so readers can find the correct
usage.
In `@docs/design-ownership-async-interfaces.md`:
- Around line 144-149: The fenced code block under the "Error Messages" section
containing the two lines "error at line 5: use of moved value 's'..." and "error
at line 8: use of moved value 'a'..." is missing a language specifier; update
that triple-backtick fence to include a language such as text or plaintext
(e.g., change ``` to ```text) so markdownlint will accept it and the block will
be treated as plain text.
In `@pkg/codegen/cgen.go`:
- Around line 93-95: checkerTypeToCString currently returns "carv_int" for
*types.MapType which produces invalid C; change the mapping to a real map
representation (e.g., return "carv_map" for *types.MapType in
checkerTypeToCString) and then either implement the corresponding C
representation and codegen paths for map operations or add a clear validation
failure where maps are encountered during code generation (produce an explicit
error rather than silently emitting carv_int). Ensure you update any downstream
codegen logic that expects the previous type string (search for uses of
checkerTypeToCString and any switch/emit code handling map-related operations)
so maps are either fully supported with the new "carv_map" or explicitly
rejected at generation time.
In `@pkg/eval/environment.go`:
- Around line 42-49: IsImmutable currently defers to outer scopes even when the
current scope defines a mutable binding, which incorrectly blocks shadowing;
update Environment.IsImmutable to first check for a mutable binding in the
current scope and return false if present, then check e.immutable for a true
value, and only if neither applies recurse to e.outer.IsImmutable(name). Use the
existing symbols (Environment.IsImmutable, e.immutable, e.outer) and the
current-scope binding/variables structure (the map/field that stores mutable
bindings in Environment) to detect shadowed mutable names before consulting
outer scopes.
In `@pkg/eval/eval_test.go`:
- Around line 934-956: The test TestBuiltinFileIO currently uses hard-coded /tmp
paths; change it to use t.TempDir() and filepath.Join to create an isolated file
path, then inject that path into the Carv program strings using fmt.Sprintf so
write_file/read_file/file_exists calls target the temp file; update the test
inputs (variables input, input2, input3) to use the generated path and keep
assertions the same, referencing TestBuiltinFileIO, testEval, write_file,
read_file, and file_exists to locate the code to modify.
In `@pkg/eval/eval.go`:
- Around line 768-810: The for-in branches for iterable types (case *Array,
*String, *Map) currently inspect Eval(node.Body, loopEnv) for RETURN_VALUE_OBJ,
ERROR_OBJ and BREAK_OBJ but never handle CONTINUE_OBJ, so continue statements
propagate incorrectly; update each branch to check result.Type() == CONTINUE_OBJ
and when seen simply continue the for-loop (skipping to the next element) while
preserving the existing checks for RETURN_VALUE_OBJ, ERROR_OBJ and BREAK_OBJ —
locate logic around Eval(node.Body, loopEnv) in the for-in handling and add a
CONTINUE_OBJ case before BREAK_OBJ handling.
In `@pkg/parser/parser.go`:
- Around line 559-563: Add an inline explanatory comment above the mutability
override so future readers understand the intent: explain that in
parseLetStatement() returns for C-style for-loop initializers (used in
stmt.Init) we intentionally set letStmt.Mutable = true because loop
post-expressions and typical C-style loop semantics expect reassignment of
initializer variables (e.g., i++/i += 1), so forcing mutability ensures those
patterns are allowed; place this comment adjacent to the letStmt.Mutable = true
line referencing parseLetStatement, letStmt, and stmt.Init.
In `@README.md`:
- Around line 50-54: The ownership example incorrectly calls s.clone() after s
has been moved to t; update the snippet so the clone is performed on a valid
binding (e.g., call clone on t or call s.clone() before assigning t) and assign
that result to u so that u receives a deep copy; reference the bindings s, t,
and u and the clone operation when making the change.
🧹 Nitpick comments (5)
.gitignore (1)
27-28: LGTM! The addition follows the existing formatting pattern.The
.sisyphusentry is properly placed in the OS section with appropriate spacing.One optional consideration: if
.sisyphusis a directory rather than a file, the pattern should be.sisyphus/to make the intent explicit and improve clarity.pkg/types/types.go (1)
188-227: Optional: lock in ownership classification for additional Type kinds.Category defaults unknown types to Copy. If
OptionalType,InterfaceType, orFunctionTypeshould be Move (or should follow inner type), consider adding explicit cases or tests to encode that intent.pkg/parser/parser.go (1)
938-955: Optional: preserve more accurate source positions for else-if blocks.The synthetic block in an
else ifchain currently uses the current token (often the closing brace) for position data. Consider capturing theelsetoken for clearer diagnostics.🔧 Possible tweak to preserve `else` position
if p.peekTokenIs(lexer.TOKEN_ELSE) { p.nextToken() // Support "else if" chains if p.peekTokenIs(lexer.TOKEN_IF) { + elseTok := p.curToken p.nextToken() nestedIf := p.parseIfExpression() if nestedIf == nil { return nil } expr.Alternative = &ast.BlockStatement{ - Token: p.curToken, - Statements: []ast.Statement{&ast.ExpressionStatement{Token: p.curToken, Expression: nestedIf}}, + Token: elseTok, + Statements: []ast.Statement{&ast.ExpressionStatement{Token: elseTok, Expression: nestedIf}}, } } else {pkg/types/checker.go (1)
331-336: Simplify redundant nil check (gosimple S1020).When
isRefis true on line 332,retTypeis guaranteed to be non-nil (already passed theretType != nilcheck on line 331). The static analyzer correctly flags this as redundant.♻️ Proposed fix
if retType != nil { - if _, isRef := retType.(*RefType); isRef { + if _, isRef := retType.(*RefType); isRef { line, col := s.ReturnValue.Pos() c.warning(line, col, "reference cannot escape function scope") } }Or simplify to:
- if retType != nil { - if _, isRef := retType.(*RefType); isRef { - line, col := s.ReturnValue.Pos() - c.warning(line, col, "reference cannot escape function scope") - } + if _, isRef := retType.(*RefType); isRef { + line, col := s.ReturnValue.Pos() + c.warning(line, col, "reference cannot escape function scope") }The type assertion
retType.(*RefType)will safely returnfalseforisRefifretTypeis nil, so the outer nil check is unnecessary.pkg/codegen/cgen.go (1)
137-149: Incomplete scope drop emission — only handlescarv_string.
emitScopeDrops()only emits drop calls forcarv_stringowned variables, but arrays (carv_*_array) and class instances are also owned types that may need cleanup. With the arena allocator this is currently a no-op, but the switch statement should be extended for consistency and future-proofing.♻️ Proposed enhancement
func (g *CGenerator) emitScopeDrops() { if g.scope == nil { return } for name, v := range g.scope.vars { if v.Owned { switch v.CType { case "carv_string": g.writeln(fmt.Sprintf("carv_string_drop(&%s);", name)) + // Arrays and other owned types use arena allocation + // No explicit drop needed currently, but interface exists for future use } } } }
| ### `parse_float(str) -> float` | ||
| Parse a string as a float. | ||
|
|
||
| ```carv | ||
| parse_float("3.14") // 3.14 | ||
| parse_float("2.0") // 2 | ||
| ``` |
There was a problem hiding this comment.
Minor inconsistency in example output comment.
The comment on line 254 shows // 2 for parse_float("2.0"), but since parse_float returns a float, the expected output should be // 2.0 to be consistent with float representation.
```carv
parse_float("3.14") // 3.14
-parse_float("2.0") // 2
+parse_float("2.0") // 2.0
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/builtins.md around lines 249 - 255, Update the example in the docs for
the parse_float built-in: change the output comment for the call
parse_float("2.0") from "// 2" to "// 2.0" so the example consistently reflects
that parse_float returns a float; edit the example block under the parse_float
description to adjust only that comment.
</details>
<!-- fingerprinting:phantom:medusa:ocelot -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ## Ownership | ||
|
|
||
| ### `clone(value) -> value` | ||
| Deep copy of any move type (string, array, map, or class instance). | ||
|
|
||
| ```carv | ||
| let original = "hello"; | ||
| let copy = original.clone(); | ||
| print(original); // OK: "hello" | ||
| print(copy); // OK: "hello" | ||
|
|
||
| let arr = [1, 2, 3]; | ||
| let arr_copy = arr.clone(); | ||
| ``` |
There was a problem hiding this comment.
Inconsistent clone() syntax between header and example.
The header declares clone(value) -> value as a function call, but the example uses method syntax original.clone(). Please clarify which syntax is canonical, or document both if both are supported.
// Header shows function syntax:
clone(value) -> value
// But example uses method syntax:
let copy = original.clone();
🤖 Prompt for AI Agents
In `@docs/builtins.md` around lines 355 - 368, The docs show inconsistent syntax
for the clone API: the header uses a function form "clone(value) -> value" while
the example uses method form "original.clone()"; update the documentation to be
explicit and consistent by either changing the header to show the method form
(e.g., "value.clone() -> value") to match the example, or document both forms
(add a brief note that both "clone(value)" and "value.clone()" are supported)
and provide matching examples for each (include both "let copy =
original.clone();" and "let copy = clone(original);" if both exist); ensure the
symbol "clone" and the example "original.clone()" are referenced so readers can
find the correct usage.
| ### Error Messages | ||
|
|
||
| ``` | ||
| error at line 5: use of moved value 's' (moved at line 3) | ||
| error at line 8: use of moved value 'a' (moved into function 'consume' at line 7) | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The error message example at line 146 is missing a language specifier, which is flagged by markdownlint. Consider adding text or plaintext as the language.
-```
+```text
error at line 5: use of moved value 's' (moved at line 3)
error at line 8: use of moved value 'a' (moved into function 'consume' at line 7)
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.20.0)</summary>
[warning] 146-146: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @docs/design-ownership-async-interfaces.md around lines 144 - 149, The fenced
code block under the "Error Messages" section containing the two lines "error at
line 5: use of moved value 's'..." and "error at line 8: use of moved value
'a'..." is missing a language specifier; update that triple-backtick fence to
include a language such as text or plaintext (e.g., change totext) so
markdownlint will accept it and the block will be treated as plain text.
</details>
<!-- fingerprinting:phantom:medusa:ocelot -->
<!-- This is an auto-generated comment by CodeRabbit -->
| if _, ok := t.(*types.MapType); ok { | ||
| return "carv_int" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a carv_map type defined
rg -n "carv_map" pkg/codegen/Repository: dev-dami/carv
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check how maps are used in tests
rg -n "MapLiteral" pkg/codegen/cgen_test.goRepository: dev-dami/carv
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Get context around the flagged lines to understand the function better
sed -n '80,110p' pkg/codegen/cgen.goRepository: dev-dami/carv
Length of output: 714
🏁 Script executed:
#!/bin/bash
# Search for other type mappings to understand the pattern
rg -n "checkerTypeToCString" pkg/codegen/cgen.go | head -20Repository: dev-dami/carv
Length of output: 254
🏁 Script executed:
#!/bin/bash
# Search for MapType usage across the codebase
rg -n "MapType" pkg/Repository: dev-dami/carv
Length of output: 973
🏁 Script executed:
#!/bin/bash
# Check if there are any map-related tests or code in the repository
rg -n "map\|Map" pkg/codegen/cgen_test.go | head -30Repository: dev-dami/carv
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about maps in the codegen package
rg -n "map" pkg/codegen/cgen.go | head -20Repository: dev-dami/carv
Length of output: 471
🏁 Script executed:
#!/bin/bash
# Check the types package to see how MapType is defined
find pkg -name "*.go" -type f | xargs grep -l "type MapType"Repository: dev-dami/carv
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for MapLiteral usage across the entire codebase
rg -n "MapLiteral" .Repository: dev-dami/carv
Length of output: 1300
🏁 Script executed:
#!/bin/bash
# Search for maps in test files to see if they're actually tested
find . -name "*test.go" -type f | xargs rg -l "map\|Map" | head -5Repository: dev-dami/carv
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or TODOs about maps in the codegen
rg -n -i "todo|fixme|hack.*map" pkg/codegen/Repository: dev-dami/carv
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Look at the full checkerTypeToCString function to understand its purpose
sed -n '59,115p' pkg/codegen/cgen.goRepository: dev-dami/carv
Length of output: 1357
🏁 Script executed:
#!/bin/bash
# Check if there are any codegen tests that use maps
rg -n "MapLiteral\|MapType" pkg/codegen/cgen_test.goRepository: dev-dami/carv
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check the entire cgen_test.go file to see what types ARE tested
grep -n "test\|Test" pkg/codegen/cgen_test.go | head -20Repository: dev-dami/carv
Length of output: 874
🏁 Script executed:
#!/bin/bash
# Search for any map variable declarations in codegen tests
rg -n "map\[" pkg/codegen/cgen_test.goRepository: dev-dami/carv
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Check if there's any error handling or rejection of maps in codegen
rg -n "Map" pkg/codegen/cgen.goRepository: dev-dami/carv
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check the full test file to verify there's no map test somewhere else
wc -l pkg/codegen/cgen_test.goRepository: dev-dami/carv
Length of output: 85
Map type mapping to carv_int needs proper C representation.
checkerTypeToCString maps *types.MapType to "carv_int", which is incorrect. Maps are fully supported in parsing, type checking, and evaluation (with dedicated tests), but C code generation lacks a proper type mapping. This causes maps to silently compile as carv_int, generating incorrect C code. Either implement a proper map representation (e.g., carv_map) or add validation to reject maps at code generation time.
🤖 Prompt for AI Agents
In `@pkg/codegen/cgen.go` around lines 93 - 95, checkerTypeToCString currently
returns "carv_int" for *types.MapType which produces invalid C; change the
mapping to a real map representation (e.g., return "carv_map" for *types.MapType
in checkerTypeToCString) and then either implement the corresponding C
representation and codegen paths for map operations or add a clear validation
failure where maps are encountered during code generation (produce an explicit
error rather than silently emitting carv_int). Ensure you update any downstream
codegen logic that expects the previous type string (search for uses of
checkerTypeToCString and any switch/emit code handling map-related operations)
so maps are either fully supported with the new "carv_map" or explicitly
rejected at generation time.
| func (e *Environment) IsImmutable(name string) bool { | ||
| if imm, ok := e.immutable[name]; ok { | ||
| return imm | ||
| } | ||
| if e.outer != nil { | ||
| return e.outer.IsImmutable(name) | ||
| } | ||
| return false |
There was a problem hiding this comment.
Fix IsImmutable to respect shadowed mutable bindings.
If a mutable name exists in the current scope, IsImmutable should return false instead of deferring to an outer immutable binding. Otherwise, shadowing an immutable with a mutable binding is incorrectly blocked.
🛠️ Suggested fix
func (e *Environment) IsImmutable(name string) bool {
- if imm, ok := e.immutable[name]; ok {
- return imm
- }
+ if _, inScope := e.store[name]; inScope {
+ return e.immutable[name]
+ }
if e.outer != nil {
return e.outer.IsImmutable(name)
}
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (e *Environment) IsImmutable(name string) bool { | |
| if imm, ok := e.immutable[name]; ok { | |
| return imm | |
| } | |
| if e.outer != nil { | |
| return e.outer.IsImmutable(name) | |
| } | |
| return false | |
| func (e *Environment) IsImmutable(name string) bool { | |
| if _, inScope := e.store[name]; inScope { | |
| return e.immutable[name] | |
| } | |
| if e.outer != nil { | |
| return e.outer.IsImmutable(name) | |
| } | |
| return false | |
| } |
🤖 Prompt for AI Agents
In `@pkg/eval/environment.go` around lines 42 - 49, IsImmutable currently defers
to outer scopes even when the current scope defines a mutable binding, which
incorrectly blocks shadowing; update Environment.IsImmutable to first check for
a mutable binding in the current scope and return false if present, then check
e.immutable for a true value, and only if neither applies recurse to
e.outer.IsImmutable(name). Use the existing symbols (Environment.IsImmutable,
e.immutable, e.outer) and the current-scope binding/variables structure (the
map/field that stores mutable bindings in Environment) to detect shadowed
mutable names before consulting outer scopes.
| func TestBuiltinFileIO(t *testing.T) { | ||
| input := ` | ||
| write_file("/tmp/carv_test_builtin.txt", "hello carv"); | ||
| let content = read_file("/tmp/carv_test_builtin.txt"); | ||
| content; | ||
| ` | ||
| evaluated := testEval(input) | ||
| str, ok := evaluated.(*String) | ||
| if !ok { | ||
| t.Fatalf("expected String, got %T (%s)", evaluated, evaluated.Inspect()) | ||
| } | ||
| if str.Value != "hello carv" { | ||
| t.Errorf("expected 'hello carv', got %q", str.Value) | ||
| } | ||
|
|
||
| input2 := `file_exists("/tmp/carv_test_builtin.txt");` | ||
| evaluated2 := testEval(input2) | ||
| testBooleanObject(t, evaluated2, true) | ||
|
|
||
| input3 := `file_exists("/tmp/nonexistent_carv_test_file_xyz.txt");` | ||
| evaluated3 := testEval(input3) | ||
| testBooleanObject(t, evaluated3, false) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the file exists and check the current implementation
fd eval_test.goRepository: dev-dami/carv
Length of output: 78
🏁 Script executed:
# Check the actual content of the test file around the specified lines
cat -n pkg/eval/eval_test.go | sed -n '930,960p'Repository: dev-dami/carv
Length of output: 1134
🏁 Script executed:
# Look at the testEval function to understand its signature
rg -A 10 "func testEval" pkg/eval/eval_test.goRepository: dev-dami/carv
Length of output: 332
🏁 Script executed:
# Check if there are imports already in the file
head -30 pkg/eval/eval_test.goRepository: dev-dami/carv
Length of output: 591
🏁 Script executed:
# Check for other similar file I/O tests in the codebase
rg -n "TempDir|/tmp" pkg/eval/Repository: dev-dami/carv
Length of output: 396
🏁 Script executed:
# Check for other uses of fmt.Sprintf in test files
rg -n "fmt.Sprintf" pkg/eval/eval_test.goRepository: dev-dami/carv
Length of output: 39
🏁 Script executed:
# Check if fmt is already imported in other test files in the eval package
rg -A 5 "^import" pkg/eval/ | grep -E "(fmt|import)"Repository: dev-dami/carv
Length of output: 225
🏁 Script executed:
# Check for other similar tests that might use temporary paths or dynamic code construction
rg -B 2 -A 10 "testEval\(" pkg/eval/eval_test.go | head -50Repository: dev-dami/carv
Length of output: 906
Use temporary directories instead of hard-coded /tmp paths for test isolation.
Hard-coded /tmp paths are non-portable and can collide in parallel test runs. Use t.TempDir() with filepath.Join() to generate per-test isolation, then inject the path into the Carv program via fmt.Sprintf().
🛠️ Suggested update
import (
+ "fmt"
+ "path/filepath"
"testing"
"github.com/dev-dami/carv/pkg/lexer"
"github.com/dev-dami/carv/pkg/parser"
) func TestBuiltinFileIO(t *testing.T) {
+ tmpDir := t.TempDir()
+ tmpFile := filepath.Join(tmpDir, "carv_test_builtin.txt")
+
- input := `
-write_file("/tmp/carv_test_builtin.txt", "hello carv");
-let content = read_file("/tmp/carv_test_builtin.txt");
+ input := fmt.Sprintf(`
+write_file(%q, "hello carv");
+let content = read_file(%q);
content;
-`
+`, tmpFile, tmpFile)
evaluated := testEval(input)
str, ok := evaluated.(*String)
if !ok {
t.Fatalf("expected String, got %T (%s)", evaluated, evaluated.Inspect())
}
if str.Value != "hello carv" {
t.Errorf("expected 'hello carv', got %q", str.Value)
}
- input2 := `file_exists("/tmp/carv_test_builtin.txt");`
+ input2 := fmt.Sprintf(`file_exists(%q);`, tmpFile)
evaluated2 := testEval(input2)
testBooleanObject(t, evaluated2, true)
- input3 := `file_exists("/tmp/nonexistent_carv_test_file_xyz.txt");`
+ missing := filepath.Join(tmpDir, "nonexistent_carv_test_file_xyz.txt")
+ input3 := fmt.Sprintf(`file_exists(%q);`, missing)
evaluated3 := testEval(input3)
testBooleanObject(t, evaluated3, false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestBuiltinFileIO(t *testing.T) { | |
| input := ` | |
| write_file("/tmp/carv_test_builtin.txt", "hello carv"); | |
| let content = read_file("/tmp/carv_test_builtin.txt"); | |
| content; | |
| ` | |
| evaluated := testEval(input) | |
| str, ok := evaluated.(*String) | |
| if !ok { | |
| t.Fatalf("expected String, got %T (%s)", evaluated, evaluated.Inspect()) | |
| } | |
| if str.Value != "hello carv" { | |
| t.Errorf("expected 'hello carv', got %q", str.Value) | |
| } | |
| input2 := `file_exists("/tmp/carv_test_builtin.txt");` | |
| evaluated2 := testEval(input2) | |
| testBooleanObject(t, evaluated2, true) | |
| input3 := `file_exists("/tmp/nonexistent_carv_test_file_xyz.txt");` | |
| evaluated3 := testEval(input3) | |
| testBooleanObject(t, evaluated3, false) | |
| } | |
| func TestBuiltinFileIO(t *testing.T) { | |
| tmpDir := t.TempDir() | |
| tmpFile := filepath.Join(tmpDir, "carv_test_builtin.txt") | |
| input := fmt.Sprintf(` | |
| write_file(%q, "hello carv"); | |
| let content = read_file(%q); | |
| content; | |
| `, tmpFile, tmpFile) | |
| evaluated := testEval(input) | |
| str, ok := evaluated.(*String) | |
| if !ok { | |
| t.Fatalf("expected String, got %T (%s)", evaluated, evaluated.Inspect()) | |
| } | |
| if str.Value != "hello carv" { | |
| t.Errorf("expected 'hello carv', got %q", str.Value) | |
| } | |
| input2 := fmt.Sprintf(`file_exists(%q);`, tmpFile) | |
| evaluated2 := testEval(input2) | |
| testBooleanObject(t, evaluated2, true) | |
| missing := filepath.Join(tmpDir, "nonexistent_carv_test_file_xyz.txt") | |
| input3 := fmt.Sprintf(`file_exists(%q);`, missing) | |
| evaluated3 := testEval(input3) | |
| testBooleanObject(t, evaluated3, false) | |
| } |
🤖 Prompt for AI Agents
In `@pkg/eval/eval_test.go` around lines 934 - 956, The test TestBuiltinFileIO
currently uses hard-coded /tmp paths; change it to use t.TempDir() and
filepath.Join to create an isolated file path, then inject that path into the
Carv program strings using fmt.Sprintf so write_file/read_file/file_exists calls
target the temp file; update the test inputs (variables input, input2, input3)
to use the generated path and keep assertions the same, referencing
TestBuiltinFileIO, testEval, write_file, read_file, and file_exists to locate
the code to modify.
| switch obj := iterable.(type) { | ||
| case *Array: | ||
| for _, elem := range obj.Elements { | ||
| loopEnv.Set(node.Value.Value, elem) | ||
| result := Eval(node.Body, loopEnv) | ||
| if result != nil { | ||
| if result.Type() == RETURN_VALUE_OBJ || result.Type() == ERROR_OBJ { | ||
| return result | ||
| } | ||
| if result.Type() == BREAK_OBJ { | ||
| break | ||
| } | ||
| } | ||
| if result.Type() == BREAK_OBJ { | ||
| break | ||
| } | ||
| case *String: | ||
| for _, ch := range obj.Value { | ||
| loopEnv.Set(node.Value.Value, &Char{Value: ch}) | ||
| result := Eval(node.Body, loopEnv) | ||
| if result != nil { | ||
| if result.Type() == RETURN_VALUE_OBJ || result.Type() == ERROR_OBJ { | ||
| return result | ||
| } | ||
| if result.Type() == BREAK_OBJ { | ||
| break | ||
| } | ||
| } | ||
| } | ||
| case *Map: | ||
| for _, pair := range obj.Pairs { | ||
| loopEnv.Set(node.Value.Value, pair.Key) | ||
| result := Eval(node.Body, loopEnv) | ||
| if result != nil { | ||
| if result.Type() == RETURN_VALUE_OBJ || result.Type() == ERROR_OBJ { | ||
| return result | ||
| } | ||
| if result.Type() == BREAK_OBJ { | ||
| break | ||
| } | ||
| } | ||
| } | ||
| default: | ||
| return &Error{Message: "for-in requires array, string, or map, got " + string(iterable.Type())} | ||
| } |
There was a problem hiding this comment.
Missing CONTINUE handling in for-in loops.
The for-in iteration branches handle BREAK_OBJ but don't handle CONTINUE_OBJ. This means continue statements inside for-in loops won't work correctly — they'll propagate up instead of skipping to the next iteration.
Compare with evalForStatement (lines 739-747) which only checks for RETURN_VALUE_OBJ, ERROR_OBJ, and BREAK_OBJ but not CONTINUE_OBJ. However, the C-style for loop naturally continues via its post-increment, whereas for-in requires explicit handling.
🐛 Proposed fix to handle CONTINUE
case *Array:
for _, elem := range obj.Elements {
loopEnv.Set(node.Value.Value, elem)
result := Eval(node.Body, loopEnv)
if result != nil {
if result.Type() == RETURN_VALUE_OBJ || result.Type() == ERROR_OBJ {
return result
}
if result.Type() == BREAK_OBJ {
break
}
+ if result.Type() == CONTINUE_OBJ {
+ continue
+ }
}
}Apply the same pattern to the *String and *Map branches.
🤖 Prompt for AI Agents
In `@pkg/eval/eval.go` around lines 768 - 810, The for-in branches for iterable
types (case *Array, *String, *Map) currently inspect Eval(node.Body, loopEnv)
for RETURN_VALUE_OBJ, ERROR_OBJ and BREAK_OBJ but never handle CONTINUE_OBJ, so
continue statements propagate incorrectly; update each branch to check
result.Type() == CONTINUE_OBJ and when seen simply continue the for-loop
(skipping to the next element) while preserving the existing checks for
RETURN_VALUE_OBJ, ERROR_OBJ and BREAK_OBJ — locate logic around Eval(node.Body,
loopEnv) in the for-in handling and add a CONTINUE_OBJ case before BREAK_OBJ
handling.
| letStmt := p.parseLetStatement() | ||
| if letStmt != nil { | ||
| letStmt.Mutable = true | ||
| } | ||
| stmt.Init = letStmt |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "parser.go" -type f 2>/dev/null | head -5Repository: dev-dami/carv
Length of output: 39
🏁 Script executed:
# First, let's look at the broader context of the parseForStatement function
grep -n "parseForStatement" pkg/parser/parser.goRepository: dev-dami/carv
Length of output: 148
🏁 Script executed:
# Get context around lines 559-563 to understand the parseForStatement function
sed -n '520,600p' pkg/parser/parser.goRepository: dev-dami/carv
Length of output: 1926
🏁 Script executed:
# Search for how Mutable is used elsewhere in the codebase
rg "\.Mutable\s*=" --context 3Repository: dev-dami/carv
Length of output: 1933
🏁 Script executed:
# Check for tests related to for-loop initialization
rg -A 5 -B 5 "for.*let" --type go | head -50Repository: dev-dami/carv
Length of output: 2101
🏁 Script executed:
# Look for documentation or comments about let/mut semantics
rg -i "let.*mutable|mutable.*let" --type goRepository: dev-dami/carv
Length of output: 458
🏁 Script executed:
# Look at the exact context around lines 559-563 with comments
sed -n '545,575p' pkg/parser/parser.go | cat -nRepository: dev-dami/carv
Length of output: 1056
🏁 Script executed:
# Check the full parseCStyleFor function for any comments explaining this behavior
sed -n '513,600p' pkg/parser/parser.go | cat -nRepository: dev-dami/carv
Length of output: 2759
🏁 Script executed:
# Look at the test more carefully to understand the rationale
sed -n '770,800p' pkg/parser/parser_test.go | cat -nRepository: dev-dami/carv
Length of output: 39
🏁 Script executed:
grep -n "TestForLoopInitMutable" pkg/parser/parser_test.goRepository: dev-dami/carv
Length of output: 104
🏁 Script executed:
# Get the full test function
sed -n '780,800p' pkg/parser/parser_test.go | cat -nRepository: dev-dami/carv
Length of output: 39
🏁 Script executed:
sed -n '420,435p' pkg/parser/parser_test.goRepository: dev-dami/carv
Length of output: 523
Add explanatory comment for for-loop init mutability override.
The code unconditionally forces for-loop initializers to be mutable (line 559), which is confirmed as intentional by the test suite. However, the code lacks an inline comment explaining the design rationale. Add a comment clarifying why C-style for-loop let declarations must be forced mutable (e.g., due to reassignment patterns in loop post-expressions).
🤖 Prompt for AI Agents
In `@pkg/parser/parser.go` around lines 559 - 563, Add an inline explanatory
comment above the mutability override so future readers understand the intent:
explain that in parseLetStatement() returns for C-style for-loop initializers
(used in stmt.Init) we intentionally set letStmt.Mutable = true because loop
post-expressions and typical C-style loop semantics expect reassignment of
initializer variables (e.g., i++/i += 1), so forcing mutability ensures those
patterns are allowed; place this comment adjacent to the letStmt.Mutable = true
line referencing parseLetStatement, letStmt, and stmt.Init.
| // ownership: move semantics | ||
| let s = "hello"; | ||
| let t = s; // s is moved, now invalid | ||
| let u = s.clone(); // explicit deep copy | ||
|
|
There was a problem hiding this comment.
Fix moved-value usage in the ownership quick-look example.
Line 53 calls s.clone() after s has been moved on Line 52, which should be invalid. Clone the valid binding instead (or clone before the move).
✅ Suggested fix
let s = "hello";
let t = s; // s is moved, now invalid
-let u = s.clone(); // explicit deep copy
+let u = t.clone(); // explicit deep copy🤖 Prompt for AI Agents
In `@README.md` around lines 50 - 54, The ownership example incorrectly calls
s.clone() after s has been moved to t; update the snippet so the clone is
performed on a valid binding (e.g., call clone on t or call s.clone() before
assigning t) and assign that result to u so that u receives a deep copy;
reference the bindings s, t, and u and the clone operation when making the
change.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/codegen/cgen.go (2)
918-958:⚠️ Potential issue | 🟠 MajorRegister loop variables in scope before generating header/body
forinit variables andfor-initer variables are emitted but never declared incgenScope. This makesresolveType/getVarTypefall back to defaults in conditions/body and skips owned drops. Enter the scope before header generation and declare the loop variables.💡 Suggested fix
func (g *CGenerator) generateForStatement(s *ast.ForStatement) { - init := "" + g.enterScope() + init := "" if s.Init != nil { if let, ok := s.Init.(*ast.LetStatement); ok { varType := g.inferType(let.Value) value := g.generateExpression(let.Value) init = fmt.Sprintf("%s %s = %s", varType, let.Name.Value, value) + owned := varType == "carv_string" || strings.HasSuffix(varType, "_array") || strings.HasSuffix(varType, "*") + g.declareVar(let.Name.Value, varType, let.Mutable, owned) } } g.flushPreamble() @@ - g.enterScope() + // scope already entered before header generation @@ - g.exitScope() + g.exitScope() } @@ g.writeln(fmt.Sprintf("for (carv_int %s = 0; %s < %s.len; %s++) {", idxVar, idxVar, iterableExpr, idxVar)) g.indent++ g.enterScope() elemType := g.inferArrayElemType(s.Iterable) + g.declareVar(iterName, elemType, false, false) g.writeln(fmt.Sprintf("%s %s = %s.data[%s];", elemType, iterName, iterableExpr, idxVar)) @@ g.exitScope()Also applies to: 960-979
1378-1381:⚠️ Potential issue | 🟡 MinorRemove unused
isArrayIdenthelperThe
isArrayIdentfunction is unused and blocks lint. Remove it:-func (g *CGenerator) isArrayIdent(name string) bool { - _, ok := g.arrayLengths[name] - return ok -}docs/architecture.md (1)
123-135:⚠️ Potential issue | 🟡 MinorFix duplicate numbering in “Future Plans”
The list repeats item “6.” (“Async/await — Next”), which breaks the sequence. Consider renumbering to keep it consistent.
✏️ Suggested edit
-6. Async/await — Next +9. Async/await — Next
🤖 Fix all issues with AI agents
In `@docs/design-ownership-async-interfaces.md`:
- Around line 366-372: The ReceiverKind constants in the doc are out of sync
with the implementation: replace RecvSelf with RecvValue and add the missing
RecvNone constant to match the code; update the constant block for type
ReceiverKind so it defines RecvNone, RecvValue, RecvRef, and RecvMutRef
(matching the symbols used in pkg/ast/statements.go) and keep the same iota
ordering as the implementation.
In `@pkg/codegen/cgen.go`:
- Around line 2037-2053: The fat-pointer construction in
CGenerator.generateCastExpression incorrectly uses the full borrow expression
string (val) so casting expressions like (&p as &Printable) produce data = &p (a
Person**), not a Person*; detect when e.Value is a borrow (e.g., an
ast.BorrowExpression) and use the borrow's inner expression (or dereference
once) when building the .data field instead of the whole borrow/paren
expression, preserving Mutable handling and using inferCastSourceClass as before
so the generated fat pointer points at the actual Person instance rather than
the stack slot holding the pointer.
In `@pkg/parser/parser_test.go`:
- Around line 754-775: The test shadows the `ok` variable across multiple type
assertions (e.g., `cast, ok :=
program.Statements[0].(*ast.ExpressionStatement)`, then `cast, ok := ...` etc.);
fix it by declaring/using a single `ok` or distinct names instead of repeatedly
using `:=` and shadowing: keep the initial `cast, ok :=
stmt.Expression.(*ast.CastExpression)` (or declare `var ok bool` before) and
change subsequent `:=` to `=` (or rename to `okBorrow`, `okRef`, `okNamed`) for
the assertions on `BorrowExpression`, `RefType`, and `NamedType` so no `ok` is
shadowed (references: stmt, cast, ref, named, and the type assertion uses of
ok).
In `@pkg/types/checker.go`:
- Around line 873-883: The method parameter and receiver handling in checker.go
defines self and parameters via c.scope.Define("self", classType) and
c.scope.Define(p.Name.Value, paramTypes[i]) but never calls trackOwnership, so
move-only values aren’t tracked; update the method-processing block (and the
similar block at the other location) to call c.trackOwnership on the resolved
type for self and for each parameter after you compute paramTypes[i] (use Any
when p.Type is nil via resolveTypeExpr/Any) and adjust to account for
receiver-kind if borrowed semantics are required (i.e., don’t transfer ownership
for borrowed receivers).
- Around line 641-675: The current isVariadicFunction list incorrectly treats
substr/exit/panic as variadic, skipping arity validation and allowing invalid
calls; update isVariadicFunction to only return true for true variadics (e.g.,
"print", "println", "exec", "exec_output"), and add explicit arity checks in the
caller (the code that does the len(e.Arguments) != len(ft.Params) check inside
Checker.checkExpression for CallExpression) to enforce: substr must have 2 or 3
arguments, and exit and panic must have exactly 1 argument (emit the same
c.error format used elsewhere). Reference functions/values: isVariadicFunction,
CallExpression e, ft.Params, substr, exit, panic, print, println, exec,
exec_output.
🧹 Nitpick comments (5)
examples/showcase.carv (2)
96-98: Misleading function name:print_lendoesn't print.The function only returns the length but doesn't print anything. Consider renaming to
get_lenorstring_lenfor clarity, especially in a showcase file meant to educate users.✏️ Suggested rename
-fn print_len(s: &string) -> int { +fn get_len(s: &string) -> int { return len(s); }And update the call site accordingly:
-let n = print_len(&msg); +let n = get_len(&msg);
109-135: Duplicate method definitions:greetis defined twice per class.The
greet(&self)method is defined both inside the class body (lines 112-114, 120-122) and again in theimpl Greeter for ...blocks (lines 126-128, 132-134) with identical implementations. This is redundant.Typically, interface implementations either:
- Define methods only in the
implblock (Rust-style), or- Have the class methods automatically satisfy the interface without re-declaration
Also, the
namefield in both classes is declared but never used.♻️ Option 1: Remove methods from class bodies, keep only impl blocks
class English { - name: string = "English" - - fn greet(&self) -> string { - return "Hello!"; - } } class Spanish { - name: string = "Spanish" - - fn greet(&self) -> string { - return "Hola!"; - } }♻️ Option 2: Remove impl blocks if the language auto-satisfies interfaces
-impl Greeter for English { - fn greet(&self) -> string { - return "Hello!"; - } -} - -impl Greeter for Spanish { - fn greet(&self) -> string { - return "Hola!"; - } -}Please clarify the intended semantics for Carv: should interface implementations be declared via
implblocks, or do class methods automatically satisfy matching interface signatures?docs/language.md (1)
207-213: Consider clarifying string assignment in interface example.Per the ownership model, strings are move types. Line 209 assigns a string literal directly. This works because string literals are implicitly owned, but for consistency with other examples that use
.clone(), consider adding a brief note or using"Alice".clone()for pedagogical clarity.pkg/parser/parser_test.go (1)
708-744: Consider adding test for impl methods without explicit receiver.The current tests only cover methods with
&selfreceivers. Consider adding a test case for methods that have no receiver (just regular parameters) to ensure the parameter parsing works correctly in that edge case.func TestImplMethodWithoutReceiver(t *testing.T) { input := `impl Helper for Util { fn compute(x: int, y: int) -> int { return x + y; } }` // ... verify parsing produces correct params }pkg/parser/parser.go (1)
1428-1436: Make parameter parsing consistent with receiver-handled cases.The code works correctly but is inconsistent with how receiver cases handle
parseRemainingParams(). When a method has no receiver and only regular params,parseRemainingParams()is called unconditionally at line 1432 without first checking forTOKEN_COMMA. In contrast, all three receiver-detected cases (lines 1392–1396, 1405–1408, 1417–1420) check forTOKEN_COMMAbefore callingparseRemainingParams()and explicitly advance tokens.While the current code functions due to graceful nil handling in the loop, it's wasteful and confusing. Align the non-receiver case with the established pattern for consistency.
🔧 Suggested refactor
param := p.parseParameter() if param != nil { params = append(params, param) } - params = append(params, p.parseRemainingParams()...) + if p.peekTokenIs(lexer.TOKEN_COMMA) { + p.nextToken() + p.nextToken() + params = append(params, p.parseRemainingParams()...) + } if !p.expectPeek(lexer.TOKEN_RPAREN) { return recv, nil }
| type ReceiverKind int | ||
| const ( | ||
| RecvSelf ReceiverKind = iota // self (consumes) | ||
| RecvRef // &self | ||
| RecvMutRef // &mut self | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Update ReceiverKind constants to match implementation.
The design document shows RecvSelf but the actual implementation in pkg/ast/statements.go uses RecvValue (and includes RecvNone for standalone functions). Update for consistency.
type ReceiverKind int
const (
- RecvSelf ReceiverKind = iota // self (consumes)
- RecvRef // &self
- RecvMutRef // &mut self
+ RecvNone ReceiverKind = iota // no receiver (standalone function)
+ RecvValue // self (consumes)
+ RecvRef // &self
+ RecvMutRef // &mut self
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ReceiverKind int | |
| const ( | |
| RecvSelf ReceiverKind = iota // self (consumes) | |
| RecvRef // &self | |
| RecvMutRef // &mut self | |
| ) | |
| ``` | |
| type ReceiverKind int | |
| const ( | |
| RecvNone ReceiverKind = iota // no receiver (standalone function) | |
| RecvValue // self (consumes) | |
| RecvRef // &self | |
| RecvMutRef // &mut self | |
| ) |
🤖 Prompt for AI Agents
In `@docs/design-ownership-async-interfaces.md` around lines 366 - 372, The
ReceiverKind constants in the doc are out of sync with the implementation:
replace RecvSelf with RecvValue and add the missing RecvNone constant to match
the code; update the constant block for type ReceiverKind so it defines
RecvNone, RecvValue, RecvRef, and RecvMutRef (matching the symbols used in
pkg/ast/statements.go) and keep the same iota ordering as the implementation.
| func (g *CGenerator) generateCastExpression(e *ast.CastExpression) string { | ||
| val := g.generateExpression(e.Value) | ||
|
|
||
| if refType, ok := e.Type.(*ast.RefType); ok { | ||
| if named, ok := refType.Inner.(*ast.NamedType); ok { | ||
| ifaceName := named.Name.Value | ||
| if _, isIface := g.interfaces[ifaceName]; isIface { | ||
| className := g.inferCastSourceClass(e.Value) | ||
| if className != "" { | ||
| if refType.Mutable { | ||
| return fmt.Sprintf("(%s_mut_ref){ .data = %s, .vt = &%s__%s__VT }", | ||
| ifaceName, val, ifaceName, className) | ||
| } | ||
| return fmt.Sprintf("(%s_ref){ .data = %s, .vt = &%s__%s__VT }", | ||
| ifaceName, val, ifaceName, className) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid storing &ptr in interface fat pointers
When casting &p as &Printable, val becomes (&p). The wrapper casts self to Person*, so data must point to the Person, not the stack slot holding p. Otherwise a Person** is reinterpreted as Person*, which is unsafe. Consider using the borrow’s inner expression (or dereferencing once) when building the fat pointer.
💡 Suggested fix
func (g *CGenerator) generateCastExpression(e *ast.CastExpression) string {
val := g.generateExpression(e.Value)
+ dataExpr := val
+ if borrow, ok := e.Value.(*ast.BorrowExpression); ok {
+ dataExpr = g.generateExpression(borrow.Value)
+ }
@@
- return fmt.Sprintf("(%s_mut_ref){ .data = %s, .vt = &%s__%s__VT }",
- ifaceName, val, ifaceName, className)
+ return fmt.Sprintf("(%s_mut_ref){ .data = %s, .vt = &%s__%s__VT }",
+ ifaceName, dataExpr, ifaceName, className)
}
- return fmt.Sprintf("(%s_ref){ .data = %s, .vt = &%s__%s__VT }",
- ifaceName, val, ifaceName, className)
+ return fmt.Sprintf("(%s_ref){ .data = %s, .vt = &%s__%s__VT }",
+ ifaceName, dataExpr, ifaceName, className)
}
}
}🤖 Prompt for AI Agents
In `@pkg/codegen/cgen.go` around lines 2037 - 2053, The fat-pointer construction
in CGenerator.generateCastExpression incorrectly uses the full borrow expression
string (val) so casting expressions like (&p as &Printable) produce data = &p (a
Person**), not a Person*; detect when e.Value is a borrow (e.g., an
ast.BorrowExpression) and use the borrow's inner expression (or dereference
once) when building the .data field instead of the whole borrow/paren
expression, preserving Mutable handling and using inferCastSourceClass as before
so the generated fat pointer points at the actual Person instance rather than
the stack slot holding the pointer.
| stmt := program.Statements[0].(*ast.ExpressionStatement) | ||
| cast, ok := stmt.Expression.(*ast.CastExpression) | ||
| if !ok { | ||
| t.Fatalf("expected CastExpression, got %T", stmt.Expression) | ||
| } | ||
|
|
||
| if _, ok := cast.Value.(*ast.BorrowExpression); !ok { | ||
| t.Fatalf("expected BorrowExpression as cast value, got %T", cast.Value) | ||
| } | ||
|
|
||
| ref, ok := cast.Type.(*ast.RefType) | ||
| if !ok { | ||
| t.Fatalf("expected RefType as cast target, got %T", cast.Type) | ||
| } | ||
| named, ok := ref.Inner.(*ast.NamedType) | ||
| if !ok { | ||
| t.Fatalf("expected NamedType inside RefType, got %T", ref.Inner) | ||
| } | ||
| if named.Name.Value != "Printable" { | ||
| t.Fatalf("expected 'Printable', got %s", named.Name.Value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix shadowed variable declaration.
Static analysis flags that ok declared at line 760 shadows the declaration at line 755. While this doesn't affect test correctness, it's a code smell.
♻️ Suggested fix
stmt := program.Statements[0].(*ast.ExpressionStatement)
cast, ok := stmt.Expression.(*ast.CastExpression)
if !ok {
t.Fatalf("expected CastExpression, got %T", stmt.Expression)
}
- if _, ok := cast.Value.(*ast.BorrowExpression); !ok {
+ if _, isBorrow := cast.Value.(*ast.BorrowExpression); !isBorrow {
t.Fatalf("expected BorrowExpression as cast value, got %T", cast.Value)
}
- ref, ok := cast.Type.(*ast.RefType)
+ ref, isRef := cast.Type.(*ast.RefType)
- if !ok {
+ if !isRef {
t.Fatalf("expected RefType as cast target, got %T", cast.Type)
}
- named, ok := ref.Inner.(*ast.NamedType)
+ named, isNamed := ref.Inner.(*ast.NamedType)
- if !ok {
+ if !isNamed {
t.Fatalf("expected NamedType inside RefType, got %T", ref.Inner)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stmt := program.Statements[0].(*ast.ExpressionStatement) | |
| cast, ok := stmt.Expression.(*ast.CastExpression) | |
| if !ok { | |
| t.Fatalf("expected CastExpression, got %T", stmt.Expression) | |
| } | |
| if _, ok := cast.Value.(*ast.BorrowExpression); !ok { | |
| t.Fatalf("expected BorrowExpression as cast value, got %T", cast.Value) | |
| } | |
| ref, ok := cast.Type.(*ast.RefType) | |
| if !ok { | |
| t.Fatalf("expected RefType as cast target, got %T", cast.Type) | |
| } | |
| named, ok := ref.Inner.(*ast.NamedType) | |
| if !ok { | |
| t.Fatalf("expected NamedType inside RefType, got %T", ref.Inner) | |
| } | |
| if named.Name.Value != "Printable" { | |
| t.Fatalf("expected 'Printable', got %s", named.Name.Value) | |
| } | |
| } | |
| stmt := program.Statements[0].(*ast.ExpressionStatement) | |
| cast, ok := stmt.Expression.(*ast.CastExpression) | |
| if !ok { | |
| t.Fatalf("expected CastExpression, got %T", stmt.Expression) | |
| } | |
| if _, isBorrow := cast.Value.(*ast.BorrowExpression); !isBorrow { | |
| t.Fatalf("expected BorrowExpression as cast value, got %T", cast.Value) | |
| } | |
| ref, isRef := cast.Type.(*ast.RefType) | |
| if !isRef { | |
| t.Fatalf("expected RefType as cast target, got %T", cast.Type) | |
| } | |
| named, isNamed := ref.Inner.(*ast.NamedType) | |
| if !isNamed { | |
| t.Fatalf("expected NamedType inside RefType, got %T", ref.Inner) | |
| } | |
| if named.Name.Value != "Printable" { | |
| t.Fatalf("expected 'Printable', got %s", named.Name.Value) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 760-760:
shadow: declaration of "ok" shadows declaration at line 755 (govet)
🤖 Prompt for AI Agents
In `@pkg/parser/parser_test.go` around lines 754 - 775, The test shadows the `ok`
variable across multiple type assertions (e.g., `cast, ok :=
program.Statements[0].(*ast.ExpressionStatement)`, then `cast, ok := ...` etc.);
fix it by declaring/using a single `ok` or distinct names instead of repeatedly
using `:=` and shadowing: keep the initial `cast, ok :=
stmt.Expression.(*ast.CastExpression)` (or declare `var ok bool` before) and
change subsequent `:=` to `=` (or rename to `okBorrow`, `okRef`, `okNamed`) for
the assertions on `BorrowExpression`, `RefType`, and `NamedType` so no `ok` is
shadowed (references: stmt, cast, ref, named, and the type assertion uses of
ok).
| isVariadic := c.isVariadicFunction(e) | ||
|
|
||
| if !isVariadic && len(e.Arguments) != len(ft.Params) { | ||
| line, col := e.Pos() | ||
| c.error(line, col, "function expects %d arguments, got %d", len(ft.Params), len(e.Arguments)) | ||
| return ft.Return | ||
| } | ||
|
|
||
| for i, arg := range e.Arguments { | ||
| argType := c.checkExpression(arg) | ||
| if i < len(ft.Params) { | ||
| paramType := ft.Params[i] | ||
| if !paramType.Equals(Any) && !c.isAssignable(paramType, argType) { | ||
| line, col := arg.Pos() | ||
| c.error(line, col, "argument %d: cannot pass %s as %s", i+1, argType.String(), paramType.String()) | ||
| } | ||
| } | ||
|
|
||
| if IsMoveType(argType) { | ||
| if ident, ok := arg.(*ast.Identifier); ok { | ||
| line, _ := arg.Pos() | ||
| c.markMoved(ident.Value, line, "function call") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return ft.Return | ||
| } | ||
|
|
||
| func (c *Checker) isVariadicFunction(e *ast.CallExpression) bool { | ||
| if ident, ok := e.Function.(*ast.Identifier); ok { | ||
| switch ident.Value { | ||
| case "print", "println", "exec", "exec_output", "substr", "exit", "panic": | ||
| return true | ||
| } |
There was a problem hiding this comment.
Tighten builtin arity checks (substr/exit/panic)
isVariadicFunction disables argument-count validation for builtins that aren’t truly variadic, so invalid calls (e.g., substr("x"), exit()) pass the checker even though codegen only supports 2–3 args for substr and 1 for exit/panic. Consider special-casing optional arity and limiting the variadic list to true variadics only.
💡 Suggested fix
- isVariadic := c.isVariadicFunction(e)
+ isVariadic := c.isVariadicFunction(e)
+
+ if ident, ok := e.Function.(*ast.Identifier); ok && ident.Value == "substr" {
+ if len(e.Arguments) < 2 || len(e.Arguments) > 3 {
+ line, col := e.Pos()
+ c.error(line, col, "substr expects 2 or 3 arguments, got %d", len(e.Arguments))
+ return ft.Return
+ }
+ } else if !isVariadic && len(e.Arguments) != len(ft.Params) {
+ line, col := e.Pos()
+ c.error(line, col, "function expects %d arguments, got %d", len(ft.Params), len(e.Arguments))
+ return ft.Return
+ }
@@
- case "print", "println", "exec", "exec_output", "substr", "exit", "panic":
+ case "print", "println", "exec", "exec_output":
return true🤖 Prompt for AI Agents
In `@pkg/types/checker.go` around lines 641 - 675, The current isVariadicFunction
list incorrectly treats substr/exit/panic as variadic, skipping arity validation
and allowing invalid calls; update isVariadicFunction to only return true for
true variadics (e.g., "print", "println", "exec", "exec_output"), and add
explicit arity checks in the caller (the code that does the len(e.Arguments) !=
len(ft.Params) check inside Checker.checkExpression for CallExpression) to
enforce: substr must have 2 or 3 arguments, and exit and panic must have exactly
1 argument (emit the same c.error format used elsewhere). Reference
functions/values: isVariadicFunction, CallExpression e, ft.Params, substr, exit,
panic, print, println, exec, exec_output.
| c.scope.Define("self", classType) | ||
|
|
||
| paramTypes := make([]Type, len(method.Parameters)) | ||
| for i, p := range method.Parameters { | ||
| if p.Type != nil { | ||
| paramTypes[i] = c.resolveTypeExpr(p.Type) | ||
| } else { | ||
| paramTypes[i] = Any | ||
| } | ||
| c.scope.Define(p.Name.Value, paramTypes[i]) | ||
| } |
There was a problem hiding this comment.
Track ownership for method params/self
Class and impl methods define self and parameters but never call trackOwnership, so move-only values passed into methods won’t be tracked and use-after-move warnings won’t fire. Align with function handling by tracking self and each parameter (and adjust if receiver-kind dictates borrowed semantics).
💡 Suggested fix
- c.scope.Define("self", classType)
+ c.scope.Define("self", classType)
+ c.trackOwnership("self", classType)
@@
- c.scope.Define(p.Name.Value, paramTypes[i])
+ c.scope.Define(p.Name.Value, paramTypes[i])
+ c.trackOwnership(p.Name.Value, paramTypes[i])
@@
- c.scope.Define("self", classType)
+ c.scope.Define("self", classType)
+ c.trackOwnership("self", classType)
@@
- c.scope.Define(p.Name.Value, paramTypes[i])
+ c.scope.Define(p.Name.Value, paramTypes[i])
+ c.trackOwnership(p.Name.Value, paramTypes[i])Also applies to: 964-968
🤖 Prompt for AI Agents
In `@pkg/types/checker.go` around lines 873 - 883, The method parameter and
receiver handling in checker.go defines self and parameters via
c.scope.Define("self", classType) and c.scope.Define(p.Name.Value,
paramTypes[i]) but never calls trackOwnership, so move-only values aren’t
tracked; update the method-processing block (and the similar block at the other
location) to call c.trackOwnership on the resolved type for self and for each
parameter after you compute paramTypes[i] (use Any when p.Type is nil via
resolveTypeExpr/Any) and adjust to account for receiver-kind if borrowed
semantics are required (i.e., don’t transfer ownership for borrowed receivers).
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests