Skip to content

Unintuitive use_small_heuristics #2636

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

Closed
Ekleog opened this issue Apr 19, 2018 · 4 comments
Closed

Unintuitive use_small_heuristics #2636

Ekleog opened this issue Apr 19, 2018 · 4 comments
Labels

Comments

@Ekleog
Copy link

Ekleog commented Apr 19, 2018

The documentation states “Whether to use different formatting for items and expressions if they satisfy a heuristic notion of 'small'.”

So my definition of “small” here is obviously subjective.

Anyway, I'd have expected rustfmt 0.4.2-nightly (dd807e2 2018-04-18) to have put the following on a single line:

             (self.code.code() % 10) as u8 + b'0',
         ];
         w.write_all(code)?;
-        w.write_all(if self.is_last == IsLastLine::Yes { b" " } else { b"-" })?;
+        w.write_all(if self.is_last == IsLastLine::Yes {
+            b" "
+        } else {
+            b"-"
+        })?;
         w.write_all(self.line)?;
         w.write_all(b"\r\n")
     }
 
 impl<'a> fmt::Debug for EhloCommand<'a> {
     fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
-        write!(f, "EhloCommand {{ domain: {:?} }}", bytes_to_dbg(self.domain))
+        write!(
+            f,
+            "EhloCommand {{ domain: {:?} }}",
+            bytes_to_dbg(self.domain)
+        )
     }
 }
 
         } else {
-            Ok(Reply { code, is_last, line })
+            Ok(Reply {
+                code,
+                is_last,
+                line,
+            })
         }

Then maybe my definition of “small line” is not the same as the one of other people, so maybe an option for tweaking the “sensitivity” of the smallness heuristic could help here?

@nrc
Copy link
Member

nrc commented Apr 19, 2018

'small' is deliberately set to be very small because we believe that the readability benefit of using more vertical space usually outweighs any benefit of keeping things on one line. For example, for struct literals, the use case for small is something like Point { x, y }. It is possible we'll tweak these things a bit in the future, but we'll probably still err on the side of too small rather than too big.

Because the small heuristic affects quite a few variables, adding sensitivity options is complex. I do intend to do it, but not in the near future (at least not before a 1.0 release).

@nrc nrc added the p-low label Apr 19, 2018
@Ekleog
Copy link
Author

Ekleog commented Apr 19, 2018

OK thanks! A 1.0 release is more important than this, indeed :)

@nickolay
Copy link

I tried to figure out the status of this problem, and as far as I understood:

  • The Rust Style Guide says "We leave it to individual tools to decide on exactly what small means" following the discussion in Define 'short' style-team#47, where there was an attempt to define "small" in general, that was eventually given up to "[allow] implementations to incrementally improve their design".
  • rustfmt currently has a number of number-of-characters constants in WidthHeuristics, e.g. "[struct_lit_width is the] Maximum width in the body of a struct lit before falling back to vertical formatting".
    • Initially configurable independently, they are now controlled by a single use_small_heuristics option (note Bring back longer lines #2437 about the problems with that).
    • When the heuristics are on, the number-of-columns options are set based on max_width (e.g. struct_lit_width = 18% * max_width), which may be unintuitive when something you deem "very simple" is split across multiple lines.

I didn't find any plans regarding improving the heuristics though, and I think that trying to incrementally define "simple" for the specific obvious cases (such as the StructLiteral {with, shorthands, only} - the last example in this issue) has the highest chance of improving the status quo. @nrc, what do you think?

@nrc
Copy link
Member

nrc commented Mar 27, 2019

One issue is that we have a backwards compatibility guarantee and changing the heuristics would break that. So either this would have to be opt-in and/or it would need a 2.0 release. That means the Rustfmt team should absolutely be sure that it is worth working on and plan out exactly when they want to do that.

I think we probably could do better than the existing heuristics, and it would be good to do so. So It think it is worth experimenting with better heuristics. However, I also think there are quite a few other things which ought to be higher priority.

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

No branches or pull requests

4 participants