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

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

Open
dmitshur opened this issue Jan 2, 2017 · 4 comments

Comments

@dmitshur
Copy link
Collaborator

dmitshur commented Jan 2, 2017

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.

@dmitshur
Copy link
Collaborator Author

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

@mdempsky
Copy link
Owner

mdempsky commented Sep 1, 2017

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.

@dmitshur
Copy link
Collaborator Author

dmitshur commented Sep 1, 2017

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.

@dmitshur
Copy link
Collaborator Author

dmitshur commented Sep 1, 2017

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.

@dmitshur dmitshur changed the title Doesn't report unnecessary type conversion for consts. Consider reporting avoidable type conversion for untyped constants (typed constants are already handled). Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants