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: rewrite Allman brace style, even if it doesn't compile? #34942

Closed
adamtabrams opened this issue Oct 16, 2019 · 4 comments
Closed

cmd/gofmt: rewrite Allman brace style, even if it doesn't compile? #34942

adamtabrams opened this issue Oct 16, 2019 · 4 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@adamtabrams
Copy link

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

$ go version
go version go1.13.1 darwin/amd64

Does this issue reproduce with the latest release?

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/aabrams7/Library/Caches/go-build"
GOENV="/Users/aabrams7/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/aabrams7/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bt/hgxc0qbd6s3d8ccbfpdv3hqng_1znh/T/go-build684397577=/tmp/go-build -gno-record-gcc-switches -fno-common"
GOROOT/bin/go version: go version go1.13.1 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.13.1
uname -v: Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.14.6
BuildVersion:	18G95
lldb --version: lldb-1001.0.13.3
  Swift-5.0

What did you do?

I have a simple hello world program like this:

package main

import "fmt"

func main()
{
    fmt.Println("Hello World")
}

To fix the formatting I ran this:

 gofmt -w ./hello.go

What did you expect to see?

I expected the file to change to this:

package main

import "fmt"

func main() {
    fmt.Println("Hello World")
}

What did you see instead?

hello.go:6:1: expected declaration, found '{'

Also, the file has not been changed at all.

@ianlancetaylor
Copy link
Contributor

Note that

func main()

is valid Go to indicate that the function main is declared in an assembler file. See https://golang.org/ref/spec#Function_declarations.

We could add a special case in gofmt for a function declaration followed by a block enclosed in curly braces, but it wouldn't be all that simple.

@agnivade
Copy link
Contributor

This program doesn't compile. AFAIR, gofmt fails to run on files which throw compiler errors.

@bradfitz
Copy link
Contributor

To @griesemer for decision.

@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 17, 2019
@bradfitz bradfitz changed the title gofmt will not alter file cmd/gofmt: rewrite Allman brace style, even if it doesn't compile? Oct 17, 2019
@griesemer
Copy link
Contributor

griesemer commented Oct 17, 2019

As has been already stated,

func main()
{ ... }

which is the same as func main(); { ... } is not valid.

The parser could possibly recognize the case of a stand-alone block (at the package level) preceded by a body-less function header, report an error, but assume the writer meant the "correct" program. We do this in other places in the parser, for better error recovery. This would lead to a better experience for the programmer, which is what we generally strive for.

But the program would still be incorrect (the parser must report an error), and gofmt won't format an erroneous program. (One could imagine the parser to report a form of "soft" error, i.e., an actual syntax error that didn't result in an invalid AST. If a program only consists of "soft" errors, one might allow gofmt to go ahead. But this seems a bit overkill in this case.)

I filed #34946 to look into improved parsing.

But even with that, gofmt won't and can't reasonably go ahead with an incorrect program.

@golang golang locked and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants