x/tools/go/analysis/passes/modernize: remove the fmtappendf modernizer

[cespare]

(I couldn't locate much previous discussion about this analyzer, so if this has been discussed already or if this isn't the best place to bring this up, feel free to close it.)

I tried applying the fmtappendf modernizer on a large codebase. After analyzing the changes, I concluded that it wasn't an obvious improvement overall and that we shouldn't use it. Based on this experience, and the assumption that it is fairly representative of the changes that this modernizer will accomplish on other codebases, I propose that we remove this modernizer. (My comments below also suggest some ways we might make the modernizer a lot smarter instead, but I'm not sure how difficult that might be.)

---

The first and biggest issue that I see is that for more than half of the sites where this modernizer adds fmt.Appendf calls, there is a much better code change to be made by taking into account a slightly larger bit of surrounding context.

A typical one is something like this:

func handleRoute(w http.ResponseWriter, r *http.Request) {
	// ...
	w.Write([]byte(fmt.Sprintf("hello, %s!", name)))
}

fmtappendf turns it into:

w.Write(fmt.Appendf(nil, "hello, %s!", name))

but the better change would be:

fmt.Fprintf(w, "hello, %s!", name)

Another type of change that pops up a fair amount is where the expression is inside an append already:

append(constructByteSlice(), []byte(fmt.Sprintf("format str %d", xyz))...)

fmtappendf turns it into:

append(constructByteSlice(), fmt.Appendf(nil, "format str %d", xyz)...)

but the better version would be:

fmt.Appendf(constructByteSlice(), "format str %d", xyz)

---

The second issue is that it's not clear to me that the change is a Pareto improvement, even in the other cases where the surrounding context doesn't suggest a different, better change. Consider:

k := []byte(fmt.Sprintf("%s:%d", name, id))

which becomes

k := fmt.Appendf(nil, "%s:%d", name, id)

There are pros and cons to this change. Pros:

• It directly constructs a []byte, which is what the user needs.
• It is three characters and one set of parentheses shorter.
• It avoids one allocation.

Cons:

• The code is a hair less obvious (why are we "appending" anything?).
• It is silly to mention nil.

Maybe on balance we can say that the pros outweigh the cons in this simple case, but it's a pretty close call.

I personally wouldn't want this kind of expression to become commonplace. I think that if we start using

fmt.Appendf(nil, ...)

a lot, we should instead add fmt.Bprintf, which would be a much clearer improvement over

[]byte(fmt.Sprintf(...))

/cc @adonovan

[adonovan]

@prattmic had very similar concerns when we first added this modernizer. A common rule for writers is that when the second reviewer points out the same problem, you need to do something about it. So let's do something about it.

Thanks for the very clear report.

[gabyhelp]

Related Issues

• x/tools/go/analysis/passes/modernize: stringsbuilder could suggest `fmt.Fprintf(&str, ...)` #76914 (closed)
• x/tools/go/analysis/passes/modernize: fmtappendf changes nil semantics of []byte conversion #77557
• x/tools/go/analysis/passes/printf: simplify fmt.Sprintf("%s", x) -> x.String() et al #73027 (closed)
• x/tools/gopls/internal/analysis/modernize: after refactoring the code using the `fmtappendf` analyzer, performance may decrease #73666 (closed)
• x/tools/gopls/internal/analysis/modernize: fmtappendf suggests invalid fix #74709 (closed)
• x/tools/gopls/internal/analysis: add "modernizer" analyzers #70815 (closed)
• fmt: add Append, Appendf, Appendln #47579 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}