Skip to content

Conversation

@rifeplight
Copy link

The new version of Go has been optimized, and variables do not need to be reassigned.

For more info: https://tip.golang.org/wiki/LoopvarExperiment#does-this-mean-i-dont-have-to-write-x--x-in-my-loops-anymore

category: refactor
ticket: none

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR attempts to remove unnecessary variable reassignments (e.g., conf := conf, exitMsg := exitMsg) that were historically required to properly capture loop variables before Go 1.22. The Go loopvar experiment, which became default in Go 1.22, changes loop variable scoping so that each iteration gets its own variable, eliminating the need for these reassignments in some cases.

However, this PR introduces critical bugs in 2 out of 3 files because the loopvar change only affects loop iteration variables, not variables from outer scopes that are modified within loops. When an outer-scope variable is modified in a loop and then captured by a goroutine or used across subtests, the old reassignment pattern is still necessary to create a loop-scoped copy.

Key Changes

  • Removes conf := conf from dkg/dkg_test.go (introduces race condition bug)
  • Removes two instances of conf := conf from cmd/createcluster_internal_test.go (introduces test isolation bug)
  • Removes two instances of exitMsg := exitMsg from app/obolapi/exit_test.go (safe - struct is copied by value)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
dkg/dkg_test.go Buggy - Removes conf := conf but conf is modified per iteration and captured by concurrent goroutines, causing a race condition
cmd/createcluster_internal_test.go Buggy - Removes conf := conf but conf modifications now leak between subtests, breaking test isolation
app/obolapi/exit_test.go Correct - Removes exitMsg := exitMsg safely because the struct is copied by value when assigned
Comments suppressed due to low confidence (1)

dkg/dkg_test.go:350

  • The removal of conf := conf introduces a bug. The conf variable is modified in each loop iteration (lines 332-336), and then captured by a goroutine at line 343. Without the local copy, all goroutines will capture the same conf reference and see the final modified state from the last loop iteration, rather than the state from their own iteration.

While the loopvar change in Go 1.22+ fixes loop variable capture for the loop index (i), it does not fix the issue of capturing mutable outer-scope variables that are modified within the loop. The conf := conf reassignment is still necessary here to create a loop-scoped copy.

	for i := range len(def.Operators) {
		conf.DataDir = path.Join(dir, fmt.Sprintf("node%d", i))

		conf.P2P.TCPAddrs = []string{testutil.AvailableAddr(t).String()}
		if len(addConfig) > 0 {
			conf.AppendConfig = &addConfig[i]
		}

		require.NoError(t, os.MkdirAll(conf.DataDir, 0o755))
		err := k1util.Save(p2pKeys[i], p2p.KeyPath(conf.DataDir))
		require.NoError(t, err)

		eg.Go(func() error {
			err := dkg.Run(peerCtx(ctx, i), conf)
			if err != nil {
				cancel()
			}

			return err
		})

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sonarqubecloud
Copy link

@rifeplight
Copy link
Author

Reverted according to the CI and copilot suggesstion.

@rifeplight
Copy link
Author

/test

@KaloyanTanev
Copy link
Collaborator

Those changes are not what loopvar change in go v1.22 is about. The change in v1.22 is regarding the variables created from the for loop line. Those 3 variables changed are not related to the loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants