Skip to content

Incorrect indent of closure inside struct #1332

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
nrc opened this issue Feb 23, 2017 · 8 comments
Closed

Incorrect indent of closure inside struct #1332

nrc opened this issue Feb 23, 2017 · 8 comments

Comments

@nrc
Copy link
Member

nrc commented Feb 23, 2017

See https://pbs.twimg.com/media/C5UtJRrXUAAQBIp.jpg:large

@jonhoo
Copy link
Contributor

jonhoo commented Feb 23, 2017

Much smaller example:

struct Foo;
impl Foo {
    fn foo(&self) -> Box<FnMut(i64) -> i64> {
        Box::new(move |aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa| {
            aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
        })
    }
}
@@ -2,7 +2,7 @@
 impl Foo {
     fn foo(&self) -> Box<FnMut(i64) -> i64> {
         Box::new(move |aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa| {
-            aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
-        })
+                     aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+                 })
     }
 }

@jonhoo
Copy link
Contributor

jonhoo commented Mar 21, 2017

This seems to be happening to me all over the place in my code. It looks like it's now standard to align the body of a closure at one indent deeper than the first | of the closure prologue, rather than one more than the current alignment. This causes a lot of code to end up being very deeply aligned (as above), and looks quite strange. Here is another diff to illustrate:

         let getters: Vec<_> = getters.into_iter()
             .enumerate()
             .map(|(i, g)| {
-                thread::Builder::new()
-                    .name(format!("GET{}", i))
-                    .spawn(move || exercise::launch_reader(g, config))
-                    .unwrap()
-            })
+                     thread::Builder::new()
+                         .name(format!("GET{}", i))
+                         .spawn(move || exercise::launch_reader(g, config))
+                         .unwrap()
+                 })
             .collect();

However, it doesn't seem to happen everywhere:

         Some(self.domain_dependencies
-            .iter()
-            .map(|(d, v)| {
-                let earliest: i64 = v.iter()
-                    .filter_map(|b| self.toplevel.get(b))
-                    .chain(self.last_migration.iter())
-                    .max()
-                    .cloned()
-                    .unwrap_or(0);
-                (*d, earliest)
-            })
-            .collect())
+                 .iter()
+                 .map(|(d, v)| {
+            let earliest: i64 = v.iter()
+                .filter_map(|b| self.toplevel.get(b))
+                .chain(self.last_migration.iter())
+                .max()
+                .cloned()
+                .unwrap_or(0);
+            (*d, earliest)
+        })
+                 .collect())

(yes, I know some of this code isn't pretty to begin with, but that's an orthogonal concern)

jonhoo added a commit to mit-pdos/noria that referenced this issue Mar 21, 2017
Two rustfmt things to be aware of:

 - rust-lang-nursery#1397 causes domain/mod.rs and domain/single.rs to
   fail to compile after a re-format.
 - a number of cases look somewhat ugly follow this, because closure
   bodies are indented too deeply. being tracked at
   rust-lang/rustfmt#1332
@nrc
Copy link
Member Author

nrc commented Mar 22, 2017

It looks like it's now standard to align the body of a closure at one indent deeper than the first | of the closure prologue

This is correct (and intentional). It's a more strict interpretation of visual indent rules. We do have a heuristic (7 lines or something, it is configurable) where we break a closure into block indent rather than visual. When we move to block indenting fn args, then this will always happen.

@nrc
Copy link
Member Author

nrc commented Mar 22, 2017

Closing because I think this is actually intended behaviour.

@nrc nrc closed this as completed Mar 22, 2017
@jonhoo
Copy link
Contributor

jonhoo commented Mar 22, 2017

@nrc Even the second example in the last comment? That doesn't seem to behave that way?

This visual style is significantly more jarring to me than the old one, and causes some pretty serious indentation anywhere you do, say, Box::new(move || {. Is there a way to override this behavior to instead always switch to a block indent?

@nrc
Copy link
Member Author

nrc commented Mar 23, 2017

Even the second example in the last comment?

That example seems to be hitting the threshold where we block indent.

Is there a way to override this behavior to instead always switch to a block indent?

I'm working on it! I expect to land this in the next two weeks.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 26, 2017

Where is the recommendation for using visual indent so aggressively? I don't see it in the fmt-rfcs example, nor in the guide?

It also keeps producing really weird results for me, especially when pushing against the line length limit, as @nical pointed out in rust-lang/style-team#8 (comment). For example:

                        m.map_data(|mut rs| {
                            rs.retain(|r| {
                                r.iter().enumerate().all(|(i, v)| {
                                    if let Some(ref rv) = on[i] {
                                        rv == v
                                    } else {
                                        true
                                    }
                                })
                            });
                            rs
                        });

becomes

                        m.map_data(|mut rs| {
                            rs.retain(|r| {
                                          r.iter().enumerate().all(|(i, v)| if let Some(ref rv) =
                                    on[i] {
                                                                       rv == v
                                                                   } else {
                                                                       true
                                                                   })
                                      });
                            rs
                        });

@nrc
Copy link
Member Author

nrc commented Mar 26, 2017

Where is the recommendation for using visual indent so aggressively?

It's a consequence of some internal reorganisation. It's actually in the service of moving to block indent in more places, but is temporarily having the opposite effect.

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

No branches or pull requests

2 participants