From efda49712b35009c0096217ce2aa262fc5ab8174 Mon Sep 17 00:00:00 2001 From: Nika Layzell Date: Sun, 4 Sep 2022 12:53:29 -0400 Subject: [PATCH] proc_macro/bridge: use the cross-thread executor for nested proc-macros While working on some other changes in the bridge, I noticed that when running a nested proc-macro (which is currently only possible using the unstable `TokenStream::expand_expr`), any symbols held by the proc-macro client would be invalidated, as the same thread would be used for the nested macro by default, and the interner doesn't handle nested use. After discussing with @eddyb, we decided the best approach might be to force the use of the cross-thread executor for nested invocations, as it will never re-use thread-local storage, avoiding the issue. This shouldn't impact performance, as expand_expr is still unstable, and infrequently used. This was chosen rather than making the client symbol interner handle nested invocations, as that would require replacing the internal interner `Vec` with a `BTreeMap` (as valid symbol id ranges could now be disjoint), and the symbol interner is known to be fairly perf-sensitive. This patch adds checks to the execution strategy to use the cross-thread executor when doing nested invocations. An alternative implementation strategy could be to track this information in the `ExtCtxt`, however a thread-local in the `proc_macro` crate was chosen to add an assertion so that `rust-analyzer` is aware of the issue if it implements `expand_expr` in the future. r? @eddyb --- library/proc_macro/src/bridge/server.rs | 37 ++++++++++++++++++- .../ui/proc-macro/auxiliary/expand-expr.rs | 23 ++++++++++-- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/library/proc_macro/src/bridge/server.rs b/library/proc_macro/src/bridge/server.rs index e47a77f6c1350..8202c40d63170 100644 --- a/library/proc_macro/src/bridge/server.rs +++ b/library/proc_macro/src/bridge/server.rs @@ -2,6 +2,7 @@ use super::*; +use std::cell::Cell; use std::marker::PhantomData; // FIXME(eddyb) generate the definition of `HandleStore` in `server.rs`. @@ -143,6 +144,38 @@ pub trait ExecutionStrategy { ) -> Buffer; } +thread_local! { + /// While running a proc-macro with the same-thread executor, this flag will + /// be set, forcing nested proc-macro invocations (e.g. due to + /// `TokenStream::expand_expr`) to be run using a cross-thread executor. + /// + /// This is required as the thread-local state in the proc_macro client does + /// not handle being re-entered, and will invalidate all `Symbol`s when + /// entering a nested macro. + static ALREADY_RUNNING_SAME_THREAD: Cell = Cell::new(false); +} + +/// Keep `ALREADY_RUNNING_SAME_THREAD` (see also its documentation) +/// set to `true`, preventing same-thread reentrance. +struct RunningSameThreadGuard(()); + +impl RunningSameThreadGuard { + fn new() -> Self { + let already_running = ALREADY_RUNNING_SAME_THREAD.replace(true); + assert!( + !already_running, + "same-thread nesting (\"reentrance\") of proc macro executions is not supported" + ); + RunningSameThreadGuard(()) + } +} + +impl Drop for RunningSameThreadGuard { + fn drop(&mut self) { + ALREADY_RUNNING_SAME_THREAD.set(false); + } +} + pub struct MaybeCrossThread

{ cross_thread: bool, marker: PhantomData

, @@ -165,7 +198,7 @@ where run_client: extern "C" fn(BridgeConfig<'_>) -> Buffer, force_show_panics: bool, ) -> Buffer { - if self.cross_thread { + if self.cross_thread || ALREADY_RUNNING_SAME_THREAD.get() { >::new().run_bridge_and_client( dispatcher, input, @@ -188,6 +221,8 @@ impl ExecutionStrategy for SameThread { run_client: extern "C" fn(BridgeConfig<'_>) -> Buffer, force_show_panics: bool, ) -> Buffer { + let _guard = RunningSameThreadGuard::new(); + let mut dispatch = |buf| dispatcher.dispatch(buf); run_client(BridgeConfig { diff --git a/src/test/ui/proc-macro/auxiliary/expand-expr.rs b/src/test/ui/proc-macro/auxiliary/expand-expr.rs index 5463e79d74e0a..1d6ef8a13610a 100644 --- a/src/test/ui/proc-macro/auxiliary/expand-expr.rs +++ b/src/test/ui/proc-macro/auxiliary/expand-expr.rs @@ -80,13 +80,21 @@ fn assert_ts_eq(lhs: &TokenStream, rhs: &TokenStream) { pub fn expand_expr_is(input: TokenStream) -> TokenStream { let mut iter = input.into_iter(); let mut expected_tts = Vec::new(); - loop { + let comma = loop { match iter.next() { - Some(TokenTree::Punct(ref p)) if p.as_char() == ',' => break, + Some(TokenTree::Punct(p)) if p.as_char() == ',' => break p, Some(tt) => expected_tts.push(tt), None => panic!("expected comma"), } - } + }; + + // Make sure that `Ident` and `Literal` objects from this proc-macro's + // environment are not invalidated when `expand_expr` recursively invokes + // another macro by taking a local copy, and checking it after the fact. + let pre_expand_span = comma.span(); + let pre_expand_ident = Ident::new("ident", comma.span()); + let pre_expand_literal = Literal::string("literal"); + let pre_expand_call_site = Span::call_site(); let expected = expected_tts.into_iter().collect::(); let expanded = iter.collect::().expand_expr().expect("expand_expr failed"); @@ -100,6 +108,15 @@ pub fn expand_expr_is(input: TokenStream) -> TokenStream { // Also compare the raw tts to make sure they line up. assert_ts_eq(&expected, &expanded); + assert!(comma.span().eq(&pre_expand_span), "pre-expansion span is still equal"); + assert_eq!(pre_expand_ident.to_string(), "ident", "pre-expansion identifier is still valid"); + assert_eq!( + pre_expand_literal.to_string(), + "\"literal\"", + "pre-expansion literal is still valid" + ); + assert!(Span::call_site().eq(&pre_expand_call_site), "pre-expansion call-site is still equal"); + TokenStream::new() }