Skip to content

Commit 5b4bd15

Browse files
committed
Auto merge of #101414 - mystor:pm_nested_cross_thread, r=eddyb
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`
2 parents 406e03f + efda497 commit 5b4bd15

File tree

2 files changed

+56
-4
lines changed

2 files changed

+56
-4
lines changed

library/proc_macro/src/bridge/server.rs

+36-1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
33
use super::*;
44

5+
use std::cell::Cell;
56
use std::marker::PhantomData;
67

78
// FIXME(eddyb) generate the definition of `HandleStore` in `server.rs`.
@@ -143,6 +144,38 @@ pub trait ExecutionStrategy {
143144
) -> Buffer;
144145
}
145146

147+
thread_local! {
148+
/// While running a proc-macro with the same-thread executor, this flag will
149+
/// be set, forcing nested proc-macro invocations (e.g. due to
150+
/// `TokenStream::expand_expr`) to be run using a cross-thread executor.
151+
///
152+
/// This is required as the thread-local state in the proc_macro client does
153+
/// not handle being re-entered, and will invalidate all `Symbol`s when
154+
/// entering a nested macro.
155+
static ALREADY_RUNNING_SAME_THREAD: Cell<bool> = Cell::new(false);
156+
}
157+
158+
/// Keep `ALREADY_RUNNING_SAME_THREAD` (see also its documentation)
159+
/// set to `true`, preventing same-thread reentrance.
160+
struct RunningSameThreadGuard(());
161+
162+
impl RunningSameThreadGuard {
163+
fn new() -> Self {
164+
let already_running = ALREADY_RUNNING_SAME_THREAD.replace(true);
165+
assert!(
166+
!already_running,
167+
"same-thread nesting (\"reentrance\") of proc macro executions is not supported"
168+
);
169+
RunningSameThreadGuard(())
170+
}
171+
}
172+
173+
impl Drop for RunningSameThreadGuard {
174+
fn drop(&mut self) {
175+
ALREADY_RUNNING_SAME_THREAD.set(false);
176+
}
177+
}
178+
146179
pub struct MaybeCrossThread<P> {
147180
cross_thread: bool,
148181
marker: PhantomData<P>,
@@ -165,7 +198,7 @@ where
165198
run_client: extern "C" fn(BridgeConfig<'_>) -> Buffer,
166199
force_show_panics: bool,
167200
) -> Buffer {
168-
if self.cross_thread {
201+
if self.cross_thread || ALREADY_RUNNING_SAME_THREAD.get() {
169202
<CrossThread<P>>::new().run_bridge_and_client(
170203
dispatcher,
171204
input,
@@ -188,6 +221,8 @@ impl ExecutionStrategy for SameThread {
188221
run_client: extern "C" fn(BridgeConfig<'_>) -> Buffer,
189222
force_show_panics: bool,
190223
) -> Buffer {
224+
let _guard = RunningSameThreadGuard::new();
225+
191226
let mut dispatch = |buf| dispatcher.dispatch(buf);
192227

193228
run_client(BridgeConfig {

src/test/ui/proc-macro/auxiliary/expand-expr.rs

+20-3
Original file line numberDiff line numberDiff line change
@@ -80,13 +80,21 @@ fn assert_ts_eq(lhs: &TokenStream, rhs: &TokenStream) {
8080
pub fn expand_expr_is(input: TokenStream) -> TokenStream {
8181
let mut iter = input.into_iter();
8282
let mut expected_tts = Vec::new();
83-
loop {
83+
let comma = loop {
8484
match iter.next() {
85-
Some(TokenTree::Punct(ref p)) if p.as_char() == ',' => break,
85+
Some(TokenTree::Punct(p)) if p.as_char() == ',' => break p,
8686
Some(tt) => expected_tts.push(tt),
8787
None => panic!("expected comma"),
8888
}
89-
}
89+
};
90+
91+
// Make sure that `Ident` and `Literal` objects from this proc-macro's
92+
// environment are not invalidated when `expand_expr` recursively invokes
93+
// another macro by taking a local copy, and checking it after the fact.
94+
let pre_expand_span = comma.span();
95+
let pre_expand_ident = Ident::new("ident", comma.span());
96+
let pre_expand_literal = Literal::string("literal");
97+
let pre_expand_call_site = Span::call_site();
9098

9199
let expected = expected_tts.into_iter().collect::<TokenStream>();
92100
let expanded = iter.collect::<TokenStream>().expand_expr().expect("expand_expr failed");
@@ -100,6 +108,15 @@ pub fn expand_expr_is(input: TokenStream) -> TokenStream {
100108
// Also compare the raw tts to make sure they line up.
101109
assert_ts_eq(&expected, &expanded);
102110

111+
assert!(comma.span().eq(&pre_expand_span), "pre-expansion span is still equal");
112+
assert_eq!(pre_expand_ident.to_string(), "ident", "pre-expansion identifier is still valid");
113+
assert_eq!(
114+
pre_expand_literal.to_string(),
115+
"\"literal\"",
116+
"pre-expansion literal is still valid"
117+
);
118+
assert!(Span::call_site().eq(&pre_expand_call_site), "pre-expansion call-site is still equal");
119+
103120
TokenStream::new()
104121
}
105122

0 commit comments

Comments
 (0)