Skip to content

reflect: Append is inconsistent with the builtin append #39359

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

Closed
zigo101 opened this issue Jun 2, 2020 · 4 comments
Closed

reflect: Append is inconsistent with the builtin append #39359

zigo101 opened this issue Jun 2, 2020 · 4 comments

Comments

@zigo101
Copy link

zigo101 commented Jun 2, 2020

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

$ go version
go version go1.14.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import "reflect"

func main() {
	a := make([]int, 2)
	a = append(a, 1, 2, 3)
	println(len(a), cap(a)) // 5 6
	b := make([]int, 2)
	b = reflect.Append(
		reflect.ValueOf(b),
		reflect.ValueOf(1),
		reflect.ValueOf(2),
		reflect.ValueOf(3),
	).Interface().([]int)
	println(len(b), cap(b)) // 5 8
	c := make([]int, 2)
	c = reflect.AppendSlice(
		reflect.ValueOf(c),
		reflect.ValueOf([]int{1, 2, 3}),
	).Interface().([]int)
	println(len(c), cap(c)) // 5 8
}

What did you expect to see?

Consistent results.

What did you see instead?

Inconsistent results.

Not a big problem. But it would be better if they are consistent.

@zigo101 zigo101 changed the title reflect: Append is isconsistent with the builtin append reflect: Append is inconsistent with the builtin append Jun 2, 2020
@martisch
Copy link
Contributor

martisch commented Jun 2, 2020

To what capacity appends grow slices is not defined in the language spec and could even change from release to release. Therefore its good to not depend on the actual implementation of how much they grow. Is there a specific reason why they should be kept consistent?

That said I wouldnt be surprised if some tests/code breaks having made assumptions as how far reflect.Append grows slices so this will add some churn when changed.

@zigo101
Copy link
Author

zigo101 commented Jun 2, 2020

I agree that it is not a bug. The weak reason to keep them consistent is reflection should try to be consistent with non-reflection.

Is it possible that reflection also could call the runtime slice-grow implementation?

@randall77
Copy link
Contributor

Something like this happens in a couple places, not just with reflect. For example:

package main

func f(s string) {
	a := []byte(s)
	b := []byte(s)
	println(cap(a), cap(b))
	sink = b
}

func main() {
	f("foo")
}

var sink []byte

Produces:

32 8

Because one is a stack allocation and one is a heap allocation.

I don't think there's any value in being consistent. At least, there is value in intentionally allowing ourselves to be inconsistent.

@zigo101
Copy link
Author

zigo101 commented Jun 2, 2020

Yes, I agree it is not a big problem, or even a problem.

@golang golang locked and limited conversation to collaborators Jun 2, 2021
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

4 participants