config_type: add unstable_variant attribute by tommilligan · Pull Request #5379...

I would also ask - how should I go about testing this? I couldn't find any tests for the existing unstable options, or the proc macro code internals. In fact, I get test failures when I run cd config_proc_macro && cargo test on master

Haven't really had a chance to look at this yet, but just wanted to say I love the idea! Attributes seem like a really clean and simple way of managing this and I'm excited about the possibilities

@tommilligan as well as updating Config::fill_from_parsed_config I also believe you'll need to modify Config::override_value, which ends up getting called when overriding the config with any values passed on the command line using --config

Do we need to define a new proc_macro_attribute macro? For example:

#[proc_macro_attribute]
pub fn unstable_variant(_args: TokenStream, input: TokenStream) -> TokenStream {
    input
}

Also I believe the tests are failing because the doc tests for nightly_only_test and stable_only_test aren't quite right. We could fix them by adding /// # use rustfmt_config_proc_macro::nightly_only_test; and
/// # use rustfmt_config_proc_macro::stable_only_test; to bring each attribute macro into scope for the doc tests. I can open up a PR for that a little later.

I can see how testing this could be a little tricky since we don't currently have any options that are stable with unstable_variants. I wonder if it would be possible to add a config_type to Config thats only available for testing.

// src/config/options.rs

/// This configuration option is only available for testing purposes
#[cfg(test)]
#[config_type]
pub enum SemiStableOption {
    Stable,
    #[unstable_variant]
    Unstable,
}

// src/config/config_types.rs

#[cfg(test)]
use crate::config::options::SemiStableOption;

macro_rules! create_config {
    ($($i:ident: $ty:ty, $def:expr, $stb:expr, $( $dstring:expr ),+ );+ $(;)*) => (
        #[cfg(test)]
        use std::collections::HashSet;
        use std::io::Write;

        use serde::{Deserialize, Serialize};

        #[derive(Clone)]
        #[allow(unreachable_pub)]
        pub struct Config {
            #[cfg(test)]
            semi_stable_option: (Cell<bool>, bool, SemiStableOption, bool),
            // For each config item, we store a bool indicating whether it has
            // been accessed and the value, and a bool whether the option was
            // manually initialised, or taken from the default,
            $($i: (Cell<bool>, bool, $ty, bool)),+
        }
...

And then we could write a few tests in tests/rustfmt/main.rs that call the rustfmt binary directly. You can take a look at some of the existing tests in that module. I also opened #5025 a while ago, which touched similar areas of the config code. It's in limbo right now, but some of the tests I wrote in tests/rustmft/main.rs could be useful to help you test that you get the expected warning messages when running on stable. You should be able to use the #[stable_only_test] attribute to only run the test when we set CFG_RELEASE_CHANNEL=stable to simulate running on a stable rustfmt.

Not 100% sure if this approach will work. This might cause some issues with the configuration_snippet_tests, which checks to make sure that each configuration option is documented so maybe we'll need to work around that? We can totally brainstorm some other options though!

Do we need to define a new proc_macro_attribute macro?

We don't - I've verified that the attribute works as intended locally. The reason is that the unstable_variant annotation is processed by the config_type proc macro and stripped off - so it's never actually 'called', if that makes sense.

Also I believe the tests are failing because the doc tests for nightly_only_test and stable_only_test aren't quite right. We could fix them by adding /// # use rustfmt_config_proc_macro::nightly_only_test; and /// # use rustfmt_config_proc_macro::stable_only_test; to bring each attribute macro into scope for the doc tests. I can open up a PR for that a little later.

Thanks for this - I've opened #5389 with a fix for this plus CI to enforce it.

@tommilligan as well as updating Config::fill_from_parsed_config I also believe you'll need to modify Config::override_value, which ends up getting called when overriding the config with any values passed on the command line using --config

Thanks for pointing this out. Updated to factor out the common logic of updating with an unstable option/value from fill_from_parsed_config, and pointed override_value to it as well.

I think this points out that you could set unstable options using --config, and this is technically a breaking change that means you can't any more. However, I think this was never expected behaviour and should be classified as a bugfix.

I wonder if it would be possible to add a config_type to Config thats only available for testing.

Thanks for the hints on this. I actually found we already have a mock config object specifically for testing, so likewise added an option type for testing that has an unstable variant on it.

@ytmimi Thanks for all your feedback - I think I've addressed everything you raised. CI appears to be failing with a lot of unrelated errors that I don't think are to do with this PR?

@tommilligan as well as updating Config::fill_from_parsed_config I also believe you'll need to modify Config::override_value, which ends up getting called when overriding the config with any values passed on the command line using --config

Thanks for pointing this out. Updated to factor out the common logic of updating with an unstable option/value from fill_from_parsed_config, and pointed override_value to it as well.

I think this points out that you could set unstable options using --config, and this is technically a breaking change that means you can't any more. However, I think this was never expected behaviour and should be classified as a bugfix.

If I had to guess I'd say it's this behavior that's causing the CI to fail, since it's only the "stable" tests that are failing. To reiterate, the "stable" tests don't actually run on a stable version of rustfmt they just run with CFG_RELEASE_CHANNEL=stable to simulate running on a stable rustfmt.

In theory I agree that this would be a bugfix for unintended behaviour, but I think there's probably a larger discussion that needs to be had around whether we want to continue to let users set unstable options via the command line. #5025 looked to solve this by giving users control of that decision, and allowing them to decide whether rustfmt exits with an abort message or warns and continues formatting when using unstable options.

For now, can you simply issue the warning in override_value and still set the value regardless of the options stability. I think that will fix the issue you're seeing with CI

If I had to guess I'd say it's this behavior that's causing the CI to fail, since it's only the "stable" tests that are failing.

ah, thank you. Yes, makes-breaking-change-and-then-wonders-why-CI-is-failing sounds about right for me.

For now, can you simply issue the warning in override_value and still set the value regardless of the options stability. I think that will fix the issue you're seeing with CI

I think it would be counterintuitive to warn about unstable options/variants, while at the same time allowing them to be set from --config. I think I will preserve the current behaviour, which is to allow setting any option/value, whether they are stable or not. I've added a comment to this effect, with a link to the relevant discussion PR that you posted.

If this isn't desired, I can look into changing the behaviour of override_value, but would like some design guidance on what the expected behaviour should be for unstable variants specifically.

Added unit tests that confirm the behaviour of both fill_from_parsed_config and override_value for unstable variants of options.

ah, thank you. Yes, makes-breaking-change-and-then-wonders-why-CI-is-failing sounds about right for me.

Oh don't worry about it. I've definitely done the same thing before

I think it would be counterintuitive to warn about unstable options/variants, while at the same time allowing them to be set from --config. I think I will preserve the current behaviour, which is to allow setting any option/value, whether they are stable or not. I've added a comment to this effect, with a link to the relevant discussion PR that you posted.

That's fine. I think restoring the original behaviour for now makes sense and we can decide what the best way forward is after we've had some more time to think on it.

Added unit tests that confirm the behaviour of both fill_from_parsed_config and override_value for unstable variants of options.

Fantastic! I took a look at the tests and everything looks good to me!

Yes I want to review this one for sure

Apologies as I'm not fully up to date with the entirety of the conversation, but given the CLI discussion there's a few things I wanted to go ahead and note:

• rustfmt already allows unstable options to be used on stable via the command line override. Whether that's a feature or a bug has been a recurrent source of debate, but it's not one we need to tackle here nor do we need to try to force different behavior
• In general I'm not a fan of trying to bifurcate between command line and config file options, as I think that just adds unnecessary code to maintain and cognitive complexity on end users to little, if any, behavioral benefit
• Our implementation for stable vs. unstable variants should be compatible with our plans in #5025 to allow users to control the behavior, hopefully that's already the case with this one

• rustfmt already allows unstable options to be used on stable via the command line override. Whether that's a feature or a bug has been a recurrent source of debate, but it's not one we need to tackle here nor do we need to try to force different behavior

Yup, that's the consensus. We won't look to deal with that here

• In general I'm not a fan of trying to bifurcate between command line and config file options, as I think that just adds unnecessary code to maintain and cognitive complexity on end users to little, if any, behavioral benefit

The only difference being made right now is that a warning will be issued if you set an unstable value on stable via the config file, but if you set it from the command line no warning will be issued. I think that's fine for now, but I'm personally in favor of issuing the warning even if we end up setting the value. Because of the way #5025 is implemented a warning will be issued regardless of how the option was set once I rebase and integrate these changes.

• Our implementation for stable vs. unstable variants should be compatible with our plans in #5025 to allow users to control the behavior, hopefully that's already the case with this one

I'm almost positive that it is. Probably a few minor tweaks need to be made when I rebase to pull these changes in, but should be relatively painless.

This has unfortunately acquired a merge conflict, could you rebase and resolve when you get a chance? Overall I'm good with the approach, though may have a couple nits (e.g. function names) we can revisit at a later date.

The only difference being made right now is that a warning will be issued if you set an unstable value on stable via the config file, but if you set it from the command line no warning will be issued. I think that's fine for now, but I'm personally in favor of issuing the warning even if we end up setting the value

We can continue that discussion on your PR to identify target behavior. The historical thinking, aiui, was to simply warn when the user asked rustfmt for an option that it wasn't going to apply. That's why the current behavior when taking advantage of the --config loophole on stable doesn't produce any warnings (there's no need to warn the user an option they specified won't be used, because it's going to be used)

This has unfortunately acquired a merge conflict, could you rebase and resolve when you get a chance?

Squashed and rebased.

Overall I'm good with the approach, though may have a couple nits (e.g. function names) we can revisit at a later date.

Happy to, I tried to stick with existing convention as far as I could, even though I don't think they're the most intuitive right now.

...simply warn when the user asked rustfmt for an option that it wasn't going to apply. That's why the current behavior when taking advantage of the --config loophole on stable doesn't produce any warnings (there's no need to warn the user an option they specified won't be used, because it's going to be used)

Thanks for confirming this - happy this PR maintains the original intent to warn only when an option will not be respected despite being specified.

Thanks, that's great to hear. Fingers crossed, look forward to seeing it in some good use cases in future! And thanks for the great iteration process from both of you, it's really made working on this feature a pleasure.