Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] rustc_mir: disallow global mutable state in proc macros. #63831

Closed
wants to merge 1 commit into from
Closed

[WIP] rustc_mir: disallow global mutable state in proc macros. #63831

wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Aug 23, 2019

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 statics 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 statics, 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.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2019
@eddyb
Copy link
Member Author

eddyb commented Aug 23, 2019

@bors try

@bors
Copy link
Contributor

bors commented Aug 23, 2019

⌛ Trying commit ee8492f with merge 8c9c32a...

bors added a commit that referenced this pull request Aug 23, 2019
@bors
Auto merge of #63831 - eddyb:proc-macro-statecheck, r=<try>
8c9c32a 
[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.
@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2019
@bors
Copy link
Contributor

bors commented Aug 23, 2019

☀️ Try build successful - checks-azure
Build commit: 8c9c32a

@eddyb
Copy link
Member Author

eddyb commented Aug 23, 2019

@craterbot run mode=check-only

@eddyb
Copy link
Member Author

eddyb commented Aug 27, 2019

@craterbot run mode=check-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-63831 created and queued.
🤖 Automatically detected try build 8c9c32a
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@pietroalbini
Copy link
Member

@craterbot crates=full

@craterbot
Copy link
Collaborator

📝 Configuration of the pr-63831 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@eddyb eddyb force-pushed the proc-macro-statecheck branch from ee8492f to 55f3acf Compare September 12, 2019 07:22
@eddyb eddyb mentioned this pull request Sep 12, 2019
Closed
@eddyb
Copy link
Member Author

eddyb commented Sep 12, 2019

@craterbot abort
(in favor of #64398)

@craterbot
Copy link
Collaborator

🗑️ Experiment pr-63831 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Sep 12, 2019
@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 12, 2019
@taiki-e taiki-e mentioned this pull request Sep 20, 2019
Merged
bors bot added a commit to taiki-e/auto_enums that referenced this pull request Sep 20, 2019
@bors @taiki-e
Merge #60
e2c893b 
60: core: Remove usage of mutable global state r=taiki-e a=taiki-e

It may be a warning in the future. So, use the hash value of the input AST instead of PRNG.

See rust-lang/rust#64398 (comment) and rust-lang/rust#63831.

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 29, 2019 •
edited
Loading

Crater completed in #64398, marking as waiting on author to decide what to do (or not to do) next.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Sep 29, 2019
@ecstatic-morse
Copy link
Contributor

Regressed crates list is here.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Oct 7, 2019 •
edited
Loading

The new functionality around crate lists has been deployed @eddyb. The following invocation should work (I don't have craterbot permissions):

@craterbot check crates=https://gist.githubusercontent.com/ecstatic-morse/ca6fe943de6937db635143472358d90d/raw/177739189815b3c52a7f69b494dbb91ea2d25e1d/gistfile1.txt

@craterbot
Copy link
Collaborator

🔒 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.
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Mark-Simulacrum
Copy link
Member

@craterbot check crates=https://gist.githubusercontent.com/ecstatic-morse/ca6fe943de6937db635143472358d90d/raw/177739189815b3c52a7f69b494dbb91ea2d25e1d/gistfile1.txt

@craterbot
Copy link
Collaborator

👌 Experiment pr-63831 created and queued.
🤖 Automatically detected try build 8c9c32a
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 7, 2019
@Mark-Simulacrum
Copy link
Member

@craterbot p=1 since this is such a small run

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@pietroalbini
Copy link
Member

@craterbot p=1

@craterbot
Copy link
Collaborator

📝 Configuration of the pr-63831 experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🚧 Experiment pr-63831 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-63831 is completed!
📊 627 regressed and 0 fixed (656 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 10, 2019
@petrochenkov
Copy link
Contributor

Marking as waiting on author to triage the regressions.
(I'm not too interested in driving this myself.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2019
This was referenced Oct 14, 2019
Merged
Closed
@JohnCSimon
Copy link
Member

Ping from triage
@eddyb Can you please address the regressions in this PR?
cc: @petrochenkov

Thanks

@eddyb
Copy link
Member Author

eddyb commented Oct 30, 2019

I can't think of a good way to categorize uses of interior mutability in statics (e.g. lazy_static! is fine, RNGs are not), and proc_macro handles can't be used across invocations anyway.

We probably can't do much here, and sandboxing (e.g. via WASM) will likely be opt-in.
Hopefully the situation isn't too bad if we ever try to put proc macro expansion through incremental.

@eddyb eddyb closed this Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@eddyb @rust-highfive @bors @craterbot @pietroalbini @petrochenkov @ecstatic-morse @Mark-Simulacrum @JohnCSimon