Repository: softdevteam/grmtools (language: rust)

Issue:
What should the return types of Lexeme::end() and Lexeme::len() be?
Currently these two functions have return types `Option<usize>` where `None` means "this lexeme was inserted by error recovery". In a sense this seems like the right thing to do, in that it stops users accidentally thinking they've got a "normal" lexeme. But in practise I'm starting to be less sure about it because it seems that I always end up doing `l.end().unwrap_or_else(|| l.start())`.

Indeed, if I look in lrlex/lrpar I always end up doing that:

```
$ rg end\\\(\\\) lrlex lrpar
lrlex/src/main.rs
74:            &input[l.start()..l.end().unwrap_or_else(|| l.start())]

lrlex/src/lib/lexer.rs
203:                    let len = m.end();
294:        &self.s[st..l.end().unwrap_or(st)]

lrlex/src/lib/parser.rs
113:        let line = self.src[i..i + line_len].trim_end();
141:        let re_str = line[..rspace].trim_end().to_string();

lrpar/src/lib/parser.rs
61:                    let lt = &input[lexeme.start()..lexeme.end().unwrap_or_else(|| lexeme.start())];
322:                    span.push(la_lexeme.end().unwrap_or_else(|| la_lexeme.start()));
444:                            span_uw.push(la_lexeme.end().unwrap_or_else(|| la_lexeme.start()));
976:                    let len = m.end();

lrpar/examples/calc_parsetree/src/main.rs
89:                        self.s[lexeme.start()..lexeme.end().unwrap_or_else(|| lexeme.start())]
```

This is making me wonder if we've been too precise for our own good. But I'm really not sure: I really can see people shooting themselves in the foot if they've forgotten that some tokens can be inserted as the result of error recover. Thoughts are appreciated!


Discussion / hints:
Should have been clear that I'm asking @ptersilie for thoughts.
Does `l.end().unwrap_or_else(|| l.start())` just return `l.start()`, which then makes `&input[` return an empty string?

Maybe we can add a method `l.was_inserted_by_error_recovery()` that allows users to easily check if a lexeme is real or not. Or we add a function `l.get_from_input(&input)`, which returns `""` if the lexeme was inserted by error recovery, and then tell users to use that instead of indexing the input themselves?
The `inserted` function allows you to tell if a lexeme was inserted by error recovery or not. My worry is that people will forget to test for that, then they'll do something like `lexeme.end()` and be surprised that they end up with a zero length lexeme (for a lexeme definition that seemingly could never be zero length). I'm just not sure if I'm being unduly paranoid or not.
I think we can't reasonably worry about everything the user might forget, especially if that means we make the life of less forgetful users more difficult by forcing them to use workarounds like `l.end().unwrap_or_else(|| l.start())`. I rather have `end/start` return 0 if the lexeme was inserted. I'm sure users will quickly remember that these are inserted lexemes when they get zero length ones.
OK, I'll raise a PR.

Fix patch:
diff --git a/lrlex/src/lib/lexer.rs b/lrlex/src/lib/lexer.rs
index 097500224..ae0bf907c 100644
--- a/lrlex/src/lib/lexer.rs
+++ b/lrlex/src/lib/lexer.rs
@@ -290,8 +290,7 @@ impl<'a, StorageT: Copy + Eq + Hash + PrimInt + Unsigned> Lexer<StorageT>
     }
 
     fn lexeme_str(&self, l: &Lexeme<StorageT>) -> &str {
-        let st = l.start();
-        &self.s[st..l.end().unwrap_or(st)]
+        &self.s[l.start()..l.end()]
     }
 }
 
@@ -321,11 +320,11 @@ mod test {
         let lex1 = lexemes[0];
         assert_eq!(lex1.tok_id(), 1u8);
         assert_eq!(lex1.start(), 0);
-        assert_eq!(lex1.len(), Some(3));
+        assert_eq!(lex1.len(), 3);
         let lex2 = lexemes[1];
         assert_eq!(lex2.tok_id(), 0);
         assert_eq!(lex2.start(), 4);
-        assert_eq!(lex2.len(), Some(3));
+        assert_eq!(lex2.len(), 3);
     }
 
     #[test]
@@ -361,11 +360,11 @@ if 'IF'
         let lex1 = lexemes[0];
         assert_eq!(lex1.tok_id(), 1u8);
         assert_eq!(lex1.start(), 0);
-        assert_eq!(lex1.len(), Some(3));
+        assert_eq!(lex1.len(), 3);
         let lex2 = lexemes[1];
         assert_eq!(lex2.tok_id(), 0);
         assert_eq!(lex2.start(), 4);
-        assert_eq!(lex2.len(), Some(2));
+        assert_eq!(lex2.len(), 2);
     }
 
     #[test]
@@ -384,15 +383,15 @@ if 'IF'
         assert_eq!(lexemes.len(), 3);
         let lex1 = lexemes[0];
         assert_eq!(lex1.start(), 0);
-        assert_eq!(lex1.len(), Some(1));
+        assert_eq!(lex1.len(), 1);
         assert_eq!(lexer.lexeme_str(&lex1), "a");
         let lex2 = lexemes[1];
         assert_eq!(lex2.start(), 2);
-        assert_eq!(lex2.len(), Some(3));
+        assert_eq!(lex2.len(), 3);
         assert_eq!(lexer.lexeme_str(&lex2), "❤");
         let lex3 = lexemes[2];
         assert_eq!(lex3.start(), 6);
-        assert_eq!(lex3.len(), Some(1));
+        assert_eq!(lex3.len(), 1);
         assert_eq!(lexer.lexeme_str(&lex3), "a");
     }
 
diff --git a/lrlex/src/main.rs b/lrlex/src/main.rs
index 46100294d..99bbb8a03 100644
--- a/lrlex/src/main.rs
+++ b/lrlex/src/main.rs
@@ -71,7 +71,7 @@ fn main() {
         println!(
             "{} {}",
             lexerdef.get_rule_by_id(l.tok_id()).name.as_ref().unwrap(),
-            &input[l.start()..l.end().unwrap_or_else(|| l.start())]
+            &input[l.start()..l.end()]
         );
     }
 }
diff --git a/lrpar/examples/calc_parsetree/src/main.rs b/lrpar/examples/calc_parsetree/src/main.rs
index c580eb892..9eb7d0057 100644
--- a/lrpar/examples/calc_parsetree/src/main.rs
+++ b/lrpar/examples/calc_parsetree/src/main.rs
@@ -86,9 +86,7 @@ impl<'a> Eval<'a> {
             } if ridx == calc_y::R_FACTOR => {
                 if nodes.len() == 1 {
                     if let Node::Term { lexeme } = nodes[0] {
-                        self.s[lexeme.start()..lexeme.end().unwrap_or_else(|| lexeme.start())]
-                            .parse()
-                            .unwrap()
+                        self.s[lexeme.start()..lexeme.end()].parse().unwrap()
                     } else {
                         unreachable!();
                     }
diff --git a/lrpar/src/lib/ctbuilder.rs b/lrpar/src/lib/ctbuilder.rs
index 1db826ed5..18244b986 100644
--- a/lrpar/src/lib/ctbuilder.rs
+++ b/lrpar/src/lib/ctbuilder.rs
@@ -636,10 +636,10 @@ where
                             "
         let {prefix}arg_{} = match {prefix}args.next().unwrap() {{
             AStackType::Lexeme(l) => {{
-                if l.len().is_some() {{
-                    Ok(l)
-                }} else {{
+                if l.inserted() {{
                     Err(l)
+                }} else {{
+                    Ok(l)
                 }}
             }},
             AStackType::ActionType(_) => unreachable!()
diff --git a/lrpar/src/lib/lex.rs b/lrpar/src/lib/lex.rs
index c2622c8dc..61eea071b 100644
--- a/lrpar/src/lib/lex.rs
+++ b/lrpar/src/lib/lex.rs
@@ -101,23 +101,26 @@ impl<StorageT: Copy> Lexeme<StorageT> {
         self.start
     }
 
-    /// Byte offset of the end of the lexeme, or `None` if this lexeme is the result of error
-    /// recovery.
-    pub fn end(&self) -> Option<usize> {
-        if let Some(l) = self.len() {
-            Some(self.start() + l)
+    /// Byte offset of the end of the lexeme.
+    ///
+    /// Note that if this lexeme was inserted by error recovery, it will end at the same place it
+    /// started (i.e. `self.start() == self.end()).
+    pub fn end(&self) -> usize {
+        if self.len == u32::max_value() {
+            self.start
         } else {
-            None
+            self.start + (self.len as usize)
         }
     }
 
-    /// Length in bytes of the lexeme, or `None` if this lexeme is the result of error recovery.
-    pub fn len(&self) -> Option<usize> {
+    /// Length in bytes of the lexeme. Note that if this lexeme was inserted by error recovery, it
+    /// will have a length of 0.
+    pub fn len(&self) -> usize {
         if self.len == u32::max_value() {
-            None
+            0
         } else {
             const_assert!(size_of::<usize>() >= size_of::<u32>());
-            Some(self.len as usize)
+            self.len as usize
         }
     }
 
diff --git a/lrpar/src/lib/parser.rs b/lrpar/src/lib/parser.rs
index f8218e1d6..cf9a41a0d 100644
--- a/lrpar/src/lib/parser.rs
+++ b/lrpar/src/lib/parser.rs
@@ -58,7 +58,7 @@ where
                 Node::Term { lexeme } => {
                     let tidx = TIdx(lexeme.tok_id());
                     let tn = grm.token_name(tidx).unwrap();
-                    let lt = &input[lexeme.start()..lexeme.end().unwrap_or_else(|| lexeme.start())];
+                    let lt = &input[lexeme.start()..lexeme.end()];
                     s.push_str(&format!("{} {}\n", tn, lt));
                 }
                 Node::Nonterm { ridx, ref nodes } => {
@@ -319,7 +319,7 @@ where
                     let la_lexeme = self.next_lexeme(laidx);
                     pstack.push(state_id);
                     astack.push(AStackType::Lexeme(la_lexeme));
-                    span.push(la_lexeme.end().unwrap_or_else(|| la_lexeme.start()));
+                    span.push(la_lexeme.end());
                     laidx += 1;
                 }
                 Action::Accept => {
@@ -441,7 +441,7 @@ where
                                 self.next_lexeme(laidx)
                             };
                             astack_uw.push(AStackType::Lexeme(la_lexeme));
-                            span_uw.push(la_lexeme.end().unwrap_or_else(|| la_lexeme.start()));
+                            span_uw.push(la_lexeme.end());
                         }
                     }
                     pstack.push(state_id);
@@ -472,7 +472,7 @@ where
             } else {
                 debug_assert!(laidx > 0);
                 let last_la = self.lexemes[laidx - 1];
-                last_la.start() + last_la.len().unwrap_or(0)
+                last_la.end()
             };
 
             Lexeme::new(


Test patch: