Deduplicate repeated Rust logic in baml_language - #4126
Closed
codeshaunted wants to merge 73 commits into
Closed
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR consolidates duplicated helpers across compiler, parser, formatter, runtime, event, bridge, LLM, SDK, and tooling crates. It also removes obsolete CLI logging, cached string hashes, unused dependencies, and repeated unsafe Salsa update implementations. ChangesCompiler and runtime consolidation
Bridges, SDKs, and tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
codeshaunted
changed the title
# Conflicts: # baml_language/crates/baml_cli/src/run_command.rs # baml_language/crates/baml_codegen_types/src/symbols.rs # baml_language/crates/baml_compiler2_mir/src/optimize.rs # baml_language/crates/baml_compiler2_ppir/src/lib.rs # baml_language/crates/baml_compiler_parser/src/parser.rs # baml_language/crates/baml_compiler_syntax/src/ast.rs # baml_language/crates/baml_type/src/runtime_ty.rs # baml_language/crates/bex_events/src/run.rs # baml_language/crates/sys_native/src/registry.rs # baml_language/crates/sys_ops/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
baml_language/crates/baml_fmt/src/ast/mod.rs (2)
26-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNew shared formatter helpers introduced without unit tests. Both sites extract previously-duplicated printing logic into new, widely-reused private helpers, but the PR adds no new tests (per PR objectives, only existing suites are relied on). As per path instructions (
**/*.rs: "Prefer writing Rust unit tests over integration tests where possible"), these are good candidates for direct unit tests given their reuse across manyPrintableimpls.
baml_language/crates/baml_fmt/src/ast/mod.rs#L26-L102: add unit tests fortry_print_single_line_comma_separatedandprint_multi_line_parenthesized_comma_separatedcovering comma/trivia edge cases (explicit vs. synthesized comma, width overflow bail-out, trailing trivia after last item).baml_language/crates/baml_fmt/src/ast/declarations.rs#L14-L49: add unit tests forprint_braced_declaration_itemscovering empty-items, single-item, and leading-blank-trim-on-first-item cases.🤖 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 `@baml_language/crates/baml_fmt/src/ast/mod.rs` around lines 26 - 102, Add focused Rust unit tests for try_print_single_line_comma_separated and print_multi_line_parenthesized_comma_separated in baml_language/crates/baml_fmt/src/ast/mod.rs, covering explicit and synthesized commas, width-overflow bailout, and trailing trivia after the final item. Also add tests for print_braced_declaration_items in baml_language/crates/baml_fmt/src/ast/declarations.rs covering empty items, a single item, and trimming leading blank space from the first item.Source: Path instructions
26-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNew shared comma-separated list printers lack dedicated unit tests.
try_print_single_line_comma_separatedandprint_multi_line_parenthesized_comma_separatedare now the single source of truth for comma-separated printing acrossAttributeArgs,CallArgs,FunctionTypeparams,ObjectInitializer, andMapLiteral. As per path instructions, prefer unit tests for new Rust logic; a bug here would ripple across all of these printers simultaneously.Also applies to: 64-102
🤖 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 `@baml_language/crates/baml_fmt/src/ast/mod.rs` around lines 26 - 63, Add dedicated unit tests for try_print_single_line_comma_separated and print_multi_line_parenthesized_comma_separated, covering comma handling, trivia, empty and single-item lists, and multiline fallback behavior. Exercise the shared printers directly or through representative AttributeArgs/CallArgs-style inputs, ensuring expected single-line and parenthesized multiline output.Source: Path instructions
🤖 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 `@baml_language/crates/baml_compiler_parser/src/parser.rs`:
- Around line 1863-1898: Update parse_quoted_content’s escape handling to use
bump_one_token_raw() for consuming the token after a backslash, matching
parse_backtick_content and ensuring escaped whitespace immediately before a
closing quote is consumed without skipping trivia. Preserve the existing escape
and termination behavior, and add Rust unit coverage for both "\<space>" and
b"\<space>" cases.
In `@baml_language/crates/baml_lsp_server/src/playground_http.rs`:
- Around line 59-87: Update the fetch handling around the asynchronous match arm
to apply the same ingest_fetch_updated and FetchLogUpdate side effects when
read(response) returns a successful synchronous SysOpOutput::Ready value.
Preserve the existing response body and metadata handling, and ensure both
wrapped output modes (text and json) are covered while errors and asynchronous
results retain their current behavior.
In `@baml_language/crates/baml_type/src/lib.rs`:
- Around line 345-377: The `strip_null` contract conflicts with its fast path,
which unwraps single-member non-null unions. Either preserve such unions
unchanged by removing or adjusting the fast path, or, if normalization is
intentional, update `strip_null`’s documentation and add a targeted Rust unit
test covering `Ty::union([Ty::Int])` becoming `Ty::Int`.
In `@baml_language/crates/bridge_cffi/src/ffi/handle.rs`:
- Around line 200-203: Update the safety documentation for the FFI function
around out_key and out_type, including the corresponding contracts at the
additionally affected declarations, to state that both output pointers are
required and must be valid for writing one pointee value; remove the allowance
that either pointer may be null, matching the existing UnexpectedNullptr
validation.
---
Nitpick comments:
In `@baml_language/crates/baml_fmt/src/ast/mod.rs`:
- Around line 26-102: Add focused Rust unit tests for
try_print_single_line_comma_separated and
print_multi_line_parenthesized_comma_separated in
baml_language/crates/baml_fmt/src/ast/mod.rs, covering explicit and synthesized
commas, width-overflow bailout, and trailing trivia after the final item. Also
add tests for print_braced_declaration_items in
baml_language/crates/baml_fmt/src/ast/declarations.rs covering empty items, a
single item, and trimming leading blank space from the first item.
- Around line 26-63: Add dedicated unit tests for
try_print_single_line_comma_separated and
print_multi_line_parenthesized_comma_separated, covering comma handling, trivia,
empty and single-item lists, and multiline fallback behavior. Exercise the
shared printers directly or through representative AttributeArgs/CallArgs-style
inputs, ensuring expected single-line and parenthesized multiline output.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1495ec11-9221-47e8-8892-52b0f87bbc49
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (83)
baml_language/crates/baml/src/main.rsbaml_language/crates/baml_base/src/attr.rsbaml_language/crates/baml_base/src/salsa_update.rsbaml_language/crates/baml_cli/src/paint.rsbaml_language/crates/baml_codegen_types/src/symbols.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_mir/src/optimize.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_fmt/src/ast/attributes.rsbaml_language/crates/baml_fmt/src/ast/declarations.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_fmt/src/ast/mod.rsbaml_language/crates/baml_fmt/src/ast/types.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/lib.rsbaml_language/crates/baml_lsp2_actions/src/listing.rsbaml_language/crates/baml_lsp2_actions/src/tokens.rsbaml_language/crates/baml_lsp2_actions/src/tokens/classify.rsbaml_language/crates/baml_lsp_server/Cargo.tomlbaml_language/crates/baml_lsp_server/src/native_vfs.rsbaml_language/crates/baml_lsp_server/src/playground_http.rsbaml_language/crates/baml_lsp_server/src/playground_server.rsbaml_language/crates/baml_project/src/db.rsbaml_language/crates/baml_release/src/skills.rsbaml_language/crates/baml_type/src/codegen_ty.rsbaml_language/crates/baml_type/src/lib.rsbaml_language/crates/baml_type/src/runtime_ty.rsbaml_language/crates/baml_type/src/simplify_sap.rsbaml_language/crates/baml_type_runtime/src/lib.rsbaml_language/crates/bex_engine/src/value_capture.rsbaml_language/crates/bex_events/src/framing.rsbaml_language/crates/bex_events/src/history/mod.rsbaml_language/crates/bex_events/src/lib.rsbaml_language/crates/bex_events/src/prof/read.rsbaml_language/crates/bex_events/src/run.rsbaml_language/crates/bex_events/src/value/read.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_project/src/fs.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_sap/src/deserializer/coercer/coerce_literal.rsbaml_language/crates/bex_sap/src/deserializer/coercer/mod.rsbaml_language/crates/bex_vm/src/package_baml/array.rsbaml_language/crates/bex_vm/src/package_baml/json.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/package_baml/root.rsbaml_language/crates/bridge_cffi/src/ffi/handle.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_wasm/src/error.rsbaml_language/crates/bridge_wasm/src/lib.rsbaml_language/crates/bridge_wasm/src/wasm_http.rsbaml_language/crates/sys_jinja_types/src/evaluate_type/pretty_print.rsbaml_language/crates/sys_llm/src/build_request/openai/chat_completions.rsbaml_language/crates/sys_llm/src/build_request/openai/mod.rsbaml_language/crates/sys_llm/src/build_request/openai/responses.rsbaml_language/crates/sys_llm/src/parse_response/google.rsbaml_language/crates/sys_llm/src/parse_response/openai/chat_completions.rsbaml_language/crates/sys_llm/src/parse_response/openai/mod.rsbaml_language/crates/sys_llm/src/parse_response/openai/responses.rsbaml_language/crates/sys_native/src/io_impls.rsbaml_language/crates/sys_native/src/registry.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/crates/tools_size_gate/src/compare.rsbaml_language/crates/tools_size_gate/src/main.rsbaml_language/crates/tools_size_gate/src/output.rsbaml_language/sdks/java/bridge_java/Cargo.tomlbaml_language/sdks/java/bridge_java/src/lib.rsbaml_language/sdks/java/sdkgen_java/src/emit.rsbaml_language/sdks/python/rust/bridge_python/Cargo.tomlbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/mod.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/leaf.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rsbaml_language/sdks/rust/sdkgen_rust/src/emit/function.rsbaml_language/sdks/typescript/bridge_typescript/Cargo.tomlbaml_language/sdks/typescript/bridge_typescript/src/runtime.rsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/emit/mod.rsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/leaf.rs
💤 Files with no reviewable changes (5)
- baml_language/sdks/java/bridge_java/Cargo.toml
- baml_language/sdks/typescript/bridge_typescript/Cargo.toml
- baml_language/crates/baml_lsp_server/Cargo.toml
- baml_language/crates/baml_base/src/salsa_update.rs
- baml_language/sdks/python/rust/bridge_python/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/crates/baml_compiler2_ppir/src/lib.rs
- baml_language/crates/bex_heap/src/gc.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
Comment on lines
+1863
to
1898
| p.parse_quoted_content( | ||
| "String parsing exceeded iteration limit", | ||
| "Unclosed string literal", | ||
| ); | ||
| }); | ||
|
|
||
| // Collect all tokens until closing quote. | ||
| // Use at_end_raw / at_raw / bump_raw throughout so that `*/` | ||
| // and `//` inside the string are kept as literal content instead | ||
| // of being mis-recognised as comment delimiters. | ||
| let mut loop_counter = 0; | ||
| while !p.at_end_raw() { | ||
| loop_counter += 1; | ||
| if loop_counter > 100_000 { | ||
| p.error_unexpected_token("String parsing exceeded iteration limit".to_string()); | ||
| return; | ||
| } | ||
| true | ||
| } | ||
|
|
||
| if p.at_raw(TokenKind::Backslash) { | ||
| p.bump_raw(); // Consume backslash | ||
| if p.current < p.tokens.len() { | ||
| p.bump_raw(); // Consume the escaped character (whatever it is) | ||
| } | ||
| continue; | ||
| } | ||
| /// Uses raw token navigation so comment-looking content remains literal text. | ||
| fn parse_quoted_content(&mut self, iteration_limit_error: &str, unclosed_error: &str) { | ||
| let mut loop_counter = 0; | ||
| while !self.at_end_raw() { | ||
| loop_counter += 1; | ||
| if loop_counter > 100_000 { | ||
| self.error_unexpected_token(iteration_limit_error.to_string()); | ||
| return; | ||
| } | ||
|
|
||
| if p.at_raw(TokenKind::Quote) { | ||
| p.bump_raw(); // Consume closing quote | ||
| return; | ||
| if self.at_raw(TokenKind::Backslash) { | ||
| self.bump_raw(); | ||
| if self.current < self.tokens.len() { | ||
| self.bump_raw(); | ||
| } | ||
| p.bump_raw(); | ||
| continue; | ||
| } | ||
|
|
||
| p.error_unexpected_token("Unclosed string literal".to_string()); | ||
| }); | ||
| if self.at_raw(TokenKind::Quote) { | ||
| self.bump_raw(); | ||
| return; | ||
| } | ||
| self.bump_raw(); | ||
| } | ||
|
|
||
| true | ||
| self.error_unexpected_token(unclosed_error.to_string()); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
parse_quoted_content's escape handling can overshoot into the closing quote.
The second bump_raw() for the escape target doesn't consume exactly one token — bump_raw/bump_impl(false) still skips leading basic trivia (whitespace/newline) before landing on a real token. So a literal like "\<space>" (escaped trailing space right before the closing quote) will have the space skipped as "leading trivia" and the closing " consumed as the escape target instead, leaving the string unclosed/mis-tokenized.
This is the exact same class of bug already found and fixed elsewhere in this file for backtick strings (parse_backtick_content, "ultrareview bug_011"), which deliberately uses bump_one_token_raw() for this reason. parse_quoted_content (now shared by parse_string and parse_byte_string) should do the same.
🐛 Proposed fix
if self.at_raw(TokenKind::Backslash) {
self.bump_raw();
if self.current < self.tokens.len() {
- self.bump_raw();
+ self.bump_one_token_raw();
}
continue;
}As per path instructions, **/*.rs: "Prefer writing Rust unit tests over integration tests where possible" — a regression test for "\<space>" and b"\<space>" right before the closing quote would pin this fix.
Also applies to: 1925-1929
🤖 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 `@baml_language/crates/baml_compiler_parser/src/parser.rs` around lines 1863 -
1898, Update parse_quoted_content’s escape handling to use bump_one_token_raw()
for consuming the token after a backslash, matching parse_backtick_content and
ensuring escaped whitespace immediately before a closing quote is consumed
without skipping trivia. Preserve the existing escape and termination behavior,
and add Rust unit coverage for both "\<space>" and b"\<space>" cases.
Source: Path instructions
Comment on lines
+59
to
+87
| match (fetch_info, read(response)) { | ||
| (Some((host_call_id, fetch_id)), SysOpOutput::Async(fut)) => { | ||
| SysOpOutput::async_op_with_throw(async move { | ||
| let body = fut.await?; | ||
| let (body_size, response_body) = describe(&body); | ||
| if let Some(patch) = state.run_store.ingest_fetch_updated( | ||
| &HostCallId::Native(host_call_id), | ||
| fetch_id, | ||
| None, | ||
| None, | ||
| Vec::new(), | ||
| Some(body_size), | ||
| None, | ||
| ) { | ||
| broadcast_run_patch(&state.broadcast_tx, &patch); | ||
| } | ||
| let _ = state.broadcast_tx.send(WsOutMessage::FetchLogUpdate { | ||
| call_id: host_call_id.0, | ||
| log_id: fetch_id, | ||
| status: None, | ||
| duration_ms: None, | ||
| response_headers: None, | ||
| response_body: Some(response_body), | ||
| error: None, | ||
| }); | ||
| Ok(body) | ||
| }) | ||
| } | ||
| (_, native_result) => native_result, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 4 'enum SysOpOutput|SysOpOutput::Ready|fn read_response_body' baml_languageRepository: BoundaryML/baml
Length of output: 13917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant playground_http implementation around the wrapper and caller paths.
sed -n '1,120p' baml_language/crates/baml_lsp_server/src/playground_http.rs
echo '--- caller section ---'
sed -n '360,405p' baml_language/crates/baml_lsp_server/src/playground_http.rs
# Find tests for read_response_body/playground http wrappers.
printf '\nTests matching read_response_body/playground_http:\n'
rg -n "read_response_body|PlaygroundHttp|FetchLogUpdate|ingest_fetch_updated" baml_language/crates/baml_lsp_server/tests baml_language/crates/baml_lsp_server/src || trueRepository: BoundaryML/baml
Length of output: 9494
Handle successful synchronous fetch responses.
The text and json wrapper branches convert successful synchronous responses to SysOpOutput::Ready, so successful synchronous reads fall through read_response_body without ingest_fetch_updated/FetchLogUpdate updates. Apply the same side effects to the ready-success path and cover both output modes.
🤖 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 `@baml_language/crates/baml_lsp_server/src/playground_http.rs` around lines 59
- 87, Update the fetch handling around the asynchronous match arm to apply the
same ingest_fetch_updated and FetchLogUpdate side effects when read(response)
returns a successful synchronous SysOpOutput::Ready value. Preserve the existing
response body and metadata handling, and ensure both wrapped output modes (text
and json) are covered while errors and asynchronous results retain their current
behavior.
Comment on lines
+345
to
377
| /// Returns the non-null payload of a nullable union. | ||
| /// | ||
| /// `Union([Null])` and unions without a direct `Null` member return `None`. | ||
| /// Member order and rebuilt-union attributes are preserved without flattening nested unions. | ||
| pub fn nullable_non_null_part(&self) -> Option<Ty> { | ||
| let Ty::Union(members, attr) = self else { | ||
| return None; | ||
| }; | ||
| if !members.iter().any(Ty::is_null) { | ||
| return None; | ||
| } | ||
| let non_null: Vec<Ty> = members.iter().filter(|m| !m.is_null()).cloned().collect(); | ||
| match non_null.len() { | ||
| 0 => None, | ||
| 1 => non_null.into_iter().next(), | ||
| _ => Some(Ty::Union(non_null, attr.clone())), | ||
| } | ||
| } | ||
|
|
||
| /// Remove `null` from a nullable union, collapsing the result: `T | null` | ||
| /// → `T`, `A | B | null` → `A | B`, a non-nullable type → unchanged. The | ||
| /// inverse direction of [`Ty::optional`]; used where the non-null payload | ||
| /// of an optional is needed (e.g. union-member metadata). | ||
| pub fn strip_null(&self) -> Ty { | ||
| match self { | ||
| Ty::Union(members, attr) => { | ||
| let non_null: Vec<Ty> = members.iter().filter(|m| !m.is_null()).cloned().collect(); | ||
| match non_null.len() { | ||
| 0 => self.clone(), | ||
| 1 => non_null.into_iter().next().expect("len checked"), | ||
| _ => Ty::Union(non_null, attr.clone()), | ||
| } | ||
| Ty::Union(members, _) if members.len() == 1 && !members[0].is_null() => { | ||
| members[0].clone() | ||
| } | ||
| _ => self.clone(), | ||
| _ => self | ||
| .nullable_non_null_part() | ||
| .unwrap_or_else(|| self.clone()), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: check for existing single-element Ty::Union construction sites and strip_null tests.
rg -nP 'Ty::union\(\[[^,\]]+\]\)|Ty::Union\(vec!\[[^,\]]+\]' baml_language/crates -g '*.rs'
rg -n 'fn strip_null_|nullable_non_null_part' baml_language/crates/baml_type/src/lib.rs -A3 -B3Repository: BoundaryML/baml
Length of output: 722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== baml_type lib relevant lines =="
sed -n '1,120p' baml_language/crates/baml_type/src/lib.rs
sed -n '300,390p' baml_language/crates/baml_type/src/lib.rs
echo "== Ty union definitions/usages =="
rg -n 'pub fn union|pub fn from_union|fn union|pub enum Ty|struct.*Union|is_nullable_union|nullable_non_null_part|strip_null|Ty::Union' baml_language/crates/baml_type/src/lib.rs baml_language/crates -g '*.rs' | head -200Repository: BoundaryML/baml
Length of output: 30491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Ty dedup_and_collapse implementation =="
sed -n '100,150p' baml_language/crates/baml_type/src/lib.rs
echo "== Ty union() implementation and reachable direct constructors =="
sed -n '440,465p' baml_language/crates/baml_type/src/lib.rs
echo "== single-element nullable_non_null_part behavior behavior probe =="
python3 - <<'PY'
from pathlib import Path
p = Path("baml_language/crates/baml_type/src/lib.rs")
s = p.read_text()
# Behavioral probe mirroring the documented/implemented contract from source.
def is_nullable_union(members):
return any(m == "Null" for m in members)
def nullable_non_null_part(members, attr):
if not is_nullable_union(members):
return None
non_null = [m for m in members if m != "Null"]
if not non_null:
return None
if len(non_null) == 1:
return non_null[0]
return ("Union", non_null, attr)
def strip_null(members, attr):
if len(members) == 1 and members[0] != "Null":
return members[0], attr
payload = nullable_non_null_part(members, attr)
return (payload, attr) if payload is not None else (("Union", members, attr), attr)
cases = {
"non_nullable_Single": (["Int"], "attr"),
"nullable_single_after_null_strip": (["Int", "Null"], "attr"),
"nullable_multi_after_null_strip": (["Int", "Null", "String"], "attr"),
}
for name, (members, attr) in cases.items():
nullable = is_nullable_union(members)
result, out_attr = strip_null(members, attr)
changed = result != (("Union", members, attr))
print(f"{name}: nullable={nullable}, input={members}, output={result}, changed={changed}")
PY
echo "== existing strip_null unit tests in baml_type =="
rg -n 'strip_null|nullable_non_null_part|dedup_and_collapse' baml_language/crates/baml_type/src -g '*.rs'Repository: BoundaryML/baml
Length of output: 3841
Align strip_null’s behavior with its contract or update the contract.
strip_null documents “a non-nullable type → unchanged,” but the fast path unwraps a single-member non-null union, e.g. Ty::union([Ty::Int]) -> Ty::Int. If this normalization is intentional, update the doc comment and add a targeted Rust unit test; otherwise preserve degenerate one-member unions unchanged.
🤖 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 `@baml_language/crates/baml_type/src/lib.rs` around lines 345 - 377, The
`strip_null` contract conflicts with its fast path, which unwraps single-member
non-null unions. Either preserve such unions unchanged by removing or adjusting
the fast path, or, if normalization is intentional, update `strip_null`’s
documentation and add a targeted Rust unit test covering `Ty::union([Ty::Int])`
becoming `Ty::Int`.
Source: Path instructions
Comment on lines
200
to
+203
| /// # Safety | ||
| /// `path` and `mime_type_or_null`, when non-null, must point to valid | ||
| /// NUL-terminated C strings. `out_key` and `out_handle_type` must be either | ||
| /// null or valid for writing one value of their pointee type. | ||
| /// `url` and `mime`, when non-null, must point to valid NUL-terminated C | ||
| /// strings. `out_key` and `out_type` must be either null or valid for | ||
| /// writing one value of their pointee type. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the output-pointer safety contract.
Lines 202-203 allow null output pointers, but Line 176 rejects either null pointer with UnexpectedNullptr. Document out_key and out_type as required valid output pointers.
Proposed fix
-/// strings. `out_key` and `out_type` must be either null or valid for
-/// writing one value of their pointee type.
+/// strings. `out_key` and `out_type` must be valid for writing one value
+/// of their pointee type.Also applies to: 215-218, 230-233
🤖 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 `@baml_language/crates/bridge_cffi/src/ffi/handle.rs` around lines 200 - 203,
Update the safety documentation for the FFI function around out_key and
out_type, including the corresponding contracts at the additionally affected
declarations, to state that both output pointers are required and must be valid
for writing one pointee value; remove the allowance that either pointer may be
null, matching the existing UnexpectedNullptr validation.
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
baml_language.+1,818/-4,184) against currentcanary.Validation
mise exec -- env -u RUSTC_WRAPPER cargo check --workspace --all-targets --all-features --lockedcargo fmt --all -- --checkgit diff --checkSummary by CodeRabbit
baml runfor cleaner command-line results.