Remove empty lines in if and for similar to functions

[flan6]

// code before formating
func main() {

  for i := 0; i < 5; i++ {

    err := doSomething()

    if  err != nil {

      fmt.Println(err)

    } 
    
    doAnotherThingVoid()
    
  }
  
}

would become

// code after formating
func main() {
  for i := 0; i < 5; i++ {

    err := doSomething()
    if  err != nil {

      fmt.Println(err)

    }
    
    doAnotherThingVoid()

  }
}

my proposal is to remove same empty lines in if and for statement as well

// my proposal
func main() {
  for i := 0; i < 5; i++ {
    err := doSomething()
    if  err != nil {
      fmt.Println(err)
    }
    
    doAnotherThingVoid()
  }
}

note that the empty lines between different instructions in same level doesn't get removed. Same way as empty lines between function declarations.
this behavior is more consistent and better to read.

[mvdan]

We already have the following rules:

No empty lines around function bodies
No empty lines around a lone statement (or comment) in a block
No empty lines before a simple error check

What extra rules would you add, exactly? I personally think that anything more than this would make gofumpt too aggressive. Empty lines can be good to logically separate bits of code.

[flan6]

First, thank you for this swift response!

I'm proposing to add if and for statements to the function rule. It makes code more regular about empty lines. I do agree empty lines are good to separate bits of code. My point is exactly about lines after opening curly bracers { and before closing ones }.

No empty lines around function, for and if bodies

This addition would produce the following effects (comments where to remove empty lines):

func main() {
  somethingSomething()

  for i := 0; i < 5; i++ {
// REMOVE: No empty lines around *for* bodies
    err := doSomething()
    if  err != nil {
// REMOVE: No empty lines around *if* bodies. 
      go reportError()
      
      fmt.Println(err)
// REMOVE: No empty lines around *if* bodies. 
    }

    go codeKeepsGoingOn()

    doAnotherThingVoid()
// REMOVE: No empty lines around *for* bodies
  }
  
  somethingSomething()
}

this behavior would produce the following final code:

// my proposal
func main() {
  somethingSomething()

  for i := 0; i < 5; i++ {
    err := doSomething()
    if  err != nil {
      go reportError()
    
      fmt.Println(err)
    }

    go codeKeepsGoingOn()

    doAnotherThingVoid()
  }
  
  somethingSomething()
}

[flan6]

Currently gofmt already removes empty lines around struct and interface declarations. So I guess we can read it as:

No empty lines around struct declarations
No empty lines around interface declarations

type A interface {
// This line is removed
  Something() bool
  
  Sometime() time.Time
// This line is removed
}

type b struct {
// This line is removed
  id int

  name string
// This line is removed
}

And gofumpt currently removes around functions. So following this pattern we would assume any empty line after an opening curly bracer { and before a closing } would be removed. But right now if statements and for statements are left untouched.

[LuigiAzevedo]

I support the proposed change to remove blank spaces after open brackets and before closing brackets. In my opinion, this change would not degrade the code readability and would make it more concise with fewer inconsistencies. I appreciate the effort put into improving the code formatter behavior and believe that this change would be a positive step forward.

[seanhagen]

If you're looking for another reason to support this change, the whitespace linter complains about these kinds of newlines; having gofumpt remove them would help auto-fix those complaints.

[davidmdm]

I also support this proposal.

I don't mind defining it as two new rules:

• No empty lines around for bodies
• No empty lines around if bodies

However, I think if we want to be succinct, the rule that we actually want is no leading or trailing empty lines for code scopes

This would apply to scopes created by if, for, func, and just to inline scopes in general.

func Something() {


  {

    fmt.Println("hello")

  }



  {
    
    fmt.Println("world")
  
  }

}

becomes:

func Something() {
  {
     fmt.Println("hello")
  }

  {
    fmt.Println("world")
  }
}

[flan6]

I totally agree @davidmdm

[cristaloleg]

Isn't this a regression? or I'm just wrong?

[davidmdm]

@cristaloleg please expand?

It can be viewed as two new rules or an expansion on an existing rule.

