No unleashing of legal code #71748
No unleashing of legal code #71748
Conversation
| // If all skipped operations could also be turned on by a feature gate, then we want | ||
| // to emit an error. So when we see a non-feature gate operation being skipped, disable | ||
| // that error reporting. | ||
| if O::feature_gate().is_none() { |
There was a problem hiding this comment.
I think the point of the flag is to make the static checker shut up. So, whether or not something requires a feature gate in the static checker should not have any influence on what happens.
So, I am not a big fan of this proposal.
| @@ -1,4 +1,3 @@ | |||
| // compile-flags: -Zunleash-the-miri-inside-of-you | |||
| #![allow(const_err)] | |||
|
|
|||
| // Make sure we cannot mutate globals. | |||
There was a problem hiding this comment.
This test should now be moved to a different directory.
There was a problem hiding this comment.
Oh wait... no, this test is fine.
There was a problem hiding this comment.
This is very similar to #71748 (comment) -- the point of the test is to make sure the dynamic checks can catch this error no matter what the static checks are doing. But after removing the flag we don't know any more if that is the case.
There was a problem hiding this comment.
Yea this is already hitting dynamic checks because our static checks can only detect mutations of statics opportunistically. While we could have a static check right now, with mutable references on the horizon that check would just stop working again.
| @@ -1,5 +1,4 @@ | |||
| // run-pass | |||
| // compile-flags: -Zunleash-the-miri-inside-of-you | |||
There was a problem hiding this comment.
This test should be moved to a different directory.
| @@ -1,5 +1,4 @@ | |||
| // revisions: default unleash | |||
| //[unleash]compile-flags: -Zunleash-the-miri-inside-of-you | |||
| // revisions: default | |||
There was a problem hiding this comment.
This basically reverts #71604. I specifically wrote that PR to make sure that the dynamic checks are able to catch this problem, even if the static checks might one day evolve to also be able to handle it (e.g. via #71526). So, this change potentially reduces future test coverage for the dynamic checks, which is not great.
That's why I think "-Zmiri-unleash will make compilation fail" is a better safety net than your proposal. It catches read_from_static.rs (which we all agree should not use the flag), but permits this test here.
There was a problem hiding this comment.
Yea, this issue is why I left a longer comment in the PR's main comment, even though I misexplained what is going on exactly in the opposite way.
RalfJung
added
the
S-waiting-on-review
|
So taking a step back, I see the flag has been used for two different purposes:
I don't think the flag should serve the second purpose, and we should come up with something else to do with those tests (e.g., introduce an incomplete experimental feature flag for const fn pointers). |
|
I think it's fine to have |
|
I agree that this PR doesn't fix the situation and has no way forward to doing so |
r? @RalfJung @ecstatic-morse
We have some tests (that are now not using unleash anymore) that I believe used unleash to guard against any changes to static checks suddenly causing code to be evaluated and then triggering dynamic checks.
This PR was created after discussion in #71631 (comment)