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

cmd/vet: for test only check format strings for core libraries #22936

Closed
mpvl opened this issue Nov 30, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@mpvl
Copy link
Member

commented Nov 30, 2017

Please answer these questions before submitting your issue. Thanks!

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

tip

What did you do?

$ go test
// file: foo_test.go
package foo

import (
"testing"

"golang.org/x/text/number"
"golang.org/x/text/language"
"golang.org/x/text/message"

)

func TestPrint(t *testing.T) {
p := message.NewPrinter(language.English)
p.Printf("num %f", number.Decimal(1.0))
}

What did you expect to see?

Print "num 1"

What did you see instead?

./foo_test.go:13: Printf format %f has arg number.Decimal(1.0) of wrong type golang.org/x/text/number.Formatter

Package message is mostly plug-in compatible with fmt. However, it has its own implementation of the parser which is more relaxed. In this case, the passed formatter implements an extended version of fmt.Formatter, which is not compatible. It is possible fmt.Format and cast it's State argument to the x/text version, but the more general point is that one cannot generally assume that format string passed to fmt are the same as those passed to other packages.
For go test, the format vet checks should, regrettably, only apply to packages defined in core.

Other examples where x/text may differ:

  • %s to accept numbers/dates to render them in written form.
  • escape flags to print non-localized versions
  • allows extra arguments (may be needed to pass gender value, for example). Could be circumvented using explicit position arguments.

See Issue #18085.

@mpvl mpvl changed the title cmd/vet: printf cmd/vet: for test only check format strings for core libraries Nov 30, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Dec 13, 2017

Change https://golang.org/cl/83838 mentions this issue: cmd/vet: limit printf check to known Printf-like functions

@gopherbot

This comment has been minimized.

Copy link

commented Dec 13, 2017

Change https://golang.org/cl/83837 mentions this issue: cmd/go: vet support for upcoming cmd/vet fixes

gopherbot pushed a commit that referenced this issue Dec 14, 2017

cmd/go: vet support for upcoming cmd/vet fixes
Two minor changes to allow fixes in cmd/vet's printf checking.

1. Pass package import path in vet config, so that vet knows
whether it is, for example, vetting "fmt".

2. Add new, but undocumented and for now unsupported
flag -vettool to control which vet binary is invoked during go vet.
This lets the cmd/vet tests build and test a throwaway vet.exe
using cmd/go to ensure type checking information, all without
installing a potentially buggy cmd/vet.

For #22936.

Change-Id: I18df7c796ebc711361c847c63eb3ee17fb041ff7
Reviewed-on: https://go-review.googlesource.com/83837
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@gopherbot gopherbot closed this in 558eeb2 Dec 14, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Jul 19, 2018

Change https://golang.org/cl/125038 mentions this issue: cmd/vet: implement old TODO from testdata/print.go

gopherbot pushed a commit that referenced this issue Aug 21, 2018

cmd/vet: implement old TODO from testdata/print.go
The code was fixed in CL 108559 but the testing TODO was not implemented.

Updates #22936

Change-Id: I20a703260a181bbcf5f87609d6fb8221a182be1a
Reviewed-on: https://go-review.googlesource.com/125038
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
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.