crates/config/src/rule_config.rs (2)

201-205: ⚠️ Potential issue

map_err converts to the wrong error type – compilation will fail

Fixer::parse returns a result with its own error type (likely FixerError).
The current call:

let parsed = Fixer::parse(fix, &env, &self.transform).map_err(RuleCoreError::Fixer)?;

changes that error into RuleCoreError, but the ? operator subsequently tries to bubble up a RuleCoreError even though get_fixer is declared to return Result<_, RuleConfigError>. This is a type mismatch and will not compile.

Proposed fix:

-      let parsed = Fixer::parse(fix, &env, &self.transform).map_err(RuleCoreError::Fixer)?;
+      let parsed = Fixer::parse(fix, &env, &self.transform)
+        .map_err(|e| RuleConfigError::Core(RuleCoreError::Fixer(e)))?;

This keeps the conversion localized and preserves the declared return type.

---

131-149: 🛠️ Refactor suggestion

insert_rewriter panics on duplicate IDs instead of reporting an error

reg.insert_rewriter(&val.id, rewriter).expect("should work");

If a user accidentally re‑uses a rewriter ID, the code will panic, aborting the whole run.
Because you already have a dedicated RuleConfigError::DuplicateRule (via ReferentRuleError), consider propagating that error instead of crashing:

-      reg.insert_rewriter(&val.id, rewriter).expect("should work");
+      reg
+        .insert_rewriter(&val.id, rewriter)
+        .map_err(|e| RuleConfigError::Rewriter(e, val.id.clone()))?;

This makes the behaviour consistent with the other insert_* helpers and avoids unexpected panics during configuration loading.

crates/config/src/rule/referent_rule.rs (1)

31-45: ⚠️ Potential issue

Unsafe interior‑mutability on shared Arc<HashMap> risks data races

Registration::write transmutes an &Arc<HashMap<…>> into a mutable &mut HashMap<…> via raw pointer cast.
With the new public GlobalRules::insert function, external callers can now mutate a GlobalRules instance after it has been shared, opening the door to UB if the map is accessed concurrently elsewhere.

Consider replacing this hand‑rolled pattern with a safe concurrent map (dashmap, parking_lot::RwLock<HashMap<…>>, or std::sync::RwLock) to keep mutation fully synchronized and sound.

crates/core/src/ops.rs (1)

209-218: ⚠️ Potential issue

Avoid mutating the caller’s environment inside Not — clone first

Not::match_node_with_env receives env by mutable reference and passes it directly to the inner matcher.
If the inner matcher succeeds, you later return None (because of the XOR), yet all metavariable bindings that were added by the inner matcher remain in env.
These stale bindings “leak” outwards and can pollute subsequent matcher evaluation (e.g. when Not is used inside All, Op::every, etc.).

-    self
-      .not
-      .match_node_with_env(node.clone(), env)
-      .xor(Some(node))
+    // work on a copy so that any bindings produced by `self.not`
+    // are discarded when it matches.
+    let mut shadow_env = Cow::Borrowed(env.as_ref());
+    let matched = self.not.match_node_with_env(node.clone(), &mut shadow_env).is_some();
+    if matched {
+      None
+    } else {
+      Some(node)
+    }

This preserves the intended boolean semantics while guaranteeing that env is only mutated when the Not pattern fails (i.e. when we actually return Some(node)).

crates/config/src/check_var.rs (1)

147-149: ⚠️ Potential issue

Incorrect reference level leads to mismatched‐type compilation error

var is already an &str. Passing &var therefore hands a &&str to HashSet<&str>::contains, which expects &str. This currently compiles only thanks to an (unstable) blanket Borrow implementation and can easily break, or trigger confusing error messages if trait bounds change.

-    if !vars.contains(&var) {
+    if !vars.contains(var) {

A direct reference keeps the type signatures clean and avoids double indirection.

crates/config/src/rule_core.rs (1)

187-192: ⚠️ Potential issue

get_env can fabricate an environment for any language — risks runtime incoherence

After dropping the generic parameter from RuleCore, the method

pub fn get_env<L: Language>(&self, lang: L) -> DeserializeEnv<L>

allows callers to request a DeserializeEnv for any Language, even if the
RuleCore, its Rule, the registered utils/rewriters, and the Fixer
instances were all built for a different language.

That silently breaks the compile‑time guarantee we previously had and can
produce undefined matcher behaviour or panics at runtime when, for example,
Tree‑sitter kind‑ids no longer match.

Recommended approaches:

1. Re‑introduce a language “marker” inside RuleCore, e.g.

phantom: std::marker::PhantomData<L>

and keep RuleCore generic over L, or

2. Store the original language’s LanguageId (or equivalent) at construction
   time and assert equality inside get_env, returning an error otherwise.

Either restores soundness and prevents cross‑language misuse.

crates/cli/src/run.rs (1)

115-128: Unnecessary allocation: avoid .clone() by making Strictness Copy

MatchStrictness is an enum and therefore already Copy.
If we derive Copy for the wrapper Strictness, the extra .clone() (heap‑free but still superfluous) disappears and the intent becomes clearer.

-  fn build_pattern(&self, lang: SgLang) -> Result<Pattern> {
...
-    if let Some(strictness) = &self.strictness {
-      Ok(pattern.with_strictness(strictness.0.clone()))
+  fn build_pattern(&self, lang: SgLang) -> Result<Pattern> {
...
+    if let Some(strictness) = &self.strictness {
+      Ok(pattern.with_strictness(strictness.0))

Add the derive:

 #[derive(Clone)]
-struct Strictness(MatchStrictness);
+#[derive(Clone, Copy)]
+struct Strictness(MatchStrictness);

Tiny win, but eliminates an avoidable method call every time the CLI builds a pattern.

crates/config/src/rule/stop_by.rs (1)

121-148: Public method lacks documentation & example usage

StopBy::find is a central control‑flow utility (“monster method” per the TODO) yet still undocumented after this refactor.
Readers now have to reverse‑engineer:

• expected invariants for once / multi / finder
• how StopBy variants change traversal semantics
• lifetime constraints on 't

Please consider adding a doc‑comment with a short example and move the inline TODO to an issue tracker to avoid bit‑rot.

crates/config/src/rule/deserialize_env.rs (1)

175-189: Minor performance tweak: reuse DeserializeEnv instead of creating one per rule

Inside parse_global_utils a fresh DeserializeEnv is built for every utility rule even though all rules for a given lang share the same GlobalRules. Constructing it once and re‑using it avoids repeated RuleRegistration::from_globals allocations:

-    for id in order {
-      let (lang, core) = utils.get(id).expect("must exist");
-      let env = DeserializeEnv::new(lang.clone()).with_globals(&registration);
+    for (lang, group) in &utils_per_language {
+      let env = DeserializeEnv::new(lang.clone()).with_globals(&registration);
+      for (id, core) in group {
         let matcher = core.get_matcher_with_hint(env, CheckHint::Global)?;
         registration
           .insert(id, matcher)
           .map_err(RuleSerializeError::MatchesReference)?;
       }
     }

Not critical, but could save noticeable CPU on large rule‑sets.

crates/core/src/matcher.rs (2)

76-90: Cache the Pattern to avoid recompiling on every call

Every invocation of match_node_with_env / get_match_len on an &str causes a fresh Pattern::str allocation and language‑clone.
If the same pattern is used repeatedly (a common scenario), this churn can become noticeable.

-    let pattern = Pattern::str(self, node.lang().clone());
+    // cache the compiled pattern once per thread
+    thread_local! {
+        static COMPILED: std::cell::RefCell<
+            std::collections::HashMap<(String, std::any::TypeId), Pattern>
+        > = const { /* ... */ };
+    }
+    let pattern = get_or_insert_compiled(self, node.lang());

A simple once_cell::sync::Lazy or thread‑local hashmap keyed by (pattern_string, lang_type_id) would amortise the cost while keeping the public API intact.

---

131-139: Compute potential_kinds once in FindAllNodes

self.matcher.potential_kinds() is evaluated on every next() call, although the result is immutable for the lifetime of the iterator.

 pub struct FindAllNodes<'tree, D: Doc, M: Matcher> {
   dfs: Pre<'tree, D>,
   matcher: M,
+  kinds: Option<BitSet>,
 }
 
 impl<'tree, D: Doc, M: Matcher> FindAllNodes<'tree, D, M> {
   pub fn new(matcher: M, node: Node<'tree, D>) -> Self {
-    Self {
-      dfs: node.dfs(),
-      matcher,
-    }
+    let kinds = matcher.potential_kinds();
+    Self { dfs: node.dfs(), matcher, kinds }
   }
 }

This micro‑optimisation reduces repeated allocations / calculations during large searches.

crates/config/src/rule/mod.rs (1)

259-275: Redundant Ok() wrapper in verify_util

verify_util already returns Result<(), _>. Wrapping it in another Ok(..) adds an unnecessary layer.

-      Rule::Matches(r) => Ok(r.verify_util()?),
+      Rule::Matches(r) => r.verify_util(),

Same behaviour, cleaner code.

crates/config/src/rule_config.rs (1)

118-127: Minor: redundant clone of DeserializeEnv

self.core.get_matcher(env.clone())? consumes env.clone() while the original env is not reused later.
DeserializeEnv is cheap, but you can remove the extra allocation:

-    let rule = self.core.get_matcher(env.clone())?;
+    let rule = self.core.get_matcher(env)?;

This also removes the need for env to implement Clone here.

crates/config/src/rule/relational_rule.rs (1)

184-195: try_new still generic over L even though the struct is language‑agnostic

Precedes::try_new, Follows::try_new, Has::try_new, and Inside::try_new now only use DeserializeEnv<L> to deserialize the inner rule. The surrounding refactor removes language generics elsewhere; you could finish the clean‑up by changing the signature to accept &DeserializeEnv without <L> and erase the type parameter entirely:

-  pub fn try_new<L: Language>(relation: Relation, env: &DeserializeEnv<L>)
+  pub fn try_new(relation: Relation, env: &DeserializeEnv)

This keeps the public API surface minimal and consistent with other de‑genericised types.

crates/core/src/ops.rs (1)

128-150: Minor: simplify the Any search loop

Inside the Any matcher you recreate new_env for every iteration.
Instead, allocate it once and clone only when a candidate pattern matches; this avoids a needless Cow re‑borrow on every miss.

-    let mut new_env = Cow::Borrowed(env.as_ref());
-    let found = self.patterns.iter().find_map(|p| {
-      new_env = Cow::Borrowed(env.as_ref());
-      p.match_node_with_env(node.clone(), &mut new_env)
-    });
+    let mut scratch = Cow::Borrowed(env.as_ref());
+    let found = self.patterns.iter().find_map(|p| {
+      scratch.to_mut().clear();               // restore to original each round
+      p.match_node_with_env(node.clone(), &mut scratch)
+    });

Not critical, but this gives a tiny perf win and makes the control‑flow clearer.

crates/config/src/rule/nth_child.rs (1)

206-230: Avoid allocating a full Vec when locating the index

find_index currently builds a Vec of all siblings only to call .position().
This creates unnecessary heap traffic when traversing large ASTs.

-    let mut children: Vec<_> = if let Some(rule) = &self.of_rule {
-      /* … */
-    } else {
-      /* … */
-    };
-    if self.reverse {
-      children.reverse()
-    }
-    children
-      .iter()
-      .position(|child| child.node_id() == node.node_id())
+    let iter = parent.children().filter(|n| n.is_named());
+    let iter = if let Some(rule) = &self.of_rule {
+      iter.filter(|child| rule.match_node_with_env(*child, env).is_some())
+    } else {
+      iter
+    };
+    let iter: Box<dyn Iterator<Item = _>> = if self.reverse {
+      Box::new(iter.collect::<Vec<_>>().into_iter().rev())
+    } else {
+      Box::new(iter)
+    };
+    iter.enumerate()
+        .find(|(_, child)| child.node_id() == node.node_id())
+        .map(|(idx, _)| idx)

This keeps the algorithm O(n) but removes the intermediate allocation (and a potential full reverse of the vector).

crates/config/src/fixer.rs (2)

139-150: matcher should be passed by reference to avoid an extra move

get_replaced_range currently takes matcher: impl Matcher by value.
Callers have to pass an owned matcher, causing an extra move (and possibly a clone if they want to reuse it).

-  fn get_replaced_range(&self, nm: &NodeMatch<D>, matcher: impl Matcher) -> Range<usize> {
+  fn get_replaced_range(
+    &self,
+    nm: &NodeMatch<D>,
+    matcher: &impl Matcher,
+  ) -> Range<usize> {

All call‑sites inside the crate already have a reference handy, so this change is API‑compatible but slightly more ergonomic.

---

41-59: Remove the lingering generic on Expansion::parse

Expansion itself is now language‑agnostic, yet parse still carries <L: Language> only to thread DeserializeEnv<L>.
Since DeserializeEnv already encapsulates the language, the extra type parameter is no longer needed and hurts readability.

-  fn parse<L: Language>(
+  fn parse(
     relation: &Maybe<Relation>,
-    env: &DeserializeEnv<L>,
+    env: &DeserializeEnv<impl Language>,
   ) -> Result<Option<Self>, FixerError> {

Or simply accept a generic E: ?Sized if you want to stay completely agnostic.

crates/config/src/check_var.rs (1)

21-28: Unnecessary double‑indirection on Fixer argument

fixer is passed as &Option<Fixer>.
All call‑sites already own an Option<Fixer> that they borrow, so the extra & buys no benefit and forces every callee to peel two levels of indirection.

Consider changing the signature (and the corresponding callee sites) to accept Option<&Fixer>:

-pub fn check_rule_with_hint<'r>(
-  ...
-  fixer: &Option<Fixer>,
+pub fn check_rule_with_hint<'r>(
+  ...
+  fixer: Option<&'r Fixer>,

This removes one layer of reference and makes intent ( “maybe there is a fixer, and if so borrow it” ) explicit.

crates/config/src/rule_core.rs (1)

140-148: Derive Debug/Clone for easier diagnostics

Now that RuleCore is non‑generic, deriving common traits no longer risks
excessive monomorphisation. Adding at least #[derive(Clone, Debug)] eases
unit‑test authoring and ad‑hoc logging.

- pub struct RuleCore {
+ #[derive(Clone, Debug)]
+ pub struct RuleCore {

Doing so has zero runtime cost and improves maintainability.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

crates/core/src/meta_var.rs (1)

37-44: 🛠️ Refactor suggestion

Returning Option<&mut Self> complicates ergonomics

insert currently yields Some(self) on success to enable chaining, but exposes None on failure.
Most callers either (a) ignore the result or (b) only care about the boolean success status.

A simpler, more idiomatic signature would be:

-pub fn insert(&mut self, id: &str, ret: N) -> Option<&mut Self>
+pub fn insert(&mut self, id: &str, ret: N) -> bool

This:

• avoids exposing a second mutable reference (preventing accidental misuse),
• mirrors HashMap::insert ergonomics (bool or Option<Old>),
• still allows chaining via if env.insert(...){ /* ... */ }.

The same observation applies to insert_multi.

crates/core/src/meta_var.rs (4)

20-25: Derive Debug and consider Default for easier diagnostics

SgMetaVarEnv is a core container that frequently travels across APIs. Deriving Debug (and possibly Default) will:

• allow println!("{:?}", env) during debugging/tests,
• avoid manual boiler-plate when the struct evolves.

-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct SgMetaVarEnv<'tree, N: SgNode<'tree>, C> {

---

71-73: API inconsistency between get_labels and get_multiple_matches

After fixing the allocation issue above, get_multiple_matches will return &[N] while get_labels returns Option<&Vec<N>>.

For uniformity and clarity, consider aligning get_labels to the same slice-based signature:

-pub fn get_labels(&self, label: &str) -> Option<&Vec<N>> {
-  self.multi_matched.get(label)
+pub fn get_labels(&self, label: &str) -> Option<&[N]> {
+  self.multi_matched.get(label).map(Vec::as_slice)
 }

This prevents accidental mutations through the shared &Vec reference and reduces cognitive overhead.

---

151-167: Potential mutable-aliasing in match_constraints

Cow::Borrowed(self) is created while you still iterate over &self.single_matched.
If m.match_node_with_env mutates the environment, Cow will be upgraded to Owned, cloning the maps — safe but potentially expensive.
Two suggestions:

1. Cheap path optimisation
   Check if var_matchers.is_empty() early and short-circuit.

2. Avoid cloning when mutation is unlikely
   Replace the Cow strategy with a two-phase approach: first read-only check, then mutate only the variables that passed.

This keeps semantics intact while sidestepping the hidden allocation cost.

---

126-139: insert_transformation unnecessarily copies slices

formatted_slice(...).to_vec() allocates even when the formatted slice is already correctly aligned.
If the intent is to avoid copying when formatting is a no-op, you can store a Cow<'_, [u8]> instead of an owned Vec<u8> inside transformed_var.
This change:

• eliminates redundant allocations,
• is backwards-compatible for immutable readers.

Example diff (simplified):

-  transformed_var: HashMap<MetaVariableID, C>,
+  transformed_var: HashMap<MetaVariableID, Cow<'tree, [u8]>>,

and update call-sites accordingly.

If zero-copy isn’t achievable, at least add a comment clarifying why allocation is unavoidable.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

crates/config/src/transform/transformation.rs (1)

79-83: ⚠️ Potential issue

unwrap() on invalid regex can panic at runtime

Replace::<MetaVariable>::compute will call Regex::new(&self.replace).unwrap().
An end-user typo in the YAML config would therefore crash the whole run instead of producing a graceful error.

- let re = Regex::new(&self.replace).unwrap();
+ let re = match Regex::new(&self.replace) {
+   Ok(r) => r,
+   Err(e) => {
+     log::error!("Invalid regex in replace transform: {e}");
+     return None; // or propagate as a TransformError if you prefer
+   }
+ };

Alternatively, compile the regex once during parse(); that turns a user error into a configuration-time failure and avoids recompiling on every match.

crates/core/src/replacer.rs (1)

21-36: 🛠️ Refactor suggestion

Avoid unnecessary clone() in get_replaced_range

matcher.get_match_len(nm.get_node().clone()) clones the entire node even though get_match_len should only need a reference for read-only access. Cloning large sub-trees can be expensive when replacements are executed in bulk.

-    if let Some(len) = matcher.get_match_len(nm.get_node().clone()) {
+    if let Some(len) = matcher.get_match_len(nm.get_node()) {

You will need to adjust the Matcher::get_match_len signature to take impl Borrow<N> or &N to make this compile.

crates/config/src/transform/rewrite.rs (1)

46-86: ⚠️ Potential issue

Edge-case: returning empty string when no edits produced

If self.join_by is None and the find_and_make_edits call returns an empty vector, make_edit will receive an empty edit list and return a byte-for-byte copy of bytes, so all good.
However, when join_by is present but no edit matches any sub-node, rewritten becomes the empty ret vector created in the if let Some(joiner) arm (lines 62-81). The function then encodes that empty vector and returns Some(String::new()), silently dropping the original content.

Consider short-circuiting when edits.is_empty():

if edits.is_empty() {
  return None; // or Some(original) depending on desired semantics
}

Failing to do so may produce surprising, destructive rewrites.

crates/core/src/matcher/node_match.rs (1)

82-101: ⚠️ Potential issue

Trait impls on the type alias will not compile

impl From<NodeMatch<'tree, D>> … and impl Borrow<…> for NodeMatch<'tree, D> try to implement traits for the alias, which is currently disallowed on stable Rust (E0215).

Replace the impl targets with the concrete type:

-impl<'tree, D: Doc> From<NodeMatch<'tree, D>> for Node<'tree, D> {
+impl<'tree, D: Doc>
+  From<SgNodeMatch<
+        'tree,
+        Node<'tree, D>,
+        Vec<<<D as Doc>::Source as Content>::Underlying>>> for Node<'tree, D>
 {
   fn from(node_match: /* … */) -> Self { node_match.0 }
 }

-impl<'tree, D: Doc> Borrow<Node<'tree, D>> for NodeMatch<'tree, D> {
+impl<'tree, D: Doc>
+  Borrow<Node<'tree, D>> for SgNodeMatch<
+        'tree,
+        Node<'tree, D>,
+        Vec<<<D as Doc>::Source as Content>::Underlying>>
 {
   fn borrow(&self) -> &Node<'tree, D> { &self.0 }
 }

(or, alternatively, resurrect NodeMatch as a new-type wrapper instead of a type alias).

Without this fix the crate will fail to compile on stable/stable-beta channels.

crates/core/src/ops.rs (1)

209-218: ⚠️ Potential issue

Not may leave stray bindings in env

When the inner pattern matches (so Not should fail), any metavariables it bound remain in env, because the original environment is passed directly.
This behaviour diverges from the logical expectation that a failed Not leaves no trace.

Please take the same “speculative env” approach used in Or / Any:

-    self
-      .not
-      .match_node_with_env(node.clone(), env)
-      .xor(Some(node))
+    let mut new_env = Cow::Borrowed(env.as_ref());
+    let matched = self.not.match_node_with_env(node.clone(), &mut new_env);
+    if matched.is_some() {
+      // inner matched → NOT fails, discard changes
+      None
+    } else {
+      // inner failed → NOT succeeds, keep original env
+      Some(node)
+    }

crates/core/src/match_tree/mod.rs (2)

14-23: Pass slices instead of Vec to avoid needless allocations

match_ellipsis receives nodes: Vec<N>, which forces every caller to allocate a fresh vector even when it only needs read-only access. Switching the parameter to &[N] (or an iterator) removes this cost and avoids one more clone when the nodes are already materialised elsewhere.

-  fn match_ellipsis(
-    &mut self,
-    var: Option<&str>,
-    nodes: Vec<N>,
-    skipped_anonymous: usize,
-  ) -> Option<()>;
+  fn match_ellipsis(
+    &mut self,
+    var: Option<&str>,
+    nodes: &[N],
+    skipped_anonymous: usize,
+  ) -> Option<()>;

All three implementors only read the slice (they call last() or drain), so the change is mechanical and backwards-compatible with the public API introduced in this PR.

---

119-140: Potential stack overflow on extremely deep trees

does_node_match_exactly is implemented recursively without tail-call optimisation. Real-world code normally keeps depth in check, but artificial or adversarial inputs (e.g. deeply nested parentheses) can overflow the stack.

If you want extra robustness, consider an explicit stack (Vec) or a try_fold style loop:

let mut stack = vec![(goal.clone(), candidate.clone())];
while let Some((g, c)) = stack.pop() {
  if g.node_id() == c.node_id() { continue; }
  if g.is_named_leaf() || c.is_named_leaf() {
    if g.text() != c.text() { return false; }
    continue;
  }
  if g.kind_id() != c.kind_id() { return false; }
  let (mut gc, mut cc) = (g.children(), c.children());
  if gc.len() != cc.len() { return false; }
  stack.extend(gc.zip(cc));
}
true

It keeps behaviour identical while capping stack use.

crates/config/src/rule/range.rs (1)

68-88: Minor performance nit: avoid repeated UTF-8 column computations

match_node_with_env calls column(&node) twice (start and end). column walks the UTF-8 string from the start each time, so on large files this adds up. Caching the result once per node is cheaper:

- if self.start.column != node_start_pos.column(&node)
-   || self.end.column != node_end_pos.column(&node)
+ let (start_col, end_col) = (node_start_pos.column(&node), node_end_pos.column(&node));
+ if self.start.column != start_col || self.end.column != end_col

Very small, but essentially free.

crates/config/src/transform/transformation.rs (3)

13-20: Double allocation when converting bytes to String

get_text_from_env first materialises the whole byte slice, then encode_bytes returns a Cow<str> that is immediately turned into an owned String. For large captures this allocates twice.

You can request an owned string directly from the content API (if available) or reuse the existing buffer:

- let bytes = ctx.env.get_var_bytes(var)?;
- Some(<<N::Doc as Doc>::Source as Content>::encode_bytes(bytes).into_owned())
+ let bytes = ctx.env.get_var_bytes(var)?;
+ Some(
+   <<N::Doc as Doc>::Source as Content>::encode_bytes(bytes)
+     .into_owned() // still one allocation; unavoidable without API change
+ )

If encode_bytes already returns an owned String (instead of Cow), drop the into_owned() call altogether.

---

38-48: Substring::compute creates an intermediate Vec<char> – consider a slice-based approach

Building chars: Vec<_> = text.chars().collect() allocates O(n) memory and copies every Unicode scalar value. You can achieve the same with char_indices() and byte slicing without the extra heap:

let start = resolve_char(&self.start_char, 0, len);
let end = resolve_char(&self.end_char, len, len);
let bytes = text
  .char_indices()
  .map(|(i, _)| i)
  .chain(std::iter::once(text.len()))
  .collect::<Vec<_>>();
let slice = &text[bytes[start]..bytes[end]];
Some(slice.to_owned())

This avoids the allocation and keeps complexity linear.

---

172-181: Avoid redundant allocation when storing transformed bytes

decode_str(&s).to_vec() copies the entire string into a new Vec<u8>. If the downstream API accepts a Cow<[u8]>, you can save one allocation:

- let bytes = if let Some(s) = opt {
-   <<N::Doc as Doc>::Source as Content>::decode_str(&s).to_vec()
+ let bytes = if let Some(s) = opt {
+   <<N::Doc as Doc>::Source as Content>::decode_str(&s).into_owned()
 } else {
   vec![]
 };

Not critical, but helps when transforms produce large blobs.

crates/core/src/replacer.rs (1)

48-53: Remove dead code or restore structural replacer

The Root<D> implementation is now commented out, but the corresponding mod structural; declaration (line 13) is still present. This leaves an empty module that is compiled but never used and may confuse future readers.

Either:

1. Delete the commented block and remove the structural module entirely, or
2. Re-enable the structural replacer and add #[cfg(feature = "structural_replace")] guards if it is planned for later.

Leaving commented code in main branches is generally discouraged.

crates/config/src/transform/rewrite.rs (2)

101-118: Double DFS may cause quadratic behaviour

replace_one performs for child in node.dfs() and invokes every rule on every descendant node. When multiple rewrites are chained (rules.len() * #descendants), this can become quadratic on large trees.

If rules are mutually exclusive or can be indexed by kind/text, consider:

• Pre-filtering descendants (child.kind_id() lookup)
• Short-circuiting the outer dfs() once a rule inside rules matches its ancestor to avoid visiting grandchildren that will be deleted anyway.

While correctness is intact, large files could show noticeable slow-downs.

---

67-67: Decode joiner once to avoid repeated UTF-8 work

let joiner = <N::Doc as Doc>::Source::decode_str(joiner); is called inside a loop even though joiner is constant. Move it outside the loop to save repeated allocations/decoding.

-        let joiner = <N::Doc as Doc>::Source::decode_str(joiner);
+        let joiner_bytes = <N::Doc as Doc>::Source::decode_str(joiner);
 ...
-          ret.extend_from_slice(&joiner);
+          ret.extend_from_slice(&joiner_bytes);

crates/core/src/replacer/template.rs (4)

41-50: generate_replacement allocates twice per fragment

decode_str is invoked on every fragment and the resulting Vec is immediately copied into ret via extend_from_slice, causing two heap allocations (decode_str → Vec, then extend → copy).

If fragments are sizeable or numerous, cache the decoded value:

-    ret.extend_from_slice(&<N::Doc as Doc>::Source::decode_str(frag));
+    let frag_bytes = <N::Doc as Doc>::Source::decode_str(frag);
+    ret.extend_from_slice(&frag_bytes);

Repeat the same change later in the loop (line 109).

---

114-155: maybe_get_var can use slice APIs to avoid indexing panics

Accessing nodes[0] and nodes[nodes.len() - 1] requires two bounds checks and will panic if the vector is empty (even though you guard for emptiness). The idiomatic pattern is .first() / .last():

-      let start = nodes[0].range().start;
-      let end = nodes[nodes.len() - 1].range().end;
-      let source = nodes[0].get_doc().get_source();
+      let start = nodes.first().unwrap().range().start;
+      let end = nodes.last().unwrap().range().end;
+      let source = nodes.first().unwrap().get_doc().get_source();

Same semantics, slightly safer & faster.

---

91-113: Minor: early-return for Textual branch simplifies control flow

Currently the function sets up variables and then immediately returns for the Textual case. An early return keeps the happy path shallow:

-  let template = match fixer {
-    TemplateFix::Textual(n) => return <N::Doc as Doc>::Source::decode_str(n).to_vec(),
-    TemplateFix::WithMetaVar(t) => t,
-  };
+  if let TemplateFix::Textual(src) = fixer {
+    return <N::Doc as Doc>::Source::decode_str(src).to_vec();
+  }
+  let TemplateFix::WithMetaVar(t) = fixer else { unreachable!() };

Purely stylistic, but improves readability.

---

158-164: Public helper leaks implementation detail

gen_replacement is a thin wrapper around TemplateFix. Exposing it perpetuates two APIs that must stay in sync. Consider marking it #[deprecated] and encouraging callers to create a TemplateFix directly, e.g.:

TemplateFix::try_new(template, nm.lang())?.generate_replacement(nm)

to avoid API surface bloat.

crates/core/src/matcher/pattern.rs (1)

517-518: Size assertion is now brittle after the PhantomData removal

Pattern no longer carries the PhantomData<L> field, so its memory layout has changed.
The ignored test still freezes the previous size (40) which is very likely wrong now and will break when the test is re-enabled.

-assert_eq!(std::mem::size_of::<Pattern>(), 40);
+// ⚠️ Update the expected value or, even better, remove the fragile
+//       size assertion – layout is an implementation detail.
+assert_eq!(std::mem::size_of::<Pattern>(), 32);

Please recompute or drop the assertion to avoid false failures.

crates/core/src/matcher.rs (2)

87-90: Avoid cloning the language object on every match

impl Matcher for str creates a fresh Pattern via Pattern::str(self, node.lang().clone()).
For languages with non-trivial clone() cost this happens for every node examined, which can dominate hot paths such as find_all.

Consider caching the compiled Pattern (e.g. inside a small once_cell::unsync::OnceCell) or switching to Pattern::borrowed(self, node.lang()) if such helper exists.

---

58-63: Minor: unnecessary env allocation in match_node helper

MatcherExt::match_node always starts with an empty MetaVarEnv, which is then cloned into the NodeMatch even when the pattern fails.
You can save one allocation per failure by constructing NodeMatch only after a successful match:

-    let mut env = Cow::Owned(MetaVarEnv::new());
-    let node = self.match_node_with_env(node, &mut env)?;
-    Some(NodeMatch::new(node, env.into_owned()))
+    let mut env = Cow::Owned(MetaVarEnv::new());
+    self.match_node_with_env(node, &mut env)
+        .map(|n| NodeMatch::new(n, env.into_owned()))

crates/core/src/node.rs (2)

38-41: Position::column now takes SgNode, but work is still O(n)

The generic signature is great, but the call still rescans the source every time.
If callers invoke column in tight loops (e.g. pretty printers) consider memoising the result or passing the pre-computed Source slice instead.

---

181-251: Potential self-recursion confusion in trait impl

Several methods delegate to identically-named inherent methods, e.g.:

fn node_id(&self) -> usize {
    self.node_id()
}

This works because inherent methods out-prioritise trait methods, but the pattern is easy to misread as infinite recursion.
Adding an explicit fully-qualified call improves clarity and avoids accidental refactor bugs:

-    self.node_id()
+    Node::node_id(self)

(same for is_named, text, etc.)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro