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

[RalfJung]

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 FOO };

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]

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]

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]

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

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[RalfJung]

@rust-lang/clippy I could not figure out why clippy has its own version of this function, and there's no comment giving any hint. The easiest way to fix the build failure was to remove the function so I did that.

[flip1995]

@Jarcho might know.

[RalfJung]

Hm, this does seem to change some of the tests. I have no clue why that is, apparently some detail of the old code was load-bearing. The total lack of comments in the code makes this rather painful to debug... (and that's on top of #78717 which already makes clippy more painful to deal with than basically every other part of this repository :/ )

[RalfJung]

The entire logic of this lint seems off to me. Why is this using valtrees in the first place? That seems to be a misuse of the valtree APIs. I think what you really want here is to use a value visitor to walk the value of the const and search for UnsafeCell, similar to this. That will then also work with non-valtree-compatible constants.

Please consult with the stakeholders of an API before using it in creative ways in clippy. Long-term that saves a lot of maintenance work. :)

[RalfJung]

I think somehow previously clippy managed to convince the valtree evaluation machinery to evaluate generic consts without throwing TooGeneric. I have no idea why that worked, but it was never supposed to work -- constants must be monomorphic to be evaluated. Furthermore, whenever it did encounter TooGeneric, clippy interprets that as an unfrozen value -- with a long comment for why that's the better choice (which I don't agree with, since TooGeneric really means you have no idea at all about the contents of the constant). However, now that more TooGeneric errors appear (and as far as I can tell, those errors are correct), this is no longer the better choice, so I changed it. That means the lint fires much less than before (it never fires on generic consts any more, which affects the majority of testcases), but I don't think it makes sense to even attempt to lint on generic consts -- rustc is not designed for that, and trying to misuse its APIs is not long a long-term viable strategy.

@oli-obk maybe you have a better idea here.

[flip1995]

Jason is working on a PR rust-lang/rust-clippy#13207 that refactors this lint and removes the ValTree API misuse.

I managed to get one of those cases to fire the lint again by more closely following the original code. No idea why that particular change made a difference for that case, or why all the other cases are still not firing.

So if this PR introduces some false negatives, I would say that isn't too bad.

[RalfJung]

Yeah, as-is the PR adds a bunch of false negatives. Though seems like I'll just have to wait until rust-lang/rust-clippy#13207 syncs to this repo, drop my changes, and rebase.

[flip1995]

This PR missed the sync by 1 day. So the next one will be in just short of 2 weeks.. I can do an out-of-cycle sync if you want it sooner.

[RalfJung]

Up to you -- if my PR lands first, you'll get a conflict when doing the sync. But this PR will not land soon anyway, first we need FCP to start and then wait another 10 days.

[flip1995]

Next sync is in exactly 10 days. So that (if nothing goes wrong) goes in first.

[jieyouxu]

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

[traviscross]

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

@rfcbot fcp merge

[rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• whole team signoff (const-eval: allow constants to refer to mutable/external memory, but reject such constants as patterns #140942 (comment))

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.

[tmandry]

@rfcbot reviewed

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[tmandry]

@rfcbot concern whole team signoff

The proposal is to allow shared references to mutable memory in consts, but not to allow those consts to be used in patterns. This is a notable expansion and it is (to my knowledge) the first time we are not allowing a const to be used in a pattern, which constrains future decisions. I don't have a problem with it, and I think we probably should do it, but I would like the full team to know about it because it didn't have an RFC.

---

On a meta note, I keep having the feeling that this cluster of features is being "designed just in time". That is not to criticize anyone. Every decision we make seems reasonable and has good rationale, but it makes me nervous because I can't tell where we will ultimately end up as we go down this road. The path we are committing to here is something I'd ideally love to have considered in an RFC.

On the other hand, maybe this is working as intended? There are a lot of details to work out, and I don't know of anyone with a crystal ball who can foresee every implication. And if we did consider every implication it might overwhelm the lang team into indecision :).

I think we are far enough down this path that there's nothing to be done here, but it's something to think about.

[bors]

☔ The latest upstream changes (presumably #141343) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

The proposal is to allow shared references to mutable memory in consts, but not to allow those consts to be used in patterns.

To be fully precise, this is about such shared references in the final value of a const. We already allowed such shared references to appear as temporaries within the computation of a const before.

On the other hand, maybe this is working as intended? There are a lot of details to work out, and I don't know of anyone with a crystal ball who can foresee every implication. And if we did consider every implication it might overwhelm the lang team into indecision :).

That's pretty much how I see it. There are a whole bunch of things one could imagine doing, but it's rather unclear what the trade-offs are, and which ones are worth it. So instead of planning a grand architecture on the whiteboard and then putting it into reality all at once, we're discovering one step at a time what it is that users actually want or need.