Alan Donovan (Gerrit)

unread,

Sep 28, 2026, 7:09:52 PMSep 28

to goph...@pubsubhelper.golang.org, Robert Griesemer, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com

Attention needed from Robert Griesemer

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

Robert Griesemer

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Sep 29, 2026, 10:23:45 AMSep 29

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Attention needed from Robert Griesemer

Alan Donovan uploaded new patchset

Alan Donovan uploaded patch set #5 to this change.

Following approvals got outdated and were removed:

TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com

Open in Gerrit

Related details

Attention is currently required from:

Robert Griesemer

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alex Putman (Gerrit)

unread,

Oct 5, 2026, 12:17:11 PMOct 5

to Alan Donovan, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Robert Griesemer, golang-co...@googlegroups.com

Attention needed from Alan Donovan and Robert Griesemer

Alex Putman added 3 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Alex Putman . resolved

the large set of code comments made this much easier to review, thanks!

File internal/analysis/intconv/intconv.go

Line 187, Patchset 6 (Latest): wrap := func(start, end token.Pos, before, after string) []analysis.TextEdit {

Alex Putman . unresolved

Naming is a bit confusing here for me. When I was reading the use of the function below, I was getting confused at where the "x" was being inserted, then I remembered that it's wrapping the "x".

Maybe less of a naming issue, but more that the call to the wrap doesn't easily distinguish `before` vs `after`. I can't think of an easy remedy, so feel free to just ignore this comment!

Line 266, Patchset 6 (Latest): addFix("Format the number as a decimal", sprintf("d"))

Alex Putman . unresolved

Do we want to highlight to the user that this is possibly behavior changing? Compared to the possible other suggested fix? And maybed we highlight that Fix 1 is behavior preserving?

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan
Robert Griesemer

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Oct 5, 2026, 1:29:07 PMOct 5

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Attention needed from Alan Donovan and Robert Griesemer

Alan Donovan uploaded new patchset

Alan Donovan <adon...@google.com> uploaded patch set #7 to this change.

Following approvals got outdated and were removed:

TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan
Robert Griesemer

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Oct 5, 2026, 1:29:26 PMOct 5

to goph...@pubsubhelper.golang.org, Robert Griesemer, Alex Putman, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com

Attention needed from Alex Putman

Alan Donovan voted and added 2 comments

Votes added by Alan Donovan <adon...@google.com>

Auto-Submit

+1

2 comments

File internal/analysis/intconv/intconv.go

Line 187, Patchset 6: wrap := func(start, end token.Pos, before, after string) []analysis.TextEdit {

Alex Putman . resolved

Naming is a bit confusing here for me. When I was reading the use of the function below, I was getting confused at where the "x" was being inserted, then I remembered that it's wrapping the "x".
Maybe less of a naming issue, but more that the call to the wrap doesn't easily distinguish `before` vs `after`. I can't think of an easy remedy, so feel free to just ignore this comment!

Alan Donovan

Fair point. I melted `wrap` down and extracted `edit`, a trivial constructor for one TextEdit.

Line 266, Patchset 6: addFix("Format the number as a decimal", sprintf("d"))

Alex Putman . resolved

Do we want to highlight to the user that this is possibly behavior changing? Compared to the possible other suggested fix? And maybed we highlight that Fix 1 is behavior preserving?

Alan Donovan

Done

Open in Gerrit

Related details

Attention is currently required from:

Alex Putman

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alex Putman (Gerrit)

unread,

Oct 5, 2026, 5:54:24 PMOct 5

to Alan Donovan, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, Robert Griesemer, golang-co...@googlegroups.com

Attention needed from Alan Donovan

Alex Putman voted Code-Review+2

Code-Review

+2

Open in Gerrit

Related details

Attention is currently required from:

Alan Donovan

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Alan Donovan (Gerrit)

unread,

Oct 5, 2026, 5:54:42 PMOct 5

to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Alex Putman, golang...@luci-project-accounts.iam.gserviceaccount.com, Robert Griesemer, golang-co...@googlegroups.com

Alan Donovan submitted the change

Change information

Commit message:

go/analysis/passes/stringintconv: make first fix behavior-preserving

Go 1.28 rejects string(x) conversions where x is a non-byte,
non-rune integer (go.dev/issue/3939). To let "go fix" migrate code
before a module's go directive is raised to go1.28, this CL makes
the analyzer's first suggested fix preserve the existing behavior,
and adds the analyzer to the go fix suite.

The old "Convert a single rune to a string" fix, string(rune(x)),
was not behavior-preserving for types wider than 32 bits due to
truncation. For such types the fix now uses fmt.Sprintf("%c", x),
which is exactly equivalent to string(x). The plain rune(x) form is
still used for constants that fit in 32 bits and for types of at
most 32 bits.

The safe fix is now offered first, since that's what go fix applies.
The decimal-formatting fix, which changes behavior, is second; it now
uses fmt.Sprintf("%d", x), for symmetry, instead of fmt.Sprint(x).
If no behavior-preserving fix is available no fixes are offered at
all, so that go fix never applies the second fix.

Also:
- simplify the edits of both Sprintf fixes by choosing the range to
  rewrite once: string(x) -> fmt.Sprintf(...), and
  mystring(x) -> mystring(fmt.Sprintf(...)).
  This fixes a bug in the old fmt.Sprint fix, which dropped the
  conversion S(x) when S was a type parameter whose sole term
  is string, yielding an ill-typed result.
- update the doc, which recommended strconv.Itoa.

Updates golang/go#81760

Change-Id: If445f650d045125475b2051e4c50e28f630a11ae

Reviewed-on: https://go-review.googlesource.com/c/tools/+/840805

LUCI-TryBot-Result: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>

Reviewed-by: Alex Putman <apu...@golang.org>

Auto-Submit: Alan Donovan <adon...@google.com>

Files:

M go/analysis/passes/stringintconv/doc.go
A go/analysis/passes/stringintconv/preserve_test.go
M go/analysis/passes/stringintconv/testdata/src/fix/fix.go.golden
M go/analysis/passes/stringintconv/testdata/src/fix/fixdot.go.golden
M go/analysis/passes/stringintconv/testdata/src/fix/fixnamed.go.golden
A go/analysis/passes/stringintconv/testdata/src/fix/fixtarget.go
A go/analysis/passes/stringintconv/testdata/src/fix/fixtarget.go.golden
A go/analysis/passes/stringintconv/testdata/src/fix/fixwide.go
A go/analysis/passes/stringintconv/testdata/src/fix/fixwide.go.golden
M go/analysis/suite/fix/fix.go
M gopls/doc/analyzers.md
M gopls/internal/doc/api.json
M internal/analysis/intconv/intconv.go

Change size: L

Delta: 13 files changed, 438 insertions(+), 69 deletions(-)

Branch: refs/heads/master

Submit Requirements:

Code-Review: +2 by Alex Putman
Review-Enforcement: Code-Review+2 by Alex Putman
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[tools] go/analysis/passes/stringintconv: make first fix behavior-preserving

Alan Donovan (Gerrit)

New activity on the change

Related details

Alan Donovan (Gerrit)

Alan Donovan uploaded new patchset

Related details

Alex Putman (Gerrit)

Alex Putman added 3 comments

Related details

Alan Donovan (Gerrit)

Alan Donovan uploaded new patchset

Related details

Alan Donovan (Gerrit)

Alan Donovan voted and added 2 comments

Votes added by Alan Donovan <adon...@google.com>

2 comments

Related details

Alex Putman (Gerrit)

Alex Putman voted Code-Review+2

Related details

Alan Donovan (Gerrit)

Alan Donovan submitted the change

Change information