Repository navigation
feat(bridges): python outbound generics, nodejs inbound explicit and outbound - #3825
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughImplements end-to-end generic class type-argument propagation for the Node.js SDK. A new ChangesGeneric type-arg propagation: TS encoder → wire → Rust engine → decoder
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over TSCaller,setInboundValue: TS → Wire (encode path)
end
participant TSCaller as TS Caller (with $types)
participant defineFunction as defineFunction / defineInstanceFunction
participant buildTypeArgs as buildTypeArgs
participant setInboundValue as setInboundValue (proto.ts)
participant encodeCallArgs as encodeCallArgs (proto.ts)
participant Wire as CallFunctionArgs (proto)
participant RustBridge as decode_args (runtime.rs)
participant CtxBuilder as function_call_context_builder
participant Engine as BAML Engine
TSCaller->>defineFunction: call(kwargs, { $types })
defineFunction->>buildTypeArgs: self, types, typeParams, classTypeParams
buildTypeArgs-->>defineFunction: ordered [typeVar, BamlType][] pairs
defineFunction->>encodeCallArgs: { kwargs, typeArgs, callId }
encodeCallArgs->>setInboundValue: generic class instance arg
setInboundValue-->>encodeCallArgs: class_value { class_ty { typeArgs } }
encodeCallArgs->>Wire: CallFunctionArgs { kwargs, type_args[], callId }
rect rgba(144, 238, 144, 0.5)
note over Wire,TSCaller: Wire → Rust → TS (decode path)
end
Wire->>RustBridge: args.type_args[]
RustBridge->>CtxBuilder: decoded.type_args (named map)
CtxBuilder->>Engine: call frame with type bindings
Engine-->>CtxBuilder: result with genericArgs[]
CtxBuilder->>TSCaller: instance.$types repopulated via outboundTyToBamlType
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⏭️ 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):
|
![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: 41a35778da
ℹ️ 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".
| let trailing = optional_param_names_arg(&optional_params); | ||
| let optional_arg = optional_param_names_arg(&optional_params); | ||
| // Free functions bind only their own `<...>` params (no generic receiver). | ||
| let tail = factory_tail(&optional_arg, &f.generic_params, &[]); |
There was a problem hiding this comment.
$types in generic callable signatures
When f.generic_params is non-empty this starts emitting the runtime generics object, so defineFunction now calls resolveTypesOption and throws unless the caller supplies $types. The generated surface type is still built by fn_type_sig just above, which only contains the BAML value parameters/optional args, so functions like one_type_arg<T>() or identity<T>(x: T) are emitted as <T>() => string / <T>(x: T) => T while the only working runtime call is an extra options argument such as { $types: { T: 'int' } }. In TS users cannot call these generic functions without a cast, and the natural typed calls fail at runtime with “$types is required”.
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: 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 `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.ts`:
- Around line 236-239: The comment for the typeArgsFor function states that it
"eagerly rejects $types on a non-generic call," but the actual code silently
ignores $types when isGeneric is false by returning an empty array. Either
update the code to validate the types option unconditionally by calling
resolveTypesOption(built.types, []) instead of just returning [], or update the
comment to accurately reflect that $types is silently ignored rather than
rejected on non-generic calls. Ensure that whichever approach is taken, the
comment and code behavior are aligned.
🪄 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: ddb75625-6050-40c3-85e5-41670ce5e888
⛔ Files ignored due to path filters (17)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.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/wire_ty.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/wire_ty.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/wire_ty.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/wire_ty.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (15)
baml_language/crates/bridge_ctypes/src/value_encode.rsbaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_generic.pybaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.tsbaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.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/wire_ty.tsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/leaf.rs
💤 Files with no reviewable changes (1)
- baml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_generic.py
|
No description provided. |
Bring the TypeScript/Node SDK to parity with Python for generic classes and functions, and make outbound decoding repopulate a generic instance's concrete type args in both SDKs. - codegen (sdkgen_typescript_node): emit a static `$generic` param list and an optional `$types` field on every generic class; pass type_params/ classTypeParams to defineFunction/defineInstanceFunction. - bridge_nodejs: encode a generic instance as a FQN-tagged class_value with the value-level class_ty channel (absent $types -> Ty::Unknown); build/encode CallFunctionArgs.type_args; decodeClass repopulates $types (non-enumerable) from the wire generic_args. Thread CallFunctionArgs.type_args into the engine call context (was dropped on the Node side). - bridge_ctypes: outbound encoder now sends a generic instance's type_args as BamlValueClass.name.generic_args, using the canonical FQN (render_dotted) so the host typemap resolves it. - re-enable the generic round-trip + instance-method tests (TS + Python). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
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 `@baml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts`:
- Around line 233-252: In the generic class instance branch where `fqn` is
obtained from `getTypeMap().jsTypeToBamlType()`, add a guard to check that `fqn`
is defined before constructing the `classValue` object. The `fqn` variable may
be `undefined` if the constructor is unmapped, and without this guard the code
will create a malformed protobuf message with `name: undefined`. This guard
should be added similar to how the handle-backed class instance path (which
checks `if (fqn)` at lines 209-221) protects against this condition, wrapping
the classValue assignment and return statement within an `if (fqn)` check.
🪄 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: 396e5198-2749-4cce-ba7f-380badaa7ffa
⛔ Files ignored due to path filters (17)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/define_function.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/define_function.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/wire_ty.d.tsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/wire_ty.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/wire_ty.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/wire_ty.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (15)
baml_language/crates/bridge_ctypes/src/value_encode.rsbaml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_generic.pybaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.tsbaml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.tsbaml_language/sdks/nodejs/bridge_nodejs/Cargo.tomlbaml_language/sdks/nodejs/bridge_nodejs/src/runtime.rsbaml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.tsbaml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.tsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.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/wire_ty.tsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/leaf.rs
💤 Files with no reviewable changes (1)
- baml_language/sdk_tests/crates/python_pydantic2/type_shapes/customizable/test_generic.py
✅ Files skipped from review due to trivial changes (2)
- baml_language/sdks/nodejs/bridge_nodejs/Cargo.toml
- baml_language/sdks/nodejs/bridge_nodejs/tests/host_callable.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_forward_refs.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_generics.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/test_generic.test.ts
- baml_language/sdks/nodejs/bridge_nodejs/tests/test_stream.test.ts
- baml_language/sdk_tests/crates/typescript_node/type_shapes/customizable/roundtrip_streams.test.ts
- baml_language/crates/bridge_ctypes/src/value_encode.rs
- baml_language/sdks/nodejs/bridge_nodejs/src/runtime.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/index.ts
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/wire_ty.ts
- baml_language/sdks/nodejs/sdkgen_typescript_node/src/leaf.rs
- baml_language/sdks/nodejs/bridge_nodejs/typescript_src/define_function.ts
sxlijin
enabled auto-merge
June 22, 2026 22:50
sxlijin
changed the title
This was referenced Jul 15, 2026
This branch was previously deployed
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
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.
Bring the TypeScript/Node SDK to parity with Python for generic classes and functions, and make outbound decoding repopulate a generic instance's concrete type args in both SDKs.
$genericparam list and an optional$typesfield on every generic class; pass type_params/ classTypeParams to defineFunction/defineInstanceFunction.Summary by CodeRabbit
New Features
$typesbindings and generic call type-arguments.type_argsacross the boundary.Bug Fixes
type_argsduring wire encoding.Tests