4 files changed, 90 insertions, 65 deletions

diff --git a/crates/hir_expand/src/db.rs b/crates/hir_expand/src/db.rs
index ade57ac1b..ff50bfd82 100644
--- a/crates/hir_expand/src/db.rs
+++ b/crates/hir_expand/src/db.rs
@@ -3,7 +3,7 @@
 use std::sync::Arc;
 
 use base_db::{salsa, SourceDatabase};
-use mbe::{ExpandResult, MacroRules};
+use mbe::{ExpandError, ExpandResult, MacroRules};
 use parser::FragmentKind;
 use syntax::{algo::diff, AstNode, GreenNode, Parse, SyntaxKind::*, SyntaxNode};
 
@@ -75,9 +75,14 @@ pub trait AstDatabase: SourceDatabase {
     #[salsa::transparent]
     fn macro_arg(&self, id: MacroCallId) -> Option<Arc<(tt::Subtree, mbe::TokenMap)>>;
     fn macro_def(&self, id: MacroDefId) -> Option<Arc<(TokenExpander, mbe::TokenMap)>>;
-    fn parse_macro(&self, macro_file: MacroFile)
-        -> Option<(Parse<SyntaxNode>, Arc<mbe::TokenMap>)>;
-    fn macro_expand(&self, macro_call: MacroCallId) -> (Option<Arc<tt::Subtree>>, Option<String>);
+    fn parse_macro_expansion(
+        &self,
+        macro_file: MacroFile,
+    ) -> ExpandResult<Option<(Parse<SyntaxNode>, Arc<mbe::TokenMap>)>>;
+    fn macro_expand(&self, macro_call: MacroCallId) -> ExpandResult<Option<Arc<tt::Subtree>>>;
+
+    /// Firewall query that returns the error from the `macro_expand` query.
+    fn macro_expand_error(&self, macro_call: MacroCallId) -> Option<ExpandError>;
 
     #[salsa::interned]
     fn intern_eager_expansion(&self, eager: EagerCallLoc) -> EagerMacroId;
@@ -102,23 +107,20 @@ pub fn expand_hypothetical(
     let token_id = tmap_1.token_by_range(range)?;
     let macro_def = expander(db, actual_macro_call)?;
     let (node, tmap_2) =
-        parse_macro_with_arg(db, macro_file, Some(std::sync::Arc::new((tt, tmap_1))))?;
+        parse_macro_with_arg(db, macro_file, Some(std::sync::Arc::new((tt, tmap_1)))).value?;
     let token_id = macro_def.0.map_id_down(token_id);
     let range = tmap_2.range_by_token(token_id)?.by_kind(token_to_map.kind())?;
     let token = syntax::algo::find_covering_element(&node.syntax_node(), range).into_token()?;
     Some((node.syntax_node(), token))
 }
 
-pub(crate) fn ast_id_map(db: &dyn AstDatabase, file_id: HirFileId) -> Arc<AstIdMap> {
+fn ast_id_map(db: &dyn AstDatabase, file_id: HirFileId) -> Arc<AstIdMap> {
     let map =
         db.parse_or_expand(file_id).map_or_else(AstIdMap::default, |it| AstIdMap::from_source(&it));
     Arc::new(map)
 }
 
-pub(crate) fn macro_def(
-    db: &dyn AstDatabase,
-    id: MacroDefId,
-) -> Option<Arc<(TokenExpander, mbe::TokenMap)>> {
+fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Option<Arc<(TokenExpander, mbe::TokenMap)>> {
     match id.kind {
         MacroDefKind::Declarative => {
             let macro_call = id.ast_id?.to_node(db);
@@ -149,7 +151,7 @@ pub(crate) fn macro_def(
     }
 }
 
-pub(crate) fn macro_arg_text(db: &dyn AstDatabase, id: MacroCallId) -> Option<GreenNode> {
+fn macro_arg_text(db: &dyn AstDatabase, id: MacroCallId) -> Option<GreenNode> {
     let id = match id {
         MacroCallId::LazyMacro(id) => id,
         MacroCallId::EagerMacro(_id) => {
@@ -162,22 +164,20 @@ pub(crate) fn macro_arg_text(db: &dyn AstDatabase, id: MacroCallId) -> Option<Gr
     Some(arg.green().clone())
 }
 
-pub(crate) fn macro_arg(
-    db: &dyn AstDatabase,
-    id: MacroCallId,
-) -> Option<Arc<(tt::Subtree, mbe::TokenMap)>> {
+fn macro_arg(db: &dyn AstDatabase, id: MacroCallId) -> Option<Arc<(tt::Subtree, mbe::TokenMap)>> {
     let arg = db.macro_arg_text(id)?;
     let (tt, tmap) = mbe::syntax_node_to_token_tree(&SyntaxNode::new_root(arg))?;
     Some(Arc::new((tt, tmap)))
 }
 
-pub(crate) fn macro_expand(
-    db: &dyn AstDatabase,
-    id: MacroCallId,
-) -> (Option<Arc<tt::Subtree>>, Option<String>) {
+fn macro_expand(db: &dyn AstDatabase, id: MacroCallId) -> ExpandResult<Option<Arc<tt::Subtree>>> {
     macro_expand_with_arg(db, id, None)
 }
 
+fn macro_expand_error(db: &dyn AstDatabase, macro_call: MacroCallId) -> Option<ExpandError> {
+    db.macro_expand(macro_call).err
+}
+
 fn expander(db: &dyn AstDatabase, id: MacroCallId) -> Option<Arc<(TokenExpander, mbe::TokenMap)>> {
     let lazy_id = match id {
         MacroCallId::LazyMacro(id) => id,
@@ -195,17 +195,20 @@ fn macro_expand_with_arg(
     db: &dyn AstDatabase,
     id: MacroCallId,
     arg: Option<Arc<(tt::Subtree, mbe::TokenMap)>>,
-) -> (Option<Arc<tt::Subtree>>, Option<String>) {
+) -> ExpandResult<Option<Arc<tt::Subtree>>> {
+    let _p = profile::span("macro_expand");
     let lazy_id = match id {
         MacroCallId::LazyMacro(id) => id,
         MacroCallId::EagerMacro(id) => {
             if arg.is_some() {
-                return (
-                    None,
-                    Some("hypothetical macro expansion not implemented for eager macro".to_owned()),
+                return ExpandResult::str_err(
+                    "hypothetical macro expansion not implemented for eager macro".to_owned(),
                 );
             } else {
-                return (Some(db.lookup_intern_eager_expansion(id).subtree), None);
+                return ExpandResult {
+                    value: Some(db.lookup_intern_eager_expansion(id).subtree),
+                    err: None,
+                };
             }
         }
     };
@@ -213,23 +216,27 @@ fn macro_expand_with_arg(
     let loc = db.lookup_intern_macro(lazy_id);
     let macro_arg = match arg.or_else(|| db.macro_arg(id)) {
         Some(it) => it,
-        None => return (None, Some("Fail to args in to tt::TokenTree".into())),
+        None => return ExpandResult::str_err("Fail to args in to tt::TokenTree".into()),
     };
 
     let macro_rules = match db.macro_def(loc.def) {
         Some(it) => it,
-        None => return (None, Some("Fail to find macro definition".into())),
+        None => return ExpandResult::str_err("Fail to find macro definition".into()),
     };
-    let ExpandResult(tt, err) = macro_rules.0.expand(db, lazy_id, &macro_arg.0);
+    let ExpandResult { value: tt, err } = macro_rules.0.expand(db, lazy_id, &macro_arg.0);
     // Set a hard limit for the expanded tt
     let count = tt.count();
     if count > 262144 {
-        return (None, Some(format!("Total tokens count exceed limit : count = {}", count)));
+        return ExpandResult::str_err(format!(
+            "Total tokens count exceed limit : count = {}",
+            count
+        ));
     }
-    (Some(Arc::new(tt)), err.map(|e| format!("{:?}", e)))
+
+    ExpandResult { value: Some(Arc::new(tt)), err }
 }
 
-pub(crate) fn expand_proc_macro(
+fn expand_proc_macro(
     db: &dyn AstDatabase,
     id: MacroCallId,
 ) -> Result<tt::Subtree, mbe::ExpandError> {
@@ -256,36 +263,37 @@ pub(crate) fn expand_proc_macro(
     expander.expand(db, lazy_id, &macro_arg.0)
 }
 
-pub(crate) fn parse_or_expand(db: &dyn AstDatabase, file_id: HirFileId) -> Option<SyntaxNode> {
+fn parse_or_expand(db: &dyn AstDatabase, file_id: HirFileId) -> Option<SyntaxNode> {
     match file_id.0 {
         HirFileIdRepr::FileId(file_id) => Some(db.parse(file_id).tree().syntax().clone()),
         HirFileIdRepr::MacroFile(macro_file) => {
-            db.parse_macro(macro_file).map(|(it, _)| it.syntax_node())
+            db.parse_macro_expansion(macro_file).value.map(|(it, _)| it.syntax_node())
         }
     }
 }
 
-pub(crate) fn parse_macro(
+fn parse_macro_expansion(
     db: &dyn AstDatabase,
     macro_file: MacroFile,
-) -> Option<(Parse<SyntaxNode>, Arc<mbe::TokenMap>)> {
+) -> ExpandResult<Option<(Parse<SyntaxNode>, Arc<mbe::TokenMap>)>> {
     parse_macro_with_arg(db, macro_file, None)
 }
 
-pub fn parse_macro_with_arg(
+fn parse_macro_with_arg(
     db: &dyn AstDatabase,
     macro_file: MacroFile,
     arg: Option<Arc<(tt::Subtree, mbe::TokenMap)>>,
-) -> Option<(Parse<SyntaxNode>, Arc<mbe::TokenMap>)> {
-    let _p = profile::span("parse_macro_query");
-
+) -> ExpandResult<Option<(Parse<SyntaxNode>, Arc<mbe::TokenMap>)>> {
     let macro_call_id = macro_file.macro_call_id;
-    let (tt, err) = if let Some(arg) = arg {
+    let result = if let Some(arg) = arg {
         macro_expand_with_arg(db, macro_call_id, Some(arg))
     } else {
         db.macro_expand(macro_call_id)
     };
-    if let Some(err) = &err {
+
+    let _p = profile::span("parse_macro_expansion");
+
+    if let Some(err) = &result.err {
         // Note:
         // The final goal we would like to make all parse_macro success,
         // such that the following log will not call anyway.
@@ -303,40 +311,50 @@ pub fn parse_macro_with_arg(
                 .join("\n");
 
                 log::warn!(
-                    "fail on macro_parse: (reason: {} macro_call: {:#}) parents: {}",
+                    "fail on macro_parse: (reason: {:?} macro_call: {:#}) parents: {}",
                     err,
                     node.value,
                     parents
                 );
             }
             _ => {
-                log::warn!("fail on macro_parse: (reason: {})", err);
+                log::warn!("fail on macro_parse: (reason: {:?})", err);
             }
         }
+    }
+    let tt = match result.value {
+        Some(tt) => tt,
+        None => return ExpandResult { value: None, err: result.err },
     };
-    let tt = tt?;
 
     let fragment_kind = to_fragment_kind(db, macro_call_id);
 
-    let (parse, rev_token_map) = mbe::token_tree_to_syntax_node(&tt, fragment_kind).ok()?;
+    let (parse, rev_token_map) = match mbe::token_tree_to_syntax_node(&tt, fragment_kind) {
+        Ok(it) => it,
+        Err(err) => {
+            return ExpandResult::only_err(err);
+        }
+    };
 
-    if err.is_none() {
-        Some((parse, Arc::new(rev_token_map)))
-    } else {
-        // FIXME:
-        // In future, we should propagate the actual error with recovery information
-        // instead of ignore the error here.
-
-        // Safe check for recurisve identity macro
-        let node = parse.syntax_node();
-        let file: HirFileId = macro_file.into();
-        let call_node = file.call_node(db)?;
-
-        if !diff(&node, &call_node.value).is_empty() {
-            Some((parse, Arc::new(rev_token_map)))
-        } else {
-            None
+    match result.err {
+        Some(err) => {
+            // Safety check for recursive identity macro.
+            let node = parse.syntax_node();
+            let file: HirFileId = macro_file.into();
+            let call_node = match file.call_node(db) {
+                Some(it) => it,
+                None => {
+                    return ExpandResult::only_err(err);
+                }
+            };
+
+            if !diff(&node, &call_node.value).is_empty() {
+                ExpandResult { value: Some((parse, Arc::new(rev_token_map))), err: Some(err) }
+            } else {
+                return ExpandResult::only_err(err);
+            }
         }
+        None => ExpandResult { value: Some((parse, Arc::new(rev_token_map))), err: None },
     }
 }
 
diff --git a/crates/hir_expand/src/diagnostics.rs b/crates/hir_expand/src/diagnostics.rs
index 1043c6aeb..bf0b85ce9 100644
--- a/crates/hir_expand/src/diagnostics.rs
+++ b/crates/hir_expand/src/diagnostics.rs
@@ -5,7 +5,7 @@
 //!
 //! `DiagnosticSink` struct is used as an emitter for diagnostic. When creating
 //! a `DiagnosticSink`, you supply a callback which can react to a `dyn
-//! Diagnostic` or to any concrete diagnostic (downcasting is sued internally).
+//! Diagnostic` or to any concrete diagnostic (downcasting is used internally).
 //!
 //! Because diagnostics store file offsets, it's a bad idea to store them
 //! directly in salsa. For this reason, every hir subsytem defines it's own
@@ -32,7 +32,12 @@ impl DiagnosticCode {
 pub trait Diagnostic: Any + Send + Sync + fmt::Debug + 'static {
     fn code(&self) -> DiagnosticCode;
     fn message(&self) -> String;
-    /// Used in highlighting and related purposes
+    /// Source element that triggered the diagnostics.
+    ///
+    /// Note that this should reflect "semantics", rather than specific span we
+    /// want to highlight. When rendering the diagnostics into an error message,
+    /// the IDE will fetch the `SyntaxNode` and will narrow the span
+    /// appropriately.
     fn display_source(&self) -> InFile<SyntaxNodePtr>;
     fn as_any(&self) -> &(dyn Any + Send + 'static);
     fn is_experimental(&self) -> bool {
diff --git a/crates/hir_expand/src/lib.rs b/crates/hir_expand/src/lib.rs
index 17f1178ed..6dad2507b 100644
--- a/crates/hir_expand/src/lib.rs
+++ b/crates/hir_expand/src/lib.rs
@@ -15,6 +15,8 @@ pub mod proc_macro;
 pub mod quote;
 pub mod eager;
 
+pub use mbe::{ExpandError, ExpandResult};
+
 use std::hash::Hash;
 use std::sync::Arc;
 
@@ -144,7 +146,7 @@ impl HirFileId {
                 let def_tt = loc.def.ast_id?.to_node(db).token_tree()?;
 
                 let macro_def = db.macro_def(loc.def)?;
-                let (parse, exp_map) = db.parse_macro(macro_file)?;
+                let (parse, exp_map) = db.parse_macro_expansion(macro_file).value?;
                 let macro_arg = db.macro_arg(macro_file.macro_call_id)?;
 
                 Some(ExpansionInfo {
@@ -253,7 +255,7 @@ pub enum MacroDefKind {
 pub struct MacroCallLoc {
     pub(crate) def: MacroDefId,
     pub(crate) krate: CrateId,
-    pub(crate) kind: MacroCallKind,
+    pub kind: MacroCallKind,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
diff --git a/crates/hir_expand/src/proc_macro.rs b/crates/hir_expand/src/proc_macro.rs
index 7505cb061..97edf0fb6 100644
--- a/crates/hir_expand/src/proc_macro.rs
+++ b/crates/hir_expand/src/proc_macro.rs
@@ -50,7 +50,7 @@ impl ProcMacroExpander {
 
                 proc_macro.expander.expand(&tt, None).map_err(mbe::ExpandError::from)
             }
-            None => Err(err!("Unresolved proc macro")),
+            None => Err(mbe::ExpandError::UnresolvedProcMacro),
         }
     }
 }