fix(python): avoid TypeVar collisions with generated class names - #4307
fix(python): avoid TypeVar collisions with generated class names#4307Vaibhav701161 wants to merge 1 commit into
Conversation
|
@Vaibhav701161 is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe compiler now resolves bare generic parameters before same-named concrete types. The Python Pydantic v2 generator allocates unique TypeVar identifiers and propagates them through runtime code, stubs, annotations, aliases, and protocol rendering. ChangesGeneric type resolution and Python TypeVar rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BAMLCompiler
participant LeafBody
participant TranslateCtx
participant GeneratedPython
BAMLCompiler->>BAMLCompiler: resolve bare generic parameter before concrete type
BAMLCompiler->>LeafBody: provide generic source names
LeafBody->>LeafBody: allocate collision-free identifiers
LeafBody->>TranslateCtx: pass TypeVar mapping
TranslateCtx->>GeneratedPython: render mapped annotations and bases
LeafBody->>GeneratedPython: emit TypeVar declarations with source names
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rs (1)
94-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct unit test for the
typevarssubstitution path.The substitution logic here is correct, but no
Casein this file'stranslate_ty_covers_phase_g3_matrixexercises a populatedctx.typevarsmap. Coverage for the substitution behavior currently comes only from full-pipeline tests inlib.rs(typevar_binding_avoids_same_leaf_symbol_name,typevar_binding_escapes_chained_and_import_collisions).Add a
Casethat builds actxwithtypevars: Some(Rc::new(map))containing a"T" -> "T_"entry, assertingtranslate_tyemits"T_", plus a case where the map is present but lacks the key, asserting the fallback to the raw source name. This keeps the substitution behavior covered at the unit level, matching this file's existing test style.As per path instructions, "
**/*.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/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rs` around lines 94 - 99, Add direct unit-test cases to translate_ty_covers_phase_g3_matrix for Ty::TypeVar: one with ctx.typevars containing “T” mapped to “T_” and asserting the substituted output, and another with a present map missing the key and asserting the raw source-name fallback. Follow the existing Case/test style and construct the context using the file’s established Rc map setup.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.
Nitpick comments:
In `@baml_language/sdks/python/rust/sdkgen_python_pydantic2/src/translate_ty.rs`:
- Around line 94-99: Add direct unit-test cases to
translate_ty_covers_phase_g3_matrix for Ty::TypeVar: one with ctx.typevars
containing “T” mapped to “T_” and asserting the substituted output, and another
with a present map missing the key and asserting the raw source-name fallback.
Follow the existing Case/test style and construct the context using the file’s
established Rc map setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc2c385b-c4f8-4ce5-8c0c-ca88f279b1a8
📒 Files selected for processing (5)
baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_project/src/client_codegen.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/translate_ty.rs
Closes #4083.
The bug
A generic parameter and an ordinary definition can share a name. When they do, the generated Python Pydantic v2 leaf declared the
TypeVarfirst and then rebound the same identifier as a class, so every generic reference in the file resolved to the class instead of the parameter:This is not cosmetic - the generated SDK does not import at all:
Root cause
Two layers had to agree, and neither did.
1. Name resolution (
baml_compiler2_tir).lower_pathonly consultedresolve_type_varinside itsErrfallback - i.e. only once concrete-type resolution had already failed. With aclass Tin the same package that lookup succeeds, so theTinfunction echo<T>(value: T)lowered toTy::Class, notTy::TypeVar. The generic parameter was being captured by the class before codegen ever ran.2. Binding allocation (
sdkgen_python_pydantic2). LeafTypeVars were emitted under their source spelling with nothing checking it against the rest of the module namespace.The change
lower_pathnow resolves a bare, unparameterized in-scope generic parameter beforeresolve_type, so the parameter shadows a same-named concrete type. Qualified paths (pkg.T) still reach the class through the namespace, and a parameterized spelling (T<int>) still falls through to the existing path.LeafBody::allocated_typevars()maps each BAML generic name to a collision-free Python identifier, appending_until the name is free. The reserved set is the leaf's own symbols, its cross-leaf import anchors, andMODULE_BINDINGS— the stdlib /baml_bridge/ builtins names a leaf can bind on its own, so a parameter namedtypingcan't rebind thetypingimport either.TranslateCtxcarries that map so declarations and every reference render from the same allocation..pyand.pyicompute the reserved set identically - it is deliberately independent of which file is being rendered — so the two files can never disagree on a spelling.Runtime-facing names are untouched: the binding is still
typing.TypeVar("T")andtype_params=["T"]still carries the BAML spelling. Only the local Python identifier moves, and only when it would otherwise collide.Tests
typevar_binding_avoids_same_leaf_symbol_name- the issue's scenario end to end; asserts the class keeps the plain name,.pyand.pyiboth follow the allocated one, andtype_paramsstays the BAML spelling.typevar_binding_escapes_chained_and_import_collisions- allocation walks a chain (TandT_both taken, so the parameter lands onT__) and escapes an import name no symbol owns (a parameter namedtypingbecomestyping_).test_bare_generic_param_shadows_same_named_class_in_codegen- covers the resolution half: withclass Tandfunction echo<T>in one file, the argument and return types lower toTy::TypeVar, with no diagnostics.Both scenarios were also checked by importing the generated package under real Pydantic v2. The pre-fix output raises
TypeErroron import; the post-fix output imports cleanly and keeps the BAML spelling at runtime:parametersafter importclass T+Box<T>(~T,)Box[int](item=3)T/T_taken, paramsT+typing(~T, ~typing)Box[int, str](item=1, tag='x')Note on scope
lower_type_expr.rsandclient_codegen.rssit outside the issue's stated affected area. They are in scope because renaming alone would not have fixed anything: the type checker was handing codegen aTy::Classwhere the schema saidTy::TypeVar, so the wrong entity was being referenced regardless of what it was called.Summary by CodeRabbit
Bug Fixes
Tests
.pyand.pyifiles.