Real GraphQL executor, native List[T], Anneal; 0.4.16#23
Conversation
Two memory-safety bugs in owned slice fields (both surfaced by collections_mako_test on the native backend): - `self.xs = self.xs[a:b]` stored a view that aliases the field's own buffer, then freed that buffer while overwriting — use-after-free (garbage reads, SIGABRT). Clone the view's data into an independent slice before dropping the old field, matching the local-assignment path. - `self.xs = append(self.xs, v)` leaked the original field header/buffer. A field read is a borrow, so append clones it and consumes the clone; the stale "consumes old field" exception then skipped dropping the original. Always drop the old field. Affects every slice-backed struct (stacks, lists, queues). Native corpus now 376/376; every native fixture is 0-leak and GuardMalloc-clean.
Replace the terse ADAPTIVE_OPT.md draft with clear prose describing the offline feedback loop: a fully compiled binary plus opt-in hot-site counters (off by default, 256 sites) exported at /debug/hot_sites and folded into the next build via an offline PGO cycle. Frame it as ahead-of-time with an offline loop; drop the runtime-code-generation phrasing from the README compilation section.
GraphQL: replace the placeholder resolver with a real recursive executor
behind graphql_schema_resolve (both C and native backends, no ABI change):
nested projection over objects/arrays, aliases, arguments + operation
variable defaults, __typename, __schema/__type introspection, named and
inline fragments, field validation, and spec-shaped {data,errors} output.
Fix a pre-existing per-request leak: resolve results and owned query-arg
temporaries were never reclaimed (allowlist owned-string-returning builtins
in scope-drop cleanup; gate the query-arg free by ownership to avoid double
free). Verified leak-free per request and ASan/UBSan-clean on Linux; new
graphql_exec_test.mko (13 cases).
Native backend: List[T] now lowers for any element type (structs, nested
List[List[int]], strings, pointer arrays) by aliasing []T instead of erroring
for non-scalar T; new native_list_generic_test.mko, differential vs C.
Anneal: name the adaptive-optimization loop; add scripts/anneal-cycle.sh over
adaptive-opt-cycle.sh (stable hot_site_* API unchanged); broaden hot_site_test
with boundary ids, top reassignment, enable transitions, and concurrent
atomic-counter tallies.
Version: 0.4.15 -> 0.4.16; update CHANGELOG, docs, and tip markers.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis release bumps Mako to 0.4.16, adds a recursive GraphQL executor, broadens native ChangesGraphQL query execution
Generic native lists
Anneal and 0.4.16 release
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant mako_graphql_schema_resolve
participant GraphQLSchema
participant Resolver
Caller->>mako_graphql_schema_resolve: Query and variables
mako_graphql_schema_resolve->>GraphQLSchema: Parse selections and validate fields
GraphQLSchema->>Resolver: Resolve requested field
Resolver-->>mako_graphql_schema_resolve: JSON payload
mako_graphql_schema_resolve-->>Caller: data and errors response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/native_ir.rs (1)
15496-15500: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc comment left over after removing
field_base_same_local.
/// True when both expressions name the same local (field-assign consume).now sits directly abovefn drop_owned_locals, an unrelated function, since the helper it documented was deleted. This is misleading and should be removed as part of this cleanup.🧹 Suggested cleanup
} - /// True when both expressions name the same local (field-assign consume). - fn drop_owned_locals(&mut self) {🤖 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/native_ir.rs` around lines 15496 - 15500, Remove the stale doc comment immediately before drop_owned_locals, since it documents the deleted field_base_same_local helper rather than drop_owned_locals. Leave the function implementation unchanged.
🧹 Nitpick comments (2)
src/native_ir.rs (2)
1478-1482: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect List[T] fix — but leaves the old scalar-only List[T] arm in
scalar_typedead.This new arm in
resolve_typecorrectly aliasesList[T]to full slice resolution (structs, nested lists, pointer arrays), matching the PR goal. However,scalar_typestill has its own narrowerList[T]arm (lines ~1234-1246, int/string/float/byte/bool only). Sinceresolve_typenow intercepts everyList[T]before reaching the_ => scalar_type(ty)fallback (line 1499), that arm inscalar_typeis unreachable for any type resolved throughresolve_type— the only path that reachesscalar_typewith aListgeneric.♻️ Suggested cleanup
- // List[T] aliases []T for the common int/string specializations. - TypeExpr::Generic(name, args) if name == "List" && args.len() == 1 => { - match args[0] { - TypeExpr::Named(ref n) if n == "int" || n == "int64" => Ok(Type::IntSlice), - TypeExpr::Named(ref n) if n == "string" => Ok(Type::StrSlice), - TypeExpr::Named(ref n) if n == "float" || n == "float64" => Ok(Type::FloatSlice), - TypeExpr::Named(ref n) if n == "byte" || n == "uint8" => Ok(Type::ByteSlice), - TypeExpr::Named(ref n) if n == "bool" => Ok(Type::BoolSlice), - _ => Err(IrError::new(format!( - "native IR: List[{}] is not supported yet", - args[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 `@src/native_ir.rs` around lines 1478 - 1482, Remove the obsolete scalar-only List[T] match arm from scalar_type, since resolve_type now handles every one-parameter List generic by converting it to an Array before the scalar_type fallback. Preserve scalar_type’s remaining scalar resolution behavior and let unsupported cases continue through its existing fallback.
3846-3905: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrectly fixes both the reslice UAF and the append leak — consider adding a regression test.
Traced the two target bugs end-to-end:
self.xs = self.xs[a:b]: the materialize block clones the borrowed view before dropping the temporary view header and before loading/dropping the old field, so the clone always reads live backing. No UAF.self.xs = append(self.xs, v): append'ssource_ownedcheck only special-casesExpr::Identsources, so a field-sourced arg is always cloned inside append. The old field is therefore never truly consumed, and the new unconditional "always drop old field" logic correctly frees it instead of leaking it.Both fixes look sound. Given this is a memory-safety fix (UAF + leak), it would be valuable to add an IR-level unit test for these two FieldAssign patterns here, mirroring the existing style used for local scope (e.g.
drops_borrowed_slice_headers_and_clones_escaping_views,clones_and_drops_int_slice_struct_fields) — asserting aSliceClone/DropSlicepair around the reslice, and aDropSliceafterappendinto a field.🤖 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/native_ir.rs` around lines 3846 - 3905, Add IR-level regression tests for the field-assignment paths in the existing native IR test module, mirroring drops_borrowed_slice_headers_and_clones_escaping_views and clones_and_drops_int_slice_struct_fields. Cover self.xs = self.xs[a:b] by asserting SliceClone occurs before DropSlice for the temporary view, and cover self.xs = append(self.xs, v) by asserting the old field receives a DropSlice after the append assignment.
🤖 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/ADAPTIVE_OPT.md`:
- Line 26: Update the availability statement in ADAPTIVE_OPT to distinguish
hot-site/offline-PGO capability availability in 0.4.14 from the Anneal naming
introduced in 0.4.16, rather than claiming all functionality starts in 0.4.15.
In `@docs/NATIVE_COMPILER_PLAN.md`:
- Around line 137-141: Reconcile the nested-list status between the completed
“3-List” entry and the deferred note around nested slices. Since the entry
claims List[List[int]] support, update the deferred note to remove or qualify
nested slices accordingly, or narrow the completion claim if nested lists remain
unsupported.
In `@runtime/mako_std.h`:
- Around line 4217-4222: Update mako_gqlx_qws to skip GraphQL comments from '#'
through the next newline, in addition to existing whitespace and commas. Ensure
mako_gqlx_qbalance handles comments encountered inside balanced argument blocks,
reusing the same comment-skipping behavior so comments work before operations,
fields, selections, and arguments. Add a regression test covering these comment
positions.
---
Outside diff comments:
In `@src/native_ir.rs`:
- Around line 15496-15500: Remove the stale doc comment immediately before
drop_owned_locals, since it documents the deleted field_base_same_local helper
rather than drop_owned_locals. Leave the function implementation unchanged.
---
Nitpick comments:
In `@src/native_ir.rs`:
- Around line 1478-1482: Remove the obsolete scalar-only List[T] match arm from
scalar_type, since resolve_type now handles every one-parameter List generic by
converting it to an Array before the scalar_type fallback. Preserve
scalar_type’s remaining scalar resolution behavior and let unsupported cases
continue through its existing fallback.
- Around line 3846-3905: Add IR-level regression tests for the field-assignment
paths in the existing native IR test module, mirroring
drops_borrowed_slice_headers_and_clones_escaping_views and
clones_and_drops_int_slice_struct_fields. Cover self.xs = self.xs[a:b] by
asserting SliceClone occurs before DropSlice for the temporary view, and cover
self.xs = append(self.xs, v) by asserting the old field receives a DropSlice
after the append assignment.
🪄 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 Plus
Run ID: 135881cb-0f5f-4424-95dc-fce97d8aac5c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
CHANGELOG.mdCargo.tomlREADME.mddocs/ADAPTIVE_OPT.mddocs/LONG_RUNNING.mddocs/MESSAGING_GRAPHQL.mddocs/NATIVE_COMPILER_PLAN.mddocs/PERFORMANCE.mddocs/ROADMAP.mddocs/SPEED_SAFE.mddocs/STATUS.mddocs/VERSIONING.mdexamples/testing/graphql_exec_test.mkoexamples/testing/hot_site_test.mkoexamples/testing/native_list_generic_test.mkoruntime/mako_std.hscripts/adaptive-opt-cycle.shscripts/anneal-cycle.shsrc/codegen/mod.rssrc/native_ir.rs
| binary, let it tell you (cheaply) which paths actually run hot in production, | ||
| then fold that knowledge into the *next* build and swap it in. The process never | ||
| patches itself; the next artifact is simply better shaped for the work it's | ||
| doing. Available in **0.4.15+**. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the availability claim with the documented release timeline.
This says 0.4.15+, while the roadmap places hot-site/offline-PGO support in 0.4.14 and the Anneal naming in 0.4.16. Distinguish capability availability from the new name.
Proposed wording
- doing. Available in **0.4.15+**.
+ doing. Hot-site/PGO support is available in **0.4.14+**; the Anneal name
+ arrives in **0.4.16**.📝 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.
| doing. Available in **0.4.15+**. | |
| doing. Hot-site/PGO support is available in **0.4.14+**; the Anneal name | |
| arrives in **0.4.16**. |
🤖 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/ADAPTIVE_OPT.md` at line 26, Update the availability statement in
ADAPTIVE_OPT to distinguish hot-site/offline-PGO capability availability in
0.4.14 from the Anneal naming introduced in 0.4.16, rather than claiming all
functionality starts in 0.4.15.
| - *3-List [done]* — `List[T]` is a transparent alias for `[]T` and lowers for | ||
| **any** element type (structs, nested `List[List[int]]`, strings, pointer | ||
| arrays), reusing the full slice support instead of the scalar-only set. | ||
| Fixture `examples/testing/native_list_generic_test.mko`, differentially | ||
| valid vs the C backend. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the nested-list support status.
This entry marks List[List[int]] as supported, but Line 184 still says nested slices are deferred. Since List[T] is a transparent alias for []T, update the deferred note or narrow this completion claim.
🤖 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/NATIVE_COMPILER_PLAN.md` around lines 137 - 141, Reconcile the
nested-list status between the completed “3-List” entry and the deferred note
around nested slices. Since the entry claims List[List[int]] support, update the
deferred note to remove or qualify nested slices accordingly, or narrow the
completion claim if nested lists remain unsupported.
| static size_t mako_gqlx_qws(MakoString q, size_t i) { | ||
| while (i < q.len && (q.data[i] == ' ' || q.data[i] == '\t' || q.data[i] == '\n' | ||
| || q.data[i] == '\r' || q.data[i] == ',')) | ||
| i++; | ||
| return i; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '#' --type-add 'mko:*.mko' -t mko examples/testing/graphql_exec_test.mko | rg -n '#' | head -50
fd -e mko . examples/testing | xargs rg -nl 'query|mutation' 2>/dev/nullRepository: loreste/mako
Length of output: 828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## files of interest\n'
git ls-files | rg '(^|/)runtime/mako_std\.h$|graphql(_exec|_http)?test\.mk|graphql' | head -120
printf '\n## qws/qbalance/lexical helpers around runtime/mako_std.h:4180-4260\n'
sed -n '4180,4260p' runtime/mako_std.h | nl -ba -v4180
printf '\n## mako_gqlx_qws usages and scanner loop context\n'
rg -n 'mako_gqlx_qws|mako_gqlx_id0|mako_gqlx_qbalance|mako_gqlx_qid|qbalance|qws' runtime/mako_std.hRepository: loreste/mako
Length of output: 583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## qws/qbalance/tokenizer helpers in runtime/mako_std.h\n'
sed -n '4180,4260p' runtime/mako_std.h | sed -n '=' | xargs -I{} sh -c 'echo "---- lines {} ----"; sed -n "{ {}, {}+35p" runtime/mako_std.h | cat -n'
printf '\n## relevant tests containing GraphQL with # comments\n'
for f in examples/testing/graphql_exec_test.mko examples/testing/graphql_http_test.mko examples/testing/graphql_seed_test.mko examples/testing/lang_queue_graphql_test.mko std/graphql/graphql.mko std/graphql/schema.mko; do
[ -f "$f" ] || continue
printf '\n### %s\n' "$f"
rg -n '^#|[0-9]+:#|query|mutation|query\s*\(|mutation\s*\(' "$f" | head -40 || true
done
printf '\n## all # comments in GraphQL-related files\n'
rg -n '^#|[0-9]+:#' examples/testing/graphql_exec_test.mko examples/testing/graphql_http_test.mko examples/testing/graphql_seed_test.mko examples/testing/lang_queue_graphql_test.mko std/graphql/graphql.mko std/graphql/schema.mko || trueRepository: loreste/mako
Length of output: 9000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
P='runtime/mako_std.h'
printf '%s\n' 4180 4181 4182 4183 4184 4185 4186 4187 4188 4189 4190 4191 4192 4193 4194 4195 4196 4197 4198 4199 4200 4201 4202 4203 4204 4205 4206 4207 4208 4209 4210 4211 4212 4213 4214 4215 4216 4217 4218 4219 4220 4221 4222 4223 > /tmp/lines.txt
while read -r L; do
sed -n "${L}p" "$P" | cat -n
done < /tmp/lines.txt
printf '\n## mako_gqlx functions and qws usage\n'
rg -n 'mako_gqlx_qws|mako_gqlx_id0|mako_gqlx_qbalance|mako_gqlx_qid|qbalance|qws' runtime/mako_std.h
printf '\n## read-only behavioral probe: current qws on comment-containing inputs\n'
python3 - <<'PY'
def qws(q, i):
while i < len(q) and q[i] in (' ', '\t', '\n', '\r', ','):
i += 1
return i
cases = [
' { name }',
' # comment\n { name }',
' { id # comment\n name }',
' { id\nname }',
]
for q in cases:
print(repr(q), 'current qws=# ->', qws(q, 0), 'expected qws=# ->', qws(q,0), 'with comment handling ->', 0))
PYRepository: loreste/mako
Length of output: 6476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '4220,4236p' runtime/mako_std.h | cat -n
sed -n '4228,4248p' runtime/mako_std.h | cat -n
rg -n 'mako_gqlx_qbalance|MakoString|static size_t mako_gqlx_qws' runtime/mako_std.h | head -80
printf '\n## deterministic tokenizer probe from source semantics\n'
python3 - <<'PY'
def qws(q, i):
while i < len(q) and q[i] in (' ', '\t', '\n', '\r', ','):
i += 1
return i
def qws_comment(q, i):
while i < len(q):
c = q[i]
if c in (' ', '\t', '\n', '\r', ','):
i += 1
elif c == '#':
while i < len(q) and q[i] != '\n':
i += 1
else:
break
return i
cases = [
'query user { name }',
'# comment\nquery user { name }',
'query # comment\n user { name }',
'query user{ # comment\nname }',
'query(user: 1) # comment\n { name }',
]
for q in cases:
print(q)
i = qws(q, 0)
print(' current_next=', repr(q[i:i+30]), 'qws_qws=', repr(q[qws(q, i+len(q.split()[0]+(' ' if (i + len(q.split()[0]+(' ')) < len(q)) else '') else ('')) for _ in range(0): None]))
PYRepository: loreste/mako
Length of output: 7046
Skip # comments in the GraphQL lexer.
GraphQL treats # through the end of the line as insignificant to both lexing and parsing, and mako_gqlx_qws currently skips only space/tab/newline/carriage-return/comma. Queries with comments before operation names, fields, selected fields, or arguments are therefore misparsed. Skip #…\n here; mako_gqlx_id0 does not accept #, so argument parsing through mako_gqlx_qbalance also needs comment handling inside balanced (...) blocks. Add a GraphQL comment test so regressions are caught.
🤖 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/mako_std.h` around lines 4217 - 4222, Update mako_gqlx_qws to skip
GraphQL comments from '#' through the next newline, in addition to existing
whitespace and commas. Ensure mako_gqlx_qbalance handles comments encountered
inside balanced argument blocks, reusing the same comment-skipping behavior so
comments work before operations, fields, selections, and arguments. Add a
regression test covering these comment positions.
|
Review verdict: not mergeable yet. The native ownership and Critical
Important
Cleanup and coverage
Verification
|
A string/slice/map returned from a user-defined function and bound to a local was never freed when dropped (one allocation leaked per call — a per-request leak in long-running servers). Recognize such call results as owned producers in scope-drop cleanup so they free at scope exit; move analysis still suppresses the drop on return/pass-on/store. Enums are excluded (match consumes them — an unconditional free double-frees) and structs are excluded (field-by-field frees). Verified on Linux: full suite green under AddressSanitizer (no double-free/UAF), leak eliminated for []int / string / List[Struct] returns, generic-enum tests still pass. New owned_return_drop_test.mko.
…-0416 Real GraphQL executor, native List[T], Anneal; 0.4.16
3 participants
Summary
graphql_schema_resolveAPI (C + native, no ABI change): nested projection over objects/arrays, aliases, arguments + operation variable defaults,__typename,__schema/__typeintrospection, named + inline fragments, field validation, spec-shaped{"data":…,"errors":[…]}.graphql_schema_resolve(owned result + query-arg temp were never reclaimed). Allowlisted owned-string-returning builtins in scope-drop cleanup; gated the arg-free by ownership to avoid double-free.List[T]now lowers for any element type (structs, nestedList[List[int]], strings, pointer arrays) by aliasing[]T.scripts/anneal-cycle.sh; broadenedhot_site_test.mko(boundary ids, top reassignment, enable transitions, concurrent atomic-counter tallies).Verification
graphql_exec_test.mko(13 cases).List[T]: correct on both backends, C↔native differential identical, ASan/UBSan clean; newnative_list_generic_test.mko.Known follow-up (pre-existing, not introduced here)
Owned values returned from user functions and dropped locally are not reclaimed (a general per-call leak, same class as the GraphQL return leak). The naive fix (treat user-fn owned returns as scope-drop-safe) double-frees generic-enum returns, so it needs deeper move-analysis work and is left for a focused, separately-validated pass.
List[Struct]inherits the existing[]Structbehavior here — this change does not worsen it.Summary by CodeRabbit
New Features
Bug Fixes
Documentation