fix(jinja): string formatting should not raise spurious errors#3263
fix(jinja): string formatting should not raise spurious errors#3263
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds targeted type inference for Python string method calls in Jinja expressions by detecting method calls on string-typed receivers and returning mapped result types for known methods; updates compiler and engine visitors and adds tests and a BAML test file. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 67e4876ecc
ℹ️ 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".
| if let ast::Expr::GetAttr(get_attr) = &expr.expr { | ||
| let parent = tracker_visit_expr(&get_attr.expr, state, types); | ||
| if parent.is_subtype_of(&Type::String) { | ||
| if let Some(ret) = string_method_return_type(get_attr.name) { | ||
| // Visit args for side effects (variable tracking, error checking) |
There was a problem hiding this comment.
This branch now returns a type for any recognized string method after only visiting the argument expressions, which means bad calls like s.startswith(), s.replace(1, 2), or "-".join() no longer produce a diagnostic here even though MiniJinja's pycompat methods reject wrong arity/types at render time. That is a real false-negative regression: templates with invalid string-method calls can now pass static analysis and only fail in production. The same early-return logic was copied into baml_compiler_tir/src/jinja/expr.rs as well.
Useful? React with 👍 / 👎.
| ("endswith", Type::Bool), | ||
| ("isalnum", Type::Bool), | ||
| ("isalpha", Type::Bool), | ||
| ("isascii", Type::Bool), | ||
| ("isdigit", Type::Bool), |
There was a problem hiding this comment.
isspace() in the string-method whitelist
The new whitelist claims to mirror the runtime pycompat string methods, but it omits str.isspace(), which MiniJinja supports. As a result, templates like {% if s.isspace() %} still fall back to the old attribute/call error path instead of being treated as a valid bool-returning string method, so this fix remains incomplete for a supported API. The compiler-side mirror in baml_compiler_tir/src/jinja/expr.rs has the same gap.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0e64d61-b1a5-4c71-9ecf-90722cdb1bef
⛔ Files ignored due to path filters (8)
baml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__01_lexer__string_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__02_parser__string_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__10_formatter__string_methods.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
baml_language/crates/baml_compiler_tir/src/jinja/expr.rsbaml_language/crates/baml_tests/projects/string_methods/string_methods.bamlengine/baml-lib/jinja/src/evaluate_type/expr.rsengine/baml-lib/jinja/src/evaluate_type/test_expr.rs
| ast::Expr::Call(call_expr) => { | ||
| // Intercept string method calls: "str".method(args) or string_var.method(args) | ||
| if let ast::Expr::GetAttr(get_attr) = &call_expr.expr { | ||
| let parent = visit_expr(&get_attr.expr, errors, env); | ||
| if parent.is_subtype_of(&JinjaType::String) { | ||
| if let Some(ret) = string_method_return_type(get_attr.name) { | ||
| // Visit args for side effects (variable tracking, error checking) | ||
| for arg in &call_expr.args { | ||
| match arg { | ||
| ast::CallArg::Pos(e) | ast::CallArg::Kwarg(_, e) => { | ||
| visit_expr(e, errors, env); | ||
| } | ||
| ast::CallArg::PosSplat(e) | ast::CallArg::KwargSplat(e) => { | ||
| visit_expr(e, errors, env); | ||
| } | ||
| } | ||
| } | ||
| return ret; | ||
| } | ||
| } | ||
| } | ||
| handle_call(call_expr, errors, env) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file and the specific code sections mentioned
cat -n baml_language/crates/baml_compiler_tir/src/jinja/expr.rs | sed -n '180,220p'Repository: BoundaryML/baml
Length of output: 2068
🏁 Script executed:
# Also look at the method table at lines 607-629
cat -n baml_language/crates/baml_compiler_tir/src/jinja/expr.rs | sed -n '600,635p'Repository: BoundaryML/baml
Length of output: 1483
🏁 Script executed:
# Find the string_method_return_type function
rg -n "fn string_method_return_type" baml_language/crates/baml_compiler_tir/src/jinja/expr.rs -A 30Repository: BoundaryML/baml
Length of output: 1224
🏁 Script executed:
# Search for any signature validation in the file
rg -n "signature\|arity\|arg.*count\|args.*len" baml_language/crates/baml_compiler_tir/src/jinja/expr.rs -iRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Look for tests related to string method calls
fd -e rs baml_language/crates/baml_compiler_tir/src/jinja/ | xargs rg -l "test.*string.*method\|upper.*arg"Repository: BoundaryML/baml
Length of output: 562
🏁 Script executed:
# Check the handle_call function to see how it validates args
rg -n "fn handle_call" baml_language/crates/baml_compiler_tir/src/jinja/expr.rs -A 50Repository: BoundaryML/baml
Length of output: 1845
🏁 Script executed:
# Search for any tests in the file itself
rg -n "#\[test\|#\[cfg\(test\)" baml_language/crates/baml_compiler_tir/src/jinja/expr.rsRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Find all test files for this module
find baml_language/crates/baml_compiler_tir -name "*test*" -o -name "*_tests.rs"Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check if there are tests at the end of expr.rs file
tail -100 baml_language/crates/baml_compiler_tir/src/jinja/expr.rs | head -100Repository: BoundaryML/baml
Length of output: 3241
🏁 Script executed:
# Look for mod tests in expr.rs
rg -n "mod tests" baml_language/crates/baml_compiler_tir/src/jinja/expr.rsRepository: BoundaryML/baml
Length of output: 75
🏁 Script executed:
# Check what the actual minijinja-contrib API looks like for upper method
# Let's search for any documentation or comments about method signatures
rg -n "upper\|capitalize\|lower" baml_language/crates/baml_compiler_tir/src/jinja/expr.rs -B 2 -A 2Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Search for tests related to string method calls
rg -n "upper\|capitalize\|lower.*\(" baml_language/crates/baml_compiler_tir/src/jinja/expr.rs | grep test -A 5 -B 5Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Look at the full test module to see if there are string method call tests
tail -200 baml_language/crates/baml_compiler_tir/src/jinja/expr.rsRepository: BoundaryML/baml
Length of output: 6082
🏁 Script executed:
# Check minijinja-contrib to understand the actual method signatures
web_search: minijinja-contrib pycompat string methods signatures arityRepository: BoundaryML/baml
Length of output: 110
🌐 Web query:
minijinja-contrib pycompat string methods signatures
💡 Result:
minijinja-contrib is a Rust crate providing utilities for MiniJinja, including the pycompat module which implements Python-like methods on primitives for better Jinja2 compatibility. It is enabled via the "pycompat" feature, which depends on minijinja/builtins. The pycompat module primarily provides a single public function: unknown_method_callback, which serves as a callback for handling unknown methods on objects, enabling dynamic Python-style method dispatch on primitives like strings. No explicit list of string method signatures (e.g., str.startswith, str.format, etc.) is documented in the official docs.rs pages. The module description states it "Implements Python methods for better compatibility," suggesting it adds Python str methods (such as capitalize, title, upper, lower, strip, etc.) to MiniJinja string values via the unknown_method_callback mechanism when the feature is enabled. To use: Add minijinja-contrib to Cargo.toml with the pycompat feature, then call minijinja_contrib::add_to_environment(&mut env); This registers pycompat support, allowing templates to call Python-like methods on strings, e.g. {{ mystring.capitalize }}. Source code is part of the minijinja monorepo at https://github.com/mitsuhiko/minijinja (minijinja-contrib subdirectory), latest version 2.18.0 as of 2026-03-15. For exact implementations and full list of supported string methods, inspect the source code in minijinja-contrib/src/pycompat.rs (not directly surfaced in search snippets).
Citations:
- 1: https://docs.rs/minijinja-contrib/latest/minijinja_contrib/
- 2: https://docs.rs/minijinja-contrib/latest/minijinja_contrib/pycompat/index.html
- 3: https://docs.rs/minijinja-contrib
- 4: https://github.com/mitsuhiko/minijinja
- 5: https://crates.io/crates/minijinja-contrib
- 6: https://docs.rs/minijinja-contrib/latest/minijinja_contrib/all.html
🏁 Script executed:
# Check if there are any tests for string method calls in the test module
rg -n "string_method\|\.upper\|\.lower\|\.capitalize" baml_language/crates/baml_compiler_tir/src/jinja/expr.rsRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Look for the complete test module section that deals with string methods
sed -n '1063,$p' baml_language/crates/baml_compiler_tir/src/jinja/expr.rs | grep -A 300 "mod tests"Repository: BoundaryML/baml
Length of output: 9903
🏁 Script executed:
# Check if there are any tests for string method calls
rg -n "\.upper\|\.lower\|\.capitalize" baml_language/crates/baml_compiler_tir/src/jinja/expr.rsRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Look at minijinja-contrib's actual method signatures to understand arity
# Check if the codebase has Cargo.lock or Cargo.toml listing minijinja-contrib version
fd "Cargo.toml" baml_language/crates/baml_compiler_tir | xargs grep -A 5 "minijinja"Repository: BoundaryML/baml
Length of output: 244
🏁 Script executed:
# Check if there's any signature validation mechanism for methods
rg -n "validate.*method\|method.*arity\|method.*signature" baml_language/crates/baml_compiler_tir/src/jinja/Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Let's look at what methods are defined and check against a reference
# First, list all string methods currently recognized
rg -A 20 "fn string_method_return_type" baml_language/crates/baml_compiler_tir/src/jinja/expr.rsRepository: BoundaryML/baml
Length of output: 814
🏁 Script executed:
# Search for any isspace references
rg -n "isspace" baml_language/crates/baml_compiler_tir/Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check the root Cargo.toml to see minijinja-contrib version
find . -name "Cargo.toml" -path "*/baml_language/*" -exec grep -l "minijinja" {} \; | head -1 | xargs cat | grep -A 3 "minijinja"Repository: BoundaryML/baml
Length of output: 520
🏁 Script executed:
# Let's confirm there are NO tests for string method calls with argument validation
rg -n "upper\|lower\|capitalize" baml_language/crates/baml_compiler_tir/src/jinja/expr.rs | grep -i testRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Let's check the minijinja fork to see what methods are actually supported
# by looking at the pycompat module directly if available
find . -name "pycompat.rs" -o -name "pycompat" -type dRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check if there's any documentation about the minijinja fork
cat ./Cargo.lock 2>/dev/null | grep -A 10 "minijinja-contrib" | head -20Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Verify by checking if Python's str.upper actually has zero arguments
python3 << 'EOF'
# Check Python string method signatures
import inspect
# upper() takes no arguments (only self)
print("str.upper signature:", inspect.signature(str.upper))
# Test it
s = "hello"
try:
result = s.upper(1)
print("ERROR: upper(1) should fail but succeeded:", result)
except TypeError as e:
print("Correct: upper(1) fails with:", e)
EOFRepository: BoundaryML/baml
Length of output: 160
🏁 Script executed:
# Check if there's test coverage for string method calls with args in the test module
tail -500 baml_language/crates/baml_compiler_tir/src/jinja/expr.rs | grep -A 10 "test.*method\|test.*upper\|test.*lower"Repository: BoundaryML/baml
Length of output: 396
Validate method signatures before returning inferred type in the fast path.
The fast path at lines 191-212 returns the inferred return type without checking argument count or parameter types. Unlike Python's str.upper() which takes no arguments, this code will accept s.upper(1). Add arity validation similar to validate_function_call() (see handle_call() at line 816), or populate a signature table for string methods alongside the return type lookup.
Additionally, the method table at lines 611-629 is missing isspace which should return Bool.
| // Intercept string method calls: "str".method(args) or string_var.method(args) | ||
| if let ast::Expr::GetAttr(get_attr) = &expr.expr { | ||
| let parent = tracker_visit_expr(&get_attr.expr, state, types); | ||
| if parent.is_subtype_of(&Type::String) { | ||
| if let Some(ret) = string_method_return_type(get_attr.name) { | ||
| // Visit args for side effects (variable tracking, error checking) | ||
| for arg in &expr.args { | ||
| match arg { | ||
| ast::CallArg::Pos(e) | ast::CallArg::Kwarg(_, e) => { | ||
| tracker_visit_expr(e, state, types); | ||
| } | ||
| ast::CallArg::PosSplat(e) | ast::CallArg::KwargSplat(e) => { | ||
| tracker_visit_expr(e, state, types); | ||
| } | ||
| } | ||
| } | ||
| return ret; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd engine && wc -l baml-lib/jinja/src/evaluate_type/expr.rsRepository: BoundaryML/baml
Length of output: 103
🏁 Script executed:
cd engine && sed -n '600,650p' baml-lib/jinja/src/evaluate_type/expr.rsRepository: BoundaryML/baml
Length of output: 2312
🏁 Script executed:
cd engine && sed -n '860,920p' baml-lib/jinja/src/evaluate_type/expr.rsRepository: BoundaryML/baml
Length of output: 2033
🏁 Script executed:
cd engine && grep -n "fn string_method_return_type" baml-lib/jinja/src/evaluate_type/expr.rsRepository: BoundaryML/baml
Length of output: 123
🏁 Script executed:
cd engine && rg "isspace" baml-lib/jinja/src/Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
cd engine && rg -i "isspace" --type rustRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
cd engine && grep -n "isspace\|isspace" baml-lib/jinja/src/ -rRepository: BoundaryML/baml
Length of output: 41
🌐 Web query:
minijinja pycompat string methods isspace
💡 Result:
In MiniJinja, the pycompat module from the minijinja-contrib crate provides Python-like string methods via the unknown_method_callback function for better Jinja2 compatibility. This callback implements various str methods such as upper, lower, strip, startswith, split, replace, isupper, isnumeric, islower, isdigit, isascii, isalpha, isalnum, but does not include isspace. To use pycompat, enable the "pycompat" feature in minijinja-contrib, add it to the environment with add_to_environment or set_unknown_method_callback(unknown_method_callback), allowing calls like {{ " hello"|isupper }} in templates. However, {{ " "|isspace }} is not supported out-of-the-box via pycompat.
Citations:
- 1: https://docs.rs/minijinja-contrib/latest/minijinja_contrib/pycompat/index.html
- 2: https://docs.rs/minijinja-contrib/latest/minijinja_contrib/
- 3: https://docs.rs/minijinja-contrib/latest/minijinja_contrib/pycompat/fn.unknown_method_callback.html
🏁 Script executed:
cd engine && rg "upper\(" baml-lib/jinja/src/evaluate_type/ -A 2 -B 2Repository: BoundaryML/baml
Length of output: 965
🏁 Script executed:
cd engine && find . -name "*test*" -type f | grep -E "(expr|string)" | head -20Repository: BoundaryML/baml
Length of output: 133
🏁 Script executed:
cd engine && sed -n '877,910p' baml-lib/jinja/src/evaluate_type/expr.rsRepository: BoundaryML/baml
Length of output: 1211
🏁 Script executed:
cd engine && cat -n baml-lib/jinja/src/evaluate_type/test_expr.rs | grep -A 10 -B 5 "upper"Repository: BoundaryML/baml
Length of output: 1642
🏁 Script executed:
cd engine && cat -n baml-lib/jinja/src/evaluate_type/test_expr.rs | grep -A 15 "test_string_method_on_variable"Repository: BoundaryML/baml
Length of output: 625
🏁 Script executed:
cd engine && rg "upper\(1\)|upper\(" baml-lib/jinja/src/evaluate_type/test_expr.rsRepository: BoundaryML/baml
Length of output: 225
🏁 Script executed:
cd engine && cat -n baml-lib/jinja/src/evaluate_type/test_expr.rs | sed -n '440,470p'Repository: BoundaryML/baml
Length of output: 1089
🏁 Script executed:
cd engine && rg "assert_fails_to.*upper" baml-lib/jinja/src/evaluate_type/test_expr.rsRepository: BoundaryML/baml
Length of output: 41
Validate string-method arguments before returning early.
The fast path at lines 611–629 visits argument expressions for side effects, then immediately returns the mapped type without checking arity or argument types. This allows invalid calls like s.upper(1) to type-check as string instead of producing a diagnostic. These methods need signature validation (arity and argument types) before the early return. The method table correctly reflects minijinja-contrib's supported string methods, which notably does not include isspace.
sxlijin
force-pushed
the
push-topnutlstrmq
branch
from
67e4876 to
08cc665
Compare
Binary size checks passed✅ 7 passed
Generated by |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f979c9d1-9c82-41f5-aae8-a126029e9b4c
⛔ Files ignored due to path filters (8)
baml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__01_lexer__string_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__02_parser__string_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__10_formatter__string_methods.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
baml_language/crates/baml_compiler_tir/src/jinja/expr.rsbaml_language/crates/baml_tests/projects/string_methods/string_methods.bamlengine/baml-lib/jinja/src/evaluate_type/expr.rsengine/baml-lib/jinja/src/evaluate_type/test_expr.rs
| fn string_method_return_type(method: &str) -> Option<Type> { | ||
| let methods: IndexMap<&str, Type> = IndexMap::from([ | ||
| // Methods returning String | ||
| ("capitalize", Type::String), | ||
| ("format", Type::String), | ||
| ("join", Type::String), | ||
| ("lower", Type::String), | ||
| ("lstrip", Type::String), | ||
| ("replace", Type::String), | ||
| ("rstrip", Type::String), | ||
| ("strip", Type::String), | ||
| ("title", Type::String), | ||
| ("upper", Type::String), | ||
| // Methods returning Bool | ||
| ("endswith", Type::Bool), | ||
| ("isalnum", Type::Bool), | ||
| ("isalpha", Type::Bool), | ||
| ("isascii", Type::Bool), | ||
| ("isdigit", Type::Bool), | ||
| ("islower", Type::Bool), | ||
| ("isnumeric", Type::Bool), | ||
| ("isupper", Type::Bool), | ||
| ("startswith", Type::Bool), | ||
| // Methods returning Int | ||
| ("count", Type::Int), | ||
| ("find", Type::Int), | ||
| ("rfind", Type::Int), | ||
| // Methods returning List[String] | ||
| ("split", Type::List(Box::new(Type::String))), | ||
| ("splitlines", Type::List(Box::new(Type::String))), | ||
| ]); | ||
| methods.get(method).cloned() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid per-call IndexMap allocation.
Creating a new IndexMap on every invocation is inefficient. Consider using a match expression (like the TIR counterpart does) or a static lazy-initialized map.
♻️ Proposed refactor using match expression
fn string_method_return_type(method: &str) -> Option<Type> {
- let methods: IndexMap<&str, Type> = IndexMap::from([
- // Methods returning String
- ("capitalize", Type::String),
- ("format", Type::String),
- ("join", Type::String),
- ("lower", Type::String),
- ("lstrip", Type::String),
- ("replace", Type::String),
- ("rstrip", Type::String),
- ("strip", Type::String),
- ("title", Type::String),
- ("upper", Type::String),
- // Methods returning Bool
- ("endswith", Type::Bool),
- ("isalnum", Type::Bool),
- ("isalpha", Type::Bool),
- ("isascii", Type::Bool),
- ("isdigit", Type::Bool),
- ("islower", Type::Bool),
- ("isnumeric", Type::Bool),
- ("isupper", Type::Bool),
- ("startswith", Type::Bool),
- // Methods returning Int
- ("count", Type::Int),
- ("find", Type::Int),
- ("rfind", Type::Int),
- // Methods returning List[String]
- ("split", Type::List(Box::new(Type::String))),
- ("splitlines", Type::List(Box::new(Type::String))),
- ]);
- methods.get(method).cloned()
+ match method {
+ // Methods returning String
+ "capitalize" | "format" | "join" | "lower" | "lstrip" | "replace" | "rstrip"
+ | "strip" | "title" | "upper" => Some(Type::String),
+ // Methods returning Bool
+ "endswith" | "isalnum" | "isalpha" | "isascii" | "isdigit" | "islower"
+ | "isnumeric" | "isupper" | "startswith" => Some(Type::Bool),
+ // Methods returning Int
+ "count" | "find" | "rfind" => Some(Type::Int),
+ // Methods returning List[String]
+ "split" | "splitlines" => Some(Type::List(Box::new(Type::String))),
+ _ => None,
+ }
}📝 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.
| fn string_method_return_type(method: &str) -> Option<Type> { | |
| let methods: IndexMap<&str, Type> = IndexMap::from([ | |
| // Methods returning String | |
| ("capitalize", Type::String), | |
| ("format", Type::String), | |
| ("join", Type::String), | |
| ("lower", Type::String), | |
| ("lstrip", Type::String), | |
| ("replace", Type::String), | |
| ("rstrip", Type::String), | |
| ("strip", Type::String), | |
| ("title", Type::String), | |
| ("upper", Type::String), | |
| // Methods returning Bool | |
| ("endswith", Type::Bool), | |
| ("isalnum", Type::Bool), | |
| ("isalpha", Type::Bool), | |
| ("isascii", Type::Bool), | |
| ("isdigit", Type::Bool), | |
| ("islower", Type::Bool), | |
| ("isnumeric", Type::Bool), | |
| ("isupper", Type::Bool), | |
| ("startswith", Type::Bool), | |
| // Methods returning Int | |
| ("count", Type::Int), | |
| ("find", Type::Int), | |
| ("rfind", Type::Int), | |
| // Methods returning List[String] | |
| ("split", Type::List(Box::new(Type::String))), | |
| ("splitlines", Type::List(Box::new(Type::String))), | |
| ]); | |
| methods.get(method).cloned() | |
| } | |
| fn string_method_return_type(method: &str) -> Option<Type> { | |
| match method { | |
| // Methods returning String | |
| "capitalize" | "format" | "join" | "lower" | "lstrip" | "replace" | "rstrip" | |
| | "strip" | "title" | "upper" => Some(Type::String), | |
| // Methods returning Bool | |
| "endswith" | "isalnum" | "isalpha" | "isascii" | "isdigit" | "islower" | |
| | "isnumeric" | "isupper" | "startswith" => Some(Type::Bool), | |
| // Methods returning Int | |
| "count" | "find" | "rfind" => Some(Type::Int), | |
| // Methods returning List[String] | |
| "split" | "splitlines" => Some(Type::List(Box::new(Type::String))), | |
| _ => None, | |
| } | |
| } |
sxlijin
force-pushed
the
push-topnutlstrmq
branch
from
08cc665 to
bdfc992
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
engine/baml-lib/jinja/src/evaluate_type/expr.rs (1)
873-909: 🧹 Nitpick | 🔵 TrivialAvoid per-call
IndexMapallocation.Creating a new
IndexMapon every invocation is inefficient. The TIR counterpart (baml_language/crates/baml_compiler_tir/src/jinja/expr.rs) already uses amatchexpression for this lookup. Consider aligning this implementation for consistency and better performance.♻️ Proposed refactor using match expression
fn string_method_return_type(method: &str) -> Option<Type> { - let methods: IndexMap<&str, Type> = IndexMap::from([ - // Methods returning String - ("capitalize", Type::String), - ("format", Type::String), - ("join", Type::String), - ("lower", Type::String), - ("lstrip", Type::String), - ("replace", Type::String), - ("rstrip", Type::String), - ("strip", Type::String), - ("title", Type::String), - ("upper", Type::String), - // Methods returning Bool - ("endswith", Type::Bool), - ("isalnum", Type::Bool), - ("isalpha", Type::Bool), - ("isascii", Type::Bool), - ("isdigit", Type::Bool), - ("islower", Type::Bool), - ("isnumeric", Type::Bool), - ("isupper", Type::Bool), - ("startswith", Type::Bool), - // Methods returning Int - ("count", Type::Int), - ("find", Type::Int), - ("rfind", Type::Int), - // Methods returning List[String] - ("split", Type::List(Box::new(Type::String))), - ("splitlines", Type::List(Box::new(Type::String))), - ]); - methods.get(method).cloned() + match method { + // Methods returning String + "capitalize" | "format" | "join" | "lower" | "lstrip" | "replace" | "rstrip" + | "strip" | "title" | "upper" => Some(Type::String), + // Methods returning Bool + "endswith" | "isalnum" | "isalpha" | "isascii" | "isdigit" | "islower" + | "isnumeric" | "isupper" | "startswith" => Some(Type::Bool), + // Methods returning Int + "count" | "find" | "rfind" => Some(Type::Int), + // Methods returning List[String] + "split" | "splitlines" => Some(Type::List(Box::new(Type::String))), + _ => None, + } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fb067a8-d5ca-477c-875c-c23eb4508a4d
⛔ Files ignored due to path filters (8)
baml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__01_lexer__string_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__02_parser__string_methods.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/string_methods/baml_tests__string_methods__10_formatter__string_methods.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
baml_language/crates/baml_compiler_tir/src/jinja/expr.rsbaml_language/crates/baml_tests/projects/string_methods/string_methods.bamlengine/baml-lib/jinja/src/evaluate_type/expr.rsengine/baml-lib/jinja/src/evaluate_type/test_expr.rs
sxlijin
force-pushed
the
push-topnutlstrmq
branch
from
bdfc992 to
d7544b7
Compare
sxlijin
dismissed
coderabbitai[bot]’s stale review
this is a known and acceptable gap, that we don't check arity on these function calls
1 participant
original discord report
str.format()comes from minijinja's pycompat, which we've added support for in the typechecker now; the list of string instance methods we support comes from the pycompat commentsWe do not check arity or arg types on these functions; this is acceptable because this is a tangible improvement on the current type checking logic and there will still be runtime errors on these
Summary by CodeRabbit
Bug Fixes
Tests