Skip to content

fix(parser): comment before next binding bleeds into previous content_position #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thedavidmeister wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(parser): comment before next binding bleeds into previous content_position #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 14 additions & 10 deletions crates/dotrain/src/parser/raindocument/logic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,24 +784,28 @@ impl RainDocument {
name = slices.0[..slices.0.len() - 1].to_owned();
name_position = [parsed_binding.1[0], parsed_binding.1[0] + boundry_offset];

let slices = content_text.split_at(boundry_offset + 1);
let trimmed_content = tracked_trim(slices.1);
content_position = if trimmed_content.0.is_empty() {
let orig_slices = content_text.split_at(boundry_offset + 1);
let trimmed_orig = tracked_trim(orig_slices.1);
// For content_position start: use leading trim from original text so comments
// at the start of a binding's content are preserved (not skipped).
// For content_position end: use trailing trim from modified doc (raw_trimmed.2)
// so comments before the next #-binding are excluded from this binding.
content_position = if trimmed_orig.0.is_empty() {
[
parsed_binding.1[0] + boundry_offset + 1,
parsed_binding.1[1],
]
} else {
[
parsed_binding.1[0] + boundry_offset + 1 + trimmed_content.1,
parsed_binding.1[1] - trimmed_content.2,
parsed_binding.1[0] + boundry_offset + 1 + trimmed_orig.1,
parsed_binding.1[1] - raw_trimmed.2,
]
};
content = if trimmed_content.0.is_empty() {
slices.1.to_owned()
} else {
trimmed_content.0.to_owned()
};
content = self
.text
.get(content_position[0]..content_position[1])
.unwrap_or("")
.to_owned();
Comment on lines 798 to +808

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all usages of content_position to assess underflow/slice-panic risk
rg -nP --type=rust 'content_position' -C3
# Specifically subtraction or range slicing on content_position
rg -nP --type=rust 'content_position\s*\[\s*1\s*\]\s*-\s*.*content_position\s*\[\s*0\s*\]' -C2
rg -nP --type=rust 'content_position\[0\]\s*\.\.\s*.*content_position\[1\]' -C2

Repository: rainlanguage/dotrain

Length of output: 403

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the relevant code and all uses of `content_position`
FILE="crates/dotrain/src/parser/raindocument/logic.rs"
echo "== file outline =="
ast-grep outline "$FILE" --view expanded || true

echo
echo "== relevant lines =="
sed -n '780,825p' "$FILE" | cat -n

echo
echo "== all content_position references =="
rg -n --type=rust 'content_position' crates/dotrain/src -C 2 || true

echo
echo "== range/slice usage patterns around content_position =="
rg -n --type=rust 'content_position\s*\[[01]\]' crates/dotrain/src -C 2 || true

echo
echo "== nearby binding fields =="
rg -n --type=rust 'binding.*content_position|content_position.*binding' crates/dotrain/src -C 2 || true

Repository: rainlanguage/dotrain

Length of output: 24199

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="crates/dotrain/src/parser/raindocument/logic.rs"

echo "== tracked_trim / related helpers =="
rg -n --type=rust 'fn tracked_trim|tracked_trim\(' "$FILE" -C 4 || true

echo
echo "== process_binding body around the range calculation =="
sed -n '758,860p' "$FILE" | cat -n

echo
echo "== AST for Binding and Offsets =="
sed -n '220,250p' crates/dotrain/src/types/ast.rs | cat -n

echo
echo "== any tests around comment-only or whitespace-only bindings =="
rg -n --type=rust 'comment-only|whitespace-only|content_position extends|comment before|binding.*comment' crates/dotrain/src/parser/raindocument/mod.rs -C 2 || true

Repository: rainlanguage/dotrain

Length of output: 8244

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="crates/dotrain/src/parser/raindocument/logic.rs"

echo "== tracked_trim definition =="
ast-grep outline "$FILE" --view expanded | sed -n '1,120p' || true

echo
echo "== exact tracked_trim / trim helper block =="
rg -n --type=rust 'tracked_trim|fn .*trim' "$FILE" -C 6 || true

echo
echo "== comment-related test block =="
sed -n '940,990p' crates/dotrain/src/parser/raindocument/mod.rs | cat -n

echo
echo "== other content_position consumers in parser/raindocument =="
rg -n --type=rust 'content_position' crates/dotrain/src/parser/raindocument -C 2 || true

Repository: rainlanguage/dotrain

Length of output: 19722

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== tracked_trim definition across repo =="
rg -n --type=rust 'fn tracked_trim|tracked_trim\(' crates/dotrain/src -C 4 || true

echo
echo "== bindings/tests that mention comment-only or empty binding =="
rg -n --type=rust 'InvalidEmptyBinding|comment-only|comment before|extends through the comment|content_position extends' crates/dotrain/src -C 3 || true

echo
echo "== any direct use of content_position as a start/end offset in composer/parser =="
rg -n --type=rust 'content_position\[[01]\]' crates/dotrain/src -C 2 || true

Repository: rainlanguage/dotrain

Length of output: 11446

Clamp content_position before storing it

Bindings whose content is only comments after leading whitespace can still end up with start > end: trimmed_orig preserves the comment text, while raw_trimmed.2 trims the comment-replaced slice back to the start of the content. The local .get(...).unwrap_or("") hides it here, but content_position is still stored as a malformed range.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/dotrain/src/parser/raindocument/logic.rs` around lines 798 - 808, The
malformed range is being preserved in the `content_position` value even though
`self.text.get(...)` masks it locally. Update the range calculation in
`logic.rs` where `content_position` is derived in this branch so it is
clamped/normalized before assignment, ensuring `start <= end` for comment-only
bindings after whitespace trimming. Keep the fix scoped to the
`content_position` computation used just before `content` is read, and use the
existing `parsed_binding`, `trimmed_orig`, and `raw_trimmed` values to adjust
the bounds consistently.

} else {
name = parsed_binding.0.clone();
name_position = parsed_binding.1;
Expand Down
44 changes: 44 additions & 0 deletions crates/dotrain/src/parser/raindocument/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,4 +947,48 @@ _: opcode-1(0xabcd 456);
assert_eq!(inner.hash, inner_hash_hex);
assert_eq!(inner.unwrap_constant_binding(), "0x1111");
}

// Mutation-validated: revert the process_binding fix (restore content_text path) and
// this test fails because b1's content_position extends through the comment.
#[test]
fn test_comment_before_binding_excluded_from_previous_content_position() {
let store = Store::new();
let meta_store = Arc::new(RwLock::new(store));

// Comment before #b2 must not appear in b1's content or content_position.
let text = "---\n#b1\n! elided\n\n/* comment for b2 */\n#b2\n! elided2\n";
let rain_document =
RainDocument::create(text.to_owned(), Some(meta_store.clone()), None, None);

let b1 = rain_document
.bindings()
.iter()
.find(|b| b.name == "b1")
.expect("binding b1 not found");

// b1 content must be the elision text only, not including the comment.
assert_eq!(b1.content, "! elided");

// b1 content_position must end at the last char of "! elided", not at the comment.
let content_end = b1.content_position[1];
let text_at_end = text.get(b1.content_position[0]..content_end).unwrap();
assert_eq!(text_at_end, "! elided");

// The comment text must not appear anywhere inside b1's content_position range.
let b1_content_range = text
.get(b1.content_position[0]..b1.content_position[1])
.unwrap();
assert!(
!b1_content_range.contains("/*"),
"comment leaked into b1 content_position: {b1_content_range:?}"
);

// b2 must also parse correctly.
let b2 = rain_document
.bindings()
.iter()
.find(|b| b.name == "b2")
.expect("binding b2 not found");
assert_eq!(b2.content, "! elided2");
}
Comment on lines +953 to +993

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Solid regression test for the exclusion path; consider covering the complementary cases.

This validates that a trailing comment before #b2 is excluded from b1. The PR also introduces an asymmetric rule where a comment at the start of a binding's own content is preserved (start bound uses original-text trim). That behavior, plus the comment-only binding edge case flagged in logic.rs, is currently untested. Adding cases like #b1\n/* doc */\n_: foo();\n#b2 (leading comment retained) and #b1\n\n/* note */\n#b2 (comment-only b1) would lock down the new positional contract.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/dotrain/src/parser/raindocument/mod.rs` around lines 953 - 993, The
new regression test only covers exclusion of a comment before the next binding,
but the positional contract also needs coverage for the complementary cases.
Extend the tests around RainDocument::create and binding parsing to verify that
a comment at the start of a binding’s own content is preserved in that binding’s
content/content_position, and add an edge case for a comment-only binding to
confirm it is handled correctly per the logic in the parser. Use the existing
test in raindocument/mod.rs as the location to add these cases and align
expectations with the behavior introduced in the binding/content extraction
logic.

}
Loading