Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/bolt.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,7 @@
## 2026-04-22 - [Avoid string allocation on single-part split]
**Learning:** Calling `.split(delimiter)` on a reference-counted string (`Arc<str>`) and `.map()`ing the results into `Arc::from(s)` unconditionally creates a new allocation for every chunk. If the delimiter doesn't exist, the entire string is re-allocated unnecessarily.
**Action:** When iterating over a split of a reference-counted string, explicitly check if `s.len() == text.len() && !text.is_empty()`. If it is, use `Arc::clone(&text)` to return another reference to the existing string, bypassing the allocation.

## 2026-05-18 - [Optimize string concatenation in interpreter]
**Learning:** Using `format!("{a}{b}")` for string concatenation in the interpreter causes unnecessary allocations and overhead due to the `Display` trait implementation.
**Action:** Implement a `to_string_fast` method on the `Value` enum to return a `std::borrow::Cow<'_, str>` for fast path string conversions. Use `String::with_capacity` and `push_str` with the `to_string_fast` outputs to concatenate strings efficiently.
Comment on lines +65 to +67

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟑 Minor

Entry date appears inconsistent with PR timeline

Line 65 uses 2026-05-18, but this PR was created on 2026-04-28. Please align the note date with the actual change date to keep the learning log chronological.

πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.jules/bolt.md around lines 65 - 67, The changelog entry uses the date
"2026-05-18" which is inconsistent with the PR timeline; update the entry date
to "2026-04-28" (the PR creation date) so the note is chronological;
specifically edit the header line containing the date in the entry that
describes the `to_string_fast` change (the line starting with "## 2026-05-18 -
[Optimize string concatenation in interpreter]") and replace the date with the
correct one.

10 changes: 8 additions & 2 deletions src/interpreter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7564,11 +7564,17 @@ impl Interpreter {
Ok(Value::Text(Arc::from(s)))
}
(Value::Text(a), b) => {
let result = format!("{a}{b}");
let b_str = b.to_string_fast();
let mut result = String::with_capacity(a.len() + b_str.len());
result.push_str(a.as_ref());
Comment on lines 7566 to +7569
result.push_str(b_str.as_ref());
Ok(Value::Text(Arc::from(result.as_str())))
Comment on lines +7568 to 7571
}
(a, Value::Text(b)) => {
let result = format!("{a}{b}");
let a_str = a.to_string_fast();
let mut result = String::with_capacity(a_str.len() + b.len());
result.push_str(a_str.as_ref());
result.push_str(b.as_ref());
Comment on lines +7567 to +7577

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | πŸ”΄ Critical

Semantic regression risk: mixed-type + now depends on non-equivalent stringification.

Line 7567 and Line 7574 switched coercion to to_string_fast(). That changes observable output if to_string_fast() differs from the previous Display path (e.g., Null text rendering), so Text + Null / Null + Text can produce different user-visible strings after this optimization.

Please align Value::to_string_fast() with Display semantics before using it here for mixed-type concatenation.

Suggested fix (root cause in src/interpreter/value.rs)
 pub fn to_string_fast(&self) -> std::borrow::Cow<'_, str> {
     match self {
         Value::Text(s) => std::borrow::Cow::Borrowed(s.as_ref()),
         Value::Number(n) => std::borrow::Cow::Owned(n.to_string()),
         Value::Bool(b) => std::borrow::Cow::Borrowed(if *b { "yes" } else { "no" }),
-        Value::Null => std::borrow::Cow::Borrowed("null"),
+        Value::Null => std::borrow::Cow::Borrowed("nothing"),
         Value::Nothing => std::borrow::Cow::Borrowed("nothing"),
         _ => std::borrow::Cow::Owned(format!("{}", self)),
     }
 }
πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/interpreter/mod.rs` around lines 7567 - 7577, The change uses
Value::to_string_fast() for mixed-type concatenation (in the concat arms
handling Value::Text), which can diverge from the previous Display-based
stringification (e.g., Null rendering) and cause semantic regressions; update
the implementation of Value::to_string_fast() in src/interpreter/value.rs so its
output matches the existing Display/Display impl for every Value variant used in
concatenation (particularly Null), or else revert the concat code to use the
Display-based path insteadβ€”ensure to_string_fast either delegates to the Display
semantics for those variants or normalizes its output to be equivalent, and
add/adjust unit tests for Value::to_string_fast(), Value::Display and the concat
cases (Text + Null / Null + Text) to lock the behavior.

Ok(Value::Text(Arc::from(result.as_str())))
Comment on lines +7575 to 7578
}
(a, b) => Err(RuntimeError::new(
Expand Down
12 changes: 12 additions & 0 deletions src/interpreter/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,18 @@ pub struct ActionSignature {
}

impl Value {
pub fn to_string_fast(&self) -> std::borrow::Cow<'_, str> {
match self {
Value::Text(s) => std::borrow::Cow::Borrowed(s.as_ref()),
Value::Number(n) => std::borrow::Cow::Owned(n.to_string()),
Value::Bool(b) => std::borrow::Cow::Borrowed(if *b { "yes" } else { "no" }),
Value::Null => std::borrow::Cow::Borrowed("null"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

πŸ”΄ to_string_fast returns "null" for Value::Null but Display returns "nothing" β€” breaking backward compatibility

to_string_fast at src/interpreter/value.rs:165 returns "null" for Value::Null, but the Display implementation at src/interpreter/value.rs:376 returns "nothing". The old code in add() used format!("{a}{b}") which invokes Display, so concatenating a Null value with a string would produce e.g. "prefixnothing". The new code calls to_string_fast() instead, so the same operation now produces "prefixnull". This silently changes the observable behavior of WFL programs, violating the backward compatibility rule in AGENTS.md ("Backward Compatibility: Sacred. Never break existing WFL programs.").

Suggested change
Value::Null => std::borrow::Cow::Borrowed("null"),
Value::Null => std::borrow::Cow::Borrowed("nothing"),
Open in Devin Review

Was this helpful? React with πŸ‘ or πŸ‘Ž to provide feedback.

Value::Nothing => std::borrow::Cow::Borrowed("nothing"),
// For complex types, just fallback to format!
_ => std::borrow::Cow::Owned(format!("{}", self)),
}
Comment on lines +160 to +169

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Value::Null fast-path changes observable string output

Line 165 returns "null", but Display for Value::Null returns "nothing" (Line 376). Since concatenation previously relied on Display, this changes runtime behavior for expressions involving null.

Proposed fix
     pub fn to_string_fast(&self) -> std::borrow::Cow<'_, str> {
         match self {
             Value::Text(s) => std::borrow::Cow::Borrowed(s.as_ref()),
             Value::Number(n) => std::borrow::Cow::Owned(n.to_string()),
             Value::Bool(b) => std::borrow::Cow::Borrowed(if *b { "yes" } else { "no" }),
-            Value::Null => std::borrow::Cow::Borrowed("null"),
+            Value::Null => std::borrow::Cow::Borrowed("nothing"),
             Value::Nothing => std::borrow::Cow::Borrowed("nothing"),
             // For complex types, just fallback to format!
             _ => std::borrow::Cow::Owned(format!("{}", self)),
         }
     }
πŸ“ 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.

Suggested change
pub fn to_string_fast(&self) -> std::borrow::Cow<'_, str> {
match self {
Value::Text(s) => std::borrow::Cow::Borrowed(s.as_ref()),
Value::Number(n) => std::borrow::Cow::Owned(n.to_string()),
Value::Bool(b) => std::borrow::Cow::Borrowed(if *b { "yes" } else { "no" }),
Value::Null => std::borrow::Cow::Borrowed("null"),
Value::Nothing => std::borrow::Cow::Borrowed("nothing"),
// For complex types, just fallback to format!
_ => std::borrow::Cow::Owned(format!("{}", self)),
}
pub fn to_string_fast(&self) -> std::borrow::Cow<'_, str> {
match self {
Value::Text(s) => std::borrow::Cow::Borrowed(s.as_ref()),
Value::Number(n) => std::borrow::Cow::Owned(n.to_string()),
Value::Bool(b) => std::borrow::Cow::Borrowed(if *b { "yes" } else { "no" }),
Value::Null => std::borrow::Cow::Borrowed("nothing"),
Value::Nothing => std::borrow::Cow::Borrowed("nothing"),
// For complex types, just fallback to format!
_ => std::borrow::Cow::Owned(format!("{}", self)),
}
}
πŸ€– Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/interpreter/value.rs` around lines 160 - 169, to_string_fast currently
returns "null" for Value::Null which diverges from the Display implementation
(fmt::Display for Value) that yields "nothing"; update to_string_fast (in the
Value enum impl) so the Null variant returns the same string as Display
("nothing") to preserve existing concatenation/format behavior and keep
fast-paths consistent with fmt::Display.

}

pub fn type_name(&self) -> &'static str {
match self {
Value::Number(_) => "Number",
Expand Down
Loading