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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@alandonovan
Copy link
Contributor

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 NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Aug 16, 2018
@andybons andybons added this to the Go1.11 milestone Aug 16, 2018
@andybons
Copy link
Member

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
Copy link
Member

Never mind. Not a release blocker.

@mvdan
Copy link
Member

mvdan 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
Copy link
Member

mvdan 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
Copy link
Contributor Author

alandonovan 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
Copy link
Member

mvdan 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
Copy link
Contributor

rsc 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 Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 18, 2018
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Aug 18, 2018
@rsc
Copy link
Contributor

rsc 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 as completed Aug 18, 2018
@golang golang locked and limited conversation to collaborators Aug 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants