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: Printf flag '0' is not ignored for strings #56486

Closed
yejianfengblue opened this issue Oct 30, 2022 · 9 comments
Closed

fmt: Printf flag '0' is not ignored for strings #56486

yejianfengblue opened this issue Oct 30, 2022 · 9 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@yejianfengblue
Copy link

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

$ go version
go version go1.19.2 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/k/.cache/go-build"
GOENV="/home/k/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/k/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/k/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build536070510=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://go.dev/play/p/thI6FNjWrY_G

package main

import "fmt"

func main() {
    fmt.Printf("%05s", "abc")
    // print 00abc
}

What did you expect to see?

abc

According to the doc, the flag '0' is ignored for strings.

'0' pad with leading zeros rather than spaces; for numbers, this moves the padding after the sign; ignored for strings, byte slices and byte arrays

What did you see instead?

00abc

Background

I originally asked this question in Stackoverflow. Pak Uula helps check the source code and this issue seems to be a bug.

The source code resets the zero flag only for - (minus) flag. It is not modified neither for strings nor for any other type.
And the function that outputs a string doesn't reset the zero flag either.

@seankhliao seankhliao changed the title fmt: Printf() flag '0' is not ignored for strings fmt: Printf flag '0' is not ignored for strings Oct 30, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 30, 2022
@seankhliao
Copy link
Member

seems to have been this way for a long time

cc @robpike

@robpike robpike self-assigned this Oct 30, 2022
@seankhliao seankhliao added this to the Unplanned milestone Nov 19, 2022
@robpike
Copy link
Contributor

robpike commented May 13, 2023

It's sometimes ignored. See https://go.dev/play/p/_-73vYP6W4p

It's a bug that it's not always ignored.

@gopherbot
Copy link

Change https://go.dev/cl/555776 mentions this issue: fmt: don't pad strings with zeros

@robpike robpike added release-blocker and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 17, 2024
@robpike
Copy link
Contributor

robpike commented Jan 23, 2024

This is not complete yet. Another change to come: must also protect quoted strings and characters, all of which are lovely demonstrated in the golden test as badly broken. Good work, guy.

@robpike robpike reopened this Jan 23, 2024
@gopherbot
Copy link

Change https://go.dev/cl/558055 mentions this issue: fmt: do not zero-pad strings and runes

@dmitshur
Copy link
Contributor

dmitshur commented Jan 29, 2024

@robpike Note that the release-blocker label applies to the milestone an issue is in, and this issue was in Unplanned, so it had no effect. I've moved it to the Go1.23 milestone since that seems to be the intention.

@dmitshur dmitshur modified the milestones: Unplanned, Go1.23 Jan 29, 2024
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 29, 2024
@gopherbot
Copy link

Change https://go.dev/cl/559197 mentions this issue: fmt: update docs for %03s

@gopherbot
Copy link

Change https://go.dev/cl/559196 mentions this issue: fmt: revert "don't pad strings with zeros"

@rsc
Copy link
Contributor

rsc commented Jan 29, 2024

CL 555776 broke a bunch of code. Documenting the long-time behavior instead seems like it makes more sense than causing breakage, so I sent CL 559196 (revert) and CL 559197 (document).

gopherbot pushed a commit that referenced this issue Jan 31, 2024
This reverts CL 555776 (commit 704401f).
Scores of tests break inside Google, and there was a test for the old behavior,
so clearly we thought it was correct at one point.

An example of code that broke inside Google is:

	func (pn ProjectNumber) PaddedHexString() string {
		return fmt.Sprintf("%016s", strconv.FormatInt(int64(pn), 16))
	}

Here is another example:

	// IPv4toISO create ISO address base on a given IPv4 address.
	func IPv4toISO(v4 string) (string, error) {
		if net.ParseIP(v4).To4() == nil {
			return "", fmt.Errorf("invalid IPv4 address")
		}
		s := strings.Split(v4, ".")
		var ss string
		for _, n := range s {
			ss = ss + fmt.Sprintf("%03s", n)
		}
		if len(ss) != 12 {
			return "", fmt.Errorf("invalid IPv4 address")
		}
		return fmt.Sprint("49.0001." + ss[0:4] + "." + ss[4:8] + "." + ss[8:12] + ".00"), nil
	}

This is doing the weird but apparently standard conversion from
IPv4 to ISO ISIS Area 1 (see for example [1]).

Here is an example from github.com/netbirdio/netbird:

	func generateNewToken() (string, string, error) {
		secret, err := b.Random(PATSecretLength)
		if err != nil {
			return "", "", err
		}

		checksum := crc32.ChecksumIEEE([]byte(secret))
		encodedChecksum := base62.Encode(checksum)
		paddedChecksum := fmt.Sprintf("%06s", encodedChecksum)
		plainToken := PATPrefix + secret + paddedChecksum
		hashedToken := sha256.Sum256([]byte(plainToken))
		encodedHashedToken := b64.StdEncoding.EncodeToString(hashedToken[:])
		return encodedHashedToken, plainToken, nil
	}

base62.Encode returns a string no leading zeros; the %06s adds leading zeros.

Are there other ways to write these examples? Yes.
Has all this code worked until now? Also yes.

The change to this behavior observed that right padding doesn't
add zeros, only left padding, but that makes sense: in numbers
without decimal points, zeros on the left preserve the value
while zeros on the right change it.

Since we agree that this case is probably not important either way,
preserve the long-time behavior of %0s.

Will document it in a followup CL: this is a clean revert.

Reopen #56486.

[1] https://community.cisco.com/t5/routing/isis-net-address-configuration/m-p/1338984/highlight/true#M127827

Change-Id: Ie7dd35227f46933ccc9bfa1eac5fa8608f6d1918
Reviewed-on: https://go-review.googlesource.com/c/go/+/559196
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Rob Pike <r@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
It's what the documentation says, and oddly it already behaves correctly
for right padding, not left. (We never pad with zeros on the right.)

Just don't do it.

Fixes golang#56486

Change-Id: I2465edea93c69084e33bee0d945d5a1b85e6cd14
Reviewed-on: https://go-review.googlesource.com/c/go/+/555776
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Rob Pike <r@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
This reverts CL 555776 (commit 704401f).
Scores of tests break inside Google, and there was a test for the old behavior,
so clearly we thought it was correct at one point.

An example of code that broke inside Google is:

	func (pn ProjectNumber) PaddedHexString() string {
		return fmt.Sprintf("%016s", strconv.FormatInt(int64(pn), 16))
	}

Here is another example:

	// IPv4toISO create ISO address base on a given IPv4 address.
	func IPv4toISO(v4 string) (string, error) {
		if net.ParseIP(v4).To4() == nil {
			return "", fmt.Errorf("invalid IPv4 address")
		}
		s := strings.Split(v4, ".")
		var ss string
		for _, n := range s {
			ss = ss + fmt.Sprintf("%03s", n)
		}
		if len(ss) != 12 {
			return "", fmt.Errorf("invalid IPv4 address")
		}
		return fmt.Sprint("49.0001." + ss[0:4] + "." + ss[4:8] + "." + ss[8:12] + ".00"), nil
	}

This is doing the weird but apparently standard conversion from
IPv4 to ISO ISIS Area 1 (see for example [1]).

Here is an example from github.com/netbirdio/netbird:

	func generateNewToken() (string, string, error) {
		secret, err := b.Random(PATSecretLength)
		if err != nil {
			return "", "", err
		}

		checksum := crc32.ChecksumIEEE([]byte(secret))
		encodedChecksum := base62.Encode(checksum)
		paddedChecksum := fmt.Sprintf("%06s", encodedChecksum)
		plainToken := PATPrefix + secret + paddedChecksum
		hashedToken := sha256.Sum256([]byte(plainToken))
		encodedHashedToken := b64.StdEncoding.EncodeToString(hashedToken[:])
		return encodedHashedToken, plainToken, nil
	}

base62.Encode returns a string no leading zeros; the %06s adds leading zeros.

Are there other ways to write these examples? Yes.
Has all this code worked until now? Also yes.

The change to this behavior observed that right padding doesn't
add zeros, only left padding, but that makes sense: in numbers
without decimal points, zeros on the left preserve the value
while zeros on the right change it.

Since we agree that this case is probably not important either way,
preserve the long-time behavior of %0s.

Will document it in a followup CL: this is a clean revert.

Reopen golang#56486.

[1] https://community.cisco.com/t5/routing/isis-net-address-configuration/m-p/1338984/highlight/true#M127827

Change-Id: Ie7dd35227f46933ccc9bfa1eac5fa8608f6d1918
Reviewed-on: https://go-review.googlesource.com/c/go/+/559196
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Rob Pike <r@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
%03s zero-pads a string with spaces; always has and now always will.

Fixes golang#56486.

Change-Id: Ia336581ae7db1c3456699e69e14a3071f50c9f2a
Reviewed-on: https://go-review.googlesource.com/c/go/+/559197
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants