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

Closed
rhysd opened this Issue Feb 4, 2017 · 24 comments

Comments

Projects
None yet
8 participants
@rhysd

rhysd commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ALTree

ALTree Feb 4, 2017

Member

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.

Member

ALTree commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Feb 4, 2017

Member

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

Member

mvdan commented Feb 4, 2017

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

@rhysd

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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

rhysd commented Feb 4, 2017

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

@ALTree

This comment has been minimized.

Show comment
Hide comment
@ALTree

ALTree Feb 4, 2017

Member

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.

Member

ALTree commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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 commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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 commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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

rhysd/gofmtrlx@76c93b2

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?

rhysd commented Feb 4, 2017

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

rhysd/gofmtrlx@76c93b2

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?

@rhysd rhysd changed the title from proposal: gofmt: Fix trailing commas automatically to proposal: gofmt: Add option to ignore parse error if no bad node in AST Feb 4, 2017

@haya14busa

This comment has been minimized.

Show comment
Hide comment
@haya14busa

haya14busa Feb 4, 2017

Contributor

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.

Contributor

haya14busa commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Feb 4, 2017

Member

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

Member

mvdan commented Feb 4, 2017

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

@haya14busa

This comment has been minimized.

Show comment
Hide comment
@haya14busa

haya14busa Feb 4, 2017

Contributor

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.

Contributor

haya14busa commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Feb 4, 2017

Member

@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

Member

mattn commented Feb 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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

rhysd commented Feb 4, 2017

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

@rhysd

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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 commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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

rhysd commented Feb 4, 2017

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

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Feb 4, 2017

Member

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.

Member

mattn commented Feb 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

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

rhysd commented Feb 4, 2017

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

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Feb 4, 2017

Member

@rhysd BTW, what is your motivation for this?

Member

mattn commented Feb 4, 2017

@rhysd BTW, what is your motivation for this?

@rhysd

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 4, 2017

@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.

rhysd commented Feb 4, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Feb 4, 2017

Member

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. 👍

Member

mattn commented Feb 4, 2017

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 rsc changed the title from proposal: gofmt: Add option to ignore parse error if no bad node in AST to proposal: cmd/gofmt: add option to insert missing end-of-line commas in program Feb 6, 2017

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Feb 6, 2017

Contributor

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.

Contributor

rsc commented Feb 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Feb 6, 2017

Contributor

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

Contributor

rsc commented Feb 6, 2017

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

@rsc rsc closed this Feb 6, 2017

@rhysd

This comment has been minimized.

Show comment
Hide comment
@rhysd

rhysd Feb 7, 2017

@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.

rhysd commented Feb 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Feb 7, 2017

Member

rbrace := p.expectClosing(token.RBRACE, "composite literal")

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.

Member

mattn commented Feb 7, 2017

rbrace := p.expectClosing(token.RBRACE, "composite literal")

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

This comment has been minimized.

Show comment
Hide comment
@minux

minux Feb 7, 2017

Member
Member

minux commented Feb 7, 2017

@golang golang locked and limited conversation to collaborators Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.