Recursive SpannedTypeExpr for precise type-error diagnostics#3220
Recursive SpannedTypeExpr for precise type-error diagnostics#3220
Conversation
Before this change, type expressions were stored as a single TypeExpr plus
one span, so diagnostics (e.g. "unresolved type") pointed at the whole
annotation. Now every sub-expression has its own span, so we can highlight
only the part that's wrong.
Examples of impact:
- Union with one bad member: For `x: int | sring | bool`, we now underline
only `sring` (e.g. 14..20) with "unresolved type: `sring`" instead of
the whole `int | sring | bool`.
- Bad return type: For `function f() -> DoesNotExist { ... }`, the error is
on `DoesNotExist` (e.g. 16..28), not the entire `-> DoesNotExist`.
- Missing return: For a function declared `-> int` with no return, the
"missing return" diagnostic is on the return type annotation `: int`
(e.g. 16..19), not the block body, so it's clear which function is wrong.
- Parameter type: For `fn f(x: Nonexistent)`, the diagnostic targets the
`Nonexistent` token (e.g. 11..25), not the whole parameter.
What changed:
- AST: SpannedTypeExpr is recursive (SpannedTypeExprKind with spanned
children). Add to_type_expr() for span-free TypeExpr.
- CST→AST: Type lowering produces SpannedTypeExpr with per-node
trimmed_text_range(); lower_cst uses it directly.
- HIR: SignatureSourceMap stores full SpannedTypeExpr trees for params,
return, and throws.
- TIR: Add lower_spanned_type_expr(), report (TirTypeError, span) per node;
use for return/param/field/alias. Report "missing return" on return-type
span via return_type_span.
- LSP: check uses lower_spanned_type_expr for precise spans.
- Syntax: UnionMemberParts::text_range(), TypeExpr::trimmed_text_range().
- Tests and tools_onionskin updated; UnresolvedType message uses backticks.
Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a recursive, span-bearing TypeExpr (SpannedTypeExpr) produced by CST->AST lowering, stores spanned trees in source/signature maps, converts to plain TypeExpr via to_type_expr(), and introduces span-aware lowering to Ty that collects diagnostics with precise TextRange locations. Changes
Sequence Diagram(s)sequenceDiagram
participant CST as CST (parser)
participant ASTL as AST Lowerer
participant Spanned as SpannedTypeExpr (AST)
participant HIR as HIR Signature/SourceMap
participant TIRL as TIR Lowerer
participant Builder as TypeInferenceBuilder
participant Reporter as Diagnostics Reporter / LSP
CST->>ASTL: provide CstTypeExpr
ASTL->>Spanned: lower to SpannedTypeExpr (recursive, per-node spans)
Spanned->>HIR: store SpannedTypeExpr in SignatureSourceMap / AstSourceMap
HIR->>Builder: supply SpannedTypeExpr entries
Builder->>TIRL: call lower_spanned_type_expr(spanned)
TIRL->>Builder: return Ty and (error, TextRange) diagnostics
Builder->>Reporter: report diagnostics at provided TextRange
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 not alter performance
|
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aacb7e5d-a876-4d09-af37-c9c1f2652992
📒 Files selected for processing (14)
baml_language/crates/baml_compiler2_ast/src/ast.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/signature.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/tools_onionskin/src/compiler.rs
- Fix use-after-move in lib.rs test (match instead of matches! + panic) - Distinct spans for synthetic container levels (lower_type_expr + test) - Skip field diagnostics in lookup_class_fields (avoid duplicates) - check_throws_contract: use throws_type_expr + lower_spanned_type_expr - LSP check: validate throws with lower_spanned_type_expr - Onionskin: hir2_spanned_type_expr_to_string helper - phase3a: unknown_type_in_union_return, unknown_type_in_throws Made-with: Cursor
…agnostics) - AstSourceMap: add type_annotation_spanned_exprs + type_annotation_spanned(id) - alloc_type_annot: take SpannedTypeExpr, store ty + span + spanned - Pattern::TypedBinding: ty is SpannedTypeExpr; consumers use .to_type_expr() - TIR: set_body_source_map from function_body_source_map; in check_stmt(Let) use lower_spanned_type_expr when spanned available, report (diag, span) per node - Tests: unknown_type_in_let_binding, unknown_type_in_let_binding_union snapshots updated for precise spans (e.g. 35..40 for sring only) Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
baml_language/crates/baml_compiler2_ast/src/lower_type_expr.rs (1)
43-47:⚠️ Potential issue | 🟠 MajorWrapper span math still drifts off the real modifier tokens.
Line 44 records
[]as a 1-byte span, and Lines 67-74 and 290-299 then build later wrappers fromresult.span.end(). After the first synthetic wrap, that end no longer matches the actual source suffix, soFoo[]?can attach the optional span to], and a flatint[][]path can even step pastfull_span.end(). Please derive each synthetic container span from the original source range (or a cumulative sub-expression range) instead of chaining through prior synthetic spans, and tighten the regression test to assert exact ranges/slices rather than just!=.As per coding guidelines,
Prefer writing Rust unit tests over integration tests where possible.Also applies to: 67-74, 289-299
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cb03c42-1995-4fbd-b33b-d39b4190680d
📒 Files selected for processing (7)
baml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/tools_onionskin/src/compiler.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
967-975:⚠️ Potential issue | 🟡 MinorReport invalid catch-binding types at the annotation span.
Pattern::TypedBinding.tyalready has the exactTextRange, but this still anchorsInvalidCatchBindingTypetobase_expr_id, so the underline lands on the catch expression instead of the banned type.🩹 Precise-span fix
- self.context.report_simple( - TirTypeError::InvalidCatchBindingType { - type_name: banned.to_string(), - }, - base_expr_id, - ); + self.context.report_at_span( + TirTypeError::InvalidCatchBindingType { + type_name: banned.to_string(), + }, + ty.span, + );
♻️ Duplicate comments (2)
baml_language/crates/baml_compiler2_tir/src/builder.rs (2)
415-422:⚠️ Potential issue | 🟠 MajorScope
MissingReturn’s return-type span to the root function body only.
check_expris used for every block expression. With the builder-widereturn_type_span, nested blocks likelet x: int = { ... }will still blame the function’s-> Tspan instead of the offending block/check site.
1249-1250:⚠️ Potential issue | 🟠 MajorTyped match/catch bindings still erase their recursive spans before lowering.
These
ty.to_type_expr()calls undo the newPattern::TypedBindingstorage, so unresolved or malformed types in typedmatch/catchbindings still report on the arm body instead of the bad type token. Lower the storedSpannedTypeExprdirectly and forward its(diag, span)results.💡 Minimal call-site change
- self.lower_pattern_type_expr(&ty.to_type_expr(), at_expr) + self.lower_spanned_pattern_type_expr(ty, at_expr)- let lowered = self.lower_pattern_type_expr(&ty.to_type_expr(), at_expr); + let lowered = self.lower_spanned_pattern_type_expr(ty, at_expr);Also applies to: 1407-1408
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f169d01-83ff-4938-aa1a-d748a6f5b110
📒 Files selected for processing (7)
baml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/tools_onionskin/src/compiler.rs
1 participant
Before this change, type expressions were stored as a single TypeExpr plus one span, so diagnostics (e.g. "unresolved type") pointed at the whole annotation. Now every sub-expression has its own span, so we can highlight only the part that's wrong.
Examples of impact
Union with one bad member: For
x: int | sring | bool, we now underline onlysring(e.g. 14..20) with "unresolved type:sring" instead of the wholeint | sring | bool.Bad return type: For
function f() -> DoesNotExist { ... }, the error is onDoesNotExist(e.g. 16..28), not the entire-> DoesNotExist.Missing return: For a function declared
-> intwith no return, the "missing return" diagnostic is on the return type annotation: int(e.g. 16..19), not the block body, so it's clear which function is wrong.Parameter type: For
fn f(x: Nonexistent), the diagnostic targets theNonexistenttoken (e.g. 11..25), not the whole parameter.What changed
to_type_expr()for span-free TypeExpr.trimmed_text_range(); lower_cst uses it directly.lower_spanned_type_expr(), report(TirTypeError, span)per node; use for return/param/field/alias. Report "missing return" on return-type span viareturn_type_span.lower_spanned_type_exprfor precise spans.UnionMemberParts::text_range(),TypeExpr::trimmed_text_range().Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests