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: fillstruct return invalid code when using pointers to builtin type #39854

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

Comments

@leitzler
Copy link
Contributor

@leitzler leitzler commented Jun 25, 2020

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 this to the Unreleased milestone Jun 25, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 25, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 25, 2020

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

@heschik
Copy link
Contributor

@heschik heschik 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
…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

@leitzler leitzler commented Jun 25, 2020

@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 :)

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
3 participants
You can’t perform that action at this time.