Improve parser error placement

crates/analyzer/src/traversal/types.rs

@@ -575,7 +575,6 @@ pub fn type_desc(
             if let Some(val) = self_type {
                 Ok(Type::SelfType(val).id(context.db()))
             } else {
-                dbg!("Reporting error");
                 Err(TypeError::new(context.error(
                     "`Self` can not be used here",
                     desc.span,

crates/parser2/src/parser/attr.rs

@@ -4,7 +4,7 @@ use crate::SyntaxKind;
 
 pub(super) fn parse_attr_list<S: TokenStream>(parser: &mut Parser<S>) -> Option<Checkpoint> {
     if let Some(SyntaxKind::DocComment) | Some(SyntaxKind::Pound) = parser.current_kind() {
-        Some(parser.parse(super::attr::AttrListScope::default(), None).1)
+        Some(parser.parse(AttrListScope::default(), None).1)
     } else {
         None
     }

crates/parser2/src/parser/expr_atom.rs

@@ -105,21 +105,21 @@ impl super::Parse for IfExprScope {
         parser.parse(BlockExprScope::default(), None);
 
         if parser.current_kind() == Some(SyntaxKind::ElseKw) {
-            parser.with_next_expected_tokens(
+            parser.bump_expected(SyntaxKind::ElseKw);
+
+            parser.with_recovery_tokens(
                 |parser| {
-                    parser.bump_expected(SyntaxKind::ElseKw);
+                    if matches!(
+                        parser.current_kind(),
+                        Some(SyntaxKind::LBrace | SyntaxKind::IfKw)
+                    ) {
+                        parse_expr(parser);
+                    } else {
+                        parser.error_and_recover("expected `{` or `if` after `else`", None);
+                    }
                 },
                 &[SyntaxKind::LBrace, SyntaxKind::IfKw],
             );
-
-            if !matches!(
-                parser.current_kind(),
-                Some(SyntaxKind::LBrace | SyntaxKind::IfKw)
-            ) {
-                parser.error_and_recover("expected `{` or `if` after `else`", None);
-                return;
-            }
-            parse_expr(parser);
         }
     }
 }

crates/parser2/src/parser/item.rs

@@ -279,7 +279,7 @@ impl super::Parse for VariantDefListScope {
             if !parser.bump_if(SyntaxKind::Comma)
                 && parser.current_kind() != Some(SyntaxKind::RBrace)
             {
-                parser.error_at_current_pos("expected comma after enum variant definition");
+                parser.error("expected comma after enum variant definition");
             }
         }
 
@@ -453,22 +453,20 @@ impl super::Parse for ConstScope {
 
         parser.with_next_expected_tokens(
             |parser| {
-                parser.bump_or_recover(
-                    SyntaxKind::Colon,
-                    "expected type annotation for `const`",
-                    None,
-                );
-                parse_type(parser, None);
+                if parser.bump_if(SyntaxKind::Colon) {
+                    parse_type(parser, None);
+                } else {
+                    parser.error_and_recover("expected type annotation for `const`", None);
+                }
             },
             &[SyntaxKind::Eq],
         );
 
-        if !parser.bump_if(SyntaxKind::Eq) {
+        if parser.bump_if(SyntaxKind::Eq) {
+            parse_expr(parser);
+        } else {
             parser.error_and_recover("expected `=` for const value definition", None);
-            return;
         }
-
-        parse_expr(parser);
     }
 }
 
@@ -485,7 +483,7 @@ impl super::Parse for ExternScope {
     }
 }
 
-define_scope! { ExternItemListScope, ExternItemList, Override(RBrace, FnKw) }
+define_scope! { ExternItemListScope, ExternItemList, Override(RBrace, PubKw, UnsafeKw, FnKw) }
 impl super::Parse for ExternItemListScope {
     fn parse<S: TokenStream>(&mut self, parser: &mut Parser<S>) {
         parse_fn_item_block(parser, true, FuncDefScope::Extern);
@@ -543,25 +541,30 @@ fn parse_fn_item_block<S: TokenStream>(
         }
 
         let mut checkpoint = attr::parse_attr_list(parser);
-        let modifier_scope = ItemModifierScope::default();
-        match parser.current_kind() {
-            Some(kind) if kind.is_modifier_head() && allow_modifier => {
-                if allow_modifier {
-                    let (_, modifier_checkpoint) = parser.parse(modifier_scope, None);
-                    checkpoint.get_or_insert(modifier_checkpoint);
-                } else {
-                    parser.error_and_recover("modifier is not allowed in the block", checkpoint);
+
+        loop {
+            match parser.current_kind() {
+                Some(kind) if kind.is_modifier_head() => {
+                    if allow_modifier {
+                        let (_, modifier_checkpoint) =
+                            parser.parse(ItemModifierScope::default(), None);
+                        checkpoint.get_or_insert(modifier_checkpoint);
+                    } else {
+                        parser
+                            .unexpected_token_error("modifier is not allowed in this block", None);
+                    }
                 }
+                _ => break,
             }
-            _ => {}
         }
 
         match parser.current_kind() {
             Some(SyntaxKind::FnKw) => {
                 parser.parse(FuncScope::new(fn_def_scope), checkpoint);
             }
             _ => {
-                parser.error_and_recover("only `fn` is allowed in the block", checkpoint);
+                parser.error_msg_on_current_token("only `fn` is allowed in this block");
+                parser.recover(checkpoint);
             }
         }
 

crates/parser2/src/parser/mod.rs

@@ -149,7 +149,7 @@ impl<S: TokenStream> Parser<S> {
                 .iter()
                 .all(|token| *token != self.current_kind().unwrap())
         {
-            self.error_and_recover("unexpected token", None);
+            self.recover(None);
         }
 
         for token in expected_tokens {
@@ -231,10 +231,8 @@ impl<S: TokenStream> Parser<S> {
     /// * If checkpoint is `None`, the current branch is wrapped up by an error
     ///   node.
     pub fn error_and_recover(&mut self, msg: &str, checkpoint: Option<Checkpoint>) {
-        let err_scope = self.error(msg);
-        let checkpoint = self.enter(err_scope, checkpoint);
-        self.recover();
-        self.leave(checkpoint);
+        self.error(msg);
+        self.recover(checkpoint);
     }
 
     /// Add `msg` as an error to the error list, then bumps consecutive tokens
@@ -320,8 +318,9 @@ impl<S: TokenStream> Parser<S> {
         }
     }
 
-    /// Proceeds the parser to the recovery token of the current scope.
-    pub fn recover(&mut self) {
+    /// Consumes tokens until a recovery token is found, and reports an error on any
+    /// unexpexted tokens.
+    pub fn recover(&mut self, checkpoint: Option<Checkpoint>) {
         let mut recovery_set: FxHashSet<SyntaxKind> = fxhash::FxHashSet::default();
         let mut scope_index = self.parents.len() - 1;
         loop {
@@ -346,8 +345,10 @@ impl<S: TokenStream> Parser<S> {
             }
         }
 
-        let is_newline_trivia = self.set_newline_as_trivia(false);
-        self.auxiliary_recovery_set.insert(SyntaxKind::Newline, 1);
+        if !self.is_newline_trivia {
+            self.add_recovery_token(SyntaxKind::Newline);
+        }
+        let mut unexpected = None;
         let mut open_brackets_in_error = FxHashMap::default();
         while let Some(kind) = self.current_kind() {
             if kind.is_open_bracket_kind() {
@@ -364,11 +365,35 @@ impl<S: TokenStream> Parser<S> {
                     break;
                 }
             }
-
+            if unexpected.is_none() {
+                if !self.parents.is_empty() {
+                    self.bump_trivias();
+                }
+                unexpected = Some((
+                    self.current_pos,
+                    checkpoint.unwrap_or_else(|| self.checkpoint()),
+                ));
+            }
             self.bump();
         }
 
-        self.set_newline_as_trivia(is_newline_trivia);
+        if !self.is_newline_trivia {
+            self.remove_recovery_token(SyntaxKind::Newline);
+        }
+
+        if let Some((start_pos, checkpoint)) = unexpected {
+            self.is_err = true;
+            if !self.is_dry_run() {
+                self.builder
+                    .start_node_at(checkpoint, SyntaxKind::Error.into());
+                self.builder.finish_node();
+
+                self.errors.push(ParseError {
+                    range: TextRange::new(start_pos, self.current_pos),
+                    msg: "unexpected syntax".to_string(),
+                });
+            }
+        }
     }
 
     /// Bumps the current token if the current token is the `expected` kind.
@@ -381,7 +406,8 @@ impl<S: TokenStream> Parser<S> {
         checkpoint: Option<Checkpoint>,
     ) {
         if !self.bump_if(expected) {
-            self.error_and_recover(msg, checkpoint);
+            self.error(msg);
+            self.recover(checkpoint);
         }
     }
 
@@ -444,8 +470,20 @@ impl<S: TokenStream> Parser<S> {
         }
     }
 
-    /// Add the `msg` to the error list.
+    /// Add the `msg` to the error list, at `current_pos`.
     fn error(&mut self, msg: &str) -> ErrorScope {
+        self.is_err = true;
+        let pos = self.current_pos;
+        self.errors.push(ParseError {
+            range: TextRange::new(pos, pos),
+            msg: msg.to_string(),
+        });
+        ErrorScope::default()
+    }
+
+    /// Add the `msg` to the error list, on `current_token()`.
+    /// Bumps trivias.
+    fn error_msg_on_current_token(&mut self, msg: &str) {
         self.bump_trivias();
         self.is_err = true;
         let start = self.current_pos;
@@ -454,23 +492,26 @@ impl<S: TokenStream> Parser<S> {
         } else {
             start
         };
-        let range = TextRange::new(start, end);
 
         self.errors.push(ParseError {
-            range,
+            range: TextRange::new(start, end),
             msg: msg.to_string(),
         });
-        ErrorScope::default()
     }
 
-    fn error_at_current_pos(&mut self, msg: &str) -> ErrorScope {
-        let pos = self.current_pos;
-        let range = TextRange::new(pos, pos);
+    /// Wrap the current token in a `SyntaxKind::Error`, and add `msg` to the error list.
+    fn unexpected_token_error(&mut self, msg: &str, checkpoint: Option<Checkpoint>) {
+        let checkpoint = self.enter(ErrorScope::default(), checkpoint);
+
+        self.is_err = true;
+        let start_pos = self.current_pos;
+        self.bump();
+
         self.errors.push(ParseError {
-            range,
+            range: TextRange::new(start_pos, self.current_pos),
             msg: msg.to_string(),
         });
-        ErrorScope::default()
+        self.leave(checkpoint);
     }
 
     /// Returns `true` if the parser is in the dry run mode.

crates/parser2/src/parser/stmt.rs

@@ -61,11 +61,18 @@ impl super::Parse for ForStmtScope {
     fn parse<S: TokenStream>(&mut self, parser: &mut Parser<S>) {
         parser.bump_expected(SyntaxKind::ForKw);
 
-        parser.with_next_expected_tokens(parse_pat, &[SyntaxKind::InKw, SyntaxKind::LBrace]);
+        parser.with_recovery_tokens(parse_pat, &[SyntaxKind::InKw, SyntaxKind::LBrace]);
 
-        parser.bump_or_recover(SyntaxKind::InKw, "expected `in` keyword", None);
+        parser.with_next_expected_tokens(
+            |p| {
+                if !p.bump_if(SyntaxKind::InKw) {
+                    p.error("expected `in` keyword");
+                }
 
-        parser.with_next_expected_tokens(parse_expr_no_struct, &[SyntaxKind::LBrace]);
+                parse_expr_no_struct(p);
+            },
+            &[SyntaxKind::LBrace],
+        );
 
         if parser.current_kind() != Some(SyntaxKind::LBrace) {
             parser.error_and_recover("expected block", None);

crates/parser2/src/parser/struct_.rs

@@ -67,7 +67,7 @@ impl super::Parse for RecordFieldDefListScope {
             if !parser.bump_if(SyntaxKind::Comma)
                 && parser.current_kind() != Some(SyntaxKind::RBrace)
             {
-                parser.error_at_current_pos("expected comma after field definition");
+                parser.error("expected comma after field definition");
             }
         }
 
@@ -99,8 +99,8 @@ impl super::Parse for RecordFieldDefScope {
         // 2. We anticipate that this error would happen often in the transition period
         //    to Fe-V2.
         if parser.current_kind() == Some(SyntaxKind::FnKw) {
-            let err_scope = parser.error("function definition in struct is not allowed");
-            let checkpoint = parser.enter(err_scope, None);
+            parser.error_msg_on_current_token("function definition in struct is not allowed");
+            let checkpoint = parser.enter(super::ErrorScope::new(), None);
             parser.parse(FuncScope::default(), None);
             parser.leave(checkpoint);
             return;

crates/parser2/src/parser/type_.rs

@@ -89,7 +89,11 @@ impl super::Parse for TupleTypeScope {
         }
 
         if !parser.bump_if(SyntaxKind::RParen) {
+            // If the close paren is missing, we want to recover on a newline,
+            // so we have to explicitly set newlines to be non-trivia.
+            parser.set_newline_as_trivia(false);
             parser.error_and_recover("expected `)`", None);
+            parser.set_newline_as_trivia(true);
             parser.bump_if(SyntaxKind::RParen);
         }
     }

crates/parser2/src/parser/use_tree.rs

@@ -23,7 +23,7 @@ impl super::Parse for UseTreeScope {
 
         if parser.current_kind() == Some(SyntaxKind::AsKw) {
             if is_glob {
-                parser.error_and_recover("can't use `as` with `*`", None);
+                parser.error_msg_on_current_token("can't use `as` with `*`");
             }
             if parser.current_kind() == Some(SyntaxKind::AsKw) {
                 parser.parse(UseTreeAliasScope::default(), None);
@@ -34,14 +34,12 @@ impl super::Parse for UseTreeScope {
         if !parser.bump_if(SyntaxKind::Colon2) {
             return;
         }
-        match parser.current_kind() {
-            Some(SyntaxKind::LBrace) if !is_glob => {
-                parser.parse(UseTreeListScope::default(), None);
-            }
-            _ => {
-                parser.error_and_recover("can't use `*` with `{}`", None);
+        if parser.current_kind() == Some(SyntaxKind::LBrace) {
+            if is_glob {
+                parser.error_msg_on_current_token("can't use `*` with `{}`");
             }
-        };
+            parser.parse(UseTreeListScope::default(), None);
+        }
     }
 }
 
@@ -90,7 +88,7 @@ impl super::Parse for UsePathScope {
             });
             if is_path_segment {
                 if self.is_glob.get() {
-                    parser.error_and_recover("can't specify path after `*`", None);
+                    parser.error_msg_on_current_token("can't specify path after `*`");
                 }
                 parser.bump_expected(SyntaxKind::Colon2);
                 self.is_glob

crates/parser2/test_files/error_recovery/exprs/array.snap

@@ -35,6 +35,5 @@ Root@0..19
       Lit@16..17
         Int@16..17 "2"
     Comma@17..18 ","
-    Error@18..18
     RBracket@18..19 "]"
 

crates/parser2/test_files/error_recovery/exprs/call.snap

@@ -66,7 +66,6 @@ Root@0..40
         PathType@32..32
           Path@32..32
             PathSegment@32..32
-              Error@32..32
       Gt@32..33 ">"
     CallArgList@33..39
       LParen@33..34 "("