work around another go/printer bug to fix andybalholm/brotli

When obfuscating the following piece of code:

	func issue_573(s struct{ f int }) {
		var _ *int = &s.f
		/*x*/
	}

the function body would roughly end up printed as:
we would roughly end up with:

	var _ *int = &dZ4xYx3N
	/*x*/.rbg1IM3V

Note that the /*x*/ comment got moved earlier in the source code.
This happens because the new identifiers are longer, so the printer
thinks that the selector now ends past the comment.

That would be fine - we don't really mind where comments end up,
because these non-directive comments end up being removed anyway.

However, the resulting syntax is wrong, as the period for the selector
must be on the first line rather than the second.
This is a go/printer bug that we should fix upstream,
but until then, we must work around it in Go 1.18.x and 1.19.x.

The fix is somewhat obvious in hindsight. To reduce the chances that
go/printer will trip over comments and produce invalid syntax,
get rid of most comments before we use the printer.
We still keep the removal of comments after printing,
since go/printer consumes some comments in ast.Node Doc fields.

Add the minimized unit test case above, and add the upstream project
that found this bug to check-third-party.
andybalholm/brotli helps cover a compression algorithm and ccgo code
generation from C to Go, and it's also a fairly popular module,
particular with HTTP implementations which want pure-Go brotli.

While here, fix the check-third-party script: it was setting GOFLAGS
a bit too late, so it may run `go get` on the wrong mod file.

Fixes #573.

.github/workflows/test.yml

@@ -40,7 +40,8 @@ jobs:
 
     # Static checks from this point forward. Only run on one Go version and on
     # Linux, since it's the fastest platform, and the tools behave the same.
-    - if: matrix.os == 'ubuntu-latest' && matrix.go-version == '1.18.x'
+    - name: Test third-party project builds
+      if: matrix.os == 'ubuntu-latest' && matrix.go-version == '1.18.x'
       run: |
         go install
         ./scripts/check-third-party.sh

position.go

