fix: resolve m8.4.2 issue batch (I-014…I-121, 13 issues) - #110
Merged
Conversation
Concurrency data-plane amendments (Pony-informed): - shared[T] access is read-only; shared mutation only via mutex/rwlock wrappers (5.6); plain-shared[T] graphs acyclic by construction - task.spawn_detached returns Handle[T]: identity-only, sendable, comparable, no dereference, drop does not cancel (9.2.1) - Send constraint restated as explicit 3-member predicate: owned move, shared[T] handle, Handle[T] (14.5.6) - Roadmap and Loom-KT concurrency proposal updated to match
The flat per-function TirRef→Value memo is sound only under three TIR producer invariants (unique-per-use instructions, BoolAnd/BoolOr merge via block params, statement-level IfStmt). Document them at the field so the memo gets re-scoped per-block when expression-level control flow or shared sub-expressions ever land.
A never-typed callee was emitted as call + trap + dead block without reload_inout_args, dropping any inout argument's mutated value. Cranelift models the callee as an ordinary returning call, so emit the reloads between the call and the trap.
name_to_decl was silently first-wins: both bodies were analyzed but calls bound to the first with no redefinition diagnostic. Emit DiagCode::DuplicateDeclaration at seed time; first-wins binding is kept for error recovery so the duplicate body still gets analyzed.
The Newline regex only matched LF, so any CRLF source (the Windows editor default) failed with 'found <error>' at the end of line one. Match \r?\n and skip the optional \r when indent::process measures indentation; spans stay byte-accurate since the \r remains inside the token's span.
Output names were derived from file_stem only, so the .o and the exe landed in the CWD: two same-stem sources built from one directory clobbered each other, and the early ? on link failure leaked the .o. Artifacts now land next to the source file, and the .o cleanup runs on the link-failure path too. The CodSpeed workflow drops the mv/rm dance; the benchmark AOT test copies sources into its temp dir so it doesn't overwrite the committed benchmark binaries.
UIR and TIR each have exactly one trusted producer (astgen, sema), so view decoders and codegen dispatch unreachable! on malformed IR rather than reporting an internal-error Diag. Document the invariant in both module docs, with the conversion obligation should a second producer (cached IR, plugins, another front end) ever land.
The ariadne report header and label both carried d.message, printing every diagnostic twice; the label now carries no text (the span stays highlighted). emit_one falls back to a plain stderr line instead of expect() when rendering fails. main returns ExitCode: Diagnostics errors were already rendered by the pipeline, so the std Termination handler's duplicate summary line is gone; other errors print a single 'error: ...' line.
…(I-054) finalize_diags now takes Vec<Diag> (sink stages feed it via DiagSink::into_diags), and the three hand-rolled lex/parse error paths use it: display_tokens and parse_source's lex-error arm call it directly, parse_source's parse-error arm via a fail_with_diags wrapper for the always-error case. The Severity::Error filter and the render+wrap convention now live in exactly one place.
run_valgrind_smoke printed 'skipping' and passed when valgrind was missing, so a local green run may have exercised nothing — and the suite exists precisely because LSan misses leaks from Cranelift- emitted code. Outside CI a missing valgrind now fails with install instructions unless RYO_SKIP_VALGRIND=1 is set; CI lanes may still skip because the dedicated valgrind lane guarantees coverage.
…(I-014, I-015, I-016, I-077, I-078) lexer::lex now takes &mut DiagSink and never fails hard: invalid characters emit E0102 with a Token::Error placeholder, bad int/float literals emit a diag and recover with a zero literal, unknown escapes emit E0103 pointing at exactly the two bytes (still preserved verbatim), and indent errors carry the offending Newline token's span via the new IndentError. LexError is eliminated; lex and parse diagnostics accumulate in one sink so several surface per run. parse_source rebuilds chumsky Rich messages pool-aware, so parse errors name the actual identifier/string text instead of <id#N>. Also reworks the I-075 name_to_decl seed loop to the entry API (clippy map_entry fires under CI's -Dwarnings). Cargo.lock picks up the ryo->ryo-core edge from the I-103 commit.
Integer literals are parsed as i64 at lex time with sign applied later by unary -, so the positive form of 9223372036854775808 overflowed and i64::MIN was unspellable. The lexer now emits a dedicated IntLitMin token for exactly i64::MAX + 1, and the parser folds '- IntLitMin' directly to Literal::Int(i64::MIN); the token is a parse error anywhere else, so the overflowing positive form stays rejected (including after a binary minus).
…088) OwnershipSidecar.functions was a HashMap<StringId, FunctionSidecar> keyed by interned name. Correct today because names are unique, but any future overloading or same-name functions would silently collide. - OwnershipSidecar.functions is now Vec<FunctionSidecar>, positional with the tirs slice. - Ownership checker pushes one sidecar per function body in order. - codegen compile_function takes sidecar_index and both compile loops enumerate positionally. - Tests look sidecars up via a take_function_sidecar helper.
The zip path staged the download as zig-download.zip inside the temp
dir and never deleted it. If a future zig zip ever lacked the
zig-{target}-{version}/ top-level dir, the fallback rename would carry
the ~100 MB staged archive into the installed toolchain dir.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change refactors lexer diagnostics, switches ownership sidecars to positional storage, updates build artifact handling, documents concurrency semantics, and removes resolved issues and obsolete development notes. ChangesCompiler diagnostics and backend behavior
Concurrency specification
Documentation maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/specification.md (1)
2967-2974: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one shutdown boundary for detached tasks.
The handle contract says cancellation occurs at process exit, while the example says detached tasks are cancelled when
main()returns. Define whethermain()return is process exit; otherwise use one boundary in both locations.🤖 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 `@docs/specification.md` around lines 2967 - 2974, Align the detached-task cancellation boundary described in the task.spawn_detached documentation and its example: either explicitly define main() returning as process exit, or update the example and contract to use the same shutdown boundary. Keep the handle and cancellation semantics consistent in both locations.docs/dev/concurrency_loom_kt.md (2)
153-167: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDefine nested FFI re-entry.
A C callback can run on the worker's system coroutine and call Ryo code that issues another FFI call. The single system-coroutine read loop is occupied by the outer call. Define overflow coroutines or an explicit rejection/deferral rule before relying on the callback behavior described at Line 243.
🤖 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 `@docs/dev/concurrency_loom_kt.md` around lines 153 - 167, Update the FFI architecture description around the scheduler-owned system coroutines and call_ffi to explicitly define nested FFI re-entry from C callbacks. Specify either how overflow coroutines are created and managed when the read loop is occupied, or the exact rejection/deferral behavior for the inner FFI call, and ensure the callback behavior referenced later matches that policy.
302-310: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winScope
with_poolto the database operation.
handle_requestperforms bothdb.queryandlibjpeg.decode. Wrapping the whole handler moves CPU work todb_pooland makes the database pool a request-execution pool. Place only the database call insidewith_pool.🤖 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 `@docs/dev/concurrency_loom_kt.md` around lines 302 - 310, Update the handler around with_pool so only the database operation within handle_request runs on db_pool; keep libjpeg.decode and other CPU work outside the pool. Preserve the existing 16-concurrent-query bound while preventing the pool from executing the entire request handler.
🧹 Nitpick comments (3)
ryo-frontend/src/lexer.rs (1)
422-480: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse checked arithmetic for the escape span and assert the char-boundary invariant.
unescapecomputes the diagnostic span with plain+on byte offsets, and line 429 unwrapsinner[i..].chars().next()on the unstated invariant thatiis always a UTF-8 char boundary. The coding guidelines require checked or saturating arithmetic for spans and offsets, anddebug_assert!for internal invariants. Both changes are local and do not alter behavior.♻️ Proposed change
while i < inner.len() { + debug_assert!(inner.is_char_boundary(i), "unescape index must be a char boundary"); let ch = inner[i..].chars().next().unwrap(); @@ Some(c) => { // Unknown escape: report it, then preserve the // backslash and the following character verbatim. - let start = token_span.start + 1 + i; + let start = token_span.start.saturating_add(1).saturating_add(i); + let end = start.saturating_add(1).saturating_add(c.len_utf8()); sink.emit(Diag::error( - SimpleSpan::new((), start..start + 1 + c.len_utf8()), + SimpleSpan::new((), start..end), DiagCode::UnknownEscape, format!("unknown escape sequence '\\{}'", c), ));As per coding guidelines: "Use
debug_assert!for internal invariants, checked or saturating arithmetic for spans, offsets, and indices".🤖 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 `@ryo-frontend/src/lexer.rs` around lines 422 - 480, Update unescape to debug_assert! that i is a UTF-8 character boundary before slicing inner[i..]. Replace plain offset additions used to construct the UnknownEscape diagnostic span with checked or saturating arithmetic, preserving the existing span range and escape behavior.Source: Coding guidelines
ryo-frontend/src/sema.rs (1)
243-263: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake
signaturesagree withname_to_declon which duplicate wins.
Sema::newkeeps the first declaration inname_to_decl, and the comment states that calls bind to the first declaration.resolve_signatures(line 309) still callsself.signatures.insert(body.name, ...)for every body, so the last duplicate's signature overwrites the first one. Call sites then type-check against the second signature while name resolution points at the first declaration.The compile already fails with
DuplicateDeclaration, so this only affects secondary diagnostics. Aligning the two tables keeps those diagnostics coherent.♻️ Proposed change in `resolve_signatures`
- self.signatures.insert( - body.name, - FunctionSig { - params: body.params.iter().map(|p| p.ty).collect(), - return_type: body.return_type, - }, - ); + // First definition wins, mirroring `name_to_decl`. + self.signatures + .entry(body.name) + .or_insert_with(|| FunctionSig { + params: body.params.iter().map(|p| p.ty).collect(), + return_type: body.return_type, + });🤖 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 `@ryo-frontend/src/sema.rs` around lines 243 - 263, Update resolve_signatures so signatures retains the first function declaration for each name, matching the first-wins behavior of name_to_decl; avoid overwriting an existing signature when processing duplicate bodies, while continuing to analyze duplicates and emit their existing diagnostics.docs/dev/concurrency_loom_kt.md (1)
383-387: 🩺 Stability & Availability | 🔵 TrivialEnforce a limit on
pool.custom.The text restricts pool creation only “by convention”. Code can still create a pool per request and starve carriers or exhaust threads. Define a hard worker/resource limit and the authority or failure behavior before exposing this API.
🤖 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 `@docs/dev/concurrency_loom_kt.md` around lines 383 - 387, Update the “Capability-gated pool creation” guidance for pool.custom to define a hard worker/resource limit, including how the limit is enforced and what authority is required. Specify the failure behavior when creation exceeds the limit, and replace the convention-only restriction with this enforceable contract before exposing the API.
🤖 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 `@docs/dev/concurrency_loom_kt.md`:
- Around line 160-165: Update the code example near scheduler.call_ffi to use a
fenced code block tagged ryo and express the snippet as compliant Ryo
pseudocode; do not leave Rust syntax under a rust fence or move the example into
an untagged block.
- Around line 295-298: Update the FFI safety statement near the runtime
comparison to bound libjpeg.decode safety by the configured RYO_FFI_STACK_SIZE,
rather than claiming safety regardless of internal stack usage; also state that
this removes the previous 128 KB limit for typical libraries.
- Around line 347-352: Update the task.supervise proposal around handle[T] to
distinguish identity-only supervision handles from join/result handles: define a
separate awaitable join/future handle, or explicitly specify that the supervisor
receives both handles. Preserve the requirement that supervision can await each
child and observe its result or failure.
- Around line 99-117: Resolve the naming collision in the scheduler API
described in the concurrency document by renaming the execution-context
abstraction and related references such as pool.custom, with_pool, and typed
parameters to a non-conflicting name like Dispatcher or Executor. Ensure the
revised terminology clearly distinguishes scheduler contexts from the existing
std.pool resource-pool namespace throughout the documented examples and plan
references.
- Around line 336-390: Revise the “Pony cross-reference” section to present the
proposal-specific data-plane rules as draft/open rather than settled, since
conflated-channel ordering, FFI callback captures, and pool-local state remain
pending formalization. Update the nearby claim and the comparison table’s “Spec
impact | None” entry consistently, without adding new rules.
- Around line 103-117: Define with_pool as a yielding operation in the
effect-analysis section, including that it may migrate execution across OS
worker threads. Specify how task.pin() and held lock guards interact with
with_pool, and ensure the compiler diagnoses using with_pool inside a
non-migrating critical section.
In `@docs/specification.md`:
- Line 1415: Update the list marker preceding “Sharing freezes.” to use a single
space after the asterisk, preserving the paragraph content unchanged.
- Line 1415: The “trivially safe to send” claim in the Sharing freezes paragraph
must be qualified: state that plain shared[T] is safely sendable only when T
consists of safe Ryo types, accounting for the unsafe and FFI exceptions in
Section 14.5.6, or require an explicit sendability bound on T. Preserve the
existing immutability and acyclicity claims.
- Line 2757: Update the Task Handles paragraph to replace the undeclared
uppercase “Handle” references with the declared lowercase handle[T] type,
including the statement about dropping a handle. Preserve the existing semantics
and wording otherwise.
- Line 2751: Update the Spawn Detached signature in the task API table to bind T
consistently: use a result-producing callback returning handle[T] if detached
handles preserve task results, or make both the callback and handle void if
detached tasks are void-only. Align the surrounding prose and task.supervise
contract with the selected behavior.
- Around line 3807-3817: Align the sendability predicate in
docs/specification.md at lines 3807-3817 and docs/dev/implementation_roadmap.md
at lines 2696-2697: state that the three allowed cases apply to safe code
outside task.scope, then explicitly list scoped views as permitted only within
task.scope and unsafe/FFI crossings as exceptions. Keep both documents’ wording
and exception scope consistent.
- Line 2755: Update the FFI Warning in the specification to state that
#[blocking] calls are routed through the runtime’s blocking pool via
pool.blocking, and remove the claim that each call spawns a dedicated thread.
Keep the guidance about blocking C functions occupying blocking execution
capacity.
In `@ryo-backend/src/codegen.rs`:
- Around line 503-507: In the compilation flow around the sidecar lookup, stop
substituting an empty FunctionSidecar when sidecar.functions[sidecar_index] is
missing. Enforce the positional contract by validating sidecar.functions.len()
against tirs.len() before processing, or propagate an error from the lookup;
keep the compiler-generated __ryo_panic path handled separately without
requiring this fallback.
In `@ryo-backend/src/toolchain.rs`:
- Around line 96-100: Ensure the staged archive cleanup in the extraction flow
handles a failed fs::remove_file(&zip_path) before any fallback rename. In the
surrounding function, propagate the removal error as a CompilerError or
otherwise successfully delete the archive before renaming temp_path to
desired_path, preserving the invariant that the fallback toolchain directory
never contains zig-download.zip.
In `@ryo-driver/src/pipeline.rs`:
- Around line 34-55: Update get_output_filenames to return (PathBuf, PathBuf),
construct filenames directly from the input path without the file_stem "output"
fallback or to_string_lossy conversion, and adjust its callers to use PathBuf
values. Update linker::link_executable to accept &Path for the object and
executable arguments, preserving the actual non-UTF-8 paths through file writing
and linking.
In `@ryo-frontend/src/ownership.rs`:
- Around line 4093-4106: Update the test helper take_function_sidecar to accept
a TIR index instead of a StringId, and directly take sidecar.functions at that
index. Update every caller to pass the corresponding positional index from tirs,
preserving correct behavior when multiple TIRs share the same name.
In `@ryo/tests/valgrind_smoke.rs`:
- Around line 52-53: Update the RYO_SKIP_VALGRIND check in the smoke test to
read and parse the environment variable, treating only the exact value "1" as
the opt-out; keep unset, empty, "0", and "false" as non-opt-out cases.
---
Outside diff comments:
In `@docs/dev/concurrency_loom_kt.md`:
- Around line 153-167: Update the FFI architecture description around the
scheduler-owned system coroutines and call_ffi to explicitly define nested FFI
re-entry from C callbacks. Specify either how overflow coroutines are created
and managed when the read loop is occupied, or the exact rejection/deferral
behavior for the inner FFI call, and ensure the callback behavior referenced
later matches that policy.
- Around line 302-310: Update the handler around with_pool so only the database
operation within handle_request runs on db_pool; keep libjpeg.decode and other
CPU work outside the pool. Preserve the existing 16-concurrent-query bound while
preventing the pool from executing the entire request handler.
In `@docs/specification.md`:
- Around line 2967-2974: Align the detached-task cancellation boundary described
in the task.spawn_detached documentation and its example: either explicitly
define main() returning as process exit, or update the example and contract to
use the same shutdown boundary. Keep the handle and cancellation semantics
consistent in both locations.
---
Nitpick comments:
In `@docs/dev/concurrency_loom_kt.md`:
- Around line 383-387: Update the “Capability-gated pool creation” guidance for
pool.custom to define a hard worker/resource limit, including how the limit is
enforced and what authority is required. Specify the failure behavior when
creation exceeds the limit, and replace the convention-only restriction with
this enforceable contract before exposing the API.
In `@ryo-frontend/src/lexer.rs`:
- Around line 422-480: Update unescape to debug_assert! that i is a UTF-8
character boundary before slicing inner[i..]. Replace plain offset additions
used to construct the UnknownEscape diagnostic span with checked or saturating
arithmetic, preserving the existing span range and escape behavior.
In `@ryo-frontend/src/sema.rs`:
- Around line 243-263: Update resolve_signatures so signatures retains the first
function declaration for each name, matching the first-wins behavior of
name_to_decl; avoid overwriting an existing signature when processing duplicate
bodies, while continuing to analyze duplicates and emit their existing
diagnostics.
🪄 Autofix
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 Plus
Run ID: 30dd316a-b07c-4d97-950f-cd678b8d432b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/codspeed.ymlISSUES.mddocs/dev/NOTES.mddocs/dev/concurrency_loom_kt.mddocs/dev/implementation_roadmap.mddocs/specification.mdryo-backend/src/codegen.rsryo-backend/src/toolchain.rsryo-core/src/diag.rsryo-core/src/ownership.rsryo-core/src/tir.rsryo-core/src/uir.rsryo-driver/src/pipeline.rsryo-frontend/benches/frontend.rsryo-frontend/src/astgen.rsryo-frontend/src/indent.rsryo-frontend/src/lexer.rsryo-frontend/src/ownership.rsryo-frontend/src/parser.rsryo-frontend/src/sema.rsryo/Cargo.tomlryo/src/main.rsryo/tests/integration_tests.rsryo/tests/valgrind_smoke.rs
💤 Files with no reviewable changes (1)
- ISSUES.md
- codegen: missing positional sidecar entry is now a hard error instead of a silently-substituted empty FunctionSidecar; the old __ryo_panic justification was wrong (it is an imported runtime call, never a compiled body). - toolchain: a failed remove_file of the staged zig zip aborts the install instead of being ignored, so the fallback rename can never carry the archive into the toolchain dir. - driver/backend: get_output_filenames returns (PathBuf, PathBuf) built by extension replacement (no 'output' fallback, no lossy conversion); link_executable takes &Path so non-UTF-8 paths survive through writing and linking. - ownership tests: take_function_sidecar takes the positional TIR index directly instead of re-deriving it by name lookup. - valgrind smoke: only RYO_SKIP_VALGRIND=1 exactly opts out; empty, 0, or false no longer skip silently. - lexer: unescape debug_asserts the slice offset is a char boundary and builds the UnknownEscape span with saturating arithmetic. - sema: resolve_signatures keeps the first declaration's signature for a duplicated name, matching name_to_decl's first-wins rule.
concurrency_loom_kt.md: - Rename the scheduler execution-context abstraction (with_pool, pool.custom/default/blocking/compute, Pool type) to Dispatcher / with_dispatcher to end the collision with the std.pool resource-pool namespace, and state the distinction explicitly. - with_dispatcher defined as a yielding operation that may migrate the task across OS workers; lock guards may not cross it and the compiler diagnoses it inside a task.pin() critical section. - Nested FFI re-entry from C callbacks gets a policy: bounded, cached overflow system coroutines per worker (RYO_FFI_OVERFLOW_DEPTH), deferral past the bound. - libjpeg.decode safety bounded by RYO_FFI_STACK_SIZE (2 MB default) instead of 'regardless of stack usage'; notes the 128 KB limit is gone for typical libraries. - The DB example now switches dispatchers only around db.query; CPU work stays on the default dispatcher. - task.supervise children yield both the identity-only handle[T] and an awaitable Future[T] join handle; the two are not interchangeable. - Pony cross-reference data-plane rules marked draft/open; Spec impact table row updated to match. - dispatcher.custom becomes an enforceable contract: RYO_MAX_DISPATCHERS plus a 4xRYOMAXPROCS extra-worker budget, ResourceExhausted on overflow, startup/main-scope authority. - The call_ffi lowering example is now ryo-fenced Ryo pseudocode. specification.md (+ implementation_roadmap.md): - shared[T] sendability claim qualified: T must be composed of safe Ryo types, per the unsafe/FFI exceptions of 14.5.6. - Undeclared uppercase Handle references replaced with handle[T]. - spawn_detached binds T consistently: fn() -> T returning handle[T], result discarded. - Sendability predicate scoped to safe code outside task.scope, with scoped views and unsafe/FFI crossings listed as exceptions; roadmap wording aligned. - FFI warning: #[blocking] routes through the runtime's blocking pool, not a dedicated thread per call (two sibling spots aligned). - Detached-task cancellation boundary unified: main() returning is process exit.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ryo-frontend/src/lexer.rs (1)
312-316: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStop
Commentbefore\rand\n.
#[^\n]*captures the carriage return in# comment\r\n, so the followingNewlinetoken spans only\n. Change the skipped comment regex to stop immediately before both line terminators, and add a span assertion for an indented CRLF comment line.🤖 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 `@ryo-frontend/src/lexer.rs` around lines 312 - 316, Update the skipped comment regex near the Newline lexer rule so it excludes both carriage return and newline characters, ensuring CRLF comments leave \r for Newline tokenization. Add a span assertion covering an indented comment followed by CRLF, verifying the Newline token includes the \r and \n bytes.
🤖 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 `@docs/dev/concurrency_loom_kt.md`:
- Around line 12-14: Revise the proposal’s introductory claim and the “Spec
impact” table to distinguish proposal-only APIs from normative language
behavior. Mark Dispatcher, with_dispatcher, and task.supervise as proposal-only
unless their definitions are added to the normative specification; do not
describe them as stable user-facing APIs.
- Around line 392-398: Qualify the “Per-carrier reclamation locality” principle
in the concurrency design section: do not present home-context reclamation as
guaranteed by mimalloc thread-local heaps. Either specify the remote-free or
deferred-reclamation mechanism used when channel ownership transfers or
with_dispatcher migrates tasks, or rewrite the statement as a desired
reclamation-locality goal.
- Line 170: Update the nested FFI re-entry policy in the documented
overflow-depth handling so exhaustion of RYO_FFI_OVERFLOW_DEPTH fails
immediately with an explicit re-entry-limit error or uses a non-blocking
fallback; do not queue the call and suspend the current task. Preserve overflow
coroutine reuse and bounded per-worker behavior below the limit.
- Around line 363-368: Update the task.supervise documentation to use the
built-in lowercase future[T] type for awaitable child results, replacing the
unquoted Future[T] references while preserving the distinction from handle[T].
- Around line 399-410: Correct the “Capability-gated dispatcher creation”
documentation so it does not claim the global numeric limits enforce
capability-based authority before capability injection. Either document actual
capability or per-caller quota enforcement before exposing dispatcher.custom, or
remove the claim that the contract holds without the capability check while
retaining the accurate global-limit behavior.
- Around line 303-312: Clarify the dispatcher precedence rule in the
`with_dispatcher` documentation: state whether explicit selection of
`db_dispatcher` overrides `#[blocking]` auto-routing. Align the `handle_request`
database example and nearby `db.query` descriptions so they do not claim bounded
execution if `#[blocking]` takes precedence, or explicitly document that the
block’s dispatcher wins.
In `@docs/specification.md`:
- Line 2755: Remove runtime blocking-pool and OS-thread routing details from the
FFI warning at docs/specification.md lines 2755-2755, retaining only the
user-visible requirement to use #[blocking] for blocking FFI calls. Also remove
worker, pool, and green-thread architecture details from the normative solution
text at docs/specification.md lines 3768-3775; keep docs/specification.md
focused strictly on language behavior without references to docs/dev/.
---
Outside diff comments:
In `@ryo-frontend/src/lexer.rs`:
- Around line 312-316: Update the skipped comment regex near the Newline lexer
rule so it excludes both carriage return and newline characters, ensuring CRLF
comments leave \r for Newline tokenization. Add a span assertion covering an
indented comment followed by CRLF, verifying the Newline token includes the \r
and \n bytes.
🪄 Autofix
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 Plus
Run ID: dcf8282d-4b9f-4d23-bbc2-c20b6592295d
📒 Files selected for processing (11)
docs/dev/concurrency_loom_kt.mddocs/dev/implementation_roadmap.mddocs/specification.mdryo-backend/src/codegen.rsryo-backend/src/linker.rsryo-backend/src/toolchain.rsryo-driver/src/pipeline.rsryo-frontend/src/lexer.rsryo-frontend/src/ownership.rsryo-frontend/src/sema.rsryo/tests/valgrind_smoke.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- ryo/tests/valgrind_smoke.rs
- docs/dev/implementation_roadmap.md
- ryo-backend/src/toolchain.rs
- ryo-frontend/src/sema.rs
- ryo-frontend/src/ownership.rs
- ryo-driver/src/pipeline.rs
- ryo-backend/src/codegen.rs
The needles 'IoError' and 'Error:' matched the old std Termination
debug print; since main now prints Display-style 'error: {e}', and
Windows' OS message is 'The system cannot find the file specified.'
rather than 'No such file or directory', none of the needles matched
on windows-latest. Assert our own stable 'IO error' prefix from
CompilerError's Display instead.
lexer: - Comment regex now excludes \r as well as \n, so in CRLF files the \r\n always stays inside the Newline token's span (previously a trailing comment swallowed the \r). New span assertion test covers an indented comment followed by CRLF. concurrency_loom_kt.md: - Dispatcher, with_dispatcher, and task.supervise marked proposal-only in the intro and the Spec impact table — not stable user-facing APIs until added to the normative spec. - Reclamation locality restated as a goal, not a guarantee: cross- thread frees from channel transfers and dispatcher migration go through mimalloc's remote-free path, so correctness holds when locality does not. - Nested FFI re-entry: past RYO_FFI_OVERFLOW_DEPTH the inner call now fails immediately with an explicit re-entry-limit error instead of queueing and suspending (the suspended task may be what the occupied coroutines wait on — queueing risks deadlock). - task.supervise join handle uses the built-in lowercase future[T]. - Capability-gated dispatcher creation: global numeric limits are resource-exhaustion protection, not authority; the claim that the contract holds without the capability check is removed. - Precedence rule stated: an enclosing with_dispatcher block overrides #[blocking] auto-routing for the block's duration; the db_dispatcher example's 16-concurrent bound now cites it explicitly. specification.md: - FFI warning and FFI Blocking Annotation section reduced to the user-visible requirement (mark blocking FFI with #[blocking]); blocking-pool/OS-thread routing details removed from the normative text (they live in docs/dev/).
The Loom/Kotlin alternative proposal wins the comparison (as recommended by ryo-proposal-review-issues.md §5): the delta doc is rewritten as a standalone implementation plan and renamed to concurrency.md, replacing the earlier draft. - All phases 1-6 from the old plan folded in, with the adopted changes applied inline: system-coroutine FFI router (§3.5) with overflow re-entry policy, Dispatcher + with_dispatcher (§4.5), task.supervise (§4.2.1), four channel modes (§4.1), pool_drained telemetry (§5.5), with_dispatcher in the effect analysis (§6.2). - Tunables, dependency summary, out-of-scope, WasmFX future section, risk register (4 new rows), and milestone summary all carried over and updated. - Delta framing, comparison tables, and open-questions section dropped; their recommendations are now simply the plan. - README index updated; the concurrency_loom_kt.md row is gone. Existing links to concurrency.md in wasm_target, go_reference, rust_reference, and memory_model_comparison now resolve to the merged plan.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/dev/concurrency.md`:
- Around line 115-119: Tag both fenced code blocks in docs/dev/concurrency.md at
lines 115-119 and 417-422 with the ryo language identifier by updating each
opening fence; make no other documentation changes.
🪄 Autofix
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 Plus
Run ID: fbc8aec9-c959-44d6-a00a-8475c91fface
📒 Files selected for processing (9)
docs/dev/README.mddocs/dev/concurrency.mddocs/dev/concurrency_loom_kt.mddocs/specification.mdryo-driver/src/pipeline.rsryo-frontend/src/astgen.rsryo-frontend/src/lexer.rsryo-frontend/src/ownership.rsryo-frontend/src/sema.rs
💤 Files with no reviewable changes (1)
- docs/dev/concurrency_loom_kt.md
🚧 Files skipped from review as they are similar to previous changes (6)
- ryo-frontend/src/sema.rs
- docs/specification.md
- ryo-frontend/src/astgen.rs
- ryo-driver/src/pipeline.rs
- ryo-frontend/src/ownership.rs
- ryo-frontend/src/lexer.rs
Comment on lines
115
to
119
| ``` | ||
| Future<T>: | ||
| future[T]: | ||
| Drop → sends CancelRequest to task | ||
| .await → suspends caller until task completes, returns T | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tag both Ryo code fences.
Add the ryo language tag to both fenced code blocks. This satisfies the documentation rendering rule and resolves MD040.
docs/dev/concurrency.md#L115-L119: change the opening fence to```ryo.docs/dev/concurrency.md#L417-L422: change the opening fence to```ryo.
As per coding guidelines, documentation code examples must use fenced code blocks tagged ryo.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 1 file
docs/dev/concurrency.md#L115-L119(this comment)docs/dev/concurrency.md#L417-L422
🤖 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 `@docs/dev/concurrency.md` around lines 115 - 119, Tag both fenced code blocks
in docs/dev/concurrency.md at lines 115-119 and 417-422 with the ryo language
identifier by updating each opening fence; make no other documentation changes.
Sources: Coding guidelines, Linters/SAST tools
- ISSUES.md: drop the now-empty 'Fix Order for m8.4.2' section — the milestone is complete and remaining entries are the plain backlog. - ryo-proposal-review-issues.md: record the concurrency promotion and which of L-1/L-2/L-3/L-6 were resolved en route, so the memo's action items aren't re-acted on. - benchmarks/.gitignore: ignore the AOT binaries 'ryo build' now writes next to benchmark sources (I-084), keeping local builds out of git status.
The positional sidecar (one entry per Tir, indexed by body) cannot detect a tirs slice reordered or filtered between ownership::check and codegen: every index resolves to a wrong-but-plausible entry and one function's frees silently fire in another. FunctionSidecar now records the Tir's name at push time and codegen debug_assert_eq!s it against tir.name after the positional lookup. Default is deliberately underivable so a nameless sidecar cannot be constructed.
Resolved ISSUES.md entries are deleted, so comments citing I-XXX become dangling pointers to context that no longer exists. Removes the citations (three already dangling, three for an open issue that would dangle on resolution) and adds a CLAUDE.md rule steering IDs to commit messages, where they survive in git history. Also updates docs/dev/NOTES.md.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ryo-core/src/types.rs`:
- Line 566: Update the safety comment near intern_str to use the exact `//
SAFETY:` prefix, preserving the existing invariant explanation after it.
🪄 Autofix
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 Plus
Run ID: 8cb79c56-5c01-401d-b9c8-028b0070dbfa
📒 Files selected for processing (12)
CLAUDE.mdISSUES.mdbenchmarks/.gitignoredocs/dev/NOTES.mddocs/dev/ryo-proposal-review-issues.mdryo-backend/src/codegen.rsryo-core/src/ownership.rsryo-core/src/types.rsryo-core/src/uir.rsryo-frontend/src/ownership.rsryo-frontend/src/parser.rsryo/tests/valgrind_smoke.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- ryo/tests/valgrind_smoke.rs
- ryo-core/src/uir.rs
- docs/dev/ryo-proposal-review-issues.md
- ISSUES.md
- ryo-backend/src/codegen.rs
- docs/dev/NOTES.md
- ryo-frontend/src/parser.rs
- benchmarks/.gitignore
- ryo-frontend/src/ownership.rs
| let (offset, len) = self.strings[id.0 as usize]; | ||
| let bytes = &self.string_bytes[offset as usize..(offset + len) as usize]; | ||
| // SAFETY (R5 exception, I-127): `intern_str` only ever pushes valid | ||
| // SAFETY (R5 exception): `intern_str` only ever pushes valid |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required safety-comment prefix.
Change the comment prefix to // SAFETY:. Keep the invariant explanation after the prefix.
Proposed fix
- // SAFETY (R5 exception): `intern_str` only ever pushes valid
+ // SAFETY: `intern_str` only ever pushes valid📝 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.
Suggested change
| // SAFETY (R5 exception): `intern_str` only ever pushes valid | |
| // SAFETY: `intern_str` only ever pushes valid |
🤖 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 `@ryo-core/src/types.rs` at line 566, Update the safety comment near intern_str
to use the exact `// SAFETY:` prefix, preserving the existing invariant
explanation after it.
Source: Coding guidelines
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.
Fixes every issue in the
## Fix Order for m8.4.2list from ISSUES.md, one commit per issue, in fix order. Resolved entries are removed from ISSUES.md per convention.Correctness
Sema::new\r?\n,\rskipped in indentation)get_output_filenames;.ocleaned up on link failureemit_oneno longer panics mid-report (ExitCodemain)i64::MINis spellable via a dedicatedToken::IntLitMinzig-download.zipdeleted after extract (fallback rename could carry it into the toolchain dir)Lexer / diagnostics hygiene
lex()routes everything throughDiagSinkwith recovery;LexErroreliminated;E0102/E0103codes; pool-awarerich_error_messagefinalize_diags(Vec<Diag>)generalization; all lex/parse error paths routed through itRefactors
Vec) instead of interned function nameDocs / process
inst_valuesinvariant documentedRYO_SKIP_VALGRIND=1Verification
cargo test --workspacegreen (197 integration tests + all unit suites)cargo clippy --workspace --all-targets -- -D warningscleanscripts/run_linux_tests.shSummary by CodeRabbit
New Features
Bug Fixes
Documentation