perf/fix: str implementation fixes (static literals, ryu, runtime cache)#74
Merged
Conversation
A 2026-05-20 spike confirmed Cranelift 0.131 has no WASM emission
backend — `isa::lookup("wasm32-wasip1")` returns `Unsupported`.
The `cranelift-wasm` crate is a WASM consumer (used by wasmtime to
JIT-execute WASM), not a producer. The "Cranelift-emits-WASM"
assumption that drove the earlier plan was wrong.
Reframes wasm_target.md as a deferred proposal under docs/dev/proposals/
with a realistic revival path (parallel TIR→WASM backend via
bytecodealliance's wasm-encoder crate, ~4–6 weeks). Drops the
attempted M8d milestone from the roadmap and its design spec. Fixes
the false "Cranelift supports WebAssembly" claim across the spec,
docs/index.md, and the landing page. Updates three cross-referencing
dev docs for the new path and to correct prior factual claims.
Three review fixes to docs/dev/proposals/wasm_target.md: - Heading uses the `(Draft — v0.4)` marker per docs/dev/CLAUDE.md draft-marking rule (pre-approval content) - Status header uses the canonical enum value `Design (v0.2+)`; the deferral context moves to a sub-header line so the convention- mandated values stay clean - Drop three broken links to `docs/analysis/m8d-cranelift-wasm-spike.md` (the analysis dir is gitignored, so the file isn't in the repo from a fresh-clone perspective). The spike substance is already inlined under "Why this is deferred." - Add the missing `Milestone:` entry to the References footer per the Spec/Dev/Milestone convention - Side fix: correct §17 → §16 for the Tooling section reference
Adds ryo-runtime crate as a staticlib workspace member. Implements ryo_str_alloc, ryo_str_free, ryo_str_realloc with C ABI. Null/zero-cap free is a no-op per spec.
Implements ryo_str_from_literal (heap-copies literal bytes), ryo_str_concat (allocates l_len+r_len), ryo_str_eq (byte-wise). All handle empty/null cases per spec contracts.
No-std implementations using stack buffers. Float uses 6 decimal places with trailing zero trimming.
Migrates inst_values from HashMap<TirRef, Value> to HashMap<TirRef, ValueRepr>. All existing paths use the Scalar variant. Prepares for fat-pointer str triple.
Makes cranelift_type_for(Str) panic. All callers now gated through is_str_type. Str paths marked with todo!() — they become reachable in subsequent tasks.
Adds build.rs + justfile for two-step runtime-then-compiler build. Introduces emit_str_literal_fat producing ValueRepr::Str triples, eval_inst_str for str-typed materialisation, StrLocals for multi-variable bindings, and declare_runtime_fn helper.
Extracts embedded runtime archive to a temp file per invocation, passes it to zig cc. Cleanup on completion.
generate_print_call now uses eval_inst_str to materialize any str-typed argument. Sema relaxed from literal-only to any str expression. Enables print(variable). Also registers ryo_str_from_literal with the JIT builder so runtime symbols resolve in JIT mode.
Adds TirTag::StrConcat. Sema accepts (str, str) for +. Codegen emits out-parameter call to ryo_str_concat and loads the resulting triple.
Adds StrCmpEq/StrCmpNe TIR tags. Sema accepts (str, str) for == and !=. Codegen calls ryo_str_eq, inverts for !=.
Registers three new builtins returning str. Codegen emits out-parameter calls to the corresponding runtime functions.
Adds Token::Dot, ExprKind::MethodCall, and postfix parsing loop. First postfix operator — M9/M22 will add field access and indexing arms.
Adds dot-method-call lowering through UIR → Sema → TIR → Codegen. StrLen reads the fat-pointer len field directly (no runtime call). StrIsEmpty compares len to 0.
Covers empty-string concat, equality, str + int_to_str chaining, and .len()/.is_empty() on empty strings. Function param/return tests are #[ignore] pending sret plumbing (Task 15).
Str params expand to 3 block params (ptr/len/cap). Str returns use the standard C ABI sret convention — a hidden first parameter points to a caller-allocated 24-byte buffer, the callee writes through it, the caller reads back the triple. Matches platform ABI on SysV x86_64, Windows x64, and AArch64; unifies user-fn ABI with the runtime call convention used for ryo_str_concat.
Fix ryo_int_to_str producing "-" for i64::MIN by using u64 magnitude. Fix ryo_float_to_str mishandling NaN/Infinity with proper guards. Replace silent iconst(0) fallback in eval_inst_str with unreachable!(). Remove dead emit_str_literal, LocalVar enum, and redundant OOM checks. Fix cleanup_runtime_temp to accept &Path instead of &PathBuf.
…ation ryo_str_from_literal now returns the input pointer directly with cap=0 as a sentinel meaning 'static, don't free'. This eliminates a heap allocation + memcpy for every string literal evaluation.
Confirms that concat with static (cap=0) inputs produces heap results, and that ryo_str_free is a safe noop on static strings.
The hand-rolled algorithm used `as u64` which saturates for values >= 2^64 and was limited to 6 fractional digits. ryu provides correct shortest-representation formatting with round-trip fidelity.
Eliminates redundant filesystem I/O on every AOT build. The embedded libryo_runtime.a is extracted once to ~/.ryo/cache/<hash>.a and reused across builds until the runtime binary changes.
Verifies round-trip fidelity for large floats (>2^64) and small decimals through the full JIT pipeline.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughImplements a first-class string type with method-call syntax and fat-pointer ABI: adds a runtime staticlib with C-FFI string primitives and formatters, build-time runtime resolution/caching, linker/pipeline wiring, parser/AST/UIR/TIR and semantic support, Cranelift lowering expanding strings to ptr/len/cap with sret returns, and extensive integration tests. ChangesString support with method calls and fat-pointer ABI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codegen.rs (1)
598-609:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
strreassignment inAssignpaths.
Assigncurrently updates onlyctx.locals(scalar vars).strvars live inctx.str_locals, so mutable string reassignment will fail or be skipped.Suggested fix
TirTag::Assign => { let view = ctx.tir.assign_view(r); + if let Some((ptr_var, len_var, cap_var)) = ctx + .str_locals + .get(&view.name) + .map(|s| (s.ptr, s.len, s.cap)) + { + let repr = Self::eval_inst_str(builder, ctx, view.value)?; + let ValueRepr::Str { ptr, len, cap } = repr else { + unreachable!("str assignment must produce ValueRepr::Str"); + }; + builder.def_var(ptr_var, ptr); + builder.def_var(len_var, len); + builder.def_var(cap_var, cap); + return Ok(false); + } let val = Self::eval_inst(builder, ctx, view.value)?; let var = ctx.locals.get(&view.name).ok_or_else(|| { format!(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codegen.rs` around lines 598 - 609, The Assign arm only updates scalar locals via ctx.locals but ignores mutable string locals in ctx.str_locals, so fix TirTag::Assign (use ctx.tir.assign_view) to check if the target name exists in ctx.str_locals first and, if so, evaluate the RHS as a string and update the string-local mapping and call the builder method that defines/assigns string locals (e.g., def_str_var or the appropriate builder API for strings); otherwise fall back to the existing ctx.locals lookup and builder.def_var behavior; ensure you reuse Self::eval_inst to produce the correct typed value and return the same Ok(false) control flow.
🧹 Nitpick comments (1)
tests/integration_tests.rs (1)
1923-1929: ⚡ Quick winRuntime-output assertions are too broad here.
These checks run against full tool output, so literals can match from non-runtime sections. Consider asserting against extracted runtime output only (the same normalization should be reused across similar new tests).
Suggested helper pattern
+fn runtime_stdout(stdout: &str) -> &str { + stdout + .split("[Codegen]") + .nth(1) + .unwrap_or(stdout) + .split("[Result]") + .next() + .unwrap_or(stdout) + .trim() +} ... - let stdout = String::from_utf8_lossy(&output.stdout); + let stdout = String::from_utf8_lossy(&output.stdout); + let runtime = runtime_stdout(&stdout); assert!( - stdout.contains("Hello"), + runtime.contains("Hello"), "Output should contain 'Hello', got: {}", - stdout + runtime );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration_tests.rs` around lines 1923 - 1929, The current assertions check the entire tool output via the stdout variable, which is too broad; instead add or reuse a helper that extracts and normalizes only the runtime section (e.g., create_or_use a function like extract_runtime_output(&stdout) or normalize_runtime_output) and run the contains checks against that extracted string; update the assertions to call that helper and assert runtime_output.contains("Hello") and runtime_output.contains("[Result] => 0") so literals from non-runtime sections cannot cause false positives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.rs`:
- Around line 19-33: The build script currently panics when libryo_runtime.a is
missing (the block computing runtime_path from env::var("RYO_RUNTIME_LIB") /
CARGO_MANIFEST_DIR / CARGO_TARGET_DIR), which breaks CI; change this to a
non-fatal behavior: if RYO_RUNTIME_LIB is unset, compute the candidate path
using the build profile from env::var("PROFILE") (instead of hardcoding
"release") and, if the archive still doesn't exist, emit a cargo:warning (or log
via println!("cargo:warning=...")) and skip the panic—set runtime_path to an
Option or empty string and make downstream code handle the missing runtime
gracefully rather than aborting the build. Ensure you update uses of
runtime_path to handle the new non-panicking path/Option and keep references to
env::var("RYO_RUNTIME_LIB"), env::var("CARGO_TARGET_DIR"), and the computed path
logic in build.rs.
In `@runtime/src/lib.rs`:
- Around line 108-124: ryo_str_concat currently does unchecked u64 addition for
total = l_len + r_len and casts to usize for ryo_str_alloc and copy sizes, which
can overflow/wrap and lead to under-allocation and OOB copies; fix by checking
for overflow and ensuring l_len, r_len, and total are <= usize::MAX (or convert
each to usize after verifying), perform checked_add (or use total =
l_len.checked_add(r_len).ok_or/handle), validate that usize conversions succeed
before calling ryo_str_alloc(total_usize) and before calling
core::ptr::copy_nonoverlapping with l_len_usize and r_len_usize, and
return/handle error or set out to null on failure so no unsafe copy occurs.
In `@src/codegen.rs`:
- Around line 115-119: Scoped emission currently saves/restores only `locals`,
but `str_locals` (type `StrLocals` with fields `ptr`, `len`, `cap`) is used for
block-local string bindings and must also be preserved; update the scoped
save/restore logic to capture the current `str_locals` before entering a scoped
body and restore it on exit (in the same places where `locals` is
saved/restored), ensuring you copy/assign all three fields (`ptr`, `len`, `cap`)
back into `str_locals` to prevent block-local `str` bindings from leaking across
scopes.
In `@src/runtime_lib.rs`:
- Around line 8-13: Change cache_dir to return Result<PathBuf, std::io::Error>
instead of unconditionally returning PathBuf so it doesn't panic on missing home
directory; replace the dirs::home_dir().expect(...) call with a check that
returns an appropriate io::Error when home_dir() is None. Update any callers
(notably extract_runtime_to_temp) to propagate or map the returned io::Error
instead of relying on panic. Apply the same pattern to the other similar helper
at the 20-24 region: make it return a Result and propagate errors rather than
calling expect.
In `@src/sema.rs`:
- Around line 1378-1466: The error handlers for builtins (check_print_args and
the int_to_str/float_to_str/bool_to_str branches) are treating TypeKind::Error
like a real type and emitting extra TypeMismatch diagnostics; update each check
so you first detect/recover error types and bail (e.g., if
sema.pool.is_error(arg_ty) or matches!(sema.pool.kind(arg_ty), TypeKind::Error)
then return fcx.builder.unreachable(...) without emitting a new TypeMismatch),
otherwise perform the existing compatibility/type-kind check (use
sema.pool.compatible or the existing matches!(sema.pool.kind(...), TypeKind::X)
check) and only emit Diag::TypeMismatch when the arg is not an error and not
compatible; apply this change in check_print_args and the int_to_str /
float_to_str / bool_to_str handlers where arg_ty is obtained via
fcx.builder.ty_of.
- Around line 968-1028: InstTag::MethodCall path currently skips analyzing
method argument expressions (view.args), so bad args like s.len(bad) never
produce diagnostics; fix by eager-walking each argument with analyze_expr(sema,
fcx, scope, arg) before returning or emitting method/arity/undefined errors
(same behavior as InstTag::Call). In the InstTag::MethodCall match (using
sema.uir.method_call_view(r) and view.args), iterate over view.args and call
analyze_expr for each arg to preserve diagnostics even when the method is
invalid, then proceed to emit the existing errors and return
fcx.builder.unreachable(sema.pool.error_type(), span) as before.
---
Outside diff comments:
In `@src/codegen.rs`:
- Around line 598-609: The Assign arm only updates scalar locals via ctx.locals
but ignores mutable string locals in ctx.str_locals, so fix TirTag::Assign (use
ctx.tir.assign_view) to check if the target name exists in ctx.str_locals first
and, if so, evaluate the RHS as a string and update the string-local mapping and
call the builder method that defines/assigns string locals (e.g., def_str_var or
the appropriate builder API for strings); otherwise fall back to the existing
ctx.locals lookup and builder.def_var behavior; ensure you reuse Self::eval_inst
to produce the correct typed value and return the same Ok(false) control flow.
---
Nitpick comments:
In `@tests/integration_tests.rs`:
- Around line 1923-1929: The current assertions check the entire tool output via
the stdout variable, which is too broad; instead add or reuse a helper that
extracts and normalizes only the runtime section (e.g., create_or_use a function
like extract_runtime_output(&stdout) or normalize_runtime_output) and run the
contains checks against that extracted string; update the assertions to call
that helper and assert runtime_output.contains("Hello") and
runtime_output.contains("[Result] => 0") so literals from non-runtime sections
cannot cause false positives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66191713-ed95-4868-983f-c978dc9d1672
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
CLAUDE.mdCargo.tomlbuild.rsdocs/dev/implementation_roadmap.mdjustfileruntime/Cargo.tomlruntime/src/lib.rssrc/ast.rssrc/astgen.rssrc/builtins.rssrc/codegen.rssrc/lexer.rssrc/linker.rssrc/main.rssrc/parser.rssrc/pipeline.rssrc/runtime_lib.rssrc/sema.rssrc/tir.rssrc/uir.rstests/integration_tests.rs
The build script requires libryo_runtime.a at target/release/. Previously this only worked due to rust-cache containing a stale artifact. New deps (sha2, ryu) invalidated the cache, exposing the missing build step.
- runtime: checked_add in ryo_str_concat to prevent overflow/wrap - codegen: save/restore str_locals in emit_scoped_body to prevent leaks - codegen: handle string reassignment in TirTag::Assign - runtime_lib: cache_dir returns Result instead of panicking - sema: suppress cascading TypeMismatch on error-typed builtin args - sema: eagerly analyze MethodCall args for diagnostics
Rust's staticlib bundles precompiled std objects that reference _Unwind_* symbols (from backtrace support) even with panic=abort. On macOS the system provides these; on Linux we must explicitly link against zig's bundled libunwind.
Add ISSUES.md entry for migrating ryo-runtime to #![no_std] to eliminate the -lunwind workaround on Linux. Document in CLAUDE.md that non-immediate architectural issues should be tracked in ISSUES.md rather than GitHub Issues.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
runtime/src/lib.rs (1)
108-127:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
checked_addstill leaves the laterusizeconversions unsafe.
l_len as usize,r_len as usize, and the allocation path still truncate values aboveusize::MAX. That can under-allocate beforecopy_nonoverlappingand reintroduce OOB writes on narrower targets or malformed FFI inputs.Suggested fix
- let total = match l_len.checked_add(r_len) { - Some(t) => t, - None => oom_abort(), - }; + let l_len = usize::try_from(l_len).unwrap_or_else(|_| oom_abort()); + let r_len = usize::try_from(r_len).unwrap_or_else(|_| oom_abort()); + let total = l_len.checked_add(r_len).unwrap_or_else(|| oom_abort()); if total == 0 { (*out).ptr = std::ptr::null_mut(); (*out).len = 0; (*out).cap = 0; return; } - let ptr = ryo_str_alloc(total); - if l_len > 0 { - core::ptr::copy_nonoverlapping(l_ptr, ptr, l_len as usize); + let ptr = ryo_str_alloc(total as u64); + if l_len > 0 { + core::ptr::copy_nonoverlapping(l_ptr, ptr, l_len); } - if r_len > 0 { - core::ptr::copy_nonoverlapping(r_ptr, ptr.add(l_len as usize), r_len as usize); + if r_len > 0 { + core::ptr::copy_nonoverlapping(r_ptr, ptr.add(l_len), r_len); } (*out).ptr = ptr; - (*out).len = total; - (*out).cap = total; + (*out).len = total as u64; + (*out).cap = total as u64;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/src/lib.rs` around lines 108 - 127, The code uses checked_add on signed/unsigned lengths but still casts l_len, r_len, and total to usize unsafely; before calling ryo_str_alloc and doing core::ptr::copy_nonoverlapping you must validate that l_len, r_len, and total fit in usize (e.g., use TryInto::<usize>::try_into or similar) and handle conversion errors by calling oom_abort() or returning early, then use the safely converted usize values for allocation, pointer arithmetic (ptr.add) and copy lengths; update references in this block (l_len, r_len, total, checked_add, ryo_str_alloc, copy_nonoverlapping, and out) to use the validated usize variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codegen.rs`:
- Around line 603-615: The branch currently uses
ctx.str_locals.contains_key(&view.name) which can be true for a shadowed outer
str binding; instead gate on the active assignment type or the active local
binding before taking the string-assignment path: check the assignment's type on
the TirAssign/view node (e.g. view.ty or view.typ) or resolve the active local
binding from the current locals map (prefer ctx.locals.get(&view.name) if
present) and only when that active type is a string invoke
Self::eval_inst_str(builder, ctx, view.value) and bind ValueRepr::Str { ptr,
len, cap } into locals.ptr/len/cap via builder.def_var; otherwise do not enter
this branch so the unreachable! cannot be hit.
In `@src/linker.rs`:
- Around line 13-30: The link step currently converts runtime_lib to a UTF-8
&str (runtime_lib.to_str().unwrap_or("libryo_runtime.a")), which can silently
substitute the fallback name when the path is non-UTF-8; instead keep the
Path/OsStr all the way into the Command so the real cache path is passed.
Replace the string conversion and fallback with passing runtime_lib.as_os_str()
(or runtime_lib.as_ref() as an &Path/OsStr) into the args passed to
Command::new(&zig_path).args(&args) (and adjust args to hold OsStr/&OsStr items
or push the OsStr directly) so runtime_lib is forwarded verbatim to the linker.
---
Duplicate comments:
In `@runtime/src/lib.rs`:
- Around line 108-127: The code uses checked_add on signed/unsigned lengths but
still casts l_len, r_len, and total to usize unsafely; before calling
ryo_str_alloc and doing core::ptr::copy_nonoverlapping you must validate that
l_len, r_len, and total fit in usize (e.g., use TryInto::<usize>::try_into or
similar) and handle conversion errors by calling oom_abort() or returning early,
then use the safely converted usize values for allocation, pointer arithmetic
(ptr.add) and copy lengths; update references in this block (l_len, r_len,
total, checked_add, ryo_str_alloc, copy_nonoverlapping, and out) to use the
validated usize variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2f686fb-e7a2-4565-90f3-f1ca2b226dee
📒 Files selected for processing (8)
.github/workflows/ci.ymlCLAUDE.mdISSUES.mdruntime/src/lib.rssrc/codegen.rssrc/linker.rssrc/runtime_lib.rssrc/sema.rs
✅ Files skipped from review due to trivial changes (2)
- ISSUES.md
- CLAUDE.md
- linker: use Command builder with as_os_str() instead of to_str() with a silent fallback, preserving non-UTF-8 paths verbatim - runtime: validate l_len/r_len/total fit in usize via try_into before pointer arithmetic and copy operations
artefactop
commented
May 21, 2026
artefactop
commented
May 21, 2026
artefactop
commented
May 21, 2026
artefactop
commented
May 21, 2026
artefactop
commented
May 21, 2026
artefactop
commented
May 21, 2026
artefactop
commented
May 21, 2026
- build.rs: automatically compile ryo-runtime on demand respecting active profile and avoiding cargo locks - codegen: gate str Assign path on sema-resolved type to support shadowing - sema/tir: simplify TIR by removing StrIsEmpty and lowering s.is_empty() to StrLen == 0 in sema - runtime: add // SAFETY: comments to unsafe blocks in runtime/src/lib.rs - cleanup: remove redundant justfile and references, standard cargo works out-of-the-box
- build.rs: remove needless borrows for generic args (profile) flagged by clippy - formatting: run cargo fmt across sema and integration tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build.rs (1)
26-38: 💤 Low valueDocument the profile inference fallback logic.
The
PROFILE→OPT_LEVELfallback at lines 30-37 is non-obvious. Custom profiles withdebug = truebutopt-level > 0will map to"release", which may surprise users expecting the debug runtime.📝 Suggested comment
let profile = match raw_profile.as_str() { "release" | "production" | "prod" => "release", "debug" | "dev" => "debug", + // For custom profiles, infer from opt_level: optimized builds use release runtime. _ => { let opt_level = env::var("OPT_LEVEL").unwrap_or_else(|_| "0".to_string()); if opt_level != "0" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build.rs` around lines 26 - 38, The PROFILE→OPT_LEVEL fallback logic in build.rs (variables raw_profile, profile and opt_level) is non-obvious and can cause custom profiles with debug = true but OPT_LEVEL > 0 to be treated as "release"; add an inline comment immediately above the match that documents the exact mapping rules (which PROFILE strings map to "release" vs "debug", and that when PROFILE is unrecognized we consult OPT_LEVEL and treat any non-"0" as "release"), plus an example note that custom profiles with debug=true but opt-level>0 will be classified as "release" to avoid surprises for maintainers and users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.rs`:
- Around line 42-71: The code currently converts PathBufs to UTF-8 strings
(custom_target_dir.to_str().unwrap() and path.to_str().expect(...)) which will
panic on non-UTF-8 paths; modify build.rs so the Command uses &Path/&OsStr (pass
custom_target_dir as a Path instead of calling to_str) and remove the unwrap
there, and for producing the cargo:rustc-env value around RYO_RUNTIME_LIB keep
the final UTF-8 conversion but replace the hard expect with a clear panic/error
that tells users to set RYO_RUNTIME_LIB explicitly when the resolved Path
(variable path) is non-UTF-8 (i.e., detect non-UTF8 and emit the guidance
instead of unwrapping). Ensure references are made to custom_target_dir, cmd,
path and the RYO_RUNTIME_LIB environment export logic.
---
Nitpick comments:
In `@build.rs`:
- Around line 26-38: The PROFILE→OPT_LEVEL fallback logic in build.rs (variables
raw_profile, profile and opt_level) is non-obvious and can cause custom profiles
with debug = true but OPT_LEVEL > 0 to be treated as "release"; add an inline
comment immediately above the match that documents the exact mapping rules
(which PROFILE strings map to "release" vs "debug", and that when PROFILE is
unrecognized we consult OPT_LEVEL and treat any non-"0" as "release"), plus an
example note that custom profiles with debug=true but opt-level>0 will be
classified as "release" to avoid surprises for maintainers and users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c5d79c0-e554-48f1-8a0d-5ad94ab8c684
📒 Files selected for processing (10)
CLAUDE.mdCargo.tomlbuild.rsruntime/src/lib.rssrc/codegen.rssrc/linker.rssrc/runtime_lib.rssrc/sema.rssrc/tir.rstests/integration_tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- Cargo.toml
- src/runtime_lib.rs
- tests/integration_tests.rs
- runtime/src/lib.rs
- src/sema.rs
- Pass custom_target_dir as reference directly to Command arg, avoiding Path to String conversions and unwraps - Handle non-UTF-8 path resolution in final env-export logic safely with clear panic message - Add comprehensive inline documentation for PROFILE-to-OPT_LEVEL mapping rules and custom optimized profiles fallback
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ryo_str_from_literalreturns rodata pointer directly (cap=0 sentinel, no heap alloc)float_to_strwithryufor correct shortest-representation formatting~/.ryo/cache/)Summary by CodeRabbit
New Features
Bug Fixes / Tests
Documentation