Repository: bombsimon/wsl
Pull request #99 (2020-10-02 17:52:07)

Avoid false positive on empty labels
Here's a solution to avoid false positives on empty labels.
Labels end position always falls into the next line, regardless if it is empty or not.
So for empty labels we simply could compensate the end position to avoid the false positive.

This solves #92

coveralls: 
[![Coverage Status](https://coveralls.io/builds/33895032/badge)](https://coveralls.io/builds/33895032)

Coverage increased (+1.5%) to 90.778% when pulling **61dfd0ddcb1d58f794a30dacf77523a945837cc5 on Oppodelldog:master** into **b5de1fb306cb8f34fe48f10f5e48cf7baa1bac8e on bombsimon:master**.

bombsimon: Thanks a bunch for this!

Interesting approach, but sadly it seems like this instead positive false. All these three tests will still pass:

```go
func main() {
    goto end
    end:

}
```

```go
func main() {
    goto end
    end:
    // Bye
}
```

```go
func main() {
    goto end
    end:

    // Bye
}
```

As seen when running the tests, the statement type isn't yet implemented and shows this warning:

```text
wsl_test.go:1851: WARNINGS: [unit-test:4: stmt type not implemented (*ast.LabeledStmt)]
```

I think the best way to approach this is to actually handle `*ast.LabeledStmt` in the big switch case in `**parseBlockStatements`.

If you think this is better than the other way around I can merge this and open a new ticket. I don't think I will merge this to `golangci-lint` though so it will probably only work for manual builds.
Oppodelldog: @bombsimon , thanks for your feedback. Surely do not merge anything that does not make sense to you.

My approach was quite technically by debugging and investigating, I did not go through the whole project.
So I might have overseen things.

So if you like, you could give me some guidance and we go step by step to solve this.


Let's start with the first case, the original one:
```
package main
func main() {
	goto end;
	end:
}
```

Without any changes to master the test finds a **reasonBlockEndsWithWS** coming from

**findLeadingAndTrailingWhitespaces**

```
	if p.nodeEnd(lastStatement) != blockEndLine-1 && !isExampleFunc(ident) {
			p.addError(blockEndPos, reasonBlockEndsWithWS)
		}
```
(This is why I put the compensation into **nodeEnd**)

Handling the LabeledStmt in **parseBlockStatements** would not help for this case
since **findLeadingAndTrailingWhitespaces** is called before **parseBlockStatements** and so the error is already added.
Also adding a case for *ast.LabeledStmt to avoid the **warnWSNodeTypeNotImplemented** would not help since the analyzed block leading to adding the error is the function body, not the label.

bombsimon: I totally understand that and the code base is quite messy. Since I don't update the source often it even took me some time to remember how this was done.

Let's use [delve](https://github.com/go-delve/delve) to step through the code! I usually create a test in the `TODO` block to make it easy to only run this. So I just add your test case there with the extra empty line which can be isolated run like this:

```sh
% go test ./... -v -run TODO -count 1
=== RUN   TestTODO
=== RUN   TestTODO/boilerplate
    wsl_test.go:1851: WARNINGS: [unit-test:4: stmt type not implemented (*ast.LabeledStmt)]
--- PASS: TestTODO (0.00s)
    --- PASS: TestTODO/boilerplate (0.00s)
PASS
ok      github.com/bombsimon/wsl/v3     0.077s
?       github.com/bombsimon/wsl/v3/cmd/wsl     [no test files]
```

We know that this should be related to find leading and trailing whitespaces so let's run this test and set a breakpoint there.

```sh
% dlv test -- -test.v -test.run TODO
Type 'help' for list of commands.
(dlv) break wsl.findLeadingAndTrailingWhitespaces
Breakpoint 1 set at 0x12d0d5b for github.com/bombsimon/wsl/v3.(*Processor).findLeadingAndTrailingWhitespaces() ./wsl.go:952
(dlv) continue
=== RUN   TestTODO
=== RUN   TestTODO/boilerplate
> github.com/bombsimon/wsl/v3.(*Processor).findLeadingAndTrailingWhitespaces() ./wsl.go:952 (hits goroutine(20):1 total:1) (PC: 0x12d0d5b)
   947:
   948: // findLeadingAndTrailingWhitespaces will find leading and trailing whitespaces
   949: // in a node. The method takes comments in consideration which will make the
   950: // parser more gentle.
   951: // nolint: gocognit
=> 952: func (p *Processor) findLeadingAndTrailingWhitespaces(ident *ast.Ident, stmt, nextStatement ast.Node) {
   953:         var (
   954:                 allowedLinesBeforeFirstStatement = 1
   955:                 commentMap                       = ast.NewCommentMap(p.fileSet, stmt, p.file.Comments)
   956:                 blockStatements                  []ast.Stmt
   957:                 blockStartLine                   int
```

Great, now we can step through the method to see what's going on. Actually, we know we wan't to check the code you changed where we call `p.nodeEnd(lastStatement)` so let's add a breakpoint there and check our locals.

```sh
(dlv) break wsl.go:1062
Breakpoint 2 set at 0x12d169d for github.com/bombsimon/wsl/v3.(*Processor).findLeadingAndTrailingWhitespaces() ./wsl.go:1062
(dlv) continue
> github.com/bombsimon/wsl/v3.(*Processor).findLeadingAndTrailingWhitespaces() ./wsl.go:1062 (hits goroutine(7):1 total:1) (PC: 0x12d169d)
  1057:                                         return
  1058:                                 }
  1059:                         }
  1060:                 }
  1061:
=>1062:                 if p.nodeEnd(lastStatement) != blockEndLine-1 && !isExampleFunc(ident) {
  1063:                         p.addError(blockEndPos, reasonBlockEndsWithWS)
  1064:                 }
  1065:
  1066:                 return
  1067:         }
(dlv) locals
allowedLinesBeforeFirstStatement = 1
commentMap = go/ast.CommentMap nil
blockStatements = []go/ast.Stmt len: 2, cap: 2, [...]
blockStartLine = 2
blockEndLine = 6
blockStartPos = 29
blockEndPos = 57
firstStatement = go/ast.Stmt(*go/ast.BranchStmt) 0xc00010f6f8
lastStatement = go/ast.Stmt(*go/ast.LabeledStmt) 0xc00010f6e8
seenCommentGroups = 0
```

Alright, we know that the block is registered ending on line 6 correctly according to `blockEndLine`, but we don't know where our last statement, the label, ends yet. We can either call it manually or just step down the code.

```sh
(dlv) step
> github.com/bombsimon/wsl/v3.(*Processor).nodeEnd() ./wsl.go:1140 (PC: 0x12d2398)
  1135:
  1136: func (p *Processor) nodeStart(node ast.Node) int {
  1137:         return p.fileSet.Position(node.Pos()).Line
  1138: }
  1139:
=>1140: func (p *Processor) nodeEnd(node ast.Node) int {
  1141:         var line = p.fileSet.Position(node.End()).Line
  1142:
  1143:         if isEmptyLabeledStmt(node) {
  1144:                 return line - 1
  1145:         }
(dlv) n 3
> github.com/bombsimon/wsl/v3.(*Processor).nodeEnd() ./wsl.go:1144 (PC: 0x12d245f)
  1139:
  1140: func (p *Processor) nodeEnd(node ast.Node) int {
  1141:         var line = p.fileSet.Position(node.End()).Line
  1142:
  1143:         if isEmptyLabeledStmt(node) {
=>1144:                 return line - 1
  1145:         }
  1146:
  1147:         return line
  1148: }
  1149:
(dlv) locals
line = 6
```

Hmm, this is interesting. We will return line 5 because it looks like the statement **ends** at line 6. This is weird because my test case shows it "ends" on line 4, this is what I have:

```go
1: package main
2: func main() {
3:     goto end
4:     end:
5: 
6: }
```

Ah, I think this is it! The label actually **ends** after all these whitespaces, oddly enough even at the same line as the block itself. What we want to look for is where it starts and ensure it's the line before where the block ends. Let's fix this!

```diff
diff --git a/wsl.go b/wsl.go
index d7b5cc8..9fa00a3 100644
--- a/wsl.go
+++ b/wsl.go
@@ -1141,7 +1141,7 @@ func (p *Processor) nodeEnd(node ast.Node) int {
        var line = p.fileSet.Position(node.End()).Line

        if isEmptyLabeledStmt(node) {
-               return line - 1
+               return p.fileSet.Position(node.Pos()).Line
        }

        return line
```

And re-run the tests!

```sh
% go test ./... -v -run TODO -count 1
=== RUN   TestTODO
=== RUN   TestTODO/boilerplate
    wsl_test.go:1851: WARNINGS: [unit-test:4: stmt type not implemented (*ast.LabeledStmt)]
unit-test:6: block should not end with a whitespace (or comment)
    wsl_test.go:1857:
                Error Trace:    wsl_test.go:1857
                Error:          "[{unit-test %!s(int=6) unit-test:6:4 block should not end with a whitespace (or comment)}]" should have 0 item(s), but has 1
                Test:           TestTODO/boilerplate
                Messages:       correct amount of errors found
--- FAIL: TestTODO (0.00s)
    --- FAIL: TestTODO/boilerplate (0.00s)
FAIL
FAIL    github.com/bombsimon/wsl/v3     0.076s
?       github.com/bombsimon/wsl/v3/cmd/wsl     [no test files]
FAIL
```

That looks better! What happens if I remove my added whitespace and run the same example you did?

```sh
% go test ./... -v -run TODO -count 1
=== RUN   TestTODO
=== RUN   TestTODO/boilerplate
    wsl_test.go:1850: WARNINGS: [unit-test:4: stmt type not implemented (*ast.LabeledStmt)]
--- PASS: TestTODO (0.00s)
    --- PASS: TestTODO/boilerplate (0.00s)
PASS
ok      github.com/bombsimon/wsl/v3     0.077s
?       github.com/bombsimon/wsl/v3/cmd/wsl     [no test files]
```

Much better! Sanity check, what happens if our block is in fact not empty? Let's try these two tests:

```go
func main() {
    goto end
end:
    fmt.Println("Hello, World")
}
```

```go
func main() {
    goto end
end:
    fmt.Println("Hello, World")

}
```

```sh
% go test ./... -v -run TODO -count 1                                                                                                                                 1 ↵
=== RUN   TestTODO
=== RUN   TestTODO/label_no_whitespace
    wsl_test.go:1861: WARNINGS: [unit-test:4: stmt type not implemented (*ast.LabeledStmt)]
=== RUN   TestTODO/label_whitespace
    wsl_test.go:1861: WARNINGS: [unit-test:4: stmt type not implemented (*ast.LabeledStmt)]
unit-test:7: block should not end with a whitespace (or comment)
    wsl_test.go:1867:
                Error Trace:    wsl_test.go:1867
                Error:          "[{unit-test %!s(int=7) unit-test:7:4 block should not end with a whitespace (or comment)}]" should have 0 item(s), but has 1
                Test:           TestTODO/label_whitespace
                Messages:       correct amount of errors found
--- FAIL: TestTODO (0.00s)
    --- PASS: TestTODO/label_no_whitespace (0.00s)
    --- FAIL: TestTODO/label_whitespace (0.00s)
FAIL
FAIL    github.com/bombsimon/wsl/v3     0.078s
?       github.com/bombsimon/wsl/v3/cmd/wsl     [no test files]
FAIL
```

Cool, as expected! This should now be fixed so we just need to add this change and probably add some more tests to prove this behaviour. Will you update your PR? :)
Oppodelldog: @bombsimon, what I originally meant in guidance was more to get information about those functions I mentioned before are connected, like general logical mechanics.
But providing a detailed debugging session and solving the issue at once is also welcome ;-)
I personally do not use delve, so I learned from your tour how simple it is, thanks for that extra lesson.

I applied the more elegant and reliable solutions as a fix here 61dfd0ddcb1d58f794a30dacf77523a945837cc5
Also added one more test to cover the expected behavior.
Just let me know if this is not enough, I did not want too many spread testcases since the possibilities are endless.


bombsimon: Ah, I see. Glad you at least learnt something from my response.

I think this is awesome, seems to cover all cases with trailing labels. Thank you!

Patch:
diff --git a/wsl.go b/wsl.go
index 6744dc2..9fa00a3 100644
--- a/wsl.go
+++ b/wsl.go
@@ -1138,7 +1138,24 @@ func (p *Processor) nodeStart(node ast.Node) int {
 }
 
 func (p *Processor) nodeEnd(node ast.Node) int {
-	return p.fileSet.Position(node.End()).Line
+	var line = p.fileSet.Position(node.End()).Line
+
+	if isEmptyLabeledStmt(node) {
+		return p.fileSet.Position(node.Pos()).Line
+	}
+
+	return line
+}
+
+func isEmptyLabeledStmt(node ast.Node) bool {
+	v, ok := node.(*ast.LabeledStmt)
+	if !ok {
+		return false
+	}
+
+	_, empty := v.Stmt.(*ast.EmptyStmt)
+
+	return empty
 }
 
 // Add an error for the file and line number for the current token.Pos with the


Test patch:
diff --git a/wsl_test.go b/wsl_test.go
index dfc9367..ffd24cc 100644
--- a/wsl_test.go
+++ b/wsl_test.go
@@ -65,6 +65,14 @@ func TestGenericHandling(t *testing.T) {
 				s := func() error { return nil }
 			}`),
 		},
+		{
+			description: "no false positives for empty labeled blocks",
+			code: []byte(`package main
+			func main() {
+				goto end;
+				end:
+			}`),
+		},
 	}
 
 	for _, tc := range cases {
@@ -200,6 +208,18 @@ func TestShouldRemoveEmptyLines(t *testing.T) {
 				}
 			}`),
 		},
+		{
+			description: "whitespaces parsed correctly in labeled blocks",
+			code: []byte(`package main
+			func main() {
+				goto end
+				end:
+
+			}`),
+			expectedErrorStrings: []string{
+				reasonBlockEndsWithWS,
+			},
+		},
 	}
 
 	for _, tc := range cases {