Merge branch 'master' into gopls-release-branch.0.21

Since nearly all recent work has been fixes, it's easier
to merge from master than to cherrypick.

For golang/go#76367

Change-Id: I6ca9ac63aaae81ee6dc3e3c6b3704ff156d9dcd7

Author: adonovan

go/analysis/internal/checker/checker.go

@@ -205,7 +205,10 @@ func Run(args []string, analyzers []*analysis.Analyzer) (exitcode int) {
 				}
 			}
 		}
-		if err := driverutil.ApplyFixes(fixActions, analysisflags.Diff, dbg('v')); err != nil {
+		write := func(filename string, content []byte) error {
+			return os.WriteFile(filename, content, 0644)
+		}
+		if err := driverutil.ApplyFixes(fixActions, write, analysisflags.Diff, dbg('v')); err != nil {
 			// Fail when applying fixes failed.
 			log.Print(err)
 			exitAtLeast(1)

go/analysis/internal/checker/testdata/json.txt

@@ -1,8 +1,8 @@
 # Test basic JSON output.
-#
-# File slashes assume non-Windows.
 
+# File slashes assume non-Windows.
 skip GOOS=windows
+
 checker -rename -json example.com/p
 exit 0
 
@@ -21,6 +21,7 @@ func f(bar int) {}
 		"rename": [
 			{
 				"posn": "/TMP/p/p.go:3:8",
+				"end": "/TMP/p/p.go:3:11",
 				"message": "renaming \"bar\" to \"baz\"",
 				"suggested_fixes": [
 					{

go/analysis/passes/ctrlflow/ctrlflow.go

@@ -179,12 +179,13 @@ func (c *CFGs) buildDecl(fn *types.Func, di *declInfo) {
 	}
 	di.started = true
 
-	noreturn := isIntrinsicNoReturn(fn)
-
-	if di.decl.Body != nil {
-		di.cfg = cfg.New(di.decl.Body, c.callMayReturn)
-		if cfginternal.IsNoReturn(di.cfg) {
-			noreturn = true
+	noreturn, known := knownIntrinsic(fn)
+	if !known {
+		if di.decl.Body != nil {
+			di.cfg = cfg.New(di.decl.Body, c.callMayReturn)
+			if cfginternal.IsNoReturn(di.cfg) {
+				noreturn = true
+			}
 		}
 	}
 	if noreturn {
@@ -233,11 +234,36 @@ func (c *CFGs) callMayReturn(call *ast.CallExpr) (r bool) {
 
 var panicBuiltin = types.Universe.Lookup("panic").(*types.Builtin)
 
-// isIntrinsicNoReturn reports whether a function intrinsically never
-// returns because it stops execution of the calling thread.
+// knownIntrinsic reports whether a function intrinsically never
+// returns because it stops execution of the calling thread, or does
+// in fact return, contrary to its apparent body, because it is
+// handled specially by the compiler.
+//
 // It is the base case in the recursion.
-func isIntrinsicNoReturn(fn *types.Func) bool {
+func knownIntrinsic(fn *types.Func) (noreturn, known bool) {
 	// Add functions here as the need arises, but don't allocate memory.
-	return typesinternal.IsFunctionNamed(fn, "syscall", "Exit", "ExitProcess", "ExitThread") ||
-		typesinternal.IsFunctionNamed(fn, "runtime", "Goexit")
+
+	// Functions known intrinsically never to return.
+	if typesinternal.IsFunctionNamed(fn, "syscall", "Exit", "ExitProcess", "ExitThread") ||
+		typesinternal.IsFunctionNamed(fn, "runtime", "Goexit") {
+		return true, true
+	}
+
+	// Compiler intrinsics known to return, contrary to
+	// what analysis of the function body would conclude.
+	//
+	// Not all such intrinsics must be listed here: ctrlflow
+	// considers any function called for its value--such as
+	// crypto/internal/constanttime.bool2Uint8--to potentially
+	// return; only functions called as a statement, for effects,
+	// are no-return candidates.
+	//
+	// Unfortunately this does sometimes mean peering into internals.
+	// Where possible, use the nearest enclosing public API function.
+	if typesinternal.IsFunctionNamed(fn, "internal/abi", "EscapeNonString") ||
+		typesinternal.IsFunctionNamed(fn, "hash/maphash", "Comparable") {
+		return false, true
+	}
+
+	return // unknown
 }

go/analysis/passes/ctrlflow/testdata/src/a/a.go

@@ -7,6 +7,7 @@ package a
 // This file tests facts produced by ctrlflow.
 
 import (
+	"hash/maphash"
 	"log"
 	"os"
 	"runtime"
@@ -192,3 +193,11 @@ func osexit() { // want osexit:"noReturn"
 	os.Exit(0)
 	print(2)
 }
+
+func intrinsic() { // (no fact)
+
+	// Comparable calls abi.EscapeNonString, whose body appears to panic;
+	// but that's a lie, as EscapeNonString is a compiler intrinsic.
+	// (go1.24 used a different intrinsic, maphash.escapeForHash.)
+	maphash.Comparable[int](maphash.Seed{}, 0)
+}

go/analysis/passes/inline/inline.go

@@ -7,7 +7,6 @@ package inline
 import (
 	"fmt"
 	"go/ast"
-	"go/token"
 	"go/types"
 	"slices"
 	"strings"
@@ -23,7 +22,6 @@ import (
 	"golang.org/x/tools/internal/analysis/analyzerutil"
 	typeindexanalyzer "golang.org/x/tools/internal/analysis/typeindex"
 	"golang.org/x/tools/internal/astutil"
-	"golang.org/x/tools/internal/diff"
 	"golang.org/x/tools/internal/moreiters"
 	"golang.org/x/tools/internal/packagepath"
 	"golang.org/x/tools/internal/refactor"
@@ -204,19 +202,12 @@ func (a *analyzer) inlineCall(call *ast.CallExpr, cur inspector.Cursor) {
 		var edits []analysis.TextEdit
 		if !lazyEdits {
 			// Inline the call.
-			content, err := a.readFile(call)
-			if err != nil {
-				a.pass.Reportf(call.Lparen, "invalid inlining candidate: cannot read source file: %v", err)
-				return
-			}
-			curFile := astutil.EnclosingFile(cur)
 			caller := &inline.Caller{
-				Fset:    a.pass.Fset,
-				Types:   a.pass.Pkg,
-				Info:    a.pass.TypesInfo,
-				File:    curFile,
-				Call:    call,
-				Content: content,
+				Fset:  a.pass.Fset,
+				Types: a.pass.Pkg,
+				Info:  a.pass.TypesInfo,
+				File:  astutil.EnclosingFile(cur),
+				Call:  call,
 				CountUses: func(pkgname *types.PkgName) int {
 					return moreiters.Len(a.index.Uses(pkgname))
 				},
@@ -245,15 +236,7 @@ func (a *analyzer) inlineCall(call *ast.CallExpr, cur inspector.Cursor) {
 				// The flag allows them to decline such fixes.
 				return
 			}
-			got := res.Content
-
-			for _, edit := range diff.Bytes(content, got) {
-				edits = append(edits, analysis.TextEdit{
-					Pos:     curFile.FileStart + token.Pos(edit.Start),
-					End:     curFile.FileStart + token.Pos(edit.End),
-					NewText: []byte(edit.New),
-				})
-			}
+			edits = res.Edits
 		}
 
 		a.pass.Report(analysis.Diagnostic{

go/analysis/passes/modernize/doc.go

@@ -199,12 +199,19 @@ are often used to express optionality.
 
 omitzero: suggest replacing omitempty with omitzero for struct fields
 
-The omitzero analyzer identifies uses of the `omitempty` JSON struct tag on
-fields that are themselves structs. The `omitempty` tag has no effect on
-struct-typed fields. The analyzer offers two suggestions: either remove the
+The omitzero analyzer identifies uses of the `omitempty` JSON struct
+tag on fields that are themselves structs. For struct-typed fields,
+the `omitempty` tag has no effect on the behavior of json.Marshal and
+json.Unmarshal. The analyzer offers two suggestions: either remove the
 tag, or replace it with `omitzero` (added in Go 1.24), which correctly
 omits the field if the struct value is zero.
 
+However, some other serialization packages (notably kubebuilder, see
+https://book.kubebuilder.io/reference/markers.html) may have their own
+interpretation of the `json:",omitzero"` tag, so removing it may affect
+program behavior. For this reason, the omitzero modernizer will not
+make changes in any package that contains +kubebuilder annotations.
+
 Replacing `omitempty` with `omitzero` is a change in behavior. The
 original code would always encode the struct field, whereas the
 modified code will omit it if it is a zero-value.
@@ -475,6 +482,22 @@ with a single call to t.Context(), which was added in Go 1.24.
 This change is only suggested if the `cancel` function is not used
 for any other purpose.
 
+# Analyzer unsafefuncs
+
+unsafefuncs: replace unsafe pointer arithmetic with function calls
+
+The unsafefuncs analyzer simplifies pointer arithmetic expressions by
+replacing them with calls to helper functions such as unsafe.Add,
+added in Go 1.17.
+
+Example:
+
+	unsafe.Pointer(uintptr(ptr) + uintptr(n))
+
+where ptr is an unsafe.Pointer, is replaced by:
+
+	unsafe.Add(ptr, n)
+
 # Analyzer waitgroup
 
 waitgroup: replace wg.Add(1)/go/wg.Done() with wg.Go

go/analysis/passes/modernize/forvar.go

@@ -35,7 +35,7 @@ var ForVarAnalyzer = &analysis.Analyzer{
 // where the two idents are the same,
 // and the ident is defined (:=) as a variable in the for statement.
 // (Note that this 'fix' does not work for three clause loops
-// because the Go specfilesUsingGoVersionsays "The variable used by each subsequent iteration
+// because the Go spec says "The variable used by each subsequent iteration
 // is declared implicitly before executing the post statement and initialized to the
 // value of the previous iteration's variable at that moment.")
 //

go/analysis/passes/modernize/maps.go

@@ -233,13 +233,16 @@ func mapsloop(pass *analysis.Pass) (any, error) {
 				assign := rng.Body.List[0].(*ast.AssignStmt)
 				if index, ok := assign.Lhs[0].(*ast.IndexExpr); ok &&
 					astutil.EqualSyntax(rng.Key, index.Index) &&
-					astutil.EqualSyntax(rng.Value, assign.Rhs[0]) &&
-					is[*types.Map](typeparams.CoreType(info.TypeOf(index.X))) &&
-					types.Identical(info.TypeOf(index), info.TypeOf(rng.Value)) { // m[k], v
+					astutil.EqualSyntax(rng.Value, assign.Rhs[0]) {
+					if tmap, ok := typeparams.CoreType(info.TypeOf(index.X)).(*types.Map); ok &&
+						types.Identical(info.TypeOf(index), info.TypeOf(rng.Value)) && // m[k], v
+						types.Identical(tmap.Key(), info.TypeOf(rng.Key)) {
 
-					// Have: for k, v := range x { m[k] = v }
-					// where there is no implicit conversion.
-					check(file, curRange, assign, index.X, rng.X)
+						// Have: for k, v := range x { m[k] = v }
+						// where there is no implicit conversion
+						// of either key or value.
+						check(file, curRange, assign, index.X, rng.X)
+					}
 				}
 			}
 		}

go/analysis/passes/modernize/modernize.go

@@ -53,6 +53,7 @@ var Suite = []*analysis.Analyzer{
 	StringsSeqAnalyzer,
 	StringsBuilderAnalyzer,
 	TestingContextAnalyzer,
+	unsafeFuncsAnalyzer,
 	WaitGroupAnalyzer,
 }
 

go/analysis/passes/modernize/modernize_test.go

@@ -55,7 +55,7 @@ func TestNewExpr(t *testing.T) {
 }
 
 func TestOmitZero(t *testing.T) {
-	RunWithSuggestedFixes(t, TestData(), modernize.OmitZeroAnalyzer, "omitzero")
+	RunWithSuggestedFixes(t, TestData(), modernize.OmitZeroAnalyzer, "omitzero/...")
 }
 
 func TestRangeInt(t *testing.T) {
@@ -109,6 +109,10 @@ func TestTestingContext(t *testing.T) {
 	RunWithSuggestedFixes(t, TestData(), modernize.TestingContextAnalyzer, "testingcontext")
 }
 
+func TestUnsafeFuncs(t *testing.T) {
+	RunWithSuggestedFixes(t, TestData(), goplsexport.UnsafeFuncsModernizer, "unsafefuncs")
+}
+
 func TestWaitGroup(t *testing.T) {
 	RunWithSuggestedFixes(t, TestData(), modernize.WaitGroupAnalyzer, "waitgroup")
 }