Make the gofmt CI test actaully fatal

smowton · smowton · commit d05657ddff0c · 2020-07-10T11:02:50.000+01:00
Turns out gofmt doesn't actually return 1 when it finds problems, only when it finds source files which don't compile (all of which are now excluded).

This also fixes existing overlooked inconsistencies as a result of this mistake.
diff --git a/.github/workflows/codeqltest.yml b/.github/workflows/codeqltest.yml
@@ -34,7 +34,7 @@ jobs:
       run: env PATH=$PATH:$HOME/codeql make
 
     - name: Check that all QL and Go code is autoformatted
-      run: env PATH=$PATH:$HOME/codeql make QL_AUTOFORMAT=--check-only GO_AUTOFORMAT=-d autoformat
+      run: env PATH=$PATH:$HOME/codeql make check-formatting
 
     - name: Test
       run: env PATH=$PATH:$HOME/codeql make test
diff --git a/Makefile b/Makefile
@@ -21,20 +21,21 @@ EXTRACTOR_PACK_OUT = build/codeql-extractor-go
 BINARIES = go-extractor go-tokenizer go-autobuilder go-bootstrap go-gen-dbscheme
 
 .PHONY: tools tools-codeql tools-codeql-full clean autoformat \
-	tools-linux64 tools-osx64 tools-win64
+	tools-linux64 tools-osx64 tools-win64 check-formatting
 
 clean:
 	rm -rf tools/bin tools/linux64 tools/osx64 tools/win64 tools/net tools/opencsv
 	rm -rf $(EXTRACTOR_PACK_OUT) build/stats build/testdb
 
-QL_AUTOFORMAT=-qq -i
-GO_AUTOFORMAT=-w # Update files in-place
-
 DATAFLOW_BRANCH=master
 
 autoformat:
-	find ql/src -name "*.ql" -or -name "*.qll" | xargs codeql query format $(QL_AUTOFORMAT)
-	git ls-files | grep \\.go$ | xargs grep -L "//\s*autoformat-ignore" | xargs gofmt $(GO_AUTOFORMAT)
+	find ql/src -name "*.ql" -or -name "*.qll" | xargs codeql query format -qq -i
+	git ls-files | grep \\.go$ | xargs grep -L "//\s*autoformat-ignore" | xargs gofmt -w
+
+check-formatting:
+	find ql/src -name "*.ql" -or -name "*.qll" | xargs codeql query format --check-only
+	test -z "$$(git ls-files | grep \\.go$ | xargs grep -L "//\s*autoformat-ignore" | xargs gofmt -l)"
 
 tools: $(addsuffix $(EXE),$(addprefix tools/bin/,$(BINARIES))) tools/tokenizer.jar
 
diff --git a/extractor/gomodextractor.go b/extractor/gomodextractor.go
@@ -172,11 +172,9 @@ func extractGoModComments(tw *trap.Writer, expr modfile.Expr, exprlbl trap.Label
 	extractLocation(tw, grouplbl, startLine, startCol, endLine, endCol)
 }
 
-
-
 func extractGoModComment(tw *trap.Writer, comment modfile.Comment, commentToken string, grouplbl trap.Label, idx int) {
 	lbl := tw.Labeler.LocalID(comment)
 	dbscheme.CommentsTable.Emit(tw, lbl, dbscheme.SlashSlashComment.Index(), grouplbl, idx, commentToken)
 
-	extractLocation(tw, lbl, comment.Start.Line, comment.Start.LineRune, comment.Start.Line, comment.Start.LineRune + (len(commentToken) - 1))
+	extractLocation(tw, lbl, comment.Start.Line, comment.Start.LineRune, comment.Start.Line, comment.Start.LineRune+(len(commentToken)-1))
 }
diff --git a/ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go b/ql/src/experimental/CWE-322/InsecureHostKeyCallbackExample.go
@@ -9,8 +9,8 @@ func main() {}
 
 func insecureIgnoreHostKey() {
 	_ = &ssh.ClientConfig{
-		User: "username",
-		Auth: []ssh.AuthMethod{nil},
+		User:            "username",
+		Auth:            []ssh.AuthMethod{nil},
 		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
 	}
 }
@@ -22,6 +22,6 @@ func insecureHostKeyCallback() {
 		HostKeyCallback: ssh.HostKeyCallback(
 			func(hostname string, remote net.Addr, key ssh.PublicKey) error {
 				return nil
-		}),
+			}),
 	}
-}
+}
diff --git a/ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go b/ql/src/experimental/CWE-322/SecureHostKeyCallbackExample.go
@@ -12,8 +12,8 @@ func secureHostKeyCallback() {
 	publicKey, _ := ssh.ParsePublicKey(publicKeyBytes)
 
 	_ = &ssh.ClientConfig{
-		User: "username",
-		Auth: []ssh.AuthMethod{nil},
+		User:            "username",
+		Auth:            []ssh.AuthMethod{nil},
 		HostKeyCallback: ssh.FixedHostKey(publicKey),
 	}
-}
+}
diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected b/ql/test/experimental/CWE-322/InsecureHostKeyCallback.expected
@@ -1,34 +1,34 @@
 edges
-| InsecureHostKeyCallbackExample.go:12:4:14:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion |
-| InsecureHostKeyCallbackExample.go:27:14:30:3 | type conversion : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback |
-| InsecureHostKeyCallbackExample.go:28:3:30:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:27:14:30:3 | type conversion : signature type |
-| InsecureHostKeyCallbackExample.go:41:3:43:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion |
+| InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion |
+| InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback |
+| InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type |
+| InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion |
 | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback |
 | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback |
-| InsecureHostKeyCallbackExample.go:63:22:66:3 | type conversion : signature type | InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type |
-| InsecureHostKeyCallbackExample.go:64:3:66:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:63:22:66:3 | type conversion : signature type |
+| InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type | InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type |
+| InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type |
 | InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type |
 | InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback |
 nodes
-| InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion | semmle.label | type conversion |
-| InsecureHostKeyCallbackExample.go:12:4:14:3 | function literal : signature type | semmle.label | function literal : signature type |
+| InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | semmle.label | type conversion |
+| InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | semmle.label | function literal : signature type |
 | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | semmle.label | call to InsecureIgnoreHostKey |
-| InsecureHostKeyCallbackExample.go:27:14:30:3 | type conversion : signature type | semmle.label | type conversion : signature type |
-| InsecureHostKeyCallbackExample.go:28:3:30:2 | function literal : signature type | semmle.label | function literal : signature type |
+| InsecureHostKeyCallbackExample.go:27:14:30:4 | type conversion : signature type | semmle.label | type conversion : signature type |
+| InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | semmle.label | function literal : signature type |
 | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | semmle.label | callback |
-| InsecureHostKeyCallbackExample.go:41:3:43:2 | function literal : signature type | semmle.label | function literal : signature type |
+| InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | semmle.label | function literal : signature type |
 | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | semmle.label | type conversion |
 | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : HostKeyCallback | semmle.label | definition of callback : HostKeyCallback |
 | InsecureHostKeyCallbackExample.go:52:39:52:46 | definition of callback : signature type | semmle.label | definition of callback : signature type |
 | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | semmle.label | callback |
-| InsecureHostKeyCallbackExample.go:63:22:66:3 | type conversion : signature type | semmle.label | type conversion : signature type |
-| InsecureHostKeyCallbackExample.go:64:3:66:2 | function literal : signature type | semmle.label | function literal : signature type |
+| InsecureHostKeyCallbackExample.go:63:22:66:4 | type conversion : signature type | semmle.label | type conversion : signature type |
+| InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | semmle.label | function literal : signature type |
 | InsecureHostKeyCallbackExample.go:68:35:68:50 | insecureCallback : signature type | semmle.label | insecureCallback : signature type |
 | InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | semmle.label | call to InsecureIgnoreHostKey : HostKeyCallback |
 #select
-| InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion | InsecureHostKeyCallbackExample.go:12:4:14:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:4 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:12:4:14:3 | function literal | this source |
+| InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal : signature type | InsecureHostKeyCallbackExample.go:11:20:14:5 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:12:4:14:4 | function literal | this source |
 | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:22:20:22:46 | call to InsecureIgnoreHostKey | this source |
-| InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | InsecureHostKeyCallbackExample.go:28:3:30:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:28:3:30:2 | function literal | this source |
-| InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | InsecureHostKeyCallbackExample.go:41:3:43:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:41:3:43:2 | function literal | this source |
-| InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | InsecureHostKeyCallbackExample.go:64:3:66:2 | function literal : signature type | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:64:3:66:2 | function literal | this source |
+| InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:35:20:35:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:28:3:30:3 | function literal | this source |
+| InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:48:20:48:48 | type conversion | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:41:3:43:3 | function literal | this source |
+| InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal : signature type | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:64:3:66:3 | function literal | this source |
 | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey : HostKeyCallback | InsecureHostKeyCallbackExample.go:56:20:56:27 | callback | Configuring SSH ClientConfig with insecure HostKeyCallback implementation from $@. | InsecureHostKeyCallbackExample.go:79:35:79:61 | call to InsecureIgnoreHostKey | this source |
diff --git a/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go b/ql/test/experimental/CWE-322/InsecureHostKeyCallbackExample.go
@@ -10,60 +10,60 @@ func insecureSSHClientConfig() {
 		Auth: []ssh.AuthMethod{nil},
 		HostKeyCallback: ssh.HostKeyCallback(
 			func(hostname string, remote net.Addr, key ssh.PublicKey) error {
-				 return nil
-		}),
-	 }
+				return nil
+			}),
+	}
 }
 
 func insecureSSHClientConfigAlt() {
 	_ = &ssh.ClientConfig{
-		User: "user",
-		Auth: []ssh.AuthMethod{nil},
+		User:            "user",
+		Auth:            []ssh.AuthMethod{nil},
 		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
-	 }
+	}
 }
 
 func insecureSSHClientConfigLocalFlow() {
 	callback := ssh.HostKeyCallback(
 		func(hostname string, remote net.Addr, key ssh.PublicKey) error {
-			 return nil
-	})
+			return nil
+		})
 
 	_ = &ssh.ClientConfig{
-		User: "user",
-		Auth: []ssh.AuthMethod{nil},
+		User:            "user",
+		Auth:            []ssh.AuthMethod{nil},
 		HostKeyCallback: callback,
-	 }
+	}
 }
 
 func insecureSSHClientConfigLocalFlowAlt() {
-	callback := 
+	callback :=
 		func(hostname string, remote net.Addr, key ssh.PublicKey) error {
-			 return nil
-	};
+			return nil
+		}
 
 	_ = &ssh.ClientConfig{
-		User: "user",
-		Auth: []ssh.AuthMethod{nil},
+		User:            "user",
+		Auth:            []ssh.AuthMethod{nil},
 		HostKeyCallback: ssh.HostKeyCallback(callback),
-	 }
+	}
 }
 
 func potentialInsecureSSHClientConfig(callback ssh.HostKeyCallback) {
 	_ = &ssh.ClientConfig{
-		User: "user",
-		Auth: []ssh.AuthMethod{nil},
+		User:            "user",
+		Auth:            []ssh.AuthMethod{nil},
 		HostKeyCallback: callback,
-	 }
+	}
 }
 
 func main() {
 	fmt.Printf("Hello insecure SSH client config!\n")
 
 	insecureCallback := ssh.HostKeyCallback(
 		func(hostname string, remote net.Addr, key ssh.PublicKey) error {
-			 return nil
-	})
+			return nil
+		})
 
 	potentialInsecureSSHClientConfig(insecureCallback)
 
@@ -72,9 +72,9 @@ func main() {
 			if hostname == "localhost" {
 				return nil
 			}
-			 return fmt.Errorf("ssh: Unexpected host for key")
-	})
+			return fmt.Errorf("ssh: Unexpected host for key")
+		})
 
 	potentialInsecureSSHClientConfig(potentiallySecureCallback)
 	potentialInsecureSSHClientConfig(ssh.InsecureIgnoreHostKey())
-}
+}
diff --git a/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected b/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected
@@ -1,12 +1,12 @@
 | UnreachableStatement.go:5:27:5:29 | increment statement | This statement is unreachable. |
-| main.go:11:2:11:14 | expression statement | This statement is unreachable. |
-| main.go:16:2:16:14 | expression statement | This statement is unreachable. |
-| main.go:20:22:20:34 | expression statement | This statement is unreachable. |
-| main.go:28:2:28:14 | expression statement | This statement is unreachable. |
-| main.go:47:2:47:14 | expression statement | This statement is unreachable. |
-| main.go:53:3:53:15 | expression statement | This statement is unreachable. |
-| main.go:55:2:55:14 | expression statement | This statement is unreachable. |
-| main.go:139:2:139:26 | return statement | This statement is unreachable. |
-| main.go:145:2:145:17 | return statement | This statement is unreachable. |
-| main.go:151:2:151:22 | return statement | This statement is unreachable. |
-| main.go:157:2:157:43 | return statement | This statement is unreachable. |
+| main.go:13:2:13:14 | expression statement | This statement is unreachable. |
+| main.go:18:2:18:14 | expression statement | This statement is unreachable. |
+| main.go:22:22:22:34 | expression statement | This statement is unreachable. |
+| main.go:30:2:30:14 | expression statement | This statement is unreachable. |
+| main.go:49:2:49:14 | expression statement | This statement is unreachable. |
+| main.go:55:3:55:15 | expression statement | This statement is unreachable. |
+| main.go:57:2:57:14 | expression statement | This statement is unreachable. |
+| main.go:141:2:141:26 | return statement | This statement is unreachable. |
+| main.go:147:2:147:17 | return statement | This statement is unreachable. |
+| main.go:153:2:153:22 | return statement | This statement is unreachable. |
+| main.go:159:2:159:43 | return statement | This statement is unreachable. |
diff --git a/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go b/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go
@@ -1,6 +1,8 @@
 package main
 
-import ("errors")
+import (
+	"errors"
+)
 
 func unreachable() {}
 
diff --git a/ql/test/query-tests/Security/CWE-312/klog.go b/ql/test/query-tests/Security/CWE-312/klog.go
@@ -23,8 +23,8 @@ func klogTest() {
 				klog.Info(mask(name, header)) // OK
 			}
 		}
-		klog.Info(r.Header.Get("Accept")) // OK
-		klog.Info(r.Header["Content-Type"]) // OK
+		klog.Info(r.Header.Get("Accept"))        // OK
+		klog.Info(r.Header["Content-Type"])      // OK
 		klog.Info(r.Header.Get("Authorization")) // NOT OK
 	})
 	http.ListenAndServe(":80", nil)
diff --git a/ql/test/query-tests/Security/CWE-312/overrides.go b/ql/test/query-tests/Security/CWE-312/overrides.go
@@ -2,7 +2,7 @@ package main
 
 import "fmt"
 
-type s struct {}
+type s struct{}
 
 func (_ s) String() string {
 	password := "horsebatterystaplecorrect"
@@ -12,4 +12,4 @@ func (_ s) String() string {
 func overrideTest(x s, y fmt.Stringer) {
 	fmt.Println(x.String()) // NOT OK
 	fmt.Println(y.String()) // OK
-}
+}

Original file line number	Diff line number	Diff line change
`@@ -172,11 +172,9 @@ func extractGoModComments(tw *trap.Writer, expr modfile.Expr, exprlbl trap.Label`
`172`	`172`	`extractLocation(tw, grouplbl, startLine, startCol, endLine, endCol)`
`173`	`173`	`}`
`174`	`174`
`175`		`-`
`176`		`-`
`177`	`175`	`func extractGoModComment(tw *trap.Writer, comment modfile.Comment, commentToken string, grouplbl trap.Label, idx int) {`
`178`	`176`	`lbl := tw.Labeler.LocalID(comment)`
`179`	`177`	`dbscheme.CommentsTable.Emit(tw, lbl, dbscheme.SlashSlashComment.Index(), grouplbl, idx, commentToken)`
`180`	`178`
`181`		`- extractLocation(tw, lbl, comment.Start.Line, comment.Start.LineRune, comment.Start.Line, comment.Start.LineRune + (len(commentToken) - 1))`
	`179`	`+ extractLocation(tw, lbl, comment.Start.Line, comment.Start.LineRune, comment.Start.Line, comment.Start.LineRune+(len(commentToken)-1))`
`182`	`180`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,8 +9,8 @@ func main() {}`
`9`	`9`
`10`	`10`	`func insecureIgnoreHostKey() {`
`11`	`11`	`_ = &ssh.ClientConfig{`
`12`		`- User: "username",`
`13`		`- Auth: []ssh.AuthMethod{nil},`
	`12`	`+ User: "username",`
	`13`	`+ Auth: []ssh.AuthMethod{nil},`
`14`	`14`	`HostKeyCallback: ssh.InsecureIgnoreHostKey(),`
`15`	`15`	`}`
`16`	`16`	`}`
`@@ -22,6 +22,6 @@ func insecureHostKeyCallback() {`
`22`	`22`	`HostKeyCallback: ssh.HostKeyCallback(`
`23`	`23`	`func(hostname string, remote net.Addr, key ssh.PublicKey) error {`
`24`	`24`	`return nil`
`25`		`- }),`
	`25`	`+ }),`
`26`	`26`	`}`
`27`		`-}`
	`27`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -23,8 +23,8 @@ func klogTest() {`
`23`	`23`	`klog.Info(mask(name, header)) // OK`
`24`	`24`	`}`
`25`	`25`	`}`
`26`		`- klog.Info(r.Header.Get("Accept")) // OK`
`27`		`- klog.Info(r.Header["Content-Type"]) // OK`
	`26`	`+ klog.Info(r.Header.Get("Accept")) // OK`
	`27`	`+ klog.Info(r.Header["Content-Type"]) // OK`
`28`	`28`	`klog.Info(r.Header.Get("Authorization")) // NOT OK`
`29`	`29`	`})`
`30`	`30`	`http.ListenAndServe(":80", nil)`