nicklockwood / SwiftFormat

A command-line tool and Xcode Extension for formatting Swift code
MIT License
7.87k stars 637 forks source link

Compile error after reformatting inline closure in guard #1556

Closed benkay closed 10 months ago

benkay commented 11 months ago

We had this (questionable) code

guard let foo = {
    if condition {
        return bar()
    } else {
        return baz()
    }
}() else {
    return nil
}

0.52.8 reformats this to

guard let foo = if condition {
    return bar()
} else {
    return baz()
} else {
    return nil
}

Which fails to compile (and if it did, would have different semantics).

I suppose this should be formatted as

guard let foo = {
    if condition {
       bar()
    } else {
       baz()
    }
}() else {
    return nil
}

Although we actually hand-changed this to the more sensible

let foo = if condition { bar() } else { baz() }
guard let foo else {
    return nil
}
nicklockwood commented 11 months ago

@benkay thanks for reporting this. It seems like you've found a a good solution, but I'll get it fixed asap anyway.

nicklockwood commented 11 months ago

@calda am I right in thinking the bug here is in the redundantClosure rule? Specifically that closures containing if or switch expressions should not be removed when inside a conditional expression?

I think the simplest test case is probably something like:

guard let foo = {
  if condition {
    bar()
  }
}() else {
  return
}
calda commented 11 months ago

Yeah. I'm surprised the changes in https://github.com/nicklockwood/SwiftFormat/pull/1553 don't prevent this though. Will take a look.

nicklockwood commented 11 months ago

@calda I've just pushed a fix to develop.

nicklockwood commented 11 months ago

Yeah. I'm surprised the changes in #1553 don't prevent this though. Will take a look.

From what I can see, you were checking for an =, but not checking if that = was conditional (e.g. if let foo = ...) where it wouldn't be allowed. My fix was pretty simple, but perhaps there's a more elegant way to integrate it with the existing logic?

calda commented 11 months ago

You code change looks reasonable to me

nicklockwood commented 10 months ago

@benkay fixed in 0.52.9