Add max_content_width option

dhardy commented 4 years ago

Currently, splitting behaviour appears to be mostly dependent on max_width, unless some specific option overrides/augments this. I don't fully understand why, either.

Example, with max_width = 140 (line = 105 chars, 85 excluding indent):

                    accum = accum.checked_add(x).unwrap_or(Duration::new(u64::max_value(), 999_999_999));

Again, with max_width = 120 (longest line = 81 chars, 57 excluding indent):

                    accum = accum
                        .checked_add(x)
                        .unwrap_or(Duration::new(u64::max_value(), 999_999_999));

As I see it, there are two reasons to break long lines:

Available width in editors. This may or may not be an issue (it's common to have lots of spare width; some people like myself like to use variable-width fonts).
Readability. Too much content on one line is hard to parse.

Note that there are already a couple of more specific width rules:

comment_width: since comments are somewhat different to code, it makes sense to have this separate
inline_attribute_width: controls two things; (a) whether to use inline attributes at all (default: no), and (b) when, specifically, to allow them

Proposal:

[ ] Add a max_content_width option; split lines when either the total line length exceeds max_width or the length minus indent exceeds max_content_width
[ ] Replace inline_attribute_width with inline_attributes (true/false), using max_content_width to control behaviour

dhardy commented 4 years ago

Note: this may provide an appropriate solution to my request in #3358 too.

calebcartwright commented 4 years ago

Currently, splitting behaviour appears to be mostly dependent on max_width, unless some specific option overrides/augments this. I don't fully understand why, either

Sounds like you're running into the other major factor that determines single vs. multi line: rustfmt's calculated width heuristics.

The width heuristics are calculated internally based on other config options (including max_width) and these establish max widths for various items (chains, struct lits, etc.).

By default, these max width's are calculated as some fraction of the max_width value, and rustfmt then uses these calculated max width values during formatting. So as an example, even if max_width is set to 100, the max width for an array that rustfmt uses is 60 https://github.com/rust-lang/rustfmt/blob/5eb06d035451d1dfbacf0795b3df7815b123581a/src/config/options.rs#L229-L249

ATM, the calculated width heuristics can only be controlled via the use_small_heuristics config option. You can try setting use_small_heuristics to Off or Max in your rustfmt config file to see how those vary.

With the rustfmt 2.x release, the plan is to expose these individual/granular max widths as config options so that users can control each of those individual max's if they want.

dhardy commented 4 years ago

Sounds good!

I've been carefully going through rustfmt's tweaks to the Rand codebase. Here are a few diffs I didn't like which may/may not be fixed by this option.

Function call gets single-lined but is less readable:

diff --git a/rand_core/src/le.rs b/rand_core/src/le.rs
index c247ab7b3f..d753c89bc4 100644
--- a/rand_core/src/le.rs
+++ b/rand_core/src/le.rs
@@ -18,10 +18,7 @@ macro_rules! read_slice {
         assert_eq!($src.len(), $size * $dst.len());

         unsafe {
-            ptr::copy_nonoverlapping(
-                $src.as_ptr(),
-                $dst.as_mut_ptr() as *mut u8,
-                $src.len());
+            ptr::copy_nonoverlapping($src.as_ptr(), $dst.as_mut_ptr() as *mut u8, $src.len());
         }
         for v in $dst.iter_mut() {
             *v = v.$which();

Expression is single-lined and less readable (this one is marginal):

diff --git a/rand_distr/src/pert.rs b/rand_distr/src/pert.rs
index 2a00a8c27f..79d2ee0b2b 100644
--- a/rand_distr/src/pert.rs
+++ b/rand_distr/src/pert.rs
@@ -119,20 +119,12 @@ mod test {

     #[test]
     fn test_pert() {
-        for &(min, max, mode) in &[
-            (-1., 1., 0.),
-            (1., 2., 1.),
-            (5., 25., 25.),
-        ] {
+        for &(min, max, mode) in &[(-1., 1., 0.), (1., 2., 1.), (5., 25., 25.)] {
             let _distr = Pert::new(min, max, mode).unwrap();
             // TODO: test correctness
         }

Data-sequences which are not really intended to be readable get spread out in an ugly way:

diff --git a/rand_distr/src/unit_sphere.rs b/rand_distr/src/unit_sphere.rs
index 31cafd6739..d97adc8f3b 100644
--- a/rand_distr/src/unit_sphere.rs
+++ b/rand_distr/src/unit_sphere.rs
@@ -81,10 +81,14 @@ mod tests {
     fn value_stability() {
         let mut rng = crate::test::rng(2);
         let expected = [
-                [0.03247542860231647, -0.7830477442152738, 0.6211131755296027],
-                [-0.09978440840914075, 0.9706650829833128, -0.21875184231323952],
-                [0.2735582468624679, 0.9435374242279655, -0.1868234852870203],
-            ];
+            [0.03247542860231647, -0.7830477442152738, 0.6211131755296027],
+            [
+                -0.09978440840914075,
+                0.9706650829833128,
+                -0.21875184231323952,
+            ],
+            [0.2735582468624679, 0.9435374242279655, -0.1868234852870203],
+        ];
         let samples: [[f64; 3]; 3] = [
             UnitSphere.sample(&mut rng),
             UnitSphere.sample(&mut rng),

Expressions were broken for readability:

diff --git a/rand_pcg/src/pcg128.rs b/rand_pcg/src/pcg128.rs
index 63f03b47cb..d92e27f116 100644
--- a/rand_pcg/src/pcg128.rs
+++ b/rand_pcg/src/pcg128.rs
@@ -167,8 +167,7 @@ impl SeedableRng for Mcg128Xsl64 {
         // Read as if a little-endian u128 value:
         let mut seed_u64 = [0u64; 2];
         le::read_u64_into(&seed, &mut seed_u64);
-        let state = u128::from(seed_u64[0])  |
-                    u128::from(seed_u64[1]) << 64;
+        let state = u128::from(seed_u64[0]) | u128::from(seed_u64[1]) << 64;
         Mcg128Xsl64::new(state)
     }
 }
diff --git a/src/distributions/other.rs b/src/distributions/other.rs
index c95060e510..e8394a2f8f 100644
--- a/src/distributions/other.rs
+++ b/src/distributions/other.rs
@@ -218,9 +218,8 @@ mod tests {
         let mut incorrect = false;
         for _ in 0..100 {
             let c = rng.sample(Alphanumeric);
-            incorrect |= !((c >= '0' && c <= '9') ||
-                           (c >= 'A' && c <= 'Z') ||
-                           (c >= 'a' && c <= 'z') );
+            incorrect |=
+                !((c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z'));
         }
         assert!(incorrect == false);
     }

I'm using inline_attribute_width = 80 but rustfmt still doesn't like the single-line version:

diff --git a/src/distributions/utils.rs b/src/distributions/utils.rs
index 2d36b02265..bc8e4ecfd2 100644
--- a/src/distributions/utils.rs
+++ b/src/distributions/utils.rs
@@ -428,13 +428,20 @@ macro_rules! simd_impl {
     };
 }

-#[cfg(feature="simd_support")] simd_impl! { f32x2, f32, m32x2, u32x2 }
-#[cfg(feature="simd_support")] simd_impl! { f32x4, f32, m32x4, u32x4 }
-#[cfg(feature="simd_support")] simd_impl! { f32x8, f32, m32x8, u32x8 }
-#[cfg(feature="simd_support")] simd_impl! { f32x16, f32, m32x16, u32x16 }
-#[cfg(feature="simd_support")] simd_impl! { f64x2, f64, m64x2, u64x2 }
-#[cfg(feature="simd_support")] simd_impl! { f64x4, f64, m64x4, u64x4 }
-#[cfg(feature="simd_support")] simd_impl! { f64x8, f64, m64x8, u64x8 }
+#[cfg(feature = "simd_support")]
+simd_impl! { f32x2, f32, m32x2, u32x2 }
+#[cfg(feature = "simd_support")]
+simd_impl! { f32x4, f32, m32x4, u32x4 }
+#[cfg(feature = "simd_support")]
+simd_impl! { f32x8, f32, m32x8, u32x8 }
+#[cfg(feature = "simd_support")]
+simd_impl! { f32x16, f32, m32x16, u32x16 }
+#[cfg(feature = "simd_support")]
+simd_impl! { f64x2, f64, m64x2, u64x2 }
+#[cfg(feature = "simd_support")]
+simd_impl! { f64x4, f64, m64x4, u64x4 }
+#[cfg(feature = "simd_support")]
+simd_impl! { f64x8, f64, m64x8, u64x8 }

Besides, there's a whole file (rand_distr/benches/distributions.rs) where single-line macro calls are used despite being quite long.

calebcartwright commented 4 years ago

Don't forget that you can always use #![rustfmt::skip] to have rustfmt ignore a given statement/expression/block/mod/etc. if there are instances where you prefer a different/manual formatting.

calebcartwright commented 4 years ago

Also just my two cents, but I'd be a -1 on adding max_content_width as yet another width option on top of the granular max widths referenced above, use_small_heuristics, and max_width itself.

IMO the existing/planned config options for width controls will cover the vast majority of use cases, and adding another with option would cause too much complexity, both for end users and the rustfmt codebase (the current state of just max_width and use_small_heuristics is already a common source of confusion for users 😄)

dhardy commented 4 years ago

![rustfmt::skip]

I know; the above are provided more for motivation of where rustfmt could do better.

the granular max widths referenced above

These may well prove sufficient. What's the status of rustfmt 2.0: is this a near or distant target?

adding another with option

More options isn't my goal; better formatting is my goal. Whichever route takes us there. I like rustfmt: it's consistent, most results are very good, and it takes some of the effort out of producing good code. But with the Rand project there are still too many exceptions IMO (see this PR, which still leaves a dozen or so "rustfmt dirty" cases without explicit skip).

calebcartwright commented 4 years ago

What's the status of rustfmt 2.0: is this a near or distant target?

No fixed timeline, but an RC/beta of 2.x will likely be out pretty soon

More options isn't my goal; better formatting is my goal. Whichever route takes us there

Understood, I was just commenting on the issue title/propsal of adding a new max_content_width option. Once the width heuristic config items are public, you'll be able to get the formatting of many of the snippets included in this thread to your liking.

Where I suspect you will still see a gap between your desired formatting and the actual formatting even with rustfmt 2.x is on the binary expressions. There's been a few other related issues on that formatting (#3551 and #3552 for example), so definitely some opportunities for improvement there.

airstrike commented 2 months ago

Don't forget that you can always use #![rustfmt::skip] to have rustfmt ignore a given statement/expression/block/mod/etc. if there are instances where you prefer a different/manual formatting.

IMHO what would be ideal would be to configure different settings for a statement/expression/block/mod/etc. instead of outright skipping. For instance, most of my files are fine with an 80 width, but in a deeply nested method for these couple files I have, a max width of 120 would make more sense. Unless I'm missing something, my choices are to either skip the whole bit or change the max width setting globally, with no compromising alternative.

rust-lang / rustfmt

Add max_content_width option #3991

![rustfmt::skip]