refactor(codegen): use canonical compiler-owned types - #4048
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ 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):
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change introduces ChangesCodegen type boundary
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CompilerTIR
participant ClientCodegen
participant CodegenTy
participant SDKGenerator
CompilerTIR->>ClientCodegen: provide compiler types
ClientCodegen->>CodegenTy: convert and canonicalize
CodegenTy-->>ClientCodegen: return canonical Ty
ClientCodegen->>SDKGenerator: provide typed symbol pool
SDKGenerator-->>SDKGenerator: translate Ty to target-language types
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
baml_language/sdks/nodejs/sdkgen_typescript_node/src/translate_ty.rs (1)
82-95: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winParenthesize callback types inside lists and unions.
Ty::Listonly wraps unions today, so a callback element renders as() => boolean[]instead of(() => boolean)[].Ty::Unionhas the same issue for nullable callbacks, producing() => boolean | nullinstead of(() => boolean) | null. Add matrix cases for a list of callbacks and a nullable callback.🤖 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/sdkgen_typescript_node/src/translate_ty.rs` around lines 82 - 95, The type translation logic must parenthesize callback/function types when they appear as list elements or union members, not only unions containing “ | ”. Update the relevant handling in translate_ty, including Ty::List and Ty::Union, so callback precedence renders as (() => boolean)[] and (() => boolean) | null; add matrix coverage for both nullable callback unions and callback lists.
🧹 Nitpick comments (1)
baml_language/crates/baml_type/src/family.rs (1)
445-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a widening/narrowing test for
CodegenTy.
conversion_matrix_round_trips(Lines 399-431) exercises widening/narrowing forRuntimeTy,RealizedTy,ConcreteTy,ConcreteRealizedTy, but the newCodegenTymember (which per the updatedlib.rsdiagram now sits betweenRuntimeTyandRealizedTy) has no equivalent round-trip coverage againstdeep_concrete()/with_typevar(). Given this crate's macro-generatedTryFrom/Fromimpls are the load-bearing invariant the whole PR stack depends on, locking inRuntimeTy::from(CodegenTy)andCodegenTy::try_from(&RuntimeTy)here would catch regressions early.🤖 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/family.rs` around lines 445 - 468, Add a CodegenTy widening/narrowing round-trip case to conversion_matrix_round_trips, using deep_concrete() and with_typevar() to verify RuntimeTy::from(CodegenTy) and CodegenTy::try_from(&RuntimeTy) both preserve the expected values. Keep the existing RuntimeTy, RealizedTy, ConcreteTy, and ConcreteRealizedTy coverage unchanged.
🤖 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_codegen_types/src/symbols.rs`:
- Around line 247-315: Update WalkAllUnions::walk_all_unions for the Ty::Union
arm to recurse through each union member and extend the result with that
member’s walk_all_unions output, while still retaining the current union itself.
Follow the existing Ty::Union(members, _) traversal used by validate_map_keys_ty
and preserve leaf behavior for the other non-container variants.
In `@baml_language/crates/baml_project/src/client_codegen.rs`:
- Around line 546-628: Update convert_tir_leaf to copy the source TyAttr from
each TirTy variant instead of constructing TyAttr::default(), preserving
non-default SAP and streaming annotations through lowering. Ensure recursive
conversions retain their own source attributes, and add a regression test
covering a TIR type with non-empty attributes through SDK generation.
---
Outside diff comments:
In `@baml_language/sdks/nodejs/sdkgen_typescript_node/src/translate_ty.rs`:
- Around line 82-95: The type translation logic must parenthesize
callback/function types when they appear as list elements or union members, not
only unions containing “ | ”. Update the relevant handling in translate_ty,
including Ty::List and Ty::Union, so callback precedence renders as (() =>
boolean)[] and (() => boolean) | null; add matrix coverage for both nullable
callback unions and callback lists.
---
Nitpick comments:
In `@baml_language/crates/baml_type/src/family.rs`:
- Around line 445-468: Add a CodegenTy widening/narrowing round-trip case to
conversion_matrix_round_trips, using deep_concrete() and with_typevar() to
verify RuntimeTy::from(CodegenTy) and CodegenTy::try_from(&RuntimeTy) both
preserve the expected values. Keep the existing RuntimeTy, RealizedTy,
ConcreteTy, and ConcreteRealizedTy coverage unchanged.
🪄 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: 95eeedc9-2c5c-4658-b8a6-7a2a9475b99a
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
baml_language/crates/baml_codegen_types/Cargo.tomlbaml_language/crates/baml_codegen_types/src/errors.rsbaml_language/crates/baml_codegen_types/src/symbols.rsbaml_language/crates/baml_codegen_types/src/ty.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_type/src/codegen_ty.rsbaml_language/crates/baml_type/src/family.rsbaml_language/crates/baml_type/src/lib.rsbaml_language/crates/baml_type/src/names.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/emit/mod.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/leaf.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/lib.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/routing.rsbaml_language/sdks/nodejs/sdkgen_typescript_node/src/translate_ty.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/python/rust/sdkgen_python_pydantic2/src/routing.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rs
adcdc38 to
ebe7610
Compare
|
@coderabbitai review Addressed all four review findings in
The branch is also rebased onto current |
|
✅ Action performedReview finished.
|
|
I'll also confirm the rebase onto ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
baml_language/crates/baml_type/src/family.rs (1)
32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
CodegenTyin every family-invariant test.The new member is absent from
borsh_round_trips, in-memory discriminant, borrowed-upcast, and downcast tests. Add it to guard the serialization and unsafe transmute contracts.As per coding guidelines, “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/baml_type/src/family.rs` around lines 32 - 36, Update every family-invariant test to include CodegenTy, specifically borsh_round_trips, the in-memory discriminant test, borrowed-upcast tests, and downcast tests. Add the new family member to each relevant test case so serialization and transmute contracts are covered consistently with the existing family variants.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/sdks/typescript/sdkgen_typescript_shared/src/translate_ty.rs`:
- Line 151: Update the Ty::EnumVariant branch in the type-rendering match to
preserve the variant’s member name instead of rendering only the enclosing enum,
while leaving Ty::Enum rendering unchanged. Emit the qualified TypeScript member
form for variants and add the corresponding cross-leaf variant case required by
the renderer.
- Line 59: Update the Ty-to-TypeScript translation match in the relevant
translator so Ty::Void continues mapping to null while Ty::Never maps to the
TypeScript never type. Split the combined Ty::Void | Ty::Never arm and add a
dedicated Ty::Never matrix case.
---
Nitpick comments:
In `@baml_language/crates/baml_type/src/family.rs`:
- Around line 32-36: Update every family-invariant test to include CodegenTy,
specifically borsh_round_trips, the in-memory discriminant test, borrowed-upcast
tests, and downcast tests. Add the new family member to each relevant test case
so serialization and transmute contracts are covered consistently with the
existing family variants.
🪄 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: 3509a98f-3722-4779-bd16-76adaa37a2f4
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
baml_language/crates/baml_codegen_types/Cargo.tomlbaml_language/crates/baml_codegen_types/src/errors.rsbaml_language/crates/baml_codegen_types/src/symbols.rsbaml_language/crates/baml_codegen_types/src/ty.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_type/src/codegen_ty.rsbaml_language/crates/baml_type/src/family.rsbaml_language/crates/baml_type/src/lib.rsbaml_language/crates/baml_type/src/names.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/python/rust/sdkgen_python_pydantic2/src/routing.rsbaml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/emit/mod.rsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/leaf.rsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/lib.rsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/routing.rsbaml_language/sdks/typescript/sdkgen_typescript_shared/src/translate_ty.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- baml_language/crates/baml_codegen_types/Cargo.toml
- baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/emit/mod.rs
- baml_language/crates/baml_type/src/lib.rs
- baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/routing.rs
- baml_language/crates/baml_type/src/names.rs
- baml_language/crates/baml_codegen_types/src/ty.rs
- baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rs
- baml_language/crates/baml_codegen_types/src/symbols.rs
- baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/leaf.rs
- baml_language/crates/baml_project/src/client_codegen.rs
- baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/lib.rs
|
@coderabbitai review Addressed the complete follow-up review in |
|
I'll also make sure nothing regressed from the earlier fixes (nested-union recursion, TIR ✅ Action performedReview finished.
|
- baml_cffi.h/cbindgen.toml/bridge_cffi tests: canary's generated-header machinery is authoritative (supersedes our header_is_current.rs); header regenerated to include BAML_BRIDGE_LANGUAGE_CPP = 6 - bridge_cpp headers ported to the renamed ABI (Buffer -> BamlBuffer, uintptr_t -> size_t, language id via the enum constant) - sdkgen_cpp ported to QualifiedTypeName accessors and the CodegenTy struct-variant shapes; Ty::EnumVariant widens to its enum - alias identity is now preserved at use sites (#4048), so generated signatures spell the using-alias name - cbindgen pinned =0.29.0 (canary), typescript sdk crate renames taken
1 participant
Summary
Builds the generator-independent codegen type foundation on top of the unified declaration namespace landed in #4047.
CodegenTyto the compiler-ownedbaml_typefamily instead of maintaining a parallel codegen enumnullQualifiedTypeNamevalues end to endDesign boundary
CodegenTydescribes shared, semantically valid BAML types. It deliberately does not encode target languages or generator capability policy.A BAML-wide rule such as legal map-key types is validated while building the shared symbol pool. Target-dependent representability (for example, whether a generator can emit recursive type aliases directly) should be checked separately using named features such as
RecursiveTypeAliases, rather than adding language-specific variants or conditions to this IR. The alias symbol retains recursion metadata so that check can be added without reconstructing the type graph.Tests
cargo fmt --all -- --checkcargo test -p baml_typecargo test -p baml_codegen_typescargo test -p baml_project client_codegencargo test -p sdkgen_typescript_nodecargo test -p sdkgen_python_pydantic2cargo check -p sdkgen_goCoverage includes primitive aliases, alias chains, recursive aliases, nullable aliases, aliases in containers, legal and illegal aliased map keys, repeated/nested null unions, and same alias names in different BAML namespaces.
Scope audit
No Go generator source, Go CLI/runtime/packaging,
OutputType::Go,sdk_import_path, language-version, CFFI, ABI-header, or wire-fixture changes are included.Summary by CodeRabbit
New Features
CodegenTyrepresentation.mapsupport and updated function/union formatting).Bug Fixes
void/never, and precedence.Refactor / Tests