Miri: unleash all feature gates

[RalfJung]

IMO it is silly to unleash features that do not even have a feature gate yet, but not unleash features that do. The only thing this achieves is making unleashed mode annoying to use as we have to figure out the feature flags to enable (and not always do the error messages say what that flag is).

Given that the point of -Z unleash-the-miri-inside-of-you is to debug the Miri internals, I see no good reason for this extra hurdle. I cannot imagine a situation where we'd use that flag, realize the program also requires some feature gate, and then be like "oh I guess if this feature is unstable I will do something else". Instead, we'll always just add that flag to the code as well, so requiring the flag achieves nothing.

r? @oli-obk @ecstatic-morse
Fixes #71630

[ecstatic-morse]

The goal of the current behavior is to prevent unnecessary use of -Zunleash in the test suite when feature gates would suffice. Requiring that test writers use feature gates makes it easy to see what tests no longer need -Zunleash when a new feature is implemented on nightly.

Without a diagnostics bug--we were not suggesting that nightly users enable #![feature(const_mut_refs)] when we saw a deref of a mutable reference--the solution to the problem you encountered would have been clear. We should fix this instead.

[RalfJung]

the solution to the problem you encountered would have been clear.

Yes, it would have been clearer.

But I still think we should not ask for feature gates with unleash-miri. Even if the compiler tells me which feature gate is missing, that's just needless hoops for me to jump through. All it achieves is annoy me as I am crafting the testcase.

The goal of the current behavior is to prevent unnecessary use of -Zunleash in the test suite when feature gates would suffice.

I'm not a fan of that kind of nudging -- not when it has harmful effects elsewhere. The way to prevent people from unnecessarily adding the flag is to tell them during review. The flag name is already scary enough to nudge people away from it.

Requiring that test writers use feature gates makes it easy to see what tests no longer need -Zunleash when a new feature is implemented on nightly.

Most (all?) unleash tests that we have should remain unleash even when we get const-checking for those features -- the point of these tests is to check the underlying engine and the dynamic checks, not to check const-qualification. So I don't buy this motivation at well, we typically do not want to remove that flag ever.

[ecstatic-morse]

This seems like a difference in values rather than a dispute over the facts. I'd rather have as many tests exercise the const-checker as possible. You would rather not have to think about it while writing -Zunleash tests.

The stakes here are pretty low, so I'm fine with any outcome, but I would appreciate it if you avoided -Zunleash going forward when it is possible to write the same test with just feature gates. Obviously this does not apply to the case in #71630.

[RalfJung]

This seems like a difference in values rather than a dispute over the facts. I'd rather have as many tests exercise the const-checker as possible. You would rather not have to think about it while writing -Zunleash tests.

For those tests, why would you use the flag at all?
As far as I am concerned, the entire purpose to use this flag -- the only reason it even exists -- is to disable the const-checker and test the dynamic checks of the underlying motivation. Using this flag when the goal is exercising the const-checker seems outright contradictory.

So when your goal is exercising the const-checker, shouldn't you just not use this flag?

[ecstatic-morse]

It's possible for the same test to exercise rare code paths in both the static checks and the dynamic ones. After this PR is merged, it will be possible for you to turn off the const-checker in tests you write (edit: both new and old) even if const-checking could handle them fine. I ask that you don't.

[RalfJung]

@ecstatic-morse I don't think I entirely understand which tests you do not want me to modify how -- so I will just make sure to Cc you on every change I make to miri-unleash tests. That way you can make sure I do not accidentally regress test coverage for the static checks.

I think maybe you are saying I should not add the miri-unleash flag to existing tests ("turn off the const-checker"). I will make sure to never do that, and instead run the test both with and without miri-unleash, like what we ended up doing in #71604. This was anyway the first time I did that, usually I just wrote new miri-unleash tests from scratch. Does that work for you?

[RalfJung]

r? @ecstatic-morse

[ecstatic-morse]

r? @oli-obk

If it were up to me, we wouldn't merge this. Someone else should approve.

[oli-obk]

The stakes here are pretty low, so I'm fine with any outcome, but I would appreciate it if you avoided -Zunleash going forward when it is possible to write the same test with just feature gates.

Side note: we can make sure that when a test specifies unleash, it needs to actually unleash anything in order to pass, otherwise we report an additional error about that or just ICE.

[bors]

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

[RalfJung]

@ecstatic-morse: @oli-obk would like us to form consensus here, and I tend to agree. So let me make another attempt at trying to understand your concern.

If I understand you correctly, you are worried that this change will reduce test coverage for the static const-eval checks. I agree that would be bad. However, I am confused about this concern: in my mind, the entire point of the miri-unleash flag is to not run the static const-eval checks. A test that sets this flag is, by definition, not meant to test anything related to the static checks. Thus I fail to understand how any change in the behavior of this flag can have bad effects on test coverage for the static checks. Maybe you have been using this flag differently?

Currently, indeed the flag dos not disable the static checks entirely, but it disables them mostly, to the extend that I don't think any test meant to cover the static checks can meaningfully use this flag. In particular, the checks for all stable features are disabled, which actually means stabilizing a feature decreases test coverage (they no longer get tested in "miri-unleashed" tests). I don't think that is a good situation even just from the perspective of testing the static checks. Tests for them should simply never use this flag, period. (EDIT: Or maybe I am misunderstanding how the feature_gate() function is used... what would change e.g. in IfOrMatch when that feature becomes stable?) (EDIT2: probably I did, presumably that code will just disappear on stabilization, so we do not need its test coverage any more anyway.)

Maybe you could give a concrete example for how one of our existing tests becomes less useful with this change, or how this change might lead to someone accidentally not testing the thing they wanted to test -- or really any example. So far, whlle I understand your concern in the abstract, I fail to understand how it relates to this PR.

[RalfJung]

The stakes here are pretty low, so I'm fine with any outcome, but I would appreciate it if you avoided -Zunleash going forward when it is possible to write the same test with just feature gates.

This is particularly confusing to me, as the unleash tests I write are usually tests that will never get accepted by rustc, even when the features all become stable. The point is to write const-unsound code and make sure it gets rejected by the dynamic checks. None of the tests in "miri-unleashed" is meant to exercise the static const-checker; that should all happen in dedicated tests (and I think it does so far, and I don't think this PR is likely to change that).

Do we have a single "pass" test with "miri-unleash"? I don't think so, they are all about exercising error paths in the CTFE engine that are otherwise unreachable because the static checks prevent even getting to those failures.

[RalfJung]

Or to put my confusion in one sentence: I don't understand how it make sense that miri-unleash disables the static checks that prevent non-const fn to be called (or any other operation that we will never permit), but does not disable the check that prevents if from being used. It allows the crazy dangerous things but disallows the "perfectly fine but not yet stable" things, which seems just nonsensical to me.

I am curious to understand why you think this makes sense.

[ecstatic-morse]

Side note: we can make sure that when a test specifies unleash, it needs to actually unleash anything in order to pass.

This is the thing I want to enforce. The goal is to prevent you or (more importantly) others from using -Zunleash as a replacement for #![feature(many,different,things)] either in-tree or downstream. I've never said that this is currently going on in-tree, which I think your recent posts are assuming? If there's another way to guarantee this that you find less onerous, such as the one Oli mentioned, I'm happy to approve it.

I'm not a fan of that kind of nudging -- not when it has harmful effects elsewhere. The way to prevent people from unnecessarily adding the flag is to tell them during review. The flag name is already scary enough to nudge people away from it.

I'm a fan of nudging. I'm sorry that you ran into a bad diagnostic, but I don't think the current behavior is "harmful". If you had encountered a scenario that couldn't be resolved just by adding a feature gate, it would be much easier to understand your desire to remove this. The automated check is currently accomplishing the goal I stated above, and I believe that things that can be automated should be: There's no need to rely on the review process.

[RalfJung]

This is the thing I want to enforce. The goal is to prevent you or (more importantly) others from using -Zunleash as a replacement for #![feature(many,different,things)] either in-tree or downstream.

So, what about making -Zunleash call delay_span_bug when it unleashes anything, to make sure that this is only ever used to test error paths? That seems easier than what @oli-obk suggests, and should also avoid your concern?

[RalfJung]

Okay I pushed my latest proposal: this is effectively a delay_span_bug (but through the Session like @oli-obk did to delay it properly) for whenever the miri-unleash flag is sued to get around a feature flag.

This lets me write tests that fail in CTFE without worrying about static checks (they fail anyway, so the delay_span_bug does not matter), but makes sure that run-pass tests properly set the feature flag.

[oli-obk]

side note: what do you think about emitting the feature name that is being skipped if there is one?

[RalfJung]

Sure... or maybe not even emit the warning in that case? Either there will be other errors or a panic at the end listing the missing gates.

[oli-obk]

I think emitting them is good, as it shows where things are being skipped. I mean, I added them on purpose so we'd be able so see exactly where a test is skipping checks, in order to notice when the checks change in any way

[RalfJung]

Otherwise I'm worried this will kill deduplication, so we'll have 5 annotations per const again.

[RalfJung]

in order to notice when the checks change in any way

The point of the flag is to not worry about the static checks, and focus on testing the underlying engine. Why would I care if the checks change? I explicitly do not want to care about that, that's why I use the flag.^^

[oli-obk]

I thought we agreed in your PR that this is not the right kind of error, and instead we should error when unleashing any feature gate (like my PR does now)?

Sorry, we've went back and forth too many times. You're right.

[oli-obk]

I guess I don't know what "labels" vs "spans" are...

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.DiagnosticBuilder.html#method.span_label allows you to add an arbitrary annotation to an arbitrary span. This is what gives us the ^^^^^ something here annotations.

[RalfJung]

Hm... I did that, but it doesn't print the labels.

            let mut diag = self.struct_warn("skipping const checks");
            for &(span, feature_gate) in unleashed_features.iter() {
                if let Some(feature_gate) = feature_gate {
                    diag.span_label(span, format!("skipping check for `{}` feature", feature_gate));
                } else {
                    diag.span_label(span, "skipping check that does not even have a feature gate");
                }
            }
            diag.emit();

becomes just

warning: skipping const checks

[RalfJung]

I tried passing a proper span to struct_warn, no luck.

Our diagnostics APIs are quite hard to use correctly, it seems. :/

[RalfJung]

I went for span_help now, that seems to work. It's quite the hack though...

[oli-obk]

the deduplication isn't necessary anymore, right? (same for the others)

[oli-obk]

@bors r+

[bors]

📌 Commit 182133f has been approved by oli-obk

[bors]

⌛ Testing commit 182133f with merge a0c61a9...

[bors]

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing a0c61a9 to master...

[RalfJung]

Another curious thing about diagnostic UI tests: looks like NOTE annotations match what the rustc API calls "labels". Do they also match what the rustc API calls "note"?