From 4a9aa10f58f5aedef2760db216249a9df5a85b88 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Wed, 26 Jul 2023 13:52:54 +0200 Subject: [PATCH] Improve the `ffi_await!` situation: mainly, the UX on syntax error --- src/_lib.rs | 6 ++ src/proc_macro/ffi_export/fn_/async_fn.rs | 112 ++++++++++++++-------- src/proc_macro/utils/macros.rs | 13 ++- 3 files changed, 88 insertions(+), 43 deletions(-) diff --git a/src/_lib.rs b/src/_lib.rs index 2833f2eaee..f3f112ed08 100644 --- a/src/_lib.rs +++ b/src/_lib.rs @@ -660,4 +660,10 @@ mod __ { } } } + + #[doc(hidden)] /** Not part of the public API! */ + #[macro_export] + macro_rules! ඞassert_expr {( $e:expr $(,)? ) => ( $e )} + #[doc(inline)] + pub use ඞassert_expr as assert_expr; } diff --git a/src/proc_macro/ffi_export/fn_/async_fn.rs b/src/proc_macro/ffi_export/fn_/async_fn.rs index 3e2648c965..b1abe34abb 100644 --- a/src/proc_macro/ffi_export/fn_/async_fn.rs +++ b/src/proc_macro/ffi_export/fn_/async_fn.rs @@ -44,54 +44,88 @@ fn export ( // ``` // where the `` make up a prelude that allow to make `` be // `'static`. - let (ref prelude, (ffi_await, ref async_body)): (Vec, (_, Expr)) = { + let (ref prelude, ffi_await, ref async_body): (Vec, _, Expr) = { let mut stmts = fun.block.stmts.clone(); - let mut async_body = None; - if let Some(err_span) = (|| match stmts.pop() { + let ill_formed_ffi_await_at = |problematic_tts: &dyn ToTokens| bail! { + "\ + `#[ffi_export(…, executor = …)]` expects the last \ + expression/statement to be an expression of the form: \ + `ffi_await!()` such as:\n\ + \x20\x20\x20\x20ffi_await!(async move {\n …\n })\n\ + " => problematic_tts + }; + match stmts.pop() { + // no trailing expression case (block ending with a `;`): | Some(Stmt::Local(Local { semi_token, .. })) | Some(Stmt::Semi(_, semi_token)) | Some(Stmt::Item( - Item::Macro(ItemMacro { semi_token: Some(semi_token), .. }) + Item::Macro(ItemMacro { + semi_token: Some(semi_token), + .. + }) )) => { - Some(semi_token.span()) - }, - | Some(Stmt::Item(ref item)) => { - Some(item.span()) + return ill_formed_ffi_await_at(&semi_token); }, - | None => Some(Span::call_site()), - | Some(Stmt::Expr(expr)) => { - let span = expr.span(); - match expr { - | Expr::Macro(ExprMacro { - attrs: _, - mac: Macro { - path: ffi_await, - tokens, - .. - }, - }) => if ffi_await.is_ident("ffi_await") { - if let Ok(expr) = parse2(tokens) { - async_body = Some((ffi_await.span(), expr)); - return None; - } + // `ffi_await!( tokens… )` or `ffi_await! { tokens… }` cases: + | Some(Stmt::Expr(Expr::Macro(ExprMacro { + mac: Macro { + path: ffi_await, + tokens, + .. + }, + .. + }))) + | Some(Stmt::Item( + Item::Macro(ItemMacro { + semi_token: None, + mac: Macro { + path: ffi_await, + tokens, + .. }, - | _ => {}, - } - Some(span) + .. + }) + )) + if ffi_await.is_ident("ffi_await") + => { + // In order to guarantee good UX when using this fake + // macro, we want to assert that these `tokens…` + // represent a valid expression. + // + // While we could let `syn` parse them and produce its + // own syntactic error when ill-formed, since we don't + // need to, ourselves, inspect that expression in and of + // itself (we just treat it as an opaque expression + // which we trust to be an `impl Future`), we will + // similarly trust its syntactic well-formedness and let + // Rust complain at the final expansion. + // + // However, since things such as `let x = #expr;` can + // actually succeed when `expr` is not an expression but + // e.g. `42; let x = 27`, we still would like some + // safeguard against that. We could use `(#expr)`, but then: + // - we need to disable the `unused_parens` lint… + // - in case of `cargo expand` people will see the parens. + // + // We instead use `identity_expr_macro!(#expr)`, which + // achieves the same safeguard, but with invis parens. + // + // At that point, we may as well have a well-typed + // `Expr` rather than a raw `TokenStream2`, which we can + // cheaply achieve with the `Verbatim` variant. + let expr = Expr::Verbatim(quote!( + ::safer_ffi::ඞ::assert_expr!(#tokens) + )); + (stmts, ffi_await.span(), expr) + }, + // Other trailing stmt case, _e.g._, `mod some_item { … }` + | Some(other_trailing_stmt) => { + return ill_formed_ffi_await_at(&other_trailing_stmt) }, - })() - { - bail!( - "\ - `#[ffi_export(…, executor = …)]` expects the last \ - expression/statement to be an expression of the form: \ - `ffi_await!()` such as:\n \ - ffi_await!(async move {\n …\n })\n\ - " => spanned!(err_span) - ); + // completely empty fn body! + | None => return ill_formed_ffi_await_at(&reified_span(None)), } - (stmts, async_body.unwrap()) }; let block_on = respan(fun.block.span(), block_on.into_token_stream()); @@ -198,7 +232,7 @@ fn export ( ); let ret: #RetTy = match ::core::marker::PhantomData::<#RetTy> { _ => { - { #async_body }.await + #async_body.await }} ; unsafe { diff --git a/src/proc_macro/utils/macros.rs b/src/proc_macro/utils/macros.rs index 9f65c50ae8..86e0e38b95 100644 --- a/src/proc_macro/utils/macros.rs +++ b/src/proc_macro/utils/macros.rs @@ -1,15 +1,20 @@ #![cfg_attr(rustfmt, rustfmt::skip)] -macro_rules! spanned {( $span:expr $(,)? ) => ( - ::proc_macro2::Ident::new("__", $span) -)} pub(in crate) use spanned; +pub(in crate) +fn reified_span(span: impl Into>) + -> impl ::quote::ToTokens +{ + ::proc_macro2::Ident::new("__", span.into().unwrap_or_else( + ::proc_macro2::Span::call_site + )) +} macro_rules! bail { ( $err_msg:expr $(,)? ) => ( $crate::utils::bail! { - $err_msg => $crate::utils::spanned!(::proc_macro2::Span::call_site()) + $err_msg => $crate::utils::reified_span(::core::prelude::v1::None) } );