diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index b81f1bd1ebf..273162c4086 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -70,65 +70,67 @@ import ( // analyzers. Use it to drive multichecker.Main, test assertions, and // doc-completeness checks so that every consumer stays in sync automatically. func All() []*analysis.Analyzer { - return []*analysis.Analyzer{ - appendbytestring.Analyzer, - appendoneelement.Analyzer, - bytesbufferstring.Analyzer, - bytescomparestring.Analyzer, - contextcancelnotdeferred.Analyzer, - ctxbackground.Analyzer, - deferinloop.Analyzer, - errormessage.Analyzer, - errortypeassertion.Analyzer, - fprintlnsprintf.Analyzer, - errstringmatch.Analyzer, - errorfwrapv.Analyzer, - execcommandwithoutcontext.Analyzer, - excessivefuncparams.Analyzer, - fileclosenotdeferred.Analyzer, - fmterrorfnoverbs.Analyzer, - hardcodedfilepath.Analyzer, - httpnoctx.Analyzer, - httprespbodyclose.Analyzer, - ioutildeprecated.Analyzer, - httpstatuscode.Analyzer, - largefunc.Analyzer, - logfatallibrary.Analyzer, - manualmutexunlock.Analyzer, - mapclearloop.Analyzer, - mapdeletecheck.Analyzer, - nilctxpassed.Analyzer, - osexitinlibrary.Analyzer, - osgetenvlibrary.Analyzer, - ossetenvlibrary.Analyzer, - panicinlibrarycode.Analyzer, - rawloginlib.Analyzer, - regexpcompileinfunction.Analyzer, - ssljson.Analyzer, - seenmapbool.Analyzer, - sortslice.Analyzer, - sprintferrdot.Analyzer, - sprintferrorsnew.Analyzer, - sprintfbool.Analyzer, - sprintfint.Analyzer, - strconvparseignorederror.Analyzer, - stringbytesroundtrip.Analyzer, - stringreplaceminusone.Analyzer, - stringsconcatloop.Analyzer, - stringsindexcontains.Analyzer, - stringsindexhasprefix.Analyzer, - stringsjoinone.Analyzer, - stringscountcontains.Analyzer, - jsonmarshalignoredeerror.Analyzer, - lenstringzero.Analyzer, - lenstringsplit.Analyzer, - timeafterleak.Analyzer, - timesleepnocontext.Analyzer, - timenowsub.Analyzer, - tolowerequalfold.Analyzer, - trimleftright.Analyzer, - uncheckedtypeassertion.Analyzer, - wgdonenotdeferred.Analyzer, - writebytestring.Analyzer, - } + return append([]*analysis.Analyzer(nil), allAnalyzers...) +} + +var allAnalyzers = []*analysis.Analyzer{ + appendbytestring.Analyzer, + appendoneelement.Analyzer, + bytesbufferstring.Analyzer, + bytescomparestring.Analyzer, + contextcancelnotdeferred.Analyzer, + ctxbackground.Analyzer, + deferinloop.Analyzer, + errormessage.Analyzer, + errortypeassertion.Analyzer, + fprintlnsprintf.Analyzer, + errstringmatch.Analyzer, + errorfwrapv.Analyzer, + execcommandwithoutcontext.Analyzer, + excessivefuncparams.Analyzer, + fileclosenotdeferred.Analyzer, + fmterrorfnoverbs.Analyzer, + hardcodedfilepath.Analyzer, + httpnoctx.Analyzer, + httprespbodyclose.Analyzer, + ioutildeprecated.Analyzer, + httpstatuscode.Analyzer, + largefunc.Analyzer, + logfatallibrary.Analyzer, + manualmutexunlock.Analyzer, + mapclearloop.Analyzer, + mapdeletecheck.Analyzer, + nilctxpassed.Analyzer, + osexitinlibrary.Analyzer, + osgetenvlibrary.Analyzer, + ossetenvlibrary.Analyzer, + panicinlibrarycode.Analyzer, + rawloginlib.Analyzer, + regexpcompileinfunction.Analyzer, + ssljson.Analyzer, + seenmapbool.Analyzer, + sortslice.Analyzer, + sprintferrdot.Analyzer, + sprintferrorsnew.Analyzer, + sprintfbool.Analyzer, + sprintfint.Analyzer, + strconvparseignorederror.Analyzer, + stringbytesroundtrip.Analyzer, + stringreplaceminusone.Analyzer, + stringsconcatloop.Analyzer, + stringsindexcontains.Analyzer, + stringsindexhasprefix.Analyzer, + stringsjoinone.Analyzer, + stringscountcontains.Analyzer, + jsonmarshalignoredeerror.Analyzer, + lenstringzero.Analyzer, + lenstringsplit.Analyzer, + timeafterleak.Analyzer, + timesleepnocontext.Analyzer, + timenowsub.Analyzer, + tolowerequalfold.Analyzer, + trimleftright.Analyzer, + uncheckedtypeassertion.Analyzer, + wgdonenotdeferred.Analyzer, + writebytestring.Analyzer, } diff --git a/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go b/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go index a19341e6533..0a57b39e24c 100644 --- a/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go +++ b/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go @@ -53,82 +53,79 @@ func run(pass *analysis.Pass) (any, error) { // string/[]byte round-trip (string([]byte(s))) or a wasteful two-copy clone // ([]byte(string(b))) and reports a diagnostic if so. func analyzeRoundTrip(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { - outer, ok := n.(*ast.CallExpr) + outer, inner, ok := roundTripCalls(n) + if !ok || shouldSkipRoundTrip(pass, outer, generatedFiles, noLintIndex) { + return + } + outerUnderlying, innerUnderlying, innerArgUnderlying, ok := roundTripUnderlyingTypes(pass, outer, inner) if !ok { return } - // Must be a type conversion (single argument, no ellipsis). - if len(outer.Args) != 1 || outer.Ellipsis.IsValid() { + if reportRedundantRoundTrip(pass, outer, inner, outerUnderlying, innerUnderlying, innerArgUnderlying) { return } + reportWastefulCloneRoundTrip(pass, outer, inner, outerUnderlying, innerUnderlying, innerArgUnderlying) +} + +func roundTripCalls(n ast.Node) (*ast.CallExpr, *ast.CallExpr, bool) { + outer, ok := n.(*ast.CallExpr) + if !ok || len(outer.Args) != 1 || outer.Ellipsis.IsValid() { + return nil, nil, false + } + inner, ok := outer.Args[0].(*ast.CallExpr) + if !ok || len(inner.Args) != 1 || inner.Ellipsis.IsValid() { + return nil, nil, false + } + return outer, inner, true +} +func shouldSkipRoundTrip(pass *analysis.Pass, outer *ast.CallExpr, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) bool { pos := pass.Fset.PositionFor(outer.Pos(), false) if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { - return - } - if nolint.HasDirectiveForLinter(pos, noLintIndex, "stringbytesroundtrip") { - return + return true } + return nolint.HasDirectiveForLinter(pos, noLintIndex, "stringbytesroundtrip") +} - // Must be a type conversion, not a function call. +func roundTripUnderlyingTypes(pass *analysis.Pass, outer, inner *ast.CallExpr) (types.Type, types.Type, types.Type, bool) { outerFunInfo, ok := pass.TypesInfo.Types[outer.Fun] if !ok || !outerFunInfo.IsType() { - return + return nil, nil, nil, false } - - outerType := pass.TypesInfo.TypeOf(outer) - if outerType == nil { - return - } - - inner, ok := outer.Args[0].(*ast.CallExpr) - if !ok { - return - } - if len(inner.Args) != 1 || inner.Ellipsis.IsValid() { - return - } - - // The inner call must also be a type conversion, not a function call. innerFunInfo, ok := pass.TypesInfo.Types[inner.Fun] if !ok || !innerFunInfo.IsType() { - return + return nil, nil, nil, false } - + outerType := pass.TypesInfo.TypeOf(outer) innerType := pass.TypesInfo.TypeOf(inner) - if innerType == nil { - return - } innerArgType := pass.TypesInfo.TypeOf(inner.Args[0]) - if innerArgType == nil { - return + if outerType == nil || innerType == nil || innerArgType == nil { + return nil, nil, nil, false } + return outerType.Underlying(), innerType.Underlying(), innerArgType.Underlying(), true +} - outerUnderlying := outerType.Underlying() - innerUnderlying := innerType.Underlying() - innerArgUnderlying := innerArgType.Underlying() - - // Check string([]byte(s)) where s is already a string. - if isStringType(outerUnderlying) && isByteSliceType(innerUnderlying) && isStringType(innerArgUnderlying) { - argText := astutil.NodeText(pass.Fset, inner.Args[0]) - pass.ReportRangef(outer, - "string([]byte(%s)) is a redundant round-trip; the inner []byte conversion copies the string unnecessarily", - argText, - ) - return +func reportRedundantRoundTrip(pass *analysis.Pass, outer, inner *ast.CallExpr, outerUnderlying, innerUnderlying, innerArgUnderlying types.Type) bool { + if !isStringType(outerUnderlying) || !isByteSliceType(innerUnderlying) || !isStringType(innerArgUnderlying) { + return false } + argText := astutil.NodeText(pass.Fset, inner.Args[0]) + pass.ReportRangef(outer, + "string([]byte(%s)) is a redundant round-trip; the inner []byte conversion copies the string unnecessarily", + argText, + ) + return true +} - // Check []byte(string(b)) where b is already a []byte. - // This is the defensive-copy idiom: the result is a non-aliasing copy, not - // a no-op. The diagnostic is therefore not "redundant" but "wasteful": - // two memory copies are made when one would suffice. - if isByteSliceType(outerUnderlying) && isStringType(innerUnderlying) && isByteSliceType(innerArgUnderlying) { - argText := astutil.NodeText(pass.Fset, inner.Args[0]) - pass.ReportRangef(outer, - "[]byte(string(%s)) makes two copies to clone %s; use slices.Clone(%s) or bytes.Clone(%s) for a single-copy independent slice", - argText, argText, argText, argText, - ) +func reportWastefulCloneRoundTrip(pass *analysis.Pass, outer, inner *ast.CallExpr, outerUnderlying, innerUnderlying, innerArgUnderlying types.Type) { + if !isByteSliceType(outerUnderlying) || !isStringType(innerUnderlying) || !isByteSliceType(innerArgUnderlying) { + return } + argText := astutil.NodeText(pass.Fset, inner.Args[0]) + pass.ReportRangef(outer, + "[]byte(string(%s)) makes two copies to clone %s; use slices.Clone(%s) or bytes.Clone(%s) for a single-copy independent slice", + argText, argText, argText, argText, + ) } func isStringType(t types.Type) bool { diff --git a/pkg/parser/remote_download_file.go b/pkg/parser/remote_download_file.go index 381f2dd4b80..93cec217407 100644 --- a/pkg/parser/remote_download_file.go +++ b/pkg/parser/remote_download_file.go @@ -383,61 +383,83 @@ func downloadFileViaRawURL(ctx context.Context, owner, repo, filePath, ref strin func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host string) ([]byte, error) { remoteLog.Printf("Attempting git clone fallback for %s/%s/%s@%s", owner, repo, path, ref) + if err := validateGitCloneFallbackInputs(ref, path); err != nil { + return nil, err + } + tmpDir, err := createGitCloneTempDir() + if err != nil { + return nil, err + } + defer os.RemoveAll(tmpDir) + + repoURL := getRepoGitURL(owner, repo, host) + if len(ref) == 40 && gitutil.IsHexString(ref) { + if err := cloneAndCheckoutSHA(ctx, repoURL, tmpDir, ref); err != nil { + return nil, err + } + } else if err := cloneBranchOrTag(ctx, repoURL, tmpDir, ref); err != nil { + return nil, err + } + + content, err := readFileFromClone(tmpDir, path) + if err != nil { + return nil, err + } + + remoteLog.Printf("Successfully downloaded file via git clone: %s/%s/%s@%s", owner, repo, path, ref) + return content, nil +} + +func validateGitCloneFallbackInputs(ref, path string) error { if err := gitutil.ValidateGitRef(ref); err != nil { - return nil, fmt.Errorf("refusing git clone fallback: %w", err) + return fmt.Errorf("refusing git clone fallback: %w", err) } if err := gitutil.ValidateGitPath(path); err != nil { - return nil, fmt.Errorf("refusing git clone fallback: %w", err) + return fmt.Errorf("refusing git clone fallback: %w", err) } + return nil +} - // Create a temporary directory for the shallow clone +func createGitCloneTempDir() (string, error) { tmpDir, err := os.MkdirTemp("", "gh-aw-git-clone-*") if err != nil { - return nil, fmt.Errorf("failed to create temp directory: %w", err) + return "", fmt.Errorf("failed to create temp directory: %w", err) } - defer os.RemoveAll(tmpDir) + return tmpDir, nil +} - var githubHost string +func getRepoGitURL(owner, repo, host string) string { + githubHost := GetGitHubHostForRepo(owner, repo) if host != "" { githubHost = "https://" + host - } else { - githubHost = GetGitHubHostForRepo(owner, repo) } - repoURL := fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo) + return fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo) +} - // Check if ref is a SHA (40 hex characters) - isSHA := len(ref) == 40 && gitutil.IsHexString(ref) - - var cloneCmd *exec.Cmd - if isSHA { - // For SHA refs, we need to clone without --branch and then checkout the specific commit - // Clone with minimal depth and no branch specified - cloneCmd = exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--no-single-branch", repoURL, tmpDir) - if output, err := cloneCmd.CombinedOutput(); err != nil { - // Try without --no-single-branch if the first attempt fails - remoteLog.Printf("Clone with --no-single-branch failed, trying full clone: %s", string(output)) - cloneCmd = exec.CommandContext(ctx, "git", "clone", repoURL, tmpDir) - if output, err := cloneCmd.CombinedOutput(); err != nil { - return nil, fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output)) - } +func cloneAndCheckoutSHA(ctx context.Context, repoURL, tmpDir, ref string) error { + if output, err := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--no-single-branch", repoURL, tmpDir).CombinedOutput(); err != nil { + remoteLog.Printf("Clone with --no-single-branch failed, trying full clone: %s", string(output)) + if output, err := exec.CommandContext(ctx, "git", "clone", repoURL, tmpDir).CombinedOutput(); err != nil { + return fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output)) } + } + // ValidateGitRef above guarantees ref does not start with '-', so it remains a revision argument. + if output, err := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--detach", ref).CombinedOutput(); err != nil { + return fmt.Errorf("failed to checkout commit %s: %w\nOutput: %s", ref, err, string(output)) + } + return nil +} - // Now checkout the specific commit in detached HEAD mode. ValidateGitRef above - // guarantees ref does not start with '-', so it remains a revision argument. - checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--detach", ref) - if output, err := checkoutCmd.CombinedOutput(); err != nil { - return nil, fmt.Errorf("failed to checkout commit %s: %w\nOutput: %s", ref, err, string(output)) - } - } else { - // For branch/tag refs, use --branch flag; the value is passed via a separate - // argument slot and cannot be confused with a flag because it follows --branch. - cloneCmd = exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", ref, repoURL, tmpDir) - if output, err := cloneCmd.CombinedOutput(); err != nil { - return nil, fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output)) - } +func cloneBranchOrTag(ctx context.Context, repoURL, tmpDir, ref string) error { + // For branch/tag refs, use --branch flag; the value is passed via a separate + // argument slot and cannot be confused with a flag because it follows --branch. + if output, err := exec.CommandContext(ctx, "git", "clone", "--depth", "1", "--branch", ref, repoURL, tmpDir).CombinedOutput(); err != nil { + return fmt.Errorf("failed to clone repository: %w\nOutput: %s", err, string(output)) } + return nil +} - // Read the file from the cloned repository +func readFileFromClone(tmpDir, path string) ([]byte, error) { filePath := filepath.Join(tmpDir, path) if err := fileutil.ValidatePathWithinBase(tmpDir, filePath); err != nil { return nil, fmt.Errorf("refusing to read file outside clone directory: %w", err) @@ -446,7 +468,5 @@ func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host s if err != nil { return nil, fmt.Errorf("failed to read file from cloned repository: %w", err) } - - remoteLog.Printf("Successfully downloaded file via git clone: %s/%s/%s@%s", owner, repo, path, ref) return content, nil }