From 6ae8d49e1554cbf99387ed83079277f5f854d187 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 16 Apr 2023 15:46:12 +0200 Subject: [PATCH] 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 }) }