-
Notifications
You must be signed in to change notification settings - Fork 453
fix(writebytestring): emit string() cast for named string types; add RunWithSuggestedFixes #44208
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package writebytestring | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "io" | ||
| "os" | ||
| ) | ||
|
|
||
| type customWriter struct{} | ||
|
|
||
| func (c *customWriter) Write(p []byte) (int, error) { return len(p), nil } | ||
|
|
||
| func bad() { | ||
| var buf bytes.Buffer | ||
| s := "hello" | ||
| io.WriteString(&buf, s) // want `buf\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(&buf, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` | ||
|
|
||
| io.WriteString(&buf, "world") // want `buf\.Write\(\[\]byte\("world"\)\) can be replaced with io\.WriteString\(&buf, "world"\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` | ||
| } | ||
|
|
||
| type myString string | ||
|
|
||
| func badNamedString() { | ||
| var buf bytes.Buffer | ||
| s := myString("hello") | ||
| io.WriteString(&buf, string(s)) // want `buf\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(&buf, string\(s\)\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` | ||
| } | ||
|
|
||
| func badFile() { | ||
| f, err := os.Create("/tmp/test.txt") | ||
| if err != nil { | ||
| return | ||
| } | ||
| defer f.Close() | ||
| msg := "hello" | ||
| io.WriteString(f, msg) // want `f\.Write\(\[\]byte\(msg\)\) can be replaced with io\.WriteString\(f, msg\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` | ||
| } | ||
|
|
||
| func badCustomWriter() { | ||
| w := &customWriter{} | ||
| s := "hello" | ||
| io.WriteString(w, s) // want `w\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(w, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` | ||
| } | ||
|
|
||
| func badInterfaceWriter(w io.Writer) { | ||
| s := "hello" | ||
| io.WriteString(w, s) // want `w\.Write\(\[\]byte\(s\)\) can be replaced with io\.WriteString\(w, s\) to potentially avoid a \[\]byte allocation if the writer implements io\.StringWriter` | ||
| } | ||
|
|
||
| func goodBytes() { | ||
| var buf bytes.Buffer | ||
| b := []byte("hello") | ||
| buf.Write(b) // already []byte — no conversion | ||
| } | ||
|
|
||
| func goodWriteString() { | ||
| // Using io.WriteString is the idiomatic form. | ||
| var buf bytes.Buffer | ||
| s := "hello" | ||
| buf.WriteString(s) | ||
| } | ||
|
|
||
| func goodNotString() { | ||
| var buf bytes.Buffer | ||
| n := 42 | ||
| buf.Write([]byte{byte(n)}) | ||
| } | ||
|
|
||
| func suppressed() { | ||
| var buf bytes.Buffer | ||
| s := "hello" | ||
| //nolint:writebytestring | ||
| buf.Write([]byte(s)) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,6 +104,14 @@ func run(pass *analysis.Pass) (any, error) { | |
| return | ||
| } | ||
|
|
||
| // io.WriteString requires an exact predeclared string argument. If the | ||
| // argument is a named string type (e.g. type MyStr string), wrap it with | ||
| // string(...) so the emitted fix compiles. | ||
| sExpr := sText | ||
| if st := pass.TypesInfo.TypeOf(strArg); st != nil && !isExactString(st) { | ||
| sExpr = "string(" + sText + ")" | ||
| } | ||
|
|
||
| // When the receiver is an addressable value whose Write method lives on | ||
| // the pointer type (e.g. var buf bytes.Buffer), io.WriteString requires | ||
| // the pointer form so that the interface conversion compiles. | ||
|
|
@@ -117,8 +125,8 @@ func run(pass *analysis.Pass) (any, error) { | |
| pass.Report(analysis.Diagnostic{ | ||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| Message: fmt.Sprintf("%s.Write([]byte(%s)) can be replaced with io.WriteString(%s, %s) to potentially avoid a []byte allocation if the writer implements io.StringWriter", wText, sText, writerArg, sText), | ||
| SuggestedFixes: buildFix(call, writerArg, sText), | ||
| Message: fmt.Sprintf("%s.Write([]byte(%s)) can be replaced with io.WriteString(%s, %s) to potentially avoid a []byte allocation if the writer implements io.StringWriter", wText, sText, writerArg, sExpr), | ||
| SuggestedFixes: buildFix(call, writerArg, sExpr), | ||
| }) | ||
| }) | ||
|
|
||
|
|
@@ -158,6 +166,15 @@ func isStringType(pass *analysis.Pass, expr ast.Expr) bool { | |
| return ok && basic.Kind() == types.String | ||
| } | ||
|
|
||
| // isExactString reports whether t is the predeclared string type, not a named | ||
| // type whose underlying type is string. io.WriteString(w Writer, s string) | ||
| // requires a predeclared string; named string types need an explicit string(...) | ||
| // conversion to satisfy the parameter type. | ||
| func isExactString(t types.Type) bool { | ||
| b, ok := t.(*types.Basic) | ||
| return ok && b.Kind() == types.String | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] 💡 Suggested testdata additiontype stringAlias = string
func badAliasString() {
var buf bytes.Buffer
s := stringAlias("hello")
buf.Write([]byte(s)) // want `...io\.WriteString\(&buf, s\)...`
// alias IS the predeclared string — no string() wrap
}This documents the alias boundary and prevents a regression if @copilot please address this. |
||
| } | ||
|
Comment on lines
+169
to
+176
|
||
|
|
||
| // implementsWriter reports whether expr's type implements io.Writer. | ||
| // It uses types.Implements against a synthetic io.Writer interface so the check | ||
| // is idiomatic and avoids manually re-implementing the signature comparison. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/diagnosing-bugs] The golden file retains
// wantregex comments in the fixed output. The golden file represents the rewritten source after fixes are applied — these// wantannotations are left over from the original and will cause the test harness to re-run diagnostics against the already-fixed code, producing unexpected second-pass diagnostics.Check how sibling linters handle this:
appendbytestring.go.goldenalso retains// wantcomments, so this may be intentional harness behaviour. IfRunWithSuggestedFixesapplies fixes and re-runs the analyser on the result, these comments would need to match the new diagnostics (onio.WriteString(...)calls which the linter does not flag). Consider verifying this is intentional and add a brief code comment explaining the pattern.@copilot please address this.