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

go/types: incorrect behavior for string conversions of byte slices #23536

Open
dotaheor opened this issue Jan 24, 2018 · 17 comments
Open

go/types: incorrect behavior for string conversions of byte slices #23536

dotaheor opened this issue Jan 24, 2018 · 17 comments
Assignees
Labels
Milestone

Comments

@dotaheor
Copy link

@dotaheor dotaheor commented Jan 24, 2018

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

go version go1.9.2 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

It looks the definitions for slice of bytes are not consistent between spec and the builtin docs.

In spec

A non-constant value x can be converted to type T in any of these cases: 
...
* x is a string and T is a slice of bytes or runes. 

In he builtin docs

The copy built-in function .... 
(As a special case, it also will copy bytes from a string to a slice of bytes.)

However:

package main

type T byte

func main() {
	var x []T
	str := "abc"
	x = []T(str) // okay
	copy(x, str) // arguments to copy have different element types: []T and string
	_ = append(x, str...) // cannot use <node SPTR> (type *uint8) as type *T in argument to runtime.memmove
}

[edit] It looks []T is treated as a slice of bytes in x = []T(str), but not in the copy and append calls.

What did you expect to see?

all fail to compile or all compile okay.

What did you see instead?

non-consistent behavior

@dotaheor

This comment has been minimized.

Copy link
Author

@dotaheor dotaheor commented Jan 24, 2018

And this is more weird (non-consistent in spec itself).

A non-constant value x can be converted to type T in any of these cases: 
...
* x is an integer or a slice of bytes or runes and T is a string type.
* x is a string and T is a slice of bytes or runes. 

.

package main

type T byte // same problem for rune

func main() {
	var x []T
	str := "abc"
	x = []T(str) // okay
	str = string(x) // cannot use x (type []T) as type []byte in argument to runtime.slicebytetostring
}

Or this is a compile bug?
It looks gc sometimes view []T as a byte slice type, sometimes not.

[edit]
So we need clarify which of the following two definitions for "slice of bytes" should be used.

  1. a slice whose underlying type is []byte.
  2. a slice whose element underlying type is byte.

If the first definition is adopted, then x = []T(str) shouldn't compile okay.
If the second definition is adopted, then all examples in this thread should compile okay.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 24, 2018

You say there is an inconsistency, but then you cite a section of the spec on explicit type conversions, and a section of the spec on the copy builtin. Those are two different things. The copy builtin does not do type conversions. As your examples show.

Closing because I see no bug here. I strongly encourage you to discuss these cases on golang-nuts before opening issues for them. Thanks.

@dotaheor

This comment has been minimized.

Copy link
Author

@dotaheor dotaheor commented Jan 25, 2018

It is not an issue about conversions. It is about the definition of "slice of bytes". Different official Go docs use different definitions.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 25, 2018

I see, I think.

@dotaheor

This comment has been minimized.

Copy link
Author

@dotaheor dotaheor commented Feb 13, 2018

It looks gccgo thinks a byte slice is a slice whose element underlying type is byte.
For most cases, gc think a byte slice is a slice whose underlying type is []byte.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 13, 2018

There should be no difference at all between gccgo and gc regarding byte slices. I don't see any mention of gccgo above; can you expand on what you mean?

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 13, 2018

I think @dotaheor is on to something. Here's a slightly modified example based on the example above:

package main

func main() {
	x1 := []byte("foo")
	_ = string(x1)

	type T byte
	x2 := []T("foo")
	_ = string(x2)
}

go/types accepts this code w/o complaints. But gc complains with:

x.go:9:12: cannot use x2 (type []T) as type []byte in argument to runtime.slicebytetostring

So at the very least we have an inconsistency. Furthermore, if gc is correct (which I need to investigate), the error message is not very good: There's no mention of runtime in this code, so the error message shouldn't mention it either.

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 13, 2018

More cases:

	x1 := []byte("foo")
	_ = string(x1)

	type T byte
	x2 := []T("foo")
	_ = string(x2)

	type S1 []byte
	x3 := S1("foo")
	_ = string(x3)

	type S2 []T
	x4 := S2("foo")
	_ = string(x4)

go/types accepts all of them. gc complains twice:

x.go:9:12: cannot use x2 (type []T) as type []byte in argument to runtime.slicebytetostring
x.go:17:12: cannot use x4 (type S2) as type []byte in argument to runtime.slicebytetostring

It looks to me that the spec is pretty clear with specific examples (https://tip.golang.org/ref/spec#Conversions). It does appear that gc is correct: A "slice of bytes" means any type whose underlying type is []byte in this case. go/types on the other hand accepts any slice type where the slice element type's underlying type is a byte, which seems incorrect according to the spec. (Whether the spec rule is too tight is a different question).

@griesemer griesemer changed the title docs: non-consistent behavior for byte slices go/types: incorrect behavior for string conversions of byte slices Feb 13, 2018
@griesemer griesemer added NeedsFix and removed Documentation labels Feb 13, 2018
@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 13, 2018

I filed separate #23813 for a better gc error message.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 13, 2018

gccgo seems to accept this code without error.

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 13, 2018

@ianlancetaylor The spec's examples (see 2. in https://tip.golang.org/ref/spec#Conversions) only shows examples with types with underlying type []byte, but one could also argue that perhaps examples are missing. It boils down to what exactly we mean with "slice of bytes". I filed #23814 for that.

@dotaheor

This comment has been minimized.

Copy link
Author

@dotaheor dotaheor commented Feb 14, 2018

Maybe not bad, the current gccgo implementation provides an inefficient way to convert values between []byte and []MyByte.

package main

// []byte -> []MyByte. Compiles ok for both gccgo and gc
func f1() {
	type MyByte byte
	bs := []byte{65, 66, 67}
	ts := []MyByte(string(bs))
	_ = ts
}

// []MyByte -> []byte. Only compile ok for gccgo
func f2() {
	type MyByte byte
	ts := []MyByte{65, 66, 67}
	bs := []byte(string(ts))
	_ = bs
}

func main(){}
@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 14, 2018

@dotaheor So does go/types. However, the spec does not explicitly permit this, it appears.

@dotaheor

This comment has been minimized.

Copy link
Author

@dotaheor dotaheor commented May 5, 2018

It is interesting that both gc and gccgo reflections think []MyByte and string are not ConvertibleTo each other.
So gccgo reflection implementation is inconsistent with the general routine.
And gc reflection implementation is half-inconsistent with the general routine.


import "reflect"
import "fmt"

type T byte // same problem for rune

func main() {
	var x []T
	str := "abc"
	typA := reflect.TypeOf(str)
	typB := reflect.TypeOf(x)
	fmt.Println(typA.ConvertibleTo(typB)) // false
	fmt.Println(typB.ConvertibleTo(typA)) // false
}
@dotaheor

This comment has been minimized.

Copy link
Author

@dotaheor dotaheor commented Apr 16, 2019

As Go spec mentions:

As a special case, append also accepts a first argument assignable to type []byte with a second argument of string type followed by .... This form appends the bytes of the string.

and

As a special case, copy also accepts a destination argument assignable to type []byte with a source argument of a string type. This form copies the bytes from the string into the byte slice.

The two places both clearly specify that the first argument must be of type []byte to form special cases.
So the following example clearly shows that gccgo violates the rules.

package main

func main() {
	type MyByte byte
	var bs []byte
	var mybs []MyByte
	var str = "abc"
	
	// gc and gccgo both accept
	copy(bs, str)
	bs = append(bs, str...)
	
	// gc denies, gccgo accepts
	copy(mybs, str)
	mybs = append(mybs, str...)
}
@griesemer griesemer modified the milestones: Go1.13, Go1.14 May 7, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot gopherbot removed the NeedsFix label Oct 22, 2019
@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 22, 2019

@mdempsky Do you have any thoughts on this? (no rush)

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

@griesemer Yeah, I added some notes/comments about this issue just a few weeks ago at #23814 (comment).

TL;DR: I think we should be liberal in interpreting "slice of bytes" and "slice of runes" to mean any slice type whose element type has underlying type byte or rune (respectively).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.