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

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

Open
ronakg opened this issue Jan 15, 2021 · 6 comments
Open

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

ronakg opened this issue Jan 15, 2021 · 6 comments
Labels
Milestone

Comments

@ronakg
Copy link

@ronakg ronakg commented Jan 15, 2021

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
@hyangah hyangah changed the title Rearranging struct fields deletes comments x/tools/gopls: rearranging struct fields deletes comments Jan 15, 2021
@hyangah hyangah transferred this issue from golang/vscode-go Jan 15, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jan 15, 2021
@heschik
Copy link
Contributor

@heschik heschik commented Jan 15, 2021

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
Copy link
Contributor

@leitzler leitzler commented Jan 17, 2021

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.

@heschik
Copy link
Contributor

@heschik heschik commented Jan 19, 2021

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
Copy link
Contributor

@leitzler leitzler commented Jan 19, 2021

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
Copy link
Author

@ronakg ronakg commented Jan 20, 2021

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.

@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Jan 22, 2021
@stamblerre stamblerre added this to To Do in gopls: v1.0.0 Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants