cmd/gofmt: inconsistent formatting of spacing around operators

[engineering-this]

What version of Go are you using (go version)?

$ go version
go version go1.12.9 linux/amd64

Does this issue reproduce with the latest release?

Yes, tested with format button on https://play.golang.org/p/Ei6Hu4QqAHa

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/{username}/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/{username}/go"
GOPROXY=""
GORACE=""
GOROOT="/snap/go/4301"
GOTMPDIR=""
GOTOOLDIR="/snap/go/4301/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build430600410=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Operators inside function calls are inconsistently formatted depending if they are inside append or not.

What did you expect to see?

  bar := []int{
    baz(1 * 2),
  }
  bar = append(bar,
    baz(1 * 2),
  )

What did you see instead?

  bar := []int{
    baz(1 * 2),
  }
  bar = append(bar,
    baz(1*2),
  )

[mvdan]

I believe that this is by design, but has nothing to do with the append function per se. It's just about the syntax. See #12105 or #1206, for example.

I'll leave it open in case @griesemer has anything to add.

[griesemer]

This has been like this probably since the early days of gofmt. For comparison, formatting of various expressions:

x = 1 * 2
x = (1 * 2)
x = f(1 * 2)
x = g(1, f(1*2))

I agree that there's little difference between a list of composite literal elements and parameter lists and this is an inconsistency.

It's not clear that we should change it as it would cause quite a bit of churn. If we do, we may want to combine it with some more gofmt cleanups/improvements.

[griesemer]

Related: #11497

[pepa65]

Concatenating strings in an assignment requires spaces around the +, same for inside function calls that have just 1 argument, but when there's a , inside the function call, the + must not have spaces...

[daniel-santos]

Well here's a sloppy patch for the issue. It's enabled with a somewhat arbitrary "-o" option. Personally, I think gofmt should have a library of configurable style options and then expose each of the predefined options such that they can be mutated. For example, --style-rules +spacesAroundBinaryOps,-keepSingleLineBlocks,+somethingElse or even something more comprehensive similar to astyle's options

Attempting to dictate to the masses precisely how thine code shall be formatted is a bad practice. There's nothing wrong with standards and style guides, but not allowing for alternatives comes off as a bit dictatorial or rigid.

From 3c24d38e11f761499b57315630760f02e0b7e471 Mon Sep 17 00:00:00 2001
From: Daniel Santos <daniel.santos@pobox.com>
Date: Thu, 22 Oct 2020 22:55:54 -0500
Subject: gofmt: Add option to not strip spaces around binary operators

---
 src/cmd/gofmt/gofmt.go    | 8 +++++++-
 src/go/printer/nodes.go   | 3 +++
 src/go/printer/printer.go | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/cmd/gofmt/gofmt.go b/src/cmd/gofmt/gofmt.go
index 8c56af7559..9488688677 100644
--- a/src/cmd/gofmt/gofmt.go
+++ b/src/cmd/gofmt/gofmt.go
@@ -32,6 +32,7 @@ var (
 	simplifyAST = flag.Bool("s", false, "simplify code")
 	doDiff      = flag.Bool("d", false, "display diffs instead of rewriting files")
 	allErrors   = flag.Bool("e", false, "report all errors (not just the first 10 on different lines)")
+	binOpSpaces = flag.Bool("o", false, "put spaces around all binary operators for Daniel")
 
 	// debugging
 	cpuprofile = flag.String("cpuprofile", "", "write cpu profile to this file")
@@ -120,7 +121,12 @@ func processFile(filename string, in io.Reader, out io.Writer, stdin bool) error
 		simplify(file)
 	}
 
-	res, err := format(fileSet, file, sourceAdj, indentAdj, src, printer.Config{Mode: printerMode, Tabwidth: tabWidth})
+	mode := printerMode
+	if *binOpSpaces {
+	    mode |= printer.SpacesAroundBinaryOps
+	}
+
+	res, err := format(fileSet, file, sourceAdj, indentAdj, src, printer.Config{Mode: mode, Tabwidth: tabWidth})
 	if err != nil {
 		return err
 	}
diff --git a/src/go/printer/nodes.go b/src/go/printer/nodes.go
index 95b9e91891..4c6f4b4a15 100644
--- a/src/go/printer/nodes.go
+++ b/src/go/printer/nodes.go
@@ -705,6 +705,9 @@ func (p *printer) binaryExpr(x *ast.BinaryExpr, prec1, cutoff, depth int) {
 		return
 	}
 
+	if cutoff >= 4 && cutoff <= 5 && (p.Mode&SpacesAroundBinaryOps) != 0  {
+		cutoff = 6
+	}
 	printBlank := prec < cutoff
 
 	ws := indent
diff --git a/src/go/printer/printer.go b/src/go/printer/printer.go
index 0077afeaff..d72ac1d772 100644
--- a/src/go/printer/printer.go
+++ b/src/go/printer/printer.go
@@ -1276,6 +1276,7 @@ const (
 	TabIndent                  // use tabs for indentation independent of UseSpaces
 	UseSpaces                  // use spaces instead of tabs for alignment
 	SourcePos                  // emit //line directives to preserve original source positions
+	SpacesAroundBinaryOps	   // put spaces around all binary operators
 )
 
 // The mode below is not included in printer's public API because
-- 
2.26.2

[l0k18]

I just bumped into this one due to a merge request I was doing that the manager pointed at all the squashed + operators in a section of code - I use gofmt pretty much religiously - and actually from a little probing and looking I've discovered that this unwanted squashing together (I think most will agree it's not as readable) only happens inside a parameter list for a function call and only if there is more than one parameter.

I guess the logic is that this groups the parameters more clearly, but I don't think it should be done this way. I also don't think it will 'break' as much code as it (as far as I can tell) only affects function calls and structured literals (I think yes also struct and slice literals).

I'm in the process of persuading my manager that gofmt solves more problems than it creates, I don't imagine this is a high priority. In any case, many go users use gofmt as a preparation step before running a linter, but it would only be linters that would break CI/CD pipelines, but, because it would be a pervasive, though trivial change I can understand why it might be considered to be a 2.0 change. But because gofmt does it I doubt there is one linter that flags this concatenation as an error.