Skip to content

Add a static analysis that prevents references to inner mutability only in the trailing expression of a constant #80373

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
Closed
wants to merge 5 commits into from
Closed

Add a static analysis that prevents references to inner mutability only in the trailing expression of a constant #80373

wants to merge 5 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 26, 2020

r? @RalfJung

I also added a feature gate at the same time so we can disable the old check that prevented

const FOO: () = {
    let x = Cell::new(42);
    let y = &x;
};

I believe we can use the same scheme for mutable references.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 26, 2020
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk mentioned this pull request Dec 26, 2020
Closed
3 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk marked this pull request as draft December 27, 2020 02:02
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 27, 2020

nevermind, this scheme can't work. Rewriting the Qualif framework to support more data than just a has/hasn't qualif bit.

@oli-obk oli-obk marked this pull request as ready for review December 27, 2020 14:23
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 27, 2020

cc @felix91gr @camelid (do we need a dataflow working group? 😆 )

I rewrote the Qualif dataflow, which required a few implementations that I think are correct, but it would be great if you could review them (the first commit)

oli-obk
oli-obk commented Dec 27, 2020
View reviewed changes
compiler/rustc_mir/src/dataflow/framework/fmt.rs

write!(f, "\u{001f}-")?;
self.fmt_with(ctxt, f)
old.fmt_with(ctxt, f)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a drive-by bugfix

oli-obk
oli-obk commented Dec 27, 2020
View reviewed changes
compiler/rustc_mir/src/transform/check_consts/qualifs.rs
/// The dataflow result type. If it's just qualified/not qualified, then
/// you can just use a `()` (most qualifs do that). But if you need more state, use a
/// custom enum.
type Result: SetChoice + std::fmt::Debug = ();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer to use a marker ZST that is individual to each qualif (or even just use the type implementing Qualif), but since that is a refactoring not necessary for this PR, I think we should not do it here.

oli-obk
oli-obk commented Dec 27, 2020
View reviewed changes
compiler/rustc_mir/src/transform/check_consts/qualifs.rs
pub enum HasMutInteriorBehindRefState {
Yes,
/// As long as we haven't encountered a reference yet, we use this state
/// which is equivalent to the `HasMutInterior` qualif.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a follow up PR we can check whether we can get rid of HasMutInterior and only run this qualif

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk mentioned this pull request Dec 27, 2020
Merged
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 27, 2020 •
edited
Loading

@eddyb had an idea that gives us the same result with a much simpler implementation, so I'm closing this. See #80418 for the continuation of this work

@oli-obk oli-obk closed this Dec 27, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 4, 2021
@bors
Auto merge of rust-lang#80418 - oli-obk:this_could_have_been_so_simpl…
8989689 
…e, r=RalfJung

Allow references to interior mutable data behind a feature gate

supercedes rust-lang#80373 by simply not checking for interior mutability on borrows of locals that have `StorageDead` and thus can never be leaked to the final value of the constant

tracking issue: rust-lang#80384

r? `@RalfJung`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RalfJung RalfJung

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oli-obk @rust-log-analyzer @RalfJung @rust-highfive