[WIP] proc_macro: check non-interned handles for "leaks" between/after invocations. #63809
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
|
⌛ Trying commit 1754dcd with merge d9c49acb7b15e0b6e1b797982100ad6c5d1424a2... |
|
☀️ Try build successful - checks-azure |
|
@craterbot run mode=check-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
[WIP] rustc_mir: disallow global mutable state in proc macros. Along the lines of #63809, this PR attempts to get rid of the main (or only?) place `proc_macro` handles could be leaked to, *and* further disallow/discourage sharing (other) state between invocations. The approach of banning (interior-)mutable `static`s was most recently mentioned in #63804 (comment), but it's likely been brought up several times, we just never tried it. (Note that this is not foolproof: one would have to scan all dependencies for such `static`s, modulo `proc_macro`/`std`, and even then it's possible there would be a lot of false positives) So this is mostly for a check-only crater run, to see what (if anything) breaks.
|
@craterbot crates=full |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot abort |
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-review
|
I'm not sure we should land this, given that all 3 regressions in #64398 (comment) that can be directly attributed to this PR are "forgetful" leaks, not stashing |
|
Maybe just make it a lint that ppl can silence? |
|
Ping from triage, any updates? @eddyb |
|
@oli-obk I'm not exactly sure how - at least it's on the server side of the proc-macro bridge - maybe I can add a method somewhere to report it and the default is At least |
eddyb
added
S-blocked
|
Blocked on rust-lang/crater#461 to find the exact set of regressions. |
|
@Mark-Simulacrum this was included in the crater run for #63831, which caused ~700 regressions. We want to be sure that that PR didn't mask any regressions in this one. I commented on the broken PR, but I meant for eddyb to trigger runs on all the others. I don't have crater perms, but could you trigger a run with: @craterbot check crates=https://gist.githubusercontent.com/ecstatic-morse/ca6fe943de6937db635143472358d90d/raw/177739189815b3c52a7f69b494dbb91ea2d25e1d/gistfile1.txt |
|
🔒 Error: you're not allowed to interact with this bot. 🔑 If you are a member of the Rust team and need access, add yourself to the whitelist. |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
@craterbot p=1 Bumping priority since this is a short run |
|
🚨 Error: failed to parse the command 🆘 If you have any trouble with Crater please ping |
|
@craterbot p=1 Bumping priority since this is a short run |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
I doubt this is worth doing - these handles can't actually be (mis)used anyway. |
10 participants
Inspired by #63804, this PR panics on some forms of
proc_macroAPI misuse more eagerly (when a proc macro invocation finishes, as opposed to on the first use of a "leaked" handle).However, it's unsure whether there are legitimate leaks of
proc_macro"objects" in the wild, and @Centril suggested we try it out on crater first (check-only mode).