Skip to content

baml-language: fix type alias resolution in compiler2#3228

Open
sxlijin wants to merge 1 commit intocanaryfrom
push-vnznwqtvoutk
Open

baml-language: fix type alias resolution in compiler2#3228
sxlijin wants to merge 1 commit intocanaryfrom
push-vnznwqtvoutk

Conversation

@sxlijin
Copy link
Contributor

@sxlijin sxlijin commented Mar 10, 2026

Summary by CodeRabbit

  • New Features

    • Enhanced type-alias resolution across compilation phases so struct literals, field access, returns, and codegen correctly follow chained aliases.
  • Bug Fixes

    • Fixed member and method lookup through chained type aliases and added cycle protection.
    • Reduced redundant runtime type checks in some pattern-match branches.
  • Tests

    • Added extensive regression tests covering alias chains, struct literals, field access, and related codegen paths.

@vercel
Copy link

vercel bot commented Mar 10, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
beps Ready Ready Preview, Comment Mar 10, 2026 6:08am
promptfiddle Ready Ready Preview, Comment Mar 10, 2026 6:08am

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

📝 Walkthrough

Walkthrough

Adds cycle-protected type-alias resolution across the compiler pipeline: TIR inference resolves chained aliases for objects and member access, MIR lowering resolves alias-to-class names for aggregates/field lookups, tests added, and codegen now forwards type-alias maps into inference.

Changes

Cohort / File(s) Summary
TIR: Alias resolution helpers & usage
baml_language/crates/baml_compiler2_tir/src/builder.rs
Added resolve_alias / resolve_alias_impl, resolve chained TypeAlias in Expr::Object, member lookups, matching, and alias-aware type handling with cycle protection.
TIR: Field access via alias chains
baml_language/crates/baml_compiler_tir/src/lib.rs
Expanded infer_field_access to iteratively resolve alias chains, attempt method lookup on resolved classes, and fallback to class field lookup when appropriate.
MIR: Class-name resolution through aliases
baml_language/crates/baml_compiler_mir/src/lower.rs
Added resolve_class_name_through_aliases and used it when creating aggregates, handling spreads, and computing field indices so aliases to classes resolve to underlying class names.
Codegen: pass type aliases into inference
baml_language/crates/baml_compiler_emit/src/lib.rs
Now passes Some(type_aliases.clone()) into baml_compiler_tir::infer_function so inference sees current type-alias mappings.
Tests: TIR/MIR alias regressions & coverage
baml_language/crates/baml_tests/src/compiler2_tir/inference.rs, baml_language/crates/baml_tests/tests/type_aliases.rs
Added regression and coverage tests for struct literals, aliased struct construction, field access through aliases, return/parameter alias propagation, and MIR emission cases.
Tests: match arm type-check adjustments
baml_language/crates/baml_tests/tests/match_types.rs
Removed certain runtime instanceof checks emitted for some match arms (adjusted emitted type-check sequence for specific pattern branches).

Sequence Diagram(s)

sequenceDiagram
    participant Parser
    participant TIR as TypeInference (TIR)
    participant MIR as MIR Lowering
    participant Codegen
    Parser->>TIR: provide AST + type_aliases
    TIR->>TIR: resolve_alias() / resolve_alias_impl (cycle-protected)
    TIR->>TIR: resolve chained TypeAlias for objects & members
    TIR->>MIR: emit TIR with resolved types/members
    MIR->>MIR: resolve_class_name_through_aliases() for aggregates/fields
    MIR->>Codegen: emit MIR with concrete class names/field indices
    Codegen->>Runtime: generate bytecode that constructs/loads resolved classes
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • imalsogreg
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main objective of the pull request, which is to fix type alias resolution across the compiler2 codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch push-vnznwqtvoutk

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Mar 10, 2026

Binary size checks passed

7 passed

Artifact Platform Gzip Baseline Delta Status
bridge_cffi Linux 4.5 MB 4.4 MB +81.2 KB (+1.8%) OK
bridge_cffi-stripped Linux 3.0 MB 2.9 MB +82.9 KB (+2.9%) OK
bridge_cffi macOS 3.6 MB 3.6 MB +39.6 KB (+1.1%) OK
bridge_cffi-stripped macOS 2.3 MB 2.3 MB +35.2 KB (+1.5%) OK
bridge_cffi Windows 3.6 MB 3.6 MB +68.4 KB (+1.9%) OK
bridge_cffi-stripped Windows 2.5 MB 2.4 MB +68.0 KB (+2.9%) OK
bridge_wasm WASM 2.2 MB 2.2 MB +50.5 KB (+2.3%) OK

