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: make printf checking more precise when arguments are changed #26555

Open
ianlancetaylor opened this issue Jul 23, 2018 · 3 comments
Open

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 23, 2018

For #26486 https://golang.org/cl/125039 changed the printf checking to only warn about functions that do not modify the arguments before passing them to fmt.Printf (or whatever). The example in that issue, drawn from real code, is:

func dbg(s string, va ...interface{}) {
	if s == "" {
		s = strings.Repeat("%v ", len(va))
	}
	_, fn, fl, _ := runtime.Caller(1)
	fmt.Printf("dbg %s:%d: ", path.Base(fn), fl)
	fmt.Printf(s, va...)
	fmt.Println()
}

This can be called as dbg("", err).

The fix in CL 125039 means that we no longer issue printf warnings for functions like this:

func Prefix(s string, args ...interface{}) {
    s = "error: " + s
    fmt.Printf(s, args)
}

We should figure out a way to continue issuing warnings for Prefix without issuing them for dbg.

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jul 23, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 23, 2018

Change https://golang.org/cl/125039 mentions this issue: cmd/vet: if a function modifies its args, it's not a print wrapper

gopherbot pushed a commit that referenced this issue Jul 23, 2018
Fixes #26486
Updates #26555

Change-Id: I402137b796e574e9b085ab54290d1b4ef73d3fcc
Reviewed-on: https://go-review.googlesource.com/125039
Reviewed-by: Russ Cox <rsc@golang.org>
@mvdan
Copy link
Member

@mvdan mvdan commented Jul 24, 2018

It seems to me like this would require the use of more advanced tooling in vet, such as the use of x/tools/go/ssa.

@blgm
Copy link

@blgm blgm commented Sep 6, 2018

I've seen an issue with the following code which now fails vet in v1.11:

func Say(expected string, args ...interface{}) *sayMatcher {
	formattedRegexp := expected
	if len(args) > 0 {
		formattedRegexp = fmt.Sprintf(expected, args...)
	}
	return &sayMatcher{
		re: regexp.MustCompile(formattedRegexp),
	}
}

Re-writing like this allows it to pass vet, but this seems like a very fragile "fix".

func Say(expected string, args ...interface{}) *sayMatcher {
	if len(args) > 0 {
		expected = fmt.Sprintf(expected, args...)
	}
	return &sayMatcher{
		re: regexp.MustCompile(expected),
	}
}

Ideally go vet would be able to tell that not all branches are a print wrapper, and ignore functions like this.

blgm added a commit to onsi/gomega that referenced this issue Sep 6, 2018
- go vet in Go v1.11 identifies `Say()` as a print wrapper
  - go vet will then fail for `Say("%")` because this is not a valid
  Sprintf template
- Because of golang/go#26486, changing the way
  that the functon is written will work around the vet issue
- I have added a comment documenting that this is not ideal in
  golang/go#26555 (comment)
nodo added a commit to onsi/gomega that referenced this issue Sep 10, 2018
- go vet in Go v1.11 identifies `Say()` as a print wrapper
  - go vet will then fail for `Say("%")` because this is not a valid
  Sprintf template
- Because of golang/go#26486, changing the way
  that the functon is written will work around the vet issue
- I have added a comment documenting that this is not ideal in
  golang/go#26555 (comment)
@fredbi fredbi mentioned this issue Sep 26, 2018
2 of 2 tasks complete
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.