Skip to content

const-eval: allow constants to refer to mutable/external memory, but reject such constants as patterns #140942

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented May 12, 2025

This fixes #140653 by accepting code such as this:

static FOO: AtomicU32 = AtomicU32::new(0);
const C: &'static AtomicU32 = &FOO;

This can be written entirely in safe code, so there can't really be anything wrong with it.

We also accept the much more questionable following code, since it looks very similar to the interpreter:

static mut FOO2: u32 = 0;
const C2: &'static u32 = unsafe { &mut FOO2 };

Using this without causing UB is at least very hard (the details are unclear since it is related to how the aliasing model deals with the staging of const-eval vs runtime code).

If a constant like C2 is used as a pattern, we emit an error:

error: constant BAD_PATTERN cannot be used as pattern
  --> $DIR/const_refs_to_static_fail.rs:30:9
   |
LL |         BAD_PATTERN => {},
   |         ^^^^^^^^^^^
   |
   = note: constants that reference mutable or external memory cannot be used as pattern

(If you somehow manage to build a pattern with constant C, you'd get the same error, but that should be impossible: we don't have a type that can be used in patterns and that has interior mutability.)

The same treatment is afforded for shared references to extern static, for the same reason: the const evaluation is entirely fine with it, we just can't build a pattern for it -- and when using interior mutability, this can be totally sound.

We do still not accept anything where there is an &mut in the final value of the const, as that should always require unsafe code and it's hard to imagine a sound use-case that would require this.

@rustbot
Copy link
Collaborator

rustbot commented May 12, 2025

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 12, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@RalfJung RalfJung added T-lang Relevant to the language team S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-lang-nominated Nominated for discussion during a lang team meeting. labels May 12, 2025
@RalfJung
Copy link
Member Author

@rust-lang/lang nominating for FCP following prior discussion in #140653.

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the const-ref-to-mut branch from f619969 to 9767f96 Compare May 12, 2025 12:13
@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the const-ref-to-mut branch from 9767f96 to 160cee0 Compare May 12, 2025 12:37
@rustbot
Copy link
Collaborator

rustbot commented May 12, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@RalfJung RalfJung force-pushed the const-ref-to-mut branch from 160cee0 to e316943 Compare May 12, 2025 12:41
@RalfJung RalfJung force-pushed the const-ref-to-mut branch from e316943 to 6722d4d Compare May 12, 2025 13:01
@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

r? @oli-obk (or someone way more familiar with const-eval)

@rustbot rustbot assigned oli-obk and unassigned jieyouxu May 12, 2025
@RalfJung RalfJung force-pushed the const-ref-to-mut branch 2 times, most recently from 57ea31d to 6e9a7f4 Compare May 12, 2025 16:46
@traviscross traviscross removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 13, 2025
@traviscross
Copy link
Contributor

traviscross commented May 13, 2025

As discussed in #140653 (comment), this sounds right to me, and I propose that we do it.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 13, 2025

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 13, 2025
@traviscross traviscross removed I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels Jun 4, 2025
@RalfJung RalfJung force-pushed the const-ref-to-mut branch 2 times, most recently from 24338f5 to 6f196f8 Compare June 13, 2025 14:18
@RalfJung
Copy link
Member Author

RalfJung commented Jun 13, 2025

@oli-obk FCP should finish fairly soon here, so this is ready for review. :)

@traviscross I guess this will also need a reference PR? Not sure when I'll get around to that.
I couldn't even find where the limitation "the final value of a const cannot refer to mutable memory" is documented, if at all. I did however find this, which has a small mistake (orthogonal to this PR): borrows in consts are also allowed to refer to static items and promoteds. (EDIT: put up a PR for that: rust-lang/reference#1858)

@traviscross
Copy link
Contributor

cc @ehuss

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

My preference would have been 3 commits

  • error reporting pulled out of query
  • stylistic refactoring of match with large arm
  • actual changes

but since I already reviewed everything, let's land it as is

@RalfJung
Copy link
Member Author

Thanks! I agree smaller commits are nicer in retrospect, but I discovered these things half-way through implementation so it'd have been a lot of work to disentangle them...

@ehuss
Copy link
Contributor

ehuss commented Jun 13, 2025

the final value of a const cannot refer to mutable memory

Maybe you are referring to this? That was added as part of const_refs_to_statics (rust-lang/reference#1610)

@RalfJung
Copy link
Member Author

Ah, yes, that's it. That should be just removed, and then instead we need something here about consts referencing mutable/extern statics.

@oli-obk

This comment was marked as off-topic.

@RalfJung
Copy link
Member Author

Reference PR: rust-lang/reference#1859

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 14, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Jun 14, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@RalfJung
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Collaborator

bors commented Jun 14, 2025

📌 Commit 6f196f8 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 14, 2025
@RalfJung
Copy link
Member Author

@bors r-
I guess the reference PR has to land first?

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 14, 2025
Comment on lines 10 to 13
static mut S: i32 = 3;
const C2: &'static mut i32 = unsafe { &mut S };
//~^ ERROR: reference to mutable memory

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this removed? It doesn't seem like its behavior was changed by this PR (it still generates the same E0080 error). Is it covered in some other test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh, I don't remember...
The error message does change, so it is unclear whether this even still tests the right thing. 🤷 But I can add the test back with the new error, sure.

@traviscross traviscross added the S-waiting-on-documentation Status: Waiting on approved PRs to documentation before merging label Jun 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-documentation Status: Waiting on approved PRs to documentation before merging T-lang Relevant to the language team to-announce Announce this issue on triage meeting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error when a const contains a shared ref to interior mutable data