From a6342436343e01b32d4482a48994b8c22bcbe659 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 2 Dec 2020 16:52:14 +0100 Subject: [PATCH 1/5] Propagate eager expansion errors --- crates/hir_def/src/body.rs | 20 ++++-- crates/hir_def/src/lib.rs | 57 ++++++++++++---- crates/hir_expand/src/eager.rs | 115 ++++++++++++++++++++++++++++----- 3 files changed, 156 insertions(+), 36 deletions(-) diff --git a/crates/hir_def/src/body.rs b/crates/hir_def/src/body.rs index 33eb5e78c44..92bcc17053d 100644 --- a/crates/hir_def/src/body.rs +++ b/crates/hir_def/src/body.rs @@ -120,18 +120,24 @@ impl Expander { self.resolve_path_as_macro(db, &path) }; - let call_id = match macro_call.as_call_id(db, self.crate_def_map.krate, resolver) { + let mut err = None; + let call_id = + macro_call.as_call_id_with_errors(db, self.crate_def_map.krate, resolver, &mut |e| { + err.get_or_insert(e); + }); + let call_id = match call_id { Some(it) => it, None => { - // FIXME: this can mean other things too, but `as_call_id` doesn't provide enough - // info. - return ExpandResult::only_err(mbe::ExpandError::Other( - "failed to parse or resolve macro invocation".into(), - )); + if err.is_none() { + eprintln!("no error despite `as_call_id_with_errors` returning `None`"); + } + return ExpandResult { value: None, err }; } }; - let err = db.macro_expand_error(call_id); + if err.is_none() { + err = db.macro_expand_error(call_id); + } let file_id = call_id.as_file(); diff --git a/crates/hir_def/src/lib.rs b/crates/hir_def/src/lib.rs index 1b22d1eec9c..ce2be8e2b9b 100644 --- a/crates/hir_def/src/lib.rs +++ b/crates/hir_def/src/lib.rs @@ -465,21 +465,37 @@ pub trait AsMacroCall { db: &dyn db::DefDatabase, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, - ) -> Option; -} + ) -> Option { + self.as_call_id_with_errors(db, krate, resolver, &mut |_| ()) + } -impl AsMacroCall for InFile<&ast::MacroCall> { - fn as_call_id( + fn as_call_id_with_errors( &self, db: &dyn db::DefDatabase, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, + error_sink: &mut dyn FnMut(mbe::ExpandError), + ) -> Option; +} + +impl AsMacroCall for InFile<&ast::MacroCall> { + fn as_call_id_with_errors( + &self, + db: &dyn db::DefDatabase, + krate: CrateId, + resolver: impl Fn(path::ModPath) -> Option, + error_sink: &mut dyn FnMut(mbe::ExpandError), ) -> Option { 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 = path::ModPath::from_src(self.value.path()?, &h)?; + let path = self.value.path().and_then(|path| path::ModPath::from_src(path, &h)); - AstIdWithPath::new(ast_id.file_id, ast_id.value, path).as_call_id(db, krate, resolver) + if path.is_none() { + error_sink(mbe::ExpandError::Other("malformed macro invocation".into())); + } + + AstIdWithPath::new(ast_id.file_id, ast_id.value, path?) + .as_call_id_with_errors(db, krate, resolver, error_sink) } } @@ -497,22 +513,32 @@ impl AstIdWithPath { } impl AsMacroCall for AstIdWithPath { - fn as_call_id( + fn as_call_id_with_errors( &self, db: &dyn db::DefDatabase, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, + error_sink: &mut dyn FnMut(mbe::ExpandError), ) -> Option { - let def: MacroDefId = resolver(self.path.clone())?; + let def: MacroDefId = resolver(self.path.clone()).or_else(|| { + error_sink(mbe::ExpandError::Other("could not resolve macro".into())); + None + })?; if let MacroDefKind::BuiltInEager(_) = def.kind { let macro_call = InFile::new(self.ast_id.file_id, self.ast_id.to_node(db.upcast())); let hygiene = Hygiene::new(db.upcast(), self.ast_id.file_id); Some( - expand_eager_macro(db.upcast(), krate, macro_call, def, &|path: ast::Path| { - resolver(path::ModPath::from_src(path, &hygiene)?) - })? + expand_eager_macro( + db.upcast(), + krate, + macro_call, + def, + &|path: ast::Path| resolver(path::ModPath::from_src(path, &hygiene)?), + error_sink, + ) + .ok()? .into(), ) } else { @@ -522,13 +548,18 @@ impl AsMacroCall for AstIdWithPath { } impl AsMacroCall for AstIdWithPath { - fn as_call_id( + fn as_call_id_with_errors( &self, db: &dyn db::DefDatabase, krate: CrateId, resolver: impl Fn(path::ModPath) -> Option, + error_sink: &mut dyn FnMut(mbe::ExpandError), ) -> Option { - let def = resolver(self.path.clone())?; + let def: MacroDefId = resolver(self.path.clone()).or_else(|| { + error_sink(mbe::ExpandError::Other("could not resolve macro".into())); + None + })?; + Some( def.as_lazy_macro( db.upcast(), diff --git a/crates/hir_expand/src/eager.rs b/crates/hir_expand/src/eager.rs index ab6b4477c6a..a48d08f3b74 100644 --- a/crates/hir_expand/src/eager.rs +++ b/crates/hir_expand/src/eager.rs @@ -26,19 +26,89 @@ use crate::{ }; use base_db::CrateId; +use mbe::ExpandResult; use parser::FragmentKind; use std::sync::Arc; use syntax::{algo::SyntaxRewriter, SyntaxNode}; +pub struct ErrorEmitted { + _private: (), +} + +trait ErrorSink { + fn emit(&mut self, err: mbe::ExpandError); + + fn option( + &mut self, + opt: Option, + error: impl FnOnce() -> mbe::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() -> mbe::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(mbe::ExpandError) { + fn emit(&mut self, err: mbe::ExpandError) { + self(err); + } +} + +fn err(msg: impl Into) -> mbe::ExpandError { + mbe::ExpandError::Other(msg.into()) +} + pub fn expand_eager_macro( db: &dyn AstDatabase, krate: CrateId, macro_call: InFile, def: MacroDefId, resolver: &dyn Fn(ast::Path) -> Option, -) -> Option { - let args = macro_call.value.token_tree()?; - let parsed_args = mbe::ast_to_token_tree(&args)?.0; + mut error_sink: &mut dyn FnMut(mbe::ExpandError), +) -> Result { + let parsed_args = error_sink.option_with( + || Some(mbe::ast_to_token_tree(¯o_call.value.token_tree()?)?.0), + || err("malformed macro invocation"), + )?; // Note: // When `lazy_expand` is called, its *parent* file must be already exists. @@ -55,17 +125,21 @@ pub fn expand_eager_macro( }); let arg_file_id: MacroCallId = arg_id.into(); - let parsed_args = mbe::token_tree_to_syntax_node(&parsed_args, FragmentKind::Expr).ok()?.0; + let parsed_args = + error_sink.result(mbe::token_tree_to_syntax_node(&parsed_args, FragmentKind::Expr))?.0; let result = eager_macro_recur( db, InFile::new(arg_file_id.as_file(), parsed_args.syntax_node()), krate, resolver, + error_sink, )?; - let subtree = to_subtree(&result)?; + let subtree = error_sink.option(to_subtree(&result), || err("failed to parse macro result"))?; if let MacroDefKind::BuiltInEager(eager) = def.kind { - let (subtree, fragment) = eager.expand(db, arg_id, &subtree).value?; + let res = eager.expand(db, arg_id, &subtree); + + let (subtree, fragment) = error_sink.expand_result_option(res)?; let eager = EagerCallLoc { def, fragment, @@ -74,9 +148,9 @@ pub fn expand_eager_macro( file_id: macro_call.file_id, }; - Some(db.intern_eager_expansion(eager)) + Ok(db.intern_eager_expansion(eager)) } else { - None + panic!("called `expand_eager_macro` on non-eager macro def {:?}", def); } } @@ -91,13 +165,16 @@ fn lazy_expand( def: &MacroDefId, macro_call: InFile, krate: CrateId, -) -> Option> { +) -> ExpandResult>> { let ast_id = db.ast_id_map(macro_call.file_id).ast_id(¯o_call.value); let id: MacroCallId = def.as_lazy_macro(db, krate, MacroCallKind::FnLike(macro_call.with_value(ast_id))).into(); - db.parse_or_expand(id.as_file()).map(|node| InFile::new(id.as_file(), node)) + let err = db.macro_expand_error(id); + let value = db.parse_or_expand(id.as_file()).map(|node| InFile::new(id.as_file(), node)); + + ExpandResult { value, err } } fn eager_macro_recur( @@ -105,7 +182,8 @@ fn eager_macro_recur( curr: InFile, krate: CrateId, macro_resolver: &dyn Fn(ast::Path) -> Option, -) -> Option { + mut error_sink: &mut dyn FnMut(mbe::ExpandError), +) -> Result { let original = curr.value.clone(); let children = curr.value.descendants().filter_map(ast::MacroCall::cast); @@ -113,7 +191,8 @@ fn eager_macro_recur( // Collect replacement for child in children { - let def: MacroDefId = macro_resolver(child.path()?)?; + let def = error_sink + .option_with(|| macro_resolver(child.path()?), || err("failed to resolve macro"))?; let insert = match def.kind { MacroDefKind::BuiltInEager(_) => { let id: MacroCallId = expand_eager_macro( @@ -122,17 +201,21 @@ fn eager_macro_recur( curr.with_value(child.clone()), def, macro_resolver, + error_sink, )? .into(); - db.parse_or_expand(id.as_file())? + db.parse_or_expand(id.as_file()) + .expect("successful macro expansion should be parseable") } MacroDefKind::Declarative | MacroDefKind::BuiltIn(_) | MacroDefKind::BuiltInDerive(_) | MacroDefKind::ProcMacro(_) => { - let expanded = lazy_expand(db, &def, curr.with_value(child.clone()), krate)?; + let res = lazy_expand(db, &def, curr.with_value(child.clone()), krate); + let val = error_sink.expand_result_option(res)?; + // replace macro inside - eager_macro_recur(db, expanded, krate, macro_resolver)? + eager_macro_recur(db, val, krate, macro_resolver, error_sink)? } }; @@ -140,5 +223,5 @@ fn eager_macro_recur( } let res = rewriter.rewrite(&original); - Some(res) + Ok(res) } From 17542d08b4316afd899dabc6c7fc4c66f257dacb Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 2 Dec 2020 17:00:48 +0100 Subject: [PATCH 2/5] Update/Fix tests --- crates/hir_def/src/body/tests.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/crates/hir_def/src/body/tests.rs b/crates/hir_def/src/body/tests.rs index baf1179f1f4..f2b57aebe75 100644 --- a/crates/hir_def/src/body/tests.rs +++ b/crates/hir_def/src/body/tests.rs @@ -78,21 +78,32 @@ fn f() { fn macro_diag_builtin() { check_diagnostics( r#" +#[rustc_builtin_macro] +macro_rules! env {} + +#[rustc_builtin_macro] +macro_rules! include {} + +#[rustc_builtin_macro] +macro_rules! format_args { + () => {} +} + fn f() { // Test a handful of built-in (eager) macros: include!(invalid); - //^^^^^^^^^^^^^^^^^ failed to parse or resolve macro invocation + //^^^^^^^^^^^^^^^^^ could not convert tokens include!("does not exist"); - //^^^^^^^^^^^^^^^^^^^^^^^^^^ failed to parse or resolve macro invocation + //^^^^^^^^^^^^^^^^^^^^^^^^^^ could not convert tokens env!(invalid); - //^^^^^^^^^^^^^ failed to parse or resolve macro invocation + //^^^^^^^^^^^^^ could not convert tokens // Lazy: format_args!(); - //^^^^^^^^^^^^^^ failed to parse or resolve macro invocation + //^^^^^^^^^^^^^^ no rule matches input tokens } "#, ); From 4634bfb332897f8478ed885970e7cb21bb9c4fce Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Wed, 2 Dec 2020 17:03:18 +0100 Subject: [PATCH 3/5] Give better diagnostic if `OUT_DIR` is unset --- crates/hir_def/src/body/tests.rs | 3 +++ crates/hir_expand/src/builtin_macro.rs | 26 +++++++++++++++++--------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/crates/hir_def/src/body/tests.rs b/crates/hir_def/src/body/tests.rs index f2b57aebe75..c7003f2a6a3 100644 --- a/crates/hir_def/src/body/tests.rs +++ b/crates/hir_def/src/body/tests.rs @@ -100,6 +100,9 @@ fn f() { env!(invalid); //^^^^^^^^^^^^^ could not convert tokens + env!("OUT_DIR"); + //^^^^^^^^^^^^^^^ `OUT_DIR` not set, enable "load out dirs from check" to fix + // Lazy: format_args!(); diff --git a/crates/hir_expand/src/builtin_macro.rs b/crates/hir_expand/src/builtin_macro.rs index 7f4db106dba..7bb42be6cb4 100644 --- a/crates/hir_expand/src/builtin_macro.rs +++ b/crates/hir_expand/src/builtin_macro.rs @@ -417,17 +417,25 @@ fn env_expand( Err(e) => return ExpandResult::only_err(e), }; - // FIXME: - // If the environment variable is not defined int rustc, then a compilation error will be emitted. - // We might do the same if we fully support all other stuffs. - // But for now on, we should return some dummy string for better type infer purpose. - // However, we cannot use an empty string here, because for - // `include!(concat!(env!("OUT_DIR"), "/foo.rs"))` will become - // `include!("foo.rs"), which might go to infinite loop - let s = get_env_inner(db, arg_id, &key).unwrap_or_else(|| "__RA_UNIMPLEMENTED__".to_string()); + let mut err = None; + let s = get_env_inner(db, arg_id, &key).unwrap_or_else(|| { + // The only variable rust-analyzer ever sets is `OUT_DIR`, so only diagnose that to avoid + // unnecessary diagnostics for eg. `CARGO_PKG_NAME`. + if key == "OUT_DIR" { + err = Some(mbe::ExpandError::Other( + r#"`OUT_DIR` not set, enable "load out dirs from check" to fix"#.into(), + )); + } + + // If the variable is unset, still return a dummy string to help type inference along. + // We cannot use an empty string here, because for + // `include!(concat!(env!("OUT_DIR"), "/foo.rs"))` will become + // `include!("foo.rs"), which might go to infinite loop + "__RA_UNIMPLEMENTED__".to_string() + }); let expanded = quote! { #s }; - ExpandResult::ok(Some((expanded, FragmentKind::Expr))) + ExpandResult { value: Some((expanded, FragmentKind::Expr)), err } } fn option_env_expand( From 883c8d177d61d34d70d4fccef788fe4b35aaa7ea Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 3 Dec 2020 15:31:04 +0100 Subject: [PATCH 4/5] Make `compile_error!` lazy and emit a diagnostic --- crates/hir_def/src/body/tests.rs | 6 ++++ crates/hir_expand/src/builtin_macro.rs | 48 +++++++++++++++----------- crates/hir_expand/src/db.rs | 1 + 3 files changed, 34 insertions(+), 21 deletions(-) diff --git a/crates/hir_def/src/body/tests.rs b/crates/hir_def/src/body/tests.rs index c7003f2a6a3..7e78340ee4b 100644 --- a/crates/hir_def/src/body/tests.rs +++ b/crates/hir_def/src/body/tests.rs @@ -84,6 +84,9 @@ macro_rules! env {} #[rustc_builtin_macro] macro_rules! include {} +#[rustc_builtin_macro] +macro_rules! compile_error {} + #[rustc_builtin_macro] macro_rules! format_args { () => {} @@ -103,6 +106,9 @@ fn f() { env!("OUT_DIR"); //^^^^^^^^^^^^^^^ `OUT_DIR` not set, enable "load out dirs from check" to fix + compile_error!("compile_error works"); + //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `compile_error!` called: compile_error works + // Lazy: format_args!(); diff --git a/crates/hir_expand/src/builtin_macro.rs b/crates/hir_expand/src/builtin_macro.rs index 7bb42be6cb4..16c3c4d69c1 100644 --- a/crates/hir_expand/src/builtin_macro.rs +++ b/crates/hir_expand/src/builtin_macro.rs @@ -86,7 +86,6 @@ pub fn find_builtin_macro( register_builtin! { LAZY: (column, Column) => column_expand, - (compile_error, CompileError) => compile_error_expand, (file, File) => file_expand, (line, Line) => line_expand, (assert, Assert) => assert_expand, @@ -97,6 +96,7 @@ register_builtin! { (format_args_nl, FormatArgsNl) => format_args_expand, EAGER: + (compile_error, CompileError) => compile_error_expand, (concat, Concat) => concat_expand, (include, Include) => include_expand, (include_bytes, IncludeBytes) => include_bytes_expand, @@ -213,25 +213,6 @@ fn file_expand( ExpandResult::ok(expanded) } -fn compile_error_expand( - _db: &dyn AstDatabase, - _id: LazyMacroId, - tt: &tt::Subtree, -) -> ExpandResult { - if tt.count() == 1 { - if let tt::TokenTree::Leaf(tt::Leaf::Literal(it)) = &tt.token_trees[0] { - let s = it.text.as_str(); - if s.contains('"') { - return ExpandResult::ok(quote! { loop { #it }}); - } - }; - } - - ExpandResult::only_err(mbe::ExpandError::BindingError( - "`compile_error!` argument be a string".into(), - )) -} - fn format_args_expand( _db: &dyn AstDatabase, _id: LazyMacroId, @@ -280,6 +261,30 @@ fn unquote_str(lit: &tt::Literal) -> Option { token.value().map(|it| it.into_owned()) } +fn compile_error_expand( + _db: &dyn AstDatabase, + _id: EagerMacroId, + tt: &tt::Subtree, +) -> ExpandResult> { + let err = match &*tt.token_trees { + [tt::TokenTree::Leaf(tt::Leaf::Literal(it))] => { + let text = it.text.as_str(); + if text.starts_with('"') && text.ends_with('"') { + // FIXME: does not handle raw strings + mbe::ExpandError::Other(format!( + "`compile_error!` called: {}", + &text[1..text.len() - 1] + )) + } else { + mbe::ExpandError::BindingError("`compile_error!` argument must be a string".into()) + } + } + _ => mbe::ExpandError::BindingError("`compile_error!` argument must be a string".into()), + }; + + ExpandResult { value: Some((quote! {}, FragmentKind::Items)), err: Some(err) } +} + fn concat_expand( _db: &dyn AstDatabase, _arg_id: EagerMacroId, @@ -646,7 +651,8 @@ mod tests { "#, ); - assert_eq!(expanded, r#"loop{"error!"}"#); + // This expands to nothing (since it's in item position), but emits an error. + assert_eq!(expanded, ""); } #[test] diff --git a/crates/hir_expand/src/db.rs b/crates/hir_expand/src/db.rs index 4fd0ba290f1..842a177db3f 100644 --- a/crates/hir_expand/src/db.rs +++ b/crates/hir_expand/src/db.rs @@ -207,6 +207,7 @@ fn macro_expand_with_arg( } else { return ExpandResult { value: Some(db.lookup_intern_eager_expansion(id).subtree), + // FIXME: There could be errors here! err: None, }; } From bca1e5fcb825c6c4e09ec197513b5568fce3d985 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Thu, 3 Dec 2020 17:54:43 +0100 Subject: [PATCH 5/5] Rename `error_sink` to `diagnostic_sink` --- crates/hir_expand/src/eager.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/hir_expand/src/eager.rs b/crates/hir_expand/src/eager.rs index a48d08f3b74..0229a836ec5 100644 --- a/crates/hir_expand/src/eager.rs +++ b/crates/hir_expand/src/eager.rs @@ -103,9 +103,9 @@ pub fn expand_eager_macro( macro_call: InFile, def: MacroDefId, resolver: &dyn Fn(ast::Path) -> Option, - mut error_sink: &mut dyn FnMut(mbe::ExpandError), + mut diagnostic_sink: &mut dyn FnMut(mbe::ExpandError), ) -> Result { - let parsed_args = error_sink.option_with( + let parsed_args = diagnostic_sink.option_with( || Some(mbe::ast_to_token_tree(¯o_call.value.token_tree()?)?.0), || err("malformed macro invocation"), )?; @@ -126,20 +126,21 @@ pub fn expand_eager_macro( let arg_file_id: MacroCallId = arg_id.into(); let parsed_args = - error_sink.result(mbe::token_tree_to_syntax_node(&parsed_args, FragmentKind::Expr))?.0; + diagnostic_sink.result(mbe::token_tree_to_syntax_node(&parsed_args, FragmentKind::Expr))?.0; let result = eager_macro_recur( db, InFile::new(arg_file_id.as_file(), parsed_args.syntax_node()), krate, resolver, - error_sink, + diagnostic_sink, )?; - let subtree = error_sink.option(to_subtree(&result), || err("failed to parse macro result"))?; + let subtree = + diagnostic_sink.option(to_subtree(&result), || err("failed to parse macro result"))?; if let MacroDefKind::BuiltInEager(eager) = def.kind { let res = eager.expand(db, arg_id, &subtree); - let (subtree, fragment) = error_sink.expand_result_option(res)?; + let (subtree, fragment) = diagnostic_sink.expand_result_option(res)?; let eager = EagerCallLoc { def, fragment, @@ -182,7 +183,7 @@ fn eager_macro_recur( curr: InFile, krate: CrateId, macro_resolver: &dyn Fn(ast::Path) -> Option, - mut error_sink: &mut dyn FnMut(mbe::ExpandError), + mut diagnostic_sink: &mut dyn FnMut(mbe::ExpandError), ) -> Result { let original = curr.value.clone(); @@ -191,7 +192,7 @@ fn eager_macro_recur( // Collect replacement for child in children { - let def = error_sink + let def = diagnostic_sink .option_with(|| macro_resolver(child.path()?), || err("failed to resolve macro"))?; let insert = match def.kind { MacroDefKind::BuiltInEager(_) => { @@ -201,7 +202,7 @@ fn eager_macro_recur( curr.with_value(child.clone()), def, macro_resolver, - error_sink, + diagnostic_sink, )? .into(); db.parse_or_expand(id.as_file()) @@ -212,10 +213,10 @@ fn eager_macro_recur( | MacroDefKind::BuiltInDerive(_) | MacroDefKind::ProcMacro(_) => { let res = lazy_expand(db, &def, curr.with_value(child.clone()), krate); - let val = error_sink.expand_result_option(res)?; + let val = diagnostic_sink.expand_result_option(res)?; // replace macro inside - eager_macro_recur(db, val, krate, macro_resolver, error_sink)? + eager_macro_recur(db, val, krate, macro_resolver, diagnostic_sink)? } };