Design Discussion for `style_edition` Configuration in rustfmt

ytmimi commented 1 year ago

Background

RFC "Style Evolution" (https://github.com/rust-lang/rfcs/pull/3338). Upstream tracking issue: https://github.com/rust-lang/rust/issues/105336

Requirements For The New `style_edition` config

After reading through the RFC these are the key requirements I found:

Evolve the current Rust style, without breaking backwards compatibility, by tying style evolution to Rust edition
style_edition 2015, 2018, or 2021 will use the existing default configuration values
Future style_edition (Rust 2024 and onwards) may use new default configuration values.
By default style_edition will use the same value as was configured for edition unless --style-edition is explicitly set when invoking rustfmt e.g. rustfmt --style-edition 2024 or specifically configured in rustfmt.toml. The precedence should be defined as:
- (CLI) --style-edition > (TOML) style_edition > (CLI) --edition > (TOML) edition
- This means that the style_edition specified on the command line has the highest priority, followed by explicitly setting style_edition in rustfmt.toml followed by edition specified on the command line and finally edition listed in rustfmt.toml. If none of the values are provided we'll fall back to the default edition which I believe is 2015.
rustfmt may choose not to support all combinations of Rust edition and style edition
rustfmt need not support every existing configuration option in new style editions and new configuration options may not be available for older style editions.
- Not a direct requirement for style_edition, but I think all configurations must define backwards compatible defaults.
New style_edition values will be initially introduced as unstable, which don't provide any stability guarantees.

Current Configuration Design

All types used for configuration must implement the ConfigType trait. There's a handy #[config_type] attribute macro which helps us easily implement ConfigType for enums.

/// Trait for types that can be used in `Config`.
pub(crate) trait ConfigType: Sized {
    /// Returns hint text for use in `Config::print_docs()`. For enum types, this is a
    /// pipe-separated list of variants; for other types it returns "<type>".
    fn doc_hint() -> String;

    /// Return `true` if the variant (i.e. value of this type) is stable.
    ///
    /// By default, return true for all values. Enums annotated with `#[config_type]`
    /// are automatically implemented, based on the `#[unstable_variant]` annotation.
    fn stable_variant(&self) -> bool {
        true
    }
}

Currently, the Config struct and all its fields are set using the create_config! macro. The macro lets us easily define the name, the type (which implements ConfigType), the default value, and the stability of the rustfmt configuration as well as giving it a description.

create_config! {
    // Fundamental stuff
    max_width: usize, 100, true, "Maximum width of each line";
    hard_tabs: bool, false, true, "Use tab characters for indentation, spaces for alignment";
    tab_spaces: usize, 4, true, "Number of spaces per tab";
    newline_style: NewlineStyle, NewlineStyle::Auto, true, "Unix or Windows line endings";
    indent_style: IndentStyle, IndentStyle::Block, false, "How do we indent expressions or items";
    // other config options ...
}

The current system only allows setting a single default value for each configuration option.

We can acquire a Config by calling one the following:

Config::default() to return the default config
Config.fill_from_parsed_config to mutate the current config with values parsed from a rustfmt.toml

New Design With `style_edition`

The Edition enum is a stable configuration option. we can use the #[unstable_variant] attribute to mark Edition::Edition2024 as unstable. However, if changing the stability of an already stable variant is an unacceptable breaking change then I'd propose adding a new StyleEdition enum that maintains parity with Edition and marking StyleEdition::Edition2024 as an #[unstable_variant]. Once style-edtion 2024 is stabilized we could remove the StyleEdition enum in favor of Edition.

Add a new StyleEditionDefault trait that will determine the default value for a given rustfmt configuration based on the edition and a new Error type to enumerate all the errors that could occur when trying to construct a Config, e.g the config not being available for the given style edition. Here's the general idea: (Note bounds may need to be revised)

use thiserror::Error;

#[derive(Error, Debug)]
pub enum ConfigError<C>
where
    C: Display
{
    /// The `Edition` used for parsing is incompatible with the `StyleEdition` used for formatting
    #[error("can't set edition={0} for parsing and style_edition={1} for formatting")]
    IncompatibleEditionAndStyleEdition(Edition, Edition),
    /// The configuration value is not supported for the specified `StyleEdition`.
    #[error("can't set {0}={1} when using style_edtion={2}")]
    IncompatibleOptionAndStyleEdition(String, C, Edition),
}

/// Defines the default value for the given style edition
pub(crate) trait StyleEditionDefault
where
    Self::ConfigType: ConfigType,
{
    type ConfigType;
    /// determine the default value for the give style_edition
    fn style_edition_default(style_edition: Edition) -> Result<Self::ConfigType, ConfigError>;

    /// Determine if the given value can be used with the configured style edition
    /// will be used to check if configs parsed from a `rustfmt.toml` can be used with the current style edition. 
    fn compatible_with_style_edition(
        value: Self::ConfigType,
        _style_edition: Edition
    ) -> Result<Self::ConfigType, ConfigError> {
        Ok(value)
    }
}

Instead of declaring default values directly in the create_config! macro we'll define new unit structs for existing non-enum configurations and implement StyleEditionDefault for those new types. Configuration values using enums will implement StyleEditionDefault directly.

For example, the max_width config, which is stored as a usize could be implemented as:

pub struct MaxWidth;

impl StyleEditionDefault for MaxWidth {
    type ConfigType = usize;
    fn style_edition_default(_edition: Edition) -> Result<Self::ConfigType, ConfigError> {
        Ok(100)
    }
}

and a config like NewlineStyle which is defined as an enum could be implement as:

impl StyleEditionDefault for NewlineStyle {
    type ConfigType = NewlineStyle;

    fn style_edition_default(_edition: Edition) -> Result<Self::ConfigType, ConfigError> {
        Ok(NewlineStyle::Auto)
    }
}

Once we've implemented StyleEditionDefault for all exiting configuration values the call to create_config! could be modified as follows:

create_config! {
    // Fundamental stuff
    style_edition: Edition, Edition, true, "The style edition being used";
    max_width: usize, MaxWidth, true, "Maximum width of each line";
    hard_tabs: bool, HardTabs, true, "Use tab characters for indentation, spaces for alignment";
    tab_spaces: usize, TabSpaces, true, "Number of spaces per tab";
    newline_style: NewlineStyle, NewlineStyle, true, "Unix or Windows line endings";
    indent_style: IndentStyle, IndentStyle, false, "How do we indent expressions or items";
    // other config options ...
}

A new constructor for Config could be added

impl Config {
    pub fn deafult_with_style_edition(style_edition: Edition) -> Result<Config, ConfigError>;
}

We'll remove Config::default() in favor of a default method that returns Result<Config, ConfigError> and modify fill_from_parsed_config to also return Result<Config, ConfigError>.

Additional changes regarding error handling / error propagation will need to be made to support the config constructors now being fallible.

Ergonomics

Similar to the #[config_type] attribute macro, I propose we also add a #[style_edition] attribute macro to make it easy to configure default values for all style editions.

macro_rules! macros can additionally help to define the necessary unit structs for configurations using primitive data types.

The #[style_edition] could work as follows:

Setting a default for all style editions

#[style_edition(100)]
pub struct MaxWidth;

#[style_edition(NewlineStyle::Auto)]
#[config_type]
pub enum NewlineStyle {
    /// Auto-detect based on the raw source input.
    Auto,
    /// Force CRLF (`\r\n`).
    Windows,
    /// Force CR (`\n).
    Unix,
    /// `\r\n` in Windows, `\n` on other platforms.
    Native,
}

Setting a default value and specific style edition values

#[style_edition(100, se2018=99, se2024=115)]
pub struct MaxWidth;

#[style_edition(NewlineStyle::Auto, se2024=NewlineStyle::Native)]
#[config_type]
pub enum NewlineStyle {
    /// Auto-detect based on the raw source input.
    Auto,
    /// Force CRLF (`\r\n`).
    Windows,
    /// Force CR (`\n).
    Unix,
    /// `\r\n` in Windows, `\n` on other platforms.
    Native,
}

Alternative for setting default values for enum configurations

#[style_edition]
#[config_type]
pub enum NewlineStyle {
    /// Auto-detect based on the raw source input.
    #[se_default]
    Auto,
    /// Force CRLF (`\r\n`).
    Windows,
    /// Force CR (`\n).
    Unix,
    /// `\r\n` in Windows, `\n` on other platforms.
    #[se2024]
    Native,
}

Specifying values that won't be supported by newer style editions

#[style_edition(Version::One)]
#[config_type]
/// rustfmt format style version.
pub enum Version {
    /// 1.x.y. When specified, rustfmt will format in the same style as 1.0.0.
    One,
    /// 2.x.y. When specified, rustfmt will format in the the latest style.
    #[supported_style_edition(2015, 2018, 2021)]
    Two,
}

Specify values that won't be supported for older style editions

Using a hypothetical new option to allow rustfmt to continue formatting even when comments would be dropped

#[style_edition]
#[config_type]
enum HandleDroppedComments {
    #[se_default]
   Dissallow,
   #[unsupported_style_edition(2015, 2018, 2021)]
   Allow
}

ytmimi commented 1 year ago

cc: @rust-lang/style

Would love some feedback on this to make sure that my ideas for how to implement style_edition line up with your expectations for the design laid out in the RFC.

joshtriplett commented 1 year ago

Most of this seems fine to me.

One thing that would be good to document explicitly: is it OK for some formatting to just directly look at the style edition, rather than requiring a configuration option for which the style edition sets a default? I'm asking because, for instance, backwards-incompatible bugfixes might not be worth a dedicated configuration option (especially not a stable one), and should perhaps just condition on the style edition directly. Or are you proposing that even those should be keying off of a (perhaps always unstable) "bugfix_2024_xyz" config option that defaults to true in 2024+ and false in 2015/2018/2021?

Stating explicitly: that's a rustfmt team call, not a style team call. My only interest in that is making sure that adding changes to a style edition has minimal overhead, and that we never find ourselves going "that change isn't worth it because there's too much overhead in adding a style edition change".

Also, I'm not sure I followed what this was intended to capture:

Not a direct requirement for style_edition, but I think all configurations must define backwards compatible defaults.

ytmimi commented 1 year ago

One thing that would be good to document explicitly: is it OK for some formatting to just directly look at the style edition, rather than requiring a configuration option for which the style edition sets a default?

Yes that's totally fine, and I was expecting things to work like that. We currently use Version::Two to gate bug fixes throughout the codebase (See https://github.com/rust-lang/rustfmt/issues/5577 for examples) and I'd expect style_edition to eventually replace version in that regard.

Also, I'm not sure I followed what this was intended to capture:

Not a direct requirement for style_edition, but I think all configurations must define backwards compatible defaults.

It's hard to say exactly what I was thinking since it's been some time since I wrote this up. It's possible I didn't articulated myself very well. At the end of my proposal above I describe two mechanisms we could implement to allow and disallow some configuration options from being set for a given style edition. I think users might find it odd if default configurations from one edition weren't available in newer editions and vice versa, but maybe this isn't an issue we need to worry about?

pitaj commented 9 months ago

@ytmimi

This means that the style_edition specified on the command line has the highest priority, followed by explicitly setting style_edition in rustfmt.toml followed by edition specified on the command line and finally edition listed in rustfmt.toml. If none of the values are provided we'll fall back to the default edition which I believe is 2015.

Is the last rustfmt.toml (emphasis mine) supposed to be Cargo.toml?

ytmimi commented 9 months ago

It's bee a very long time since I've looked at this, but I'm pretty sure I meant rustfmt.toml since rustfmt doesn't use Cargo.toml for any of its configurations. That being said, cargo fmt will look up the edition listed in the Cargo.toml file and pass that along to rustfmt (not sure if you were aware of that), and you can run cargo fmt -v to see the underlying rustfmt invokation and what arguments cargo fmt passes along.

rust-lang / rustfmt