Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/gofmt: inconsistent formatting of spacing around operators #34426

Open
engineering-this opened this issue Sep 20, 2019 · 6 comments
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@engineering-this
Copy link

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
Copy link
Member

mvdan commented Sep 20, 2019

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.

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 20, 2019
@toothrot toothrot changed the title Inconsistent formatting of operators inside append cmd/go: gofmt inconsistent formatting of operators inside multi-line function calls Sep 20, 2019
@toothrot toothrot added this to the Go1.14 milestone Sep 20, 2019
@toothrot toothrot changed the title cmd/go: gofmt inconsistent formatting of operators inside multi-line function calls cmd/go: gofmt inconsistent formatting of spacing around operators Sep 20, 2019
@griesemer
Copy link
Contributor

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 griesemer self-assigned this Sep 21, 2019
@griesemer griesemer modified the milestones: Go1.14, Unplanned Sep 21, 2019
@griesemer
Copy link
Contributor

Related: #11497

@jayconrod jayconrod changed the title cmd/go: gofmt inconsistent formatting of spacing around operators cmd/gofmt: inconsistent formatting of spacing around operators Nov 11, 2019
@pepa65
Copy link

pepa65 commented May 31, 2020

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
Copy link

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
Copy link

l0k18 commented Jun 7, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants