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: printf: enforce 'f' suffix convention for printf wrappers, and only them #27036

Closed
alandonovan opened this issue Aug 16, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@alandonovan
Copy link
Contributor

commented Aug 16, 2018

Vet could flag as problematic any function whose name ends in "f" and whose parameter types end with (string, ...interface{}) but that does not forward the last two parameters to a printf wrapper.

func wrapf(format string, args...interface{}) string { // vet error: wrapf looks like a printf wrapper but does not call a printf wrapper
	fmt.Printf("%s %v", format, args) // this is not a proper forward
}

(False positives are possible for packages that implement fmt-style formatting without using fmt, such as mpvl's localized text support. A workaround would be for such functions to contain a dummy unreachable forwarding call to fmt so that vet treats them as printf wrappers.)

Conversely, if a function has that signature and does forward to a printf wrapper, the function name should end in "f".

func wrap(format string, args...interface{}) string { // vet error: printf wrapper wrap does not end in 'f'
	fmt.Printf(format, args...)
}



@andybons andybons added this to the Go1.11 milestone Aug 16, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

Putting as release blocker for now. Please change if you don't believe it's a problem for 1.11 final and can be done in a point release.

/cc @rsc

@andybons andybons modified the milestones: Go1.11, Unplanned Aug 16, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

Never mind. Not a release blocker.

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

I'm not sure if this would qualify for vet's frequency or precision criteria. Do you have any numbers to support that this is a common occurrence? I do see typos when people use fmt.Println("foo %d", var) and such, but not really when naming print/printf wrapper funcs.

As for precision - I think it would be fairly easy for this check to run into false positives. For example:

func LogRequest(w io.Writer, method string, parameters ...interface{}) {
    fmt.Fprintf(w, "Received %s with parameters %v", method, parameters)
}

I don't think the code above is wrong, and I don't think naming it LogRequestf is valid either.

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Another thought about false positives - perhaps they would go away if this check only ran if the string parameter was named format. func LogRequest(w io.Writer, format string, args ...interface{}) definitely looks buggy to me.

@alandonovan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

Your LogRequest example is not a printf-wrapper, strictly defined, because it doesn't forward the last two parameters. (You need the ... to properly forward the call.) I think the false positive rate will be close to zero.

@mvdan

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Ah - I misunderstood the idea, then. I agree that there shouldn't be any false positives, but I'm still unsure if this happens often enough.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2018

I don't think vet is for enforcing naming conventions.
I'm quite happy that the printf check no longer cares
what the names of wrappers are, and I'd rather not reintroduce that.

@rsc rsc added the NeedsDecision label Aug 18, 2018

@gopherbot gopherbot removed the NeedsFix label Aug 18, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2018

Rereading, I think both the examples are clearly fine as is.
We're certainly not going to start requiring that every possible printf wrapper end its name in f.

@rsc rsc closed this Aug 18, 2018

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.