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.

[RalfJung]

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.

[flip1995]

on top of #78717 which already makes clippy more painful to deal with than basically every other part of this repository

I want to talk to the bootstrap team during the all-hands what we can do in Clippy to finally improve this situation.

The entire logic of this lint seems off to me. Why is this using valtrees in the first place?

I don't know. But I tried fixing an issue of this lint before and got stuck because I couldn't wrap around how the ValTree API is used here... I think I remember now that the function in question here was added as patch work to "fix" something in the lint. But it seemed that it made it worse long-term. Sorry about that!

I asked on Zulip if someone knows more of the situation here.

[RalfJung]

I want to talk to the bootstrap team during the all-hands what we can do in Clippy to finally improve this situation.

Great! Let me know if I can help
FWIW based on your comment I think a solution should be fairly simple, but I've been waiting for #119899 before looking into it myself as it seems pointless to touch any of the staging part now if it's all going to change anyway... and I didn't want to create more conflicts for that PR.

I think I remember now that the function in question here was added as patch work to "fix" something in the lint.

That is useful background, thanks! A comment in the code explaining that and linking to the issue this fixes would have been helpful.^^

[flip1995]

Samuel removed the test I was referring to in this comment in rust-lang/rust-clippy#14788. So we should be a tiny bit closer.

[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.

[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

No concerns currently listed.

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.