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

bytes: ReplaceAll returns nil slice when empty slice is given #46415

Closed
KaitoHH opened this issue May 27, 2021 · 7 comments
Closed

bytes: ReplaceAll returns nil slice when empty slice is given #46415

KaitoHH opened this issue May 27, 2021 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@KaitoHH
Copy link

KaitoHH commented May 27, 2021

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 (
	"bytes"
	"fmt"
)

func main() {
	old := []byte("")
	b := bytes.ReplaceAll(old, []byte("oink"), []byte("moo"))
	fmt.Println(old == nil)
	fmt.Println(b == nil)
}

What did you expect to see?

false
true

the call of bytes.ReplaceAll on an empty slice should also return an empty slice. However, it returns nil

What did you see instead?

false
false
@mvdan
Copy link
Member

mvdan commented May 27, 2021

Can I ask why you want them to be equally nil or not nil? As far as I know, that's not a guarantee of any of Go's APIs by default.

@KaitoHH
Copy link
Author

KaitoHH commented May 27, 2021

Can I ask why you want them to be equally nil or not nil? As far as I know, that's not a guarantee of any of Go's APIs by default.

The inconsistent type of slice (empty slice or nil slice) looks confusing. To me: Empty slice in, empty slice out. Nil slice in, nil slice out.

@mknyszek mknyszek changed the title Unexpected manner of bytes.ReplaceAll bytes: ReplaceAll returns nil slice when empty slice is given May 27, 2021
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 27, 2021
@mknyszek mknyszek added this to the Backlog milestone May 27, 2021
@mknyszek
Copy link
Contributor

FWIW, I think generally the pattern here is to check length instead of nil-ness. The length of a nil slice is also zero.

CC @ianlancetaylor via https://dev.golang.org/owners

@ianlancetaylor
Copy link
Contributor

The bytes package makes no promise to preserve the nil status of an argument slice. The general guideline of Go style is to treat nil slices and empty slices the same, which means to always use len(s) == 0 rather than s == nil. While there are packages that intentionally distinguish an empty slice and a nil slice, those packages are always clearly documented. The bytes package is not one of them.

@novns
Copy link

novns commented Oct 8, 2021

Please fix this issue.

Using bytes.ReplaceAll corrupts empty slices when they are used as arguments of database queries.
It turns empty string arguments '' to NULL values.

Currently additional checks are required for special cases of empty slices and a lot of unnecessary debugging.

@novns
Copy link

novns commented Oct 8, 2021

The documentation clearly says "ReplaceAll returns a copy of the slice..."

If the slice is empty it should return the empty slice as it was exactly before without any changes.
Because it breaks things otherwise, and introduces inconsistency.

@ianlancetaylor
Copy link
Contributor

The byte.Replace function has worked as it does today since the Go 1.0 release, and the bytes.ReplaceAll function has worked this way since it was introduced in the Go 1.12 release. The behavior is not buggy and it matches the documentation. We aren't going to change it now. Sorry.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants