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.

Sign up for GitHub

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

Jump to bottom

cmd/vet: make printf checking more precise when arguments are changed #26555

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

cmd/vet: make printf checking more precise when arguments are changed #26555

ianlancetaylor opened this issue Jul 23, 2018 · 3 comments
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
Unplanned

Comments

@ianlancetaylor
Copy link
Member

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
Contributor

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
@ianlancetaylor
cmd/vet: if a function modifies its args, it's not a print wrapper
240ae7e 
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 blgm mentioned this issue Sep 4, 2018
Closed
@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
@blgm
Work around go vet issue with Go v1.11
927a333 
- 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)
@blgm blgm mentioned this issue Sep 6, 2018
Merged
nodo pushed a commit to onsi/gomega that referenced this issue Sep 10, 2018
@blgm
Work around go vet issue with Go v1.11 (#300)
40dd6ad 
- 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
Closed
2 tasks
@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 11, 2018
@timothy-king timothy-king mentioned this issue Dec 14, 2022
Closed
@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
Assignees
No one assigned
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
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@ianlancetaylor @mvdan @adonovan @gopherbot @blgm