It would not perform invalid formatting as per gofmt.

[cristaloleg]

please expand?

I thought that gofumpt was removing those lines already.

    if  err != nil {

        fmt.Println(err)

    } 

was formatted to

    if  err != nil {
        fmt.Println(err)
    } 

I might be wrong or something else changed in my local setup.

[flan6]

@cristaloleg I used a very simple example just to show what I intended. I tested if and yes, if you have only one instruction inside the if statement it indeed removes. But if you have a more complex block of code it does not.

flan on ArchLinux in ~/tmp
$ nvim main.go
flan on ArchLinux in ~/tmp
$ cat main.go
package main

import "fmt"

func main() {
        print := true

        if print {

                fmt.Println("remove lines")

                fmt.Println("please format my code")

                fmt.Println("some instructions")

        }

        {

                someSlice := []int{1, 2, 3}

                for i := range someSlice {
                        fmt.Println("this slice is fun times", i)
                }

        }
}
flan on ArchLinux in ~/tmp
$ gofumpt --version
v0.5.0 (go1.21.0)
flan on ArchLinux in ~/tmp
$ gofumpt -w main.go
flan on ArchLinux in ~/tmp
$ cat main.go
package main

import "fmt"

func main() {
        print := true

        if print {

                fmt.Println("remove lines")

                fmt.Println("please format my code")

                fmt.Println("some instructions")

        }

        {

                someSlice := []int{1, 2, 3}

                for i := range someSlice {
                        fmt.Println("this slice is fun times", i)
                }

        }
}

[davidmdm]

@mvdan any input? I created a PR that seems to accomplish the goal.

[mvdan]

I took a brief look at this. In some cases I agree this removes empty lines which aren't necessary, but in other cases it removes empty lines which I find useful to logically separate different chunks of code. For example, in a project of mine:

diff --git a/syntax/printer.go b/syntax/printer.go
index 7c3ff9e6..ba1e6c29 100644
--- a/syntax/printer.go
+++ b/syntax/printer.go
@@ -62,7 +62,6 @@ func KeepPadding(enabled bool) PrinterOption {
 			p.keepPadding = true
 			p.cols.Writer = p.bufWriter.(*bufio.Writer)
 			p.bufWriter = &p.cols
-
 		} else if !enabled && p.keepPadding {
 			// Ensure we reset the state to that of NewPrinter.
 			p.keepPadding = false
@@ -1496,7 +1495,6 @@ func (e *extraIndenter) WriteByte(b byte) error {
 		e.firstIndent = lineIndent
 		e.firstChange = e.baseIndent - lineIndent
 		lineIndent = e.baseIndent
-
 	} else if lineIndent < e.firstIndent {
 		// This line did not have enough indentation; simply indent it
 		// like the first line.
diff --git a/syntax/typedjson/json_test.go b/syntax/typedjson/json_test.go
index 5292fa7c..53b1c773 100644
--- a/syntax/typedjson/json_test.go
+++ b/syntax/typedjson/json_test.go
@@ -26,7 +26,6 @@ func TestRoundtrip(t *testing.T) {
 	shellPaths, err := filepath.Glob(filepath.Join(dir, "*.sh"))
 	qt.Assert(t, err, qt.IsNil)
 	for _, shellPath := range shellPaths {
-
 		shellPath := shellPath // do not reuse the range var
 		name := strings.TrimSuffix(filepath.Base(shellPath), ".sh")
 		jsonPath := filepath.Join(dir, name+".json")

The first two don't seem right to me - I want to split the "else if", much like I might do with different cases in a switch. You might prefer to not use that empty line yourself, but I don't think that falls under "an empty line there is definitely pointless and gofumpt should remove it".

I'd be fine with strenghtening this rule as long as it leaves empty lines at the end of blocks followed by an else-if. All other removed lines I can see across my projects seem fine to me. Happy to review a PR with the above tweak as well as tests for all the various edge cases; note that #286 has no tests and so it isn't enough on its own. Look at testdata/script/block-single.txtar, for example.

The README would also need to be tweaked, but I can do that after the change is merged.