Merge #1337

1337: Move syntax errors our of syntax tree r=matklad a=matklad

I am not really sure if it's a good idea, but `SyntaxError` do not really belong to a `SyntaxTree`. So let's just store them on the side?

Co-authored-by: Aleksey Kladov <[email protected]>

6 files changed, 105 insertions, 95 deletions

diff --git a/crates/ra_syntax/src/ast.rs b/crates/ra_syntax/src/ast.rs
index f7e33366e..319110b6a 100644
--- a/crates/ra_syntax/src/ast.rs
+++ b/crates/ra_syntax/src/ast.rs
@@ -80,7 +80,9 @@ fn test_doc_comment_none() {
         // non-doc
         mod foo {}
         "#,
-    );
+    )
+    .ok()
+    .unwrap();
     let module = file.syntax().descendants().find_map(Module::cast).unwrap();
     assert!(module.doc_comment_text().is_none());
 }
@@ -93,7 +95,9 @@ fn test_doc_comment_of_items() {
         // non-doc
         mod foo {}
         "#,
-    );
+    )
+    .ok()
+    .unwrap();
     let module = file.syntax().descendants().find_map(Module::cast).unwrap();
     assert_eq!("doc", module.doc_comment_text().unwrap());
 }
@@ -110,7 +114,9 @@ fn test_doc_comment_preserves_indents() {
         /// ```
         mod foo {}
         "#,
-    );
+    )
+    .ok()
+    .unwrap();
     let module = file.syntax().descendants().find_map(Module::cast).unwrap();
     assert_eq!("doc1\n```\nfn foo() {\n    // ...\n}\n```", module.doc_comment_text().unwrap());
 }
@@ -133,7 +139,9 @@ where
    for<'a> F: Fn(&'a str)
 {}
         "#,
-    );
+    )
+    .ok()
+    .unwrap();
     let where_clause = file.syntax().descendants().find_map(WhereClause::cast).unwrap();
 
     let mut predicates = where_clause.predicates();
diff --git a/crates/ra_syntax/src/fuzz.rs b/crates/ra_syntax/src/fuzz.rs
index af11b2e1a..6a9905bd1 100644
--- a/crates/ra_syntax/src/fuzz.rs
+++ b/crates/ra_syntax/src/fuzz.rs
@@ -5,12 +5,11 @@ use std::str::{self, FromStr};
 fn check_file_invariants(file: &SourceFile) {
     let root = file.syntax();
     validation::validate_block_structure(root);
-    let _ = file.errors();
 }
 
 pub fn check_parser(text: &str) {
     let file = SourceFile::parse(text);
-    check_file_invariants(&file);
+    check_file_invariants(&file.tree);
 }
 
 #[derive(Debug, Clone)]
@@ -44,16 +43,18 @@ impl CheckReparse {
     }
 
     pub fn run(&self) {
-        let file = SourceFile::parse(&self.text);
-        let new_file = file.reparse(&self.edit);
-        check_file_invariants(&new_file);
-        assert_eq!(&new_file.syntax().text().to_string(), &self.edited_text);
+        let parse = SourceFile::parse(&self.text);
+        let new_parse = parse.reparse(&self.edit);
+        check_file_invariants(&new_parse.tree);
+        assert_eq!(&new_parse.tree.syntax().text().to_string(), &self.edited_text);
         let full_reparse = SourceFile::parse(&self.edited_text);
-        for (a, b) in new_file.syntax().descendants().zip(full_reparse.syntax().descendants()) {
+        for (a, b) in
+            new_parse.tree.syntax().descendants().zip(full_reparse.tree.syntax().descendants())
+        {
             if (a.kind(), a.range()) != (b.kind(), b.range()) {
-                eprint!("original:\n{}", file.syntax().debug_dump());
-                eprint!("reparsed:\n{}", new_file.syntax().debug_dump());
-                eprint!("full reparse:\n{}", full_reparse.syntax().debug_dump());
+                eprint!("original:\n{}", parse.tree.syntax().debug_dump());
+                eprint!("reparsed:\n{}", new_parse.tree.syntax().debug_dump());
+                eprint!("full reparse:\n{}", full_reparse.tree.syntax().debug_dump());
                 assert_eq!(
                     format!("{:?}", a),
                     format!("{:?}", b),
diff --git a/crates/ra_syntax/src/lib.rs b/crates/ra_syntax/src/lib.rs
index 0ceabc203..930a643b7 100644
--- a/crates/ra_syntax/src/lib.rs
+++ b/crates/ra_syntax/src/lib.rs
@@ -31,6 +31,12 @@ pub mod ast;
 #[doc(hidden)]
 pub mod fuzz;
 
+use std::{sync::Arc, fmt::Write};
+
+use ra_text_edit::AtomTextEdit;
+
+use crate::syntax_node::GreenNode;
+
 pub use rowan::{SmolStr, TextRange, TextUnit};
 pub use ra_parser::SyntaxKind;
 pub use ra_parser::T;
@@ -43,45 +49,72 @@ pub use crate::{
     parsing::{tokenize, classify_literal, Token},
 };
 
-use ra_text_edit::AtomTextEdit;
-use crate::syntax_node::GreenNode;
-
-/// `SourceFile` represents a parse tree for a single Rust file.
-pub use crate::ast::SourceFile;
+/// `Parse` is the result of the parsing: a syntax tree and a collection of
+/// errors.
+///
+/// Note that we always produce a syntax tree, even for completely invalid
+/// files.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Parse {
+    pub tree: TreeArc<SourceFile>,
+    pub errors: Arc<Vec<SyntaxError>>,
+}
 
-impl SourceFile {
-    fn new(green: GreenNode, errors: Vec<SyntaxError>) -> TreeArc<SourceFile> {
-        let root = SyntaxNode::new(green, errors);
-        if cfg!(debug_assertions) {
-            validation::validate_block_structure(&root);
+impl Parse {
+    pub fn ok(self) -> Result<TreeArc<SourceFile>, Arc<Vec<SyntaxError>>> {
+        if self.errors.is_empty() {
+            Ok(self.tree)
+        } else {
+            Err(self.errors)
         }
-        assert_eq!(root.kind(), SyntaxKind::SOURCE_FILE);
-        TreeArc::cast(root)
     }
 
-    pub fn parse(text: &str) -> TreeArc<SourceFile> {
-        let (green, errors) = parsing::parse_text(text);
-        SourceFile::new(green, errors)
+    pub fn reparse(&self, edit: &AtomTextEdit) -> Parse {
+        self.incremental_reparse(edit).unwrap_or_else(|| self.full_reparse(edit))
     }
 
-    pub fn reparse(&self, edit: &AtomTextEdit) -> TreeArc<SourceFile> {
-        self.incremental_reparse(edit).unwrap_or_else(|| self.full_reparse(edit))
+    pub fn debug_dump(&self) -> String {
+        let mut buf = self.tree.syntax().debug_dump();
+        for err in self.errors.iter() {
+            writeln!(buf, "err: `{}`", err).unwrap();
+        }
+        buf
     }
 
-    pub fn incremental_reparse(&self, edit: &AtomTextEdit) -> Option<TreeArc<SourceFile>> {
-        parsing::incremental_reparse(self.syntax(), edit, self.errors())
-            .map(|(green_node, errors, _reparsed_range)| SourceFile::new(green_node, errors))
+    fn incremental_reparse(&self, edit: &AtomTextEdit) -> Option<Parse> {
+        // FIXME: validation errors are not handled here
+        parsing::incremental_reparse(self.tree.syntax(), edit, self.errors.to_vec()).map(
+            |(green_node, errors, _reparsed_range)| Parse {
+                tree: SourceFile::new(green_node),
+                errors: Arc::new(errors),
+            },
+        )
     }
 
-    fn full_reparse(&self, edit: &AtomTextEdit) -> TreeArc<SourceFile> {
-        let text = edit.apply(self.syntax().text().to_string());
+    fn full_reparse(&self, edit: &AtomTextEdit) -> Parse {
+        let text = edit.apply(self.tree.syntax().text().to_string());
         SourceFile::parse(&text)
     }
+}
+
+/// `SourceFile` represents a parse tree for a single Rust file.
+pub use crate::ast::SourceFile;
+
+impl SourceFile {
+    fn new(green: GreenNode) -> TreeArc<SourceFile> {
+        let root = SyntaxNode::new(green);
+        if cfg!(debug_assertions) {
+            validation::validate_block_structure(&root);
+        }
+        assert_eq!(root.kind(), SyntaxKind::SOURCE_FILE);
+        TreeArc::cast(root)
+    }
 
-    pub fn errors(&self) -> Vec<SyntaxError> {
-        let mut errors = self.syntax.root_data().to_vec();
-        errors.extend(validation::validate(self));
-        errors
+    pub fn parse(text: &str) -> Parse {
+        let (green, mut errors) = parsing::parse_text(text);
+        let tree = SourceFile::new(green);
+        errors.extend(validation::validate(&tree));
+        Parse { tree, errors: Arc::new(errors) }
     }
 }
 
@@ -98,14 +131,15 @@ fn api_walkthrough() {
     ";
     // `SourceFile` is the main entry point.
     //
-    // Note how `parse` does not return a `Result`: even completely invalid
-    // source code might be parsed.
-    let file = SourceFile::parse(source_code);
+    // The `parse` method returns a `Parse` -- a pair of syntax tree and a list
+    // of errors. That is, syntax tree is constructed even in presence of errors.
+    let parse = SourceFile::parse(source_code);
+    assert!(parse.errors.is_empty());
 
     // Due to the way ownership is set up, owned syntax Nodes always live behind
     // a `TreeArc` smart pointer. `TreeArc` is roughly an `std::sync::Arc` which
     // points to the whole file instead of an individual node.
-    let file: TreeArc<SourceFile> = file;
+    let file: TreeArc<SourceFile> = parse.tree;
 
     // `SourceFile` is the root of the syntax tree. We can iterate file's items:
     let mut func = None;
diff --git a/crates/ra_syntax/src/parsing/reparsing.rs b/crates/ra_syntax/src/parsing/reparsing.rs
index 3b6687f61..cf27a3393 100644
--- a/crates/ra_syntax/src/parsing/reparsing.rs
+++ b/crates/ra_syntax/src/parsing/reparsing.rs
@@ -166,9 +166,11 @@ fn merge_errors(
 
 #[cfg(test)]
 mod tests {
+    use std::sync::Arc;
+
     use test_utils::{extract_range, assert_eq_text};
 
-    use crate::{SourceFile, AstNode};
+    use crate::{SourceFile, AstNode, Parse};
     use super::*;
 
     fn do_check(before: &str, replace_with: &str, reparsed_len: u32) {
@@ -181,14 +183,14 @@ mod tests {
             let f = SourceFile::parse(&before);
             let edit = AtomTextEdit { delete: range, insert: replace_with.to_string() };
             let (green, new_errors, range) =
-                incremental_reparse(f.syntax(), &edit, f.errors()).unwrap();
+                incremental_reparse(f.tree.syntax(), &edit, f.errors.to_vec()).unwrap();
             assert_eq!(range.len(), reparsed_len.into(), "reparsed fragment has wrong length");
-            SourceFile::new(green, new_errors)
+            Parse { tree: SourceFile::new(green), errors: Arc::new(new_errors) }
         };
 
         assert_eq_text!(
-            &fully_reparsed.syntax().debug_dump(),
-            &incrementally_reparsed.syntax().debug_dump(),
+            &fully_reparsed.tree.syntax().debug_dump(),
+            &incrementally_reparsed.tree.syntax().debug_dump(),
         );
     }
 
diff --git a/crates/ra_syntax/src/ptr.rs b/crates/ra_syntax/src/ptr.rs
index cee9503ca..10cddb852 100644
--- a/crates/ra_syntax/src/ptr.rs
+++ b/crates/ra_syntax/src/ptr.rs
@@ -76,7 +76,7 @@ impl<N: AstNode> From<AstPtr<N>> for SyntaxNodePtr {
 fn test_local_syntax_ptr() {
     use crate::{ast, AstNode, SourceFile};
 
-    let file = SourceFile::parse("struct Foo { f: u32, }");
+    let file = SourceFile::parse("struct Foo { f: u32, }").ok().unwrap();
     let field = file.syntax().descendants().find_map(ast::NamedFieldDef::cast).unwrap();
     let ptr = SyntaxNodePtr::new(field.syntax());
     let field_syntax = ptr.to_node(file.syntax());
diff --git a/crates/ra_syntax/src/syntax_node.rs b/crates/ra_syntax/src/syntax_node.rs
index 80054f529..4105b5220 100644
--- a/crates/ra_syntax/src/syntax_node.rs
+++ b/crates/ra_syntax/src/syntax_node.rs
@@ -9,7 +9,6 @@
 use std::{
     ops::RangeInclusive,
     fmt::{self, Write},
-    any::Any,
     borrow::Borrow,
     iter::successors,
 };
@@ -133,10 +132,8 @@ pub enum Direction {
 }
 
 impl SyntaxNode {
-    pub(crate) fn new(green: GreenNode, errors: Vec<SyntaxError>) -> TreeArc<SyntaxNode> {
-        let errors: Option<Box<Any + Send + Sync>> =
-            if errors.is_empty() { None } else { Some(Box::new(errors)) };
-        let ptr = TreeArc(rowan::SyntaxNode::new(green, errors));
+    pub(crate) fn new(green: GreenNode) -> TreeArc<SyntaxNode> {
+        let ptr = TreeArc(rowan::SyntaxNode::new(green, None));
         TreeArc::cast(ptr)
     }
 
@@ -259,37 +256,18 @@ impl SyntaxNode {
     }
 
     pub fn debug_dump(&self) -> String {
-        let mut errors: Vec<_> = match self.ancestors().find_map(SourceFile::cast) {
-            Some(file) => file.errors(),
-            None => self.root_data().to_vec(),
-        };
-        errors.sort_by_key(|e| e.offset());
-        let mut err_pos = 0;
         let mut level = 0;
         let mut buf = String::new();
-        macro_rules! indent {
-            () => {
-                for _ in 0..level {
-                    buf.push_str("  ");
-                }
-            };
-        }
 
         for event in self.preorder_with_tokens() {
             match event {
                 WalkEvent::Enter(element) => {
-                    indent!();
+                    for _ in 0..level {
+                        buf.push_str("  ");
+                    }
                     match element {
                         SyntaxElement::Node(node) => writeln!(buf, "{:?}", node).unwrap(),
-                        SyntaxElement::Token(token) => {
-                            writeln!(buf, "{:?}", token).unwrap();
-                            let off = token.range().end();
-                            while err_pos < errors.len() && errors[err_pos].offset() <= off {
-                                indent!();
-                                writeln!(buf, "err: `{}`", errors[err_pos]).unwrap();
-                                err_pos += 1;
-                            }
-                        }
+                        SyntaxElement::Token(token) => writeln!(buf, "{:?}", token).unwrap(),
                     }
                     level += 1;
                 }
@@ -298,23 +276,10 @@ impl SyntaxNode {
         }
 
         assert_eq!(level, 0);
-        for err in errors[err_pos..].iter() {
-            writeln!(buf, "err: `{}`", err).unwrap();
-        }
 
         buf
     }
 
-    pub(crate) fn root_data(&self) -> &[SyntaxError] {
-        match self.0.root_data() {
-            None => &[],
-            Some(data) => {
-                let data: &Vec<SyntaxError> = std::any::Any::downcast_ref(data).unwrap();
-                data.as_slice()
-            }
-        }
-    }
-
     pub(crate) fn replace_with(&self, replacement: GreenNode) -> GreenNode {
         self.0.replace_with(replacement)
     }
@@ -386,7 +351,7 @@ impl SyntaxNode {
         let len = new_children.iter().map(|it| it.text_len()).sum::<TextUnit>();
         let new_node = GreenNode::new(rowan::SyntaxKind(self.kind() as u16), new_children);
         let new_file_node = self.replace_with(new_node);
-        let file = SourceFile::new(new_file_node, Vec::new());
+        let file = SourceFile::new(new_file_node);
 
         // FIXME: use a more elegant way to re-fetch the node (#1185), make
         // `range` private afterwards
@@ -629,13 +594,13 @@ impl SyntaxTreeBuilder {
         (green, self.errors)
     }
 
-    pub fn finish(self) -> TreeArc<SyntaxNode> {
+    pub fn finish(self) -> (TreeArc<SyntaxNode>, Vec<SyntaxError>) {
         let (green, errors) = self.finish_raw();
-        let node = SyntaxNode::new(green, errors);
+        let node = SyntaxNode::new(green);
         if cfg!(debug_assertions) {
             crate::validation::validate_block_structure(&node);
         }
-        node
+        (node, errors)
     }
 
     pub fn token(&mut self, kind: SyntaxKind, text: SmolStr) {