Feature Proposal: Add newline-after-block checks to whitespace linter

[breml]

Hi!

I'd like to propose extending the whitespace linter with complementary functionality that enforces blank lines after block statements as a new "opt-in" feature. This would pair naturally with whitespace's existing checks that remove unnecessary whitespace inside blocks, creating a comprehensive block-level whitespace management tool.

An implementation (based on go/analysis, including tests) of the proposed linter functionality can be found in newline-after-block.

Background

I am a contributor to multiple projects in the Incus ecosystem like incus, operations-center and migration-manager. All of these projects have the use of a simple linting shell script in common, which is called newline-after-block.sh. This script ensures a newline after each functional block (after if, for, switch/select, etc.). This is done by some sed and grep magic, which has some critical limitations:

• It does not distinguish between block statements and composite literals, it would incorrectly flag struct/map/slice literals because the detection logic relies on the ending curly brace }.
• It does not handle nested blocks correctly due to the way empty lines are detected after a closing curly brace, it would fail to detect a missing empty line after two closing curly braces (e.g. nested if)
• it does not provide automatic fix capability, all corrections need to be done manually.

I created a simple newline-after-block linter in Go using the go/analysis framework, which does the job and resolves the above mentioned shortcomings of the shell script.

Why include in whitespace

Since our projects are also linted using golangci-lint, we would like to get this functionality included, which leaves us with two options:

1. Ask for inclusion of newline-after-block into golangci-lint directly.
2. Extend an existing linter with the required functionality.

In regards to option 1, I have already gone this route in the past (bidichk, errchkjson) and from this experience I know, that the maintainers of golangci-lint prefer to reduce duplication by multiple linters. Therefore I checked the existing linters already included in golangci-lint for similar functionality, which left me with two candidates:

1. whitespace (this linter)
2. wsl

After a quick analysis of both, I figured, that whitespace is the more natural fit for the following reasons:

• Both linters manage whitespaces
• Both linters share the same technical foundation, in particular go/analysis framework and analysistest
• Both implement SuggestedFixes with TextEdit for automatic corrections
• Both use analysistest for testing
• Both operate at the package level

In contrast, wsl does not build on the go/analysis framework and uses a lot of custom logic instead. Inclusion into wsl would require a lot more work and some goals of newline-after-block might even be conflicting with the rules currently present in wsl.

The proposed checks are the natural counterpart to whitespace's existing functionality without overlap or conflict.

Proposed Checks

Newline After Block Statements

Enforce blank lines after these block statements (when not at the end of a statement list):

• if statements (only when not followed by else)
• for loops
• range loops
• switch statements
• type switch statements
• select statements

Newline Between Case Clauses

Enforce blank lines between non-empty case blocks within switch, type switch, and select statements (except before the closing brace).

Examples of What Would Be Detected

Example 1: Missing newline after if block

// Would be flagged
func process(condition bool) {
    if condition {
        doSomething()
    } // want "missing newline after block statement"
    nextStatement()
}

// After auto-fix
func process(condition bool) {
    if condition {
        doSomething()
    }

    nextStatement()
}

Example 2: Missing newline after for loop

// Would be flagged
func iterate() {
    for i := 0; i < 10; i++ {
        process(i)
    } // want "missing newline after block statement"
    fmt.Println("done")
}

// After auto-fix
func iterate() {
    for i := 0; i < 10; i++ {
        process(i)
    }

    fmt.Println("done")
}

Example 3: Missing newline after switch statement

// Would be flagged
func handleValue(v int) {
    switch v {
    case 1:
        handleOne()

    case 2:
        handleTwo()
    } // want "missing newline after block statement"
    cleanup()
}

// After auto-fix
func handleValue(v int) {
    switch v {
    case 1:
        handleOne()

    case 2:
        handleTwo()
    }

    cleanup()
}

Example 4: Missing newlines between case clauses

// Would be flagged
func handleSwitch(v int) {
    switch v {
    case 1:
        handleOne() // want "missing newline after case block"
    case 2:
        handleTwo() // want "missing newline after case block"
    default:
        handleDefault()
    }
}

// After auto-fix
func handleSwitch(v int) {
    switch v {
    case 1:
        handleOne()

    case 2:
        handleTwo()

    default:
        handleDefault()
    }
}

Example 5: Correctly ignored cases

// Block at end of function
func endOfFunc() {
    if condition {
        doSomething()
    }
}

// if-else chain
func withElse() {
    if condition {
        doSomething()
    } else {
        doSomethingElse()
    }
}

// Composite literal (not a block statement)
func structInit() {
    p := Person{
        Name: "Alice",
        Age:  30,
    }
    fmt.Println(p)
}

// Empty case (no newline needed)
func emptyCase(v int) {
    switch v {
    case 1:
    case 2:
        handleTwo()
    }
}

For more examples, check out the test cases.

Suggested Configuration Options

Following whitespace's pattern with -multi-if and -multi-func, I suggest adding:

• -newline-after-block, Enforce newline after blocks (if, for, switch/select) (default: false for backwards compatibility)
• -newline-between-cases, Enforce newlines between case clauses in switch/select (default: false for backwards compatibility)

Potential Future Extensions

• Rules about newlines around defer (I prefer to have a newline after a defer statement. In most cases, I don't want to have a newline before defer, but this is not a strict rule. But it should be allowed to omit the newline after a block if defer is used right after the block (e.g. in the case, open file, check error, defer close, where these 3 parts belong together).

Final thoughs

I hope, this proposal finds your interest and we can work together on the inclusion of this functionality in the whitespace linter.

CC: @stgraber, @ldez

Edits:

• wsl does also use go/analysis

[ldez]

Have you tried wsl?

• https://github.com/bombsimon/wsl
• https://golangci-lint.run/docs/linters/configuration/#wsl_v5

[ldez]

In contrast, wsl does not build on the go/analysis framework and uses a lot of custom logic instead.

This is false, wsl, like all the linters (except one) inside golangci-lint, is using go/analysis

[breml]

In contrast, wsl does not build on the go/analysis framework and uses a lot of custom logic instead.

This is false, wsl like all the linters (except one) inside golangci-lint are using go/analysis

My bad, I guess I checked on an old version of the code.

[ldez]

cases 1 and 2 are already handled by wsl

code

package sandbox

import "fmt"

func a() {}
func b() {}
func c() {}

// Example 1

// Would be flagged
func process(condition int) {
	if condition == 1 {
		a()
	} // want "missing newline after block statement"
	b()
}

// Example 2

// Would be flagged
func iterate() {
	for i := 0; i < 10; i++ {
		process(i)
	} // want "missing newline after block statement"
	fmt.Println("done")
}

// Example 3

// Would be flagged
func handleSwitch(v int) {
	switch v {
	case 1:
		a() // want "missing newline after case block"
	case 2:
		b() // want "missing newline after case block"
	default:
		c()
	}
}

// ---

.golangci.yml

version: "2"

linters:
  default: none
  enable:
    - wsl_v5
  settings:
    wsl_v5:
      default: all

output

$ golangci-lint run
main.go:16:2: missing whitespace above this line (invalid statement above expr) (wsl_v5)
        b()
        ^
main.go:26:2: missing whitespace above this line (invalid statement above expr) (wsl_v5)
        fmt.Println("done")
        ^
2 issues:
* wsl_v5: 

[ldez]

For me, your issue is off-topic for whitespace: wsl is the right linter to handle this topic.

[robinknaapen]

First of all, thanks for your issue, it's very well formed.

I glanced over your arguments and proposals real quick, but I will have to dive deeper next week. This also includes trying to get opinions of co-workers.

Please note that we're currently quite busy in office, and I might not be able to get this done in the coming week. I'll thank you for your patience in advance 🙏

[breml]

@robinknaapen Thanks for considering and taking the time to look into this. I will have an other look at wsl if I can come up with a config, which just enforces the rules we care about in our code base. So far I failed and the error messages from wsl have not helped me to figure out, which options are needed to be tweaked to make the false positives go away.

[breml]

@ldez I did some more testing with wsl and there are quite some cases, which are handled differently by wsl as we would expect them to be handled, in particular the following examples:

Case clause (from https://github.com/breml/newline-after-block/blob/master/testdata/src/caseclauses/caseclauses.go):

func nestedSwitchWithoutNewline() {
	x := 1
	y := 2 // <-- "false positive" by wsl: "missing whitespace above this line (too many statements above switch) (wsl_v5)", we don't want a newline above this line
	switch x {
	case 1:
		switch y {
		case 2:
			fmt.Println("x=1, y=2")
		} // want "missing newline after block statement" <-- catched by wsl
		fmt.Println("x=1") // want "missing newline after case block" <-- missed by wsl
	case 2:
		fmt.Println("x=2")
	} // want "missing newline after block statement" <-- catched by wsl
	fmt.Println("after outer switch")
}

func switchWithoutNewlineBetweenCases() {
	x := 2
	switch x {
	case 1:
		fmt.Println("one") // want "missing newline after case block" <-- missed by wsl
	case 2:
		fmt.Println("two") // want "missing newline after case block" <-- missed by wsl
	default:
		fmt.Println("other") // No blank line needed before }
	}
}

func switchMultipleStatementsInCase() {
	x := 1
	switch x {
	case 1:
		fmt.Println("one")
		fmt.Println("still one") // want "missing newline after case block" <-- missed by wsl
	case 2:
		fmt.Println("two")
	}
}

func switchFallthrough() {
	x := 1
	switch x {
	case 1:
		fmt.Println("one")
		fallthrough // want "missing newline after case block" <-- missed by wsl
	case 2:
		fmt.Println("two")
	}
}

func selectWithoutNewlineBetweenCases() {
	ch1 := make(chan int)
	ch2 := make(chan int) // "false positive" by wsl: "missing whitespace above this line (too many statements above select) (wsl_v5)", we don't want a newline above this line, same case as above.
	select {
	case v := <-ch1:
		fmt.Println(v) // want "missing newline after case block" <-- missed by wsl
	case ch2 <- 42:
		fmt.Println("sent") // want "missing newline after case block" <-- missed by wsl
	default:
		fmt.Println("default")
	}
}

func typeSwitchWithoutNewlineBetweenCases() {
	var x interface{} = "hello"
	switch v := x.(type) {
	case string:
		fmt.Println("string:", v) // want "missing newline after case block" <-- missed by wsl
	case int:
		fmt.Println("int:", v) // want "missing newline after case block" <-- missed by wsl
	default:
		fmt.Println("unknown") // No blank line needed before }
	}
}

func switchWithCommentNoNewline() {
	x := 1
	switch x {
	case 1:
		fmt.Println("one") // want "missing newline after case block" <-- missed by wsl
	// This comment needs a blank line above
	case 2:
		fmt.Println("two") // No blank line needed before }
	}
}

Comments (basically wsl missed all cases in https://github.com/breml/newline-after-block/blob/master/testdata/src/comments/comments.go except for one, where the comment is after the statement):

func ifStatementWithCommentNoNewline() {
	x := 5
	if x > 0 {
		fmt.Println("positive")
	} // want "missing newline after block statement" <-- missed by wsl (also similar other cases with else, for, range, select)
	// This comment should have a blank line above
	fmt.Println("next statement")
}

func multiLineCommentNoNewline() {
	x := 5
	if x > 0 {
		fmt.Println("positive")
	} // want "missing newline after block statement" <-- missed by wsl
	/*
		This is a multi-line comment
		that should have a blank line above
	*/
	fmt.Println("next statement")
}

Struct literals (https://github.com/breml/newline-after-block/blob/master/testdata/src/structliterals/structliterals.go):

func inlineTypeDefinition() {
	type Company struct {
		Name    string
		Address Address
	} // "false positive" by wsl: "missing whitespace above this line (no shared variables above expr) (wsl_v5)", this is not a block and therefore we don't require a whitespace here
	fmt.Println(Company{})
}

I tested with the golangci-lint config you have provided:

version: "2"

linters:
  default: none
  enable:
    - wsl_v5
  settings:
    wsl_v5:
      default: all

issues:
  max-issues-per-linter: 0
  max-same-issues: 0

[ldez]

I think this is not the place to discuss WSL.
Open an issue on WSL, and discuss it with Simon.

Note, please don't call "false positives" the rules that don't follow your style.

[bombsimon]

Feel free to open an issue in wsl as well to discuss further, however I will try to quickly address some questions here directly.

For the case blocks, there's a setting called case-max-lines which is off by default (set to 0). If this is set to 1, wsl will require an empty line at the end of each case block. (The setting is max allowed lines before a trailing whitespace is required).

To me this sounds a lot like the goal with wsl but with the main difference that wsl looks at it from the statement after the block and not the block itself. In an example like this:

if true {
    _ = 1
}
foo := 1

The check that wsl needs enabled is the check for assign since the assignment wans an empty line above it. This makes it a bit tricky because to ensure no statement after the block is allowed we would basically have to enable all the checks which might not be desirable.

Maybe it could be as simple as adding a new check with the v5 check system that simply checks whitespace after a block 🤔 Most checks treat comments as empty lines in wsl but they are handled differently in e.g. start of blocks so it could make sense to handle that in a new check requiring empty lines after blocks as well.

EDIT: This does not mean that I've decided this is a good fit for wsl, but just to get a feel for it I did bombsimon/wsl@9a0e7d3. As seen it does not require a lot of code and it just adds a new check for newline-after-block. It does not treat comments as an empty line. For the test case I also set case-max-lines to 1 to demonstrate that one and also enabled another "conflicting" check to show how the multi-report would look.

[breml]

Thanks @bombsimon for joining the conversation and for the insights about how wsl works, especially the part about "wsl looks at it from the statement after the block". Also thanks for the implementation, which demonstrates how this could be added to wsl. I will try to have a look at this later today or this week.

[breml]

@robinknaapen I would like to politely ask, if you had have the time to discuss this topic with your team and if you have come to a conclusion.