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 indentation with comments in switch #38495

Open
seankhliao opened this issue Apr 17, 2020 · 6 comments
Open

cmd/gofmt: inconsistent indentation with comments in switch #38495

seankhliao opened this issue Apr 17, 2020 · 6 comments
Assignees
Milestone

Comments

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Apr 17, 2020

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

$ go version
go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

yes, also in the playground

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/arccy/.cache/go-build"
GOENV="/home/arccy/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/arccy/data/xdg/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/arccy/testrepo-82/go.mod"
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-build346952514=/tmp/go-build -gno-record-gcc-switches"

What did you do?

write inconsistently indented code/comments in a switch
https://play.golang.org/p/QWIBPjclreK

package main

func a() {
	switch "" {
	case "a":
// a comment
	case "b":
	}
}

func b() {
	switch "" {
	case "a":
	// a comment
case "b":
	}
}

What did you expect to see?

consistently indented comments

package main

func a() {
	switch "" {
	case "a":
	// a comment
	case "b":
	}
}

func b() {
	switch "" {
	case "a":
	// a comment
	case "b":
	}
}

What did you see instead?

package main

func a() {
	switch "" {
	case "a":
		// a comment
	case "b":
	}
}

func b() {
	switch "" {
	case "a":
		// a comment
	case "b":
	}
}

observations

this does not happen when the comment and the case below it are indented by the same number of whitespace characters, so the below formats properly

package main

func a() {
	switch "" {
	case "a":
 // leading single space
	case "b": // leading tab
	}
}
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 17, 2020

@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 17, 2020

Hm, indeed. Looks like a bug at first glance. Thanks for reporting.

@griesemer griesemer self-assigned this Apr 17, 2020
@griesemer griesemer added this to the Go1.15 milestone Apr 17, 2020
@kortschak
Copy link
Contributor

@kortschak kortschak commented Apr 17, 2020

Why is this a bug? The gofmt formatting of switch case comments has two valid arrangements, 1) aligned with the leading c of case and 2) indented one level from the case statement.

If the code in the expected section is formatted, it remains the same (this can be tried here). The issue is how gofmt can know which of the two cases the author wants (one would likely be a case-leading comment and the other an in case-block comment as shown here - which remains unchanged when gofmt is run).

I note there is some discussion on this in gophers #tools that would be relevant to bring in here.

@seankhliao
Copy link
Contributor Author

@seankhliao seankhliao commented Apr 17, 2020

The issue from the slack discussion was when you manually moved the comments into alignment with the case (leading c) gofmt would still reformat and indent it one level in. Looking at it now I think the examples included here don't accurately reflect that behavior.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Apr 18, 2020

I'm familiar with this gofmt behavior, and I've spent some time thinking whether it's a bug or not (if it is, it's very minor). I agree with @kortschak's characterization of this issue.

Given there are two valid gofmt-ed outcomes, I think gofmt should be making the edit which results in a smaller diff. So, given this non-gofmted block:

for {
	switch n {
	case 1:
		foo()
			// Comment that applies to case 1 or case 2 depending on its indentation.
	case 2:
		bar()
	}
}

After formatting, it should become:

for {
	switch n {
	case 1:
		foo()
		// Comment that applies to case 1 or case 2 depending on its indentation.
	case 2:
		bar()
	}
}

This is the current behavior. I think it's good.

However, the other side doesn't work as expected. Starting with this non-gofmted block:

for {
	switch n {
	case 1:
		foo()
// Comment that applies to case 1 or case 2 depending on its indentation.
	case 2:
		bar()
	}
}

gofmt currently inserts two tabs instead of one tab.

Given both one tab and two tabs would make the program gofmted, I think it should be making the smaller edit rather than the bigger.

@tommyknows
Copy link

@tommyknows tommyknows commented May 1, 2020

Seems like this and similar cases have come up a couple times now, see #30327 and its linked issues.

I guess the issue here is that there is no agreed upon definition on how comments in switch-statements should be inlined, but I agree with the above proposal.

However, a question that would come up is still the comment at the end of the whole switch statement block, as shown in #30327:

package main

func main() {
	val := 1
	switch val {
	case 1:
		_ = 1
	// case 5: // gets aligned to the case <- this would be left as-is, which makes sense.
	// 	_ = 5
	case 2:
		_ = 2
		// case 3: // gets aligned to the code block inside the case <- this would be left-as-is?
		// 	_ = 3
	}
}

While the case 5 comment is clear, I think case 3 should be indented to the same level as the other cases.
The reason for this is that I suspect (I don't have any numbers on this) that comments right before the end of a switch statement are rarely referring to the code above them.

This would be an enhancement to what has been said by @dmitshur, for completeness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.