Repository navigation
[codex] Fix string splitting and map key diagnostics - #3861
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
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 Unicode-aware string character iteration and enforces string-only map keys through lowering, diagnostics, tests, and updated fixtures. ChangesString iteration
Map key validation
Sequence Diagram(s)sequenceDiagram
participant TypeExprMap as "TypeExpr::Map lowering"
participant TirTypeError as "TirTypeError::InvalidMapKeyType"
participant Check as "tir_type_error_to_diagnostic_id"
participant DiagnosticId as "DiagnosticId::InvalidMapKeyType"
TypeExprMap->>TirTypeError: emit error for unsupported key type
TirTypeError->>Check: propagate InvalidMapKeyType
Check->>DiagnosticId: map to diagnostic id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
aaronvg
marked this pull request as ready for review
June 26, 2026 21:27
aaronvg
marked this pull request as draft
June 26, 2026 21:30
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
aaronvg
marked this pull request as ready for review
June 26, 2026 21:31
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_compiler2_tir/src/infer_context.rs`:
- Around line 484-490: The InvalidMapKeyType message in
infer_context::TirTypeError handling is too narrow because it also applies to
invalid map<K, V> annotations, not just .set()/.get() usage. Update the
user-facing text to lead with declaring the map as map<string, V> when the key
type is wrong, and keep the .to_string() conversion hint as secondary guidance;
use the TirTypeError::InvalidMapKeyType branch and key.render_user_facing() as
the place to adjust the wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb07f59f-98d1-4de1-be89-d673f40dec2f
⛔ Files ignored due to path filters (7)
baml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/attr_disambiguation/baml_tests__diagnostic_errors__attr_disambiguation__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
baml_language/crates/baml_builtins2/baml_std/baml/ns_iter/iter.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_ops/comparison.bamlbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/map_enums_and_literals.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/map_types.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/map_types2.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/secure_types.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/maps/key_and_value_typecheck.bamlbaml_language/crates/baml_tests/tests/string_regressions.rsbaml_language/crates/baml_tests/tests/type_error_repro.rsbaml_language/crates/bex_vm/src/package_baml/string.rsbaml_language/sdk_tests/fixtures/type_shapes/baml_src/ns_maps/types.baml
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 30c7ede. Configure here.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs`:
- Around line 70-101: Treat seen_aliases as a recursion stack in
is_supported_map_key_type_expr: when expanding a TypeExpr::Path alias, keep the
alias inserted only for the duration of the recursive check and remove it before
returning. Update the alias-handling branch in lower_type_expr.rs so a
successful alias expansion does not leave the qtn permanently in seen_aliases,
while still preventing true cycles during recursive descent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c55029d8-0bf9-4c35-ab86-b9b33c6e0eea
📒 Files selected for processing (2)
baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_tests/tests/type_error_repro.rs
aaronvg
enabled auto-merge
June 26, 2026 22:22
# Conflicts: # baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/map_types2.baml # baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/class/secure_types.baml # baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snap # baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snap
# Conflicts: # baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snap # baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snap
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
String.split("")to return codepoint strings without leading/trailing empty entries, including"".split("") == [].String.chars() -> string[]for explicit character access.Iterable for string, sofor ... inover a string iterates codepoint strings via.chars().iter().map<K, V>key annotations during type checking with a clear E0067 diagnostic instead of compiling to a VM.set()/.get()crash.map<string, V>and refresh affected LSP/stdlib/bytecode snapshots.type_shapesmap fixture to use string-keyed maps.origin/canaryinto the PR branch and refresh bytecode snapshots from the merged tree.Why
B-252:
split("")was exposing Rust/Python-style empty-boundary padding even though users need character-like substrings and string iteration.B-533: runtime maps are string-keyed today, but the compiler accepted
map<int, V>, creating a compile-clean/fail-at-runtime trap. The validation now preserves valid aliases such astype Key = stringinstead of treating aliases as invalid by default.Validation
cargo fmtcargo nextest run -p baml_tests --test string_regressions --test type_error_reprocargo nextest run -p baml_tests -p baml_lsp2_actions -p baml_lsp2_actions_tests --no-fail-fastcargo nextest run -p baml_tests diagnostic_errors::attr_disambiguation::test_05_diagnostics bytecode_display_formatsorigin/canary:INSTA_UPDATE=always cargo nextest run -p baml_tests bytecode_display_formatsorigin/canary:cargo nextest run -p baml_tests --test string_regressions --test type_error_repro.chars():INSTA_UPDATE=always cargo nextest run -p baml_tests compiles::__baml_std__::test_03_hir compiles::__baml_std__::test_04_tir compiles::__baml_std__::test_04_5_mir compiles::__baml_std__::test_06_codegen bytecode_display_formats.chars():cargo nextest run -p baml_cli describe_command_tests::render_builtin_package_listingcargo nextest run -p baml_tests --test type_error_repro --test string_regressionscargo nextest run -p baml_lsp2_actions_tests test_maps_key_and_value_typecheckSKIP=no-commit-to-branch prek run --all-files --show-diff-on-failure --hook-stage manualSummary by CodeRabbit
String.chars()for Unicode-aware per-character iteration (string[]).split("")to return correct per-character results for empty and multibyte strings.mapkeys that aren’tstring, including guidance to convert keys via.to_string()before.set()/.get().Note
Medium Risk
Non-string
map<K, V>annotations that previously typechecked will now fail at compile time (intentional breaking change). Map-key validation and alias resolution are new type-check paths worth regression-testing.Overview
Adds
String.chars()and makesstringimplementIterable(viachars().iter()), sofor ... inover strings yields Unicode codepoint strings. StdlibEqualsfor maps is narrowed tomap<string, V>to match the runtime.The compiler now rejects
mapannotations whose key type does not resolve to a string-key shape (with alias expansion), emitting E0067 (InvalidMapKeyType) and LSP wiring instead of compiling maps that fail at.set()/.get(). Test and bytecode/HIR/MIR snapshots are updated for the new builtins and diagnostics.Reviewed by Cursor Bugbot for commit f23c0a0. Bugbot is set up for automated code reviews on this repo. Configure here.