From b273dc5942d7689c76a4ea7ce499cf0d7a26a83d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 12:20:50 +0100 Subject: [PATCH] Enable linter 'copyloopvar' and fix the issues (#2160) ## Changes - Remove all unnecessary copies of the loop variable, it is not necessary since Go 1.22 https://go.dev/blog/loopvar-preview - Enable the linter that catches this issue https://github.com/karamaru-alpha/copyloopvar ## Tests Existing tests. --- .golangci.yaml | 2 + .../config/validate/validate_sync_patterns.go | 8 +- cmd/bundle/generate/utils.go | 4 +- integration/cmd/fs/cat_test.go | 18 ++-- integration/cmd/fs/cp_test.go | 86 +++++++------------ integration/cmd/fs/ls_test.go | 30 +++---- integration/cmd/fs/mkdir_test.go | 18 ++-- integration/cmd/fs/rm_test.go | 30 +++---- integration/libs/filer/filer_test.go | 12 +-- integration/libs/locker/locker_test.go | 3 +- libs/filer/files_client.go | 2 - 11 files changed, 74 insertions(+), 139 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 9711a70af..ea6d65db1 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -42,6 +42,8 @@ linters-settings: disable: # good check, but we have too many assert.(No)?Errorf? so excluding for now - require-error + copyloopvar: + check-alias: true issues: exclude-dirs-use-default: false # recommended by docs https://golangci-lint.run/usage/false-positives/ max-issues-per-linter: 1000 diff --git a/bundle/config/validate/validate_sync_patterns.go b/bundle/config/validate/validate_sync_patterns.go index f5787a81d..04acd28ab 100644 --- a/bundle/config/validate/validate_sync_patterns.go +++ b/bundle/config/validate/validate_sync_patterns.go @@ -47,15 +47,13 @@ func checkPatterns(patterns []string, path string, rb bundle.ReadOnlyBundle) (di var errs errgroup.Group var diags diag.Diagnostics - for i, pattern := range patterns { - index := i - fullPattern := pattern + for index, pattern := range patterns { // If the pattern is negated, strip the negation prefix // and check if the pattern matches any files. // Negation in gitignore syntax means "don't look at this path' // So if p matches nothing it's useless negation, but if there are matches, // it means: do not include these files into result set - p := strings.TrimPrefix(fullPattern, "!") + p := strings.TrimPrefix(pattern, "!") errs.Go(func() error { fs, err := fileset.NewGlobSet(rb.BundleRoot(), []string{p}) if err != nil { @@ -72,7 +70,7 @@ func checkPatterns(patterns []string, path string, rb bundle.ReadOnlyBundle) (di mu.Lock() diags = diags.Append(diag.Diagnostic{ Severity: diag.Warning, - Summary: fmt.Sprintf("Pattern %s does not match any files", fullPattern), + Summary: fmt.Sprintf("Pattern %s does not match any files", pattern), Locations: []dyn.Location{loc.Location()}, Paths: []dyn.Path{loc.Path()}, }) diff --git a/cmd/bundle/generate/utils.go b/cmd/bundle/generate/utils.go index cbea0bfcc..c2c9bbb55 100644 --- a/cmd/bundle/generate/utils.go +++ b/cmd/bundle/generate/utils.go @@ -138,9 +138,7 @@ func (n *downloader) FlushToDisk(ctx context.Context, force bool) error { } errs, errCtx := errgroup.WithContext(ctx) - for k, v := range n.files { - targetPath := k - filePath := v + for targetPath, filePath := range n.files { errs.Go(func() error { reader, err := n.w.Workspace.Download(errCtx, filePath) if err != nil { diff --git a/integration/cmd/fs/cat_test.go b/integration/cmd/fs/cat_test.go index 3e964fe6e..14ec8140e 100644 --- a/integration/cmd/fs/cat_test.go +++ b/integration/cmd/fs/cat_test.go @@ -18,13 +18,11 @@ func TestFsCat(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) err := f.Write(context.Background(), "hello.txt", strings.NewReader("abcd"), filer.CreateParentDirectories) require.NoError(t, err) @@ -40,13 +38,11 @@ func TestFsCatOnADir(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) err := f.Mkdir(context.Background(), "dir1") require.NoError(t, err) @@ -61,13 +57,11 @@ func TestFsCatOnNonExistentFile(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - _, tmpDir := tc.setupFiler(t) + _, tmpDir := testCase.setupFiler(t) _, _, err := testcli.RequireErrorRun(t, ctx, "fs", "cat", path.Join(tmpDir, "non-existent-file")) assert.ErrorIs(t, err, fs.ErrNotExist) diff --git a/integration/cmd/fs/cp_test.go b/integration/cmd/fs/cp_test.go index 76aef7acf..6d0266555 100644 --- a/integration/cmd/fs/cp_test.go +++ b/integration/cmd/fs/cp_test.go @@ -126,14 +126,12 @@ func TestFsCpDir(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", sourceDir, targetDir, "--recursive") @@ -147,14 +145,12 @@ func TestFsCpFileToFile(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceFile(t, context.Background(), sourceFiler) testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "foo.txt"), path.Join(targetDir, "bar.txt")) @@ -168,14 +164,12 @@ func TestFsCpFileToDir(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceFile(t, context.Background(), sourceFiler) testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "foo.txt"), targetDir) @@ -205,14 +199,12 @@ func TestFsCpDirToDirFileNotOverwritten(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) // Write a conflicting file to target @@ -231,14 +223,12 @@ func TestFsCpFileToDirFileNotOverwritten(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) // Write a conflicting file to target @@ -255,14 +245,12 @@ func TestFsCpFileToFileFileNotOverwritten(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) // Write a conflicting file to target @@ -279,14 +267,12 @@ func TestFsCpDirToDirWithOverwriteFlag(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) // Write a conflicting file to target @@ -303,14 +289,12 @@ func TestFsCpFileToFileWithOverwriteFlag(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) // Write a conflicting file to target @@ -327,14 +311,12 @@ func TestFsCpFileToDirWithOverwriteFlag(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) // Write a conflicting file to target @@ -351,13 +333,11 @@ func TestFsCpErrorsWhenSourceIsDirWithoutRecursiveFlag(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - _, tmpDir := tc.setupFiler(t) + _, tmpDir := testCase.setupFiler(t) _, _, err := testcli.RequireErrorRun(t, ctx, "fs", "cp", path.Join(tmpDir), path.Join(tmpDir, "foobar")) r := regexp.MustCompile("source path .* is a directory. Please specify the --recursive flag") @@ -376,14 +356,12 @@ func TestFsCpSourceIsDirectoryButTargetIsFile(t *testing.T) { t.Parallel() for _, testCase := range copyTests() { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - sourceFiler, sourceDir := tc.setupSource(t) - targetFiler, targetDir := tc.setupTarget(t) + sourceFiler, sourceDir := testCase.setupSource(t) + targetFiler, targetDir := testCase.setupTarget(t) setupSourceDir(t, context.Background(), sourceFiler) // Write a conflicting file to target diff --git a/integration/cmd/fs/ls_test.go b/integration/cmd/fs/ls_test.go index 25929fdf3..0f53193bf 100644 --- a/integration/cmd/fs/ls_test.go +++ b/integration/cmd/fs/ls_test.go @@ -43,13 +43,11 @@ func TestFsLs(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) setupLsFiles(t, f) stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "ls", tmpDir, "--output=json") @@ -77,13 +75,11 @@ func TestFsLsWithAbsolutePaths(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) setupLsFiles(t, f) stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "ls", tmpDir, "--output=json", "--absolute") @@ -111,13 +107,11 @@ func TestFsLsOnFile(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) setupLsFiles(t, f) _, _, err := testcli.RequireErrorRun(t, ctx, "fs", "ls", path.Join(tmpDir, "a", "hello.txt"), "--output=json") @@ -131,13 +125,11 @@ func TestFsLsOnEmptyDir(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - _, tmpDir := tc.setupFiler(t) + _, tmpDir := testCase.setupFiler(t) stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "ls", tmpDir, "--output=json") assert.Equal(t, "", stderr.String()) @@ -155,13 +147,11 @@ func TestFsLsForNonexistingDir(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - _, tmpDir := tc.setupFiler(t) + _, tmpDir := testCase.setupFiler(t) _, _, err := testcli.RequireErrorRun(t, ctx, "fs", "ls", path.Join(tmpDir, "nonexistent"), "--output=json") assert.ErrorIs(t, err, fs.ErrNotExist) diff --git a/integration/cmd/fs/mkdir_test.go b/integration/cmd/fs/mkdir_test.go index eff0599a7..5cea0599c 100644 --- a/integration/cmd/fs/mkdir_test.go +++ b/integration/cmd/fs/mkdir_test.go @@ -17,13 +17,11 @@ func TestFsMkdir(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) // create directory "a" stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "mkdir", path.Join(tmpDir, "a")) @@ -43,13 +41,11 @@ func TestFsMkdirCreatesIntermediateDirectories(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) // create directory "a/b/c" stdout, stderr := testcli.RequireSuccessfulRun(t, ctx, "fs", "mkdir", path.Join(tmpDir, "a", "b", "c")) @@ -81,13 +77,11 @@ func TestFsMkdirWhenDirectoryAlreadyExists(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) // create directory "a" err := f.Mkdir(context.Background(), "a") diff --git a/integration/cmd/fs/rm_test.go b/integration/cmd/fs/rm_test.go index 018c7920e..fc19bb5b5 100644 --- a/integration/cmd/fs/rm_test.go +++ b/integration/cmd/fs/rm_test.go @@ -17,14 +17,12 @@ func TestFsRmFile(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() // Create a file ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) err := f.Write(context.Background(), "hello.txt", strings.NewReader("abcd"), filer.CreateParentDirectories) require.NoError(t, err) @@ -48,14 +46,12 @@ func TestFsRmEmptyDir(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() // Create a directory ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) err := f.Mkdir(context.Background(), "a") require.NoError(t, err) @@ -79,14 +75,12 @@ func TestFsRmNonEmptyDirectory(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() // Create a directory ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) err := f.Mkdir(context.Background(), "a") require.NoError(t, err) @@ -110,13 +104,11 @@ func TestFsRmForNonExistentFile(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - _, tmpDir := tc.setupFiler(t) + _, tmpDir := testCase.setupFiler(t) // Expect error if file does not exist _, _, err := testcli.RequireErrorRun(t, ctx, "fs", "rm", path.Join(tmpDir, "does-not-exist")) @@ -129,13 +121,11 @@ func TestFsRmDirRecursively(t *testing.T) { t.Parallel() for _, testCase := range fsTests { - tc := testCase - - t.Run(tc.name, func(t *testing.T) { + t.Run(testCase.name, func(t *testing.T) { t.Parallel() ctx := context.Background() - f, tmpDir := tc.setupFiler(t) + f, tmpDir := testCase.setupFiler(t) // Create a directory err := f.Mkdir(context.Background(), "a") diff --git a/integration/libs/filer/filer_test.go b/integration/libs/filer/filer_test.go index 21c839e1b..bc1713b30 100644 --- a/integration/libs/filer/filer_test.go +++ b/integration/libs/filer/filer_test.go @@ -128,11 +128,9 @@ func TestFilerRecursiveDelete(t *testing.T) { {"files", setupUcVolumesFiler}, {"workspace files extensions", setupWsfsExtensionsFiler}, } { - tc := testCase - t.Run(testCase.name, func(t *testing.T) { t.Parallel() - f, _ := tc.f(t) + f, _ := testCase.f(t) ctx := context.Background() // Common tests we run across all filers to ensure consistent behavior. @@ -239,11 +237,9 @@ func TestFilerReadWrite(t *testing.T) { {"files", setupUcVolumesFiler}, {"workspace files extensions", setupWsfsExtensionsFiler}, } { - tc := testCase - t.Run(testCase.name, func(t *testing.T) { t.Parallel() - f, _ := tc.f(t) + f, _ := testCase.f(t) ctx := context.Background() // Common tests we run across all filers to ensure consistent behavior. @@ -348,11 +344,9 @@ func TestFilerReadDir(t *testing.T) { {"files", setupUcVolumesFiler}, {"workspace files extensions", setupWsfsExtensionsFiler}, } { - tc := testCase - t.Run(testCase.name, func(t *testing.T) { t.Parallel() - f, _ := tc.f(t) + f, _ := testCase.f(t) ctx := context.Background() commonFilerReadDirTest(t, ctx, f) diff --git a/integration/libs/locker/locker_test.go b/integration/libs/locker/locker_test.go index 524996465..93cb1ffce 100644 --- a/integration/libs/locker/locker_test.go +++ b/integration/libs/locker/locker_test.go @@ -66,9 +66,8 @@ func TestLock(t *testing.T) { } var wg sync.WaitGroup - for i := range numConcurrentLocks { + for currentIndex := range numConcurrentLocks { wg.Add(1) - currentIndex := i go func() { defer wg.Done() time.Sleep(time.Duration(rand.Intn(100)) * time.Millisecond) diff --git a/libs/filer/files_client.go b/libs/filer/files_client.go index 98a534684..88bbadd32 100644 --- a/libs/filer/files_client.go +++ b/libs/filer/files_client.go @@ -303,8 +303,6 @@ func (w *FilesClient) recursiveDelete(ctx context.Context, name string) error { group.SetLimit(maxFilesRequestsInFlight) for _, file := range filesToDelete { - file := file - // Skip the file if the context has already been cancelled. select { case <-groupCtx.Done():