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.

Sign up for GitHub

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

Jump to bottom

bytes: Trim returns empty slice instead of nil in 1.18 #51793

Closed
nphmuller opened this issue Mar 18, 2022 · 12 comments
Closed

bytes: Trim returns empty slice instead of nil in 1.18 #51793

nphmuller opened this issue Mar 18, 2022 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@nphmuller
Copy link

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 seankhliao changed the title Go 1.18 regression: bytes.Trim() with empty result returns empty string instead of nil bytes: Trim returns empty slice instead of nil in 1.18 Mar 18, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 18, 2022
@seankhliao
Copy link
Member

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

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

dsnet commented Mar 18, 2022

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

@dsnet
Copy link
Member

dsnet commented Mar 18, 2022 •
edited

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

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

@randall77
Copy link
Contributor

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

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

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

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 gopherbot mentioned this issue Mar 18, 2022
Closed
@gopherbot
Copy link

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

@CAFxX
Copy link
Contributor

CAFxX commented Mar 20, 2022 •
edited

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)

@efd6 efd6 mentioned this issue Mar 23, 2022
Closed
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 30, 2022
@ianlancetaylor @heschi
[release-branch.go1.18] bytes: restore old Trim/TrimLeft behavior for…
8b0583a 
… nil

Keep returning nil for the cases where we historically returned nil,
even though this is slightly different for TrimLeft and TrimRight.

For #51793
Fixes #51796

Change-Id: Ifbdfc6b09d52b8e063cfe6341019f9b2eb8b70e9
Reviewed-on: https://go-review.googlesource.com/c/go/+/393876
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
(cherry picked from commit 32fdad1)
Reviewed-on: https://go-review.googlesource.com/c/go/+/396294
Reviewed-by: Austin Clements <austin@google.com>
@golang golang locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@CAFxX @ianlancetaylor @nphmuller @dsnet @randall77 @gopherbot @seankhliao