mdempsky/unconvert

Consider reporting avoidable type conversion for untyped constants (typed constants are already handled).

dmitshur opened this issue · 4 comments

Consider the following Go program:

package main

import "fmt"

func main() {
	const foo = "foo"

	f(string(foo) + "bar")
}

func f(s string) {
	fmt.Println(s)
}

I expected unconvert to report that string(foo) is an unnecessary conversion, but it didn't.

$ unconvert
$ echo $?
0

Is that intentional, or a missing feature?

Tested using Go 1.8beta2 and latest version of unconvert and its depenencies:

$ go version
go version go1.8beta2 darwin/amd64
$ gostatus -v .../unconvert
     github.com/mdempsky/unconvert/...
$ go list -f '{{join .Deps "\n"}}' github.com/mdempsky/unconvert | gostatus -stdin -v
     github.com/kisielk/gotool/...
     golang.org/x/text/...
   $ golang.org/x/tools/...
	$ Stash exists
$ binstale unconvert
unconvert
	up to date: github.com/mdempsky/unconvert

/cc @dominikh in case this is a better fit for one of your tools.

Not sure if this is the same issue, or another but related one...

unconvert is also missing opportunities to point out unnecessary conversion of nil slices:

package main

import (
	"fmt"
)

func main() {
	var s []string
	fmt.Println(s)
	s = []string(nil)
	fmt.Println(s)
}

The conversion of nil to []string is unnecessary, meaning s = []string(nil) could be simplified to just s = nil.

Using go1.9 and latest unconvert and all of its dependencies. /cc @mdempsky

Interesting. So strictly speaking, string("foo") and []string(nil) do not meet the definition of "unnecessary" that unconvert uses: "foo" and nil are an untyped string and untyped nil, respectively, whereas the conversion converts them to string and []string, respectively.

That said, I think there's merit to supporting these cases. Maybe behind a flag.

The second one can probably already be handled by recognizing that s = []string(...) is a "safe" context.

The first case is theoretically just an issue of recognizing another safe context, but it's a bit trickier because of the extra string concatenation.

Very good point about the distinction between typed vs untyped constants. I agree that supporting it would still be helpful, but being able to control this behavior via a flag would also make sense.

I've confirmed, if I change the const in the original program from untyped string to typed string, by doing this:

-const foo = "foo"
+const foo string = "foo"

Or this:

-const foo = "foo"
+const foo = string("foo")

Then unconvert reports the unnecessary conversion on line 8:

$ unconvert
main.go:8:10: unnecessary conversion

I'll rename this issue to more accurately represent this issue, which is now more of a feature request. It's not that unconvert doesn't handle constants, because it does. It's only about untyped constants.

but it's a bit trickier because of the extra string concatenation.

It seems to handle that part fine. I'm not too surprised, given you're using go/types, which figures this stuff out.