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

Closed
dotaheor opened this issue Jan 24, 2018 · 27 comments
Closed

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

dotaheor opened this issue Jan 24, 2018 · 27 comments
Assignees
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@dotaheor
Copy link

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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

@ianlancetaylor
Copy link
Contributor

I see, I think.

@dotaheor
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. and removed Documentation labels Feb 13, 2018
@griesemer
Copy link
Contributor

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

@ianlancetaylor
Copy link
Contributor

gccgo seems to accept this code without error.

@griesemer
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

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

@dotaheor
Copy link
Author

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
Copy link
Author

dotaheor commented Jan 5, 2022

If looks generic code allows string([]MyByte{}) now, but non-generic code still disallows it.

package main

type MyByte byte

func f[T []MyByte](x T) string {
	return string(x) // okay
}

func main() {
	var y []MyByte
	_ = string(y) // cannot use <node SPTR> (type *MyByte) as type *byte in argument to runtime.slicebytetostring
}

@griesemer griesemer modified the milestones: Backlog, Go1.18 Jan 6, 2022
@griesemer griesemer added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 6, 2022
@griesemer
Copy link
Contributor

Interesting. The code is actually accepted (now?) by the type checker and the error comes later, from the compiler.
We need to address this for 1.18 to avoid an inconsistency.

@danscales would there be an issue to make this last example work in the compiler?

@josharian
Copy link
Contributor

I encountered this in #36965. I implemented it then, and it was very straightforward.

@gopherbot
Copy link

Change https://golang.org/cl/376056 mentions this issue: cmd/compile: fix conv of slice of user-define byte type to string

@danscales danscales assigned danscales and unassigned griesemer Jan 6, 2022
@danscales
Copy link
Contributor

@griesemer : yes, I think we can fix this. I have change uploaded that seems to work well, but will see what the feedback (from Keith/Matthew) is on my fix.

@dotaheor
Copy link
Author

dotaheor commented Jan 8, 2022

There are two remaining problems of this issue:

  1. reflect needs some adjustments.

The following program should prints two true.

package main

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
}
  1. append and copy implementations need some adjustments
package main

func main() {
	type MyByte byte
	var mybs []MyByte
	var str = "abc"

	
	// gc denies the two lines
	copy(mybs, str)
	mybs = append(mybs, str...)
}

And the two issues could be closed if all are done.

@danscales
Copy link
Contributor

OK, maybe we should leave this issue open until the reflect and append/copy issues are addressed. By the way #23813 is essentially fixed, since there is no error or error message any more.

@danscales danscales reopened this Jan 8, 2022
@danscales danscales removed their assignment Jan 8, 2022
@griesemer
Copy link
Contributor

Yes, let's leave this open. It's one thing to accept different types in a conversion, but a different thing if we permit changing types in what amounts to assignments (via append and copy). At any rate, we should be making these changes very deliberately, and not in a rush.

@griesemer griesemer self-assigned this Jan 8, 2022
@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 8, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 8, 2022
@ianlancetaylor
Copy link
Contributor

Changing milestone to 1.19.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.18, Go1.19 Jan 28, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
types2 allows the conversion of a slice of a user-defined byte type B
(not builtin uint8 or byte) to string. But runtime.slicebytetostring
requires a []byte argument, so add in a CONVNOP from []B to []byte if
needed. Same for the conversion of a slice of user-defined rune types to
string.

I made the same change in the transformations of the old typechecker, so
as to keep tcConv() and transformConv() in sync. That fixes the bug for
-G=0 mode as well.

Fixes golang#23536

Change-Id: Ic79364427f27489187f3f8015bdfbf0769a70d69
Reviewed-on: https://go-review.googlesource.com/c/go/+/376056
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@griesemer
Copy link
Contributor

Conversions between string types and slice of bytes types are now maximally liberal and simply require an underlying string type or an underlying byte type for "bytes slices". This is now also expressed in the spec.

append and copy require the types of the arguments to have a core type (or underlying type) of []byte or string respectively. This is also expressed in the spec.

That is, append and copy are less flexible than conversions but that seems ok: both of them are copying (= assigning) elements of slices or strings and we don't generally allow mixed types in such situations elsewhere in the spec unless we go from unnamed to named or vice versa, but here we are copying between named (element) types.

With respect to adjusting the reflect package, I filed #53523.

Closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Development

No branches or pull requests

8 participants