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

brace_style not implemented, among other things #5433

Closed
BenjaminBrienen opened this issue Jul 8, 2022 · 6 comments
Closed

brace_style not implemented, among other things #5433

BenjaminBrienen opened this issue Jul 8, 2022 · 6 comments

Comments

@BenjaminBrienen
Copy link

BenjaminBrienen commented Jul 8, 2022

rustfmt.toml

#array_width = 60
#attr_fn_like_width = 70
binop_separator = "Front"
blank_lines_lower_bound = 0
blank_lines_upper_bound = 1
brace_style = "AlwaysNextLine"
#chain_width = 60
color = "Always"
combine_control_expr = false
comment_width = 80
condense_wildcard_suffixes = false
control_brace_style = "AlwaysNextLine"
disable_all_formatting = false
doc_comment_code_block_width = 140
edition = "2021"
empty_item_single_line = true
enum_discrim_align_threshold = 30
error_on_line_overflow = false
error_on_unformatted = false
fn_args_layout = "Vertical"
#fn_call_width = 60
fn_single_line = true
force_explicit_abi = true
force_multiline_blocks = true
format_code_in_doc_comments = true
format_generated_files = true
format_macro_bodies = true
format_macro_matchers = true
format_strings = false
group_imports = "StdExternalCrate"
hard_tabs = true
hex_literal_case = "Upper"
hide_parse_errors = false
ignore = []
imports_granularity = "Crate"
imports_indent = "Block"
imports_layout = "Vertical"
indent_style = "Block"
inline_attribute_width = 0
match_arm_blocks = true
match_arm_leading_pipes = "Never"
match_block_trailing_comma = false
max_width = 140
merge_derives = true
newline_style = "Unix"
normalize_comments = true
normalize_doc_attributes = true
overflow_delimited_expr = false
remove_nested_parens = true
reorder_impl_items = true
reorder_imports = true
reorder_modules = true
#required_version = "1.4.38"
short_array_element_width_threshold = 10
#single_line_if_else_max_width = 50
skip_children = false
space_after_colon = true
space_before_colon = false
spaces_around_ranges = false
struct_field_align_threshold = 30
struct_lit_single_line = true
#struct_lit_width = 30
#struct_variant_width = 35
tab_spaces = 100
trailing_comma = "Vertical"
trailing_semicolon = false
type_punctuation_density = "Wide"
unstable_features = true
use_field_init_shorthand = true
use_small_heuristics = "Default"
use_try_shorthand = true
#version = "Two"
where_single_line = true
wrap_comments = true

#emit_mode = "Files"
#make_backup = false
#print_misformatted_file_names = false

main.rs

#![feature(int_log)]

fn main()
{
	(1..=1_000_000u32).for_each(|n| {
		let result: u32 = n.log2();
		println!("{result}");
	});
	let lorem = MyStruct
	{
		i: 1,
	};
	let mut value1:MyStruct=MyStruct{i:69_420i32,};
	let value2:MyStruct=MyStruct
	{
i
:
6969_420i32
,
	};

	unsafe {
		value1=MyStruct{
i:value2.i, };
	}

	let value1 = value1;
	let my_num = ((value1.i + value2.i).log(2));
	println!("{my_num}");
}

struct MyStruct
{
	i: i32,
}

How?

This is after cargo fmt. I don't know why it gets it so wrong. Why is it not even enforcing indentation? Why are the braces not on the next line according to brace_style = "AlwaysNextLine"? Why is lorem not condensed onto one line according to struct_lit_single_line = true? Why does it not put spaces around characters like : or =? Why does it not remove superfluous parentheses on line 28? This is on the latest nightly. I assume I'm doing something wrong.
Untitled

@calebcartwright
Copy link
Member

Thanks for reaching out. As a general ask/recommendation, it's a lot easier to share, review, and discuss specifics in focused conversation, with as minimal a repro as possible (both config and code) and to try to refrain from a catch-all set of issues/questions.

Given that you started the title and initial question relative to the brace_style, I'll try to speak to this first to hopefully clarify what this option does, and what it doesn't do.brace_style as originally introduced only applied to items and not every possible syntactical construct where braces may occur. There's more discussion and detail on this in the stabilization tracking issue for brace_style #3376, but suffice to say it's currently expected that the option doesn't apply in certain contexts. You've probably got something else going on as well, but I suspect the nature of the option may be at least part of it.

Myself or someone else will try to take a closer look at the "other things" as you put it 😄 when time permits, but in the interim if you have the time, it would speed things along if you could try to reduce down the set of config options to the subset that are producing the problem you are experiencing

@BenjaminBrienen
Copy link
Author

Turns out this is the same root issue as #5431
This formatting madness is mostly solved by lowering tab_spaces so that it doesn't crash.

@BenjaminBrienen
Copy link
Author

BenjaminBrienen commented Jul 8, 2022

only applied to items and not every possible syntactical construct where braces may occur

Why was this the go-to implementation of a feature called braces_style? I imagine it's not very difficult to put a newline character and the proper number of tabs before each { in the file, compared to parsing the "syntactical constructs" and only applying the rule to some of them. Obviously there's other things to consider like other, more specific rules like one-lining a struct instance, but still, this has been an open issue for literally years. I'm going to close this issue and further discussion can take place in #3376. I appreciate the work being done keeping everything in order and carefully considered.

@calebcartwright
Copy link
Member

Why was this the go-to implementation of a feature called braces_style? I imagine it's not very difficult to put a newline character and the proper number of tabs before each { in the file, compared to parsing the "syntactical constructs" and only applying the rule to some of them.

I'd encourage you read through the discussion in the issue I linked in my last comment, where the name of the still-unstable option is an explicit discussion point. Also keep in mind that while some users, perhaps including yourself, may want a categorical application of brace styling, there's plenty of others that want different behavior depending on the context (e.g. items vs patterns vs expressions). In these scenarios it's best for us to try to support the community at large instead of trying to pick one camp at the expense of another.

Your second sentence is tantamount to "why wasn't the entire product implemented in a completely different manner", which isn't a topic that's really worth debating at this point, but in general: there's two typical flavors of code formatters, one which follows the token processing and matching from the source file as you describe, and the other that deals with parsing the input and formatting the AST representation of the program against a set of rules (a pretty-printer).

The latter is the approach the Rust community took many, many years ago and is not going to change. There's benefits and tradeoffs to each approach, but the AST pretty-printer approach was taken because it was the best solution to achieve Rust's goals of having a unified default style for Rust code across codebases and to best support the more advanced types of syntactical transformations and restructuring that the community wanted.

That default style is codified in the Rust Style Guide, rustfmt is simply the implementing tool, where we've also added configuration options to allow folks to get results that differ from those codified prescriptions.

@BenjaminBrienen
Copy link
Author

I appreciate the very well thought-out reply. I hope I didn't come across as rude. I actually did read the discussion before making the issue here, but not so closely that I remembered that talking point. I was just trying to figure out what's been going on since 2020 when someone commented on closures and such. I completely agree that having fine-tuned behavior per syntactical construct is the optimal solution. I didn't mean to imply that the ideal solution was a one-size-fits-all setting true/false. Rather, I was wondering why that wasn't that starting point, or at least the base behavior, which other, more specific settings could tweak from there. From what you say, I'm assuming that the style of processing done by rustfmt makes a blanket all_braces_newline_default_style option infeasible. I can accept that. I'm just surprised the extremely common preference of such a theoretical setting being set to true hasn't been implemented, or I'm surprised that it isn't a more common request. Again, I appreciate the work that has been done so far, and I look forward to this tool reaching its full potential. I'd be willing to help it along the way once I become more proficient in rust.

@calebcartwright
Copy link
Member

calebcartwright commented Jul 8, 2022

No worries, and no rudeness whatsoever, just wanted to make sure you'd seen the other thread. Certainly feel free to ask any questions (there or here) if anything is unclear; I realize some of the discussion over there may presume some inherent knowledge of rustfmt usage that's not necessarily obvious to those new to the tool.

what's been going on since 2020 when someone commented on closures and such
I'm just surprised the extremely common preference of such a theoretical setting being set to true hasn't been implemented
I'd be willing to help it along the way once I become more proficient in rust.

Starting with the last one first, that would be fantastic! We've probably got some decent issues in this repository that relatively simple and perhaps a good way to get some Rust practice in.

As to the other points, which share a general theme, this basically just boils down to the basics of open source volunteerism and workloads/priorities vs bandwidth constraints. rustfmt's core goal is to format Rust code according to the style guide as we were discussing earlier, and there's more than enough work on that front to occupy the entirety of the bandwidth of the rustfmt team.

Config options exist to allow users to do something that diverges from that core goal, because they are all about formatting code in ways that's different from the style guide. As such that takes a priority backseat by default, and that's why we typically don't do much work on config options ourselves these days (though there's occasionally exceptions as and when an option has strong alignment to some other goals).

Most often, new config options get added by someone from the community when there's a different style they want badly enough that they are willing to contribute themselves, and the same type of pattern largely holds true for stabilization (someone sufficiently motivated to have an option available on stable for their usage). The difference with stabilization is that a lot more users are on nightly toolchains than one might suspect, so the potential pool of users is even smaller compared to the existence/adding of a new option. We assist users on both fronts, but it's just rarely something we drive ourselves.

Additionally, brace_style, especially in non-item and non-control flow expression contexts hasn't been a hot request/need at all, especially compared to other options (compare the number of commentors, reactions, etc. of that option as compared to one like say imports_granularity.

I'm assuming that the style of processing done by rustfmt makes a blanket all_braces_newline_default_style option infeasible

Nope, your assumption is incorrect (fortunately!). I spoke to this in #3376 (comment), but that same type of pattern of having one top-level option that controls everything, but which also influences/works with more granular options is not only possible, it's actually something we've already done before in other cases.

(edit: typo fixes)

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

No branches or pull requests

2 participants