Stabilize intrinsics and compiler to get ready for tests #3
Conversation
|
Testing the discord webhook :D |
WalkthroughMultiple subsystems were reworked. The backend module and its grammar were removed. MIR expression lowering switched to interned StrId keys with a shared StringPool. Cranelift now imports clif_type. IR moved from SeaHash to FxHash. Zeta gained new intrinsic, memory, pointer, array, math, OS syscall, FS, net, and time modules; parser grammar expanded significantly; a fiber runtime was removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Z as Zeta Code
participant LF as LinuxFilesystemX86_64
participant LN as LinuxNetworkX86_64
participant LT as LinuxTimeSystem
participant OSI as os_intrinsics::syscallN
participant K as Kernel
rect rgba(230,240,255,0.5)
note over Z,LF: Filesystem operation
Z->>LF: read(fd, buf, len)
LF->>OSI: syscall3(SYS_READ, fd, buf.addr(), len)
OSI->>K: invoke
K-->>OSI: isize
OSI-->>LF: isize
LF-->>Z: isize
end
rect rgba(230,255,230,0.5)
note over Z,LN: Networking operation
Z->>LN: socket(domain, type, proto)
LN->>OSI: syscall3(SYS_SOCKET, domain, type, proto)
OSI->>K: invoke
K-->>OSI: fd/isize
OSI-->>LN: isize
LN-->>Z: isize
end
rect rgba(255,245,230,0.5)
note over Z,LT: Time operation
Z->>LT: clockGetMonotonic(ts)
LT->>OSI: syscall2(SYS_CLOCK_GETTIME, CLOCK_MONOTONIC, ts.addr())
OSI->>K: invoke
K-->>OSI: isize
OSI-->>LT: isize
LT-->>Z: isize
end
sequenceDiagram
autonumber
participant L as MirExprLowerer
participant SP as StringPool
participant CMS as class_method_slots
participant CMM as class_mangled_map
participant EM as emit_call
note over L,SP: Interned symbol resolution
L->>SP: intern(ident) / resolve(StrId)
SP-->>L: StrId / &str
alt Direct method slot
L->>CMS: lookup(class_id, method_id)
CMS-->>L: Some(slot)
L->>EM: ClassCall(object, slot, args)
else Mangled free function
L->>CMM: lookup(class_id, method_id)
CMM-->>L: Some(fn_id)
L->>EM: Call(fn_id, args)
else Fallback
L->>EM: Call(scoped_name, args)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests✅ Unit Test PR creation complete.
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
emberforge-compiler/src/midend/ir/expr_lowerer.rs (1)
190-201: Wrong SSA result type for comparisons.Binary ops are always marked
I64; comparisons should yieldBool.Apply:
let l = self.lower_expr(left); let r = self.lower_expr(right); let v = self.current_block_data.fresh_value(); + let result_ty = match op { + Operator::Equals + | Operator::NotEquals + | Operator::LessThan + | Operator::LessThanOrEqual + | Operator::GreaterThan + | Operator::GreaterThanOrEqual => SsaType::Bool, + _ => SsaType::I64, + }; self.emit(Instruction::Binary { dest: v, op: lower_operator_bin(op), left: Operand::Value(l), right: Operand::Value(r) }); - self.current_block_data.value_types.insert(v, SsaType::I64); + self.current_block_data.value_types.insert(v, result_ty);engraver-assembly-emit/src/cranelift/cranelift_backend.rs (2)
193-201: Don’t fabricate zero values for missing Phi incomings.Silently passing
0masks IR bugs and can corrupt state. Prefer explicit failure (or a debug assertion) so invalid IR doesn’t proceed.- // invalid IR; use zero to avoid panic - let z = builder.ins().iconst(types::I64, 0); - args.push(BlockArg::Value(z)); + // invalid IR; fail fast + panic!("Phi incoming from predecessor {:?} missing for dest {:?}", curr_bb, dest);Repeat the same change for both then/else paths.
Also applies to: 231-236, 254-260
451-454: Avoid emitting a return forVoid.Cranelift signatures shouldn’t include a return when the function returns void.
- sig.returns.push(AbiParam::new(clif_type(&func.ret_type))); + if !matches!(func.ret_type, SsaType::Void) { + sig.returns.push(AbiParam::new(clif_type(&func.ret_type))); + } @@ - sig.returns.push(AbiParam::new(clif_type(&func.ret_type))); + if !matches!(func.ret_type, SsaType::Void) { + sig.returns.push(AbiParam::new(clif_type(&func.ret_type))); + } @@ - sig.returns.push(AbiParam::new(clif_type(&func.ret_type))); + if !matches!(func.ret_type, SsaType::Void) { + sig.returns.push(AbiParam::new(clif_type(&func.ret_type))); + }Also applies to: 468-471, 613-616
engraver-assembly-emit/src/cranelift/cranelift_intrinsics.rs (3)
30-41: Stray closing brace before block comment breaks parsing.There’s an extra
}before/* ... */.-}/* +/*
131-152: Fix U128 mapping — avoid truncationHirType::U128 is mapped to SsaType::U64 (silent truncation 128→64). Do not keep this.
- Preferred: add SsaType::U128, update layout_of (size: 16, align: 16) and any CLIF/type maps, then map HirType::U128 => SsaType::U128.
- Minimal (temporary): map HirType::U128 => SsaType::I128 and add a clear comment/TODO about the sign mismatch and that a U128 variant should be added.
Location: engraver-assembly-emit/src/cranelift/cranelift_intrinsics.rs — clif_type_of (around lines 131–152). If adding U128 also update layout_of in the same file.
Diff (minimal temporary change):
- HirType::U128 => SsaType::U64, + HirType::U128 => SsaType::I128, // preserve width; replace with SsaType::U128 + layout later
297-324: Normalize CLIF boolean in PtrCastToRaw — return a consistent integer boolean (I64)File: engraver-assembly-emit/src/cranelift/cranelift_intrinsics.rs (PtrCastToRaw). icmp returns a b1 but the else branch builds an I8 const; produce a canonical b1 false and bint it to the repo's integer-boolean (types::I64) before returning.
- let nonzero_test = if val_ty == ptr_ty { + let nonzero_b1 = if val_ty == ptr_ty { let zero = builder.ins().iconst(ptr_ty, 0); builder.ins().icmp(IntCC::NotEqual, val, zero) } else if val_ty.is_int() { // widen/truncate to pointer width then compare let normalized = ensure_value_is_ptr_width(builder, val, ptr_ty, val_ty); let zero = builder.ins().iconst(ptr_ty, 0); builder.ins().icmp(IntCC::NotEqual, normalized, zero) } else { - // Not a pointer-ish or integer-ish typed SSA value: produce constant false. - // Make sure to return a boolean-typed value consistent with your convention. - let zero_bool = builder.ins().iconst(types::I8, 0); // boolean false - return Some(zero_bool); + // Not pointer/int-like: produce canonical `b1` false by comparing 0 == 0. + let z = builder.ins().iconst(ptr_ty, 0); + builder.ins().icmp(IntCC::Equal, z, z) }; - // `nonzero_test` is an i1/icmp predicate represented as an int by CLIF. - // Return it directly (we used I8 earlier as our boolean conv). - Some(nonzero_test) + // Convert `b1` predicate into the repository's integer-boolean (I64). + Some(builder.ins().bint(types::I64, nonzero_b1))Verify boolean convention across loads/stores/returns (cranelift_backend.rs uses types::I64 for booleans).
🧹 Nitpick comments (42)
input.zeta (1)
17-31: Vec9f intent and naming: minor clarity nits
- The “f” suffix suggests float, but all components are
i32. ConsiderVec3i/Vec9ior switching fields tof32if floats were intended.multiply(Vec3f by)mutates in place. If you expect functional style too, consider adding a non‑mutating variant that returns a new value.scribe-parser/src/grammar.pest (9)
88-89: Duplicate/competing visibility rules.visibility_modifier and visibility/public/private/protected duplicate concepts and diverge (“protected” exists only in the latter). Keep one source of truth.
-visibility = _{ public_keyword | private_keyword | protected_keyword } -public_keyword = { "public" } -private_keyword = { "private" } -protected_keyword = { "protected" } +// Remove duplicate visibility rules or align visibility_modifier to include protected if supported: +// visibility_modifier = { "public" | "private" | "protected" | "package" | "module" }Also applies to: 109-113
136-137: Shift operators inconsistent; operator_shift is unused.Either remove operator_shift or include all operators in shift for consistency.
-shift = { term ~ (("<<" | ">>") ~ term)* } +shift = { term ~ (("<<" | ">>" | "<<<" | ">>>") ~ term)* } @@ -operator_shift = { ">>" | "<<" | ">>>" | "<<<" } +// operator_shift no longer needed; remove if unused elsewhere.Also applies to: 175-176
161-165: error_prefix is a suffix; naming/placement is misleading and may block intended syntax.If the language uses “!T” (prefix) and “T?” (nullable suffix), adjust accordingly; if “T!” is intended, rename to error_suffix.
-var_type = { (lambda_type | basic_type) ~ error_prefix? ~ nullable_suffix? } -error_prefix = { "!" } +var_type = { error_prefix? ~ (lambda_type | basic_type) ~ nullable_suffix? } +error_prefix = { "!" }Or, if “T!” is correct, rename:
-error_prefix = { "!" } +error_suffix = { "!" } -var_type = { (lambda_type | basic_type) ~ error_prefix? ~ nullable_suffix? } +var_type = { (lambda_type | basic_type) ~ error_suffix? ~ nullable_suffix? }
61-61: Dead/duplicate rules: stmt_ending and comparison_expr appear unused.Remove to simplify the grammar and reduce maintenance cost.
-stmt_ending = _{ "\n" | WHITESPACE* } @@ -comparison_expr = { expr ~ comparison_op ~ expr }Also applies to: 179-179
75-79: fun_decl modifiers ordering: be explicit about extern-only placement and duplication.You allow (extern_modifier ~ string)? and unsafe_modifier? in the same cluster; confirm intended combinations and that extern strings can’t appear twice. Consider a dedicated rule for function modifiers with a deterministic order.
If ordering matters, refactor to:
-fun_decl = { - visibility_modifier? ~ unsafe_modifier? ~ (extern_modifier ~ string)? ~ - mut_keyword? ~ static_modifier? ~ (inline_keyword | noinline_keyword)? ~ - (type_expr ~ ".")? ~ ident ~ generic_params? ~ param_list ~ function_type_annotation? ~ (block | arrow_expr | SEMI?) -} +fun_decl = { + visibility_modifier? ~ unsafe_modifier? ~ extern_spec? ~ storage_spec? ~ inlining_spec? ~ + (type_expr ~ ".")? ~ ident ~ generic_params? ~ param_list ~ function_type_annotation? ~ (block | arrow_expr | SEMI?) +} +extern_spec = { (extern_modifier ~ string) }? +storage_spec = { (mut_keyword? ~ static_modifier?) } +inlining_spec = { (inline_keyword | noinline_keyword)? }
33-36: generic_param: constraints list grammar is ambiguous (“: A, B, C” attaches to the same param?).If constraints are a list, wrap them as a group; otherwise, this can read as multiple generic_params.
-generic_param = { const_keyword? ~ ident ~ (":" ~ ident ~ ("," ~ ident)*)? } +generic_param = { const_keyword? ~ ident ~ (":" ~ ident ~ ("," ~ ident)*)? } +// Consider: ":" ~ ident_list where ident_list = { ident ~ ("," ~ ident)* }
52-55: package_stmt placement and let short form.package_stmt is a top-level directive; ensure it’s accepted before other stmts (consider SOI anchored form). Also, shorthand_let_stmt is defined but wasn’t used until wired in earlier.
Optional stricter top-level:
-program = { SOI ~ stmt* ~ EOI } +program = { SOI ~ package_stmt? ~ stmt* ~ EOI }
148-156: Compound assignment overlaps with assign_op; ensure precedence is intended.assign_op includes “=” or compound_assign_op. Keep compound_assign_op separate to avoid mis-parsing “=”.
-assign_op = { "=" | compound_assign_op } +assign_op = { "=" | "+=" | "-=" | "*=" | "/=" | "%=" | ">>=" | "<<=" | "&=" | "|=" | "^=" }
99-101: match arms require block; consider allowing arrow_expr for terser matches.If desired, add an arrow form to match_arm.
-match_arm = { pattern ~ "=>" ~ block } +match_arm = { pattern ~ "=>" ~ (block | arrow_expr) }emberforge-compiler/src/midend/ir/expr_lowerer.rs (2)
396-432: Remove unused variable in interface upcast.
inner_value_nameis computed but unused; drop it (or useget_typeif a check is intended).Apply:
- let interface_val = match self.current_block_data.value_types.get(&obj_val).cloned() { - Some(SsaType::User(ref name, args)) => { - let Some(inner_value_name) = self.string_pool.resolve_string(&*name).split("_").last() else { panic!() }; + let interface_val = match self.current_block_data.value_types.get(&obj_val).cloned() { + Some(SsaType::User(_name, args)) => { let upcast_dest = self.current_block_data.fresh_value(); self.emit(Instruction::UpcastToInterface { dest: upcast_dest, object: obj_val, interface_id: iface_id }); self.current_block_data.value_types.insert(upcast_dest, SsaType::User(interface, args)); upcast_dest }, _ => obj_val, };
228-244: Prefer slices over&Vecfor APIs.Takes
&Vec<Value>where&[Value]is more idiomatic and flexible. Call sites already pass&args, which will coerce to a slice.Apply:
- fn init_class_fields_from_args(&mut self, obj: Value, class_name: StrId, args: &Vec<Value>) { + fn init_class_fields_from_args(&mut self, obj: Value, class_name: StrId, args: &[Value]) {lib/zeta/os/async/threads.zeta (3)
3-18: Move Linux syscall numbers out of a cross‑platform interface.These constants are Linux x86_64‑specific. Keeping them in a generic
os.asyncsurface leaks platform details and risks misuse on other arches (aarch64 differs). Relocate to the Linux backend (e.g.,os/async/linux.zeta) or gate behind a platform cfg.-const SYS_CLONE = 56 -const SYS_FORK = 57 -... -const SYS_EPOLL_PWAIT = 281 +// (moved to zeta.os.async.linux; keep the interface here platform-neutral)
20-59: Separate atomics from OS thread/process APIs.Mixing user‑land atomics with process/thread/syscall APIs in one interface reduces cohesion. Consider a dedicated
AtomicsI32interface (or a genericAtomics<T>) and keepOsThreadSystemfocused on OS calls.
55-58: Return old value for CAS to match common semantics.
atomicCompareExchangeI32(...)-> booleanloses the observed value; most users need the old value whether or not the exchange succeeded. Consider returning the oldi32(and document success criterion) or a struct{ success: boolean, old: i32 }.-atomicCompareExchangeI32(addr: Ptr<i32>, old: i32, new: i32) -> boolean; +// Option A: +atomicCompareExchangeI32(addr: Ptr<i32>, expected: i32, desired: i32) -> i32; +// Option B: +type CasResultI32(boolean success, i32 old); +atomicCompareExchangeI32(addr: Ptr<i32>, expected: i32, desired: i32) -> CasResultI32;lib/zeta/os_intrinsics.zeta (1)
5-28: Keep signature style consistent across all syscallN decls.All decls should use the same “type name” ordering. After fixing Line 3, the set will be consistent.
lib/zeta/lang/ranges.zeta (2)
13-15: Unify operand widths in contains.
Range<i32>compared against ani64 valueinvites silent widening. Preferi32value or promote the range toi64.-contains(Range<i32> range, i64 value) -> boolean { +contains(Range<i32> range, i32 value) -> boolean {
17-19: Clarify length semantics (negative ranges).
end - startcan be negative if callers passstart > end. Either enforcestart <= endat construction or define whether negative lengths are allowed.-length(Range<i32> range) -> i64 { - range.end - range.start -} +length(Range<i32> range) -> i64 { + let d = (range.end as i64) - (range.start as i64) + if d < 0 { 0 } else { d } +}lib/zeta/os/time/time_system.zeta (1)
6-12: Prefer i32 clockId to match Linux clockid_t; document return convention.Use
i32forclockId. Also document whether negative returns are-errnoto guide callers.- clockGetTime(usize clockId, Ptr<TimeSpec> ts) -> isize; + clockGetTime(i32 clockId, Ptr<TimeSpec> ts) -> isize;lib/zeta/intrinsics.zeta (1)
13-15: Keep declaration style consistent (optional).Consider using the same “module extern …” style for stack_alloc[_zeroed] for readability.
-module extern "intrinsic" fn stack_alloc<T, const N: usize>() -> Ptr<T>; - -module extern "intrinsic" fn stack_alloc_zeroed<T, const N: usize>() -> Ptr<T>; +module extern "intrinsic" stack_alloc<T, const N: usize>() -> Ptr<T>; +module extern "intrinsic" stack_alloc_zeroed<T, const N: usize>() -> Ptr<T>;lib/zeta/os/fs/filesystem.zeta (3)
38-40: Unify async offsets with pread/pwrite (isize).Use isize for asyncRead/asyncWrite to match pread/pwrite and Linux off_t semantics.
- asyncRead(usize fd, Ptr<u8> buf, usize count, usize offset) -> isize; - asyncWrite(usize fd, Ptr<u8> buf, usize count, usize offset) -> isize; + asyncRead(usize fd, Ptr<u8> buf, usize count, isize offset) -> isize; + asyncWrite(usize fd, Ptr<u8> buf, usize count, isize offset) -> isize;
47-54: AsyncOp.offset should match API (isize).Keep struct and methods consistent.
- usize offset; // optional for pread/pwrite + isize offset; // optional for pread/pwrite
74-80: Avoid using the field nametype— rename tod_typeRename
u8 type->u8 d_typeto avoid collision with thetypekeyword and to mirror linux_dirent64'sd_type. Repo search shows DirEntry is only passed to getdents64 in lib/zeta/os/fs/linux.zeta and no.typeaccesses were found; change appears safe.-type DirEntry { +type DirEntry { usize ino; usize offset; usize reclen; - u8 type; + u8 d_type; Ptr<u8> name; }lib/zeta/lang/maths.zeta (5)
41-49: floor/ceil don’t handle NaN/±Inf or out‑of‑range casts.Casting NaN/Inf to i64 is undefined in many ABIs; also large |x| may overflow i64. Guard before casting.
Apply:
inline floor(f64 x) -> f64 { - xi := x as i64 + if (x != x) { return F64_NAN } // NaN + if (x == 1.0/0.0) { return x } // +Inf + if (x == -1.0/0.0) { return x } // -Inf + if (x > (1<<63)-1 as f64) { return x } // too large to cast + if (x < -(1<<63) as f64) { return x } + xi := x as i64 if (x < xi as f64) { return (xi - 1) as f64 } else { xi as f64 } } inline ceil(f64 x) -> f64 { - xi := x as i64 + if (x != x) { return F64_NAN } + if (x == 1.0/0.0 or x == -1.0/0.0) { return x } + if (x > (1<<63)-1 as f64 or x < -(1<<63) as f64) { return x } + xi := x as i64 if (x > xi as f64) { return (xi + 1) as f64 } else { xi as f64 } }
63-84: ln() lacks downscale range reduction; very slow/unstable for x ≪ 1.Add y<0.5 path (multiply by e, decrement n) and use a named E constant.
Apply:
- while (y > 2.0) { - y /= 2.718281828459045 - n += 1 - } + const E: f64 = 2.71828182845904523536 + while (y > 2.0) { y /= E; n += 1 } + while (y < 0.5) { y *= E; n -= 1 }
86-96: exp() needs argument reduction; series is inaccurate/slow for large |x|.Reduce by ln2 and use 1/x symmetry for negatives to improve stability.
Example change (sketch):
const LN2: f64 = 0.6931471805599453 k := floor(x / LN2) r := x - k * LN2 // series on r, then scale by 2^k
97-119: sin/cos need argument reduction; error explodes for large |x|.Reduce x into [-PI, PI] (or tighter) before the series. Also consider 12–14 terms for target precision.
Apply:
- mut term := x - mut sum := x + // Reduce x to [-PI, PI] + x := x - floor(x / (2.0 * PI)) * (2.0 * PI) + mut term := x + mut sum := x(and analogously for cos)
121-123: tan() should guard division by ~0.Return NaN (or ±Inf) when |cos(x)| underflows a small epsilon to avoid noisy infinities.
Suggested:
c := cos(x) if (c == 0.0) { return F64_NAN } sin(x) / clib/zeta/os/net/linux.zeta (2)
39-106: Duplicate wire types risk divergence across modules.SockAddr*, IoVec, MsgHdr, etc., appear here and in other packages. Centralize these in a shared header to keep ABI/layout consistent.
161-164: select() uses syscall6 with a dummy 0; prefer syscall5 if available.Passing an extra arg is harmless on x86_64 but obscures intent. If syscall5 exists in os_intrinsics, use it; otherwise leave a comment.
Apply:
-return os_intrinsics::syscall6(SYS_SELECT, nfds, - readfds.addr(), writefds.addr(), exceptfds.addr(), timeout.addr(), 0) +return os_intrinsics::syscall5(SYS_SELECT, nfds, + readfds.addr(), writefds.addr(), exceptfds.addr(), timeout.addr())lib/zeta/os/async/linux.zeta (1)
80-87: io_uring wrappers: match fs module usage and unify.Keep signatures consistent with fs and net backends; ensure both call os_intrinsics::syscall6 with all six params.
lib/zeta/os/fs/linux.zeta (1)
43-46: open(): prefer O_DIRECTORY for opendir and pass mode only when O_CREAT.Minor ergonomics tweak to avoid accidental file opens where a directory is expected.
lib/zeta/os/net/network.zeta (1)
96-99: hton/ntoh should return fixed-width types, not usize.**Use u16/u32 to avoid accidental widening and UB on platforms where usize ≠ u{16,32}.
Apply:
- hton16(usize v) -> usize; - ntoh16(usize v) -> usize; - hton32(usize v) -> usize; - ntoh32(usize v) -> usize; + hton16(u16 v) -> u16; + ntoh16(u16 v) -> u16; + hton32(u32 v) -> u32; + ntoh32(u32 v) -> u32;engraver-assembly-emit/src/cranelift/cranelift_backend.rs (4)
124-132: Inconsistent handling of non-int operands in binary; avoid mixedtodo!/unimplemented!.Left side uses
todo!(), right sideunimplemented!(). Also,ConstBoolisn’t supported. Prefer consistent handling and support booleans.Apply:
- _ => todo!(), + Operand::ConstBool(b) => builder.ins().iconst(types::I64, if *b { 1 } else { 0 }), + _ => unimplemented!("Binary left: unsupported operand"), @@ - _ => unimplemented!(), + Operand::ConstBool(b) => builder.ins().iconst(types::I64, if *b { 1 } else { 0 }), + _ => unimplemented!("Binary right: unsupported operand"),
156-165: Redundant lookup in Phi param map.The
or_elserepeats the same key; drop it.- let idx = phi_param_map - .get(&(curr_bb, *dest)) - .or_else(|| phi_param_map.get(&(curr_bb, *dest))) // redundant but explicit - .expect("phi param mapping missing for dest"); + let idx = phi_param_map + .get(&(curr_bb, *dest)) + .expect("phi param mapping missing for dest");
526-551: Interpolation lowering likely incorrect callee and args.This calls
enum_newand treatsargs[0]as an array pointer. If the intent is string interpolation, this should use__interp(declared) and pass a pointer/len or a slice, not a single element.Clarify intended ABI for interpolation. If it’s
fn __interp(ptr, len) -> ptr, switch toself.interp_funcand build a contiguous buffer (or an array object) before the call.
420-441: String data lacks a NUL terminator (if interoperating with C/OS).If runtime consumers expect C-strings, append
\0. Otherwise, keep as-is.- data_ctx.define(s.as_bytes().to_vec().into_boxed_slice()); + let mut bytes = s.as_bytes().to_vec(); + if !bytes.ends_with(&[0]) { bytes.push(0); } + data_ctx.define(bytes.into_boxed_slice());Confirm expected string ABI in the runtime.
engraver-assembly-emit/src/cranelift/cranelift_intrinsics.rs (5)
6-6: Remove unused import.
use std::io::Read;is unused.-use std::io::Read;
66-69: Remove unusedDynimport and keepModule/Linkage.
cranelift_object::object::read::elf::Dynisn’t used and can shadow names.-use cranelift_module::{Linkage, Module}; -use cranelift_object::object::read::elf::Dyn; +use cranelift_module::{Linkage, Module};
154-183: Syscall scaffold stubs will panic at runtime. Gate or implement.
runtime_syscall_signatureandextract_argsaretodo!(). Either implement or guard call sites to avoid hits during testing.I can wire a minimal
syscallNABI and arg extraction if you share the IR shape for syscalls.Also applies to: 185-192
259-266: Tag size thresholds: double-check 65,536+ and 4,294,967,296+ bounds.If
variantsequals 65,536 or 4,294,967,296, ensure it lands in the next bucket as intended.Quick test expectations:
- 256 -> 1 byte
- 65,536 -> 2 or 4? Decide and encode accordingly.
351-358:PtrDerefload type must be a memory type.If
ret_hirmaps to boolean and your convention isI8, ensuresuper::clif_type(&clif_ret)won’t yieldb1. Otherwise Cranelift will reject the load.Add an assertion or a mapping rule to translate boolean loads to
I8.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
emberforge-compiler/Cargo.toml(1 hunks)emberforge-compiler/src/backend/grammar.pest(0 hunks)emberforge-compiler/src/backend/mod.rs(0 hunks)emberforge-compiler/src/midend/ir/expr_lowerer.rs(5 hunks)engraver-assembly-emit/src/cranelift/cranelift_backend.rs(2 hunks)engraver-assembly-emit/src/cranelift/cranelift_intrinsics.rs(8 hunks)input.zeta(2 hunks)ir/Cargo.toml(1 hunks)ir/src/sea_hasher.rs(0 hunks)lib/zeta/intrinsics.zeta(1 hunks)lib/zeta/lang/arrays.zeta(1 hunks)lib/zeta/lang/maths.zeta(1 hunks)lib/zeta/lang/mem.zeta(1 hunks)lib/zeta/lang/pointer.zeta(1 hunks)lib/zeta/lang/ranges.zeta(1 hunks)lib/zeta/os/async/linux.zeta(1 hunks)lib/zeta/os/async/threads.zeta(1 hunks)lib/zeta/os/fs/filesystem.zeta(1 hunks)lib/zeta/os/fs/linux.zeta(1 hunks)lib/zeta/os/net/linux.zeta(1 hunks)lib/zeta/os/net/network.zeta(1 hunks)lib/zeta/os/time/linux_time_system.zeta(1 hunks)lib/zeta/os/time/time_system.zeta(1 hunks)lib/zeta/os_intrinsics.zeta(1 hunks)scribe-parser/src/grammar.pest(6 hunks)zetaruntime/Cargo.toml(1 hunks)zetaruntime/src/fiber.rs(0 hunks)
💤 Files with no reviewable changes (4)
- emberforge-compiler/src/backend/grammar.pest
- emberforge-compiler/src/backend/mod.rs
- ir/src/sea_hasher.rs
- zetaruntime/src/fiber.rs
🧰 Additional context used
🧬 Code graph analysis (2)
emberforge-compiler/src/midend/ir/expr_lowerer.rs (3)
emberforge-compiler/src/midend/ir/lowerer.rs (3)
lower_operator_bin(35-50)lower_type_hir(13-33)new(67-106)emberforge-compiler/src/midend/ir/module_lowerer.rs (1)
new(21-32)zetaruntime/src/string_pool.rs (1)
resolve_string(57-62)
engraver-assembly-emit/src/cranelift/cranelift_intrinsics.rs (2)
zetaruntime/src/arena.rs (1)
std(236-236)engraver-assembly-emit/src/cranelift/cranelift_backend.rs (1)
new(53-82)
🔇 Additional comments (18)
input.zeta (1)
1-1: Struct → type: confirm nominal record semantics and mutability are preservedRepo sweep found no remaining
structdeclarations and shows in-file uses that rely on mutable fields and aggregate initializers. Verify thattypepreserves: mutable fields (e.g.this.x *= …), method dispatch, and aggregate construction by compiling/running tests or checking the language spec.Relevant locations:
- input.zeta:1-4 —
type Vec3f(...)andthis.x *= by.x();- input.zeta:17-20 —
type Vec9f(...)andthis.x.multiply(by);- input.zeta:24-30 —
x()/y()/z()/sum()accessors- input.zeta:34-37 —
Vec3f { 1, 1, 1 }andVec9f { ... }initializersscribe-parser/src/grammar.pest (2)
1-3: Housekeeping: add COMMENT skipping if intended globally.You defined COMMENT but didn’t combine it with WHITESPACE skipping in Pest settings if not already configured. Ensure the parser config skips both.
1-179: Fix & re-run reachability audit — script broken (sed + ripgrep regex errors).Your run shows the audit failed: the "Defined rules" list printed line numbers (sed bug) and ripgrep/grep raised many regex parse errors. Use this corrected script (extracts rule names safely, searches as fixed strings) and re-run; paste results if it still fails.
#!/usr/bin/env bash set -euo pipefail file="scribe-parser/src/grammar.pest" [ -f "$file" ] || { echo "File not found: $file" >&2; exit 2; } echo "Defined rules:" sed -nE 's/^[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*=.*/\1/p' "$file" echo echo "Likely unreachable rules (defined but never referenced by others):" rules=$(sed -nE 's/^[[:space:]]*([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*=.*/\1/p' "$file") while IFS= read -r r; do refs=$(rg -n --fixed-strings "$r" "$file" || true) # remove the definition line(s) like: 42:rule_name = ... refs_no_def=$(printf "%s\n" "$refs" | rg -v "^\\d+:\\s*$r\\s*=" || true) if [ -z "$refs_no_def" ]; then echo " - $r" fi done <<< "$rules" echo echo "Literals used as keywords (verify they appear in the keyword rule):" rg -nP '"(public|private|protected|module|package|interface|impl|using|match|break|continue|defer|unsafe|extern|static|inline|noinline|sealed|const|own)"' "$file" | sed -E 's/^[[:space:]]*//'zetaruntime/Cargo.toml (1)
7-13: Replace pre-release dashmap with stable 6.1.0 or justify keeping the RCzetaruntime/Cargo.toml (lines 7–13): currently pins dashmap = "7.0.0-rc2" — latest stable is 6.1.0 (as of 2025-09-16). Use "dashmap = "6.1.0"" in the runtime unless an RC-only feature is required.
- Verify whether both papaya and dashmap are needed; consolidate to a single concurrent-map dependency if one covers your use cases.
lib/zeta/lang/ranges.zeta (1)
27-32: Iterator logic is correct ([start, end)).Yields inclusive start and excludes end; no off‑by‑one issues spotted.
lib/zeta/os/time/linux_time_system.zeta (1)
19-25: LGTM on the convenience wrappers.The inline monotonic/realtime helpers are clean once clockGetTime is fixed.
lib/zeta/lang/arrays.zeta (1)
9-17: Confirm stack allocation lifetime; ensure inlining is honored.Array.new/zeroed rely on stack_alloc escaping to caller. If inlining can be elided, this risks dangling pointers. Verify the compiler guarantees inlining here or add docs.
lib/zeta/lang/pointer.zeta (2)
5-11: LGTM on Ptr surface.Constructors and wrappers line up with intrinsics once the module name is fixed.
13-15: Rename stor → store to match intrinsic and callers.rg search for '\bstor\s*<' returned only the declaration at lib/zeta/lang/pointer.zeta:13 — rename is safe.
-inline mut stor<T>(Ptr<T> ptr, T value) = intrinsics::ptr_store<T>(ptr, value) +inline mut store<T>(Ptr<T> ptr, T value) = intrinsics::ptr_store<T>(ptr, value)lib/zeta/os/net/linux.zeta (3)
7-38: Syscall numbers look correct for x86_64.Nice coverage and correct IDs for socket APIs and epoll/select.
If you have a constants module, consider reusing it to avoid drift across files.
124-142: accept/accept4 argument handling LGTM.Addr and addrlen pointers are passed correctly; return type matches -errno convention.
144-151: TUNSETIFF constant and ioctl wrapper LGTM.The value 0x400454ca matches Linux; openat use of AT_FDCWD is correct.
Ensure devname points to "/dev/net/tun" and flags include O_RDWR.
lib/zeta/os/async/linux.zeta (1)
72-78: epollPwait: argument order/width OK — scan inconclusive; verify no bare syscall[0-6] calls remainGood use of syscall6 here; align all epoll wrappers to syscall6.
Run one of these locally and paste the output so I can re-check:
# ripgrep (preferred) rg -nP --hidden -S '\bsyscall[0-6]\s*\(' -C1 || true # fallback (if rg not available) git grep -n -E 'syscall[0-6]\s*\(' || trueengraver-assembly-emit/src/cranelift/cranelift_intrinsics.rs (2)
22-28: LGTM: intrinsic name mapping aligned toptr_*.
80-87: Good: route HIR types throughclif_type_ofthen centralsuper::clif_type.Keeps one source of truth for CLIF types.
Also applies to: 101-103
engraver-assembly-emit/src/cranelift/cranelift_backend.rs (3)
15-15: Centralize CLIF type mapping — confirm definition & visibilityImporting
crate::cranelift::clif_typeis the right direction; I couldn't locate aclif_typedefinition in the repo with the provided search. Ensureclif_typeis declaredpuborpub(super), exported fromcrate::cranelift, and that this import doesn't introduce a module cycle.Location: engraver-assembly-emit/src/cranelift/cranelift_backend.rs:15
Run to verify locally:
rg -n --type=rust '\bclif_type\b' -S -C3 rg -n --type=rust '\bfn\s+clif_type\s*\(' -S -C2
472-478: Reuse existing FuncId from self.func_ids — don't re-declare.rg shows only this
declare_function(&func.name)occurrence, but if functions are declared earlier (e.g. inemit_module) you must reuse the storedFuncIdinstead of callingdeclare_functionagain.File: engraver-assembly-emit/src/cranelift/cranelift_backend.rs Lines: 472-478
- let fid = self.module.declare_function(&func.name, Linkage::Local, &sig).unwrap(); + let fid = *self.func_ids.get(&func.name) + .unwrap_or_else(|| panic!("FuncId for {} not declared", func.name));
281-286: Fix alloc: remove placeholder and compute size from type.
- Remove the unused
alloc_funcplaceholder and use the dedicatedself.alloc_func.- Replace hardcoded
3 * 8with a layout-derived size (uselayout_ofwithTargetInfo/ pointer size).- let size_bytes = 3i64 * 8; // <= conservative default; ideally compute from `ty` - let alloc_func = self.module.declare_func_in_func(self.interp_func /* placeholder? see below */, &mut builder.func); - // Actually we create a dedicated alloc FuncId in new() — use that (see below as `alloc_func_id`). - // Here I'll assume you added `self.alloc_func` as FuncId in the struct, so: - let alloc_ref = self.module.declare_func_in_func(self.alloc_func, &mut builder.func); - let size_val = builder.ins().iconst(types::I64, size_bytes); + // Compute object size using layout_of + let ptr_bytes = (self.module.isa().pointer_type().bytes() as u64); + let target = crate::cranelift::cranelift_intrinsics::TargetInfo { ptr_bytes }; + let size_u64 = crate::cranelift::cranelift_intrinsics::layout_of(&_ty, &target) + .expect("Alloc: cannot layout type") + .size; + let alloc_ref = self.module.declare_func_in_func(self.alloc_func, &mut builder.func); + let size_val = builder.ins().iconst(types::I64, size_u64 as i64);
- If
SsaTypefor_tycan be unsized (slice/dyn), handle that case separately (runtime length or explicit size).
| rayon = "1.10.0" | ||
| smallvec = "1.15.0" | ||
| arrayvec = "0.7.6" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
New deps look fine; verify necessity and cleanup hashing deps.
- Confirm
arrayvecis actually used; drop if not. - You still depend on
seahashelsewhere in this crate while IR moved to FxHash-based maps. Ifseahashis now unused here, remove it to avoid drift. - Edition 2024: make sure CI toolchain is pinned/new enough.
Run to check usage and prune:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "arrayvec usage:"
rg -n --type=rust -C2 '\bArrayVec\b|arrayvec::|use\s+arrayvec' || echo "No matches"
echo
echo "seahash usage in emberforge-compiler:"
rg -n --type=rust -C2 '\bseahash\b|SeaHash' emberforge-compiler || echo "No matches"Length of output: 10521
Remove unused arrayvec; keep seahash.
- arrayvec: no usages found — remove from emberforge-compiler/Cargo.toml (the new entry on lines ~17–19).
- seahash: actively used (ir::sea_hasher::SeaHashBuilder / HashMap::with_hasher) in emberforge-compiler/src/midend/ir/{module_lowerer.rs, lowerer.rs, block_data.rs} — do not remove.
- CI: ensure toolchain is pinned / recent enough for Edition 2024.
🤖 Prompt for AI Agents
In emberforge-compiler/Cargo.toml around lines 17 to 19, remove the unused
dependency entry for arrayvec (delete the arrayvec = "0.7.6" line) while leaving
seahash intact; verify no other references to arrayvec remain in the repo and
run cargo check to confirm. Also ensure the CI toolchain is pinned or updated to
a Rust version that supports Edition 2024 (update rust-toolchain or CI config
accordingly) so the project builds cleanly.
| HirExpr::Binary { left, op, right } => | ||
| self.lower_expr_binary(left, op, right), | ||
|
|
||
| HirExpr::Ident(name) => self.var_map[name], |
There was a problem hiding this comment.
Indexing bug: missing reference causes type mismatch.
Use a reference when indexing var_map.
Apply:
- HirExpr::Ident(name) => self.var_map[name],
+ HirExpr::Ident(name) => self.var_map[&name],📝 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.
| HirExpr::Ident(name) => self.var_map[name], | |
| HirExpr::Ident(name) => self.var_map[&name], |
🤖 Prompt for AI Agents
In emberforge-compiler/src/midend/ir/expr_lowerer.rs around line 62, the branch
HirExpr::Ident(name) indexes var_map with name by value causing a type mismatch;
change the index to use a reference (e.g. self.var_map[&name]) or otherwise
borrow the key when accessing var_map so the lookup type matches the map's key
type.
| fn handle_ident(&mut self, op: AssignmentOperator, rhs: Value, name: StrId) -> Value { | ||
| let var_val = self.var_map[&name]; | ||
|
|
||
| // Determine if this is a simple assign or a compound assign | ||
| let result = match op { | ||
| AssignmentOperator::Assign => rhs, | ||
| AssignmentOperator::AddAssign | ||
| | AssignmentOperator::SubtractAssign | ||
| | AssignmentOperator::MultiplyAssign | ||
| | AssignmentOperator::DivideAssign | ||
| | AssignmentOperator::ModuloAssign | ||
| | AssignmentOperator::BitAndAssign | ||
| | AssignmentOperator::BitOrAssign | ||
| | AssignmentOperator::BitXorAssign | ||
| | AssignmentOperator::ShiftLeftAssign | ||
| | AssignmentOperator::ShiftRightAssign => { | ||
| let dest = self.new_value(); | ||
| let bin_op = assign_op_to_bin_op(op); | ||
|
|
||
| self.emit(Instruction::Binary { | ||
| dest, | ||
| op: bin_op, | ||
| left: Operand::Value(var_val), | ||
| right: Operand::Value(rhs), | ||
| }); | ||
| dest | ||
| } | ||
| }; | ||
|
|
||
| self.var_map.insert(name.clone(), result); | ||
| result | ||
| } |
There was a problem hiding this comment.
assign_op_to_bin_op is used but not defined/imported.
This will not compile. Add the helper mapping.
Apply at file end (before the final closing brace or after the impl):
+// Maps compound assignment ops to the corresponding binary op.
+fn assign_op_to_bin_op(op: AssignmentOperator) -> BinOp {
+ match op {
+ AssignmentOperator::AddAssign => BinOp::Add,
+ AssignmentOperator::SubtractAssign => BinOp::Sub,
+ AssignmentOperator::MultiplyAssign => BinOp::Mul,
+ AssignmentOperator::DivideAssign => BinOp::Div,
+ AssignmentOperator::ModuloAssign => BinOp::Mod,
+ AssignmentOperator::BitAndAssign => BinOp::BitAnd,
+ AssignmentOperator::BitOrAssign => BinOp::BitOr,
+ AssignmentOperator::BitXorAssign => BinOp::BitXor,
+ AssignmentOperator::ShiftLeftAssign => BinOp::Shl,
+ AssignmentOperator::ShiftRightAssign => BinOp::Shr,
+ AssignmentOperator::Assign => unreachable!("plain Assign is handled earlier"),
+ }
+}If some BinOp variants don’t exist yet, either add them to ssa_ir::BinOp or gate unsupported ops with unimplemented! to keep the build green.
📝 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.
| fn handle_ident(&mut self, op: AssignmentOperator, rhs: Value, name: StrId) -> Value { | |
| let var_val = self.var_map[&name]; | |
| // Determine if this is a simple assign or a compound assign | |
| let result = match op { | |
| AssignmentOperator::Assign => rhs, | |
| AssignmentOperator::AddAssign | |
| | AssignmentOperator::SubtractAssign | |
| | AssignmentOperator::MultiplyAssign | |
| | AssignmentOperator::DivideAssign | |
| | AssignmentOperator::ModuloAssign | |
| | AssignmentOperator::BitAndAssign | |
| | AssignmentOperator::BitOrAssign | |
| | AssignmentOperator::BitXorAssign | |
| | AssignmentOperator::ShiftLeftAssign | |
| | AssignmentOperator::ShiftRightAssign => { | |
| let dest = self.new_value(); | |
| let bin_op = assign_op_to_bin_op(op); | |
| self.emit(Instruction::Binary { | |
| dest, | |
| op: bin_op, | |
| left: Operand::Value(var_val), | |
| right: Operand::Value(rhs), | |
| }); | |
| dest | |
| } | |
| }; | |
| self.var_map.insert(name.clone(), result); | |
| result | |
| } | |
| fn handle_ident(&mut self, op: AssignmentOperator, rhs: Value, name: StrId) -> Value { | |
| let var_val = self.var_map[&name]; | |
| // Determine if this is a simple assign or a compound assign | |
| let result = match op { | |
| AssignmentOperator::Assign => rhs, | |
| AssignmentOperator::AddAssign | |
| | AssignmentOperator::SubtractAssign | |
| | AssignmentOperator::MultiplyAssign | |
| | AssignmentOperator::DivideAssign | |
| | AssignmentOperator::ModuloAssign | |
| | AssignmentOperator::BitAndAssign | |
| | AssignmentOperator::BitOrAssign | |
| | AssignmentOperator::BitXorAssign | |
| | AssignmentOperator::ShiftLeftAssign | |
| | AssignmentOperator::ShiftRightAssign => { | |
| let dest = self.new_value(); | |
| let bin_op = assign_op_to_bin_op(op); | |
| self.emit(Instruction::Binary { | |
| dest, | |
| op: bin_op, | |
| left: Operand::Value(var_val), | |
| right: Operand::Value(rhs), | |
| }); | |
| dest | |
| } | |
| }; | |
| self.var_map.insert(name.clone(), result); | |
| result | |
| } | |
| // Maps compound assignment ops to the corresponding binary op. | |
| fn assign_op_to_bin_op(op: AssignmentOperator) -> BinOp { | |
| match op { | |
| AssignmentOperator::AddAssign => BinOp::Add, | |
| AssignmentOperator::SubtractAssign => BinOp::Sub, | |
| AssignmentOperator::MultiplyAssign => BinOp::Mul, | |
| AssignmentOperator::DivideAssign => BinOp::Div, | |
| AssignmentOperator::ModuloAssign => BinOp::Mod, | |
| AssignmentOperator::BitAndAssign => BinOp::BitAnd, | |
| AssignmentOperator::BitOrAssign => BinOp::BitOr, | |
| AssignmentOperator::BitXorAssign => BinOp::BitXor, | |
| AssignmentOperator::ShiftLeftAssign => BinOp::Shl, | |
| AssignmentOperator::ShiftRightAssign => BinOp::Shr, | |
| AssignmentOperator::Assign => unreachable!("plain Assign is handled earlier"), | |
| } | |
| } |
🤖 Prompt for AI Agents
In emberforge-compiler/src/midend/ir/expr_lowerer.rs around lines 102 to 133,
the call to assign_op_to_bin_op is missing and causes a compile error; add a
helper function (placed at the end of the file before the final closing brace or
after the impl) that maps AssignmentOperator variants to the corresponding
ssa_ir::BinOp variants and returns the BinOp for use in the Binary instruction;
for any AssignmentOperator that has no corresponding ssa_ir::BinOp either add
the missing BinOp variant(s) to ssa_ir::BinOp or handle those cases in the
helper by returning an explicit unimplemented!() (or similar) to keep the build
green.
| let cls_name = match self.current_block_data.value_types.get(&obj_val) { | ||
| Some(SsaType::Ptr(inner)) => match &**inner { SsaType::User(name) => name.clone(), other => panic!("FieldAccess on non-user pointer type {:?}", other) }, | ||
| Some(SsaType::User(name, args) ) => { | ||
| if let Some(inner_value_name) = self.string_pool.resolve_string(name).split("_").last() { | ||
| StrId(self.string_pool.intern(inner_value_name)) | ||
| } else { | ||
| panic!("Could not determine object's class for FieldAccess.") | ||
| } | ||
| } | ||
| other => panic!("Could not determine object's class for FieldAccess: {:?}", other), | ||
| }; |
There was a problem hiding this comment.
Fragile class-name parsing via string split. Use canonical helper.
Splitting on "_" will break for names containing underscores. Use optimized_string_buffering::get_type.
Apply:
- let cls_name = match self.current_block_data.value_types.get(&obj_val) {
- Some(SsaType::User(name, args) ) => {
- if let Some(inner_value_name) = self.string_pool.resolve_string(name).split("_").last() {
- StrId(self.string_pool.intern(inner_value_name))
- } else {
- panic!("Could not determine object's class for FieldAccess.")
- }
- }
+ let cls_name = match self.current_block_data.value_types.get(&obj_val) {
+ Some(SsaType::User(name, _args)) => {
+ optimized_string_buffering::get_type(*name, self.string_pool)
+ }
other => panic!("Could not determine object's class for FieldAccess: {:?}", other),
};📝 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.
| let cls_name = match self.current_block_data.value_types.get(&obj_val) { | |
| Some(SsaType::Ptr(inner)) => match &**inner { SsaType::User(name) => name.clone(), other => panic!("FieldAccess on non-user pointer type {:?}", other) }, | |
| Some(SsaType::User(name, args) ) => { | |
| if let Some(inner_value_name) = self.string_pool.resolve_string(name).split("_").last() { | |
| StrId(self.string_pool.intern(inner_value_name)) | |
| } else { | |
| panic!("Could not determine object's class for FieldAccess.") | |
| } | |
| } | |
| other => panic!("Could not determine object's class for FieldAccess: {:?}", other), | |
| }; | |
| let cls_name = match self.current_block_data.value_types.get(&obj_val) { | |
| Some(SsaType::User(name, _args)) => { | |
| optimized_string_buffering::get_type(name, &self.string_pool) | |
| } | |
| other => panic!("Could not determine object's class for FieldAccess: {:?}", other), | |
| }; |
🤖 Prompt for AI Agents
In emberforge-compiler/src/midend/ir/expr_lowerer.rs around lines 261 to 270,
the code extracts the class name by splitting the interned string on "_" which
is fragile for names containing underscores; replace that logic with
optimized_string_buffering::get_type to obtain the canonical type name, then
intern that returned type name and wrap with StrId as before (handle any
Option/Result the helper returns and keep the existing panic path if it cannot
resolve). Ensure you call self.string_pool.intern on the helper's type string
and construct StrId(...) instead of using split("_").last().
| let maybe_cls_name: Option<SsaType> = self.current_block_data.value_types.get(&obj_val).cloned(); | ||
| let maybe_cls_name: Option<&str> = match maybe_cls_name { | ||
| Some(SsaType::User(ref name, _args)) => self.string_pool.resolve_string(name).split("_").last(), | ||
| _ => None | ||
| }; | ||
|
|
||
| let cls_name_id: Option<StrId> = if let Some(cls_name) = maybe_cls_name { | ||
| Some(StrId(self.string_pool.intern(cls_name))) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| if let Some(value) = self.emit_call(field, obj_val, &mut operands, cls_name_id) { | ||
| return value; | ||
| } | ||
|
|
||
| let direct_name: StrId = optimized_string_buffering::build_scoped_name(maybe_cls_name, field, self.string_pool); | ||
|
|
There was a problem hiding this comment.
Same underscore-split issue in method calls; normalize via get_type.
Avoid duplicating parsing logic; derive cls_name_id from get_type and pass a resolved &str to build_scoped_name.
Apply:
- let maybe_cls_name: Option<SsaType> = self.current_block_data.value_types.get(&obj_val).cloned();
- let maybe_cls_name: Option<&str> = match maybe_cls_name {
- Some(SsaType::User(ref name, _args)) => self.string_pool.resolve_string(name).split("_").last(),
- _ => None
- };
-
- let cls_name_id: Option<StrId> = if let Some(cls_name) = maybe_cls_name {
- Some(StrId(self.string_pool.intern(cls_name)))
- } else {
- None
- };
+ let cls_name_id: Option<StrId> = match self.current_block_data.value_types.get(&obj_val) {
+ Some(SsaType::User(name, _)) => Some(optimized_string_buffering::get_type(*name, self.string_pool)),
+ _ => None,
+ };
+ let maybe_cls_name: Option<&str> = cls_name_id
+ .as_ref()
+ .map(|id| self.string_pool.resolve_string(&**id));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In emberforge-compiler/src/midend/ir/expr_lowerer.rs around lines 330 to 347,
replace the current ad-hoc underscore-split and duplicated parsing by using the
existing get_type(obj_val) path: call self.get_type(obj_val) to obtain the
SsaType, match SsaType::User to resolve the name string with
self.string_pool.resolve_string(...) (without splitting on "_"), create
cls_name_id from that resolved &str (interning into StrId), pass the resolved
&str (or None) into optimized_string_buffering::build_scoped_name, and remove
the duplicate underscore-splitting logic so cls_name_id and build_scoped_name
both derive from the same normalized type/name source.
| @@ -0,0 +1,13 @@ | |||
| package zeta.os.time | |||
|
|
|||
| struct TimeSpec(isize sec, isize nsec); | |||
There was a problem hiding this comment.
Use fixed 64‑bit fields for TimeSpec for cross‑arch correctness.
isize maps to 32‑bit on 32‑bit targets, but Linux __kernel_timespec uses 64‑bit fields. Using isize risks ABI/layout mismatches on 32‑bit.
-struct TimeSpec(isize sec, isize nsec);
+struct TimeSpec(i64 sec, i64 nsec);📝 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.
| struct TimeSpec(isize sec, isize nsec); | |
| struct TimeSpec(i64 sec, i64 nsec); |
🤖 Prompt for AI Agents
In lib/zeta/os/time/time_system.zeta around line 3, the TimeSpec struct
currently uses isize for sec and nsec which can be 32‑bit on 32‑bit targets and
lead to ABI/layout mismatches with Linux __kernel_timespec; change both fields
to fixed 64‑bit signed types (i64) so sec and nsec are always 64 bits, update
any code that constructs or reads TimeSpec to use i64 values, and run/adjust any
FFI bindings or serialization logic that depends on the old layout to ensure
compatibility.
| statem_decl = { "statem" ~ ident ~ "{" ~ transition* ~ "}" } | ||
|
|
||
| interface_decl = { visibility_modifier? ~ "interface" ~ ident ~ "(" ~ param_list ~ ")" } | ||
|
|
||
| transition = { state_ref ~ "=>" ~ state_ref ~ ";" } | ||
|
|
There was a problem hiding this comment.
Unreachable top-level rules (not included in stmt): statem_decl, interface_decl, extern_block, impl_decl, type_decl.
These constructs can’t be parsed because they aren’t referenced from stmt/block_stmt. Also, type_decl conflicts with struct_decl ("type …") unless ordered/disambiguated.
Apply these diffs to wire them in and avoid conflicts:
- block_stmt = _{ if_stmt | while_stmt | for_stmt | match_stmt | fun_decl | struct_decl | enum_decl | unsafe_block | block }
+ block_stmt = _{ if_stmt | while_stmt | for_stmt | match_stmt
+ | fun_decl | struct_decl | type_decl | enum_decl
+ | interface_decl | impl_decl | extern_block | statem_decl
+ | unsafe_block | block }
- simple_stmt = _{ (import_stmt | let_stmt | const_stmt | return_stmt | expr_stmt | break_stmt | continue_stmt | defer_stmt) ~ stmt_terminator? }
+ simple_stmt = _{ (package_stmt | import_stmt | let_stmt | shorthand_let_stmt
+ | const_stmt | return_stmt | expr_stmt | break_stmt | continue_stmt | defer_stmt) ~ stmt_terminator? }To prevent ambiguity between struct_decl and type_decl, either place type_decl before struct_decl in block_stmt (as above) or fold the aliasing into struct_decl:
-struct_decl = { visibility_modifier? ~ "type" ~ ident ~ generic_params? ~ param_list? }
+struct_decl = { visibility_modifier? ~ "type" ~ ident ~ generic_params? ~ ( "=" ~ type_expr | param_list ) }
- type_decl = { "type" ~ ident ~ "=" ~ ident }
+// type_decl can now be removed, or kept for back-compat delegating to the same form.Also applies to: 27-31, 69-70, 73-74, 93-94
🤖 Prompt for AI Agents
In scribe-parser/src/grammar.pest around lines 8-13 (and similarly apply to the
other referenced ranges 27-31, 69-70, 73-74, 93-94), several top-level rules
(statem_decl, interface_decl, extern_block, impl_decl, type_decl) are
unreachable because they are not referenced from stmt or block_stmt and
type_decl conflicts with struct_decl; update the grammar to (1) include these
rules into the appropriate production(s) so they are reachable (e.g., add
statem_decl, interface_decl, extern_block, impl_decl, and type_decl into stmt
and/or block_stmt alternatives where other top-level decls live), and (2)
resolve the "type …" vs "struct …" ambiguity by either placing type_decl before
struct_decl in the block_stmt/stmt alternatives or by folding aliasing into
struct_decl (merge the alias form into the struct_decl alternative) so parsing
is disambiguated.
| qualified_type = { generic_type ~ ("." ~ ident)* } | ||
|
|
||
| transition = { | ||
| state_ref ~ "=>" ~ state_ref ~ ";" | ||
| generic_type = { | ||
| ident ~ generic_params? | ||
| | ident ~ "<" ~ generic_placeholder ~ ("," ~ generic_placeholder)* ~ ">" | ||
| } | ||
|
|
||
| generic_placeholder = { ident ~ (":" ~ ident)? } | ||
|
|
There was a problem hiding this comment.
Generic type syntax is internally inconsistent; second alternative is shadowed.
generic_type’s first alt (ident ~ generic_params?) will greedily match “<…>” with generic_params (declaration form), making the second alt (use-site placeholders) unreachable. Use type arguments (type_expr) for use sites.
-qualified_type = { generic_type ~ ("." ~ ident)* }
-
-generic_type = {
- ident ~ generic_params?
- | ident ~ "<" ~ generic_placeholder ~ ("," ~ generic_placeholder)* ~ ">"
-}
-
-generic_placeholder = { ident ~ (":" ~ ident)? }
+qualified_type = { generic_type ~ ("." ~ ident)* }
+generic_type = { ident ~ ("<" ~ type_expr ~ ("," ~ type_expr)* ~ ">")? }
+// generic_placeholder no longer needed at use sites; remove or repurpose if still used elsewhere.📝 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.
| qualified_type = { generic_type ~ ("." ~ ident)* } | |
| transition = { | |
| state_ref ~ "=>" ~ state_ref ~ ";" | |
| generic_type = { | |
| ident ~ generic_params? | |
| | ident ~ "<" ~ generic_placeholder ~ ("," ~ generic_placeholder)* ~ ">" | |
| } | |
| generic_placeholder = { ident ~ (":" ~ ident)? } | |
| qualified_type = { generic_type ~ ("." ~ ident)* } | |
| generic_type = { ident ~ ("<" ~ type_expr ~ ("," ~ type_expr)* ~ ">")? } | |
| // generic_placeholder no longer needed at use sites; remove or repurpose if still used elsewhere. |
🤖 Prompt for AI Agents
In scribe-parser/src/grammar.pest around lines 16 to 24, the first alternative
of generic_type (ident ~ generic_params?) greedily matches angle-bracket forms
and therefore shadows the second alternative; to fix this, split declaration vs
use-site forms so they are unambiguous: make the declaration form only match a
named generic-parameter list (rename or constrain generic_params to a
declaration-specific rule) and make the use-site form use type arguments
(type_expr) instead of generic_placeholder; either reorder with a more specific
lookahead for the use-site "< ... >" case or change the first alternative so it
does not optionally consume "<...>" (so the second alternative becomes
reachable).
| import_stmt = { "using" ~ string } | ||
| package_stmt = { "package" ~ (ident ~ ("." ~ ident)*) } | ||
| let_stmt = { mut_keyword? ~ var_type ~ atom ~ "=" ~ expr } | ||
| shorthand_let_stmt = { mut_keyword? ~ atom ~ ":=" ~ expr } | ||
| const_stmt = { "const" ~ var_type ~ ident ~ "=" ~ expr } | ||
| return_stmt = { "return" ~ expr? } | ||
| if_stmt = { "if" ~ "(" ~ expr ~ ")" ~ block ~ (("else" ~ (block | if_stmt))?) } | ||
| while_stmt = { "while" ~ "(" ~ expr ~ ")" ~ block } | ||
| for_stmt = { "for" ~ "(" ~ atom ~ ":=" ~ expr ~ ")" ~ block } | ||
|
|
There was a problem hiding this comment.
Reserve all language keywords to avoid identifiers like “using”, “package”, “interface”, “match”, etc.
keyword omits many tokens introduced in this grammar, allowing them as identifiers and causing surprising parses.
-import_stmt = { "using" ~ string }
+import_stmt = { "using" ~ string }
@@
-keyword = _{ "true" | "false" | "if" | "else" | "while" | "return" | "let" | "for" | "enum" | "type" }
+keyword = _{ "true" | "false" | "if" | "else" | "while" | "return" | "let" | "for"
+ | "enum" | "type" | "match" | "break" | "continue" | "defer"
+ | "unsafe" | "extern" | "static" | "inline" | "noinline" | "sealed"
+ | "using" | "package" | "module" | "public" | "private" | "interface"
+ | "impl" | "const" | "own" }Optionally reserve builtin type names too (i32, u64, boolean, str, …) if they shouldn’t be valid identifiers.
Also applies to: 115-118
| primary = { atom ~ ( ( ("." | "?.") ~ atom ) | call_args)* } | ||
| array_literal = { "[" ~ (expr ~ ("," ~ expr)*)? ~ "]" } | ||
| atom = _{ parenthesized_expr | ident | number | string | class_initialization | boolean | array_literal } | ||
| atom = _{ parenthesized_expr | pattern | class_initialization | boolean | array_literal } | ||
|
|
There was a problem hiding this comment.
Indexing suffix is defined but never used; also it should accept any expr, not atom.
Currently primary can’t parse a[b]. Wire array_suffix into primary and allow full expressions as indices.
-primary = { atom ~ ( ( ("." | "?.") ~ atom ) | call_args)* }
+primary = { atom ~ ( ( ("." | "?.") ~ atom ) | call_args | array_suffix )* }
@@
-array_suffix = { "[" ~ atom ~ "]" }
+array_suffix = { "[" ~ expr ~ "]" }Also applies to: 166-167
🤖 Prompt for AI Agents
In scribe-parser/src/grammar.pest around lines 142-145 (and similarly at
166-167), the primary rule currently never uses the indexing suffix and the
index is limited to atom; update primary to include the indexing suffix in its
repetition (e.g. allow ( ("." | "?.") ~ atom | array_suffix | call_args )* ),
and change the array_suffix definition so the brackets contain expr (not atom)
so full expressions can be used as indices; ensure the array_literal rule
remains for literal syntax and that the array_suffix rule is referenced where
dotted/call suffixes are handled.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
1 similar comment
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Caution CodeRabbit timed out while planning changes for emberforge-compiler/tests/test_backend_parser.rs. Skipping this file. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
Stabilize intrinsics and compiler so that compile-time errors are gone and we can focus on testing
Summary by CodeRabbit