Go-tools: gosimple: either false positive or hard to understand S1016 advice

[dmitshur]

Either this is a false positive, or the advice gosimple (as part of megacheck) gives here is hard to understand, or I'm making some sort of mistake. I've been seeing this suggestion for weeks and I still haven't figured out how I can make my code better as a result.

I'm not going to make a minimal reproduce case because it might not communicate the issue appropriately. Instead, here are easy steps to reproduce:

$ export GOPATH=/tmp/empty
$ go get -u github.com/shurcooL/home
$ megacheck github.com/shurcooL/home
index.go:294:14: should use type conversion instead of struct literal (S1016)
index.go:365:15: should use type conversion instead of struct literal (S1016)

Those lines are index.go:294 and index.go:365 if you prefer to view them on github.

[dmitshur]

Okay, after reading https://staticcheck.io/docs/gosimple#S1016 and thinking hard, I've figured out what the check is trying to suggest.

It's saying I can use a type conversion on the variable p, which in those two cases is one of https://godoc.org/github.com/shurcooL/events/event#Push or https://godoc.org/github.com/shurcooL/events/event#Gollum:

package event // import "github.com/shurcooL/events/event"

// Push is a push event.
type Push struct {
    Commits []Commit
}

To one of the unexported custom types for rendering HTML in home package:

package main // import "github.com/shurcooL/home"

type commits struct {
    Commits []event.Commit
}

func (d commits) Render() []*html.Node { ... }

Like so:

case event.Push:
    displayEvent = activityEvent{
        basicEvent: &basicEvent,
        Icon:       octiconssvg.GitCommit,
        Action:     component.Text("pushed to"),
-       Details: commits{
-           Commits: p.Commits,
-       },
+       Details: commits(p),
    }

case event.Gollum:
    displayEvent = activityEvent{
        basicEvent: &basicEvent,
        Icon:       octiconssvg.Book,
        Action:     component.Text("edited the wiki in"),
-       Details: &pages{
-           ActorAvatarURL: p.ActorAvatarURL,
-           Pages:          p.Pages,
-       },
+       Details: pages(p),
    }

Note, those types are defined in another package, so that's one of the reasons the advice was hard to understand. It would potentially be a lot more readable if it mentioned the types involved in the type conversion:

should use type conversion from commits to event.Push instead of struct literal

Secondly, I still need to think about whether I want to apply the advice. One of my concerns is that the fact those two structs in separate packages match is somewhat of a coincidence, and it may stop being the case if either of the two unrelated packages changes. So changing github.com/shurcooL/events/event.Push struct (e.g., adding a new field to it) would cause github.com/shurcooL/home, a package it knows nothing about, to fail to compile.

[dmitshur]

Another observation. Note the first literal was a value commits, but the second was a pointer to pages:

Details: commits{ ... },

Details: &pages{ ... },

So to apply this potential code simplification correctly (without changing behavior unintentionally), it should actually be:

Details: commits(p),

Details: (*pages)(&p),

Which is pretty subtle and easy to forget or get wrong since the check doesn't actually tell you what type conversion you should be doing.

This is a point in favor for it telling the user more explicitly what simplification it found.

[dmitshur]

Not to mention taking an address of an existing variable, depending on context, can change behavior dangerously, compared to creating a pointer to a new variable with a struct literal. Something to consider.

[dominikh]

I believe I have addressed all of your concerns. From now on, the suggestion is only made for types that live in the same packages, and never for pointers. I've also improved the message itself.