Skip to content

Commit 58b2e50

Browse files
mpecanclaude
andauthored
fix(lexer): compute span ends by character count, not byte length (#72)
## Summary The lexer scans the source as a `Vec<char>`, so token positions and AST `Span`s are **character indices**. But span *ends* were computed as `pos + value.len()` — adding the token value's **byte** length to a **character** position. For any token containing multibyte UTF-8, the end overshoots by the byte/char delta, so `Node::source_text` returns the wrong slice for every **non-final** token. The existing `source_text_multibyte_utf8` test never caught this because its multibyte word was the *last* token, where the overshoot clamps harmlessly to the source end. ### Example (before) ``` echo "✓ valid" || echo "✗ bad" Command 0..16 source_text = `echo "✓ valid" |` ← wrong (bleeds into the `||`) ``` ### After ``` Command 0..14 source_text = `echo "✓ valid"` ← correct ``` ## The fix Use `value.chars().count()` at the four sites that derive an end offset from a start position: - `lexer/mod.rs` — `last_token_end` (the source of most node ends via `parser/mod.rs:134`) - `parser/simple_command.rs` — word-node span - `parser/case_select.rs` — pattern word span - `token.rs` — `Token::adjacent_to` …and correct the `Span` doc comment to state offsets are character indices. ## Downstream impact Surfaced as [tokf #383](mpecan/tokf#383): compound shell commands mixing an emoji/`✓`/`✗` `echo` with a rewritten pipe were spliced into invalid shell (`| ||`, dropped `; `). tokf consumes these spans to split compound commands. ## Tests Adds regression tests for multibyte in **non-final** words, compound segments, pipelines, and astral-plane emoji (`🎉`). Full suite: **262 passing**, clippy + fmt clean. No ASCII spans change (char count == byte len for ASCII), so no existing test moved. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 411dd22 commit 58b2e50

6 files changed

Lines changed: 107 additions & 6 deletions

File tree

src/ast.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
use crate::lexer::word_builder::{QuotingContext, WordSpanKind};
22

3-
/// Source span representing a byte range in the original input.
3+
/// Source span representing a **character** range in the original input.
4+
///
5+
/// Offsets are character indices (the lexer scans the source as a `Vec<char>`),
6+
/// not byte offsets — they diverge for multibyte UTF-8. Use
7+
/// [`Node::source_text`] to recover the source slice; it converts these
8+
/// character indices to byte offsets before slicing.
49
#[derive(Debug, Clone, PartialEq, Eq)]
510
pub struct Span {
611
pub start: usize,

src/lexer/mod.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,11 @@ impl Lexer {
317317
} else {
318318
self.read_token()?
319319
};
320-
self.last_token_end = tok.pos + tok.value.len();
320+
// `pos` is a character index (the lexer scans a `Vec<char>`), so the
321+
// span end must advance by the token's character count, not its byte
322+
// length. Using `value.len()` (bytes) corrupts spans for any token
323+
// containing multibyte UTF-8 (tokf #383).
324+
self.last_token_end = tok.pos + tok.value.chars().count();
321325
Ok(tok)
322326
}
323327

src/parser/case_select.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ impl Parser {
1919
value: word_tok.value.clone(),
2020
spans: word_tok.spans,
2121
},
22-
Span::new(word_tok.pos, word_tok.pos + word_tok.value.len()),
22+
// Char count, not byte length: `pos` is a character index (#383).
23+
Span::new(word_tok.pos, word_tok.pos + word_tok.value.chars().count()),
2324
));
2425

2526
self.lexer.set_command_start();

