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: will change some comments to a less correct format #54789

Closed
Abirdcfly opened this issue Aug 31, 2022 · 5 comments
Closed

cmd/gofmt: will change some comments to a less correct format #54789

Abirdcfly opened this issue Aug 31, 2022 · 5 comments

Comments

@Abirdcfly
Copy link
Contributor

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

$ go version
go version go1.19 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN="/usr/local/opt/go/libexec/bin"
GOCACHE="/Users/fupeng/Library/Caches/go-build"
GOENV="/Users/fupeng/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/fupeng/go/pkg/mod"
GONOPROXY="*tenxcloud.com"
GONOSUMDB="*tenxcloud.com"
GOOS="darwin"
GOPATH="/Users/fupeng/go"
GOPRIVATE="*tenxcloud.com"
GOPROXY="https://goproxy.cn"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/fupeng/go/src/tools/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3v/mkwyck4x0f5bxv2gdv4_l8v00000gn/T/go-build1860409478=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Let me use a few examples of code from the https://github.com/golang/tools
example:

https://github.com/golang/tools/blob/248c34b88a4148128f89e41923498bd86f805b7d/gopls/internal/coverage/coverage.go#L15

https://github.com/golang/tools/blob/248c34b88a4148128f89e41923498bd86f805b7d/internal/lsp/diff/lcs/labels.go#L11

https://github.com/golang/tools/blob/248c34b88a4148128f89e41923498bd86f805b7d/internal/typeparams/coretype.go#L84

https://github.com/golang/tools/blob/248c34b88a4148128f89e41923498bd86f805b7d/internal/typeparams/typeterm.go#L13

1. in go1.18 gofmt does not change these files

$ docker run --rm -v $PWD:/go/src/tools  --rm -ti golang:1.18 gofmt -w -d /go/src/tools/gopls/internal/coverage/coverage.go
$ docker run --rm -v $PWD:/go/src/tools  --rm -ti golang:1.18 gofmt -w -d /go/src/tools/internal/lsp/diff/lcs/labels.go
$ docker run --rm -v $PWD:/go/src/tools  --rm -ti golang:1.18 gofmt -w -d /go/src/tools/internal/typeparams/coretype.go
$ docker run --rm -v $PWD:/go/src/tools  --rm -ti golang:1.18 gofmt -w -d /go/src/tools/internal/typeparams/typeterm.go

2. in go1.19 gofmt make some comments to a less correct format

$ gofmt -w -d gopls/internal/coverage/coverage.go internal/lsp/diff/lcs/labels.go internal/typeparams/coretype.go internal/typeparams/typeterm.go
diff gopls/internal/coverage/coverage.go.orig gopls/internal/coverage/coverage.go
--- gopls/internal/coverage/coverage.go.orig
+++ gopls/internal/coverage/coverage.go
@@ -12,9 +12,13 @@
 // -o controls where the coverage file is written, defaulting to /tmp/cover.out
 // -i coverage-file will generate the report from an existing coverage file
 // -v controls verbosity (0: only report coverage, 1: report as each directory is finished,
-//      2: report on each test, 3: more details, 4: too much)
+//
+//     2: report on each test, 3: more details, 4: too much)
+//
 // -t tests only tests packages in the given comma-separated list of directories in gopls.
-//      The names should start with ., as in ./internal/regtest/bench
+//
+//     The names should start with ., as in ./internal/regtest/bench
+//
 // -run tests. If set, -run tests is passed on to the go test command.
 //
 // Despite gopls' use of goroutines, the counts are almost deterministic.
diff internal/lsp/diff/lcs/labels.go.orig internal/lsp/diff/lcs/labels.go
--- internal/lsp/diff/lcs/labels.go.orig
+++ internal/lsp/diff/lcs/labels.go
@@ -8,7 +8,8 @@
        "fmt"
 )

-//  For each D, vec[D] has length D+1,
+//     For each D, vec[D] has length D+1,
+//
 // and the label for (D, k) is stored in vec[D][(D+k)/2].
 type label struct {
        vec [][]int
diff internal/typeparams/coretype.go.orig internal/typeparams/coretype.go
--- internal/typeparams/coretype.go.orig
+++ internal/typeparams/coretype.go
@@ -81,13 +81,13 @@
 // restrictions may be arbitrarily complex. For example, consider the
 // following:
 //
-//  type A interface{ ~string|~[]byte }
-//
-//  type B interface{ int|string }
-//
-//  type C interface { ~string|~int }
-//
-//  type T[P interface{ A|B; C }] int
+//     type A interface{ ~string|~[]byte }
+//
+//     type B interface{ int|string }
+//
+//     type C interface { ~string|~int }
+//
+//     type T[P interface{ A|B; C }] int
 //
 // In this example, the structural type restriction of P is ~string|int: A|B
 // expands to ~string|~[]byte|int|string, which reduces to ~string|~[]byte|int,
diff internal/typeparams/typeterm.go.orig internal/typeparams/typeterm.go
--- internal/typeparams/typeterm.go.orig
+++ internal/typeparams/typeterm.go
@@ -10,11 +10,10 @@

 // A term describes elementary type sets:
 //
-//   ∅:  (*term)(nil)     == ∅                      // set of no types (empty set)
-//   𝓤:  &term{}          == 𝓤                      // set of all types (𝓤niverse)
-//   T:  &term{false, T}  == {T}                    // set of type T
-//  ~t:  &term{true, t}   == {t' | under(t') == t}  // set of types with underlying type t
-//
+//      ∅:  (*term)(nil)     == ∅                      // set of no types (empty set)
+//      𝓤:  &term{}          == 𝓤                      // set of all types (𝓤niverse)
+//      T:  &term{false, T}  == {T}                    // set of type T
+//     ~t:  &term{true, t}   == {t' | under(t') == t}  // set of types with underlying type t
 type term struct {
        tilde bool // valid if typ != nil
        typ   types.Type

What did you expect to see?

I guess there are at least some comments that should not be changed

What did you see instead?

The comments on these files have changed

@mvdan
Copy link
Member

mvdan commented Aug 31, 2022

This is intended behavior; see https://tip.golang.org/doc/go1.19#go-doc and #51082. If you find a particular bug with the rewriting, I would suggest to make your bug report clearer, like #54312. You can always try to argue that this whole rewriting feature shouldn't happen at all, but then that's more of a feature reversal request than a bug report.

@seankhliao
Copy link
Member

+// For each D, vec[D] has length D+1,

That an error in source formatting (extra space), that needs to be fixed manually

The rest look like gofmt working as intended.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2022
@Abirdcfly
Copy link
Contributor Author

Abirdcfly commented Aug 31, 2022

@seankhliao please look at

https://github.com/golang/tools/blob/248c34b88a4148128f89e41923498bd86f805b7d/gopls/internal/coverage/coverage.go#L15

The original spacing looks correct:

// -v controls verbosity (0: only report coverage, 1: report as each directory is finished,
//      2: report on each test, 3: more details, 4: too much)

will gofmt format as fellow:

// -v controls verbosity (0: only report coverage, 1: report as each directory is finished,
//
//	2: report on each test, 3: more details, 4: too much)
//

But this is incorrect, the 2: xxx 3: xxx 4: xxx here is just a manual line break because the previous line is too long.
the 2: xxx 3: xxx 4: xxx should be next to the previous line 0:xxx 1:xxx with some Space but no newline.

I have try some option:

  1. //(Space)(Space)2:
  2. //(Space)(Space)(Space)2:
  3. //(Tab)2:
  4. //(Tab)(Tab)2:
  5. //(Space)2:
  6. use /* */

only option 5 and option 6 Keep my spacing from being gofmt to the above form. but option 5 make it look like:

// -v controls verbosity (0: only report coverage, 1: report as each directory is finished,
// 2: report on each test, 3: more details, 4: too much)

It is also not good. So the only option available is to use /* */:

/* -v controls verbosity (0: only report coverage, 1: report as each directory is finished,
        2: report on each test, 3: more details, 4: too much)
*/

I would like to know if the above changes are bugs or feature? 😂

@mvdan
Copy link
Member

mvdan commented Aug 31, 2022

The original godoc was buggy: it considered the second line as a blockquote due to the spacing. The godoc needs to be manually fixed to either blockquote the whole thing, or to not use any leading spaces and keep the whole thing as a regular paragraph. The gofmt rewriting is making the bad godoc surface, so I think that's a good outcome, even if the godoc is still buggy.

I would like to know if the above changes are bugs or feature? 😂

Please keep it civil and respectful :)

@Abirdcfly
Copy link
Contributor Author

Please keep it civil and respectful :)

