From 6ae8d49e1554cbf99387ed83079277f5f854d187 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 16 Apr 2023 15:46:12 +0200 Subject: [PATCH 1/4] Simplify eager macro error handling --- crates/hir-def/src/body.rs | 16 +- crates/hir-def/src/lib.rs | 42 ++--- .../hir-def/src/macro_expansion_tests/mod.rs | 18 +- crates/hir-def/src/nameres/collector.rs | 40 ++-- crates/hir-expand/src/db.rs | 40 ++-- crates/hir-expand/src/eager.rs | 171 ++++++------------ 6 files changed, 118 insertions(+), 209 deletions(-) diff --git a/crates/hir-def/src/body.rs b/crates/hir-def/src/body.rs index 928aadfbcc..f4304ae7e8 100644 --- a/crates/hir-def/src/body.rs +++ b/crates/hir-def/src/body.rs @@ -138,6 +138,7 @@ impl Expander { db: &dyn DefDatabase, macro_call: ast::MacroCall, ) -> Result>, UnresolvedMacro> { + // FIXME: within_limit should support this, instead of us having to extract the error let mut unresolved_macro_err = None; let result = self.within_limit(db, |this| { @@ -146,22 +147,13 @@ impl Expander { let resolver = |path| this.resolve_path_as_macro(db, &path).map(|it| macro_id_to_def_id(db, it)); - let mut err = None; - let call_id = match macro_call.as_call_id_with_errors( - db, - this.def_map.krate(), - resolver, - &mut |e| { - err.get_or_insert(e); - }, - ) { + match macro_call.as_call_id_with_errors(db, this.def_map.krate(), resolver) { Ok(call_id) => call_id, Err(resolve_err) => { unresolved_macro_err = Some(resolve_err); - return ExpandResult { value: None, err: None }; + ExpandResult { value: None, err: None } } - }; - ExpandResult { value: call_id.ok(), err } + } }); if let Some(err) = unresolved_macro_err { diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs index 5a0c1b66b9..55e1e91220 100644 --- a/crates/hir-def/src/lib.rs +++ b/crates/hir-def/src/lib.rs @@ -65,11 +65,11 @@ use hir_expand::{ builtin_attr_macro::BuiltinAttrExpander, builtin_derive_macro::BuiltinDeriveExpander, builtin_fn_macro::{BuiltinFnLikeExpander, EagerExpander}, - eager::{expand_eager_macro, ErrorEmitted, ErrorSink}, + eager::expand_eager_macro, hygiene::Hygiene, proc_macro::ProcMacroExpander, - AstId, ExpandError, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroDefId, - MacroDefKind, UnresolvedMacro, + AstId, ExpandError, ExpandResult, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, + MacroDefId, MacroDefKind, UnresolvedMacro, }; use item_tree::ExternBlock; use la_arena::Idx; @@ -795,7 +795,7 @@ pub trait AsMacroCall { krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, ) -> Option { - self.as_call_id_with_errors(db, krate, resolver, &mut |_| ()).ok()?.ok() + self.as_call_id_with_errors(db, krate, resolver).ok()?.value } fn as_call_id_with_errors( @@ -803,8 +803,7 @@ pub trait AsMacroCall { db: &dyn db::DefDatabase, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, - error_sink: &mut dyn FnMut(ExpandError), - ) -> Result, UnresolvedMacro>; + ) -> Result>, UnresolvedMacro>; } impl AsMacroCall for InFile<&ast::MacroCall> { @@ -813,21 +812,15 @@ impl AsMacroCall for InFile<&ast::MacroCall> { db: &dyn db::DefDatabase, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, - mut error_sink: &mut dyn FnMut(ExpandError), - ) -> Result, UnresolvedMacro> { + ) -> Result>, UnresolvedMacro> { let expands_to = hir_expand::ExpandTo::from_call_site(self.value); let ast_id = AstId::new(self.file_id, db.ast_id_map(self.file_id).ast_id(self.value)); let h = Hygiene::new(db.upcast(), self.file_id); let path = self.value.path().and_then(|path| path::ModPath::from_src(db.upcast(), path, &h)); - let path = match error_sink - .option(path, || ExpandError::Other("malformed macro invocation".into())) - { - Ok(path) => path, - Err(error) => { - return Ok(Err(error)); - } + let Some(path) = path else { + return Ok(ExpandResult::only_err(ExpandError::Other("malformed macro invocation".into()))); }; macro_call_as_call_id( @@ -836,7 +829,6 @@ impl AsMacroCall for InFile<&ast::MacroCall> { expands_to, krate, resolver, - error_sink, ) } } @@ -860,21 +852,23 @@ fn macro_call_as_call_id( expand_to: ExpandTo, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, - error_sink: &mut dyn FnMut(ExpandError), -) -> Result, UnresolvedMacro> { +) -> Result>, UnresolvedMacro> { let def = resolver(call.path.clone()).ok_or_else(|| UnresolvedMacro { path: call.path.clone() })?; let res = if let MacroDefKind::BuiltInEager(..) = def.kind { let macro_call = InFile::new(call.ast_id.file_id, call.ast_id.to_node(db.upcast())); - expand_eager_macro(db.upcast(), krate, macro_call, def, &resolver, error_sink)? + expand_eager_macro(db.upcast(), krate, macro_call, def, &resolver)? } else { - Ok(def.as_lazy_macro( - db.upcast(), - krate, - MacroCallKind::FnLike { ast_id: call.ast_id, expand_to }, - )) + ExpandResult { + value: Some(def.as_lazy_macro( + db.upcast(), + krate, + MacroCallKind::FnLike { ast_id: call.ast_id, expand_to }, + )), + err: None, + } }; Ok(res) } diff --git a/crates/hir-def/src/macro_expansion_tests/mod.rs b/crates/hir-def/src/macro_expansion_tests/mod.rs index 314bf22b95..73a495d89b 100644 --- a/crates/hir-def/src/macro_expansion_tests/mod.rs +++ b/crates/hir-def/src/macro_expansion_tests/mod.rs @@ -125,21 +125,15 @@ pub fn identity_when_valid(_attr: TokenStream, item: TokenStream) -> TokenStream for macro_call in source_file.syntax().descendants().filter_map(ast::MacroCall::cast) { let macro_call = InFile::new(source.file_id, ¯o_call); - let mut error = None; - let macro_call_id = macro_call - .as_call_id_with_errors( - &db, - krate, - |path| { - resolver.resolve_path_as_macro(&db, &path).map(|it| macro_id_to_def_id(&db, it)) - }, - &mut |err| error = Some(err), - ) - .unwrap() + let res = macro_call + .as_call_id_with_errors(&db, krate, |path| { + resolver.resolve_path_as_macro(&db, &path).map(|it| macro_id_to_def_id(&db, it)) + }) .unwrap(); + let macro_call_id = res.value.unwrap(); let macro_file = MacroFile { macro_call_id }; let mut expansion_result = db.parse_macro_expansion(macro_file); - expansion_result.err = expansion_result.err.or(error); + expansion_result.err = expansion_result.err.or(res.err); expansions.push((macro_call.value.clone(), expansion_result, db.macro_arg(macro_call_id))); } diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index 1d625fa3c7..8ab0b3dbd1 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/crates/hir-def/src/nameres/collector.rs @@ -16,8 +16,8 @@ use hir_expand::{ builtin_fn_macro::find_builtin_macro, name::{name, AsName, Name}, proc_macro::ProcMacroExpander, - ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroCallLoc, MacroDefId, - MacroDefKind, + ExpandResult, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroCallLoc, + MacroDefId, MacroDefKind, }; use itertools::{izip, Itertools}; use la_arena::Idx; @@ -1116,9 +1116,8 @@ impl DefCollector<'_> { *expand_to, self.def_map.krate, resolver_def_id, - &mut |_err| (), ); - if let Ok(Ok(call_id)) = call_id { + if let Ok(ExpandResult { value: Some(call_id), .. }) = call_id { push_resolved(directive, call_id); res = ReachedFixedPoint::No; return false; @@ -1414,7 +1413,6 @@ impl DefCollector<'_> { .take_macros() .map(|it| macro_id_to_def_id(self.db, it)) }, - &mut |_| (), ); if let Err(UnresolvedMacro { path }) = macro_call_as_call_id { self.def_map.diagnostics.push(DefDiagnostic::unresolved_macro_call( @@ -2112,7 +2110,6 @@ impl ModCollector<'_, '_> { let ast_id = AstIdWithPath::new(self.file_id(), mac.ast_id, ModPath::clone(&mac.path)); // Case 1: try to resolve in legacy scope and expand macro_rules - let mut error = None; match macro_call_as_call_id( self.def_collector.db, &ast_id, @@ -2133,21 +2130,20 @@ impl ModCollector<'_, '_> { ) }) }, - &mut |err| { - error.get_or_insert(err); - }, ) { - Ok(Ok(macro_call_id)) => { + Ok(res) => { // Legacy macros need to be expanded immediately, so that any macros they produce // are in scope. - self.def_collector.collect_macro_expansion( - self.module_id, - macro_call_id, - self.macro_depth + 1, - container, - ); + if let Some(val) = res.value { + self.def_collector.collect_macro_expansion( + self.module_id, + val, + self.macro_depth + 1, + container, + ); + } - if let Some(err) = error { + if let Some(err) = res.err { self.def_collector.def_map.diagnostics.push(DefDiagnostic::macro_error( self.module_id, MacroCallKind::FnLike { ast_id: ast_id.ast_id, expand_to: mac.expand_to }, @@ -2157,16 +2153,6 @@ impl ModCollector<'_, '_> { return; } - Ok(Err(_)) => { - // Built-in macro failed eager expansion. - - self.def_collector.def_map.diagnostics.push(DefDiagnostic::macro_error( - self.module_id, - MacroCallKind::FnLike { ast_id: ast_id.ast_id, expand_to: mac.expand_to }, - error.unwrap().to_string(), - )); - return; - } Err(UnresolvedMacro { .. }) => (), } diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs index d93f3b08d3..d7aff6b02f 100644 --- a/crates/hir-expand/src/db.rs +++ b/crates/hir-expand/src/db.rs @@ -279,25 +279,28 @@ fn parse_macro_expansion( let mbe::ValueResult { value, err } = db.macro_expand(macro_file.macro_call_id); if let Some(err) = &err { - // Note: - // The final goal we would like to make all parse_macro success, - // such that the following log will not call anyway. - let loc: MacroCallLoc = db.lookup_intern_macro_call(macro_file.macro_call_id); - let node = loc.kind.to_node(db); + if tracing::enabled!(tracing::Level::DEBUG) { + // Note: + // The final goal we would like to make all parse_macro success, + // such that the following log will not call anyway. + let loc: MacroCallLoc = db.lookup_intern_macro_call(macro_file.macro_call_id); + let node = loc.kind.to_node(db); - // collect parent information for warning log - let parents = - std::iter::successors(loc.kind.file_id().call_node(db), |it| it.file_id.call_node(db)) - .map(|n| format!("{:#}", n.value)) - .collect::>() - .join("\n"); + // collect parent information for warning log + let parents = std::iter::successors(loc.kind.file_id().call_node(db), |it| { + it.file_id.call_node(db) + }) + .map(|n| format!("{:#}", n.value)) + .collect::>() + .join("\n"); - tracing::debug!( - "fail on macro_parse: (reason: {:?} macro_call: {:#}) parents: {}", - err, - node.value, - parents - ); + tracing::debug!( + "fail on macro_parse: (reason: {:?} macro_call: {:#}) parents: {}", + err, + node.value, + parents + ); + } } let tt = match value { Some(tt) => tt, @@ -466,7 +469,8 @@ fn macro_expand( Ok(it) => it, // FIXME: This is weird -- we effectively report macro *definition* // errors lazily, when we try to expand the macro. Instead, they should - // be reported at the definition site (when we construct a def map). + // be reported at the definition site when we construct a def map. + // (Note we do report them also at the definition site in the late diagnostic pass) Err(err) => { return ExpandResult::only_err(ExpandError::Other( format!("invalid macro definition: {err}").into(), diff --git a/crates/hir-expand/src/eager.rs b/crates/hir-expand/src/eager.rs index 6b646f5e4f..74adced8c6 100644 --- a/crates/hir-expand/src/eager.rs +++ b/crates/hir-expand/src/eager.rs @@ -32,77 +32,16 @@ use crate::{ MacroCallLoc, MacroDefId, MacroDefKind, UnresolvedMacro, }; -#[derive(Debug)] -pub struct ErrorEmitted { - _private: (), -} - -pub trait ErrorSink { - fn emit(&mut self, err: ExpandError); - - fn option( - &mut self, - opt: Option, - error: impl FnOnce() -> ExpandError, - ) -> Result { - match opt { - Some(it) => Ok(it), - None => { - self.emit(error()); - Err(ErrorEmitted { _private: () }) - } - } - } - - fn option_with( - &mut self, - opt: impl FnOnce() -> Option, - error: impl FnOnce() -> ExpandError, - ) -> Result { - self.option(opt(), error) - } - - fn result(&mut self, res: Result) -> Result { - match res { - Ok(it) => Ok(it), - Err(e) => { - self.emit(e); - Err(ErrorEmitted { _private: () }) - } - } - } - - fn expand_result_option(&mut self, res: ExpandResult>) -> Result { - match (res.value, res.err) { - (None, Some(err)) => { - self.emit(err); - Err(ErrorEmitted { _private: () }) - } - (Some(value), opt_err) => { - if let Some(err) = opt_err { - self.emit(err); - } - Ok(value) - } - (None, None) => unreachable!("`ExpandResult` without value or error"), - } - } -} - -impl ErrorSink for &'_ mut dyn FnMut(ExpandError) { - fn emit(&mut self, err: ExpandError) { - self(err); - } -} - pub fn expand_eager_macro( db: &dyn ExpandDatabase, krate: CrateId, macro_call: InFile, def: MacroDefId, resolver: &dyn Fn(ModPath) -> Option, - diagnostic_sink: &mut dyn FnMut(ExpandError), -) -> Result, UnresolvedMacro> { +) -> Result>, UnresolvedMacro> { + let MacroDefKind::BuiltInEager(eager, _) = def.kind else { + panic!("called `expand_eager_macro` on non-eager macro def {def:?}") + }; let hygiene = Hygiene::new(db, macro_call.file_id); let parsed_args = macro_call .value @@ -129,40 +68,34 @@ pub fn expand_eager_macro( }); let parsed_args = mbe::token_tree_to_syntax_node(&parsed_args, mbe::TopEntryPoint::Expr).0; - let result = match eager_macro_recur( + let ExpandResult { value, mut err } = eager_macro_recur( db, &hygiene, InFile::new(arg_id.as_file(), parsed_args.syntax_node()), krate, resolver, - diagnostic_sink, - ) { - Ok(Ok(it)) => it, - Ok(Err(err)) => return Ok(Err(err)), - Err(err) => return Err(err), + )?; + let Some(value ) = value else { + return Ok(ExpandResult { value: None, err }) }; - let subtree = to_subtree(&result); + let subtree = to_subtree(&value); - if let MacroDefKind::BuiltInEager(eager, _) = def.kind { - let res = eager.expand(db, arg_id, &subtree); - if let Some(err) = res.err { - diagnostic_sink(err); - } - - let loc = MacroCallLoc { - def, - krate, - eager: Some(EagerCallInfo { - arg_or_expansion: Arc::new(res.value.subtree), - included_file: res.value.included_file, - }), - kind: MacroCallKind::FnLike { ast_id: call_id, expand_to }, - }; - - Ok(Ok(db.intern_macro_call(loc))) - } else { - panic!("called `expand_eager_macro` on non-eager macro def {def:?}"); + let res = eager.expand(db, arg_id, &subtree); + if err.is_none() { + err = res.err; } + + let loc = MacroCallLoc { + def, + krate, + eager: Some(EagerCallInfo { + arg_or_expansion: Arc::new(res.value.subtree), + included_file: res.value.included_file, + }), + kind: MacroCallKind::FnLike { ast_id: call_id, expand_to }, + }; + + Ok(ExpandResult { value: Some(db.intern_macro_call(loc)), err }) } fn to_subtree(node: &SyntaxNode) -> crate::tt::Subtree { @@ -201,23 +134,25 @@ fn eager_macro_recur( curr: InFile, krate: CrateId, macro_resolver: &dyn Fn(ModPath) -> Option, - mut diagnostic_sink: &mut dyn FnMut(ExpandError), -) -> Result, UnresolvedMacro> { +) -> Result>, UnresolvedMacro> { let original = curr.value.clone_for_update(); let children = original.descendants().filter_map(ast::MacroCall::cast); let mut replacements = Vec::new(); + // Note: We only report a single error inside of eager expansions + let mut error = None; + // Collect replacement for child in children { let def = match child.path().and_then(|path| ModPath::from_src(db, path, hygiene)) { Some(path) => macro_resolver(path.clone()).ok_or(UnresolvedMacro { path })?, None => { - diagnostic_sink(ExpandError::Other("malformed macro invocation".into())); + error = Some(ExpandError::Other("malformed macro invocation".into())); continue; } }; - let insert = match def.kind { + let ExpandResult { value, err } = match def.kind { MacroDefKind::BuiltInEager(..) => { let id = match expand_eager_macro( db, @@ -225,45 +160,49 @@ fn eager_macro_recur( curr.with_value(child.clone()), def, macro_resolver, - diagnostic_sink, ) { - Ok(Ok(it)) => it, - Ok(Err(err)) => return Ok(Err(err)), + Ok(it) => it, Err(err) => return Err(err), }; - db.parse_or_expand(id.as_file()) - .expect("successful macro expansion should be parseable") - .clone_for_update() + id.map(|call| { + call.and_then(|call| db.parse_or_expand(call.as_file())) + .map(|it| it.clone_for_update()) + }) } MacroDefKind::Declarative(_) | MacroDefKind::BuiltIn(..) | MacroDefKind::BuiltInAttr(..) | MacroDefKind::BuiltInDerive(..) | MacroDefKind::ProcMacro(..) => { - let res = lazy_expand(db, &def, curr.with_value(child.clone()), krate); - let val = match diagnostic_sink.expand_result_option(res) { - Ok(it) => it, - Err(err) => return Ok(Err(err)), - }; + let ExpandResult { value, err } = + lazy_expand(db, &def, curr.with_value(child.clone()), krate); - // replace macro inside - let hygiene = Hygiene::new(db, val.file_id); - match eager_macro_recur(db, &hygiene, val, krate, macro_resolver, diagnostic_sink) { - Ok(Ok(it)) => it, - Ok(Err(err)) => return Ok(Err(err)), - Err(err) => return Err(err), + match value { + Some(val) => { + // replace macro inside + let hygiene = Hygiene::new(db, val.file_id); + let ExpandResult { value, err: error } = + eager_macro_recur(db, &hygiene, val, krate, macro_resolver)?; + let err = if err.is_none() { error } else { err }; + ExpandResult { value, err } + } + None => ExpandResult { value: None, err }, } } }; - + if err.is_some() { + error = err; + } // check if the whole original syntax is replaced if child.syntax() == &original { - return Ok(Ok(insert)); + return Ok(ExpandResult { value, err: error }); } - replacements.push((child, insert)); + if let Some(insert) = value { + replacements.push((child, insert)); + } } replacements.into_iter().rev().for_each(|(old, new)| ted::replace(old.syntax(), new)); - Ok(Ok(original)) + Ok(ExpandResult { value: Some(original), err: error }) } From 71b50f9f09d21e33e87c565617c1043284db80c1 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 16 Apr 2023 16:11:59 +0200 Subject: [PATCH 2/4] Record eager expansion errors in EagerCallInfo --- crates/hir-expand/src/db.rs | 3 +-- crates/hir-expand/src/eager.rs | 10 ++++++---- crates/hir-expand/src/lib.rs | 11 ++++++----- crates/mbe/src/lib.rs | 2 +- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs index d7aff6b02f..e8fba15601 100644 --- a/crates/hir-expand/src/db.rs +++ b/crates/hir-expand/src/db.rs @@ -451,8 +451,7 @@ fn macro_expand( if let Some(eager) = &loc.eager { return ExpandResult { value: Some(eager.arg_or_expansion.clone()), - // FIXME: There could be errors here! - err: None, + err: eager.error.clone(), }; } diff --git a/crates/hir-expand/src/eager.rs b/crates/hir-expand/src/eager.rs index 74adced8c6..7d00b682a9 100644 --- a/crates/hir-expand/src/eager.rs +++ b/crates/hir-expand/src/eager.rs @@ -60,10 +60,11 @@ pub fn expand_eager_macro( let arg_id = db.intern_macro_call(MacroCallLoc { def, krate, - eager: Some(EagerCallInfo { + eager: Some(Box::new(EagerCallInfo { arg_or_expansion: Arc::new(parsed_args.clone()), included_file: None, - }), + error: None, + })), kind: MacroCallKind::FnLike { ast_id: call_id, expand_to: ExpandTo::Expr }, }); @@ -88,10 +89,11 @@ pub fn expand_eager_macro( let loc = MacroCallLoc { def, krate, - eager: Some(EagerCallInfo { + eager: Some(Box::new(EagerCallInfo { arg_or_expansion: Arc::new(res.value.subtree), included_file: res.value.included_file, - }), + error: err.clone(), + })), kind: MacroCallKind::FnLike { ast_id: call_id, expand_to }, }; diff --git a/crates/hir-expand/src/lib.rs b/crates/hir-expand/src/lib.rs index d26fdbf7d6..9685320cf5 100644 --- a/crates/hir-expand/src/lib.rs +++ b/crates/hir-expand/src/lib.rs @@ -52,7 +52,7 @@ use crate::{ pub type ExpandResult = ValueResult; -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone, Hash)] pub enum ExpandError { UnresolvedProcMacro(CrateId), Mbe(mbe::ExpandError), @@ -114,7 +114,7 @@ impl_intern_key!(MacroCallId); pub struct MacroCallLoc { pub def: MacroDefId, pub(crate) krate: CrateId, - eager: Option, + eager: Option>, pub kind: MacroCallKind, } @@ -141,6 +141,7 @@ struct EagerCallInfo { /// NOTE: This can be *either* the expansion result, *or* the argument to the eager macro! arg_or_expansion: Arc, included_file: Option<(FileId, TokenMap)>, + error: Option, } #[derive(Debug, Clone, PartialEq, Eq, Hash)] @@ -206,8 +207,8 @@ impl HirFileId { HirFileIdRepr::FileId(id) => break id, HirFileIdRepr::MacroFile(MacroFile { macro_call_id }) => { let loc: MacroCallLoc = db.lookup_intern_macro_call(macro_call_id); - file_id = match loc.eager { - Some(EagerCallInfo { included_file: Some((file, _)), .. }) => file.into(), + file_id = match loc.eager.as_deref() { + Some(&EagerCallInfo { included_file: Some((file, _)), .. }) => file.into(), _ => loc.kind.file_id(), }; } @@ -320,7 +321,7 @@ impl HirFileId { match self.macro_file() { Some(macro_file) => { let loc: MacroCallLoc = db.lookup_intern_macro_call(macro_file.macro_call_id); - matches!(loc.eager, Some(EagerCallInfo { included_file: Some(..), .. })) + matches!(loc.eager.as_deref(), Some(EagerCallInfo { included_file: Some(..), .. })) } _ => false, } diff --git a/crates/mbe/src/lib.rs b/crates/mbe/src/lib.rs index ac107a0d6d..23ec3235d2 100644 --- a/crates/mbe/src/lib.rs +++ b/crates/mbe/src/lib.rs @@ -69,7 +69,7 @@ impl fmt::Display for ParseError { } } -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq, Clone, Hash)] pub enum ExpandError { BindingError(Box>), LeftoverTokens, From d1632c2727474c0a88b19de3718af806d42e4450 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 16 Apr 2023 17:22:06 +0200 Subject: [PATCH 3/4] Report syntax errors from item level macro expansions --- crates/hir-def/src/body.rs | 42 +++++++-------- crates/hir-def/src/body/lower.rs | 6 ++- crates/hir-def/src/data.rs | 66 +++++++++++++++-------- crates/hir-def/src/generics.rs | 2 +- crates/hir-def/src/nameres/diagnostics.rs | 23 +++++++- crates/hir-expand/src/db.rs | 11 ++-- crates/hir-expand/src/eager.rs | 23 ++++---- crates/hir-ty/src/lower.rs | 3 +- crates/hir/src/diagnostics.rs | 12 ++++- crates/hir/src/lib.rs | 22 ++++---- crates/ide-diagnostics/src/lib.rs | 13 +++++ 11 files changed, 147 insertions(+), 76 deletions(-) diff --git a/crates/hir-def/src/body.rs b/crates/hir-def/src/body.rs index f4304ae7e8..1d082d5554 100644 --- a/crates/hir-def/src/body.rs +++ b/crates/hir-def/src/body.rs @@ -21,7 +21,7 @@ use limit::Limit; use once_cell::unsync::OnceCell; use profile::Count; use rustc_hash::FxHashMap; -use syntax::{ast, AstPtr, SyntaxNode, SyntaxNodePtr}; +use syntax::{ast, AstPtr, Parse, SyntaxNode, SyntaxNodePtr}; use crate::{ attr::Attrs, @@ -137,7 +137,7 @@ impl Expander { &mut self, db: &dyn DefDatabase, macro_call: ast::MacroCall, - ) -> Result>, UnresolvedMacro> { + ) -> Result)>>, UnresolvedMacro> { // FIXME: within_limit should support this, instead of us having to extract the error let mut unresolved_macro_err = None; @@ -167,37 +167,37 @@ impl Expander { &mut self, db: &dyn DefDatabase, call_id: MacroCallId, - ) -> ExpandResult> { + ) -> ExpandResult)>> { self.within_limit(db, |_this| ExpandResult::ok(Some(call_id))) } fn enter_expand_inner( db: &dyn DefDatabase, call_id: MacroCallId, - mut err: Option, - ) -> ExpandResult> { - if err.is_none() { - err = db.macro_expand_error(call_id); + mut error: Option, + ) -> ExpandResult>>> { + let file_id = call_id.as_file(); + let ExpandResult { value, err } = db.parse_or_expand_with_err(file_id); + + if error.is_none() { + error = err; } - let file_id = call_id.as_file(); - - let raw_node = match db.parse_or_expand_with_err(file_id) { - // FIXME: report parse errors - Some(it) => it.syntax_node(), + let parse = match value { + Some(it) => it, None => { // Only `None` if the macro expansion produced no usable AST. - if err.is_none() { + if error.is_none() { tracing::warn!("no error despite `parse_or_expand` failing"); } - return ExpandResult::only_err(err.unwrap_or_else(|| { + return ExpandResult::only_err(error.unwrap_or_else(|| { ExpandError::Other("failed to parse macro invocation".into()) })); } }; - ExpandResult { value: Some((file_id, raw_node)), err } + ExpandResult { value: Some(InFile::new(file_id, parse)), err: error } } pub fn exit(&mut self, db: &dyn DefDatabase, mut mark: Mark) { @@ -259,7 +259,7 @@ impl Expander { &mut self, db: &dyn DefDatabase, op: F, - ) -> ExpandResult> + ) -> ExpandResult)>> where F: FnOnce(&mut Self) -> ExpandResult>, { @@ -286,15 +286,15 @@ impl Expander { }; Self::enter_expand_inner(db, call_id, err).map(|value| { - value.and_then(|(new_file_id, node)| { - let node = T::cast(node)?; + value.and_then(|InFile { file_id, value }| { + let parse = value.cast::()?; self.recursion_depth += 1; - self.cfg_expander.hygiene = Hygiene::new(db.upcast(), new_file_id); - let old_file_id = std::mem::replace(&mut self.current_file_id, new_file_id); + self.cfg_expander.hygiene = Hygiene::new(db.upcast(), file_id); + let old_file_id = std::mem::replace(&mut self.current_file_id, file_id); let mark = Mark { file_id: old_file_id, bomb: DropBomb::new("expansion mark dropped") }; - Some((mark, node)) + Some((mark, parse)) }) }) } diff --git a/crates/hir-def/src/body/lower.rs b/crates/hir-def/src/body/lower.rs index b5487dda1b..db619b97db 100644 --- a/crates/hir-def/src/body/lower.rs +++ b/crates/hir-def/src/body/lower.rs @@ -824,7 +824,11 @@ impl ExprCollector<'_> { self.db.ast_id_map(self.expander.current_file_id), ); - let id = collector(self, Some(expansion)); + if record_diagnostics { + // FIXME: Report parse errors here + } + + let id = collector(self, Some(expansion.tree())); self.ast_id_map = prev_ast_id_map; self.expander.exit(self.db, mark); id diff --git a/crates/hir-def/src/data.rs b/crates/hir-def/src/data.rs index 6d2c88660f..95581727ad 100644 --- a/crates/hir-def/src/data.rs +++ b/crates/hir-def/src/data.rs @@ -7,7 +7,7 @@ use std::sync::Arc; use hir_expand::{name::Name, AstId, ExpandResult, HirFileId, InFile, MacroCallId, MacroDefKind}; use intern::Interned; use smallvec::SmallVec; -use syntax::ast; +use syntax::{ast, Parse}; use crate::{ attr::Attrs, @@ -604,13 +604,10 @@ impl<'a> AssocItemCollector<'a> { continue 'attrs; } } - match self.expander.enter_expand_id::(self.db, call_id) { - ExpandResult { value: Some((mark, _)), .. } => { - self.collect_macro_items(mark); - continue 'items; - } - ExpandResult { .. } => {} - } + + let res = self.expander.enter_expand_id::(self.db, call_id); + self.collect_macro_items(res, &|| loc.kind.clone()); + continue 'items; } } @@ -641,22 +638,24 @@ impl<'a> AssocItemCollector<'a> { self.items.push((item.name.clone(), def.into())); } AssocItem::MacroCall(call) => { - if let Some(root) = - self.db.parse_or_expand_with_err(self.expander.current_file_id()) - { - // FIXME: report parse errors - let root = root.syntax_node(); - + // TODO: These are the wrong errors to report, report in collect_macro_items instead + let file_id = self.expander.current_file_id(); + let root = self.db.parse_or_expand(file_id); + if let Some(root) = root { let call = &item_tree[call]; - let ast_id_map = self.db.ast_id_map(self.expander.current_file_id()); - let call = ast_id_map.get(call.ast_id).to_node(&root); - let _cx = - stdx::panic_context::enter(format!("collect_items MacroCall: {call}")); - let res = self.expander.enter_expand::(self.db, call); - - if let Ok(ExpandResult { value: Some((mark, _)), .. }) = res { - self.collect_macro_items(mark); + let ast_id_map = self.db.ast_id_map(file_id); + let macro_call = ast_id_map.get(call.ast_id).to_node(&root); + let _cx = stdx::panic_context::enter(format!( + "collect_items MacroCall: {macro_call}" + )); + if let Ok(res) = + self.expander.enter_expand::(self.db, macro_call) + { + self.collect_macro_items(res, &|| hir_expand::MacroCallKind::FnLike { + ast_id: InFile::new(file_id, call.ast_id), + expand_to: hir_expand::ExpandTo::Items, + }); } } } @@ -664,7 +663,28 @@ impl<'a> AssocItemCollector<'a> { } } - fn collect_macro_items(&mut self, mark: Mark) { + fn collect_macro_items( + &mut self, + ExpandResult { value, err }: ExpandResult)>>, + error_call_kind: &dyn Fn() -> hir_expand::MacroCallKind, + ) { + let Some((mark, parse)) = value else { return }; + + if let Some(err) = err { + self.inactive_diagnostics.push(DefDiagnostic::macro_error( + self.module_id.local_id, + error_call_kind(), + err.to_string(), + )); + } + if let errors @ [_, ..] = parse.errors() { + self.inactive_diagnostics.push(DefDiagnostic::macro_expansion_parse_error( + self.module_id.local_id, + error_call_kind(), + errors.into(), + )); + } + let tree_id = item_tree::TreeId::new(self.expander.current_file_id(), None); let item_tree = tree_id.item_tree(self.db); let iter: SmallVec<[_; 2]> = diff --git a/crates/hir-def/src/generics.rs b/crates/hir-def/src/generics.rs index 30edaed109..c1e20d657b 100644 --- a/crates/hir-def/src/generics.rs +++ b/crates/hir-def/src/generics.rs @@ -350,7 +350,7 @@ impl GenericParams { match expander.enter_expand::(db, macro_call) { Ok(ExpandResult { value: Some((mark, expanded)), .. }) => { let ctx = expander.ctx(db); - let type_ref = TypeRef::from_ast(&ctx, expanded); + let type_ref = TypeRef::from_ast(&ctx, expanded.tree()); self.fill_implicit_impl_trait_args(db, expander, &type_ref); expander.exit(db, mark); } diff --git a/crates/hir-def/src/nameres/diagnostics.rs b/crates/hir-def/src/nameres/diagnostics.rs index a57896e546..6b922b5785 100644 --- a/crates/hir-def/src/nameres/diagnostics.rs +++ b/crates/hir-def/src/nameres/diagnostics.rs @@ -4,7 +4,10 @@ use base_db::CrateId; use cfg::{CfgExpr, CfgOptions}; use hir_expand::{attrs::AttrId, MacroCallKind}; use la_arena::Idx; -use syntax::ast::{self, AnyHasAttrs}; +use syntax::{ + ast::{self, AnyHasAttrs}, + SyntaxError, +}; use crate::{ item_tree::{self, ItemTreeId}, @@ -29,6 +32,8 @@ pub enum DefDiagnosticKind { MacroError { ast: MacroCallKind, message: String }, + MacroExpansionParseError { ast: MacroCallKind, errors: Box<[SyntaxError]> }, + UnimplementedBuiltinMacro { ast: AstId }, InvalidDeriveTarget { ast: AstId, id: usize }, @@ -91,7 +96,7 @@ impl DefDiagnostic { Self { in_module: container, kind: DefDiagnosticKind::UnresolvedProcMacro { ast, krate } } } - pub(super) fn macro_error( + pub(crate) fn macro_error( container: LocalModuleId, ast: MacroCallKind, message: String, @@ -99,6 +104,20 @@ impl DefDiagnostic { Self { in_module: container, kind: DefDiagnosticKind::MacroError { ast, message } } } + pub(crate) fn macro_expansion_parse_error( + container: LocalModuleId, + ast: MacroCallKind, + errors: &[SyntaxError], + ) -> Self { + Self { + in_module: container, + kind: DefDiagnosticKind::MacroExpansionParseError { + ast, + errors: errors.to_vec().into_boxed_slice(), + }, + } + } + pub(super) fn unresolved_macro_call( container: LocalModuleId, ast: MacroCallKind, diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs index e8fba15601..1749698387 100644 --- a/crates/hir-expand/src/db.rs +++ b/crates/hir-expand/src/db.rs @@ -100,7 +100,10 @@ pub trait ExpandDatabase: SourceDatabase { #[salsa::transparent] fn parse_or_expand(&self, file_id: HirFileId) -> Option; #[salsa::transparent] - fn parse_or_expand_with_err(&self, file_id: HirFileId) -> Option>; + fn parse_or_expand_with_err( + &self, + file_id: HirFileId, + ) -> ExpandResult>>; /// Implementation for the macro case. fn parse_macro_expansion( &self, @@ -262,11 +265,11 @@ fn parse_or_expand(db: &dyn ExpandDatabase, file_id: HirFileId) -> Option Option> { +) -> ExpandResult>> { match file_id.repr() { - HirFileIdRepr::FileId(file_id) => Some(db.parse(file_id).to_syntax()), + HirFileIdRepr::FileId(file_id) => ExpandResult::ok(Some(db.parse(file_id).to_syntax())), HirFileIdRepr::MacroFile(macro_file) => { - db.parse_macro_expansion(macro_file).value.map(|(parse, _)| parse) + db.parse_macro_expansion(macro_file).map(|it| it.map(|(parse, _)| parse)) } } } diff --git a/crates/hir-expand/src/eager.rs b/crates/hir-expand/src/eager.rs index 7d00b682a9..84f391316c 100644 --- a/crates/hir-expand/src/eager.rs +++ b/crates/hir-expand/src/eager.rs @@ -21,7 +21,7 @@ use std::sync::Arc; use base_db::CrateId; -use syntax::{ted, SyntaxNode}; +use syntax::{ted, Parse, SyntaxNode}; use crate::{ ast::{self, AstNode}, @@ -111,7 +111,7 @@ fn lazy_expand( def: &MacroDefId, macro_call: InFile, krate: CrateId, -) -> ExpandResult>> { +) -> ExpandResult>>> { let ast_id = db.ast_id_map(macro_call.file_id).ast_id(¯o_call.value); let expand_to = ExpandTo::from_call_site(¯o_call.value); @@ -121,13 +121,8 @@ fn lazy_expand( MacroCallKind::FnLike { ast_id: macro_call.with_value(ast_id), expand_to }, ); - let err = db.macro_expand_error(id); - let value = - db.parse_or_expand_with_err(id.as_file()).map(|node| InFile::new(id.as_file(), node)); - // FIXME: report parse errors - let value = value.map(|it| it.map(|it| it.syntax_node())); - - ExpandResult { value, err } + db.parse_or_expand_with_err(id.as_file()) + .map(|parse| parse.map(|parse| InFile::new(id.as_file(), parse))) } fn eager_macro_recur( @@ -183,8 +178,14 @@ fn eager_macro_recur( Some(val) => { // replace macro inside let hygiene = Hygiene::new(db, val.file_id); - let ExpandResult { value, err: error } = - eager_macro_recur(db, &hygiene, val, krate, macro_resolver)?; + let ExpandResult { value, err: error } = eager_macro_recur( + db, + &hygiene, + // FIXME: We discard parse errors here + val.map(|it| it.syntax_node()), + krate, + macro_resolver, + )?; let err = if err.is_none() { error } else { err }; ExpandResult { value, err } } diff --git a/crates/hir-ty/src/lower.rs b/crates/hir-ty/src/lower.rs index b37fe1d63d..33dc5e2d69 100644 --- a/crates/hir-ty/src/lower.rs +++ b/crates/hir-ty/src/lower.rs @@ -381,7 +381,8 @@ impl<'a> TyLoweringContext<'a> { match expander.enter_expand::(self.db.upcast(), macro_call) { Ok(ExpandResult { value: Some((mark, expanded)), .. }) => { let ctx = expander.ctx(self.db.upcast()); - let type_ref = TypeRef::from_ast(&ctx, expanded); + // FIXME: Report syntax errors in expansion here + let type_ref = TypeRef::from_ast(&ctx, expanded.tree()); drop(expander); let ty = self.lower_ty(&type_ref); diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index b735decfcb..f81f8b0b01 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -10,7 +10,7 @@ use cfg::{CfgExpr, CfgOptions}; use either::Either; use hir_def::path::ModPath; use hir_expand::{name::Name, HirFileId, InFile}; -use syntax::{ast, AstPtr, SyntaxNodePtr, TextRange}; +use syntax::{ast, AstPtr, SyntaxError, SyntaxNodePtr, TextRange}; use crate::{AssocItem, Field, Local, MacroKind, Type}; @@ -38,8 +38,9 @@ diagnostics![ IncorrectCase, InvalidDeriveTarget, IncoherentImpl, - MacroError, MacroDefError, + MacroError, + MacroExpansionParseError, MalformedDerive, MismatchedArgCount, MissingFields, @@ -132,6 +133,13 @@ pub struct MacroError { pub message: String, } +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct MacroExpansionParseError { + pub node: InFile, + pub precise_location: Option, + pub errors: Box<[SyntaxError]>, +} + #[derive(Debug, Clone, Eq, PartialEq)] pub struct MacroDefError { pub node: InFile>, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 7e9b89db7a..3adb484b12 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -87,12 +87,12 @@ pub use crate::{ attrs::{HasAttrs, Namespace}, diagnostics::{ AnyDiagnostic, BreakOutsideOfLoop, ExpectedFunction, InactiveCode, IncoherentImpl, - IncorrectCase, InvalidDeriveTarget, MacroDefError, MacroError, MalformedDerive, - MismatchedArgCount, MissingFields, MissingMatchArms, MissingUnsafe, NeedMut, NoSuchField, - PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, - UndeclaredLabel, UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate, - UnresolvedField, UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall, - UnresolvedModule, UnresolvedProcMacro, UnusedMut, + IncorrectCase, InvalidDeriveTarget, MacroDefError, MacroError, MacroExpansionParseError, + MalformedDerive, MismatchedArgCount, MissingFields, MissingMatchArms, MissingUnsafe, + NeedMut, NoSuchField, PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, + TypeMismatch, UndeclaredLabel, UnimplementedBuiltinMacro, UnreachableLabel, + UnresolvedExternCrate, UnresolvedField, UnresolvedImport, UnresolvedMacroCall, + UnresolvedMethodCall, UnresolvedModule, UnresolvedProcMacro, UnusedMut, }, has_source::HasSource, semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo, VisibleTraits}, @@ -753,7 +753,6 @@ fn emit_def_diagnostic_( .into(), ); } - DefDiagnosticKind::UnresolvedProcMacro { ast, krate } => { let (node, precise_location, macro_name, kind) = precise_macro_call_location(ast, db); acc.push( @@ -761,7 +760,6 @@ fn emit_def_diagnostic_( .into(), ); } - DefDiagnosticKind::UnresolvedMacroCall { ast, path } => { let (node, precise_location, _, _) = precise_macro_call_location(ast, db); acc.push( @@ -774,12 +772,16 @@ fn emit_def_diagnostic_( .into(), ); } - DefDiagnosticKind::MacroError { ast, message } => { let (node, precise_location, _, _) = precise_macro_call_location(ast, db); acc.push(MacroError { node, precise_location, message: message.clone() }.into()); } - + DefDiagnosticKind::MacroExpansionParseError { ast, errors } => { + let (node, precise_location, _, _) = precise_macro_call_location(ast, db); + acc.push( + MacroExpansionParseError { node, precise_location, errors: errors.clone() }.into(), + ); + } DefDiagnosticKind::UnimplementedBuiltinMacro { ast } => { let node = ast.to_node(db.upcast()); // Must have a name, otherwise we wouldn't emit it. diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index 59976ecf29..7c8cb7a447 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/crates/ide-diagnostics/src/lib.rs @@ -265,6 +265,19 @@ pub fn diagnostics( AnyDiagnostic::InvalidDeriveTarget(d) => handlers::invalid_derive_target::invalid_derive_target(&ctx, &d), AnyDiagnostic::MacroDefError(d) => handlers::macro_error::macro_def_error(&ctx, &d), AnyDiagnostic::MacroError(d) => handlers::macro_error::macro_error(&ctx, &d), + AnyDiagnostic::MacroExpansionParseError(d) => { + res.extend(d.errors.iter().take(32).map(|err| { + { + Diagnostic::new( + "syntax-error", + format!("Syntax Error in Expansion: {err}"), + ctx.resolve_precise_location(&d.node.clone(), d.precise_location), + ) + } + .experimental() + })); + continue; + }, AnyDiagnostic::MalformedDerive(d) => handlers::malformed_derive::malformed_derive(&ctx, &d), AnyDiagnostic::MismatchedArgCount(d) => handlers::mismatched_arg_count::mismatched_arg_count(&ctx, &d), AnyDiagnostic::MissingFields(d) => handlers::missing_fields::missing_fields(&ctx, &d), From 0f4ffaa5afac3d5df27905cbab4630de4d8556ed Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 16 Apr 2023 18:29:42 +0200 Subject: [PATCH 4/4] Fix duplicate eager expansion errors --- crates/hir-def/src/data.rs | 1 - crates/hir-def/src/item_tree.rs | 1 + crates/hir-def/src/lib.rs | 12 +++- crates/hir-def/src/nameres/collector.rs | 55 +++++++++---------- .../hir-def/src/nameres/tests/incremental.rs | 2 +- crates/hir-expand/src/db.rs | 20 +++++-- crates/hir/src/db.rs | 4 +- crates/ide-db/src/apply_change.rs | 1 - crates/ide-db/src/lib.rs | 8 --- .../src/handlers/macro_error.rs | 16 ++++++ 10 files changed, 70 insertions(+), 50 deletions(-) diff --git a/crates/hir-def/src/data.rs b/crates/hir-def/src/data.rs index 95581727ad..668b436e01 100644 --- a/crates/hir-def/src/data.rs +++ b/crates/hir-def/src/data.rs @@ -638,7 +638,6 @@ impl<'a> AssocItemCollector<'a> { self.items.push((item.name.clone(), def.into())); } AssocItem::MacroCall(call) => { - // TODO: These are the wrong errors to report, report in collect_macro_items instead let file_id = self.expander.current_file_id(); let root = self.db.parse_or_expand(file_id); if let Some(root) = root { diff --git a/crates/hir-def/src/item_tree.rs b/crates/hir-def/src/item_tree.rs index 8546d36d79..48c1baf308 100644 --- a/crates/hir-def/src/item_tree.rs +++ b/crates/hir-def/src/item_tree.rs @@ -101,6 +101,7 @@ pub struct ItemTree { top_level: SmallVec<[ModItem; 1]>, attrs: FxHashMap, + // FIXME: Remove this indirection, an item tree is almost always non-empty? data: Option>, } diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs index 55e1e91220..34d704942a 100644 --- a/crates/hir-def/src/lib.rs +++ b/crates/hir-def/src/lib.rs @@ -823,7 +823,7 @@ impl AsMacroCall for InFile<&ast::MacroCall> { return Ok(ExpandResult::only_err(ExpandError::Other("malformed macro invocation".into()))); }; - macro_call_as_call_id( + macro_call_as_call_id_( db, &AstIdWithPath::new(ast_id.file_id, ast_id.value, path), expands_to, @@ -852,6 +852,16 @@ fn macro_call_as_call_id( expand_to: ExpandTo, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, +) -> Result, UnresolvedMacro> { + macro_call_as_call_id_(db, call, expand_to, krate, resolver).map(|res| res.value) +} + +fn macro_call_as_call_id_( + db: &dyn db::DefDatabase, + call: &AstIdWithPath, + expand_to: ExpandTo, + krate: CrateId, + resolver: impl Fn(path::ModPath) -> Option, ) -> Result>, UnresolvedMacro> { let def = resolver(call.path.clone()).ok_or_else(|| UnresolvedMacro { path: call.path.clone() })?; diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index 8ab0b3dbd1..6592c6b902 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/crates/hir-def/src/nameres/collector.rs @@ -1117,8 +1117,9 @@ impl DefCollector<'_> { self.def_map.krate, resolver_def_id, ); - if let Ok(ExpandResult { value: Some(call_id), .. }) = call_id { + if let Ok(Some(call_id)) = call_id { push_resolved(directive, call_id); + res = ReachedFixedPoint::No; return false; } @@ -1354,26 +1355,30 @@ impl DefCollector<'_> { let file_id = macro_call_id.as_file(); // First, fetch the raw expansion result for purposes of error reporting. This goes through - // `macro_expand_error` to avoid depending on the full expansion result (to improve + // `parse_macro_expansion_error` to avoid depending on the full expansion result (to improve // incrementality). - let loc: MacroCallLoc = self.db.lookup_intern_macro_call(macro_call_id); - let err = self.db.macro_expand_error(macro_call_id); + let ExpandResult { value, err } = self.db.parse_macro_expansion_error(macro_call_id); if let Some(err) = err { + let loc: MacroCallLoc = self.db.lookup_intern_macro_call(macro_call_id); let diag = match err { + // why is this reported here? hir_expand::ExpandError::UnresolvedProcMacro(krate) => { always!(krate == loc.def.krate); - // Missing proc macros are non-fatal, so they are handled specially. DefDiagnostic::unresolved_proc_macro(module_id, loc.kind.clone(), loc.def.krate) } - _ => DefDiagnostic::macro_error(module_id, loc.kind, err.to_string()), + _ => DefDiagnostic::macro_error(module_id, loc.kind.clone(), err.to_string()), }; self.def_map.diagnostics.push(diag); } + if let Some(errors) = value { + let loc: MacroCallLoc = self.db.lookup_intern_macro_call(macro_call_id); + let diag = DefDiagnostic::macro_expansion_parse_error(module_id, loc.kind, &errors); + self.def_map.diagnostics.push(diag); + } // Then, fetch and process the item tree. This will reuse the expansion result from above. let item_tree = self.db.file_item_tree(file_id); - // FIXME: report parse errors for the macro expansion here let mod_dir = self.mod_dirs[&module_id].clone(); ModCollector { @@ -1395,6 +1400,7 @@ impl DefCollector<'_> { for directive in &self.unresolved_macros { match &directive.kind { MacroDirectiveKind::FnLike { ast_id, expand_to } => { + // FIXME: we shouldn't need to re-resolve the macro here just to get the unresolved error! let macro_call_as_call_id = macro_call_as_call_id( self.db, ast_id, @@ -2110,7 +2116,7 @@ impl ModCollector<'_, '_> { let ast_id = AstIdWithPath::new(self.file_id(), mac.ast_id, ModPath::clone(&mac.path)); // Case 1: try to resolve in legacy scope and expand macro_rules - match macro_call_as_call_id( + if let Ok(res) = macro_call_as_call_id( self.def_collector.db, &ast_id, mac.expand_to, @@ -2131,29 +2137,18 @@ impl ModCollector<'_, '_> { }) }, ) { - Ok(res) => { - // Legacy macros need to be expanded immediately, so that any macros they produce - // are in scope. - if let Some(val) = res.value { - self.def_collector.collect_macro_expansion( - self.module_id, - val, - self.macro_depth + 1, - container, - ); - } - - if let Some(err) = res.err { - self.def_collector.def_map.diagnostics.push(DefDiagnostic::macro_error( - self.module_id, - MacroCallKind::FnLike { ast_id: ast_id.ast_id, expand_to: mac.expand_to }, - err.to_string(), - )); - } - - return; + // Legacy macros need to be expanded immediately, so that any macros they produce + // are in scope. + if let Some(val) = res { + self.def_collector.collect_macro_expansion( + self.module_id, + val, + self.macro_depth + 1, + container, + ); } - Err(UnresolvedMacro { .. }) => (), + + return; } // Case 2: resolve in module scope, expand during name resolution. diff --git a/crates/hir-def/src/nameres/tests/incremental.rs b/crates/hir-def/src/nameres/tests/incremental.rs index 13e6825f82..b07462cde0 100644 --- a/crates/hir-def/src/nameres/tests/incremental.rs +++ b/crates/hir-def/src/nameres/tests/incremental.rs @@ -140,7 +140,7 @@ m!(Z); let n_recalculated_item_trees = events.iter().filter(|it| it.contains("item_tree")).count(); assert_eq!(n_recalculated_item_trees, 6); let n_reparsed_macros = - events.iter().filter(|it| it.contains("parse_macro_expansion")).count(); + events.iter().filter(|it| it.contains("parse_macro_expansion(")).count(); assert_eq!(n_reparsed_macros, 3); } diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs index 1749698387..afc2be0741 100644 --- a/crates/hir-expand/src/db.rs +++ b/crates/hir-expand/src/db.rs @@ -9,7 +9,7 @@ use mbe::syntax_node_to_token_tree; use rustc_hash::FxHashSet; use syntax::{ ast::{self, HasAttrs, HasDocComments}, - AstNode, GreenNode, Parse, SyntaxNode, SyntaxToken, T, + AstNode, GreenNode, Parse, SyntaxError, SyntaxNode, SyntaxToken, T, }; use crate::{ @@ -132,15 +132,18 @@ pub trait ExpandDatabase: SourceDatabase { /// just fetches procedural ones. fn macro_def(&self, id: MacroDefId) -> Result, mbe::ParseError>; - /// Expand macro call to a token tree. This query is LRUed (we keep 128 or so results in memory) + /// Expand macro call to a token tree. fn macro_expand(&self, macro_call: MacroCallId) -> ExpandResult>>; /// Special case of the previous query for procedural macros. We can't LRU /// proc macros, since they are not deterministic in general, and /// non-determinism breaks salsa in a very, very, very bad way. @edwin0cheng /// heroically debugged this once! fn expand_proc_macro(&self, call: MacroCallId) -> ExpandResult; - /// Firewall query that returns the error from the `macro_expand` query. - fn macro_expand_error(&self, macro_call: MacroCallId) -> Option; + /// Firewall query that returns the errors from the `parse_macro_expansion` query. + fn parse_macro_expansion_error( + &self, + macro_call: MacroCallId, + ) -> ExpandResult>>; fn hygiene_frame(&self, file_id: HirFileId) -> Arc; } @@ -448,6 +451,7 @@ fn macro_def( fn macro_expand( db: &dyn ExpandDatabase, id: MacroCallId, + // FIXME: Remove the OPtion if possible ) -> ExpandResult>> { let _p = profile::span("macro_expand"); let loc: MacroCallLoc = db.lookup_intern_macro_call(id); @@ -498,8 +502,12 @@ fn macro_expand( ExpandResult { value: Some(Arc::new(tt)), err } } -fn macro_expand_error(db: &dyn ExpandDatabase, macro_call: MacroCallId) -> Option { - db.macro_expand(macro_call).err +fn parse_macro_expansion_error( + db: &dyn ExpandDatabase, + macro_call_id: MacroCallId, +) -> ExpandResult>> { + db.parse_macro_expansion(MacroFile { macro_call_id }) + .map(|it| it.map(|(it, _)| it.errors().to_vec().into_boxed_slice())) } fn expand_proc_macro(db: &dyn ExpandDatabase, id: MacroCallId) -> ExpandResult { diff --git a/crates/hir/src/db.rs b/crates/hir/src/db.rs index 0935b5ea51..7ec27af04b 100644 --- a/crates/hir/src/db.rs +++ b/crates/hir/src/db.rs @@ -6,8 +6,8 @@ pub use hir_def::db::*; pub use hir_expand::db::{ AstIdMapQuery, ExpandDatabase, ExpandDatabaseStorage, ExpandProcMacroQuery, HygieneFrameQuery, - InternMacroCallQuery, MacroArgTextQuery, MacroDefQuery, MacroExpandErrorQuery, - MacroExpandQuery, ParseMacroExpansionQuery, + InternMacroCallQuery, MacroArgTextQuery, MacroDefQuery, MacroExpandQuery, + ParseMacroExpansionQuery, }; pub use hir_ty::db::*; diff --git a/crates/ide-db/src/apply_change.rs b/crates/ide-db/src/apply_change.rs index 3b8458980c..8d14371d03 100644 --- a/crates/ide-db/src/apply_change.rs +++ b/crates/ide-db/src/apply_change.rs @@ -80,7 +80,6 @@ impl RootDatabase { hir::db::MacroDefQuery hir::db::MacroExpandQuery hir::db::ExpandProcMacroQuery - hir::db::MacroExpandErrorQuery hir::db::HygieneFrameQuery // DefDatabase diff --git a/crates/ide-db/src/lib.rs b/crates/ide-db/src/lib.rs index 11b2a5e1c2..12b6e3c4bb 100644 --- a/crates/ide-db/src/lib.rs +++ b/crates/ide-db/src/lib.rs @@ -152,7 +152,6 @@ impl RootDatabase { let lru_capacity = lru_capacity.unwrap_or(base_db::DEFAULT_LRU_CAP); base_db::ParseQuery.in_db_mut(self).set_lru_capacity(lru_capacity); hir::db::ParseMacroExpansionQuery.in_db_mut(self).set_lru_capacity(lru_capacity); - hir::db::MacroExpandQuery.in_db_mut(self).set_lru_capacity(lru_capacity); } pub fn update_lru_capacities(&mut self, lru_capacities: &FxHashMap, usize>) { @@ -167,12 +166,6 @@ impl RootDatabase { .copied() .unwrap_or(base_db::DEFAULT_LRU_CAP), ); - hir_db::MacroExpandQuery.in_db_mut(self).set_lru_capacity( - lru_capacities - .get(stringify!(MacroExpandQuery)) - .copied() - .unwrap_or(base_db::DEFAULT_LRU_CAP), - ); macro_rules! update_lru_capacity_per_query { ($( $module:ident :: $query:ident )*) => {$( @@ -201,7 +194,6 @@ impl RootDatabase { hir_db::MacroDefQuery // hir_db::MacroExpandQuery hir_db::ExpandProcMacroQuery - hir_db::MacroExpandErrorQuery hir_db::HygieneFrameQuery // DefDatabase diff --git a/crates/ide-diagnostics/src/handlers/macro_error.rs b/crates/ide-diagnostics/src/handlers/macro_error.rs index af74015cf9..7547779a95 100644 --- a/crates/ide-diagnostics/src/handlers/macro_error.rs +++ b/crates/ide-diagnostics/src/handlers/macro_error.rs @@ -238,6 +238,22 @@ fn f() { //^^^ error: invalid macro definition: expected subtree } +"#, + ) + } + + #[test] + fn expansion_syntax_diagnostic() { + check_diagnostics( + r#" +macro_rules! foo { + () => { struct; }; +} + +fn f() { + foo!(); + //^^^ error: Syntax Error in Expansion: expected a name +} "#, ) }