src/parser/simple_command.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,9 @@ impl Parser {
146146
/// which uses `Node::empty` (no span) for AST constructors where positional
147147
/// information is not available.
148148
fn build_word_node(tok: Token) -> Node {
149-
let word_span = Span::new(tok.pos, tok.pos + tok.value.len());
149+
// `pos` is a character index; advance the span end by the token's char
150+
// count, not its byte length, so multibyte words span correctly (#383).
151+
let word_span = Span::new(tok.pos, tok.pos + tok.value.chars().count());
150152
let parts = word_parts::decompose_word_with_spans(&tok.value, &tok.spans);
151153
Node::new(
152154
NodeKind::Word {

src/parser/tests.rs

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,30 @@ fn parse(source: &str) -> Vec<Node> {
88
parser.parse_all().unwrap()
99
}
1010

11+
/// Flatten a (possibly nested) `List`/`Pipeline` into its leaf command nodes,
12+
/// in source order. Test helper for span-boundary assertions.
13+
fn collect_leaf_commands<'a>(node: &'a Node, out: &mut Vec<&'a Node>) {
14+
match &node.kind {
15+
NodeKind::List { items } => {
16+
for item in items {
17+
collect_leaf_commands(&item.command, out);
18+
}
19+
}
20+
NodeKind::Pipeline { commands, .. } => {
21+
for cmd in commands {
22+
collect_leaf_commands(cmd, out);
23+
}
24+
}
25+
_ => out.push(node),
26+
}
27+
}
28+
29+
/// Slice `source` by **character** offsets, mirroring how `Node::source_text`
30+
/// resolves spans. Test helper for asserting inter-span gaps.
31+
fn char_slice(source: &str, start: usize, end: usize) -> String {
32+
source.chars().skip(start).take(end - start).collect()
33+
}
34+
1135
#[test]
1236
fn simple_command() {
1337
let nodes = parse("echo hello");
@@ -401,6 +425,67 @@ fn source_text_multibyte_utf8() {
401425
assert_eq!(words[1].source_text(source), "café");
402426
}
403427

428+
#[test]
429+
fn source_text_multibyte_non_last_word() {
430+
// Regression (tokf #383): a multibyte word that is NOT the last token.
431+
// Before the char-count span fix, the word's span end was computed as
432+
// `pos + byte_len`, overshooting into the following token — here `café`
433+
// would bleed into ` bar`. The previous `source_text_multibyte_utf8`
434+
// test missed this because its multibyte word was last, so the
435+
// overshooting span clamped harmlessly to the source end.
436+
let source = "echo café bar";
437+
let nodes = parse(source);
438+
assert_eq!(nodes[0].source_text(source), "echo café bar");
439+
let NodeKind::Command { words, .. } = &nodes[0].kind else {
440+
unreachable!("expected Command");
441+
};
442+
assert_eq!(words[0].source_text(source), "echo");
443+
assert_eq!(words[1].source_text(source), "café");
444+
assert_eq!(words[2].source_text(source), "bar");
445+
}
446+
447+
#[test]
448+
fn source_text_multibyte_compound_segments() {
449+
// Regression (tokf #383): multibyte in non-final segments of a compound
450+
// list. Each leaf command's source_text must be exact, and the spans
451+
// must leave the operator gaps intact for a consumer that slices
452+
// `source[prev.span.end .. next.span.start]` as the separator.
453+
let source = "echo \"✓ ok\" || echo \"✗ no\"; ls";
454+
let nodes = parse(source);
455+
let mut leaves = Vec::new();
456+
collect_leaf_commands(&nodes[0], &mut leaves);
457+
let texts: Vec<&str> = leaves.iter().map(|n| n.source_text(source)).collect();
458+
assert_eq!(texts, vec!["echo \"✓ ok\"", "echo \"✗ no\"", "ls"]);
459+
// The gap between the first two leaves must be exactly the `||` operator
460+
// (converting the character spans to byte offsets, as source_text does).
461+
let gap = char_slice(source, leaves[0].span.end, leaves[1].span.start);
462+
assert_eq!(gap, " || ");
463+
}
464+
465+
#[test]
466+
fn source_text_multibyte_pipeline() {
467+
// Regression (tokf #383): multibyte in the left command of a pipeline.
468+
let source = "echo \"\" | grep x";
469+
let nodes = parse(source);
470+
let NodeKind::Pipeline { commands, .. } = &nodes[0].kind else {
471+
unreachable!("expected Pipeline");
472+
};
473+
assert_eq!(commands[0].source_text(source), "echo \"\"");
474+
assert_eq!(commands[1].source_text(source), "grep x");
475+
}
476+
477+
#[test]
478+
fn source_text_astral_plane_emoji() {
479+
// Regression (tokf #383): a 4-byte (astral-plane) char before a newline
480+
// separator. Previously `echo 🎉` overshot its span and the second
481+
// command's slice underflowed, panicking on a non-char-boundary index.
482+
let source = "echo 🎉\nls";
483+
let nodes = parse(source);
484+
assert_eq!(nodes.len(), 2);
485+
assert_eq!(nodes[0].source_text(source), "echo 🎉");
486+
assert_eq!(nodes[1].source_text(source), "ls");
487+
}
488+
404489
#[test]
405490
fn source_text_if_compound() {
406491
let source = "if true; then echo yes; fi";

src/token.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,12 @@ impl Token {
183183
}
184184

185185
/// Returns true if this token is immediately adjacent to `other` (no whitespace).
186-
pub const fn adjacent_to(&self, other: &Self) -> bool {
187-
self.pos + self.value.len() == other.pos
186+
///
187+
/// `pos` is a character index, so adjacency is measured in characters: the
188+
/// token's char count, not its byte length (which diverges for multibyte
189+
/// UTF-8 — tokf #383).
190+
pub fn adjacent_to(&self, other: &Self) -> bool {
191+
self.pos + self.value.chars().count() == other.pos
188192
}
189193

190194
pub const fn eof(pos: usize, line: usize) -> Self {

0 commit comments

Comments
 (0)