Generated by cargo size-gate · workflow run

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 989120b3-880b-46b9-8be1-a70275d5ef7b

📥 Commits

Reviewing files that changed from the base of the PR and between 06910c5 and 1385be7.

⛔ Files ignored due to path filters (4)
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__alias_in_return_type_annotation.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__chained_alias_struct_literal.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__field_access_through_alias.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__struct_literal_through_alias.snap is excluded by !**/*.snap
📒 Files selected for processing (5)
  • baml_language/crates/baml_compiler2_tir/src/builder.rs
  • baml_language/crates/baml_compiler_mir/src/lower.rs
  • baml_language/crates/baml_compiler_tir/src/lib.rs
  • baml_language/crates/baml_tests/src/compiler2_tir/inference.rs
  • baml_language/crates/baml_tests/tests/type_aliases.rs

Comment on lines +4636 to +4670
// Resolve the alias to its underlying type and look up the field there.
let mut current_name = fqn.name.clone();
let mut seen = std::collections::HashSet::new();
let mut result: Option<Ty> = None;
while seen.insert(current_name.clone()) {
match ctx.lookup_type_alias(&current_name) {
Some(Ty::Class(class_fqn, _)) => {
let key = class_fqn.display_name();
let method_qn = QualifiedName {
namespace: class_fqn.namespace.clone(),
name: QualifiedName::local_method_from_str(
class_fqn.name.as_str(),
field.as_str(),
),
};
let method_lookup_name = method_qn.display_name();
if let Some(method_ty) = ctx.lookup(&method_lookup_name).cloned() {
if let Some(expr_id) = expr_id {
ctx.set_expr_resolution(
expr_id,
ResolvedValue::Function(method_qn),
);
}
return method_ty;
}
result = ctx.lookup_class_field(&key, field).cloned();
break;
}
Some(Ty::TypeAlias(next_fqn, _)) => {
current_name = next_fqn.name.clone();
}
_ => break,
}
}
result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding unit tests for type alias resolution.

The new chained alias resolution logic handles several important cases:

  1. Single alias → class
  2. Chained aliases → class
  3. Cycle detection (preventing infinite loops)
  4. Aliases resolving to non-class types

Per coding guidelines, unit tests are preferred for Rust code. Consider adding tests for these scenarios to validate correctness and prevent regressions. As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible."


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if builtins::lookup_method handles Ty::TypeAlias
echo "=== Checking builtins module for TypeAlias handling ==="
rg -n "fn lookup_method" baml_language/crates/baml_compiler_tir/src/builtins
rg -n "TypeAlias" baml_language/crates/baml_compiler_tir/src/builtins

Repository: BoundaryML/baml

Length of output: 315


🏁 Script executed:

#!/bin/bash
# Explore the codebase structure and find lookup_method
echo "=== Finding files related to builtins ==="
fd -t f "builtins" baml_language/crates/baml_compiler_tir/src/

echo ""
echo "=== Searching for lookup_method definition ==="
rg -n "fn lookup_method" baml_language/crates/baml_compiler_tir/src/

echo ""
echo "=== Searching for lookup_method calls ==="
rg -n "lookup_method" baml_language/crates/baml_compiler_tir/src/lib.rs | head -20

Repository: BoundaryML/baml

Length of output: 572


🏁 Script executed:

#!/bin/bash
# Read the lookup_method function definition
echo "=== lookup_method implementation ==="
sed -n '288,350p' baml_language/crates/baml_compiler_tir/src/builtins.rs

echo ""
echo "=== Context around line 4734 in lib.rs ==="
sed -n '4720,4750p' baml_language/crates/baml_compiler_tir/src/lib.rs

Repository: BoundaryML/baml

Length of output: 3794


🏁 Script executed:

#!/bin/bash
# Find match_pattern function
echo "=== Searching for match_pattern definition ==="
rg -n "fn match_pattern" baml_language/crates/baml_compiler_tir/src/

echo ""
echo "=== Reading match_pattern implementation ==="
rg -A 30 "fn match_pattern" baml_language/crates/baml_compiler_tir/src/builtins.rs

Repository: BoundaryML/baml

Length of output: 1814


🏁 Script executed:

#!/bin/bash
# Read the full match_pattern_inner implementation
echo "=== Full match_pattern_inner implementation ==="
sed -n '72,180p' baml_language/crates/baml_compiler_tir/src/builtins.rs

Repository: BoundaryML/baml

Length of output: 3858


🏁 Script executed:

#!/bin/bash
# Read the complete code flow from line 4636 to 4750
echo "=== Complete code flow for infer_field_access resolution ==="
sed -n '4636,4750p' baml_language/crates/baml_compiler_tir/src/lib.rs

Repository: BoundaryML/baml

Length of output: 5395


🏁 Script executed:

#!/bin/bash
# Search for unit tests related to type aliases and field/method access
echo "=== Checking for unit tests related to type alias field access ==="
rg -n "test.*alias.*field\|test.*field.*alias" baml_language/crates/baml_compiler_tir/src/lib.rs -i

echo ""
echo "=== Checking for any test module in lib.rs ==="
rg -n "mod tests\|#\[cfg.*test" baml_language/crates/baml_compiler_tir/src/lib.rs | head -20

echo ""
echo "=== Look for test files ==="
fd -t f "test" baml_language/crates/baml_compiler_tir/src/ | head -10

Repository: BoundaryML/baml

Length of output: 202


Fix builtin method lookup to handle type aliases resolving to non-class types.

The code at lines 4636–4670 resolves type alias chains to classes correctly, but for aliases resolving to non-class types (e.g., Ty::Int, Ty::List, Ty::Enum), it returns None and falls through to builtins::lookup_method(base, ...) at line 4734 with base still being Ty::TypeAlias.

The match_pattern_inner function in builtins.rs (lines 72–150) has no case for Ty::TypeAlias, so it returns false for any pattern match attempt. This breaks method calls on aliases to primitives and collections:

type IntList = int[]
let xs: IntList = [1, 2, 3]
xs.length()  // Fails: lookup_method doesn't recognize Ty::TypeAlias

Either:

  1. Extend the alias resolution loop (lines 4636–4670) to resolve non-class types and continue to builtin lookup with the resolved type, or
  2. Update match_pattern_inner to resolve Ty::TypeAlias to its underlying type before pattern matching.

Also add unit tests for this case per the Rust guidelines (cargo test --lib).

Comment on lines +2172 to +2197
/// Resolve a type through aliases to its underlying non-alias type.
/// Follows alias chains (A -> B -> C -> Class).
/// Returns the input type unchanged if it's not an alias or the alias is unknown.
/// Handles recursive aliases by returning the alias type for self-referential chains.
fn resolve_alias<'a>(&'a self, ty: &'a Ty) -> &'a Ty {
self.resolve_alias_impl(ty, &mut HashSet::new())
}

fn resolve_alias_impl<'a>(
&'a self,
ty: &'a Ty,
seen: &mut HashSet<crate::ty::QualifiedTypeName>,
) -> &'a Ty {
match ty {
Ty::TypeAlias(qn) => {
if !seen.insert(qn.clone()) {
return ty; // cycle — return as-is
}
match self.aliases.get(qn) {
Some(target) => self.resolve_alias_impl(target, seen),
None => ty,
}
}
_ => ty,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for existing tests covering type alias resolution in this crate
echo "=== Searching for alias resolution tests ==="
rg -n "resolve_alias" --type rust baml_language/crates/baml_compiler2_tir/

echo ""
echo "=== Searching for type alias tests in test files ==="
fd -e rs -p 'test' baml_language/crates/ --exec rg -l "TypeAlias|type_alias" {} \;

echo ""
echo "=== Check test module in builder.rs ==="
rg -n "mod tests" baml_language/crates/baml_compiler2_tir/src/builder.rs

Repository: BoundaryML/baml

Length of output: 1880


🏁 Script executed:

#!/bin/bash
echo "=== Check for test modules in baml_compiler2_tir crate ==="
find baml_language/crates/baml_compiler2_tir/src -name "*.rs" -type f -exec grep -l "^#\[cfg(test)\]" {} \;

echo ""
echo "=== Search for any test references to resolve_alias or alias resolution ==="
find baml_language/crates/baml_tests -name "*.rs" -type f | xargs rg -n "resolve_alias|alias.*test|test.*alias" 2>/dev/null | head -50

echo ""
echo "=== Check builder.rs file size and structure ==="
wc -l baml_language/crates/baml_compiler2_tir/src/builder.rs

echo ""
echo "=== Check if there's a separate test file for builder ==="
find baml_language/crates/baml_compiler2_tir -name "*builder*test*" -o -name "*test*builder*"

Repository: BoundaryML/baml

Length of output: 706


🏁 Script executed:

#!/bin/bash
echo "=== Check git history for resolve_alias functions ==="
git log --oneline -p baml_language/crates/baml_compiler2_tir/src/builder.rs 2>/dev/null | grep -A 5 -B 5 "resolve_alias" | head -80

echo ""
echo "=== Check if the file was modified recently ==="
git log --oneline -n 20 -- baml_language/crates/baml_compiler2_tir/src/builder.rs 2>/dev/null || echo "No git history available"

echo ""
echo "=== Look at the actual implementation around line 2176 to see context ==="
sed -n '2170,2200p' baml_language/crates/baml_compiler2_tir/src/builder.rs

Repository: BoundaryML/baml

Length of output: 4747


Add unit tests for alias resolution logic per coding guidelines.

The implementation correctly handles cycle detection and returns proper references, but this is new code that changes the builder.rs file. Per coding guidelines, new Rust code requires unit test coverage and cargo test --lib must be run.

Add tests for these scenarios:

  • Simple alias resolution (type A = SomeClass)
  • Chained aliases (A → B → C → Class)
  • Cyclic aliases (A → B → A) terminates without panic
  • Missing alias target returns input unchanged

Comment on lines +11 to +109
#[tokio::test]
async fn struct_literal_through_alias() {
let output = baml_test!(
"
class Foo { x int }
type Bar = Foo;

function main() -> Bar {
let v = Bar { x: 1 };
v
}
"
);

insta::assert_snapshot!(output.bytecode);

assert_eq!(
output.result,
Ok(BexExternalValue::instance(
"Foo",
indexmap! {
"x" => BexExternalValue::Int(1),
}
))
);
}

#[tokio::test]
async fn chained_alias_struct_literal() {
let output = baml_test!(
"
class Foo { x int }
type A = B;
type B = Foo;

function main() -> Foo {
let v = A { x: 42 };
v
}
"
);

insta::assert_snapshot!(output.bytecode);

assert_eq!(
output.result,
Ok(BexExternalValue::instance(
"Foo",
indexmap! {
"x" => BexExternalValue::Int(42),
}
))
);
}

#[tokio::test]
async fn field_access_through_alias() {
let output = baml_test!(
"
class Foo { x int }
type Bar = Foo;

function main() -> int {
let v = Bar { x: 7 };
v.x
}
"
);

insta::assert_snapshot!(output.bytecode);

assert_eq!(output.result, Ok(BexExternalValue::Int(7)));
}

#[tokio::test]
async fn alias_in_return_type_annotation() {
let output = baml_test!(
"
class Foo { x int }
type Bar = Foo;

function main() -> Bar {
Foo { x: 5 }
}
"
);

insta::assert_snapshot!(output.bytecode);

assert_eq!(
output.result,
Ok(BexExternalValue::instance(
"Foo",
indexmap! {
"x" => BexExternalValue::Int(5),
}
))
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add one runtime regression for aliased object spreads.

lower.rs now resolves aliases in both object-literal branches, but every new test here only exercises the no-spread path. Please add one end-to-end case that uses the spread form of an aliased object literal so the new class_fields lookup branch is covered too.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1385be7393

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +353 to +355
match self.resolve_alias(&alias_ty) {
Ty::Class(qn) => Ty::Class(qn.clone()),
_ => Ty::Unknown,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve alias identity for thrown object literals

When an object literal is written with a type alias (e.g. throw Bar { ... }), this branch now rewrites the inferred type to Ty::Class(user.Foo) instead of keeping Ty::TypeAlias(user.Bar). In compiler2 this breaks throws contract checking because declared facts are still collected as Bar while effective thrown facts become Foo, producing a false throws contract violation/extraneous throws declaration for valid code like function f() -> int throws Bar { throw Bar { x: 1 }; }.

Useful? React with 👍 / 👎.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 10, 2026

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 15 untouched benchmarks
⏩ 91 skipped benchmarks1


Comparing push-vnznwqtvoutk (0f7462e) with canary (06910c5)

Open in CodSpeed

Footnotes

  1. 91 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (4)
baml_language/crates/baml_compiler_mir/src/lower.rs (1)

223-233: ⚠️ Potential issue | 🔴 Critical

Match the actual TIR variants and preserve the canonical class key.

type_aliases stores baml_compiler_tir::Ty, and the provided enum shape is Ty::Class(qn) / Ty::TypeAlias(qn), not two-field variants. Also, Line 231 returning qn.name still strips the module/display portion, so aliases to imported or builtin classes will keep missing the class_fields / emitter lookups this PR is trying to fix. Match the single payload and return the same canonical name shape class_name_from_ty() uses.

Possible fix
-            match self.type_aliases.get(&current) {
-                Some(baml_compiler_tir::Ty::Class(qn, _)) => return qn.name.to_string(),
-                Some(baml_compiler_tir::Ty::TypeAlias(qn, _)) => {
-                    current = qn.name.clone();
-                }
+            match self.type_aliases.get(&current) {
+                Some(baml_compiler_tir::Ty::Class(qn)) => {
+                    return qn.display_name.to_string();
+                }
+                Some(baml_compiler_tir::Ty::TypeAlias(qn)) => {
+                    current = qn.name.clone();
+                }
                 _ => break,
             }

Run this to verify the enum shape and the current match arms:

#!/bin/bash
set -euo pipefail

echo "== TIR Ty enum definitions =="
fd 'ty.rs' baml_language/crates --exec sh -c '
  for file; do
    if rg -n "pub enum Ty" "$file" >/dev/null; then
      echo "-- $file --"
      rg -n -A20 -B2 "pub enum Ty|Class\\(|TypeAlias\\(" "$file"
    fi
  done
' sh {} +

echo
echo "== Current helper match arms =="
rg -n -C2 'Ty::Class\(qn, _\)|Ty::TypeAlias\(qn, _\)|qn\.name|display_name' \
  baml_language/crates/baml_compiler_mir/src/lower.rs

Expected result: the TIR enum shows single-payload Class / TypeAlias variants, and this helper is where the short-name collapse happens today.

baml_language/crates/baml_compiler_tir/src/lib.rs (1)

4636-4670: ⚠️ Potential issue | 🟠 Major

Resolve aliases to non-class receivers before falling back to builtins.

This loop only unwraps alias chains when they end at Ty::Class. If an alias resolves to Ty::List, Ty::Map, Ty::String, etc., it breaks with result = None, and Line 4734 still calls builtins::lookup_method(base, ...) on the original Ty::TypeAlias. Aliased builtin receivers like type IntList = int[]; xs.length() will still miss builtin methods. Either carry the resolved non-class Ty into the builtin lookup path, or make builtin pattern matching unwrap Ty::TypeAlias first.

baml_language/crates/baml_compiler2_tir/src/builder.rs (1)

2079-2093: 🧹 Nitpick | 🔵 Trivial

Value equality check for alias resolution differs from other locations.

This location uses value equality (resolved == *base_ty) after cloning, while other locations (lines 1091, 1453, 2153) use std::ptr::eq(resolved, ty) without cloning. Both approaches are functionally correct for detecting "no resolution occurred," but the inconsistency could cause confusion during maintenance.

Consider aligning with the std::ptr::eq pattern for consistency:

♻️ Suggested refactor for consistency
 Ty::TypeAlias(_) => {
-    let resolved = self.resolve_alias(base_ty).clone();
-    if resolved == *base_ty {
+    let resolved = self.resolve_alias(base_ty);
+    if std::ptr::eq(resolved, base_ty) {
         self.context.report_simple(
             TirTypeError::UnresolvedMember {
                 base_type: base_ty.clone(),
                 member: member.clone(),
             },
             at,
         );
         Ty::Unknown
     } else {
-        self.resolve_member(&resolved, member, at)
+        self.resolve_member(resolved, member, at)
     }
 }
baml_language/crates/baml_tests/tests/type_aliases.rs (1)

11-109: 🧹 Nitpick | 🔵 Trivial

Add a test case for aliased object spreads.

The test suite comprehensively covers struct literal construction and field access through aliases, but per past review feedback, there's no test exercising the spread form of an aliased object literal. Since lower.rs now resolves aliases in both object-literal branches, please add one end-to-end case that uses the spread form (e.g., ...baseObj) so the new class_fields lookup branch is covered.

Example test scenario:

#[tokio::test]
async fn alias_struct_literal_with_spread() {
    let output = baml_test!(
        "
        class Foo { x int  y int }
        type Bar = Foo;

        function main() -> int {
            let base = Bar { x: 1, y: 2 };
            let extended = Bar { ...base, y: 10 };
            extended.x + extended.y
        }
    "
    );
    // Expect result: 1 + 10 = 11
}

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d81cee77-eac6-4ded-ae61-05b847733f50

📥 Commits

Reviewing files that changed from the base of the PR and between 1385be7 and 0f7462e.

⛔ Files ignored due to path filters (14)
  • baml_language/crates/baml_tests/snapshots/format_checks/baml_tests__format_checks__06_codegen.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/snapshots/match_exhaustiveness/baml_tests__match_exhaustiveness__06_codegen.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_textual.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__alias_in_return_type_annotation.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__alias_multi_field_access.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__alias_param_field_access.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__chained_alias_field_access.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__chained_alias_struct_literal.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__field_access_through_alias.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__mir_alias_aggregate_resolution.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__mir_chained_alias_aggregate_resolution.snap is excluded by !**/*.snap
  • baml_language/crates/baml_tests/tests/snapshots/type_aliases__struct_literal_through_alias.snap is excluded by !**/*.snap
📒 Files selected for processing (7)
  • baml_language/crates/baml_compiler2_tir/src/builder.rs
  • baml_language/crates/baml_compiler_emit/src/lib.rs
  • baml_language/crates/baml_compiler_mir/src/lower.rs
  • baml_language/crates/baml_compiler_tir/src/lib.rs
  • baml_language/crates/baml_tests/src/compiler2_tir/inference.rs
  • baml_language/crates/baml_tests/tests/match_types.rs
  • baml_language/crates/baml_tests/tests/type_aliases.rs
💤 Files with no reviewable changes (1)
  • baml_language/crates/baml_tests/tests/match_types.rs

Comment on lines 474 to 483
let inference = baml_compiler_tir::infer_function(
db,
&signature,
Some(&sig_source_map),
&body,
Some(typing_context.clone()),
Some(class_field_types.clone()),
None, // type_aliases - not needed for codegen
Some(type_aliases.clone()),
Some(enum_variant_names.clone()), // enum_variants - needed for enum variant detection
*func_loc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check if tools_onionskin/src/compiler.rs exists and inspect the claimed lines
fd -t f "compiler.rs" -p "*/tools_onionskin/*"

Repository: BoundaryML/baml

Length of output: 165


🏁 Script executed:

# Search for all infer_function calls to understand the scope
rg "infer_function\(" --type rs -n

Repository: BoundaryML/baml

Length of output: 85


🏁 Script executed:

# Get context on the tools_onionskin file structure
git ls-files | grep -E "tools_onionskin|baml_compiler_emit" | head -20

Repository: BoundaryML/baml

Length of output: 859


🏁 Script executed:

# Search for all infer_function calls with proper rg syntax
rg "infer_function\(" --context 2

Repository: BoundaryML/baml

Length of output: 5089


🏁 Script executed:

# Inspect the tools_onionskin compiler.rs around the claimed lines
wc -l baml_language/crates/tools_onionskin/src/compiler.rs

Repository: BoundaryML/baml

Length of output: 116


🏁 Script executed:

# Check baml_compiler_emit lib.rs for the changed lines
sed -n '470,490p' baml_language/crates/baml_compiler_emit/src/lib.rs | cat -n

Repository: BoundaryML/baml

Length of output: 1238


🏁 Script executed:

# Find the exact line numbers of infer_function calls in tools_onionskin
rg "infer_function\(" baml_language/crates/tools_onionskin/src/compiler.rs -n

Repository: BoundaryML/baml

Length of output: 307


🏁 Script executed:

# Get detailed context for each infer_function call in tools_onionskin (first one)
sed -n '3560,3590p' baml_language/crates/tools_onionskin/src/compiler.rs | cat -n

Repository: BoundaryML/baml

Length of output: 1640


🏁 Script executed:

# Get context for all three infer_function calls in tools_onionskin
rg "infer_function\(" baml_language/crates/tools_onionskin/src/compiler.rs -A 10 -B 2

Repository: BoundaryML/baml

Length of output: 1743


Update the infer_function call at line 3570 in tools_onionskin/src/compiler.rs to pass type_aliases.

This call currently passes None for the type_aliases parameter, while two other calls in the same file (lines 3211 and 3319) and the emit path have been updated to pass Some(type_aliases_map.clone()). This inconsistency will cause alias-typed parameters and member access to behave differently in the onionskin tool. Update line 3570 to match the pattern, or document if the onionskin path is intentionally alias-unaware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant