From b2954bbd5ff644c73845344949209d16ce29ff00 Mon Sep 17 00:00:00 2001 From: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Date: Wed, 16 Sep 2026 18:03:38 +0000 Subject: [PATCH 1/4] refactor(config): migrate template variables through MiniJinja's own parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The retired-variable rewrite found its identifiers with a hand-rolled scanner that re-derived MiniJinja's delimiters, whitespace control, string quoting, `{% raw %}` handling and assignment-target grammar. Every place the two readings disagreed was a bug in the file worktrunk writes (#4117), and closing each one grew the scanner further. Enable MiniJinja's `unstable_machinery` and parse instead. `TemplateVars` walks the AST once: the reads are the `Expr::Var` nodes, which is by construction the set the renderer resolves against the context, and the bindings are the statements' own target expressions. Rewriting splices at the byte ranges the parser gave those nodes. That deletes the tag walker, the target-list reader, the quote and identifier scanners and the `{% raw %}` state — seventeen functions for nine short visitor methods — and it collapses detection and migration into a single call rather than two functions sharing a predicate. Behavior is unchanged except where the scanner was wrong: a tag opened inside a `{% raw %}` block no longer leaves the whole template unmigrated, since the parser reads that content as the literal it is. `unstable_machinery` carries no semver guarantee, so the coupling is to the AST's shape alone: the `Stmt` and `Expr` matches are exhaustive, so a MiniJinja that moves a node fails the build rather than quietly mis-migrating a config. It pulls in `internal_debug`, which costs ~116KB of release binary (~67KB stripped). --- Cargo.toml | 12 +- src/config/deprecation.rs | 700 +++++++++++++++++--------------------- 2 files changed, 331 insertions(+), 381 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 67a486f463..076e88e42b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -195,8 +195,16 @@ tracing-log = { version = "0.2.0", optional = true } # `EnvFilter::builder().with_default_directive(...).from_env_lossy()` (used in # `src/logging.rs`) stabilized on top of the per-layer filtering rework. tracing-subscriber = { version = "0.3.17", default-features = false, features = ["ansi", "env-filter", "fmt", "registry"], optional = true } -# Only enable needed features - saves ~200KB by excluding debug, macros, multi_template -minijinja = { version = "2.20", default-features = false, features = ["builtins", "serde", "std_collections"] } +# Only enable needed features - saves ~200KB by excluding debug, macros, multi_template. +# `unstable_machinery` exposes the parser `src/config/deprecation.rs` migrates retired +# template variables through, rather than re-deriving MiniJinja's grammar by hand; it +# pulls in `internal_debug`, which costs ~116KB of that saving back (~67KB stripped). +minijinja = { version = "2.20", default-features = false, features = [ + "builtins", + "serde", + "std_collections", + "unstable_machinery", +] } rayon = "1.12" serde = { version = "1.0.228", features = ["derive"] } serde_json = "1.0.150" diff --git a/src/config/deprecation.rs b/src/config/deprecation.rs index 4b086d6c9f..98183b4f24 100644 --- a/src/config/deprecation.rs +++ b/src/config/deprecation.rs @@ -27,12 +27,13 @@ use std::borrow::Cow; use std::collections::HashSet; use std::io::Write; +use std::ops::Range; use std::path::{Path, PathBuf}; use std::sync::{LazyLock, Mutex, OnceLock}; use anyhow::Context; use color_print::cformat; -use minijinja::Environment; +use minijinja::machinery::{ast, parse as parse_template}; use shell_escape::unix::escape; use crate::config::WorktrunkConfig; @@ -172,413 +173,298 @@ pub const DEPRECATED_SECTION_KEYS: &[DeprecatedSection] = &[ /// /// Returns `Cow::Borrowed` if no replacements needed, avoiding allocation. pub fn normalize_template_vars(template: &str) -> Cow<'_, str> { - let replacements = deprecated_vars_in_template(template); - if replacements.is_empty() { - return Cow::Borrowed(template); + match migrate_retired_vars(template) { + Some((migrated, _)) => Cow::Owned(migrated), + None => Cow::Borrowed(template), } - - rewrite_template_var_identifiers(template, &replacements) - .map(Cow::Owned) - .unwrap_or(Cow::Borrowed(template)) } -/// The retired `(old, new)` pairs it is safe to rewrite in `template`, in -/// [`RETIRED_VARS`] order. Empty when none appear, when the template doesn't -/// parse, or when a block binds either half of a pair — see -/// [`template_bound_names`] for why a bound name drops the pair instead of -/// renaming half a scope. An identifier appearing only as an attribute name -/// (`{{ foo.repo_root }}`) or an assignment target isn't a use, so it doesn't -/// bring a pair in on its own. +/// Rewrite every retired template variable in `template`, returning the new +/// text alongside the `(old, new)` pairs it replaced, in [`RETIRED_VARS`] +/// order. +/// +/// `None` leaves the template exactly as written: no retired name is read, it +/// doesn't parse, or a statement binds either half of a pair — see +/// [`TemplateVars`] for why a bound name drops the pair rather than renaming +/// half a scope. An identifier that isn't a variable read — an attribute +/// (`{{ foo.repo_root }}`), a keyword argument, an assignment target — isn't a +/// use, so it doesn't bring a pair in on its own. /// -/// Detection and migration share this one predicate: the rule rewrites what -/// this reports, so the two cannot drift. -fn deprecated_vars_in_template(template: &str) -> Vec<(&'static str, &'static str)> { +/// Detection and migration are this one call: the rule warns about the pairs +/// the same invocation rewrites, so the two cannot drift. +fn migrate_retired_vars(template: &str) -> Option<(String, Vec<(&'static str, &'static str)>)> { // Quick check: if none of the retired vars appear, skip parsing if !RETIRED_VARS.iter().any(|(old, _)| template.contains(old)) { - return Vec::new(); + return None; } - let env = Environment::new(); - let Ok(parsed) = env.template_from_str(template) else { - return Vec::new(); - }; - let used_vars = parsed.undeclared_variables(false); - let bound = template_bound_names(template); - RETIRED_VARS + let vars = TemplateVars::of(template)?; + let replacements = RETIRED_VARS .iter() .copied() .filter(|(old, new)| { - used_vars.contains(*old) && !bound.contains(old) && !bound.contains(new) + vars.reads.iter().any(|(name, _)| name == old) + && !vars.bound.contains(old) + && !vars.bound.contains(new) }) - .collect() -} + .collect::>(); + if replacements.is_empty() { + return None; + } -/// Every name a `{% … %}` block binds anywhere in `template`. -/// -/// `undeclared_variables` answers whether a name is read before it is bound, -/// not which of its occurrences are the global one, so -/// `{{ repo_root }}{% set repo_root = "local" %}{{ repo_root }}` reports -/// `repo_root` even though the last use reads the local. The rewriter has no -/// scope tracking and would rename both. A deprecated name that is bound -/// anywhere is therefore dropped from the replacement set — and because -/// detection reads that same set, such a template neither migrates nor -/// warns. Losing the warning is the price of not quietly rendering -/// something else. -/// -/// Since [`RETIRED_VARS`] became a [`DeprecationRule::Structural`] row, that -/// price is paid at render time rather than deferred: the retired name -/// survives the load-path rewrite and reaches a renderer that has nothing to -/// resolve it to, failing a `SemiStrict` expansion loudly or rendering empty -/// in a squash template. Both beat renaming one scope's worth of uses out -/// from under the template that bound the name. -/// -/// The *canonical* name is checked against this set too. Binding it captures -/// the global use the rename produces: `{% for repo_path in items %}` around a -/// `{{ repo_root }}` reads the global today and the loop variable once -/// renamed. The same collision reaches every pair — `{% for commit_details in -/// … %}{{ commits }}` is the squash-template shape of it. -fn template_bound_names(template: &str) -> HashSet<&str> { - let mut bound = HashSet::new(); + let mut edits = vars + .reads + .into_iter() + .filter_map(|(name, at)| { + let (_, new) = replacements.iter().find(|(old, _)| *old == name)?; + Some((at, *new)) + }) + .collect::>(); + // Reads are collected in visit order, which is not source order — a + // conditional expression is visited test-first (`{{ a if b }}` yields `b` + // before `a`), and a map's keys all precede its values. + edits.sort_by_key(|(at, _)| at.start); + + let mut migrated = String::with_capacity(template.len()); let mut cursor = 0; - let mut in_raw = false; - while let Some((tag_start, kind)) = find_next_template_tag(template, cursor) { - let body_start = tag_start + 2; - let close_delim = match kind { - TemplateTagKind::Variable => "}}", - TemplateTagKind::Block => "%}", - TemplateTagKind::Comment => "#}", - }; - // A comment runs to its first `#}` — MiniJinja doesn't tokenize - // strings inside one — so it takes the plain scan the rewriter uses. - let tag_end = match kind { - TemplateTagKind::Comment => template[body_start..] - .find(close_delim) - .map(|rel| body_start + rel), - _ => template_tag_end(template, body_start, close_delim), - }; - // A tag this scanner can't terminate is one it must not guess at: the - // rewrite bails on the same tag, so reporting no further bindings - // changes nothing. - let Some(tag_end) = tag_end else { break }; - if kind == TemplateTagKind::Block { - let body = &template[body_start..tag_end]; - match template_block_name(body) { - // `{% raw %}` content is literal text to MiniJinja, so a tag - // inside one binds nothing — the same state the rewriter keeps. - Some("raw") => in_raw = true, - Some("endraw") => in_raw = false, - _ if !in_raw => bound.extend(template_block_bindings(body)), - _ => {} - } - } - cursor = tag_end + close_delim.len(); + for (at, new) in edits { + migrated.push_str(&template[cursor..at.start]); + migrated.push_str(new); + cursor = at.end; } - bound + migrated.push_str(&template[cursor..]); + Some((migrated, replacements)) } -/// The names one block tag binds in the surrounding scope. +/// A template as MiniJinja's own parser reads it: every variable read, with +/// the byte range of the identifier behind it, and every name a statement +/// binds. /// -/// `set`, `for`, and `with` are the binding statements this MiniJinja build -/// has: `Cargo.toml` takes it with `default-features = false`, so `macro` and -/// `call` are not statements at all and a template using one fails to parse -/// before reaching here. Adding the `macros` feature would mean adding their -/// parameter lists to this match. +/// Both halves were once scanned by hand, which meant re-deriving MiniJinja's +/// delimiters, whitespace control, string quoting, `{% raw %}` handling and +/// assignment-target grammar. Each place the two readings disagreed was +/// visible in the file worktrunk writes (#4117): a `}}` inside a string ended +/// a tag early, so the reference after it never migrated, and an unrecognized +/// target list renamed a local's uses out from under its binding. Parsing +/// through [`minijinja::machinery`] removes the second reading instead of +/// correcting it — the reads are the `Expr::Var` nodes, which is by +/// construction the set the renderer resolves against the context, and the +/// bindings are the statements' own target expressions. /// -/// Only the binding keywords are inspected — `{% if repo_root %}` reads a name -/// rather than binding it. Over-reporting is the safer direction: a name this -/// returns is left unmigrated and stops warning, while one it misses would be -/// renamed at a use that no longer refers to the global. -fn template_block_bindings(body: &str) -> Vec<&str> { - let (keyword, rest) = split_block_keyword(body); - match keyword { - // `{% set a, (b, c) = expr %}`, `{% set a %}…{% endset %}` - "set" => binding_targets(rest, TargetsEndAt::Assign), - // `{% with a = 1, b = 2 %}` - "with" => binding_targets(rest, TargetsEndAt::AssignPairs), - "for" => binding_targets(rest, TargetsEndAt::In), - _ => Vec::new(), - } +/// `machinery` carries no semver guarantee, so the coupling is to the AST's +/// shape alone: the matches below are exhaustive over `Stmt` and `Expr`, and +/// `Cargo.toml` takes MiniJinja with `default-features = false`, so the +/// `macros`, `multi_template` and `loop_controls` variants don't exist to +/// handle. A MiniJinja that adds or moves a node fails this build rather than +/// quietly mis-migrating a config, and the compile error names what to +/// handle. +struct TemplateVars<'a> { + /// The name and byte range of every `Expr::Var`, in visit order. + reads: Vec<(&'a str, Range)>, + /// Every name a `set`, `for`, or `with` binds, at any depth. + /// + /// The rewrite has no notion of scope — it replaces reads wherever they + /// sit — so a deprecated name bound anywhere is dropped from the + /// replacement set entirely rather than renamed per scope: + /// `{{ repo_root }}{% set repo_root = "local" %}{{ repo_root }}` must not + /// have its last use renamed away from the binding it reads. Detection + /// reads that same set, so such a template neither migrates nor warns. + /// Losing the warning is the price of not quietly rendering something + /// else. + /// + /// Since [`RETIRED_VARS`] became a [`DeprecationRule::Structural`] row + /// that price is paid at render time rather than deferred: the retired + /// name survives the load-path rewrite and reaches a renderer that has + /// nothing to resolve it to, failing a `SemiStrict` expansion loudly or + /// rendering empty in a squash template. Both beat renaming one scope's + /// worth of uses out from under the template that bound the name. + /// + /// The *canonical* name is checked against this set too. Binding it + /// captures the global use the rename produces: `{% for repo_path in items + /// %}` around a `{{ repo_root }}` reads the global today and the loop + /// variable once renamed. The same collision reaches every pair — `{% for + /// commit_details in … %}{{ commits }}` is the squash-template shape of it. + bound: HashSet<&'a str>, } -/// What separates a binding tag's targets from its values. -#[derive(Clone, Copy, PartialEq, Eq)] -enum TargetsEndAt { - /// `{% for a, (b, c) in expr %}` — the `in` keyword, once. - In, - /// `{% set a, b = expr %}` — the first top-level `=`. MiniJinja's `set` - /// takes one assignment, so a top-level comma after it builds a tuple - /// *value* and starts no further target. - Assign, - /// `{% with a = 1, b = 2 %}` — each `=`, with a top-level comma starting - /// the next pair's target. - AssignPairs, -} +impl<'a> TemplateVars<'a> { + /// `None` when MiniJinja can't parse `template` — the templates its + /// renderer rejects too, left untouched rather than guessed at. + fn of(template: &'a str) -> Option { + // The syntax and whitespace defaults, which are what the environments + // that render these templates use. Spelled `Default::default()` + // because `SyntaxConfig` is a unit struct without MiniJinja's + // `custom_syntax` feature. + let ast = + parse_template(template, "", Default::default(), Default::default()).ok()?; + let mut vars = TemplateVars { + reads: Vec::new(), + bound: HashSet::new(), + }; + vars.stmt(&ast); + Some(vars) + } -/// The identifiers a binding tag's target list binds. -/// -/// MiniJinja's `parse_assignment` accepts arbitrarily nested parenthesized -/// tuples — `{% for (a, (b, c)) in … %}` — so the targets are gathered by -/// scanning the target region for identifiers rather than by splitting it on -/// commas, which read `(repo_root` as "not a bare identifier" and bound -/// nothing. A shape this doesn't model contributes extra names, leaving a -/// template unmigrated; a missed one gets renamed at a use that no longer -/// refers to the global, so the scan errs toward collecting. -/// -/// A dotted target binds nothing — `{% set repo_root.x = … %}` mutates an -/// attribute of the global, so neither half of the path is a binding. -fn binding_targets(rest: &str, targets_end_at: TargetsEndAt) -> Vec<&str> { - let mut names = Vec::new(); - let mut depth = 0usize; - let mut in_targets = true; - let mut cursor = 0; - while let Some(ch) = rest.get(cursor..).and_then(|s| s.chars().next()) { - if ch == '"' || ch == '\'' { - cursor = quoted_template_string_end(rest, cursor, ch); - continue; - } - if is_template_identifier_start(ch) { - let end = identifier_end(rest, cursor); - let name = &rest[cursor..end]; - if targets_end_at == TargetsEndAt::In && depth == 0 && name == "in" { - in_targets = false; - } else if in_targets - && !rest[..cursor].trim_end().ends_with('.') - && !rest[end..].trim_start().starts_with('.') - { - names.push(name); - } - cursor = end; - continue; + fn body(&mut self, body: &[ast::Stmt<'a>]) { + for stmt in body { + self.stmt(stmt); } - match ch { - '(' | '[' | '{' => depth += 1, - ')' | ']' | '}' => depth = depth.saturating_sub(1), - '=' if targets_end_at != TargetsEndAt::In - && depth == 0 - && !rest[cursor..].starts_with("==") => - { - in_targets = false; - } - ',' if targets_end_at == TargetsEndAt::AssignPairs && depth == 0 => in_targets = true, - // A `set` block's filter chain is value, not target: - // `{% set x | default(repo_root) %}…{% endset %}` binds only `x`. - '|' if depth == 0 => in_targets = false, - _ => {} - } - cursor += ch.len_utf8(); } - names -} -fn rewrite_template_var_identifiers( - template: &str, - replacements: &[(&str, &'static str)], -) -> Option { - let mut out = String::with_capacity(template.len()); - let mut cursor = 0; - let mut changed = false; - let mut in_raw = false; - - while let Some((tag_start, tag_kind)) = find_next_template_tag(template, cursor) { - out.push_str(&template[cursor..tag_start]); - - let (body_start, close_delim) = match tag_kind { - TemplateTagKind::Variable => (tag_start + 2, "}}"), - TemplateTagKind::Block => (tag_start + 2, "%}"), - TemplateTagKind::Comment => { - let end = template[tag_start + 2..].find("#}")? + tag_start + 4; - out.push_str(&template[tag_start..end]); - cursor = end; - continue; + fn stmt(&mut self, stmt: &ast::Stmt<'a>) { + match stmt { + ast::Stmt::Template(node) => self.body(&node.children), + ast::Stmt::EmitExpr(node) => self.expr(&node.expr), + // Literal output — the text around the tags, and everything inside + // a `{% raw %}` block. + ast::Stmt::EmitRaw(_) => {} + ast::Stmt::ForLoop(node) => { + self.target(&node.target); + self.expr(&node.iter); + if let Some(filter) = &node.filter_expr { + self.expr(filter); + } + self.body(&node.body); + self.body(&node.else_body); } - }; - let tag_end = template_tag_end(template, body_start, close_delim)?; - let full_tag_end = tag_end + close_delim.len(); - - if tag_kind == TemplateTagKind::Block - && matches!( - template_block_name(&template[body_start..tag_end]), - Some("raw") - ) - { - in_raw = true; - } - - if in_raw { - out.push_str(&template[tag_start..full_tag_end]); - if tag_kind == TemplateTagKind::Block - && matches!( - template_block_name(&template[body_start..tag_end]), - Some("endraw") - ) - { - in_raw = false; + ast::Stmt::IfCond(node) => { + self.expr(&node.expr); + self.body(&node.true_body); + self.body(&node.false_body); } - } else { - let body_start = - body_start + usize::from(template[body_start..tag_end].starts_with('-')); - let body_end = tag_end - usize::from(template[body_start..tag_end].ends_with('-')); - let (rewritten_body, body_changed) = - rewrite_template_tag_body(&template[body_start..body_end], replacements); - out.push_str(&template[tag_start..body_start]); - out.push_str(&rewritten_body); - out.push_str(&template[body_end..full_tag_end]); - changed |= body_changed; + ast::Stmt::WithBlock(node) => { + for (target, value) in &node.assignments { + self.target(target); + self.expr(value); + } + self.body(&node.body); + } + ast::Stmt::Set(node) => { + self.target(&node.target); + self.expr(&node.expr); + } + ast::Stmt::SetBlock(node) => { + self.target(&node.target); + if let Some(filter) = &node.filter { + self.expr(filter); + } + self.body(&node.body); + } + ast::Stmt::AutoEscape(node) => { + self.expr(&node.enabled); + self.body(&node.body); + } + ast::Stmt::FilterBlock(node) => { + self.expr(&node.filter); + self.body(&node.body); + } + ast::Stmt::Do(node) => self.call(&node.call), } - - cursor = full_tag_end; } - out.push_str(&template[cursor..]); - changed.then_some(out) -} - -#[derive(Clone, Copy, PartialEq, Eq)] -enum TemplateTagKind { - Variable, - Block, - Comment, -} - -fn find_next_template_tag(template: &str, from: usize) -> Option<(usize, TemplateTagKind)> { - let mut search_from = from; - loop { - let rel = template[search_from..].find('{')?; - let idx = search_from + rel; - let rest = &template[idx..]; - let kind = if rest.starts_with("{{") { - TemplateTagKind::Variable - } else if rest.starts_with("{%") { - TemplateTagKind::Block - } else if rest.starts_with("{#") { - TemplateTagKind::Comment - } else { - search_from = idx + 1; - continue; - }; - return Some((idx, kind)); - } -} - -fn template_block_name(body: &str) -> Option<&str> { - let (keyword, _) = split_block_keyword(body); - (!keyword.is_empty()).then_some(keyword) -} - -/// A block tag's body split into its leading keyword and the rest. -/// -/// MiniJinja spells whitespace control `-` *or* `+` (`Whitespace::from_byte` -/// maps both, one trimming and one preserving), and accepts either on both -/// ends of a tag. Stripping only `-` left `{%+ raw %}` unrecognized — its -/// literal contents were rewritten — and `{%+ set x = … %}` binding nothing, -/// so the name it shadows was renamed at every use. Both readers of a block -/// keyword go through here so that stays one answer. -fn split_block_keyword(body: &str) -> (&str, &str) { - let body = body.strip_prefix(['-', '+']).unwrap_or(body); - let body = body.strip_suffix(['-', '+']).unwrap_or(body).trim(); - let split = body - .find(|c: char| !is_template_identifier_char(c)) - .unwrap_or(body.len()); - body.split_at(split) -} - -fn rewrite_template_tag_body(body: &str, replacements: &[(&str, &'static str)]) -> (String, bool) { - let mut out = String::with_capacity(body.len()); - let mut cursor = 0; - let mut changed = false; - - while let Some(ch) = body.get(cursor..).and_then(|s| s.chars().next()) { - if ch == '"' || ch == '\'' { - let end = quoted_template_string_end(body, cursor, ch); - out.push_str(&body[cursor..end]); - cursor = end; - } else if is_template_identifier_start(ch) { - let end = identifier_end(body, cursor); - let ident = &body[cursor..end]; - if !is_template_attribute_or_assignment(body, cursor, end) - && let Some((_, new)) = replacements.iter().find(|(old, _)| *old == ident) - { - out.push_str(new); - changed = true; - } else { - out.push_str(ident); + fn expr(&mut self, expr: &ast::Expr<'a>) { + match expr { + ast::Expr::Var(node) => { + let span = node.span(); + self.reads.push(( + node.id, + span.start_offset as usize..span.end_offset as usize, + )); + } + ast::Expr::Const(_) => {} + ast::Expr::Slice(node) => { + self.expr(&node.expr); + for bound in [&node.start, &node.stop, &node.step].into_iter().flatten() { + self.expr(bound); + } + } + ast::Expr::UnaryOp(node) => self.expr(&node.expr), + ast::Expr::BinOp(node) => { + self.expr(&node.left); + self.expr(&node.right); + } + ast::Expr::Compare(node) => { + self.expr(&node.expr); + for op in &node.ops { + self.expr(&op.expr); + } + } + ast::Expr::IfExpr(node) => { + self.expr(&node.test_expr); + self.expr(&node.true_expr); + if let Some(false_expr) = &node.false_expr { + self.expr(false_expr); + } + } + // A filter or test names a function the environment supplies, not + // a variable, so only its input and arguments are reads. + ast::Expr::Filter(node) => { + if let Some(expr) = &node.expr { + self.expr(expr); + } + self.args(&node.args); + } + ast::Expr::Test(node) => { + self.expr(&node.expr); + self.args(&node.args); + } + // `{{ foo.repo_root }}` reads `foo`; the attribute belongs to + // whatever that resolves to, never to the deprecated global. + ast::Expr::GetAttr(node) => self.expr(&node.expr), + ast::Expr::GetItem(node) => { + self.expr(&node.expr); + self.expr(&node.subscript_expr); + } + ast::Expr::Call(node) => self.call(node), + ast::Expr::List(node) => { + for item in &node.items { + self.expr(item); + } + } + ast::Expr::Map(node) => { + for entry in node.keys.iter().chain(&node.values) { + self.expr(entry); + } } - cursor = end; - } else { - out.push(ch); - cursor += ch.len_utf8(); } } - (out, changed) -} - -/// The offset of `close_delim` that ends this tag, skipping any occurrence -/// inside a quoted string. -/// -/// MiniJinja tokenizes strings before it looks for the tag end, so a plain -/// `find` disagrees with the parser that decided which variables are in scope. -/// On `{{ "}} " ~ repo_root }}` that disagreement cut the tag at the quoted -/// delimiter, leaving the real reference as outside-tag text (no migration) and -/// exposing literal text in a later string to the rewriter (a changed -/// template). -fn template_tag_end(template: &str, body_start: usize, close_delim: &str) -> Option { - let mut cursor = body_start; - while let Some(ch) = template.get(cursor..).and_then(|s| s.chars().next()) { - if template[cursor..].starts_with(close_delim) { - return Some(cursor); - } else if ch == '"' || ch == '\'' { - cursor = quoted_template_string_end(template, cursor, ch); - } else { - cursor += ch.len_utf8(); - } + fn call(&mut self, call: &ast::Call<'a>) { + self.expr(&call.expr); + self.args(&call.args); } - None -} -fn quoted_template_string_end(body: &str, start: usize, quote: char) -> usize { - let mut escaped = false; - let mut cursor = start + quote.len_utf8(); - while let Some(ch) = body.get(cursor..).and_then(|s| s.chars().next()) { - cursor += ch.len_utf8(); - if escaped { - escaped = false; - } else if ch == '\\' { - escaped = true; - } else if ch == quote { - break; + /// A keyword argument's name belongs to the call it is passed to, so only + /// the argument values are reads. + fn args(&mut self, args: &[ast::CallArg<'a>]) { + for arg in args { + match arg { + ast::CallArg::Pos(expr) + | ast::CallArg::Kwarg(_, expr) + | ast::CallArg::PosSplat(expr) + | ast::CallArg::KwargSplat(expr) => self.expr(expr), + } } } - cursor -} -fn identifier_end(body: &str, start: usize) -> usize { - let mut cursor = start; - while let Some(ch) = body.get(cursor..).and_then(|s| s.chars().next()) { - if !is_template_identifier_char(ch) { - break; + /// The names an assignment target binds. + fn target(&mut self, target: &ast::Expr<'a>) { + match target { + ast::Expr::Var(node) => { + self.bound.insert(node.id); + } + // A tuple target, nested arbitrarily: `{% for (a, (b, c)) in … %}`. + ast::Expr::List(node) => { + for item in &node.items { + self.target(item); + } + } + // A dotted target mutates an attribute of whatever the path + // resolves to, so `{% set repo_root.x = … %}` reads the global + // rather than binding it. + read => self.expr(read), } - cursor += ch.len_utf8(); } - cursor -} - -fn is_template_attribute_or_assignment(body: &str, start: usize, end: usize) -> bool { - let previous = body[..start].chars().rev().find(|c| !c.is_whitespace()); - if previous == Some('.') { - return true; - } - - let next = body[end..].trim_start(); - next.starts_with('=') && !next.starts_with("==") -} - -fn is_template_identifier_start(ch: char) -> bool { - ch == '_' || ch.is_ascii_alphabetic() -} - -fn is_template_identifier_char(ch: char) -> bool { - ch == '_' || ch.is_ascii_alphanumeric() } /// Replace every [`RETIRED_VARS`] template variable in every string value of @@ -614,13 +500,9 @@ fn migrate_template_vars_doc(doc: &mut toml_edit::DocumentMut) -> Deprecations { fn walk_value(value: &mut toml_edit::Value, replaced: &mut Replaced) { match value { toml_edit::Value::String(s) => { - let pairs = deprecated_vars_in_template(s.value()); - if pairs.is_empty() { - return; - } - if let Some(new) = rewrite_template_var_identifiers(s.value(), &pairs) { + if let Some((migrated, pairs)) = migrate_retired_vars(s.value()) { let decor = s.decor().clone(); - let mut formatted = toml_edit::Formatted::new(new); + let mut formatted = toml_edit::Formatted::new(migrated); *formatted.decor_mut() = decor; *value = toml_edit::Value::String(formatted); replaced.extend(pairs); @@ -2938,16 +2820,32 @@ timeout = 30 ); } - /// A tag the scanner can't terminate leaves the whole template - /// unmigrated rather than guessing where it ends. `{% raw %}` content is - /// literal to MiniJinja, so a template can parse while holding something - /// this scanner reads as an unterminated tag. + /// A template MiniJinja can't parse has no reading to migrate against, so + /// it is left untouched rather than guessed at — and it raises no warning + /// either, since detection reads the same parse. #[test] - fn test_normalize_bails_on_a_tag_it_cannot_terminate() { - let template = r#"{{ repo_root }}{% raw %}{{ " {% endraw %}"#; + fn test_normalize_leaves_an_unparseable_template_untouched() { + let template = "{{ repo_root"; let result = normalize_template_vars(template); assert!(matches!(result, Cow::Borrowed(_)), "should not rewrite"); assert_eq!(result, template); + assert!( + detect_deprecations(&format!("worktree-path = \"{template}\"\n")).is_empty(), + "a template left unrewritten must not warn" + ); + } + + /// `{% raw %}` content is literal text, so a tag opened inside one never + /// has to be terminated. The hand-rolled scan this replaced had to find + /// that tag's end itself and gave up on the whole template; MiniJinja's + /// parser reads the raw block as the literal it is, and the reference + /// before it migrates. + #[test] + fn test_normalize_migrates_past_a_tag_opened_inside_a_raw_block() { + assert_eq!( + normalize_template_vars(r#"{{ repo_root }}{% raw %}{{ " {% endraw %}"#), + r#"{{ repo_path }}{% raw %}{{ " {% endraw %}"# + ); } /// Binding detection only looks at the binding keywords — a deprecated @@ -3006,6 +2904,47 @@ timeout = 30 ); } + /// Identifier positions that are not variable reads: a keyword argument's + /// name and a map key. Neither resolves against the render context, so + /// neither is rewritten — while a real reference beside it still is. + #[test] + fn test_normalize_skips_identifiers_that_are_not_reads() { + assert_eq!( + normalize_template_vars("{{ dict(repo_root=1) }}{{ repo_root }}"), + "{{ dict(repo_root=1) }}{{ repo_path }}" + ); + assert_eq!( + normalize_template_vars(r#"{{ {"repo_root": repo_root} }}"#), + r#"{{ {"repo_root": repo_path} }}"# + ); + } + + /// The parser hands reads back in visit order, which is not source order: + /// a conditional expression is visited test-first, and a map's keys all + /// precede its values. Two migrations whose visit order inverts their + /// positions must still splice into one coherent template. + #[test] + fn test_normalize_rewrites_reads_out_of_visit_order() { + assert_eq!( + normalize_template_vars("{{ repo_root if worktree }}"), + "{{ repo_path if worktree_path }}" + ); + assert_eq!( + normalize_template_vars("{{ {a: repo_root, worktree: b} }}"), + "{{ {a: repo_path, worktree_path: b} }}" + ); + } + + /// `{% do %}` is a statement of its own, not an expression emitted by a + /// `{{ }}`, so its call's arguments are reads like any other. + #[test] + fn test_normalize_rewrites_inside_a_do_statement() { + assert_eq!( + normalize_template_vars("{% do dict(x=repo_root) %}"), + "{% do dict(x=repo_path) %}" + ); + } + /// Identifiers inside `{# #}` comments must not be rewritten. #[test] fn test_normalize_skips_comment_tags() { @@ -3999,6 +3938,9 @@ hostname = "forge.example" // binding the *canonical* name holds the rename back from the // other side "worktree-path = \"{{ repo_root }}{% for repo_path in items %}{{ repo_root }}{% endfor %}\"\n", + // a template MiniJinja can't parse has no reading to migrate + // against, and the renderer won't take it either + "worktree-path = \"{{ repo_root\"\n", // Live variables whose names merely contain a retired one. The // rewrite matches whole identifiers, and it now runs on every // load, so a substring match here would mangle a current variable From 374359c4b742d9946c3673c6580e2a394f243b80 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 16 Sep 2026 18:04:33 +0000 Subject: [PATCH 2/4] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/config/deprecation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/deprecation.rs b/src/config/deprecation.rs index 98183b4f24..d14540861e 100644 --- a/src/config/deprecation.rs +++ b/src/config/deprecation.rs @@ -2824,7 +2824,7 @@ timeout = 30 /// it is left untouched rather than guessed at — and it raises no warning /// either, since detection reads the same parse. #[test] - fn test_normalize_leaves_an_unparseable_template_untouched() { + fn test_normalize_leaves_an_unparsable_template_untouched() { let template = "{{ repo_root"; let result = normalize_template_vars(template); assert!(matches!(result, Cow::Borrowed(_)), "should not rewrite"); From a5f81f6e5cbc02686346d565c5a9c6b624bfbc91 Mon Sep 17 00:00:00 2001 From: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> Date: Wed, 16 Sep 2026 18:05:46 +0000 Subject: [PATCH 3/4] test(config): pin a read in every expression shape the walk descends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The visitor has one arm per AST node, and a missed arm loses a migration silently rather than failing. One case per shape — slices, unary and binary ops, tests, filters, call splats, autoescape and filter blocks, a for-else and an if-else body — keeps every arm exercised. --- src/config/deprecation.rs | 54 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/config/deprecation.rs b/src/config/deprecation.rs index d14540861e..da3f147b43 100644 --- a/src/config/deprecation.rs +++ b/src/config/deprecation.rs @@ -2945,6 +2945,60 @@ timeout = 30 ); } + /// A read is a read wherever the expression puts it, so every node the + /// walk descends through has to reach the `Expr::Var` underneath. One case + /// per shape the parser can produce. + #[test] + fn test_normalize_rewrites_reads_in_every_expression_shape() { + for (template, expected) in [ + ("{{ items[repo_root:] }}", "{{ items[repo_path:] }}"), + ( + "{{ items[:repo_root:worktree] }}", + "{{ items[:repo_path:worktree_path] }}", + ), + ("{{ items[repo_root] }}", "{{ items[repo_path] }}"), + ("{{ not repo_root }}", "{{ not repo_path }}"), + ("{{ -repo_root }}", "{{ -repo_path }}"), + ( + "{{ repo_root ~ worktree }}", + "{{ repo_path ~ worktree_path }}", + ), + ("{{ repo_root is defined }}", "{{ repo_path is defined }}"), + ( + "{{ repo_root | default(worktree) }}", + "{{ repo_path | default(worktree_path) }}", + ), + ("{{ dict(**repo_root) }}", "{{ dict(**repo_path) }}"), + ("{{ dict(*repo_root) }}", "{{ dict(*repo_path) }}"), + ( + "{{ repo_root(worktree) }}", + "{{ repo_path(worktree_path) }}", + ), + ( + "{% autoescape repo_root %}{{ worktree }}{% endautoescape %}", + "{% autoescape repo_path %}{{ worktree_path }}{% endautoescape %}", + ), + ( + "{% filter upper %}{{ repo_root }}{% endfilter %}", + "{% filter upper %}{{ repo_path }}{% endfilter %}", + ), + ( + "{% for x in items %}{% else %}{{ repo_root }}{% endfor %}", + "{% for x in items %}{% else %}{{ repo_path }}{% endfor %}", + ), + ( + "{% if a %}{% else %}{{ repo_root }}{% endif %}", + "{% if a %}{% else %}{{ repo_path }}{% endif %}", + ), + ] { + assert_eq!( + normalize_template_vars(template), + expected, + "for: {template}" + ); + } + } + /// Identifiers inside `{# #}` comments must not be rewritten. #[test] fn test_normalize_skips_comment_tags() { From deef2f2f3b314087efd3964ce89cf6837595915d Mon Sep 17 00:00:00 2001 From: worktrunk-bot Date: Wed, 16 Sep 2026 18:48:25 +0000 Subject: [PATCH 4/4] test(config): cover the chained-comparison and else arms of the walk `codecov/patch` missed `Expr::Compare` and `IfExpr.false_expr`, the two arms `test_normalize_rewrites_reads_in_every_expression_shape` claims to pin one case each of. `Compare` is the non-obvious one: MiniJinja's `parse_compare` lowers a single comparison to `Expr::BinOp` and only emits `Expr::Compare` for a chained one, so the existing `repo_root == "x"` case never reached it. Also correct the comment justifying `Default::default()` on the parse. `expand_template_with` renders with `keep_trailing_newline(true)`, so the configs don't match; what makes the mismatch safe is that a `WhitespaceConfig` only shapes literal text, never an `Expr::Var` span. --- src/config/deprecation.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/config/deprecation.rs b/src/config/deprecation.rs index da3f147b43..0a49fc38ab 100644 --- a/src/config/deprecation.rs +++ b/src/config/deprecation.rs @@ -291,10 +291,16 @@ impl<'a> TemplateVars<'a> { /// `None` when MiniJinja can't parse `template` — the templates its /// renderer rejects too, left untouched rather than guessed at. fn of(template: &'a str) -> Option { - // The syntax and whitespace defaults, which are what the environments - // that render these templates use. Spelled `Default::default()` + // The syntax and whitespace defaults, spelled `Default::default()` // because `SyntaxConfig` is a unit struct without MiniJinja's - // `custom_syntax` feature. + // `custom_syntax` feature. Neither has to match the environment that + // renders the template — `expand_template_with` sets + // `keep_trailing_newline(true)` for every `ShellEscapeMode` but + // `Literal`, and a `WhitespaceConfig` only ever shapes literal text + // (which byte the tokenizer stops at, where an `EmitRaw` node's + // boundaries fall), never an `Expr::Var` span. Everything outside + // those spans is copied from the original string, so the two readings + // cannot move an edit apart. let ast = parse_template(template, "", Default::default(), Default::default()).ok()?; let mut vars = TemplateVars { @@ -2959,6 +2965,13 @@ timeout = 30 ("{{ items[repo_root] }}", "{{ items[repo_path] }}"), ("{{ not repo_root }}", "{{ not repo_path }}"), ("{{ -repo_root }}", "{{ -repo_path }}"), + // `Expr::Compare` needs a *chained* comparison: `parse_compare` + // lowers a single one to `Expr::BinOp`, so `repo_root == "x"` + // never reaches the arm that walks the operand list. + ("{{ 1 < repo_root < 3 }}", "{{ 1 < repo_path < 3 }}"), + // A conditional's `else` arm, which the visit-order case above + // leaves off. + ("{{ a if b else repo_root }}", "{{ a if b else repo_path }}"), ( "{{ repo_root ~ worktree }}", "{{ repo_path ~ worktree_path }}",