Go: proposal: cmd/gofmt: add option to insert missing end-of-line commas in program

[rhysd]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.4 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/rhysd/.go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.4_1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.4_1/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/sv/mr3y8ytd7gzfdww8gxx568z80000gn/T/go-build764880956=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

First, save below code as blah.go

package main

func main() {
    a := []int{
        1,
        2
    }
}

Then execute below command

$ gofmt blah.go

What did you expect to see?

Automatically fix trailing comma at 2 (line 6, column 9) and formatting has been done successfully.

What did you see instead?

Formatting failed with below error

blah.go:6:4: missing ',' before newline in composite literal

I believe this is because gofmt can't format a code which contains syntax error. Trailing comma such as line 6 in above is a syntax error in Go. So gofmt cannot fix this. However, we tend to forget adding trailing comma when expose one line code to multi-line code. If gofmt can fix it automatically, we no longer need to take care about trailing commas.

I guess implementing this is not so difficult. When parsing failed, then fix the source and retry to parse it again.

[ALTree]

gofmt is a code formatter, I don't think it should be required to handle invalid go code, much less change it to correct errors.

[mvdan]

It also goes down a slippery slope. What syntax errors should be fixed? Where do you draw the line?

[rhysd]

How about adding a tool like a goimports? It is a code fixer for import statements.

[ALTree]

gofixtrailingcommas ? Mmh...

Honestly I'm not sure it would meet the bar for inclusion in x/tools/cmd; but you can always publish it as an open source project. I don't think it'll be used as widely as goimports though, this is a much smaller problem.

[rhysd]

I agree that adding new tool like this is bike-shedding... But is there any chance to add more generic tool like go fix? For example, go fix imports does the same as goimports. go fix trailling-comma fixes commas, ...

[rhysd]

but you can always publish it as an open source project.

Hmm... I can create a fixer which fixes something before passing a code to gofmt. I think I should try it (although parsing twice and spawning a process for gofmt may be not good for performance).

[rhysd]

How about adding a new option to ignore parse error if no bad node is in AST? I tried modifying gofmt code as below.

https://github.com/rhysd/gofmtrlx

https://github.com/rhysd/gofmtrlx/commit/76c93b220c40631f1e8e7385ecb4685923526c21

I found that there is no bad node in AST even if trailing comma error occurs. So, as gofmt is formatter, it can ignore the parse error, just format the code even if parse error is contained when there is no bad node in AST. How do you think?

[haya14busa]

btw, gofmt already trims trailing , in function call.

$ gofmt -d test.go
diff test.go gofmt/test.go
--- /tmp/gofmt596094646 2017-02-04 23:43:27.384131425 +0900
+++ /tmp/gofmt073783965 2017-02-04 23:43:27.384131425 +0900
@@ -7,7 +7,7 @@
                3,
        }

-       f(1, 2, )
+       f(1, 2)
 }

I think it's reasonable to add missing trailing comma to some extent considering this behavior.
Especially if we can draw a reasonable line with detecting ast.Bad* node.
It's also really useful.

[mvdan]

@haya14busa note that what you're describing there is transforming a valid program into another valid program.

[haya14busa]

Oh, that's true. I didn't know it's valid syntax because I'm always using gofmt before compile.

But my main point is, if go/parser can correctly created ast node, why not using it instead of discarding it with trivial error.

[mattn]

@rhysd I think that gofmt ignore the syntax error is not good. I'm not sure but, how about ignoring the error in your editor or something using gofmt? For example, vim-go may be possible to ignore the output of gofmt by filtering that error. missing ',' before newline in composite literal

[rhysd]

@mattn If ignoring the error, I can't know why formatting failed and nothing was formatted. I don't want to stop formatting.

[rhysd]

After all, what I want to say is what @haya14busa said (thanks @haya14busa). Even if there is a parse error, Go's parser can parse it. gofmt is not a syntax checker. IMO gofmt should be able to format every code which parser can parse without bad AST node.

[rhysd]

Please note that I don't say this should be default behavior. But I feel there should be an option for this behavior.

[mattn]

Even though adding the new option to ignore bad AST node, gofmt should exit by non zero, I think. But it can output verbosy syntax warnings.

[rhysd]

@mattn yeah, it should show warnings and return non-zero exit code if there was warning. Thank you for the point. I agree :+1:

[mattn]

@rhysd BTW, what is your motivation for this?

[rhysd]

@mattn

From description of this issue:

we tend to forget adding trailing comma when expose one line code to multi-line code.

I needed to fix trailing comma errors while writing a code. I thought gofmt might be able to fix them automatically. It was the original point I made this issue.

[mattn]

I thought gofmt might be able to fix it automatically. It was the original point I made this issue.

It will be more approval than this proposal, I guess. 👍

[rsc]

Whether there's a bad node in AST is not necessarily indicative of how close the file was to real Go. It sounds like the actual request is to insert missing end-of-line commas.

[rsc]

If the input said:

 []int{1, 2
     +3, 4}

then inserting a comma after 2 could possibly be breaking the program instead of helpfully fixing it. This seems undeniably convenient, but also dangerous. And it opens a slippery slope for other "help".

Also, gofmt is the "cat" of Go programs. There can be other programs that process them instead. Not everything has to go into gofmt.

I've been assuming that editor integration would make it easy to jump from the error message to the exact place noted in the error message, and the message itself says "put a comma here". That seems pretty direct and easy. If any editor integrations do _not_ make it easy to jump from error messages to the named locations, please file bugs against them.

-rsc for @golang/proposal-review

[rhysd]

@rsc Ah, you're correct. I could not come up with the edge case. The edge case can't be fixed only by filtering error message hence it looks hard to fix.

Thank you all for discussion.

[mattn]

https://github.com/golang/go/blob/2e5116bd999be18691d860e47cb87f1446cf70fe/src/go/parser/parser.go#L1359

blah.go:6:4: missing ',' before newline in composite literal

expectClosing seems to be called after finished to parse element list of composite literal. thus, I guess we can change the message for this like missing '}' in composite literal.

[minux]

proposing changes to diagnostic messages are hard because it's quite easy
to consider one case and overlooked another.

For this particular case, I argue that it's more common to left out a comma
than to left out a closing brace.
Usually the program will have balanced braces (most editors will help with
that), so missing commas should be the primary cause here.