Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces type-attribute support throughout the BAML compiler by extending the Changes
Sequence Diagram(s)Given the nature of this change—primarily type representation refactoring and PPIR infrastructure introduction—generating sequence diagrams is not appropriate. The changes involve:
The PPIR expansion logic, while substantial, operates as a static pre-processing transformation without multi-component interactions that would benefit from sequencing visualization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Justification: This PR exhibits high complexity and scope: it introduces a new PPIR stream-expansion subsystem (483 lines in Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Merging this PR will degrade performance by 41.88%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | bench_incremental_rename_type |
15.4 ms | 22.1 ms | -30.44% |
| ❌ | WallTime | bench_incremental_close_string |
11.1 ms | 13.8 ms | -19.71% |
| ❌ | WallTime | bench_scale_100_functions |
35.8 ms | 61 ms | -41.36% |
| ❌ | WallTime | bench_single_simple_file |
8.2 ms | 11 ms | -25.24% |
| ❌ | WallTime | bench_incremental_add_new_file |
5.9 ms | 8.5 ms | -30.32% |
| ❌ | WallTime | bench_incremental_add_string_char |
11.1 ms | 14 ms | -20.46% |
| ❌ | WallTime | bench_incremental_add_user_field |
13.1 ms | 18.4 ms | -29.13% |
| ❌ | WallTime | bench_incremental_add_field |
4.6 ms | 6.3 ms | -27.18% |
| ❌ | WallTime | bench_incremental_add_attribute |
11.1 ms | 13.9 ms | -20.09% |
| ❌ | WallTime | bench_incremental_modify_function |
4.6 ms | 6.3 ms | -26.92% |
| ❌ | WallTime | bench_scale_deep_nesting |
11.2 ms | 19.4 ms | -41.88% |
| ❌ | WallTime | bench_empty_project |
7.5 ms | 9.3 ms | -19.3% |
Comparing push-pqsmnrvrxlxy (d2abfb7) with canary (248a49f)
Footnotes
-
98 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![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: eed6a53ca5
ℹ️ 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".
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
eed6a53 to
127240c
Compare
Binary size checks passed✅ 7 passed
Generated by |
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
127240c to
8471286
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
8471286 to
10e6ebf
Compare
sxlijin
force-pushed
the
paulo/rust-functions-2
branch
from
8ae3391 to
291af76
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
10e6ebf to
b49f6fb
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
b49f6fb to
1adcd79
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
1adcd79 to
a8bd49e
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
a8bd49e to
c22189e
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
c22189e to
69485c1
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
69485c1 to
74ca579
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
74ca579 to
6271e5b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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_sap/src/sap_model/convert.rs (1)
15-25: 🧹 Nitpick | 🔵 TrivialRemove the unused
ParseFloaterror variant.The
ParseFloatvariant (line 18) is unreachable—no code in this module or elsewhere in the codebase produces astd::num::ParseFloatError. The variant can be safely removed.baml_language/crates/tools_onionskin/src/compiler.rs (1)
2834-2838:⚠️ Potential issue | 🟡 MinorRender SAP attrs in the TIR2 column browser too.
The flat
run_tir2()view now shows@sap.*, but this branch drops_sap_attrs, so the column/detail pane renders the same field as unannotated. That makes the two TIR2 views disagree on the resolved field shape.Possible fix
- for (fname, fty, _sap_attrs) in &resolved.fields { - detail.push(vec![ - DetailSpan::Code(format!(" {fname}")), - DetailSpan::TypeAnnotation(format!(": {fty}")), - ]); + for (fname, fty, sap_attrs) in &resolved.fields { + let mut line = vec![ + DetailSpan::Code(format!(" {fname}")), + DetailSpan::TypeAnnotation(format!(": {fty}")), + ]; + if !sap_attrs.is_empty() { + line.push(DetailSpan::Code(format!( + " {}", + sap_attrs + .iter() + .map(|a| format!("@{a}")) + .collect::<Vec<_>>() + .join(" ") + ))); + } + detail.push(line); }
♻️ Duplicate comments (9)
baml_language/crates/baml_compiler2_tir/src/package_interface.rs (1)
272-275:⚠️ Potential issue | 🔴 Critical
selftype resolution still exports the wrong package for non-bamlclasses.Line 272 correctly matches
TypeExpr::Unknown { .. }, but this branch still routes throughbuild_self_type_for_class, which hardcodes package"baml"(Line 458). That produces incorrect exported method signatures for dependency classes and inconsistent behavior versus own-package lookup.baml_language/crates/baml_tests/src/compiler2_tir/mod.rs (1)
895-900:⚠️ Potential issue | 🟡 MinorPreserve union precedence in
type_expr_to_string_hir.Without wrapping union inners here,
(A | B)?renders asA | B?and(A | B)[]renders asA | B[], which changes the snapshot meaning.🛠️ Suggested fix
- baml_compiler2_ast::TypeExpr::Optional { inner, .. } => { - format!("{}?", type_expr_to_string_hir(inner, pkg_prefix)) - } - baml_compiler2_ast::TypeExpr::List { inner, .. } => { - format!("{}[]", type_expr_to_string_hir(inner, pkg_prefix)) - } + baml_compiler2_ast::TypeExpr::Optional { inner, .. } => { + let s = type_expr_to_string_hir(inner, pkg_prefix); + if matches!(**inner, baml_compiler2_ast::TypeExpr::Union { .. }) { + format!("({s})?") + } else { + format!("{s}?") + } + } + baml_compiler2_ast::TypeExpr::List { inner, .. } => { + let s = type_expr_to_string_hir(inner, pkg_prefix); + if matches!(**inner, baml_compiler2_ast::TypeExpr::Union { .. }) { + format!("({s})[]") + } else { + format!("{s}[]") + } + }baml_language/crates/baml_compiler2_ast/src/lib.rs (1)
990-1036: 🧹 Nitpick | 🔵 TrivialTest documents current parser limitation but assertion is permissive.
The test at lines 1026-1029 accepts
@stream.doneas either a type attribute OR a field attribute. Per the learnings,@stream.doneshould only be a type-level attribute. While the comment documents this as a "current parser limitation," the permissive assertion means this test will pass even if the parser is later fixed, potentially masking regressions in either direction.Consider splitting into two assertions to explicitly document the expected vs. actual behavior:
Suggested improvement
- assert!( - stream_done_is_type_attr || stream_done_is_field_attr, - "expected `@stream.done` somewhere: type_attrs={type_attr_names:?}, field_attrs={field_attr_names:?}", - ); + // Document current behavior: `@stream.done` ends up as field attr when after `@alias` + // TODO: When parser is fixed, change this to assert stream_done_is_type_attr + assert!( + stream_done_is_field_attr, + "documenting current parser behavior: `@stream.done` should be field attr here, \ + type_attrs={type_attr_names:?}, field_attrs={field_attr_names:?}", + );Based on learnings:
alias,description, andskipare the only field-level attributes. All other@attributes includingstream.*are type-level attributes.baml_language/crates/baml_compiler2_tir/src/inference.rs (1)
433-434:⚠️ Potential issue | 🟠 MajorKeep alias-cycle validation on the pre-expansion package view.
Switching this query to
baml_compiler2_ppir::package_items()pulls synthesized*$streamaliases into the cycle graph. The new recursion snapshots already show the regression: every invalid user alias now reports a secondA$stream/Loop$streamdiagnostic at0..0.Possible fix
- let pkg_items = baml_compiler2_ppir::package_items(db, pkg_id); + let pkg_items = baml_compiler2_hir::package::package_items(db, pkg_id);If you still need PPIR items elsewhere, filter generated
*$streamaliases out before callingcollect_type_aliases.baml_language/crates/baml_compiler2_ast/src/lower_type_expr.rs (2)
119-123:⚠️ Potential issue | 🟠 MajorPreserve attrs on parenthesized types instead of dropping them.
collect_type_attrs()has already captured outer attrs here, but these paths recurse into the inner type and return it unchanged, andapply_modifiers_from_parts()only reattaches attrs when a postfix wrapper exists.(Foo | Bar)@sap.pending_neverand `A | (B) `@stream.donetherefore lose their type attrs entirely.Please add a Rust unit regression for an attributed parenthesized type. Based on learnings:
alias,description, andskipare the only field-level attributes; all other@attributes are type-level attributes that apply to type expressions.Also applies to: 207-220, 291-296
225-230:⚠️ Potential issue | 🟠 MajorRoute union-member attrs to the outer wrapper when
[]or?are present.These branches still build the base with
attrsand then callapply_modifiers_from_parts(..., vec![]). For members likeFoo[]@stream.doneor `map<string, Foo>? `@sap.pending_never, the attr lands on the inner base instead of the outerList/Optional, so downstream.attrs()readers miss it on the actual member type.Please add a Rust unit regression for an attributed union member with a postfix modifier. Based on learnings: `alias`, `description`, and `skip` are the only field-level attributes; all other `@` attributes are type-level attributes that apply to type expressions.Fix pattern
- let base = TypeExpr::Literal { - value: baml_base::Literal::String(s), - attrs, - }; - return apply_modifiers_from_parts(base, parts, vec![]); + let base = TypeExpr::Literal { + value: baml_base::Literal::String(s), + attrs: vec![], + }; + return apply_modifiers_from_parts(base, parts, attrs);Apply the same pattern to the integer/float/map/named-type branches in this function.
Also applies to: 233-238, 241-246, 262-267, 272-283
baml_language/crates/baml_compiler2_ppir/src/lib.rs (2)
202-211:⚠️ Potential issue | 🟠 MajorSynthetic fields drop field-level attributes (
@alias,@description,@skip).Line 208 uses
attributes: vec![], which discards field-level attributes from the source field. This changes the serialized shape and skip semantics of$streamclasses relative to the original type.Suggested fix
fields.push(ast::FieldDef { name: raw.name, type_expr: Some(ast::SpannedTypeExpr { expr: type_expr, span: TextRange::default(), }), - attributes: vec![], + attributes: field.attributes.clone(), span: TextRange::default(), name_span: TextRange::default(), });Based on learnings,
alias,description, andskipare the only field-level attributes and should be preserved on stream class fields.
227-235:⚠️ Potential issue | 🟠 MajorSynthetic
$streamclasses drop generic type parameters.Line 229 uses
generic_params: Vec::new(), but the generated fields may still reference type parameters likeT. Forclass Box<T> { value T }, the syntheticBox$streamwill contain an unboundTthat downstream resolution treats as a named type.Suggested fix
synthetic_items.push(ast::Item::Class(ast::ClassDef { name: SmolStr::new(format!("{}$stream", c.name)), - generic_params: Vec::new(), + generic_params: c.generic_params.clone(), fields, methods: Vec::new(),baml_language/crates/baml_compiler2_ppir/src/expand.rs (1)
302-314:⚠️ Potential issue | 🟠 MajorAlias body attributes are overwritten rather than merged, losing alias-defined attributes.
At lines 305-306, direct assignment overwrites the alias body's
stream_must_existandstream_doneattributes without merging. This means attributes declared on the alias definition itself are lost when the alias is used without explicitly repeating those attributes at the use site.This is inconsistent with how
@@stream.*block attributes are handled at lines 458-459, which useTyAttrValue::or()to preserve existing values. Since alias bodies are populated from type alias definitions viaPpirTy::from_type_expr()and include extracted type attributes, use-site values should merge with alias-defined attributes, not replace them.Recommend using
TyAttrValue::or()to merge like the block attribute logic:Suggested fix
if let Some(body) = alias_bodies.get(path) { let mut resolved = body.clone(); - resolved.attrs_mut().stream_must_exist = must_exist; - resolved.attrs_mut().stream_done = done; + let alias_must_exist = resolved.attrs().stream_must_exist; + let alias_done = resolved.attrs().stream_done; + resolved.attrs_mut().stream_must_exist = alias_must_exist.or(must_exist); + resolved.attrs_mut().stream_done = alias_done.or(done);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77a61edc-46da-4f6d-bc1d-088eee6d68d1
⛔ Files ignored due to path filters (53)
baml_language/crates/baml_tests/snapshots/attribute_validation/baml_tests__attribute_validation__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/basic_types/baml_tests__basic_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/duplicate_class_span/baml_tests__duplicate_class_span__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/error_cases/baml_tests__error_cases__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/format_checks/baml_tests__format_checks__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/function_call/baml_tests__function_call__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/function_types/baml_tests__function_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/generator/baml_tests__generator__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/generics/baml_tests__generics__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/header_in_llm_function/baml_tests__header_in_llm_function__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/match_exhaustiveness/baml_tests__match_exhaustiveness__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/paren_union_test/baml_tests__paren_union_test__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_constructors/baml_tests__parser_constructors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_error_recovery/baml_tests__parser_error_recovery__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_expressions/baml_tests__parser_expressions__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_statements/baml_tests__parser_statements__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_stress/baml_tests__parser_stress__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_strings/baml_tests__parser_strings__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/pending_greaters_fix/baml_tests__pending_greaters_fix__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_annotations/baml_tests__stream_annotations__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_crossfile/baml_tests__stream_crossfile__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__03_hir2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__04_7_mir2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__04_tir2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__05_diagnostics2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__06_codegen2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_aliases/baml_tests__type_aliases__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_annotation/baml_tests__type_annotation__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_annotation_errors/baml_tests__type_annotation_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_test/baml_tests__type_builder_test__04_tir.snapis excluded by!**/*.snap
📒 Files selected for processing (41)
baml_language/crates/baml_base/src/attr.rsbaml_language/crates/baml_builtins2_codegen/src/extract.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/companions.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/expand.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_ppir/src/ty.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/generics.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_tests/projects/stream_types/attrs.bamlbaml_language/crates/baml_tests/projects/stream_types/basic.bamlbaml_language/crates/baml_tests/projects/stream_types/complex.bamlbaml_language/crates/baml_tests/projects/stream_types/edge_cases.bamlbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a_recursion.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase5.rsbaml_language/crates/baml_type/src/attr.rsbaml_language/crates/baml_type/src/defs.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/tlab.rsbaml_language/crates/bex_sap/src/sap_model/convert.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/tools_onionskin/src/compiler.rs
💤 Files with no reviewable changes (4)
- baml_language/crates/bex_heap/src/gc.rs
- baml_language/crates/bex_heap/src/tlab.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
- baml_language/crates/bex_vm_types/src/types.rs
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
f7f00a1 to
1de517a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs (1)
787-800:⚠️ Potential issue | 🟠 MajorDon't lower bare builtin type tokens as unresolved paths.
When the parser takes this fallback,
string,int,bool,null, etc. are all turned intoTypeExpr::Path, so typed match/catch bindings resolve differently depending on whether the CST produced aTYPE_EXPRnode or just aWORDtoken. Reuse the normal token-to-TypeExprmapping here before defaulting toPath.baml_language/crates/tools_onionskin/src/compiler.rs (2)
25-71:⚠️ Potential issue | 🟡 MinorRender
TypeExpr.attrsin Onionskin's type strings.
hir2_type_expr_to_stringstill makesstringandstring@stream.done`` render identically, so the AST/HIR/HIR2/TIR views cannot show whether the new type-level attrs survived parsing and lowering. At minimum, append each node'sattrswhen building the display string.
2834-2838:⚠️ Potential issue | 🟡 MinorKeep the class detail pane in sync with the flat TIR2 view.
The flat renderer above now shows
@..., but this detail view still binds_sap_attrsand suppresses them. Selecting a class therefore hides the new per-field SAP state even though the same data is already available here.Diff to keep the detail pane aligned
- for (fname, fty, _sap_attrs) in &resolved.fields { - detail.push(vec![ - DetailSpan::Code(format!(" {fname}")), - DetailSpan::TypeAnnotation(format!(": {fty}")), - ]); - } + for (fname, fty, sap_attrs) in &resolved.fields { + let mut line = vec![ + DetailSpan::Code(format!(" {fname}")), + DetailSpan::TypeAnnotation(format!(": {fty}")), + ]; + if !sap_attrs.is_empty() { + let attrs = sap_attrs + .iter() + .map(|a| format!("@{a}")) + .collect::<Vec<_>>() + .join(" "); + line.push(DetailSpan::Code(format!(" {attrs}"))); + } + detail.push(line); + }
♻️ Duplicate comments (13)
baml_language/crates/baml_lsp2_actions/src/utils.rs (1)
194-194:⚠️ Potential issue | 🟡 MinorUse
Displayfor media hover strings.
format!("{kind:?}").to_lowercase()is only accidentally correct for the concrete media kinds;MediaKind::Genericstill renders asgeneric, so hover text diverges from the source-syntax renderer.kind.to_string()fixes that without changing the existing concrete cases.Suggested fix
- TypeExpr::Media { kind, .. } => format!("{kind:?}").to_lowercase(), + TypeExpr::Media { kind, .. } => kind.to_string(),baml_language/crates/baml_compiler2_tir/src/package_interface.rs (1)
272-275:⚠️ Potential issue | 🔴 CriticalThis now exports dependency
selfunder the wrong package.Routing untyped
selfthroughbuild_self_type_for_class()exposes the helper's hard-codedName::new("baml"). A dependency method likepkg.User.foo(self)will now publishselfasbaml.User, whilelookup_own_class_method()still resolves the same parameter against the defining package. Please derive the package from the defining file and reuse that logic in both paths.baml_language/crates/baml_compiler2_ast/src/lib.rs (1)
991-1035:⚠️ Potential issue | 🟠 MajorMake this test fail on misplaced
@stream.done.Allowing
@stream.doneto pass as either a type attr or a field attr makes the regression invisible. The new PPIR path only consumesTypeExpr::attrs(), so this test still passes while stream expansion silently drops the flag.Suggested tightening
- assert!( - stream_done_is_type_attr || stream_done_is_field_attr, - "expected `@stream.done` somewhere: type_attrs={type_attr_names:?}, field_attrs={field_attr_names:?}", - ); + assert!( + stream_done_is_type_attr, + "expected `@stream.done` as a type attribute: type_attrs={type_attr_names:?}, field_attrs={field_attr_names:?}", + ); + assert!( + !stream_done_is_field_attr, + "@stream.done should not be accepted as a field attribute: field_attrs={field_attr_names:?}", + );baml_language/crates/baml_compiler2_ppir/src/ty.rs (1)
279-287:⚠️ Potential issue | 🟠 MajorDon't erase float literal types to
Unknown.
TypeExpr::Literalstill distinguishes float literals, but this branch collapses them before PPIR/stream expansion. A type like3.14 | nullnow takes theUnknownpath instead of the literal path, which changes behavior relative to the other literal cases. Please preserve floats here with a lossless PPIR representation.baml_language/crates/baml_lsp2_actions/src/type_info.rs (1)
221-225:⚠️ Potential issue | 🟠 MajorHover still drops the new
@sap.*field metadata.
resolve_class_fields()now carries the stream-field SAP attrs in the third tuple element, but this map throws them away. Hover for$streamcompanion classes will therefore render onlynull | ..., whilerender_tirnow includes@sap.parse_without_null/@sap.in_progress_neveron the same fields.Suggested fix
let fields = resolved .fields .iter() - .map(|(field_name, ty, _)| (field_name.as_str().to_string(), utils::display_ty(ty))) + .map(|(field_name, ty, sap_attrs)| { + let attrs = sap_attrs + .iter() + .map(|a| format!("@{a}")) + .collect::<Vec<_>>() + .join(" "); + let rendered_ty = if attrs.is_empty() { + utils::display_ty(ty) + } else { + format!("{} {}", utils::display_ty(ty), attrs) + }; + (field_name.as_str().to_string(), rendered_ty) + }) .collect();baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
1411-1417:⚠️ Potential issue | 🟠 MajorBare primitive/media pattern sugar still lowers to
unknown.These branches synthesize
TypeExpr::Pathforint,string,image, etc., butlower_type_expr_in_ns()only recognizes those spellings through the dedicated primitive/media variants. The lowered type becomesunknown, somatch x { int => ... }stops narrowing andcatch (e: string)can turn into an accidental catch-all becausety_covers_fact(Unknown, _)is true.Suggested fix
- self.lower_pattern_type_expr( - &TypeExpr::Path { - segments: vec![name.clone()], - attrs: vec![], - }, - at_expr, - ) + self.lower_bare_type_sugar(name, at_expr)fn lower_bare_type_sugar(&mut self, name: &Name, at_expr: ExprId) -> Ty { match name.as_str() { "int" => Ty::Primitive(PrimitiveType::Int), "float" => Ty::Primitive(PrimitiveType::Float), "string" => Ty::Primitive(PrimitiveType::String), "bool" => Ty::Primitive(PrimitiveType::Bool), "null" => Ty::Primitive(PrimitiveType::Null), "image" => Ty::Primitive(PrimitiveType::Image), "audio" => Ty::Primitive(PrimitiveType::Audio), "video" => Ty::Primitive(PrimitiveType::Video), "pdf" => Ty::Primitive(PrimitiveType::Pdf), _ => self.lower_pattern_type_expr( &TypeExpr::Path { segments: vec![name.clone()], attrs: vec![], }, at_expr, ), } }Also applies to: 1543-1549
baml_language/crates/baml_tests/projects/stream_types/attrs.baml (1)
1-15:⚠️ Potential issue | 🟡 MinorBroaden this fixture past the easy cases.
This still only exercises
@stream.*on bare leaf types plus a pure block-level case. Please add at least one wrapped type (string?,int[], etc.), one class that mixes@@stream.donewith a field-level@stream.*, and one real field-level attr (@alias/@description/@skip) next to a@stream.*type so the parser boundary this PR touches is actually covered.Based on learnings,
alias,description, andskipare the only field-level attributes; all other@attributes includingstream.*are type-level attributes that apply to type expressions.baml_language/crates/baml_tests/src/compiler2_tir/mod.rs (1)
895-900:⚠️ Potential issue | 🟠 MajorParenthesize union inners in
type_expr_to_string_hir.The TIR helper above already handles this, but the HIR renderer still prints
A | B?/A | B[]forOptionalandListover a union, so(A | B)?and(A | B)[]snapshots change meaning.🛠️ Proposed fix
baml_compiler2_ast::TypeExpr::Optional { inner, .. } => { - format!("{}?", type_expr_to_string_hir(inner, pkg_prefix)) + let s = type_expr_to_string_hir(inner, pkg_prefix); + if matches!(**inner, baml_compiler2_ast::TypeExpr::Union { .. }) { + format!("({s})?") + } else { + format!("{s}?") + } } baml_compiler2_ast::TypeExpr::List { inner, .. } => { - format!("{}[]", type_expr_to_string_hir(inner, pkg_prefix)) + let s = type_expr_to_string_hir(inner, pkg_prefix); + if matches!(**inner, baml_compiler2_ast::TypeExpr::Union { .. }) { + format!("({s})[]") + } else { + format!("{s}[]") + } }baml_language/crates/baml_compiler2_ppir/src/expand.rs (2)
72-84:⚠️ Potential issue | 🟠 MajorResolve aliases before computing a union's pending default.
Every alias returns
PendingDefault::Nullhere. A union likeRows | intwheretype Rows = string[]will still prependnull, even thoughRowsalready has a non-null pending default from its body. Threadalias_bodiesand the same depth guard throughpending_default/union_pending_defaultso this decision is based on the resolved alias body.Also applies to: 94-106
302-306:⚠️ Potential issue | 🟠 MajorDon't clobber alias-defined
@stream.*attrs during alias resolution.
bodyalready carries the alias body's top-level stream attrs. Replacing them withmust_exist/donehere clears flags declared on the alias itself, sotype Foo = string@stream.done`` behaves as undecorated unless each reference repeats the flag.🛠️ Proposed fix
if let Some(body) = alias_bodies.get(path) { // Set merged attrs on the resolved body and recurse let mut resolved = body.clone(); - resolved.attrs_mut().stream_must_exist = must_exist; - resolved.attrs_mut().stream_done = done; + let alias_must_exist = resolved.attrs().stream_must_exist; + let alias_done = resolved.attrs().stream_done; + resolved.attrs_mut().stream_must_exist = + alias_must_exist.or(must_exist); + resolved.attrs_mut().stream_done = + alias_done.or(done); return stream_expand_inner( &resolved, package_items,baml_language/crates/baml_compiler2_ppir/src/lib.rs (3)
50-76:⚠️ Potential issue | 🟠 MajorQualify PPIR's block-attr and alias-body maps by package.
These helpers scan
project.files(db)but key the results only bynamespace_path + name. If two packages both defineFooin the same namespace, stream expansion in one package can pick up the other package's alias body or@@stream.*flags.Also applies to: 82-106
202-210:⚠️ Potential issue | 🟠 MajorPreserve field metadata on generated
$streamfields.Clearing
attributeshere strips@alias,@description, and@skipfrom the synthetic field, which changes serialized names and skip semantics for the streamed class.Based on learnings: `alias`, `description`, and `skip` are the only field-level attributes (applied to class/enum fields). All other `@` attributes including `check`, `assert`, and `stream.*` are type-level attributes that apply to type expressions.🛠️ Proposed fix
fields.push(ast::FieldDef { name: raw.name, type_expr: Some(ast::SpannedTypeExpr { expr: type_expr, span: TextRange::default(), }), - attributes: vec![], + attributes: field.attributes.clone(), span: TextRange::default(), name_span: TextRange::default(), });
227-229:⚠️ Potential issue | 🟠 MajorKeep generic parameters on the synthesized
$streamclass.
stream_expandcan still emit field types containingT/K/V, but this syntheticClassDefdropsgeneric_params.class Box<T> { value T }will therefore produceBox$stream.value: TwithTunresolved, typically at synthetic0..0spans.🛠️ Proposed fix
synthetic_items.push(ast::Item::Class(ast::ClassDef { name: SmolStr::new(format!("{}$stream", c.name)), - generic_params: Vec::new(), + generic_params: c.generic_params.clone(), fields, methods: Vec::new(), attributes: class_attrs,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 332a31c7-4d12-4809-b801-a595d10b8244
⛔ Files ignored due to path filters (53)
baml_language/crates/baml_tests/snapshots/attribute_validation/baml_tests__attribute_validation__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/basic_types/baml_tests__basic_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/duplicate_class_span/baml_tests__duplicate_class_span__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/error_cases/baml_tests__error_cases__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/format_checks/baml_tests__format_checks__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/function_call/baml_tests__function_call__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/function_types/baml_tests__function_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/generator/baml_tests__generator__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/generics/baml_tests__generics__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/header_in_llm_function/baml_tests__header_in_llm_function__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/match_exhaustiveness/baml_tests__match_exhaustiveness__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/paren_union_test/baml_tests__paren_union_test__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_constructors/baml_tests__parser_constructors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_error_recovery/baml_tests__parser_error_recovery__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_expressions/baml_tests__parser_expressions__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_statements/baml_tests__parser_statements__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_stress/baml_tests__parser_stress__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_strings/baml_tests__parser_strings__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/pending_greaters_fix/baml_tests__pending_greaters_fix__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_annotations/baml_tests__stream_annotations__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_crossfile/baml_tests__stream_crossfile__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__03_hir2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__04_7_mir2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__04_tir2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__05_diagnostics2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__06_codegen2.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_aliases/baml_tests__type_aliases__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_annotation/baml_tests__type_annotation__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_annotation_errors/baml_tests__type_annotation_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_test/baml_tests__type_builder_test__04_tir.snapis excluded by!**/*.snap
📒 Files selected for processing (41)
baml_language/crates/baml_base/src/attr.rsbaml_language/crates/baml_builtins2_codegen/src/extract.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/companions.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/expand.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_ppir/src/ty.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/generics.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_tests/projects/stream_types/attrs.bamlbaml_language/crates/baml_tests/projects/stream_types/basic.bamlbaml_language/crates/baml_tests/projects/stream_types/complex.bamlbaml_language/crates/baml_tests/projects/stream_types/edge_cases.bamlbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a_recursion.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase5.rsbaml_language/crates/baml_type/src/attr.rsbaml_language/crates/baml_type/src/defs.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/tlab.rsbaml_language/crates/bex_sap/src/sap_model/convert.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/tools_onionskin/src/compiler.rs
💤 Files with no reviewable changes (4)
- baml_language/crates/baml_compiler2_emit/src/lib.rs
- baml_language/crates/bex_heap/src/gc.rs
- baml_language/crates/bex_heap/src/tlab.rs
- baml_language/crates/bex_vm_types/src/types.rs
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
2 times, most recently
from
69d48c2 to
94c1604
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
94c1604 to
0ad17d9
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
0ad17d9 to
94d8e42
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
105-214:⚠️ Potential issue | 🟠 MajorPreserve
TyAttrwhen lowering TIR into MIR types.
convert_tir2_ty()initializes a freshTyAttr::default()and reuses it for every arm, so any attrs attached in TIR disappear beforeexpr_ty(),pat_ty(), and parameter/return lowering feed MIR. That blocks end-to-end type-attribute behavior even if earlier phases populateTir2Tycorrectly. Map the incoming attr through each variant here instead of zeroing it out.Based on learnings: New language features for BAML require coordinated updates across Parser (parser-database), IR/validation (baml-core), Compiler (baml-compiler), and VM (baml-vm).
baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs (1)
52-210:⚠️ Potential issue | 🔴 CriticalLower the new
TypeExprattrs instead of discarding them.Every branch here emits
TyAttr::default()and never reads the record-styleattrsnow carried onTypeExpr. That means@check,@assert,@stream.*, etc. are dropped at the AST→TIR boundary, so later phases never even see them. Threadtype_expr.attrs()into the emittedTyAttrfor both leaf and recursive variants.Based on learnings: all
@attributes exceptalias,description, andskipare type-level attributes that apply to type expressions.baml_language/crates/tools_onionskin/src/compiler.rs (2)
29-70:⚠️ Potential issue | 🟠 MajorRender type-level attrs in this formatter.
Every arm here drops the new metadata with
.., so HIR2/TIR2 signatures collapse attributed types back to plainFoo/string. Onionskin will hide the new PPIR/type-attribute surface instead of showing what was actually parsed.Based on learnings,
check,assert, andstream.*are type-level attributes that apply to type expressions.
2834-2838:⚠️ Potential issue | 🟡 MinorKeep
sap_attrsin the TIR2 class detail pane.The flat TIR2 renderer now includes these attrs, but the column-browser detail view throws them away. Selecting a class will therefore hide field type attrs even though the summary view shows them.
Based on learnings,
check,assert, andstream.*are type-level attributes that apply to type expressions.baml_language/crates/baml_compiler2_tir/src/throw_inference.rs (1)
447-468:⚠️ Potential issue | 🟠 MajorDon't let
TyAttrchange throw-fact identity.
throw_fact_from_expr()normalizes runtime throws to default attrs andfact_display_name()already ignores attrs, but_ => out.insert(ty.clone())keeps declared leaf attrs verbatim. The same nominal throw type can then land in the set twice depending only on whether it came from athrowexpression or athrowscontract. Canonicalize the leaf before inserting it so throw sets stay keyed by the thrown type, not incidental metadata.baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
704-723:⚠️ Potential issue | 🟠 MajorKeep the annotation’s
TyAttron annotatedletbindings.This path stores
tyfromcheck_exprintobindings/locals, butcheck_exprusually returns the initializer’s inferred attr, not the annotation’s. Forlet x: int@stream.done= 1,xis recorded as plain1/int, so later reads drop the declared streaming metadata.🛠️ Suggested fix
- let ty = self.check_expr(*init, body, &ann_ty); + let ty = self + .check_expr(*init, body, &ann_ty) + .with_attr(ann_ty.attr().clone());Also applies to: 738-750
♻️ Duplicate comments (10)
baml_language/crates/baml_compiler_syntax/src/ast.rs (1)
262-267: 🛠️ Refactor suggestion | 🟠 MajorAdd a focused lib test for attributed union members.
This helper is now the preservation point for union-member
@...metadata; please add a unit test for cases likeA@x| B,"lit"@x| T, and(A | B)@x``, then runcargo test --lib.As per coding guidelines "Prefer writing Rust unit tests over integration tests where possible" and "Always run
cargo test --libif you changed any Rust code".baml_language/crates/baml_lsp2_actions/src/type_info.rs (1)
224-224:⚠️ Potential issue | 🟠 MajorHover drops field type-attribute metadata.
At Line 224, destructuring
(field_name, ty, _)discards the new field metadata fromresolve_class_fields, soTypeInfo::Classhover cannot surface those annotations. Please plumb the third value throughTypeInfoand hover rendering.baml_language/crates/baml_compiler2_tir/src/inference.rs (2)
438-452:⚠️ Potential issue | 🟠 MajorRun cycle detection on pre-expansion package items.
These helpers now walk
baml_compiler2_ppir::package_items, which pulls synthesized*$streamaliases into the structural cycle graph again. That can recreate duplicate/0..0diagnostics for user-defined cycles. Use the pre-expansion HIR package view here, or filter generated PPIR items before collecting aliases/fields.Possible fix
- let pkg_items = baml_compiler2_ppir::package_items(db, pkg_id); + let pkg_items = baml_compiler2_hir::package::package_items(db, pkg_id);Apply the same change in both
detect_invalid_alias_cycles()anddetect_invalid_class_cycles().
581-586:⚠️ Potential issue | 🟠 MajorAlias-declared attrs still don't survive
ResolvedClassFields.
sap_attrsonly read the field's immediatetype_expr, sof: MyAliasstill loses attrs declared ontype MyAlias = ... @.... Downstream behavior now depends on whether the alias was spelled inline or expanded manually. Pull attr names from the resolved alias/TyAttr path too, not just the field syntax.Based on learnings: all
@attributes exceptalias,description, andskipare type-level attributes that apply to type expressions.baml_language/crates/baml_tests/src/compiler2_tir/mod.rs (1)
896-901:⚠️ Potential issue | 🟠 MajorKeep union inners parenthesized in
type_expr_to_string_hir.The HIR renderer still appends
?/[]directly to the inner string. That turns(A | B)?intoA | B?and(A | B)[]intoA | B[], so the snapshots describe a different type than the AST.🛠️ Suggested fix
- baml_compiler2_ast::TypeExpr::Optional { inner, .. } => { - format!("{}?", type_expr_to_string_hir(inner, pkg_prefix)) - } - baml_compiler2_ast::TypeExpr::List { inner, .. } => { - format!("{}[]", type_expr_to_string_hir(inner, pkg_prefix)) - } + baml_compiler2_ast::TypeExpr::Optional { inner, .. } => { + let s = type_expr_to_string_hir(inner, pkg_prefix); + if matches!(**inner, baml_compiler2_ast::TypeExpr::Union { .. }) { + format!("({s})?") + } else { + format!("{s}?") + } + } + baml_compiler2_ast::TypeExpr::List { inner, .. } => { + let s = type_expr_to_string_hir(inner, pkg_prefix); + if matches!(**inner, baml_compiler2_ast::TypeExpr::Union { .. }) { + format!("({s})[]") + } else { + format!("{s}[]") + } + }baml_language/crates/baml_compiler2_ppir/src/expand.rs (2)
50-54:⚠️ Potential issue | 🟠 MajorResolve aliases before choosing a union's pending default.
pending_default()still hard-codes everyTypeAliastoNull. Fortype Rows = string[]; x: Rows | int, that forces the union down the null-prepend path instead of reusing the alias body's empty-array default, so the generated$streamtype is wrong for alias-backed unions. Threadalias_bodiesand the depth guard throughpending_default()/union_pending_default()so this decision uses the resolved alias body.Also applies to: 79-84, 94-100
301-306:⚠️ Potential issue | 🟠 MajorMerge alias-body
@stream.*flags instead of overwriting them.
body.clone()already carries the alias body's own top-level stream attrs. Replacing them here discards alias-defined@stream.must_exist/@stream.done, sotype Foo = int@stream.must_exist`` behaves as if the alias had no flag unless every use-site repeats it.Suggested fix
if let Some(body) = alias_bodies.get(path) { // Set merged attrs on the resolved body and recurse let mut resolved = body.clone(); - resolved.attrs_mut().stream_must_exist = must_exist; - resolved.attrs_mut().stream_done = done; + let alias_must_exist = resolved.attrs().stream_must_exist; + let alias_done = resolved.attrs().stream_done; + resolved.attrs_mut().stream_must_exist = + alias_must_exist.or(must_exist); + resolved.attrs_mut().stream_done = + alias_done.or(done); return stream_expand_inner(baml_language/crates/baml_compiler2_ppir/src/lib.rs (3)
70-75:⚠️ Potential issue | 🟠 MajorUse one canonical key shape for
block_attrsandalias_bodies.These helpers key entries as
namespace_path + nameand omit the package, butPpirTy::from_type_expr()later looks them up with the raw AST path segments. Relative names in non-root namespaces will silently miss, and same namespace/type pairs from different packages will collide. Either qualify both sides to the same package-aware path, or scope these maps per package/file and keep the raw AST spelling on both sides.Also applies to: 104-106
202-208:⚠️ Potential issue | 🟠 MajorPreserve field-level metadata on generated
$streamfields.This resets every synthetic field to
attributes: vec![], which drops@alias,@description, and@skipfrom the source field. The generated$streamclass then stops matching the original field name/description/skip semantics.Suggested fix
fields.push(ast::FieldDef { name: raw.name, type_expr: Some(ast::SpannedTypeExpr { expr: type_expr, span: TextRange::default(), }), - attributes: vec![], + attributes: field.attributes.clone(), span: TextRange::default(), name_span: TextRange::default(), });Based on learnings,
alias,description, andskipare the only field-level attributes (applied to class/enum fields).
227-230:⚠️ Potential issue | 🟠 MajorKeep the original generic parameters on the synthesized
$streamclass.The generated fields can still mention
T/K/V, butgeneric_params: Vec::new()leaves those names unbound. Downstream resolution will treat them as named types instead of type parameters.Suggested fix
synthetic_items.push(ast::Item::Class(ast::ClassDef { name: SmolStr::new(format!("{}$stream", c.name)), - generic_params: Vec::new(), + generic_params: c.generic_params.clone(), fields, methods: Vec::new(),
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85e8af45-4af8-45d7-a1d0-2f8b1668245c
⛔ Files ignored due to path filters (48)
baml_language/crates/baml_tests/snapshots/attribute_validation/baml_tests__attribute_validation__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/basic_types/baml_tests__basic_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/duplicate_class_span/baml_tests__duplicate_class_span__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/error_cases/baml_tests__error_cases__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/format_checks/baml_tests__format_checks__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/function_call/baml_tests__function_call__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/function_types/baml_tests__function_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/generator/baml_tests__generator__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/generics/baml_tests__generics__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/header_in_llm_function/baml_tests__header_in_llm_function__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/match_exhaustiveness/baml_tests__match_exhaustiveness__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/paren_union_test/baml_tests__paren_union_test__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_constructors/baml_tests__parser_constructors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_error_recovery/baml_tests__parser_error_recovery__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_expressions/baml_tests__parser_expressions__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_speculative/baml_tests__parser_speculative__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_statements/baml_tests__parser_statements__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_stress/baml_tests__parser_stress__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_strings/baml_tests__parser_strings__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/pending_greaters_fix/baml_tests__pending_greaters_fix__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/simple_function/baml_tests__simple_function__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_annotations/baml_tests__stream_annotations__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_crossfile/baml_tests__stream_crossfile__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__01_lexer__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__02_parser__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__attrs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__basic.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__complex.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/stream_types/baml_tests__stream_types__10_formatter__edge_cases.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_aliases/baml_tests__type_aliases__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_annotation/baml_tests__type_annotation__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_annotation_errors/baml_tests__type_annotation_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_errors/baml_tests__type_builder_errors__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/type_builder_test/baml_tests__type_builder_test__04_tir.snapis excluded by!**/*.snap
📒 Files selected for processing (43)
baml_language/crates/baml_base/src/attr.rsbaml_language/crates/baml_builtins2_codegen/src/extract.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/companions.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/expand.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_ppir/src/ty.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/generics.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/narrowing.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_lsp2_actions/src/annotations.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/tokens.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_tests/projects/stream_types/attrs.bamlbaml_language/crates/baml_tests/projects/stream_types/basic.bamlbaml_language/crates/baml_tests/projects/stream_types/complex.bamlbaml_language/crates/baml_tests/projects/stream_types/edge_cases.bamlbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a_recursion.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase5.rsbaml_language/crates/tools_onionskin/src/compiler.rsfix_ty.pyfix_ty2.pyfix_ty3.pyfix_ty_final.py
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
94d8e42 to
160bf8f
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
160bf8f to
c910f19
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
c910f19 to
85f8003
Compare
sxlijin
force-pushed
the
push-pqsmnrvrxlxy
branch
from
85f8003 to
a6815a2
Compare
baml-language: ppir fixes fix pending_default() alias resolution preserve field/class attributes on stream expansion via clone-and-modify scope block attrs and alias bodies by package fix tyassert
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests