Unexpected formatting of if-else inside closure

[jeanm]

This seems to be related to #1771. Not sure if it's intended behaviour. I find it much easier to understand what's going on when if and else are aligned:

  fn main() {⏎
-    something.any(|x|⏎
-        if 1_000_000_000 == 1_000_000_000 {⏎
-            true⏎
-        } else {⏎
-            false⏎
-        }⏎
-    );⏎
+    something.any(|x| if 1_000_000_000 == 1_000_000_000 {⏎
+        true⏎
+    } else {⏎
+        false⏎
+    });⏎
 }⏎

$ rustfmt --version
0.1.9-nightly (05d043c 2017-07-14)

[topecongiro]

This is an intended behaviour (see rust-lang/style-team#35 (comment)), although I agree that the former looks bettern in this case.

[bluss]

I have an issue with a formatting change in the same vein, but I think it's even worse with for. Rust is expression-oriented, but for never evaluates to a useful value, so I think it should not be contracted like this:

@@ -94,12 +86,10 @@ fn zip_default_zip3(b: &mut test::Bencher)
     let ys = black_box(ys);
     let zs = black_box(zs);
 
-    b.iter(|| {
-        for ((&x, &y), &z) in xs.iter().zip(&ys).zip(&zs) {
-            test::black_box(x);
-            test::black_box(y);
-            test::black_box(z);
-        }
+    b.iter(|| for ((&x, &y), &z) in xs.iter().zip(&ys).zip(&zs) {
+        test::black_box(x);
+        test::black_box(y);
+        test::black_box(z);
     })
 }

configuration: This happens with default configuration (haven't found something to switch it off)

[ghost]

I've also found this to be a problem.

Instead of this (rustfmt):

s.spawn(move || for i in 0..COUNT {
    tx.send(i).unwrap();
});

...the following might look better:

s.spawn(move || {
    for i in 0..COUNT {
        tx.send(i).unwrap();
    }
});

IME, the only situation where putting something in between || and { is sensible is when marking the beginning of an unsafe block:

s.spawn(move || unsafe {
    // ...
});

[joshtriplett]

This definitely shouldn't happen with for. For if, it's reasonable in some cases (if the branches don't contain full statements, just expression values), but not in others.

As an aside, I'm also wondering (outside the context of rustfmt) if we should have a lint for conditionals of the form if cond { true } else { false }, or vice versa if cond { false } else { true }, and suggest changing them to cond and !cond respectively.

[philipc]

@joshtriplett There is one in clippy: https://github.com/rust-lang-nursery/rust-clippy/wiki#needless_bool

[ghost]

Another example:

fn transition(&mut self, len: usize, deadline: Option<Instant>) {
    match *self {
        State::TryOnce { closed_count } => if closed_count < len {
            *self = State::Blocked;
        } else {
            *self = State::Disconnected;
        },
        State::SpinTry { closed_count } => if closed_count < len {
            actor::current().reset();
            *self = State::Promise { closed_count: 0 };
        } else {
            *self = State::Disconnected;
        },
    }
}

I would prefer:

fn transition(&mut self, len: usize, deadline: Option<Instant>) {
    match *self {
        State::TryOnce { closed_count } => {
            if closed_count < len {
                *self = State::Blocked;
            } else {
                *self = State::Disconnected;
            }
        }
        State::SpinTry { closed_count } => {
            if closed_count < len {
                actor::current().reset();
                *self = State::Promise { closed_count: 0 };
            } else {
                *self = State::Disconnected;
            }
        }
    }
}

[ghost]

And in some cases I think this style might be even sort of dangerous.

When glancing over following example, the lack of indentation makes it easy to miss that block is in fact a loop.

epoch::pin(|scope| loop {
    let head = self.head.load(SeqCst, scope);
    let next = head.deref().next.load(SeqCst, scope);

    if next.is_null() {
        if self.tail.load(SeqCst, scope).as_raw() == head.as_raw() {
            return None;
        }
    } else {
        if self.head
            .compare_and_swap(head, next.with_tag(MULTI), SeqCst, scope)
            .is_ok()
        {
            self.recv_count.fetch_add(1, SeqCst);
            scope.defer_free(head);
            return Some(ptr::read(&next.deref().value));
        }
    }
})

Now removing the keyword loop would be a hardly visible change, yet it would completely change the flow of the program. This is very concerning to me.

[ghost]

Sorry for spamming this thread, but I've just got bitten by this formatting for real.

Here's a concrete scenario where I spent 15 minutes debugging the code and not realizing what went wrong:

crossbeam::scope(|s| for i in 0..555 {
    s.spawn(move || loop {
        if let Ok(x) = rx.select() {
            assert_ne!(i, x);
            break;
        }
        if let Ok(()) = tx.select(i) {
            break;
        }
    });
    tx.send(!0).unwrap(); // I added this line, expecting it to be run once after the loop ends.
});

It took me way too long to realize that I wanted to do the following instead:

crossbeam::scope(|s| {
    for i in 0..555 {
        s.spawn(move || loop {
            if let Ok(x) = rx.select() {
                assert_ne!(i, x);
                break;
            }
            if let Ok(()) = tx.select(i) {
                break;
            }
        });
    }
    tx.send(!0).unwrap(); // Now that's better!
});

Doh! :) I wanted to add that line to end of the lambda, but it actually ended up being in the loop.
Very very subtle!

[nrc]

I commented on this over on fmt-rfcs at rust-lang/style-team#35 (comment)

[spinda]

Is there any way to turn this off? I really can't stand it, it reminds me of braceless if statements.

[emilio]

@spinda you can probably just apply #1889 and build locally for now, I guess.

[nrc]

Fixed by #1877