[davecheney]

go fix cannot load TinyGo's src packages or target-specific code because it does not know TinyGo's overlays and build tags.

Instead, tinygo fix runs the go fix analyzers through TinyGo's loader, so the same modernizations can be applied to TinyGo itself and to programs built for a TinyGo target.

/ht to @dgryski for the suggestion

[davecheney]

This is mostly working now. It has some usability issues compared to regular go fix because the intersectional nature of tinygo's use of build tags. However, if applied surgically, it does work, examples follow.

More than modernising existing tinygo code, it unlocks the //go:fix inline pragma which lets us deprecate older methods and have code automatically rewriting from the forwarding stubs to newer forms. I'll include an example as well

[davecheney]

example one

./build/tinygo fix -target pico -diff -rangeint ./src/internal/reflectlite

-for i := uintptr(0); i < size; i++ {
+for range size {

example two

./build/tinygo fix -target pico -diff -rangeint ./src/machine/usb/msc

-for i := 0; i < length; i++ {
+for i := range length {

example three

./build/tinygo fix -C ~/devel/drivers -target pico -diff -rangeint ./ssd1289

-for i := uint8(0); i < 16; i++ {
+for i := range uint8(16) {
     pin := machine.Pin(i + uint8(firstPin))
     pin.Configure(machine.PinConfig{Mode: machine.PinOutput})
 }

example four; interface{} -> any

./build/tinygo fix -C ~/devel/drivers -target pico -diff -any ./tester

- Fatalf(f string, a ...interface{})
+ Fatalf(f string, a ...any)

#5966 adds RandomUint32 and marks the old GetRNG wrapper with //go:fix inline.

TINYGOROOT=<rename-branch-sources> ./build/tinygo fix \
  -C <caller-module> -target pico -inline -diff .

-value, err := machine.GetRNG()
+value, err := machine.RandomUint32()

[davecheney]

The test failure is karma for fixing the tests so they actually run on windows :)

[deadprogram]

@davecheney thanks for working on fixing fix. I checked out this PR and ran the tests locally. Here are some notes edited from an automated review:

1. /... patterns under TinyGo's src find no packages. ./src/machine/..., ./src/machine/usb/..., ./src/internal/... and ./src/runtime/... all end with no packages matched. The cached GOROOT has those directories as symlinks, and go list does not follow symlinks for .... ./src/... turns into std, which fails with package net (test) imports internal/poll: build constraints exclude all Go files. The error for plain ./src also suggests ./src/.... Perhaps expand these patterns by walking $TINYGOROOT/src instead of passing them to go list, or at least change that hint?

2. The -diff output is always one hunk from the first change to the last, so unchanged lines in between show up as removed and added. For example tinygo fix -target pico -diff ./src/machine shows 83 lines of -/+ in machine_rp2_i2c.go and the whole roscRandByte function in machine_rp2_rng.go for two rangeint changes. The patch still applies, but it is hard to read. Separate hunks with 3 lines of context, like go fix -diff, might help.

3. Test files are always loaded, so one test file that does not build for the target blocks the whole package. tinygo fix -target pico -diff ./src/os/... fails type checking in os_test.go and stat_test.go (chtmpdir, os.SameFile). Maybe add a way to leave tests out, or skip test packages that fail to type check with a warning?

4. go1.25 is hardcoded in loader/analysis.go and must match minorMin in builder/config.go. Sharing one constant would keep them from drifting when the minimum Go version goes up.

What do you think? Thanks!

[davecheney]

@davecheney thanks for working on fixing fix. I checked out this PR and ran the tests locally. Here are some notes edited from an automated review:

1. /... patterns under TinyGo's src find no packages. ./src/machine/..., ./src/machine/usb/..., ./src/internal/... and ./src/runtime/... all end with no packages matched. The cached GOROOT has those directories as symlinks, and go list does not follow symlinks for .... ./src/... turns into std, which fails with package net (test) imports internal/poll: build constraints exclude all Go files. The error for plain ./src also suggests ./src/.... Perhaps expand these patterns by walking $TINYGOROOT/src instead of passing them to go list, or at least change that hint?
2. The -diff output is always one hunk from the first change to the last, so unchanged lines in between show up as removed and added. For example tinygo fix -target pico -diff ./src/machine shows 83 lines of -/+ in machine_rp2_i2c.go and the whole roscRandByte function in machine_rp2_rng.go for two rangeint changes. The patch still applies, but it is hard to read. Separate hunks with 3 lines of context, like go fix -diff, might help.
3. Test files are always loaded, so one test file that does not build for the target blocks the whole package. tinygo fix -target pico -diff ./src/os/... fails type checking in os_test.go and stat_test.go (chtmpdir, os.SameFile). Maybe add a way to leave tests out, or skip test packages that fail to type check with a warning?
4. go1.25 is hardcoded in loader/analysis.go and must match minorMin in builder/config.go. Sharing one constant would keep them from drifting when the minimum Go version goes up.

What do you think? Thanks!

Thanks for the feedback. I'll take another pass at this in the morning. buenas noches

[davecheney]

Thanks for the examples. I’ve addressed points 1 and 2.

1. Recursive patterns under TinyGo’s src now expand to packages from the TinyGo source tree. For example, this now finds internal/reflectlite:

tinygo fix -target pico -diff -rangeint ./src/internal/...

@@ -10,7 +10,7 @@
-	for i := uintptr(0); i < size; i++ {
+	for range size {

2. -diff now prints separate hunks. For example, ./src/machine reports separate changes in machine_rp2040_usb.go:

@@ -108,7 +108,7 @@
-		for i := 0; i < 16; i++ {
+		for i := range 16 {
@@ -118,7 +118,7 @@

The package-pattern and diff tests pass. Points 3 and 4 remain open.

[davecheney]

Thanks for the suggestions. I’ve addressed points 3 and 4.

3. tinygo fix now accepts -tests=false to exclude test files. Tests remain included by default. For example:

tinygo fix -target pico -tests=false -diff -rangeint ./src/machine/usb/...

This now produces 4 hunks across 3 files. ./src/os/... also runs without the test-file errors. The full ./src/... tree can still fail on non-test packages that do not build for the target, such as crypto/tls importing net for Pico.

4. The minimum Go version now has one shared definition:

const MinimumGoMinorVersion = 25

Both the builder’s compatibility check and the loader’s language-version setting use it.

The focused tests and vet checks pass.

[davecheney]

@deadprogram PTAL. Should be close now

[deadprogram]

Thanks @davecheney for the updates. Edited from an automated review.

I rebuilt at 3918697 and reran the earlier examples against the pico target.

1. ./src/internal/... now finds internal/reflectlite and prints its hunks. Running ./internal/... from inside src works too.
2. -diff now prints separate hunks per change, for example two in machine_rp2040_usb.go.
3. -tests=false lets ./src/os/... run cleanly, and ./src/machine/usb/... gives 4 hunks.
4. MinimumGoMinorVersion is now used in both the builder check and the loader.

The fix, internal/diff and loader tests pass, plus the Fix tests in the main package.

One small optional idea. The diff headers use absolute paths, so git apply will not take the output directly. Paths relative to the current directory might be easier to use, but this could be a follow-up.

Looks good to me. Thanks for getting us fixed up!

[davecheney]

Thanks @davecheney for the updates. Edited from an automated review.

I rebuilt at 3918697 and reran the earlier examples against the pico target.

1. ./src/internal/... now finds internal/reflectlite and prints its hunks. Running ./internal/... from inside src works too.
2. -diff now prints separate hunks per change, for example two in machine_rp2040_usb.go.
3. -tests=false lets ./src/os/... run cleanly, and ./src/machine/usb/... gives 4 hunks.
4. MinimumGoMinorVersion is now used in both the builder check and the loader.

The fix, internal/diff and loader tests pass, plus the Fix tests in the main package.

One small optional idea. The diff headers use absolute paths, so git apply will not take the output directly. Paths relative to the current directory might be easier to use, but this could be a follow-up.

Looks good to me. Thanks for getting us fixed up!

-diff is probably only useful in a dry run mode, I'm not sure, and i certainly haven't used it as an actual tool to two step {tiny,}go fix. The diff code is copied from biggo, so that limitation might exist upstream. I'm happy to keep working on this and polishing it over time.

btw, here's a teaser of what tinygo fix can do beyond rewriting old syntax, tinygo-org/drivers#936