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

fmt: should clearflags also set wid and prec to zero #61913

Closed
mitar opened this issue Aug 9, 2023 · 3 comments
Closed

fmt: should clearflags also set wid and prec to zero #61913

mitar opened this issue Aug 9, 2023 · 3 comments
Assignees
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mitar
Copy link
Contributor

mitar commented Aug 9, 2023

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

$ go version
go version go1.20.7 linux/amd64

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="amd64"
GOBIN=""
GOCACHE="/home/mitar/.cache/go-build"
GOENV="/home/mitar/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/mitar/.go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/mitar/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.7"
GCCGO="/usr/bin/gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3086644443=/tmp/go-build -gno-record-gcc-switches"

What did you do?

So I have implemented fmt.Formatter interface for my own struct and wrote bunch of tests. In my case, formatting makes sense only for precision 0 and 1, so I have such check in my code:

		precision, _ := s.Precision()
		if precision < 0 || precision > 1 {
			io.WriteString(s, badPrecString)
			break
		}

What did you expect to see?

I expected to see that all tests which do not provide precision at all in format strings succeed.

What did you see instead?

I see random failures. After debugging I think the reason is that newPrinter() in fmt package uses a sync.Pool and reuses printers. The code calls p.fmt.clearflags() before for simpleFormat formats just calls printArg which calls handleMethods. Because it is a simple format precision is never parsed nor set (just precPresent is reset). Because I do not check for ok value from s.Precision() but just use precision I think I get precision from some other printer instance when it was set in the past.

I find this surprising. In general my experience in Go was that uninitialized values are always 0. So even if precision has not been set I would expect it always to be 0. This happens only because instances are cached (an optimization).

I would suggest that clearflags simply sets wid and prec fields to zero, too. Maybe clearflags should also just be renamed to clear?

I am not sure how to write a test case or reproduction for this.

@mitar
Copy link
Contributor Author

mitar commented Aug 9, 2023

@robpike
Copy link
Contributor

robpike commented Aug 9, 2023

I suspect you're right and the diagnosis is correct. I'll investigate.

@robpike robpike self-assigned this Aug 9, 2023
@gopherbot
Copy link

Change https://go.dev/cl/556215 mentions this issue: fmt: clear width and precision when recovering formatting object from the pool

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. labels Jan 29, 2024
@dmitshur dmitshur added this to the Go1.23 milestone Jan 29, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
… the pool

Probably a day 1 oversight, and almost always inconsequential, but
there is evidence of occasional trouble. There is no reason not to
clear them.

I tried and failed to write a test to catch this, but the change should
be harmless and is all but certain to fix the problem.

Fixes golang#61913

Change-Id: I0f7bbb4ab2780d8999d3ff7a35255dc07fb5c7e1
Reviewed-on: https://go-review.googlesource.com/c/go/+/556215
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants