Sam/generics#3727
Closed
sxlijin wants to merge 6 commits into
Closed
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR implements explicit generic type arguments (BEP-039) and opaque host-only values across the BAML compiler, engine, VM, and SDKs. Generic function signatures are captured as un-erased templates during compilation, then resolved and substituted at call boundaries with explicit host type arguments. Opaque host values are sealed handles that round-trip through generics while preserving identity, binding only to ChangesCore Infrastructure for Generics & Opaque Values
Node.js & Python SDK Integration
Rust Engine Tests & Enablement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsThanks 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4e8e4305e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+430
to
+439
| if (ht === BamlHandleType.HOST_VALUE_OPAQUE) { | ||
| // Same-process rehydration (bridge generics): an opaque | ||
| // host-only value decodes back to the ORIGINAL JS reference. | ||
| // Dead/foreign keys degrade to the bare BamlHandle. | ||
| const original = tryRehydrateHostValueByKey( | ||
| holder.handleValue.key as unknown as import('./native.js').HandleKey, | ||
| ht, | ||
| ); | ||
| if (original !== undefined) return original; | ||
| return handle; |
There was a problem hiding this comment.
When this HOST_VALUE_OPAQUE branch runs, handle has already been constructed as a normal BamlHandle, and the miss path returns it. BamlHandle finalization always calls baml_handle_release, but HOST_VALUE_* keys are explicitly not in HANDLE_TABLE and the Node host-value registry and CFFI handle table both allocate keys starting at 1, so decoding an opaque value can release an unrelated media/heap handle with the same numeric key (even same-process hits create a temporary BamlHandle before returning the original). This needs the same host-value skip behavior added for Python/WASM, or avoid constructing a BamlHandle for HOST_VALUE_OPAQUE unless it is non-releasing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/crates/bex_engine/src/conversion.rs (1)
547-638:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-callable host values before function binding.
At Line 559,
HostValueKind::Opaque/HostValueKind::Errorcan still flow intopeel_function_ty(ty)and get materialized asObject::HostClosurewhen the declared type is function-shaped. That violates the host-boundary contract (host-only values should only bind to unknown/generic positions).🔧 Proposed fix
if accepts_unknown && prefers_opaque { let dyn_arc: std::sync::Arc<dyn std::any::Any + Send + Sync> = arc; return Ok(Value::object( holder.holder_mut().tlab_mut().alloc_rust_data(dyn_arc), )); } + if !matches!(arc.kind, bex_external_types::HostValueKind::Callable) { + return Err(EngineError::TypeMismatch { + message: format!( + "host-only value cannot be passed where the declared type \ + is `{ty}`; host-only values bind only to generic/unknown \ + positions" + ), + }); + } // Peel through Optional / Union to land on the function type. let function_ty = peel_function_ty(ty).ok_or_else(|| {🤖 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/bex_engine/src/conversion.rs` around lines 547 - 638, The code must reject host values that are host-only (arc.kind not HostValueKind::Callable) before converting them into a HostClosure: add a guard that if arc.kind is not Callable and peel_function_ty(ty).is_some() then return an EngineError::TypeMismatch (same wording used elsewhere: e.g. "host-only value cannot be passed where the declared type is `{ty}`; host-only values bind only to generic/unknown positions"). Place this check alongside the existing accepts_unknown / prefers_opaque logic (using the same symbols ty_accepts_opaque_host_value, prefers_opaque, arc.kind, peel_function_ty, and the EngineError::TypeMismatch path) so non-callable HostValueKind::Opaque/Error never reach the HostClosure construction.baml_language/crates/bex_engine/src/lib.rs (1)
1760-1775:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
call_callablestill drops instantiated generic types at the host boundary.This path seeds
seed_type_argsinto the VM entry frame, but it coerces arguments and re-types the return value fromfunc.param_types/func.return_typebefore that. For generic closures and bound methods, those fields are the erased versions, so handle-based calls no longer enforce the same instantiated signature thatcall_function_bound_args()now uses.Also applies to: 1800-1830
🤖 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/bex_engine/src/lib.rs` around lines 1760 - 1775, call_callable is using the erased Function.param_types and Function.return_type when coercing args and re-typing returns, which drops instantiated generic type information seeded by seed_type_args; update call_callable to apply the seeded/instantiated type arguments from the entry frame (the same source call_function_bound_args uses) before performing argument coercion and before wrapping the return value so generic closures and bound methods use the instantiated signature, i.e. fetch the entry frame/type args (seed_type_args) from the VM frame created around seed_type_args and use that instantiated param_types/return_type for coercion and return re-typing instead of func.param_types/func.return_type; keep the existing NativeUnresolved check (matches!(func.kind, FunctionKind::NativeUnresolved)) and only change where param/return types are derived for coercion and wrapping.
🧹 Nitpick comments (3)
baml_language/crates/bridge_ctypes/src/value_decode.rs (1)
62-79: ⚡ Quick winAdd a unit test for
HostValueOpaquedecode mapping.This branch is new but only callable/error are directly asserted in tests. A dedicated opaque case will lock the wire contract.
🧪 Suggested test addition
+ #[test] + fn decode_inbound_host_value_opaque() { + let table = CffiHandleTable::new(); + let handle = BamlHandle { + key: 555, + handle_type: BamlHandleType::HostValueOpaque as i32, + }; + let inbound = InboundValue { + value: Some(InboundValueVariant::Handle(handle)), + }; + let result = inbound_to_external(inbound, &table).expect("decode succeeds"); + match result { + BexExternalValue::HostValue(arc) => { + assert_eq!(arc.key, 555); + assert_eq!(arc.kind, bex_project::HostValueKind::Opaque); + } + other => panic!("unexpected variant: {other:?}"), + } + assert!( + table.resolve(555).is_none(), + "HOST_VALUE_OPAQUE must not touch HANDLE_TABLE" + ); + }🤖 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_ctypes/src/value_decode.rs` around lines 62 - 79, Add a unit test that asserts the wire-to-enum mapping for the opaque host-value branch: construct a Baml handle with handle_type == BamlHandleType::HostValueOpaque as i32 and a sample key, call the same decode function that produces host_value_kind (the code branch that creates arc via bex_project::HostValueArc::intern and returns BexExternalValue::HostValue), and assert the result is a BexExternalValue::HostValue containing an interned HostValueArc with kind == bex_project::HostValueKind::Opaque and that the key is preserved; place the test alongside the other HostValue tests so it exercises the HostValueOpaque branch (referencing host_value_kind, BamlHandleType::HostValueOpaque, bex_project::HostValueKind::Opaque, bex_project::HostValueArc::intern, and BexExternalValue::HostValue).baml_language/crates/bex_resource_types/src/host_value.rs (1)
30-37: ⚡ Quick winAdd a unit test for
HostValueKind::Opaquelifecycle parity.
Opaqueis a new handle kind, but tests currently pin release semantics forCallable/Erroronly. Add a unit test that verifies last-drop release behavior (and optionallyinternidentity reuse) forOpaqueas well.As per coding guidelines,
**/*.rs: Prefer writing Rust unit tests over integration tests where possible.🤖 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/bex_resource_types/src/host_value.rs` around lines 30 - 37, Add a unit test under the same host_value tests module mirroring the existing lifecycle tests for Callable/Error that exercises HostValueKind::Opaque: create two interned Opaque HostValue instances (using HostValue::intern or the same factory used by other tests), clone/drop them to ensure the host-side release callback is invoked only on the final drop (verify last-drop release behavior), and optionally assert that interning reuses identity after complete release; place assertions and setup/teardown the same way the Callable/Error tests do so the Opaque kind gets identical lifecycle coverage.Source: Coding guidelines
baml_language/crates/bex_vm_types/src/types.rs (1)
375-391: Please confirm the Rust lib test suite was run for this change.Run
cargo test --libbefore merge since this PR modifies Rust code.
As per coding guidelines,Always run cargo test --lib if you changed any Rust code.🤖 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/bex_vm_types/src/types.rs` around lines 375 - 391, This change touches the FunctionSignatureTemplate struct (param_types, return_type) in bex_vm_types which may affect downstream behavior; run the Rust library test suite with `cargo test --lib` from the crate root, fix any failing tests caused by the changes (update tests, adjust implementation or serialization for FunctionSignatureTemplate, and ensure BorshSerialize/BorshDeserialize behavior remains correct for baml_type::TyTemplate), then re-run the tests until they pass before merging.Source: Coding guidelines
🤖 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_compiler2_emit/src/lib.rs`:
- Around line 1505-1512: The current code only stores param_types/return_type in
signature_template (when signature_has_type_var) but leaves
compiled_fn.throws_type coming from the erased compute_throws_type() path, so
generic `throws` information is lost; fix by preserving the throws template
alongside parameters/return by including the throws template when building
bex_vm_types::FunctionSignatureTemplate (or by making compiled_fn.throws_type
take the template result when signature_has_type_var is true), i.e., call the
generic-aware throws computation used for compute_throws_type() and attach that
throws template into signature_template (or override compiled_fn.throws_type
from that template) so calls with concrete type_args can specialize `throws`
correctly (refer to signature_template, signature_has_type_var,
compiled_fn.throws_type, compute_throws_type()).
In `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 94-102: The bug is that ctx.registered mixes callable and opaque
keys but rollback only releases callables, leaking opaque registrations; update
registration to record the kind for each entry (e.g., push an object/tuple like
{ kind: "opaque", key } instead of raw key) in encodeOpaque (which calls
registerHostOpaque) and in the analogous callable registration sites, and change
the rollback logic to inspect each entry's kind and call the correct release
function (e.g., releaseHostOpaque for "opaque" and releaseHostCallable for
"callable"). Also update any other sites mentioned (the other encode/register
locations) to use the kind-tagged registration so rollback uniformly frees the
right resource.
---
Outside diff comments:
In `@baml_language/crates/bex_engine/src/conversion.rs`:
- Around line 547-638: The code must reject host values that are host-only
(arc.kind not HostValueKind::Callable) before converting them into a
HostClosure: add a guard that if arc.kind is not Callable and
peel_function_ty(ty).is_some() then return an EngineError::TypeMismatch (same
wording used elsewhere: e.g. "host-only value cannot be passed where the
declared type is `{ty}`; host-only values bind only to generic/unknown
positions"). Place this check alongside the existing accepts_unknown /
prefers_opaque logic (using the same symbols ty_accepts_opaque_host_value,
prefers_opaque, arc.kind, peel_function_ty, and the EngineError::TypeMismatch
path) so non-callable HostValueKind::Opaque/Error never reach the HostClosure
construction.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1760-1775: call_callable is using the erased Function.param_types
and Function.return_type when coercing args and re-typing returns, which drops
instantiated generic type information seeded by seed_type_args; update
call_callable to apply the seeded/instantiated type arguments from the entry
frame (the same source call_function_bound_args uses) before performing argument
coercion and before wrapping the return value so generic closures and bound
methods use the instantiated signature, i.e. fetch the entry frame/type args
(seed_type_args) from the VM frame created around seed_type_args and use that
instantiated param_types/return_type for coercion and return re-typing instead
of func.param_types/func.return_type; keep the existing NativeUnresolved check
(matches!(func.kind, FunctionKind::NativeUnresolved)) and only change where
param/return types are derived for coercion and wrapping.
---
Nitpick comments:
In `@baml_language/crates/bex_resource_types/src/host_value.rs`:
- Around line 30-37: Add a unit test under the same host_value tests module
mirroring the existing lifecycle tests for Callable/Error that exercises
HostValueKind::Opaque: create two interned Opaque HostValue instances (using
HostValue::intern or the same factory used by other tests), clone/drop them to
ensure the host-side release callback is invoked only on the final drop (verify
last-drop release behavior), and optionally assert that interning reuses
identity after complete release; place assertions and setup/teardown the same
way the Callable/Error tests do so the Opaque kind gets identical lifecycle
coverage.
In `@baml_language/crates/bex_vm_types/src/types.rs`:
- Around line 375-391: This change touches the FunctionSignatureTemplate struct
(param_types, return_type) in bex_vm_types which may affect downstream behavior;
run the Rust library test suite with `cargo test --lib` from the crate root, fix
any failing tests caused by the changes (update tests, adjust implementation or
serialization for FunctionSignatureTemplate, and ensure
BorshSerialize/BorshDeserialize behavior remains correct for
baml_type::TyTemplate), then re-run the tests until they pass before merging.
In `@baml_language/crates/bridge_ctypes/src/value_decode.rs`:
- Around line 62-79: Add a unit test that asserts the wire-to-enum mapping for
the opaque host-value branch: construct a Baml handle with handle_type ==
BamlHandleType::HostValueOpaque as i32 and a sample key, call the same decode
function that produces host_value_kind (the code branch that creates arc via
bex_project::HostValueArc::intern and returns BexExternalValue::HostValue), and
assert the result is a BexExternalValue::HostValue containing an interned
HostValueArc with kind == bex_project::HostValueKind::Opaque and that the key is
preserved; place the test alongside the other HostValue tests so it exercises
the HostValueOpaque branch (referencing host_value_kind,
BamlHandleType::HostValueOpaque, bex_project::HostValueKind::Opaque,
bex_project::HostValueArc::intern, and BexExternalValue::HostValue).
🪄 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
Run ID: df0a1053-4f0d-48c5-968f-c37af25c76c4
⛔ Files ignored due to path filters (14)
baml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/host_error_registry.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/index.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto/baml_cffi.jsis excluded by!**/dist/**
📒 Files selected for processing (41)
baml_language/crates/baml_compiler2_emit/src/emit.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/host_boundary_generics.rsbaml_language/crates/bex_engine/tests/host_boundary_opaque.rsbaml_language/crates/bex_project/src/bex.rsbaml_language/crates/bex_resource_types/src/host_value.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm/tests/load_type.rsbaml_language/crates/bex_vm/tests/method_class_type_args.rsbaml_language/crates/bex_vm_types/src/lib.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_ctypes/src/value_decode.rsbaml_language/crates/bridge_ctypes/src/value_encode.rsbaml_language/crates/bridge_ctypes/types/baml_core/cffi/v1/baml_inbound.protobaml_language/crates/bridge_wasm/src/handle.rsbaml_language/crates/bridge_wasm/tests/host_callable.rsbaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_generic.pybaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_opaque.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/type_args.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/host_error_registry.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.d.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto/baml_cffi.jsbaml_language/sdks/python/rust/bridge_python/src/host_value.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/py_handle.rsbaml_language/sdks/python/rust/bridge_python/src/runtime.rsbaml_language/sdks/python/src/baml_core/__init__.pybaml_language/sdks/python/src/baml_core/baml_py.pyibaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pybaml_language/sdks/python/src/baml_core/cffi/v1/baml_inbound_pb2.pyibaml_language/sdks/python/src/baml_core/proto.pybaml_language/sdks/python/tests/test_host_callable.pybaml_language/sdks/python/tests/test_host_opaque.pybaml_language/sdks/python/tests/test_type_args.py
💤 Files with no reviewable changes (1)
- baml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_generic.py
Comment on lines
+1505
to
+1512
| // Only carry a template when some signature position actually references | ||
| // a generic parameter — the erased `param_types`/`return_type` are | ||
| // authoritative otherwise, and most functions stay template-free. | ||
| let signature_template = | ||
| signature_has_type_var.then(|| bex_vm_types::FunctionSignatureTemplate { | ||
| param_types: param_templates, | ||
| return_type: return_template, | ||
| }); |
Contributor
There was a problem hiding this comment.
Generic throws contracts are still erased here.
signature_template only preserves parameter and return templates, while compiled_fn.throws_type is still populated from the separately erased compute_throws_type() path. That means a signature like throws E can never be specialized from explicit host type_args, so the engine will keep validating/converting throws against unknown even when the call boundary has concrete generic arguments.
🤖 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_compiler2_emit/src/lib.rs` around lines 1505 -
1512, The current code only stores param_types/return_type in signature_template
(when signature_has_type_var) but leaves compiled_fn.throws_type coming from the
erased compute_throws_type() path, so generic `throws` information is lost; fix
by preserving the throws template alongside parameters/return by including the
throws template when building bex_vm_types::FunctionSignatureTemplate (or by
making compiled_fn.throws_type take the template result when
signature_has_type_var is true), i.e., call the generic-aware throws computation
used for compute_throws_type() and attach that throws template into
signature_template (or override compiled_fn.throws_type from that template) so
calls with concrete type_args can specialize `throws` correctly (refer to
signature_template, signature_has_type_var, compiled_fn.throws_type,
compute_throws_type()).
Comment on lines
+94
to
102
| function encodeOpaque(iv: baml_core.cffi.v1.IInboundValue, inner: unknown, ctx: EncodeCtx): void { | ||
| let key = ctx.opaqueKeys?.get(inner); | ||
| if (key === undefined) { | ||
| key = registerHostOpaque(inner); | ||
| ctx.registered.push(key); | ||
| ctx.opaqueKeys?.set(inner, key); | ||
| } | ||
| iv.handle = { key, handleType: BamlHandleType.HOST_VALUE_OPAQUE }; | ||
| } |
Contributor
There was a problem hiding this comment.
Opaque registrations are leaked on encode rollback paths.
ctx.registered currently mixes callable and opaque keys, but rollback only calls releaseHostCallable(...). When an encode fails before dispatch to Rust, opaque entries remain in the TS registry and retain host objects indefinitely.
Suggested fix (separate cleanup by registration kind)
interface EncodeCtx {
syncMode: boolean;
- registered: HandleKey[];
+ registeredCallables: HandleKey[];
+ registeredOpaque: HandleKey[];
opaqueKeys?: Map<unknown, HandleKey>;
}
+function cleanupEncodeCtx(ctx: EncodeCtx): void {
+ for (const k of ctx.registeredCallables) {
+ try { releaseHostCallable(k); } catch {}
+ }
+ for (const k of ctx.registeredOpaque) {
+ // add/export a host_error_registry helper that deletes the TS map entry by key
+ try { releaseHostOpaque(k); } catch {}
+ }
+}
+
function encodeOpaque(iv: ..., inner: unknown, ctx: EncodeCtx): void {
let key = ctx.opaqueKeys?.get(inner);
if (key === undefined) {
key = registerHostOpaque(inner);
- ctx.registered.push(key);
+ ctx.registeredOpaque.push(key);
ctx.opaqueKeys?.set(inner, key);
}
iv.handle = { key, handleType: BamlHandleType.HOST_VALUE_OPAQUE };
}
// callable registration site
-ctx.registered.push(key);
+ctx.registeredCallables.push(key);
// all rollback sites
-for (const k of ctx.registered) { ...releaseHostCallable(k)... }
+cleanupEncodeCtx(ctx);Also applies to: 289-295, 727-741, 833-857
🤖 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/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts` around lines
94 - 102, The bug is that ctx.registered mixes callable and opaque keys but
rollback only releases callables, leaking opaque registrations; update
registration to record the kind for each entry (e.g., push an object/tuple like
{ kind: "opaque", key } instead of raw key) in encodeOpaque (which calls
registerHostOpaque) and in the analogous callable registration sites, and change
the rollback logic to inspect each entry's kind and call the correct release
function (e.g., releaseHostOpaque for "opaque" and releaseHostCallable for
"callable"). Also update any other sites mentioned (the other encode/register
locations) to use the kind-tagged registration so rollback uniformly frees the
right resource.
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 by CodeRabbit
New Features
$typessyntax to bind concrete types for generic parameters, enabling precise type resolution at call time.