bytes: Trim returns empty slice instead of nil in 1.18

[nphmuller]

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

go version go1.18 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

❯ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/nphmuller/Library/Caches/go-build"
GOENV="/Users/nphmuller/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/nphmuller/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/nphmuller/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.18"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/nphmuller/Dev/cbgms/backend/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/l5/4r4nnb4j1_79bc7w3swx85980000gn/T/go-build1290554738=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

func TestBytesTrim(t *testing.T) {
	if bytes.Trim([]byte(`""`), `"`) != nil {
		t.Fail()
	}
}

What did you expect to see?

Test should succeed (it succeeds on Go 1.17)

What did you see instead?

Test fails

[seankhliao]

I don't think the API makes any promises about empty vs nil slices.
This looks like CL 332771 for #46446
cc @CAFxX @josharian @dsnet

[nphmuller]

To be fair, returning an empty string sound more logical to me then returning null. This change in logic however broke a code base I work on in at least 2 places. But if it doesn’t count as a regression that’s totally fine too, of course.

[dsnet]

This is not guaranteed by the documented behavior, but I'm inclined to say that this is a regression.

[dsnet]

Oh, it's a change in the opposite direction where the new behavior is that it returns non-nil for a non-nil input. I actually think this behavior is more correct.

[seankhliao]

I agree the new behavior is more correct, though #31038 was a similar issue in the past

[randall77]

I think we should revert to the old behavior. The improvement in behavior is pretty minor and doesn't seem worth breaking existing programs.

[ianlancetaylor]

I don't see anything wrong with the new behavior, but I agree with @randall77 that on balance we should keep the behavior the same. There's not enough benefit to changing.

[ianlancetaylor]

@gopherbot Please open backport issue for 1.18

Assuming we do go ahead and restore the old behavior, as I think we should, we need to backport the fix to 1.18 so that we are consistent across releases.

[gopherbot]

Backport issue(s) opened: #51796 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[gopherbot]

Change https://go.dev/cl/393876 mentions this issue: bytes: restore old Trim/TrimLeft behavior for nil

[CAFxX]

I also think the new behavior was more intuitive and internally consistent, but I'm ok with that partial revert if it's hurting users.

Would just be nice if we had a way to get rid of these paper cuts over time (IIRC at a certain point there was a discussion about using the go version in go.mod for this?), or maybe we could start aggressively randomizing non-guaranteed behavior during testing. (Obviously, this specific instance of the problem is not important enough by itself, but having a structured, documented approach would certainly help in the long run)

[gopherbot]

Change https://go.dev/cl/396294 mentions this issue: [release-branch.go1.18] bytes: restore old Trim/TrimLeft behavior for nil