x/tools/gopls: rearranging struct fields deletes comments #43735

ronakg · 2021-01-15T22:14:09Z

What version of Go, VS Code & VS Code Go extension are you using?

Run go version to get version of Go
- go1.15.6 darwin/amd64
Run gopls -v version to get version of Gopls if you are using the language server.

Build info
----------
golang.org/x/tools/gopls v0.6.3
    golang.org/x/tools/gopls@v0.6.3 h1:FCjXQzsa/jFSqbp2McJowMeshacTc8jhmMdJ22HpqiE=
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/google/go-cmp@v0.5.4 h1:L8R9j+yAqZuZjsqh/z+F1NCffTKKLShY6zXTItVIZ8M=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/mod@v0.4.0 h1:8pl+sMODzuvGJkmj2W4kZihvVb5mKm8pB/X44PIQHv8=
    golang.org/x/sync@v0.0.0-20201020160332-67f06af15bc9 h1:SQFwaSi55rU7vdNs9Yr0Z324VNlrF+0wMqRXT4St8ck=
    golang.org/x/tools@v0.0.0-20210112235408-75fd75db8797 h1:kcvRujT1OsSzHGjvqsV0XWy92+z4TUgV2YwQH9aQt8I=
    golang.org/x/xerrors@v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
    honnef.co/go/tools@v0.0.1-2020.1.6 h1:W18jzjh8mfPez+AwGLxmOImucz/IFjpNlrKVnaj2YVc=
    mvdan.cc/gofumpt@v0.1.0 h1:hsVv+Y9UsZ/mFZTxJZuHVI6shSQCtzZ11h1JEFPAZLw=
    mvdan.cc/xurls/v2@v2.2.0 h1:NSZPykBXJFCetGZykLAxaL6SIpvbVy/UFEniIfHAa8A=

Run code -v or code-insiders -v to get version of VS Code or VS Code Insiders

1.52.1
ea3859d4ba2f3e577a159bc91e3074c5d85c0523
x64

Check your installed extensions to get the version of the VS Code Go extension
- 0.20.2
Run go env to get the go development environment details

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/rogandhi/Library/Caches/go-build"
GOENV="/Users/rogandhi/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/rogandhi/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/rogandhi/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/rogandhi/work/test/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/f1/d20cq36s6gn1kj0vgdv855t00000gn/T/go-build048821892=/tmp/go-build -gno-record-gcc-switches -fno-common"

Share the Go related settings you have added/edited

Run Preferences: Open Settings (JSON) command to open your settings.json file.
Share all the settings with the go. or ["go"] or gopls prefixes.

{
    "[go]": {
        "editor.codeActionsOnSave": {
            "source.fixAll": true,
            "source.organizeImports": true
        },
        "editor.formatOnSave": true
    },
    "go.addTags": {
        "transform": "camelcase"
    },
    "go.autocompleteUnimportedPackages": true,
    "go.buildOnSave": "workspace",
    "go.coverOnSingleTest": true,
    "go.coverOnSingleTestFile": true,
    "go.coverOnTestPackage": true,
    "go.coverShowCounts": false,
    "go.coverageDecorator": {
        "type": "highlight"
    },
    "go.editorContextMenuCommands": {
        "addImport": true,
        "addTags": true,
        "debugTestAtCursor": true,
        "generateTestForFile": true,
        "generateTestForFunction": true,
        "generateTestForPackage": true,
        "playground": true,
        "removeTags": true,
        "testAtCursor": true,
        "testCoverage": true,
        "testFile": true,
        "testPackage": true,
        "toggleTestFile": true
    },
    "go.enableCodeLens": {
        "references": false,
        "runtest": true
    },
    "go.formatTool": "goimports",
    "go.gotoSymbol.ignoreFolders": [
        "vendor"
    ],
    "go.gotoSymbol.includeImports": true,
    "go.languageServerExperimentalFeatures": {
        "diagnostics": true,
        "documentLink": true
    },
    "go.lintOnSave": "workspace",
    "go.lintTool": "golangci-lint",
    "go.testFlags": [
        "-v"
    ],
    "go.testTimeout": "10m",
    "go.useCodeSnippetsOnFunctionSuggestWithoutType": true,
    "go.useLanguageServer": true,
    "gopls": {
        "analyses": {
            "fillreturns": true,
            "nonewvars": true,
            "undeclaredname": true,
            "unreachable": true,
            "unusedparams": true,
            "fieldalignment": true,
            "shadow": true
        },
        "gofumpt": true,
        "usePlaceholders": true
    },
    "search.exclude": {
        "**/vendor": true
    },
    "editor.wordWrap": "bounded",
    "editor.wordWrapColumn": 100
}

Describe the bug

One of the recommended refactoring actions is to rearrange the struct fields to better align and save memory. When this action is carried out, the comments written for the fields get deleted.

Steps to reproduce the behavior:

Example struct:

type foo struct {
	timeout time.Duration // timeout
	flag1   bool          // flag1
	name1   string        // name1
	flag2   bool          // flag2
	name2   string        // name2
	value   int           // value1
	u       url.URL       // url
}

If you rearrange the struct fields using the suggested action, the comments are deleted (check the attached recording).

Screenshots or recordings

Screen.Recording.2021-01-15.at.2.10.16.PM.mov

The text was updated successfully, but these errors were encountered:

hyangah · 2021-01-15T23:00:34Z

cc @leitzler

@heschik found https://go-review.googlesource.com/c/tools/+/278872

heschi · 2021-01-15T23:11:46Z

I don't think we should have a feature that unexpectedly deletes user's code. If go/ast is too unhelpful, perhaps we could change the title of the suggested fix to "Rearrange fields (may delete comments)" or some such?

leitzler · 2021-01-17T19:11:34Z

Yes, as per CL 278872 this is indeed how it works (also see #20744). I agree that it isn't optimal.

Given the niched nature of this analyser (and the fact that it is opt-in), how about adding a note to the documentation? That would be the entry-point for users.

Updating the title to "Rearrange fields (removes comments)" could add confusion, especially to those who wouldn't notice since they don't have comments. In that case, I'd rather see that we only update the title if there are any comments.

The long term goal is to keep the comments, but as I see it (with very limited experience of AST) it isn't trivial. If there are ideas on how to achieve that please let me know.

heschi · 2021-01-19T18:52:47Z

FWIW, I think having the suffix appear only when there are comments that will be destroyed is a good idea and should be completely possible.

My understanding matches yours: there is a long-standing feature request in goimports that is blocked by poor comment support in the AST. So I think noting the problem in the title is actually the best path forward to get this on by default :-/

leitzler · 2021-01-19T19:07:29Z

Just to clarify, I think that fieldalignment is a bit too niched to ever be on by default.

It does have its place but will probably generate too much irrelevant noise for the average user. In most cases readability-ordered structs gives more than optimal aligned.

I'll see if I can send a CL for updating the title based on comment removal when time allows.

ronakg · 2021-01-20T00:00:49Z

Just to clarify, I think that fieldalignment is a bit too niched to ever be on by default.

It does have its place but will probably generate too much irrelevant noise for the average user. In most cases readability-ordered structs gives more than optimal aligned.

I'll see if I can send a CL for updating the title based on comment removal when time allows.

Can the severity of this check be changed from warning to info? In that case, I can keep it on by default but it appears as an FYI rather than a warning.

dnp1 · 2024-04-11T16:15:22Z

+1 about this

hyangah changed the title ~~Rearranging struct fields deletes comments~~ x/tools/gopls: rearranging struct fields deletes comments Jan 15, 2021

hyangah transferred this issue from golang/vscode-go Jan 15, 2021

gopherbot added Tools gopls labels Jan 15, 2021

gopherbot added this to the Unreleased milestone Jan 15, 2021

stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Jan 22, 2021

stamblerre removed this from the gopls/v1.0.0 milestone Jun 28, 2021

seankhliao added this to the gopls/unplanned milestone Aug 27, 2022

estensen mentioned this issue Oct 21, 2022

Order fields in structs prysmaticlabs/prysm#11564

Closed

marclop mentioned this issue Dec 21, 2022

perf: Align structs to eliminate padding elastic/apm-data#4

Merged

findleyr added the Refactoring label Nov 28, 2023

findleyr modified the milestones: gopls/unplanned, gopls/backlog Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/tools/gopls: rearranging struct fields deletes comments #43735

x/tools/gopls: rearranging struct fields deletes comments #43735

ronakg commented Jan 15, 2021

hyangah commented Jan 15, 2021

heschi commented Jan 15, 2021

leitzler commented Jan 17, 2021

heschi commented Jan 19, 2021

leitzler commented Jan 19, 2021

ronakg commented Jan 20, 2021

dnp1 commented Apr 11, 2024

x/tools/gopls: rearranging struct fields deletes comments #43735

x/tools/gopls: rearranging struct fields deletes comments #43735

Comments

ronakg commented Jan 15, 2021

What version of Go, VS Code & VS Code Go extension are you using?

Share the Go related settings you have added/edited

Describe the bug

Steps to reproduce the behavior:

Screenshots or recordings

hyangah commented Jan 15, 2021

heschi commented Jan 15, 2021

leitzler commented Jan 17, 2021

heschi commented Jan 19, 2021

leitzler commented Jan 19, 2021

ronakg commented Jan 20, 2021

dnp1 commented Apr 11, 2024