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

implement 2024 expression overflowing #6260

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rustfmt.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
error_on_line_overflow = true
error_on_unformatted = true
style_edition = "2024"
overflow_delimited_expr = false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to reduce churn, as the 2024 default (true) will require some updates to the formatting of the rustfmt codebase itself

65 changes: 64 additions & 1 deletion src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use getopts::{Matches, Options};

use crate::rustfmt::{
load_config, CliOptions, Color, Config, Edition, EmitMode, FileLines, FileName,
FormatReportFormatterBuilder, Input, Session, StyleEdition, Verbosity,
FormatReportFormatterBuilder, Input, Session, StyleEdition, Verbosity, Version,
};

const BUG_REPORT_URL: &str = "https://github.com/rust-lang/rustfmt/issues/new?labels=bug";
Expand Down Expand Up @@ -734,6 +734,25 @@ impl CliOptions for GetOptsOptions {
fn config_path(&self) -> Option<&Path> {
self.config_path.as_deref()
}

fn edition(&self) -> Option<Edition> {
self.inline_config
.get("edition")
.map_or(self.edition, |e| Edition::from_str(e).ok())
}

fn style_edition(&self) -> Option<StyleEdition> {
self.inline_config
.get("style_edition")
.map_or(self.style_edition, |se| StyleEdition::from_str(se).ok())
}

fn version(&self) -> Option<Version> {
self.inline_config
.get("version")
.map(|version| Version::from_str(version).ok())
.flatten()
}
}

fn edition_from_edition_str(edition_str: &str) -> Result<Edition> {
Expand Down Expand Up @@ -802,6 +821,17 @@ mod test {
options.inline_config = HashMap::from([("version".to_owned(), "Two".to_owned())]);
let config = get_config(None, Some(options));
assert_eq!(config.style_edition(), StyleEdition::Edition2024);
assert_eq!(config.overflow_delimited_expr(), true);
}

#[nightly_only_test]
#[test]
fn version_config_file_sets_style_edition_override_correctly() {
let options = GetOptsOptions::default();
let config_file = Some(Path::new("tests/config/style-edition/just-version"));
let config = get_config(config_file, Some(options));
assert_eq!(config.style_edition(), StyleEdition::Edition2024);
assert_eq!(config.overflow_delimited_expr(), true);
}

#[nightly_only_test]
Expand Down Expand Up @@ -846,6 +876,7 @@ mod test {
]);
let config = get_config(None, Some(options));
assert_eq!(config.style_edition(), StyleEdition::Edition2024);
assert_eq!(config.overflow_delimited_expr(), true);
}

#[nightly_only_test]
Expand Down Expand Up @@ -903,4 +934,36 @@ mod test {
let config = get_config(config_file, Some(options));
assert_eq!(config.style_edition(), StyleEdition::Edition2021);
}

#[nightly_only_test]
#[test]
fn correct_defaults_for_style_edition_loaded() {
let mut options = GetOptsOptions::default();
options.style_edition = Some(StyleEdition::Edition2024);
let config = get_config(None, Some(options));
assert_eq!(config.style_edition(), StyleEdition::Edition2024);
assert_eq!(config.overflow_delimited_expr(), true);
}

#[nightly_only_test]
#[test]
fn style_edition_defaults_overridden_from_config() {
let options = GetOptsOptions::default();
let config_file = Some(Path::new("tests/config/style-edition/overrides"));
let config = get_config(config_file, Some(options));
assert_eq!(config.style_edition(), StyleEdition::Edition2024);
assert_eq!(config.overflow_delimited_expr(), false);
}