Apologies for my poor English and translation tools, no rudeness intended here, I added emoticons to prevent misunderstandings, but it looks like it failed 😢

Thank you for your patience and answers.

ccx1024cc pushed a commit to ccx1024cc/image-service that referenced this issue Jan 19, 2023
Golang-lint 1.48+(included) uses gofmt based on go 1.9. The result of
Gofmt based on go1.9 is different from Gofmt based on
go1.18([different](golang/go#54789)). As a
result, Golang-lint 1.48+ reports "File is not `gofmt`-ed with `-s` (gofmt)",
although gofmt(1.8 based) says the code is ok.

Scroll back to golang-lint 1.47.3, which uses gofmt based on go 1.8. In
the future, upgrade golang-lint when go version is higher or equal to
1.9.

Signed-off-by: 泰友 <cuichengxu.ccx@antgroup.com>
ccx1024cc pushed a commit to ccx1024cc/image-service that referenced this issue Jan 19, 2023
Golang-lint 1.48+(included) uses gofmt based on go 1.9. The result of
Gofmt based on go1.9 is different from Gofmt based on
go1.18([different](golang/go#54789)). As a
result, Golang-lint 1.48+ reports "File is not `gofmt`-ed with `-s` (gofmt)",
although gofmt(1.8 based) says the code is ok.

Scroll back to golang-lint 1.47.3, which uses gofmt based on go 1.8. In
the future, upgrade golang-lint when go version is higher or equal to
1.9.

Signed-off-by: 泰友 <cuichengxu.ccx@antgroup.com>
ccx1024cc pushed a commit to ccx1024cc/image-service that referenced this issue Jan 19, 2023
Golang-lint 1.48+(included) uses gofmt based on go 1.9. The result of
Gofmt based on go1.9 is different from Gofmt based on
go1.18([different](golang/go#54789)). As a
result, Golang-lint 1.48+ reports "File is not `gofmt`-ed with `-s` (gofmt)",
although gofmt(1.8 based) says the code is ok.

Scroll back to golang-lint 1.47.3, which uses gofmt based on go 1.8. In
the future, upgrade golang-lint when go version is higher or equal to
1.9.

Signed-off-by: 泰友 <cuichengxu.ccx@antgroup.com>
ccx1024cc pushed a commit to ccx1024cc/image-service that referenced this issue Jan 19, 2023
Golang-lint 1.48+(included) uses gofmt based on go 1.9. The result of
Gofmt based on go1.9 is different from Gofmt based on
go1.18([different](golang/go#54789)). As a
result, Golang-lint 1.48+ reports "File is not `gofmt`-ed with `-s` (gofmt)",
although gofmt(1.8 based) says the code is ok.

Scroll back to golang-lint 1.47.3, which uses gofmt based on go 1.8. In
the future, upgrade golang-lint when go version is higher or equal to
1.9.

Signed-off-by: 泰友 <cuichengxu.ccx@antgroup.com>
@golang golang locked and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants