fmt: padding flag is not accepted with minus flag

[mitar]

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?

I am trying to use a custom Format method on my struct to control how it is formatted using fmt standard package. I am formatting stack traces for an error struct I have in my errors package. I wanted to support both 0 and - flags at the same time with custom meaning for stack tracing formatting and error unwrapping.

So I tried %0+v format string.

What did you expect to see?

That true is returned from both s.Flag('0') and s.Flag('-').

What did you see instead?

Only s.Flag('-') returns true.

Discussion

So this is similar to #51419. And I think decision there is fine. But the issue is that it seems disabling padding flag when also minus flag is provided is done too early in the process so that even custom Format method does not get both flags, but for the custom method providing both flags at the same time might have sense.

So I think both flags should be parsed out and passed on to Format, but then standard implementation for string, byte slices and byte arrays should ignore padding flag if minus flag is provided. Like I could decide to do the same inside Format method.

[mknyszek]

CC @robpike @martisch via https://dev.golang.org/owners

At a quick glance, if the defaults don't understand both of these flags at the same time, I would be somewhat surprised if they could both be passed on to the custom formatter. In a sense it's part of the printf string format now that using both together only gets you one. But I'll let Rob and/or Martin chime in.

[robpike]

This is not straightforward to do because, at least in the existing code, '0' and '-' cannot be set at the same time because of how padding works. In fact, this snippet tells the story:

			case '0':
				p.fmt.zero = !p.fmt.minus // Only allow zero padding to the left.

I believe the work it would take to make this behave as requested is disproportionate to the general utility, and would like to not do it, marking it "Unfortunate".

[mitar]

From me looking at the code the change necessary seems to be trivial because the code is so well written and organized:

diff --git a/src/fmt/format.go b/src/fmt/format.go
index 617f78f15e..55cfcf67a0 100644
--- a/src/fmt/format.go
+++ b/src/fmt/format.go
@@ -75,7 +75,8 @@ func (f *fmt) writePadding(n int) {
        }
        // Decide which byte the padding should be filled with.
        padByte := byte(' ')
-       if f.zero {
+       // Zero padding is allowed only to the left.
+       if f.zero && !f.minus {
                padByte = byte('0')
        }
        // Fill padding with padByte.
@@ -223,7 +224,7 @@ func (f *fmt) fmtInteger(u uint64, base int, isSigned bool, verb rune, digits st
                        f.zero = oldZero
                        return
                }
-       } else if f.zero && f.widPresent {
+       } else if f.zero && !f.minus && f.widPresent { // Zero padding is allowed only to the left.
                prec = f.wid
                if negative || f.plus || f.space {
                        prec-- // leave room for sign
@@ -580,7 +581,8 @@ func (f *fmt) fmtFloat(v float64, size int, verb rune, prec int) {
        if f.plus || num[0] != '+' {
                // If we're zero padding to the left we want the sign before the leading zeros.
                // Achieve this by writing the sign out and then padding the unsigned number.
-               if f.zero && f.widPresent && f.wid > len(num) {
+               // Zero padding is allowed only to the left.
+               if f.zero && !f.minus && f.widPresent && f.wid > len(num) {
                        f.buf.writeByte(num[0])
                        f.writePadding(f.wid - len(num))
                        f.buf.write(num[1:])
diff --git a/src/fmt/print.go b/src/fmt/print.go
index 9c3bd3efec..2997f78c61 100644
--- a/src/fmt/print.go
+++ b/src/fmt/print.go
@@ -1050,12 +1050,11 @@ formatLoop:
                        case '#':
                                p.fmt.sharp = true
                        case '0':
-                               p.fmt.zero = !p.fmt.minus // Only allow zero padding to the left.
+                               p.fmt.zero = true
                        case '+':
                                p.fmt.plus = true
                        case '-':
                                p.fmt.minus = true
-                               p.fmt.zero = false // Do not pad with zeros to the right.
                        case ' ':
                                p.fmt.space = true
                        default:

So just moving the check from the point where it is set to where it is used.

Am I missing something? Tests pass. FormatString changes now if you pass both, but that is probably a good thing for the general case this issue is about.

[robpike]

OK, I buy that. Nicely done. Please send a CL.

[mitar]

Done #61836.

[gopherbot]

Change https://go.dev/cl/516975 mentions this issue: fmt: allow padding and minus flags at the same time