feat: conditionally allow unstable opts on stable/beta

[calebcartwright]

One of two possible approaches to resolve #4081, and my vote would be for this approach.

AIUI, unstable_features is basically an no-op configuration item today that rustfmt doesn't actually use. With config options set in the configuration file, all unstable options are already allowed on nightly and disallowed on stable/beta. For config options specified/overriden via the CLI, rustfmt doesn't do any stability/channel checks whatsoever (unstable config opts can be used on stable even with unstable_features not set/set to false, see #4081)

With the changes in this PR:

• unstable options are still always available on nightly
• unstable options on stable/beta can be leveraged, but only when unstable_features is set to true (regardless of whether the unstable config options are set via the CLI and/or config file)

The other approach would be to completely remove unstable_features and completely disable the ability to opt-in to unstable rustfmt config options on stable/beta. IMHO rustfmt users have articulated some valid use cases when it would be beneficial for them to be able to opt-in to using unstable rustfmt opts on stable, which is why I prefer this approach over that alternative.

Also refs #4064 (comment) and #3387

[calebcartwright]

I will fix the tests on beta (would need the now-required unstable_options setting) tomorrow barring any objections to this approach. The integration test failures are unrelated to this PR and were caused by #4226

[calebcartwright]

These three integration test jobs are failing since rustfmt 2.0 is removing existing trailing commas from empty match blocks after #4226, so adding these to the allowed failure list til after the 2.0 release and the formatting changes are applied upstream:

 Diff in /tmp/tmp.hWkMNIst66/src/lib.rs:734:
 
                     // Empty match
                     match self.matches_from(follows_separator, file.clone(), i + ti + 1, options) {
-                        SubPatternDoesntMatch => {}, // keep trying
+                        SubPatternDoesntMatch => {} // keep trying
                         m => return m,
                     };

[calebcartwright]

Figured this would be better than just disabling the cfg snippet tests outright on beta

[calebcartwright]

@topecongiro - know you've already approved the core changes here, but before I merge I want to make sure you are okay with this one check added here in 788eb5a to fix the tests on beta vs adding an inline // rustfmt-unstable-features: true comment into each of the hundreds of test files under tests/{source,target} that would need them

[topecongiro]

Yeah, that sounds way better to me.

[Nemo157]

Allowing optin to unstable features on the stable channel seems to subvert the general stability guarantees that all other Rust tooling provides, rustc and cargo do not allow access to -Z flags or features on stable.

I'm mostly surprised that this did not require any kind of FCP, it seems like deviating from the general policy should be signed off by the dev-tools team.

[calebcartwright]

rustc and cargo do not allow access to -Z flags or features on stable.

Is this strictly true? using the unstable json format with libtest seems to work just fine on stable

cargo +stable test -- -Z unstable-options --format json

Allowing optin to unstable features on the stable channel seems to subvert the general stability guarantees that all other Rust tooling provide

I don't personally view unstable compiler features and rustfmt configuration options for formatting to be in the same mental bucket, but the thought/question does tie into some other related questions of late around how various Rust constraints are applied to rustfmt (refs #4346)

I'm mostly surprised that this did not require any kind of FCP, it seems like deviating from the general policy should be signed off by the dev-tools team.

It would be really helpful for me if we could get an articulation on which aspects of rustfmt the rustfmt team has the autonomy to make decisions on vs. which require approval of the Dev Tools team vs. any other RFC/FCP/etc. process, etc.

To my knowledge, the only things that have been codified are the RFC process to the Style Guide for style/formatting changes and the independent versioning of rustfmt in rust-lang/rfcs#2437

I'm happy to operate within whatever process we want to work with, but I'd prefer to get such a process documented to avoid any ambiguity and subjectivity.

cc @rust-lang/devtools

[ehuss]

@calebcartwright I opened rust-dev-tools/dev-tools-team#49 to discuss this in more detail.

[Manishearth]

Is this strictly true? using the unstable json format with libtest seems to work just fine on stable

That flag is not being passed to rustc, it's being passed to the test binary. That's not a binary shipped with rust, that's a binary built by the user, so it's a different scenario.

the independent versioning of rustfmt in rust-lang/rfcs#2437

Does that actually specify an independent versioning? It mentions an independent versioning of rustfmt for the cargo install one, however, it also says this (emphasis mine):

If using Rustfmt as a tool, the recommended way to install is via Rustup (rustup component add rustfmt-preview). This method does not require a nightly toolchain. Versioning is linked to the Rust toolchain (Rustfmt is not meaningfully versioned outside of the Rust version). The version of Rustfmt available on the nightly channel depends on the version of the rustfmt submodule in the Rust repo. This is manually updated approximately once per week

It also specifies:

Rustfmt can be configured with many options from the command line and a config file (rustfmt.toml). Options can be stable or unstable. Unstable options can only be used on the nightly toolchain and require opt-in with the --unstable-features command line flag. All options have default values and users are strongly encouraged to use these defaults.

(I understand that that's not the case in practice, without looking into it too much this seems like a bug but i would love to understand this situation better)

It later goes on to specify that breaking unstable options is a breaking change.

[Manishearth]

Also, independently of the RFC, i would ask myself what users would consider to be a major/breaking change, and if this falls in that bucket it might be worth an additional RFC!

[calebcartwright]

I will note that unstable rustfmt config options are already available to users today on stable, but only via the cli --confg opt. Whether that's a deliberate feature or a bug is questionable, but that is current state and just about any change to it could arguably be viewed as a breaking change.

This change hasn't been released yet in any version of rustfmt, and since I think there's a lot of things up in the air right now, it's probably best discussed in rust-dev-tools/dev-tools-team#49 and #4346. If we end up needing to revert this or pivot to a different (likely also breaking) approach we can certainly do so!

[bearcage]

@calebcartwright how would you feel about me doing a quick PR to backport this to 1.4?

I'm one of those users who uses the stable toolchain, but wants some unstable formatter options. As you've pointed out, the behavior exists today, it's just unwieldy.

If anybody else finds this useful, it's mostly doable to work around this on 1.4.36:

Right now I wrap my rustfmt invocations in a script which reads rustfmt.toml and translates it to --config options, ignoring anything that's an Array or a Table in toml (see below), then sets --config-path to an empty file. That said, though, doing this doesn't seem to be able to set an ignore list, since rustfmt doesn't appear to handle those as --config flags either. Just a guess, but I think the CLI parser is using ::parse() and IgnoreList errors out of FromStr unconditionally, causing ignore to always present as an invalid config key at the CLI.

Wrapping with extra CLI args is unfortunately not an uncommon pattern with rust tools (e.g. I do the same with linter config to work around clippy still not having an allow/deny list in config). Even so, though, reducing the number of places that's necessary seems like a worthwhile goal.