#[nightly_only_test]
#[test]
fn style_edition_defaults_overridden_from_cli() {
let mut options = GetOptsOptions::default();
let config_file = Some(Path::new("tests/config/style-edition/just-style-edition"));
options.inline_config =
HashMap::from([("overflow_delimited_expr".to_owned(), "false".to_owned())]);
let config = get_config(config_file, Some(options));
assert_eq!(config.style_edition(), StyleEdition::Edition2024);
assert_eq!(config.overflow_delimited_expr(), false);
}
}
27 changes: 1 addition & 26 deletions src/config/config_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ macro_rules! create_config {
"fn_args_layout" => self.0.set_fn_args_layout(),
"hide_parse_errors" => self.0.set_hide_parse_errors(),
"version" => self.0.set_version(),
"edition" => self.0.set_edition(),
&_ => (),
}
}
Expand Down Expand Up @@ -165,7 +164,6 @@ macro_rules! create_config {
"fn_args_layout" => self.0.set_fn_args_layout(),
"hide_parse_errors" => self.0.set_hide_parse_errors(),
"version" => self.0.set_version(),
"edition" => self.0.set_edition(),
&_ => (),
}
}
Expand Down Expand Up @@ -210,7 +208,7 @@ macro_rules! create_config {
)+

#[allow(unreachable_pub)]
pub fn default_with_style_edition(style_edition: StyleEdition) -> Config {
pub(super) fn default_with_style_edition(style_edition: StyleEdition) -> Config {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have just been caught up in the difficulty of naming things, but I did feel odd having this function as well as default_for_possible_style_edition. While the function names imply a lot of overlap, I do feel they are doing very different things.

This function is very much instantiating a config with the provided style edition, whereas default_for_possible_style_edition is more of a "figure out what style edition we should be using, and then give me the default config for that style edition"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding doc comments to explain what each function's purpose is within the codebase.

Config {
$(
$i: (
Expand Down Expand Up @@ -264,7 +262,6 @@ macro_rules! create_config {
self.set_fn_args_layout();
self.set_hide_parse_errors();
self.set_version();
self.set_edition();
self
}

Expand Down Expand Up @@ -367,7 +364,6 @@ macro_rules! create_config {
"fn_args_layout" => self.set_fn_args_layout(),
"hide_parse_errors" => self.set_hide_parse_errors(),
"version" => self.set_version(),
"edition" => self.set_edition(),
&_ => (),
}
}
Expand Down Expand Up @@ -585,30 +581,9 @@ macro_rules! create_config {
option which takes precedence. \
The value of the `version` option will be ignored."
);
} else if matches!(self.version(), Version::Two) {
self.style_edition.2 = StyleEdition::Edition2024;
} else {
self.style_edition.2 = StyleEdition::Edition2015;
}
}

fn set_edition(&mut self) {
let style_edition_set = self.was_set().style_edition()
|| self.was_set_cli().style_edition();

if style_edition_set || self.was_set().version() {
return;
}

// User has explicitly specified an Edition value without
// explicitly specifying a Style Edition value, so the Style Edition
// must default to whatever value was provided for Edition
// as per: https://rust-lang.github.io/rfcs/3338-style-evolution.html#explanation
self.style_edition.2 = self.edition().into();
}



#[allow(unreachable_pub)]
/// Returns `true` if the config key was explicitly set and is the default value.
pub fn is_default(&self, key: &str) -> bool {
Expand Down
112 changes: 96 additions & 16 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,48 @@ impl PartialConfig {

::toml::to_string(&cloned).map_err(ToTomlError)
}

pub(super) fn to_parsed_config(
self,
style_edition_override: Option<StyleEdition>,
edition_override: Option<Edition>,
version_override: Option<Version>,
dir: &Path,
) -> Config {
Config::default_for_possible_style_edition(
style_edition_override.or(self.style_edition),
edition_override.or(self.edition),
version_override.or(self.version),
)
.fill_from_parsed_config(self, dir)
}
}

impl Config {
pub fn default_for_possible_style_edition(
style_edition: Option<StyleEdition>,
edition: Option<Edition>,
version: Option<Version>,
) -> Config {
// Ensures the configuration defaults associated with Style Editions
// follow the precedence set in
// https://rust-lang.github.io/rfcs/3338-style-evolution.html
// 'version' is a legacy alias for 'style_edition' that we'll support
// for some period of time
// FIXME(calebcartwright) - remove 'version' at some point
match (style_edition, version, edition) {
(Some(se), _, _) => Self::default_with_style_edition(se),
(None, Some(Version::Two), _) => {
Self::default_with_style_edition(StyleEdition::Edition2024)
}
(None, Some(Version::One), _) => {
Self::default_with_style_edition(StyleEdition::Edition2015)
}
(None, None, Some(e)) => Self::default_with_style_edition(e.into()),
(None, None, None) => Config::default(),
}
}

pub(crate) fn version_meets_requirement(&self) -> bool {
if self.was_set().required_version() {
let version = env!("CARGO_PKG_VERSION");
Expand All @@ -243,12 +282,23 @@ impl Config {
///
/// Returns a `Config` if the config could be read and parsed from
/// the file, otherwise errors.
pub(super) fn from_toml_path(file_path: &Path) -> Result<Config, Error> {
pub(super) fn from_toml_path(
file_path: &Path,
edition: Option<Edition>,
style_edition: Option<StyleEdition>,
version: Option<Version>,
) -> Result<Config, Error> {
let mut file = File::open(&file_path)?;
let mut toml = String::new();
file.read_to_string(&mut toml)?;
Config::from_toml(&toml, file_path.parent().unwrap())
.map_err(|err| Error::new(ErrorKind::InvalidData, err))
Config::from_toml_for_style_edition(
&toml,
file_path.parent().unwrap(),
edition,
style_edition,
version,
)
.map_err(|err| Error::new(ErrorKind::InvalidData, err))
}

/// Resolves the config for input in `dir`.
Expand All @@ -260,7 +310,12 @@ impl Config {
///
/// Returns the `Config` to use, and the path of the project file if there was
/// one.
pub(super) fn from_resolved_toml_path(dir: &Path) -> Result<(Config, Option<PathBuf>), Error> {
pub(super) fn from_resolved_toml_path(
dir: &Path,
edition: Option<Edition>,
style_edition: Option<StyleEdition>,
version: Option<Version>,
) -> Result<(Config, Option<PathBuf>), Error> {
/// Try to find a project file in the given directory and its parents.
/// Returns the path of the nearest project file if one exists,
/// or `None` if no project file was found.
Expand Down Expand Up @@ -305,12 +360,27 @@ impl Config {
}

match resolve_project_file(dir)? {
None => Ok((Config::default(), None)),
Some(path) => Config::from_toml_path(&path).map(|config| (config, Some(path))),
None => Ok((
Config::default_for_possible_style_edition(style_edition, edition, version),
None,
)),
Some(path) => Config::from_toml_path(&path, edition, style_edition, version)
.map(|config| (config, Some(path))),
}
}

pub(crate) fn from_toml(toml: &str, dir: &Path) -> Result<Config, String> {
#[allow(dead_code)]
pub(super) fn from_toml(toml: &str, dir: &Path) -> Result<Config, String> {
Self::from_toml_for_style_edition(toml, dir, None, None, None)
}
Comment on lines +373 to +375
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debated with myself whether this was necessary, but I opted to put it in for now so as to reduce the amount of diff in the PR. I think we can consider removing it later and just updating the various call sites (which were all in tests if I recall correctly)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't see any issue with from_toml, and if it's only used in test code then we don't have to make any changes right now


pub(crate) fn from_toml_for_style_edition(
toml: &str,
dir: &Path,
edition: Option<Edition>,
style_edition: Option<StyleEdition>,
version: Option<Version>,
) -> Result<Config, String> {
let parsed: ::toml::Value = toml
.parse()
.map_err(|e| format!("Could not parse TOML: {}", e))?;
Expand All @@ -324,12 +394,13 @@ impl Config {
err.push_str(msg)
}
}
match parsed.try_into() {

match parsed.try_into::<PartialConfig>() {
Ok(parsed_config) => {
if !err.is_empty() {
eprint!("{err}");
}
Ok(Config::default().fill_from_parsed_config(parsed_config, dir))
Ok(parsed_config.to_parsed_config(style_edition, edition, version, dir))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also didn't love the parsed_config.to_parsed_config piece here, as the idents make for a confusing combination. I think this could be avoided with a simple rename but as always naming things is hard 🤷

Copy link
Contributor

@ytmimi ytmimi Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I saw that and thought the wording was a bit confusing, but nothing too bad to stop us from moving forward with these changes.

}
Err(e) => {
err.push_str("Error: Decoding config file failed:\n");
Expand All @@ -347,17 +418,26 @@ pub fn load_config<O: CliOptions>(
file_path: Option<&Path>,
options: Option<O>,
) -> Result<(Config, Option<PathBuf>), Error> {
let over_ride = match options {
Some(ref opts) => config_path(opts)?,
None => None,
let (over_ride, edition, style_edition, version) = match options {
Some(ref opts) => (
config_path(opts)?,
opts.edition(),
opts.style_edition(),
opts.version(),
),
None => (None, None, None, None),
};

let result = if let Some(over_ride) = over_ride {
Config::from_toml_path(over_ride.as_ref()).map(|p| (p, Some(over_ride.to_owned())))
Config::from_toml_path(over_ride.as_ref(), edition, style_edition, version)
.map(|p| (p, Some(over_ride.to_owned())))
} else if let Some(file_path) = file_path {
Config::from_resolved_toml_path(file_path)
Config::from_resolved_toml_path(file_path, edition, style_edition, version)
} else {
Ok((Config::default(), None))
Ok((
Config::default_for_possible_style_edition(style_edition, edition, version),
None,
))
};

result.map(|(mut c, p)| {
Expand Down Expand Up @@ -768,7 +848,7 @@ binop_separator = "Front"
remove_nested_parens = true
combine_control_expr = true
short_array_element_width_threshold = 10
overflow_delimited_expr = false
overflow_delimited_expr = true
struct_field_align_threshold = 0
enum_discrim_align_threshold = 0
match_arm_blocks = true
Expand Down
5 changes: 4 additions & 1 deletion src/config/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,9 @@ pub trait CliOptions {
/// It is ok if the returned path doesn't exist or is not canonicalized
/// (i.e. the callers are expected to handle such cases).
fn config_path(&self) -> Option<&Path>;
fn edition(&self) -> Option<Edition>;
fn style_edition(&self) -> Option<StyleEdition>;
fn version(&self) -> Option<Version>;
}

/// The edition of the syntax and semantics of code (RFC 2052).
Expand Down Expand Up @@ -625,7 +628,7 @@ config_option_with_style_edition_default!(
RemoveNestedParens, bool, _ => true;
CombineControlExpr, bool, _ => true;
ShortArrayElementWidthThreshold, usize, _ => 10;
OverflowDelimitedExpr, bool, _ => false;
OverflowDelimitedExpr, bool, Edition2024 => true, _ => false;
StructFieldAlignThreshold, usize, _ => 0;
EnumDiscrimAlignThreshold, usize, _ => 0;
MatchArmBlocks, bool, _ => true;
Expand Down
13 changes: 12 additions & 1 deletion src/git-rustfmt/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ use getopts::{Matches, Options};
use rustfmt_nightly as rustfmt;
use tracing_subscriber::EnvFilter;

use crate::rustfmt::{load_config, CliOptions, FormatReportFormatterBuilder, Input, Session};
use crate::rustfmt::{
load_config, CliOptions, FormatReportFormatterBuilder, Input, Session, Version,
};

fn prune_files(files: Vec<&str>) -> Vec<&str> {
let prefixes: Vec<_> = files
Expand Down Expand Up @@ -89,6 +91,15 @@ impl CliOptions for NullOptions {
fn config_path(&self) -> Option<&Path> {
unreachable!();
}
fn edition(&self) -> Option<rustfmt_nightly::Edition> {
unreachable!();
}
fn style_edition(&self) -> Option<rustfmt_nightly::StyleEdition> {
unreachable!();
}
fn version(&self) -> Option<Version> {
unreachable!();
}
}

fn uncommitted_files() -> Vec<String> {
Expand Down
Loading
Loading