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

spec: review/clarify uses of "slice of bytes" #23814

Closed
griesemer opened this issue Feb 13, 2018 · 18 comments
Closed

spec: review/clarify uses of "slice of bytes" #23814

griesemer opened this issue Feb 13, 2018 · 18 comments

Comments

@griesemer
Copy link
Contributor

This is a follow-up issue for #23536: Comb through documentation (spec, builtins) to verify the use of the term "slice of bytes". The spec appears to mean by this any type whose underlying type is []byte. But not all implementations agree.

@mdempsky
Copy link
Contributor

mdempsky commented Sep 27, 2019

I just ran into this issue. I think it's valuable to at least come to a shared agreement on what we think the right behavior is, so we can start aligning the implementations.

I think the spec cases relevant here are:

  1. Conversions: string -> []byte, string -> []rune, []byte -> string, []rune -> string
  2. copy([]byte, string)
  3. append([]byte, string...)

My 2c is that the spec should allow this compilation unit:

package p

type (
	String string
	Byte   byte
	Bytes  []Byte
	Rune   rune
	Runes  []Rune
)

var (
	s  String
	bs Bytes
	rs Runes
)

func f() {
	bs = Bytes(s)
	rs = Runes(s)

	s = String(bs)
	s = String(rs)

	copy(bs, s)
	bs = append(bs, s...)
}

That is, I think we should liberally interpret "slice of bytes" and "slice of runes" to mean "slice type with element type of underlying type byte/rune". I.e., defined slice types and (user-)defined element type should be allowed.

gccgo accepts this.

cmd/compile and go/types currently reject the copy call, claiming that Bytes's element type (Byte) is different from byte.

go/types currently additionally rejects the append call too.

(Also notably, cmd/compile's typechecker allows String(Bytes(nil)) and String(Runes(nil)), but then walk.go fails to generate the appropriate conversions when calling runtime.slicebytetostring and runtime.slicerunetostring, so compilation fails. It has a similar problem with the append call too.)

@mdempsky
Copy link
Contributor

/cc @griesemer @ianlancetaylor

@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 22, 2019
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Oct 22, 2019
@griesemer griesemer modified the milestones: Unplanned, Backlog Oct 22, 2019
@griesemer
Copy link
Contributor Author

@rsc, @ianlancetaylor, @robpike Do you have any opinions here?

We're inconsistent at the moment and different implementations (cmd/compile, gccgo, go/types) differ for operations where this matters.

@bradfitz
Copy link
Contributor

Personally, if there's any ambiguity that needs to be specified, I'd prefer to be a permissive as possible, letting copies/appends/conversions work as expected if the underlying representation is the same.

@ianlancetaylor
Copy link
Contributor

I'm inclined to agree with @mdempsky 's comment #23814 (comment) . I don't see any particular reason to restrict operations on a "slice of bytes" to only the type []byte.

@josharian
Copy link
Contributor

I just rediscovered this in #36965.

Is there sufficient consensus here that I can fix cmd/compile’s implementation to use a maximally liberal interpretation of “byte slices?

Other things to check on once the desired direction is clear: is the spec clear enough? Do go/types, package reflect, cmd/compile, and gccgo agree?

@josharian
Copy link
Contributor

(The reason I’d like to fix cmd/compile now/soon is that I’m touching this code anyway for #36890.)

@griesemer
Copy link
Contributor Author

@josharian The reason for this issue is that not all implementations agree. @mdempsky 's comment explains how they disagree.

I think we should first get the spec to be crystal clear before making changes. It sounds like we're leaning towards making the interpretation more liberal, so this should not break future code, but enable more. This is not urgent, either.

@griesemer
Copy link
Contributor Author

Marking for Go 1.15.

@griesemer griesemer modified the milestones: Backlog, Go1.15 Feb 3, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Go1.16 May 19, 2020
@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Feb 5, 2021
@griesemer
Copy link
Contributor Author

It looks like the consensus here is that "slice of bytes" should be maximally liberal, which would mean any slice of elements whose underlying type is bytes.

@griesemer griesemer modified the milestones: Go1.17, Go1.18 Apr 29, 2021
@griesemer griesemer removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 29, 2021
@ianlancetaylor
Copy link
Contributor

@griesemer This issue has milestone 1.18; shall we move to 1.19 or backlog?

@griesemer
Copy link
Contributor Author

This is documentation that we might be able to address. Prefer to leaving open for now.

@griesemer
Copy link
Contributor Author

griesemer commented Mar 10, 2022

Moving to 1.19.

See also: test cases.

@griesemer griesemer modified the milestones: Go1.18, Go1.19 Mar 10, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/412094 mentions this issue: spec: clarify "slice of bytes" and "slice of runes" through examples

@griesemer
Copy link
Contributor Author

The above CL adjusts the spec to interpret slice of bytes/runes in the most liberal way (underlying type of the slices' element types must be byte/rune) for conversions. This matches the current implementation (and closely matches 1.17 - see the CL description).

The behavior for copy/append is (and always was) more strict; also, also no explicit conversion happens in those cases, so the more strict behavior can be justified.

Since this issue is about "slice of bytes/runes", terminology that is only used in the Conversions section of the spec, the CL addresses only that aspect.

If we want to change the behavior for append/copy, we probably need to file a mini-proposal.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/412095 mentions this issue: test: add tests for string/[]byte/[]rune conversions

gopherbot pushed a commit that referenced this issue Jun 15, 2022
Matches examples in spec section on string conversions.

For #23814.

Change-Id: I08099c27bfdb98735868266f5a42901321b97b56
Reviewed-on: https://go-review.googlesource.com/c/go/+/412095
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Robert Griesemer <gri@google.com>
@zigo101
Copy link

zigo101 commented Jun 15, 2022

Maybe, the reflect implementation should also be made consistent with this change: #23536 (comment)

@griesemer
Copy link
Contributor Author

@go101 Filed #53523.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants