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

Jump to bottom

bytes: TrimSpace returns non-nil byte slice when given all-space input #31038

Closed
andybons opened this issue Mar 25, 2019 · 9 comments
Closed

bytes: TrimSpace returns non-nil byte slice when given all-space input #31038

andybons opened this issue Mar 25, 2019 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.13

Comments

@andybons
Copy link
Member

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

$ go version
go version devel +501632339f Mon Mar 25 18:16:51 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No. This is a regression from go1.12.1 and earlier.

package main

import (
	"bytes"
	"fmt"
)

func main() {
	fmt.Println(bytes.TrimSpace([]byte("   ")) == nil)
}

On tip (1.13) it outputs false while on previous versions it prints true. This was caused by https://golang.org/cl/152917 because it no longer uses bytes.TrimFunc(s, unicode.IsSpace) which calls bytes.TrimLeftFunc on an empty string, causing it to return nil due to indexFunc returning -1:

// TrimFunc returns a subslice of s by slicing off all leading and trailing
// UTF-8-encoded code points c that satisfy f(c).
func TrimFunc(s []byte, f func(r rune) bool) []byte {
	return TrimRightFunc(TrimLeftFunc(s, f), f)
}

// TrimLeftFunc treats s as UTF-8-encoded bytes and returns a subslice of s by slicing off
// all leading UTF-8-encoded code points c that satisfy f(c).
func TrimLeftFunc(s []byte, f func(r rune) bool) []byte {
	i := indexFunc(s, f, false)
	if i == -1 {
		return nil
	}
	return s[i:]
}

// TrimRightFunc returns a subslice of s by slicing off all trailing
// UTF-8-encoded code points c that satisfy f(c).
func TrimRightFunc(s []byte, f func(r rune) bool) []byte {
	i := lastIndexFunc(s, f, false)
	if i >= 0 && s[i] >= utf8.RuneSelf {
		_, wid := utf8.DecodeRune(s[i:])
		i += wid
	} else {
		i++
	}
	return s[0:i]
}

It seems weird that TrimRightFunc returns a non-nil slice every time, while TrimLeftFunc returns a nil slice if it can't find a rune that satisfies f(c), but I imagine changing that behavior would cause more breakage in weird places.

@benhoyt @josharian
@andybons andybons added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Mar 25, 2019
@andybons andybons added this to the Go1.13 milestone Mar 25, 2019
@FiloSottile
Copy link
Contributor

This is unfortunate, because "returns a subslice of s" to me suggests it returns an empty slice, not nil, on an all-space input. (Might not be worth breaking, though.)

@josharian
Copy link
Contributor

This is unfortunate, indeed, but we should try to preserve the existing behavior. From a glance, looks like we just need to replace return s[start:stop] with r := s[start:stop]; if len(r) == 0 { r = nil }; return r. @benhoyt are you interested in following up on this? I.e. investigate more deeply, write some tests, and send a CL?

@benhoyt
Copy link
Contributor

benhoyt commented Mar 25, 2019

@josharian Yes, I'm interested in following up. What timeline are we looking at? I can probably do this in the next 7-10 days.

Are we definitely sure we want to preserve backwards compatibility here, and the weirdness? Or move to always returning a non-nil slice?

@josharian
Copy link
Contributor

Sooner is better of course, but we have at least a month worst case. If it is remotely reasonable, let’s remain perfectly backwards compatible, weird or no.

@andybons
Copy link
Member Author

Spoke with @ianlancetaylor about this and while it is odd behavior and people shouldn't be relying on these functions returning a nil slice in practice, it's not worth the breakage of code that does.

@gopherbot
Copy link

Change https://golang.org/cl/169518 mentions this issue: bytes: Fix regression so TrimSpace again returns nil on all-space input

@benhoyt
Copy link
Contributor

benhoyt commented Mar 27, 2019

Submitted. While I was in there, I noticed there were no tests for TrimLeftFunc or TrimRightFunc (though there are a couple of examples for each). It seems like there should be tests for these, including the nil-returning case, so if you folks agree I can submit another CL for that (should I open a new issue as well?).

@ianlancetaylor
Copy link
Contributor

Some tests would be nice, yes, in a separate CL. No need to open an issue. Thanks.

@gopherbot
Copy link

Change https://golang.org/cl/170061 mentions this issue: bytes, strings: add tests for TrimLeftFunc and TrimRightFunc

gopherbot pushed a commit that referenced this issue Mar 29, 2019
@benhoyt @ianlancetaylor
bytes, strings: add tests for TrimLeftFunc and TrimRightFunc
154e5ab 
When I was working on the fix for #31038 (make TrimSpace return nil on
all-space input) I noticed that there were no tests for TrimLeftFunc
and TrimRightFunc, including the funky nil behavior. So add some!

I've just reused the existing TrimFunc test cases for TrimLeftFunc and
TrimRightFunc, as well as adding new tests for the empty string and
all-trimmed cases (which test the nil-returning behavior of TrimFunc and
TrimLeftFunc).

Change-Id: Ib580d4364e9b3c91350305f9d9873080d7862904
Reviewed-on: https://go-review.googlesource.com/c/go/+/170061
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
6 participants
@josharian @andybons @benhoyt @FiloSottile @ianlancetaylor @gopherbot