baml-language: Add definition spans to argument count and missing field diagnostics#3142
baml-language: Add definition spans to argument count and missing field diagnostics#3142miguelcsx wants to merge 4 commits intoBoundaryML:canaryfrom
Conversation
|
@miguelcsx is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughAdds optional definition_location to several TypeError variants and threads definition-span resolution from HIR/TIR into diagnostics, emitting conditional secondary notes ("Function defined here" / "Type defined here") when a definition span is available. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_compiler_diagnostics/src/to_diagnostic.rs (1)
1035-1098: 🛠️ Refactor suggestion | 🟠 MajorMissing unit tests for
definition_locationpaths inArgumentCountMismatchandNoSuchField.The existing suite covers
TypeMismatchwith and withoutinfo_location(lines 1064-1097) but has no equivalent coverage for the two new variants. Consider adding:#[test] fn test_argument_count_mismatch_with_definition_location() { let def_span = Span { file_id: baml_base::FileId::new(1), range: TextRange::new(5.into(), 10.into()), }; let error: TypeError<SpanContext> = TypeError::ArgumentCountMismatch { expected: 1, found: 2, location: test_span(), definition_location: Some(def_span), }; let diag = error.to_diagnostic(Clone::clone, |s| *s); assert_eq!(diag.code(), "E0004"); // or whichever DiagnosticId maps here assert_eq!(diag.annotations.len(), 2); // primary + "Function defined here" assert_eq!(diag.phase, DiagnosticPhase::Type); } #[test] fn test_no_such_field_with_definition_location() { let def_span = Span { file_id: baml_base::FileId::new(1), range: TextRange::new(5.into(), 10.into()), }; let error: TypeError<SpanContext> = TypeError::NoSuchField { ty: "Person".to_string(), field: "age".to_string(), location: test_span(), definition_location: Some(def_span), }; let diag = error.to_diagnostic(Clone::clone, |s| *s); assert_eq!(diag.annotations.len(), 2); // primary + "Type defined here" assert_eq!(diag.phase, DiagnosticPhase::Type); }As per coding guidelines, Rust unit tests are preferred over integration tests where possible.
miguelcsx
force-pushed
the
refac/typerror
branch
from
4210212 to
7edc2da
Compare
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 (2)
baml_language/crates/baml_compiler_diagnostics/src/to_diagnostic.rs (1)
115-155: 🧹 Nitpick | 🔵 TrivialAdd unit tests for
ArgumentCountMismatchandNoSuchFieldwith and withoutdefinition_location.The existing test suite covers
TypeMismatchwith/withoutinfo_location(verifying annotation count) but there are no tests for the two changed arms. The critical behavior is: annotations count is 2 whendefinition_locationisSome, 1 whenNone.💡 Suggested tests to add in the `#[cfg(test)]` block
+ #[test] + fn test_argument_count_mismatch_without_def_location() { + let error: TypeError<SpanContext> = TypeError::ArgumentCountMismatch { + expected: 1, + found: 2, + location: test_span(), + definition_location: None, + }; + let diag = error.to_diagnostic(Clone::clone, |s| *s); + assert_eq!(diag.annotations.len(), 1); + assert!(diag.message.contains("Expected 1 arguments, found 2")); + assert_eq!(diag.phase, DiagnosticPhase::Type); + } + + #[test] + fn test_argument_count_mismatch_with_def_location() { + let def_span = Span { + file_id: baml_base::FileId::new(1), + range: TextRange::new(5.into(), 15.into()), + }; + let error: TypeError<SpanContext> = TypeError::ArgumentCountMismatch { + expected: 1, + found: 2, + location: test_span(), + definition_location: Some(def_span), + }; + let diag = error.to_diagnostic(Clone::clone, |s| *s); + assert_eq!(diag.annotations.len(), 2); // primary + "Function defined here" + assert_eq!(diag.phase, DiagnosticPhase::Type); + } + + #[test] + fn test_no_such_field_with_def_location() { + let def_span = Span { + file_id: baml_base::FileId::new(1), + range: TextRange::new(5.into(), 15.into()), + }; + let error: TypeError<SpanContext> = TypeError::NoSuchField { + ty: "Person".to_string(), + field: "age".to_string(), + location: test_span(), + definition_location: Some(def_span), + }; + let diag = error.to_diagnostic(Clone::clone, |s| *s); + assert_eq!(diag.annotations.len(), 2); // primary + "Type defined here" + assert_eq!(diag.phase, DiagnosticPhase::Type); + }As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible."
baml_language/crates/baml_compiler_diagnostics/src/errors/type_error.rs (1)
72-86:⚠️ Potential issue | 🔴 CriticalFix match patterns in
error_format.rs— field names do not match enum definition.The match patterns for
ArgumentCountMismatchandNoSuchFieldinerror_format.rsreference aspanfield that does not exist. The enum variants intype_error.rsdefinelocationanddefinition_locationinstead. This causes a compilation error:
ArgumentCountMismatchpattern usesspanbut enum defineslocationanddefinition_locationNoSuchFieldpattern usesspanbut enum defineslocationanddefinition_locationUpdate both patterns in
baml_language/crates/baml_compiler_diagnostics/src/compiler_error/error_format.rsto destructurelocationanddefinition_locationfields instead ofspan.
miguelcsx
force-pushed
the
refac/typerror
branch
from
7edc2da to
616cd7b
Compare
There was a problem hiding this comment.
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_compiler_diagnostics/src/to_diagnostic.rs (1)
1035-1143: 🧹 Nitpick | 🔵 TrivialAdd unit tests for
ArgumentCountMismatchandNoSuchFieldwithdefinition_location.The existing test suite covers
TypeMismatchwithinfo_location(line 1080) but has no analogous tests exercising the newdefinition_locationfield onArgumentCountMismatchorNoSuchField. Two tests would be valuable: one verifying the secondary annotation appears whendefinition_locationisSome, and one verifying it's absent whenNone.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible."
🧪 Suggested test sketch
+ #[test] + fn test_argument_count_mismatch_with_definition_location() { + let error: TypeError<SpanContext> = TypeError::ArgumentCountMismatch { + expected: 1, + found: 2, + location: test_span(), + definition_location: Some(Span { + file_id: baml_base::FileId::new(0), + range: TextRange::new(20.into(), 30.into()), + }), + }; + + let diag = error.to_diagnostic(Clone::clone, |s| *s); + assert_eq!(diag.code(), "E0006"); + assert!(diag.message.contains("Expected 1 arguments")); + assert_eq!(diag.annotations.len(), 2); // primary + definition + assert_eq!(diag.phase, DiagnosticPhase::Type); + } + + #[test] + fn test_no_such_field_with_definition_location() { + let error: TypeError<SpanContext> = TypeError::NoSuchField { + ty: "Person".to_string(), + field: "age".to_string(), + location: test_span(), + definition_location: Some(Span { + file_id: baml_base::FileId::new(0), + range: TextRange::new(40.into(), 50.into()), + }), + }; + + let diag = error.to_diagnostic(Clone::clone, |s| *s); + assert_eq!(diag.code(), "E0008"); + assert!(diag.message.contains("no field `age`")); + assert_eq!(diag.annotations.len(), 2); // primary + definition + assert_eq!(diag.phase, DiagnosticPhase::Type); + }
miguelcsx
force-pushed
the
refac/typerror
branch
from
616cd7b to
bacd138
Compare
| fn fqn_symbol_table(&self) -> baml_compiler_hir::SymbolTable<'db> { | ||
| let project = self.db.project(); | ||
| baml_compiler_hir::symbol_table(self.db, project) | ||
| } | ||
|
|
||
| /// Returns a position-independent location for the definition of a callable value. | ||
| /// | ||
| /// Uses `ErrorLocation::FunctionItem` so that `InferenceResult` does not embed | ||
| /// spans from the callee's file. Span resolution happens at render time via | ||
| /// `project_function_item_spans`, which is intentionally separate from the | ||
| /// type-inference Salsa query graph. | ||
| fn function_definition_location( | ||
| &self, | ||
| fqn: &baml_compiler_hir::QualifiedName, | ||
| ) -> Option<ErrorLocation> { | ||
| // Verify the function exists before recording the location. | ||
| self.fqn_symbol_table().lookup_value(self.db, fqn)?; | ||
| Some(ErrorLocation::FunctionItem(fqn.clone())) | ||
| } | ||
|
|
||
| /// Returns a position-independent location for a type definition. | ||
| /// | ||
| /// Uses the existing `ErrorLocation::TypeItem(Name)` variant, resolved at | ||
| /// render time by `project_type_item_spans`. No syntax-tree access occurs | ||
| /// inside the type-inference query. | ||
| fn type_definition_location( | ||
| &self, | ||
| fqn: &baml_compiler_hir::QualifiedName, | ||
| ) -> Option<ErrorLocation> { | ||
| // Verify the type exists before recording the location. | ||
| self.fqn_symbol_table().lookup_type(self.db, fqn)?; | ||
| Some(ErrorLocation::TypeItem(fqn.name.clone())) | ||
| } | ||
|
|
||
| fn call_definition_location(&self, callee: ExprId) -> Option<ErrorLocation> { | ||
| self.expr_resolutions | ||
| .get(&callee) | ||
| .and_then(ResolvedValue::as_function) | ||
| .and_then(|qn| self.function_definition_location(qn)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing unit tests for new helper methods
fqn_symbol_table, function_definition_location, type_definition_location, and call_definition_location are the only new logic added to TypeContext and have no unit-test coverage. At minimum, type_definition_location (which has the correctness risk above) and call_definition_location (timing dependency on expr_resolutions) warrant targeted tests.
Based on coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".
| fn type_definition_location( | ||
| &self, | ||
| fqn: &baml_compiler_hir::QualifiedName, | ||
| ) -> Option<ErrorLocation> { | ||
| // Verify the type exists before recording the location. | ||
| self.fqn_symbol_table().lookup_type(self.db, fqn)?; | ||
| Some(ErrorLocation::TypeItem(fqn.name.clone())) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ErrorLocation definition to check TypeItem's field type
rg "enum ErrorLocation|struct ErrorLocation" --type=rs -A 20Repository: BoundaryML/baml
Length of output: 85
🏁 Script executed:
# Find TypeItem usage in rendering or resolution
rg "TypeItem" --type=rs -B 2 -A 3 | head -100Repository: BoundaryML/baml
Length of output: 85
🏁 Script executed:
# Verify the file structure and line numbers
wc -l baml_language/crates/baml_compiler_tir/src/lib.rsRepository: BoundaryML/baml
Length of output: 113
🏁 Script executed:
# Find ErrorLocation definition without type flag
rg "enum ErrorLocation|struct ErrorLocation" -A 20 | head -80Repository: BoundaryML/baml
Length of output: 4885
🏁 Script executed:
# Find files that define ErrorLocation
fd "\.rs$" | xargs rg -l "enum ErrorLocation" | head -5Repository: BoundaryML/baml
Length of output: 115
🏁 Script executed:
# Look at lines around 821-828 in the target file
sed -n '810,835p' baml_language/crates/baml_compiler_tir/src/lib.rsRepository: BoundaryML/baml
Length of output: 1139
🏁 Script executed:
# Get the full ErrorLocation enum definition
sed -n '1,150p' baml_language/crates/baml_compiler_hir/src/source_map.rs | tail -60Repository: BoundaryML/baml
Length of output: 2904
🏁 Script executed:
# Find FunctionItem variant in ErrorLocation
rg "FunctionItem" baml_language/crates/baml_compiler_hir/src/source_map.rs -B 2 -A 2Repository: BoundaryML/baml
Length of output: 637
🏁 Script executed:
# Find the rendering/resolution code that uses TypeItem
rg "project_type_item_spans|TypeItem\(" baml_language/crates/baml_compiler_hir/src/ -A 5 | head -80Repository: BoundaryML/baml
Length of output: 3628
🏁 Script executed:
# Check if there are namespace-aware types (builtin types with namespaces)
rg "baml\.http|baml\." baml_language/crates/baml_compiler_hir/src/ | head -20Repository: BoundaryML/baml
Length of output: 2569
🏁 Script executed:
# Get the full implementation of project_type_item_spans
rg "pub fn project_type_item_spans" baml_language/crates/baml_compiler_hir/src/lib.rs -A 30Repository: BoundaryML/baml
Length of output: 1251
🏁 Script executed:
# Check if any builtin types are registered in type_spans
rg "project_type_item_spans\|type_spans" baml_language/crates/baml_compiler_hir/src/lib.rs -B 3 -A 10 | head -100Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check if project_items includes any builtin types or if it's user-defined only
rg "pub fn project_items" baml_language/crates/baml_compiler_hir/src/lib.rs -A 20Repository: BoundaryML/baml
Length of output: 913
🏁 Script executed:
# Search for where builtin types are defined/registered
rg "baml\.http\|baml\.llm" baml_language/crates/baml_compiler_hir/src/lib.rs -B 2 -A 2 | head -40Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check how builtin types are handled in symbol resolution
rg "lookup_type|builtin" baml_language/crates/baml_compiler_hir/src/symbol_table.rs -B 2 -A 5 | head -80Repository: BoundaryML/baml
Length of output: 315
🏁 Script executed:
# Verify whether namespaces are actually used in user-defined types or only for builtins
fd "symbol_table" baml_language/crates/baml_compiler_hir/src/ | head -3Repository: BoundaryML/baml
Length of output: 117
🏁 Script executed:
# Find where type_definition_location is called
rg "type_definition_location" baml_language/crates/baml_compiler_tir/src/lib.rs -B 3 -A 3Repository: BoundaryML/baml
Length of output: 616
🏁 Script executed:
# Check the symbol_table.rs to see how builtin types are handled
sed -n '1,100p' baml_language/crates/baml_compiler_hir/src/symbol_table.rsRepository: BoundaryML/baml
Length of output: 3530
🏁 Script executed:
# Check if there's a separate builtin type resolution path
rg "builtin.*type|BuiltinType" baml_language/crates/baml_compiler_hir/src/ -l | head -5Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check if builtin types are registered anywhere in type resolution
rg "baml\.http|baml\.Array|builtin.*Response" baml_language/crates/ --type-list | head -10Repository: BoundaryML/baml
Length of output: 448
🏁 Script executed:
# Look for where builtin types like Request/Response are defined
fd "Request|Response" baml_language/crates/baml_compiler_hir/src/ -type f | head -10Repository: BoundaryML/baml
Length of output: 229
🏁 Script executed:
# Check the actual usage of type_definition_location more thoroughly
sed -n '3440,3470p' baml_language/crates/baml_compiler_tir/src/lib.rsRepository: BoundaryML/baml
Length of output: 789
🏁 Script executed:
# Verify what types can actually hit the type_definition_location code path
rg "Ty::TypeAlias|Ty::Class" baml_language/crates/baml_compiler_tir/src/lib.rs -B 5 -A 5 | head -50Repository: BoundaryML/baml
Length of output: 2282
🏁 Script executed:
# Check if builtin types are part of the type system at all
rg "parse_builtin_path" baml_language/crates/baml_compiler_tir/src/lib.rs -B 3 -A 3Repository: BoundaryML/baml
Length of output: 636
🏁 Script executed:
# See what Ty::Class can be - both builtin and user-defined
rg "Ty::Class\(" baml_language/crates/baml_compiler_tir/src/lib.rs | head -20Repository: BoundaryML/baml
Length of output: 643
🏁 Script executed:
# Check if symbol_table.lookup_type can find builtin types
rg "lookup_type" baml_language/crates/baml_compiler_hir/src/symbol_table.rs -A 10Repository: BoundaryML/baml
Length of output: 334
🏁 Script executed:
# Check where types method gets its data from
sed -n '80,200p' baml_language/crates/baml_compiler_hir/src/symbol_table.rsRepository: BoundaryML/baml
Length of output: 4231
🏁 Script executed:
# Check if symbol_table includes any builtin file-based classes
rg "baml\.llm\|baml\.http" baml_language/crates/baml_compiler_hir/src/symbol_table.rsRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Verify if lookup_type can actually succeed for builtin types
rg "class_qualified_name" baml_language/crates/baml_compiler_hir/src/ -A 3 | head -30Repository: BoundaryML/baml
Length of output: 1319
🏁 Script executed:
# Check if there are actual namespaced user-defined classes
rg "QualifiedName.*baml\.|namespace.*baml\." baml_language/crates/ -l | head -5Repository: BoundaryML/baml
Length of output: 344
🏁 Script executed:
# Look for test cases that might exercise type_definition_location with namespaced types
rg "type_definition_location" baml_language/crates/baml_compiler_tir/src/ -B 5 -A 5Repository: BoundaryML/baml
Length of output: 1997
🏁 Script executed:
# Check if lookup_type has any special handling for builtin types
sed -n '80,150p' baml_language/crates/baml_compiler_hir/src/symbol_table.rsRepository: BoundaryML/baml
Length of output: 2272
🏁 Script executed:
# Verify that project_items includes both user-defined and builtin file classes
rg "project_items\|builtin.*file" baml_language/crates/baml_compiler_hir/src/lib.rs -B 3 -A 10 | head -60Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check if there are any user-defined types in multiple namespaces (non-Local)
rg "QualifiedName::" baml_language/crates/baml_compiler_hir/src/ | grep -v "local\|Local" | head -10Repository: BoundaryML/baml
Length of output: 334
🏁 Script executed:
# Verify how namespace is used - all user types should be Local namespace
rg "Namespace::" baml_language/crates/baml_base/src/qualified_name.rs -B 2 -A 5Repository: BoundaryML/baml
Length of output: 6983
🏁 Script executed:
# Check if builtin types can appear in Ty::Class or Ty::TypeAlias during type checking
rg "Ty::Class.*baml\.|parse_builtin_path" baml_language/crates/baml_compiler_tir/src/lib.rs -B 3 -A 3 | head -40Repository: BoundaryML/baml
Length of output: 636
🏁 Script executed:
# Verify what happens when lookup_type is called with a BamlStd namespace
sed -n '140,180p' baml_language/crates/baml_compiler_hir/src/symbol_table.rsRepository: BoundaryML/baml
Length of output: 1355
🏁 Script executed:
# Check if project_items includes builtin file classes (those in baml.* namespaces)
rg "project_items\|ItemId::" baml_language/crates/baml_compiler_hir/src/lib.rs | grep -A 5 "pub fn project_items"Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Look for actual error cases with builtin types
rg "baml\.http\|baml\.llm" baml_language/crates/baml_compiler_tir/src/lib.rs -B 2 -A 2Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Verify that builtin types will return None from lookup_type
rg "Namespace::BamlStd|Namespace::Builtin" baml_language/crates/baml_base/src/qualified_name.rs -B 2 -A 2 | head -20Repository: BoundaryML/baml
Length of output: 816
🏁 Script executed:
# Check if there are any scenarios where a BamlStd/Builtin type reaches type_definition_location
rg "lookup_type.*fqn\|BamlStd.*lookup" baml_language/crates/ -B 3 -A 3 | head -40Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Verify that project_type_item_spans only contains user-defined types
sed -n '1,80p' baml_language/crates/baml_compiler_hir/src/lib.rs | grep -A 30 "project_type_item_spans"Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check one critical thing - can builtin types appear in Ty::Class that reaches type_definition_location?
rg "infer_field_access" baml_language/crates/baml_compiler_tir/src/lib.rs -A 30 | head -50Repository: BoundaryML/baml
Length of output: 2678
🏁 Script executed:
# Trace where Ty::Class values come from in infer_field_access context
sed -n '3435,3465p' baml_language/crates/baml_compiler_tir/src/lib.rsRepository: BoundaryML/baml
Length of output: 899
🏁 Script executed:
# Verify: if lookup_type fails (e.g. for builtin types), does type_definition_location return None?
sed -n '821,828p' baml_language/crates/baml_compiler_tir/src/lib.rsRepository: BoundaryML/baml
Length of output: 374
🏁 Script executed:
# Critical check: Are builtin file classes (e.g., baml.http.Response) included in project_items and symbol_table?
rg "class_qualified_name" baml_language/crates/baml_compiler_hir/src/lib.rs -A 15 | head -40Repository: BoundaryML/baml
Length of output: 1049
🏁 Script executed:
# Check what namespace builtin file classes get
rg "BamlStd\|baml_std" baml_language/crates/baml_compiler_hir/src/lib.rs -B 2 -A 2 | head -40Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Final verification: are builtin file classes stored with BamlStd namespace in symbol_table?
rg "class_qualified_name.*baml\|ItemId::Class" baml_language/crates/baml_compiler_hir/src/lib.rs -B 3 -A 8Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check if project_type_item_spans includes BOTH builtin file classes and user-defined classes
sed -n '10,60p' baml_language/crates/baml_compiler_hir/src/lib.rs | grep -A 40 "project_type_item_spans"Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Verify if a user can define a class with the same name as a builtin (e.g., Response)
rg "class.*Response|class.*Object" baml_language/crates/baml_compiler_hir/tests/ -l | head -3Repository: BoundaryML/baml
Length of output: 144
🏁 Script executed:
# Look for test files that might exercise name collisions
fd "test" baml_language/crates/baml_compiler_hir/ -type f | head -5Repository: BoundaryML/baml
Length of output: 229
🏁 Script executed:
# Check if root.files() includes builtin files
rg "fn files\|builtin.*file" baml_language/crates/baml_workspace/src/ -B 2 -A 5 | head -40Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Determine: does symbol_table actually contain builtin types, or only user-defined?
rg "symbol_table\|project_items" baml_language/crates/baml_compiler_tir/src/lib.rs -B 2 -A 10 | head -60Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Final check: trace the actual call to type_definition_location - what types reach it?
sed -n '3445,3465p' baml_language/crates/baml_compiler_tir/src/lib.rsRepository: BoundaryML/baml
Length of output: 568
type_definition_location drops namespace — can produce wrong secondary label for types with colliding names
type_definition_location stores ErrorLocation::TypeItem(fqn.name.clone()), discarding the fqn.namespace component. In contrast, function_definition_location (line 813) correctly stores the full QualifiedName via ErrorLocation::FunctionItem(fqn.clone()).
This creates an asymmetry that causes a real problem: when rendering errors, project_type_item_spans uses a HashMap<Name, Span> keyed only by Name. If both a builtin type and a user-defined type share the same local name (e.g., a user-defined class Response {} and a builtin baml.http.Response), the lookup will return whichever one was inserted last or cached, potentially pointing to the wrong definition.
The fix is to make TypeItem namespace-aware, either by:
- Changing
ErrorLocation::TypeItemto acceptQualifiedNameinstead ofName, or - Adding a new variant
ErrorLocation::TypeDefinition(QualifiedName)(analogous toFunctionItem)
Then update project_type_item_spans to use HashMap<QualifiedName, Span> or add a separate rendering path for qualified type lookups.
| /// Test that whitespace-only changes in a calle do not invalidate | ||
| /// type inference in a caller when there is an arity error. | ||
| /// | ||
| /// This guards against a regression where span changes inside the callee | ||
| /// would progate into the caller's cached InferenceResult. | ||
| #[test] | ||
| fn callee_whitespace_change_does_not_invalidate_caller_inference_on_arity_error() { | ||
| let mut test_db = IncrementalTestDb::new(); | ||
|
|
||
| // Callee: a function with one parameter. | ||
| let callee_file = test_db.db_mut().add_file( | ||
| "callee.baml", | ||
| r##" | ||
| function Add(x: int, y: int) -> int { | ||
| client GPT4 | ||
| prompt #"Add {{x}} and {{y}}"# | ||
| } | ||
| "##, | ||
| ); | ||
|
|
||
| // Caller: calls Add with the wrong number of arguments → ArgumentCountMismatch. | ||
| // The error's `definition_location` must not embed callee file spans. | ||
| let caller_file = test_db.db_mut().add_file( | ||
| "caller.baml", | ||
| r##" | ||
| function UseAdd(n: int) -> int { | ||
| client GPT4 | ||
| prompt #"Use {{n}}"# | ||
| } | ||
| "##, | ||
| ); | ||
|
|
||
| // First run — both functions get type-checked. | ||
| test_db.assert_executed( | ||
| |db| { | ||
| query_all_type_inference(db, callee_file); | ||
| query_all_type_inference(db, caller_file); | ||
| }, | ||
| &[("function_type_inference", 2)], | ||
| ); | ||
|
|
||
| // Whitespace-only change to the callee file (blank line before the function). | ||
| // This shifts every span inside callee.baml but changes no semantics. | ||
| callee_file.set_text(test_db.db_mut()).to(r##" | ||
|
|
||
| function Add(x: int, y: int) -> int { | ||
| client GPT4 | ||
| prompt #"Add {{x}} and {{y}}"# | ||
| } | ||
| "## | ||
| .to_string()); | ||
|
|
||
| // After a whitespace-only change to callee: | ||
| // - lex/parse re-run for the callee | ||
| // - both callee and caller type inference remain cached | ||
| test_db.assert_executed( | ||
| |db| { | ||
| query_all_type_inference(db, callee_file); | ||
| query_all_type_inference(db, caller_file); | ||
| }, | ||
| &[ | ||
| ("lex_file", 1), // callee re-lexed | ||
| ("parse_result", 1), // callee re-parsed | ||
| ("function_type_inference", 0), // caller inference cached — the regression guard | ||
| ], | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Run the Rust test suite for library changes.
Please run cargo test --lib for these Rust changes.
As per coding guidelines: Always run cargo test --lib if you changed any Rust code.
| // Caller: calls Add with the wrong number of arguments → ArgumentCountMismatch. | ||
| // The error's `definition_location` must not embed callee file spans. | ||
| let caller_file = test_db.db_mut().add_file( | ||
| "caller.baml", | ||
| r##" | ||
| function UseAdd(n: int) -> int { | ||
| client GPT4 | ||
| prompt #"Use {{n}}"# | ||
| } | ||
| "##, | ||
| ); |
There was a problem hiding this comment.
Test does not trigger the arity mismatch it claims.
The caller fixture never calls Add, so no ArgumentCountMismatch is produced; this test won’t guard the intended regression. Consider making the caller an expression function that calls Add with the wrong arity.
💡 Suggested fix
- let caller_file = test_db.db_mut().add_file(
- "caller.baml",
- r##"
-function UseAdd(n: int) -> int {
- client GPT4
- prompt #"Use {{n}}"#
-}
-"##,
- );
+ let caller_file = test_db.db_mut().add_file(
+ "caller.baml",
+ r##"
+function UseAdd(n: int) -> int {
+ Add(n, n, n)
+}
+"##,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Caller: calls Add with the wrong number of arguments → ArgumentCountMismatch. | |
| // The error's `definition_location` must not embed callee file spans. | |
| let caller_file = test_db.db_mut().add_file( | |
| "caller.baml", | |
| r##" | |
| function UseAdd(n: int) -> int { | |
| client GPT4 | |
| prompt #"Use {{n}}"# | |
| } | |
| "##, | |
| ); | |
| // Caller: calls Add with the wrong number of arguments → ArgumentCountMismatch. | |
| // The error's `definition_location` must not embed callee file spans. | |
| let caller_file = test_db.db_mut().add_file( | |
| "caller.baml", | |
| r##" | |
| function UseAdd(n: int) -> int { | |
| Add(n, n, n) | |
| } | |
| "##, | |
| ); |
miguelcsx
force-pushed
the
refac/typerror
branch
from
bacd138 to
38209c0
Compare
- include missing filed diagnostics
- fix the dead match base inside the watch accessor - extract functions to helpers
- add query to map FQN to salsa span name - wire everything with function spans
- update tests - general code clean up
miguelcsx
force-pushed
the
refac/typerror
branch
from
38209c0 to
0ac2872
Compare
1 participant
Changes
Add optional definition locations to
TypeError::ArgumentCountMismatchandTypeError::NoSuchField.definition_name_span.definition_spans.bamland update snapshots forsimple_type_error.PR Checklist
Additional Notes
Snapshot updates were regenerated for the
simple_type_errorto capture the new secondary spans.Summary by CodeRabbit