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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: fillstruct return invalid code when using pointers to builtin type #39854

Closed
leitzler opened this issue Jun 25, 2020 · 4 comments
Closed

x/tools/gopls: fillstruct return invalid code when using pointers to builtin type #39854

leitzler opened this issue Jun 25, 2020 · 4 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@leitzler
Copy link
Contributor

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

$ go version
go version go1.14.4 darwin/amd64
$ go list -m golang.org/x/tools golang.org/x/tools/gopls
golang.org/x/tools@v0.0.0-20200624163319-25775e59acb7
golang.org/x/tools/gopls@v0.0.0-20200624163319-25775e59acb7

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
n/a

What did you do?

Called the fillstruct code action on the following code:

package main

type Foo struct {
    a *bool
    b *int
    c *string
}

func main() {
    _ = Foo{}
}

What did you expect to see?

Valid code, using new(..) instead?

What did you see instead?

package main

type Foo struct {
    a *bool
    b *int
    c *string
}

func main() {
    _ = Foo{
        a: &false,
        b: &0,
        c: &"",
    }
}

/cc @joshbaum
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jun 25, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jun 25, 2020
@gopherbot
Copy link

Change https://golang.org/cl/239978 mentions this issue: internal/lsp/analysis/fillstruct: correct pointer to builtin values

@gopherbot
Copy link

Change https://golang.org/cl/239979 mentions this issue: [gopls-release-branch.0.4] internal/lsp/analysis/fillstruct: correct pointer to builtin values

@heschi
Copy link
Contributor

heschi commented Jun 25, 2020

Is new actually useful? I assumed the idea of filling the struct in was that you could easily change the values. But new can only give zero values. Why not nil?

gopherbot pushed a commit to golang/tools that referenced this issue Jun 25, 2020
@joshbaum @stamblerre
[gopls-release-branch.0.4] internal/lsp/analysis/fillstruct: correct …
0e13de1 
…pointer to builtin values

The current implementation correctly calls 'new' when filling a
pointer to a builtin type.

Fixes: golang/go#39854
Change-Id: I0c2b27bb57fd865c4376279059ad060608d48ba3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/239978
Run-TryBot: Josh Baum <joshbaum@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 727c06e)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/239979
Reviewed-by: Josh Baum <joshbaum@google.com>
@leitzler
Copy link
Contributor Author

@heschik probably not useful but acts more consistent with pointer-to-struct for example. And gives the user a direct hint that just nil would hide. I really have no strong opinion except that it shouldn't produce invalid code at least :)

@golang golang locked and limited conversation to collaborators Jun 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@leitzler @gopherbot @heschi