@@ -17,12 +17,29 @@ import (
 var printBuf1, printBuf2 bytes.Buffer
 
 // printFile prints a Go file to a buffer, while also removing non-directive
-// comments and adding extra compiler directives to obfuscate position
-// information.
+// comments and adding extra compiler directives to obfuscate position information.
 func printFile(file *ast.File) ([]byte, error) {
-	printConfig := printer.Config{Mode: printer.RawFormat}
+	if curPkg.ToObfuscate {
+		// Omit comments from the final Go code.
+		// Keep directives, as they affect the build.
+		// We do this before printing to print fewer bytes below.
+		var newComments []*ast.CommentGroup
+		for _, group := range file.Comments {
+			var newGroup ast.CommentGroup
+			for _, comment := range group.List {
+				if strings.HasPrefix(comment.Text, "//go:") {
+					newGroup.List = append(newGroup.List, comment)
+				}
+			}
+			if len(newGroup.List) > 0 {
+				newComments = append(newComments, &newGroup)
+			}
+		}
+		file.Comments = newComments
+	}
 
 	printBuf1.Reset()
+	printConfig := printer.Config{Mode: printer.RawFormat}
 	if err := printConfig.Fprint(&printBuf1, fset, file); err != nil {
 		return nil, err
 	}
@@ -71,7 +88,6 @@ func printFile(file *ast.File) ([]byte, error) {
 	// Make sure the entire file gets a zero filename by default,
 	// in case we miss any positions below.
 	// We use a //-style comment, because there might be build tags.
-	// toAdd is for /*-style comments, so add it to printBuf2 directly.
 	fmt.Fprintf(&printBuf2, "//line %s:1\n", newPrefix)
 
 	// We use an empty filename when tokenizing below.
@@ -92,10 +108,10 @@ func printFile(file *ast.File) ([]byte, error) {
 			printBuf2.Write(src[copied:])
 			return printBuf2.Bytes(), nil
 		case token.COMMENT:
-			// Omit comments from the final Go code.
-			// Keep directives, as they affect the build.
-			// This is superior to removing the comments before printing,
-			// because then the final source would have different line numbers.
+			// Omit comments from the final Go code, again.
+			// Before we removed the comments from file.Comments,
+			// but go/printer also grabs comments from some Doc ast.Node fields.
+			// TODO: is there an easy way to filter all comments at once?
 			if strings.HasPrefix(lit, "//go:") {
 				continue // directives are kept
 			}

scripts/cached_modfiles/github.com_andybalholm_brotli_v1.0.4.mod

@@ -0,0 +1,5 @@
+module test
+
+go 1.19
+
+require github.com/andybalholm/brotli v1.0.4 // indirect

scripts/cached_modfiles/github.com_andybalholm_brotli_v1.0.4.sum

@@ -0,0 +1,2 @@
+github.com/andybalholm/brotli v1.0.4 h1:V7DdXeJtZscaqfNuAdSRuRFzuiKlHSC/Zh3zl9qY3JY=
+github.com/andybalholm/brotli v1.0.4/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig=

scripts/cached_modfiles/github.com_samber_lo_v1.21.0.mod

@@ -1,6 +1,6 @@
 module test
 
-go 1.18
+go 1.19
 
 require (
 	github.com/samber/lo v1.21.0 // indirect

scripts/cached_modfiles/golang.zx2c4.com_wireguard_0.0.20220316.mod

@@ -1,6 +1,6 @@
 module test
 
-go 1.18
+go 1.19
 
 require (
 	golang.org/x/crypto v0.0.0-20220315160706-3147a52a75dd // indirect

scripts/check-third-party.sh

@@ -19,14 +19,21 @@ modules=(
 	# google.golang.org/protobuf v1.28.0
 
 	# Wireguard helps cover networking and cryptography.
-	golang.zx2c4.com/wireguard 0.0.20220316 
+	golang.zx2c4.com/wireguard 0.0.20220316
 
 	# Lo helps cover generics.
 	# TODO: would be nice to find a more popular alternative,
 	# at least once generics are more widespread.
 	github.com/samber/lo v1.21.0
 
-	# TODO: consider a SQL driver like modernc.org/sqlite
+	# Brotli is a compression algorithm popular with HTTP.
+	# It's also transpiled from C with a gitlab.com/cznic/ccgo,
+	# so this helps stress garble with Go code that few humans would write.
+	# Unlike other ccgo-generated projects, like modernc.org/sqlite,
+	# it's relatively small without any transitive dependencies.
+	github.com/andybalholm/brotli v1.0.4
+
+	# TODO: consider github.com/mattn/go-sqlite3 to cover a DB and more cgo
 )
 
 SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
@@ -54,16 +61,18 @@ for (( i=0; i<${#modules[@]}; i+=2 )); do
 		# We cache the files between runs and commit them in git,
 		# because each "go get module/...@version" is slow as it has to figure
 		# out where the module lives, even if GOMODCACHE is populated.
+		# Use the custom go.mod file for the rest of the commands via GOFLAGS.
+		# We should delete these cached modfiles with major Go updates,
+		# to recreate their "tidy" version with newer Go toolchains.
 		cached_modfile="${module}_${version}"
 		cached_modfile="${CACHED_MODFILES}/${cached_modfile//[^A-Za-z0-9._-]/_}.mod"
+		export GOFLAGS="${BASE_GOFLAGS} -modfile=${cached_modfile}"
+
 		if [[ ! -f "${cached_modfile}" ]]; then
 			show go mod init test
 			show go get "${module}/...@${version}"
 		fi
 
-		# Use the custom go.mod file for the rest of the commands.
-		export GOFLAGS="${BASE_GOFLAGS} -modfile=${cached_modfile}"
-
 		# Run "go build" first, to ensure the regular Go build works.
 		show go build "${module}/..."
 

testdata/scripts/position.txt

@@ -64,6 +64,13 @@ func main() {
 	// The nested expression is needed to prevent spaces.
 	fmt.Printf("%v\n", 10*float64(3.0)/float64(4.0))
 }
+
+// Replacing `s` and `f` with obfuscated names triggered a bug in go/printer,
+// where it would move `.f` after the comment, breaking the syntax.
+func issue_573(s struct{ f int }) {
+	var _ *int = &s.f
+	/*x*/
+}
 -- garble_other_filename.go --
 package main