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

strings: broken backward compatibility in 1.12 #31121

Closed
kirillDanshin opened this issue Mar 28, 2019 · 6 comments
Closed

strings: broken backward compatibility in 1.12 #31121

kirillDanshin opened this issue Mar 28, 2019 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Go1.13

Comments

@kirillDanshin
Copy link

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

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

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 28, 2019
@ALTree ALTree added this to the Go1.13 milestone Mar 28, 2019
@ALTree
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
Copy link
Author

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

martisch commented Mar 28, 2019 •
edited

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

martisch commented Mar 28, 2019 •
edited

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.

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 29, 2019
@kirillDanshin kirillDanshin mentioned this issue Apr 1, 2019
Open
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@golang golang locked and limited conversation to collaborators Apr 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 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
5 participants
@martisch @ALTree @bcmills @gopherbot @kirillDanshin