Merge pull request #258 from nikomatsakis/issue-253

error out with extra tokens, even in parse table mode

.gitignore

@@ -36,4 +36,8 @@ lalrpop-test/src/sub_table.rs
 lalrpop-test/src/unit.rs
 lalrpop-test/src/use_super.rs
 lalrpop-test/src/no_clone_tok.rs
+lalrpop-test/src/partial_parse.rs
 lalrpop-test/src/match_section.rs
+lalrpop-test/src/expr_module_attributes.rs
+
+

lalrpop-test/src/main.rs

@@ -25,6 +25,7 @@ mod expr_lalr;
 mod expr_intern_tok;
 
 /// tests #![attributes] for generated module
+#[allow(dead_code, unknown_lints)]
 mod expr_module_attributes;
 
 /// test that passes in lifetime/type/formal parameters and threads
@@ -83,6 +84,9 @@ mod unit;
 /// test for match section
 mod match_section;
 
+/// regression test for issue #253.
+mod partial_parse;
+
 // Check that error recovery (which requires cloneable tokens) is not created if it is not used
 #[allow(unused)]
 mod no_clone_tok;
@@ -437,3 +441,8 @@ fn test_match_section() {
 fn issue_113() {
     assert!(error_issue_113::parse_Items("+").is_err());
 }
+
+#[test]
+fn issue_253() {
+    assert!(partial_parse::parse_Term("(22))").is_err());
+}

lalrpop-test/src/partial_parse.lalrpop

@@ -0,0 +1,14 @@
+// From issue #253 -- this grammar is not that important, the key
+// thing is that when we parse we expect to get an error because the
+// entire input is not consumed.
+
+use std::str::FromStr;
+
+grammar;
+
+pub Term: i32 = {
+    <n:Num> => n,
+    "(" <t:Term> ")" => t,
+};
+
+Num: i32 = <s:r"[0-9]+"> => i32::from_str(s).unwrap();

lalrpop/src/lr1/codegen/parse_table.rs

@@ -103,7 +103,9 @@ pub fn compile<'grammar, W: Write>(grammar: &'grammar Grammar,
 //                symbols.push(symbol);
 //                continue 'shift;
 //            } else if action < 0 { // reduce
-//                try!(reduce(action, Some(&lookahead.0), &mut states, &mut symbols));
+//                if let Some(_) = reduce(action, Some(&lookahead.0), &mut states, &mut symbols) {
+//                    return Err(lalrpop_util::ParseError::ExtraToken { token: lookahead });
+//                }
 //            } else {
 //                try_error_recovery(...)?;
 //            }
@@ -486,7 +488,7 @@ impl<'ascent, 'grammar, W: Write> CodeGenerator<'ascent, 'grammar, W, TableDrive
             rust!(self.out, "println!(\"--> reduce\");");
         }
         rust!(self.out,
-              "if let Some(r) = {}reduce({}{}action, Some(&{}lookahead.0), &mut {}states, &mut \
+              "if let Some(_) = {}reduce({}{}action, Some(&{}lookahead.0), &mut {}states, &mut \
                {}symbols, {}) {{",
               self.prefix,
               self.grammar.user_parameter_refs(),
@@ -495,7 +497,10 @@ impl<'ascent, 'grammar, W: Write> CodeGenerator<'ascent, 'grammar, W, TableDrive
               self.prefix,
               self.prefix,
               phantom_data_expr);
-        rust!(self.out, "return r;");
+        rust!(self.out,
+              "return Err({}lalrpop_util::ParseError::ExtraToken {{ token: {}lookahead }});",
+              self.prefix,
+              self.prefix);
         rust!(self.out, "}}");
 
         // Error.
@@ -1069,17 +1074,16 @@ impl<'ascent, 'grammar, W: Write> CodeGenerator<'ascent, 'grammar, W, TableDrive
         rust!(self.out, "break;");
         rust!(self.out, "}}");
         rust!(self.out, "{}states.pop();", self.prefix);
+        if DEBUG_PRINT {
+            rust!(self.out, "println!(\"Dropping state: {{}}\", {}state);",
+                self.prefix);
+        }
         rust!(self.out, "}}"); // Some
         rust!(self.out, "None => {{");
         rust!(self.out, "return Err({}error);", self.prefix);
         rust!(self.out, "}}");
         rust!(self.out, "}}"); // match
 
-        if DEBUG_PRINT {
-            rust!(self.out, "println!(\"Dropping state: {{}}\", {}state);",
-                self.prefix);
-        }
-
         rust!(self.out, "}}"); // loop
 
         Ok(())

lalrpop/src/lr1/lane_table/construct/mod.rs

@@ -114,9 +114,25 @@ impl<'grammar> LaneTableConstruct<'grammar> {
         debug!("resolve_inconsistencies(inconsistent_state={:?}/{:#?}",
                inconsistent_state, states[inconsistent_state.0]);
 
-        let actions = super::conflicting_actions(&states[inconsistent_state.0]);
+        let mut actions = super::conflicting_actions(&states[inconsistent_state.0]);
         if actions.is_empty() {
-            return Ok(());
+            // This can mean one of two things: only shifts, or a
+            // single reduction. We have to be careful about states
+            // with a single reduction: even though such a state is
+            // not inconsistent (there is only one possible course of
+            // action), we still want to run the lane table algorithm,
+            // because otherwise we get states with "complete"
+            // lookahead, which messes with error recovery.
+            //
+            // In particular, if there is too much lookahead, we will
+            // reduce even when it is inappropriate to do so.
+            actions = states[inconsistent_state.0].reductions
+                                                  .iter()
+                                                  .map(|&(_, prod)| Action::Reduce(prod))
+                                                  .collect();
+            if actions.is_empty() {
+                return Ok(());
+            }
         }
 
         debug!("resolve_inconsistencies: conflicting_actions={:?}", actions);
@@ -168,6 +184,7 @@ impl<'grammar> LaneTableConstruct<'grammar> {
             Ok(columns) => {
                 debug!("attempt_lalr, columns={:#?}", columns);
                 columns.apply(state, actions);
+                debug!("attempt_lalr, state={:#?}", state);
                 true
             }
             Err(OverlappingLookahead) => {