Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config tracking of non-default CLI flags #6252

Closed
calebcartwright opened this issue Jul 26, 2024 · 0 comments · Fixed by #6253
Closed

Config tracking of non-default CLI flags #6252

calebcartwright opened this issue Jul 26, 2024 · 0 comments · Fixed by #6253

Comments

@calebcartwright
Copy link
Member

rustfmt's configuration loading/building process includes a flag that indicates whether the user explicitly set the value. However, for unknown reasons, that flag is only captured when the config option is specified from two of the three possible ways.

The third way configuration values can be set is directly via a CLI flag, such as --edition or --unstable-features, but when options are set this way they aren't properly "flagged" as having been set by the user.

Internally rustfmt uses this was_set() flag for various options for various reasons, so this gap has been a minor inconvenience in the past. While working on some 2024 edition work I found this inconvenience had escalated to a more problematic state and I think it's something that we need to address (though I think we can technically work around it if we must)

Some relevant code blocks

// For each config item, we store:
//
// - 0: true if the value has been access
// - 1: true if the option was manually initialized
// - 2: the option value
// - 3: true if the option is unstable
$($i: (Cell<bool>, bool, <$ty as StyleEditionDefault>::ConfigType, bool)),+

rustfmt/src/config/mod.rs

Lines 729 to 733 in ea02de2

// When we don't set the config from toml or command line options it
// doesn't get marked as set by the user.
assert_eq!(config.was_set().unstable_features(), false);
config.set().unstable_features(true);
assert_eq!(config.unstable_features(), true);

Here the flag is set (for options specified in the config file or via the --config ... command line

fn fill_from_parsed_config(mut self, parsed: PartialConfig, dir: &Path) -> Config {
$(
if let Some(option_value) = parsed.$i {
let option_stable = self.$i.3;
if $crate::config::config_type::is_stable_option_and_value(
stringify!($i), option_stable, &option_value
) {
self.$i.1 = true;
self.$i.2 = option_value;

pub fn override_value(&mut self, key: &str, val: &str)
{
match key {
$(
stringify!($i) => {
let value = val.parse::<<$ty as StyleEditionDefault>::ConfigType>()
.expect(
&format!(
"Failed to parse override for {} (\"{}\") as a {}",
stringify!($i),
val,
stringify!(<$ty as StyleEditionDefault>::ConfigType)
)
);
// Users are currently allowed to set unstable
// options/variants via the `--config` options override.
//
// There is ongoing discussion about how to move forward here:
// https://github.com/rust-lang/rustfmt/pull/5379
//
// For now, do not validate whether the option or value is stable,
// just always set it.
self.$i.1 = true;
self.$i.2 = value;

Whereas config options set programmatically by rustfmt (e.g. via a top-level cli option) are not:

impl<'a> ConfigSetter<'a> {
$(
#[allow(unreachable_pub)]
pub fn $i(&mut self, value: <$ty as StyleEditionDefault>::ConfigType) {
(self.0).$i.2 = value;
match stringify!($i) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant