Verify finish_if handling of multiple spans ending at same position

[coderabbitai]

Issue Description

finish_if may call finish_node multiple times for the same position.

Verify that handling multiple spans ending at the same position does not cause panics or corrupt the tree structure.

Location

src/parser/cst_builder/tree.rs:38

Code Context

    fn finish_if(&mut self, builder: &mut GreenNodeBuilder, pos: usize) {
        if matches!(self.iter.peek(), Some(s) if pos >= s.end) {
            builder.finish_node();

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 #81 details
gh issue view 81 --json title,body,labels,state

# Find README and documentation files
fd -t f 'README|CONTRIBUTING|doc' -i

# Find the main source files to understand the codebase structure
fd -e py -e js -e ts -e go -e java -e rs | head -20

Length of output: 1422

---

🏁 Script executed:

#!/bin/bash

# Check if the file still exists and view the finish_if implementation
cat src/parser/cst_builder/tree.rs

# Search for any tests related to finish_if or multiple spans
rg -n "finish_if|multiple.*span" --type rust -C 3

# Check PR #79 comments for more context
gh pr view 79 --json title,body,comments

# Look for any panic handling or assertions around this code
rg -n "finish_node|finish_if" src/parser/cst_builder/tree.rs -C 5

Length of output: 102169

---

🏁 Script executed:

#!/bin/bash

# Check the current implementation and look for any tests or handling of overlapping spans
cat src/parser/cst_builder/tree.rs

# Search for tests related to overlapping or multiple spans at same position
rg -n "overlap|same.*position|multiple.*span.*end" --type rust -C 2

# Check the span validation logic
cat src/parser/cst_builder/spans.rs

# Look for any documentation about span constraints
rg -n "span.*overlap|span.*end|span.*constraint" --type rust -C 2

Length of output: 37539

---

🏁 Script executed:

#!/bin/bash

# Look for any existing tests that might cover this scenario
rg -n "finish_if|finish_node" --type rust -C 3 src/parser/cst_builder/

# Check if there are any integration tests that might trigger this
rg -n "nested|hierarchy" --type rust src/parser/cst_builder/

# Check the rowan documentation or usage to understand finish_node behavior
rg -n "GreenNodeBuilder|finish_node" --type rust -C 2 | head -50

# Look for expression spans that might overlap with statement spans
cat src/parser/span_scanner.rs | grep -A 20 "expression"

Length of output: 5181

---

@leynos, I've analysed issue #81 in detail.

Relevance: STILL RELEVANT

The issue remains relevant. The current implementation in src/parser/cst_builder/tree.rs shows that finish_if() processes one span per cursor per position. Whilst the span validation ensures spans within the same category (e.g., imports) cannot overlap, spans from different categories can and do end at the same position - specifically, expression spans are nested within statement spans.

The code shows 8 span cursor categories:

• 7 statement types (imports, typedefs, relations, indexes, functions, transformers, rules)
• 1 expression category (nested within rules)

When an expression ends at the same position as its containing rule, cursors.finish() will call finish_node() once for the expression cursor and once for the rule cursor at the same position.

Code Status

The validation in spans.rs enforces non-overlapping spans within each category:

if first.end > second.start {
    return Err(SpanOrderError { ... });
}

However, there are no tests specifically verifying the handling of nested spans ending at the same position, which is precisely what the issue asks to verify.

Documentation Status

The documentation does not address this scenario. The module-level docs describe span storage but don't explain the nesting behaviour or guarantee correctness when multiple finish_node() calls occur at the same position.

Criticality: MEDIUM

• Risk to users: Medium - If the nesting is incorrect, this could cause panics or corrupt the syntax tree, breaking parsing entirely
• Blast radius: Medium - Affects any code containing expressions (which is most real-world Datalog code)
• Likelihood: Low - The rowan library's stack-based GreenNodeBuilder should handle this correctly, but it's unverified

Required Changes Impact

Code changes needed:

1. Add integration tests with nested expressions to verify correct tree structure
2. Consider adding debug assertions to validate node stack depth matches span nesting
3. Potentially add runtime checks or better error messages if nesting violations occur

Estimated effort: Low-Medium - Primarily adding tests rather than fixing bugs

I'll now apply the appropriate label.

---

🏁 Script executed:

#!/bin/bash

# Apply the "medium" criticality label to issue #81
gh issue edit 81 --add-label "medium"

Length of output: 42

---

Perfect! I've successfully applied the medium label to issue #81.

Summary

Issue Status: ✅ Still Relevant - Requires verification

Criticality: 🟡 MEDIUM

Labels Applied: medium

Documentation Status: Documentation does not address the nesting scenario where multiple spans end at the same position.

Impact Assessment

Required Changes

Type: Testing + potential assertions

Changes needed:

1. Add integration tests verifying correct CST structure when expression spans end at the same position as their containing statement spans
2. Consider adding debug assertions to validate the node stack depth matches expected span nesting levels
3. Optionally enhance error messages if nesting violations are detected

Estimated complexity: Low-Medium

• Primary work is adding test coverage for existing behaviour
• The rowan GreenNodeBuilder should handle multiple finish_node() calls correctly (it's stack-based)
• Risk is low that actual bugs exist, but verification is needed

Files affected:

• src/parser/cst_builder/tree.rs (potential assertions)
• src/parser/cst_builder/tests.rs or similar (new tests)

The issue correctly identifies that the current implementation calls finish_node() multiple times at the same position when nested spans (like expressions within rules) end simultaneously. This needs verification that it doesn't corrupt the tree structure.