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
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 23, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jul 23, 2018
@gopherbot
Copy link

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 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 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 pushed 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)
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 11, 2018
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants