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

strings: broken backward compatibility in 1.12 #31121

Open
kirillDanshin opened this Issue Mar 28, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@kirillDanshin
Copy link

kirillDanshin commented Mar 28, 2019

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

$ go version
1.12

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
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/apple/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/apple/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/cs/0p7xq1h91795xymyb6h7pbhh0000gn/T/go-build419608424=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Example 1.

go get -v github.com/fasthttp/router
cd $GOPATH/src/github.com/fasthttp/router
go test -v -run TestTreeFindCaseInsensitivePath # fails using go1.12

# switch back to 1.11.x

go test -v -run TestTreeFindCaseInsensitivePath # works

# switch back to 1.12, vendor strings.ToLower and strings.Map from 1.11.x, replace all strings.ToLower with vendored e.g. toLower()

go test -v # works

Example 2: the exactly same steps, but with github.com/gramework/gramework before hotfix
Example 3: or https://github.com/julienschmidt/httprouter
Example 4: or https://github.com/buaazp/fasthttprouter
Example ∞: or any program that relies on lowering UTF-8 characters.

What did you expect to see?

Tests pass on 1.12 without copying two functions from the standard strings package.

What did you see instead?

Broken tests and projects that can be fixed only by vendoring stdlib functions, which is at least strange keeping in mind the backward compatibility promise.

@kirillDanshin

This comment has been minimized.

Copy link
Author

kirillDanshin commented Mar 28, 2019

Update: the bug seems to be introduced by https://go-review.googlesource.com/c/go/+/131495

@ALTree ALTree added this to the Go1.13 milestone Mar 28, 2019

@ALTree

This comment has been minimized.

Copy link
Member

ALTree commented Mar 28, 2019

Thanks for the report. I'm tentatively putting this into the 1.13 milestone, pending investigation.

It would be nice to have a self-contained example that shows how the new behaviour is different from the old, and why this is causing tests breakage.

cc @martisch, author of that CL, in the meantime.

@kirillDanshin

This comment has been minimized.

Copy link
Author

kirillDanshin commented Mar 28, 2019

@ALTree I still trying to figure out what exactly triggers that bug, but no luck for now. anyway, for now only solution for me is to swap toLower implementation with vendored one in the router core using build constraints.

@martisch

This comment has been minimized.

Copy link
Member

martisch commented Mar 28, 2019

The difference is how invalid UTF8 sequences are treated and this is WAI and wont change back. They now will always be converted to RuneRrror runes. Before they were only converted to RuneError runes when there also was another change e.g. upper case character converted to lower case. Which was inconsistent and a bug. The new behaviour is consistent and always converts invalid sequences.

For the noted examples such as https://github.com/julienschmidt/httprouter a bug was filed before go1.12 release that the tests will fail as the package assumes the old behaviour. julienschmidt/httprouter#263

Some of the problems seen with package tests breaking happen when the packages incorrectly assume that they can convert strings byte by byte which the new behaviour converts into RuneErrors for non ASCII characters and this only worked accidentally before when not needing to convert part of the string with invalid sequences. For example: go-openapi/swag#26

@martisch

This comment has been minimized.

Copy link
Member

martisch commented Mar 28, 2019

Of course there can still be a bug in ToLower that converts a string incorrectly. For that it would be good to have an example string that can be passed to ToLower as a minimal reproducer to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.