Optimize span usage to avoid unnecessary cloning

[coderabbitai]

Issue Description

Cloning the span for src.get may be unnecessary.

Consider using a reference or passing Span by value if it implements Copy, as this may improve performance and code clarity.

Location

src/parser/cst_builder/tree.rs:117

Code Context

fn push_token(builder: &mut GreenNodeBuilder, kind: SyntaxKind, span: Span, src: &str) {
    let text = src.get(span.clone()).map_or_else(
        || {

Suggested Improvement

Consider whether span.clone() is necessary or if span can be used directly, especially if Span implements Copy.

References

• PR: Refactor CST construction into module #79
• Comment: Refactor CST construction into module #79 (comment)

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.\n\nOnce a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

Please also provide an impact of the required changes.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Fetch issue #86 details
gh issue view 86 --json title,body,state,labels,comments --jq '{title: .title, body: .body, state: .state, labels: [.labels[].name], comments: [.comments[] | {author: .author.login, body: .body}]}'

Length of output: 1506

---

🏁 Script executed:

#!/bin/bash

# Check the current code at the specified location
echo "=== Checking src/parser/cst_builder/tree.rs around line 117 ==="
cat src/parser/cst_builder/tree.rs | sed -n '110,125p'

echo -e "\n=== Finding Span type definition ==="
# Search for Span type definition to check if it implements Copy
rg -n "struct Span" --type rust -A 5

echo -e "\n=== Checking if Span derives Copy ==="
# Look for Copy trait implementation for Span
rg -n "impl.*Copy.*Span|derive.*Copy.*Span|Span.*Copy" --type rust

echo -e "\n=== Search for Span type definition in type aliases ==="
rg -n "type Span" --type rust -A 2

Length of output: 2126

---

🏁 Script executed:

#!/bin/bash

# Get the full push_token function to see if span is used after the clone
echo "=== Full push_token function ==="
ast-grep --pattern $'fn push_token($$$) {
  $$$
}'

echo -e "\n=== Let's get more context with sed ==="
# Get more lines to see the full function
sed -n '117,135p' src/parser/cst_builder/tree.rs

Length of output: 1854

---

🏁 Script executed:

#!/bin/bash

# Check if there are other similar patterns in the codebase
echo "=== Looking for other span.clone() patterns ==="
rg -n "span\.clone\(\)" --type rust -C 2

echo -e "\n=== Check if str::get accepts references ==="
# Let's see if there are examples of using span without clone in the codebase
rg -n "src\.get\(span\)" --type rust -C 2

echo -e "\n=== Check README or documentation for performance considerations ==="
fd -e md | head -5 | xargs -I {} sh -c 'echo "=== {} ==="; cat {}'

Length of output: 96956

---

🏁 Script executed:

#!/bin/bash

# Check if str::get can accept references to Range
echo "=== Checking if there are patterns using &span or references ==="
rg -n "\.get\(&span" --type rust -C 2

echo -e "\n=== Check str::get signature documentation ==="
# Let's see if we can understand how get is used elsewhere
rg -n "src\.get\(" --type rust | head -20

echo -e "\n=== Check if span could be borrowed in the push_token signature ==="
# Look at how push_token is called to see if we could change the signature
rg -n "push_token\(" --type rust -C 3

Length of output: 2685

---

🏁 Script executed:

#!/bin/bash

# Apply the 'low' criticality label to issue #86
gh issue edit 86 --add-label "low"

# Verify the label was added
gh issue view 86 --json labels --jq '.labels[].name'

Length of output: 46

---

@leynos, I've analysed the codebase and can confirm that issue #86 is still relevant—the code has not been updated since the issue was opened.

Current State

The code at src/parser/cst_builder/tree.rs:119 still contains the unnecessary clone:

fn push_token(builder: &mut GreenNodeBuilder, kind: SyntaxKind, span: Span, src: &str) {
    let text = src.get(span.clone()).map_or_else(
        || {
            warn!(
                "token span {:?} out of bounds for source of length {}",
                span,
                src.len()
            );
            ""
        },
        |t| t,
    );
    // ...
}

The clone is currently necessary because span is passed by value and then used again in the warning message. However, this pattern is suboptimal.

Why It Matters (and Why It Doesn't Much)

Span is defined as type Span = std::ops::Range<usize>, and Range<usize> does not implement Copy. Rust's str::get() accepts impl SliceIndex<str>, which includes &Range<usize>, so the clone is avoidable.

However, the performance impact is negligible: cloning a Range<usize> means copying two usize values (16 bytes on 64-bit systems) once per token. This is a micro-optimization.

Criticality Assessment: LOW

Risk: Minimal. The issue affects performance imperceptibly and has no correctness implications.

Blast radius: Very low. The change is isolated to a single helper function and its call site.

User impact: None. Users will not notice any difference in behaviour or performance.

Impact of Required Changes

Scope: Minimal—two lines of code:

1. Change push_token signature to accept &Span instead of Span
2. Update the call site at line 109 from span.clone() to span

Risk: Very low. The change is straightforward and type-checked by the compiler.

Testing: Existing tests should continue to pass without modification.

Similar patterns: The codebase contains 30+ instances of span.clone(), but this issue specifically addresses the most unnecessary case where the clone happens solely because of function signature design rather than genuine ownership requirements.

Recommended Fix

- fn push_token(builder: &mut GreenNodeBuilder, kind: SyntaxKind, span: Span, src: &str) {
+ fn push_token(builder: &mut GreenNodeBuilder, kind: SyntaxKind, span: &Span, src: &str) {
-     let text = src.get(span.clone()).map_or_else(
+     let text = src.get(span).map_or_else(
          || {
              warn!(
                  "token span {:?} out of bounds for source of length {}",
                  span,
                  src.len()
              );
              ""
          },
          |t| t,
      );

And at the call site:

-         push_token(&mut builder, kind, span.clone(), src);
+         push_token(&mut builder, kind, span, src);

I've applied the low criticality label to the issue.