Thank you for the PR! You've raised some good questions that I need to think on a bit.

An interesting wrinkle to the testing is that we actually only run our own CI/local builds on nightly, so we'd either need some trickery for the unit tests, or would otherwise just have to drop them and rely on human testing/validation.

The integration test story might be "easier" in the sense that we could compile on nightly and then use the stable version of cargo fmt and the associated env var to utilize the built version of rustfmt instead of the bundled one.

As far as the behavior goes, as long as all the unstable/unavailable options are still printed (as opposed to just the first one and then bailing), then I think that should be good

Thinking about the implementation a bit more and glancing over the diff, I've a few thoughts:

• The behavior should be baked into the rustfmt library, and not the responsibility of consumers (e.g. the rustfmt bin, users of the lib, etc.) to insert another call to perform the check. The load_config function seems like a reasonable place for this at first blush, though may require a new custom error definition with corresponding updates to the signature and call sites
• Are we accounting for unstable options from both config files and command line overrides? (e.g. rustfmt foo.rs --config some_unstable_thing=bar)
• Instead of blending the concerns of aggregating options and display text the user, what do you think about defining a new struct to represent the options (and values), storing those (instead of error message Strings), and then encapsulating all the display text content in one place?

• The behavior should be baked into the rustfmt library, and not the responsibility of consumers (e.g. the rustfmt bin, users of the lib, etc.) to insert another call to perform the check. The load_config function seems like a reasonable place for this at first blush, though may require a new custom error definition with corresponding updates to the signature and call sites

Also agree that it would be a lot better to have this baked into the rustfmt library. I think I initially implemented it the way that I did because I didn't want to make changes to function calls / signatures, but if you're alright with it then I'll update the implementation!

Would it make sense for load_config to return an anyhow::Result instead of a Result<(Config, Option<PathBuf>), std::io::Error>? Might make it easier to return a custom error type.

• Are we accounting for unstable options from both config files and command line overrides? (e.g. rustfmt foo.rs --config some_unstable_thing=bar)

currently, I think that we're only tracking unstable options in a configuration file. Could you point me to where we parse the command line overrides.

• Instead of blending the concerns of aggregating options and display text the user, what do you think about defining a new struct to represent the options (and values), storing those (instead of error message Strings), and then encapsulating all the display text content in one place?

I think this is a great idea. I was hoping you could give an example to go off of.

I didn't want to make changes to function calls / signatures, but if you're alright with it then I'll update the implementation

Understood. Fortunately now that we're no longer publishing rustfmt to crates.io we don't need to be constrained by semver compatibility on the public API of the library. There's a handful of folks that are still attempting to consume as a library directly from source, but we've made it clear to them that the API is subject to change without notice.

Feel free to suggest any changes that you think would be beneficial 👍

Would it make sense for load_config to return an anyhow::Result instead of a Result<(Config, Option), std::io::Error>? Might make it easier to return a custom error type.

Feel free to give it a try

currently, I think that we're only tracking unstable options in a configuration file. Could you point me to where we parse the command line overrides.

Ah that's right, and there's been some past debate about whether the ability to use unstable opts on stable via the cli override is a feature or a bug. IIRC the returned Config already accounts for the values loaded from config files as well as any cli overrides so there shouldn't be any concern (at least for now given current behavior).

I was hoping you could give an example to go off of.

This one is a bit more complicated than what we need on the config side, but here's the main error kind def we return from the main entry point

Note that we don't necessarily have to use an error, but I think we'll want some kind of extension to the return type either way that callers (including the rustfmt bin) can use

I've been doing a little more work on this PR to implement the custom error return type for load_config. and I've run into an issue. I'm not 100% sure if load_config is the right place to emit the error because it leads to some inconsistencies.

The main issue is that load_config is called twice from main::format, and if there are unstable options passed from the command line, then we never get to the second invocation of load_config. This leads to 3 different scenarios where we could encounter unstable options. For the examples below assume we're using this rustfmt.toml file.

error_on_line_overflow = true
error_on_unformatted = true
version = "Two"

1) Unstable options are passed from the command line and we don't use --config-path:

Regardless of whether or not Unstable options exist in the default (unspecifed) rustfmt.toml only the command line options get warned / aborted on.

cargo run --bin rustfmt -- --check --config=abort_on_unrecognised_options=true,reorder_impl_items=true src\macros.rs

Can't set nightly options when using stable rustfmt
    - `reorder_impl_items = true`

Set `abort_on_unrecognised_options = false` to convert this error into a warning

2) Unstable options are not used on the command line but they exist in the default rustfmt.toml

If we don't pass unstable options from the command line and we don't pass the --config-path flag, and there are unstable options in the default rustfmt.toml file, then we'll abort / warn on the unstable options listed in the default rustfmt.toml.

cargo run --bin rustfmt -- --check --config=abort_on_unrecognised_options=true src\macros.rs

Can't set nightly options when using stable rustfmt
    - `error_on_unformatted = true`
    - `version = Two`
    - `error_on_line_overflow = true`

Set `abort_on_unrecognised_options = false` to convert this error into a warning

3) Unstable options are passed from the command line and the --config-path

This is the only scenario where all unstable options are aborted / warned on.

cargo run --bin rustfmt -- --check --config=abort_on_unrecognised_options=true,reorder_impl_items=true src\macros.rs --config-path=rustfmt.toml

Can't set nightly options when using stable rustfmt
    - `version = Two`
    - `error_on_unformatted = true`
    - `error_on_line_overflow = true`
    - `reorder_impl_items = true`

Set `abort_on_unrecognised_options = false` to convert this error into a warning

How to move forward?

In one of your earlier comments you mentioned you didn't want this type of behavior.

As far as the behavior goes, as long as all the unstable/unavailable options are still printed (as opposed to just the first one and then bailing), then I think that should be good

I guess what I'm wrestling with is these changes bring the abort/warn functionality into the rustfmt lib, but it's the consumer of the lib main::format that's not implemented in a way to properly collect all unstable options.

Do you want me to also make changes to how main::format is implemented or is that a separate issue outside the scope of getting this feature into the project?

I'm not 100% sure if load_config is the right place to emit the error because it leads to some inconsistencies

Just to clarify my original line of thinking, I don't think load_config should be doing any emission itself whatsoever, but I was suggesting that it's return type include the information about specified (but unavailable) unstable options.

As a general philosophy I think libraries should be cautious about writing directly to stdout/stderr buffers, especially in a manner that callers/consumers can't control. This starts to become more concretely relevant for us as we've had requests for error presentation to be incorporated into certain emitters vs. dumped to std{out,err} (e.g. if a user has selected the json emitter they'd like to get operational errors like invalid config included in the json object too)

I'm a bit surprised by the scenario you reported in number 1 because I thought the CLI overrides were set with a dedicated function that's on an entirely different call path than the standard config loading, and accordingly I was thinking it'd be possible to handle config file vs. cli overrides differently. Perhaps I'm remembering incorrectly though

Regardless, and to clarify a bit more, I'm not terribly concerned if we can't display all unstable opts from both config files and command line overrides. I still have a sense that we can, but the scenario I was really trying to avoid was with your example config file:

error_on_line_overflow = true
error_on_unformatted = true
version = "Two"

rustfmt only emitting a complaint about error_on_line_overflow the first time, then after the user removes that one it complains about error_on_unformatted, etc.

I realize that may not have addressed each individual item from your last comment, but does it help?

As a general philosophy I think libraries should be cautious about writing directly to stdout/stderr buffers, especially in a manner that callers/consumers can't control. This starts to become more concretely relevant for us as we've had requests for error presentation to be incorporated into certain emitters vs. dumped to std{out,err} (e.g. if a user has selected the json emitter they'd like to get operational errors like invalid config included in the json object too)

I see what you're saying. The current implementation still prints a warning to stderr, which is the same behavior as before, but after some updates to the implementation the abort error bubbles up to src::bin::main.rs::main where it's caught and printed to stderr.

I've made sure to include the unstable options in the new LoadConfigurationError enum, so if we wanted to make some updates in the future to emit the unstable options differently we can probably do it without too much hassle.

I'm a bit surprised by the scenario you reported in number 1 because I thought the CLI overrides were set with a dedicated function that's on an entirely different call path than the standard config loading, and accordingly I was thinking it'd be possible to handle config file vs. cli overrides differently. Perhaps I'm remembering incorrectly though

You're right about the command line options being set using Config::override_value and toml configuration being loaded using Config::fill_from_parsed_config, however when we call main::format (code linked bellow) the first thing we do is call load_config.

In scenario 1) above, this causes load_config to immediately return a LoadConfigurationError, but we only parse the inline configuration. In scenario 2) we make it to the second load_config call, and in scenario 3 we bail again at the first load_config call.

I guess one scenario I didn't explore was if the abort_on_unrecognised_options=true was set in the default toml file. In that case I suspect we'd make it to the second load_config call and then bail from there.

Regardless, and to clarify a bit more, I'm not terribly concerned if we can't display all unstable opts from both config files and command line overrides.

Got it! In that case I think the current implementation achieves what you had in mind.

I realize that may not have addressed each individual item from your last comment, but does it help?

Helps out a lot, thanks! I think we're both on the same page with everything. I think the implementation achieves everything you were looking for, but if not please let me know so I can make adjustments.

The last piece of this is the testing story. I know you mentioned that we aren't testing "stable" in CI, but I was wondering if toggling the CFG_RELEASE_CHANNEL environment variable from nightly to stable is possible / would have any affect. I'm using the is_nightly_channel! macro along with abort_on_unrecognised_options to determine if rustfmt should abort, warn, or continue.

Now that #5109 #5112 have been merged into master we have a mechanism to test nightly only and stable only code, and we're now testing both stable and nightly in CI. I'll update the code to use the new testing attribute macros.

I might've confused myself with the labels 😆 Remind me, is this just waiting on me to take another pass or were there still some other changes you had in mind?

Remind me, is this just waiting on me to take another pass or were there still some other changes you had in mind?

There are still a few minor tweaks that I'd like to make, like using the #[nightly_only_test] and #[stable_only_test] attributes. I also need to go back to the docs and update the new configuration section with the link to the tracking issue.

There might be other little changes too, but it's been a little while so I need to review the PR myself to know for sure 😅

@calebcartwright Okay, now I think I'm ready for a re-review (assuming all the CI that's running right now passes successfully!). The PR is on the larger side, so no rush on this.

I added end-to-end tests to tests/rustfmt/main.rs to cover almost all of the different test cases that I outlined in the original comment. One thing the tests currently fail to do is verify that files were either formatted or left alone. I'm not sure what the best way to do that would be, but maybe that's something we can look to address in the future. I made some updates to the existing test macro assert_that! so that I could also test rustfmt's exit code.

Definitely let me know if we still need to make changes here.

The last thing that I want to make sure about is that the new configuration option has the most descriptive name we can give it. Do you think that abort_on_unrecognised_options does the job?

Apologies in advance for what may seem like me moving the goal posts, but I'm actually now thinking that we may want to perform the check elsewhere.

Main points driving my rationale:

• minimize breaking changes to the API surface (yes we've got some flexibility, but better to avoid a change unless we really need it)
• separation of concerns and ensuring the gate is enforced strictly within rustfmt itself (the lib)
• ability to better interop error with emitters (not something we should do now, but making it easier to do so down the road)

Accordingly, what if we instead performed the "are we on stable, were there any unstable options, and if both then what's the expected behavior" logic check within format_input_inner similar to the version check:

https://github.com/rust-lang/rustfmt/blob/master/src/formatting.rs#L31-L40

I think a new ErrorKind variant that has a UnstableOptions field would work and be able to largely reuse most of the error message logic you have. I'd also like to have the continue/abort behavior be binary, with the continue path still printing out the warning regardless (i.e. I don't want to introduce a silent continue type of behavior)

The last thing that I want to make sure about is that the new configuration option has the most descriptive name we can give it. Do you think that abort_on_unrecognised_options does the job?

I always strive to avoid double negatives as those tend to be far too easy for folks to mix up or otherwise cause confusion, so would suggest we invert with something like "format with unusable, unstable options specified on stable" that's defaulted to true as oppose to current "abort" with default of false

Thoughts?

I like all your suggestions. I moved the exit early / warning logic to format_input_inner, and I added a new ErrorKind variant. I'm still going to think on the name for a little bit.

I still need to:

• Set the default value for the option to true
• Setup a tracking issue and update the README

Let me know if there's anything else you'd like me to do on this that I'm missing.