all: merge master (82473ce) into gopls-release-branch.0.19

Merge List:

+ 2025-06-04 82473ce934 gopls/doc/release: tweak v0.19
+ 2025-06-04 f3c581ff0c gopls/internal/protocol: add DocumentURI.Base accessor
+ 2025-06-04 d9bacab54d gopls/internal/server: improve "editing generated file" warning
+ 2025-06-04 1afeefa815 internal/mcp: unexport FileResourceHandler
+ 2025-06-04 33d59880f3 gopls/internal/server: Organize Imports of generated files
+ 2025-06-04 cb39a5f00b gopls/internal/golang: Format generated files
+ 2025-06-04 e43ca0ca5d internal/mcp: validate tool input schemas
+ 2025-06-04 61f37dc0fc gopls: use new gomodcache index
+ 2025-06-04 fed8cc83fd internal/refactor: keep comments with same import
+ 2025-06-03 c7873a32f0 gopls/internal/golang: eliminate dot import: skip keyed fields

Updates golang/go#73965

Change-Id: I0a76ac5e1fbfc367635a685ac3f69ff1ea874b99
diff --git a/gopls/doc/release/v0.19.0.md b/gopls/doc/release/v0.19.0.md
index 05aeb2e..a5e8956 100644
--- a/gopls/doc/release/v0.19.0.md
+++ b/gopls/doc/release/v0.19.0.md
@@ -1,71 +1,15 @@
 # Configuration Changes
 
-- The `gopls check` subcommant now accepts a `-severity` flag to set a minimum
+- The `gopls check` subcommand now accepts a `-severity` flag to set a minimum
   severity for the diagnostics it reports. By default, the minimum severity
   is "warning", so `gopls check` may report fewer diagnostics than before. Set
   `-severity=hint` to reproduce the previous behavior.
 
-# New features
+# Navigation features
 
-##  "Rename" of method receivers
+## "Implementations" supports signature types (within same package)
 
-The Rename operation, when applied to the declaration of a method
-receiver, now also attempts to rename the receivers of all other
-methods associated with the same named type. Each other receiver that
-cannot be fully renamed is quietly skipped.
-
-Renaming a _use_ of a method receiver continues to affect only that
-variable.
-
-```go
-type Counter struct { x int }
-
-                 Rename here to affect only this method
-                          ↓
-func (c *Counter) Inc() { c.x++ }
-func (c *Counter) Dec() { c.x++ }
-      ↑
-  Rename here to affect all methods
-```
-
-## Many `staticcheck` analyzers are enabled by default
-
-Slightly more than half of the analyzers in the
-[Staticcheck](https://staticcheck.dev/docs/checks) suite are now
-enabled by default. This subset has been chosen for precision and
-efficiency.
-
-Previously, Staticcheck analyzers (all of them) would be run only if
-the experimental `staticcheck` boolean option was set to `true`. This
-value continues to enable the complete set, and a value of `false`
-continues to disable the complete set. Leaving the option unspecified
-enables the preferred subset of analyzers.
-
-Staticcheck analyzers, like all other analyzers, can be explicitly
-enabled or disabled using the `analyzers` configuration setting; this
-setting now takes precedence over the `staticcheck` setting, so,
-regardless of what value of `staticcheck` you use (true/false/unset),
-you can make adjustments to your preferred set of analyzers.
-
-## "Inefficient recursive iterator" analyzer
-
-A common pitfall when writing a function that returns an iterator
-(iter.Seq) for a recursive data type is to recursively call the
-function from its own implementation, leading to a stack of nested
-coroutines, which is inefficient.
-
-The new `recursiveiter` analyzer detects such mistakes; see
-[https://golang.org/x/tools/gopls/internal/analysis/recursiveiter](its
-documentation) for details, including tips on how to define simple and
-efficient recursive iterators.
-
-##  "Inefficient range over maps.Keys/Values" analyzer
-
-This analyzer detects redundant calls to `maps.Keys` or `maps.Values`
-as the operand of a range loop; maps can of course be ranged over
-directly.
-
-##  "Implementations" supports signature types
+<!-- golang/go#56572 -->
 
 The Implementations query reports the correspondence between abstract
 and concrete types and their methods based on their method sets.
@@ -89,9 +33,11 @@
 and queries using signatures should be invoked on a `func` or `(` token.
 
 Only the local (same-package) algorithm is currently supported.
-TODO: implement global.
+(https://go.dev/issue/56572 tracks the global algorithm.)
 
-## Go to Implementation
+## "Go to Implementation" reports interface-to-interface relations
+
+<!-- golang/go#68641 -->
 
 The "Go to Implementation" operation now reports relationships between
 interfaces. Gopls now uses the concreteness of the query type to
@@ -126,19 +72,102 @@
 
 <img title="Type Hierarchy: subtypes of io.Writer" src="../assets/subtypes.png" width="400">
 
-## "Eliminate dot import" code action
 
-This code action, available on a dotted import, will offer to replace
-the import with a regular one and qualify each use of the package
-with its name.
+# Editing features
 
-### Auto-complete package clause for new Go files
+## Completion: auto-complete package clause for new Go files
 
 Gopls now automatically adds the appropriate `package` clause to newly created Go files,
 so that you can immediately get started writing the interesting part.
 
 It requires client support for `workspace/didCreateFiles`
 
+## New GOMODCACHE index for faster Organize Imports and unimported completions
+
+By default, gopls now builds and maintains a persistent index of
+packages in the module cache (GOMODCACHE). The operations of Organize
+Imports and completion of symbols from unimported pacakges are an
+order of magnitude faster.
+
+To revert to the old behavior, set the `importsSource` option (whose
+new default is `"gopls"`) to `"goimports"`. Users who don't want the
+module cache used at all for imports or completions can change the
+option to "off".
+
+# Analysis features
+
+## Most `staticcheck` analyzers are enabled by default
+
+Slightly more than half of the analyzers in the
+[Staticcheck](https://staticcheck.dev/docs/checks) suite are now
+enabled by default. This subset has been chosen for precision and
+efficiency.
+
+Previously, Staticcheck analyzers (all of them) would be run only if
+the experimental `staticcheck` boolean option was set to `true`. This
+value continues to enable the complete set, and a value of `false`
+continues to disable the complete set. Leaving the option unspecified
+enables the preferred subset of analyzers.
+
+Staticcheck analyzers, like all other analyzers, can be explicitly
+enabled or disabled using the `analyzers` configuration setting; this
+setting now takes precedence over the `staticcheck` setting, so,
+regardless of what value of `staticcheck` you use (true/false/unset),
+you can make adjustments to your preferred set of analyzers.
+
+## `recursiveiter`: "inefficient recursive iterator"
+
+A common pitfall when writing a function that returns an iterator
+(`iter.Seq`) for a recursive data type is to recursively call the
+function from its own implementation, leading to a stack of nested
+coroutines, which is inefficient.
+
+The new `recursiveiter` analyzer detects such mistakes; see
+[its documentation](https://golang.org/x/tools/gopls/internal/analysis/recursiveiter)
+for details, including tips on how to define simple and efficient
+recursive iterators.
+
+## `maprange`: "inefficient range over maps.Keys/Values"
+
+The new `maprange` analyzer detects redundant calls to `maps.Keys` or
+`maps.Values` as the operand of a range loop; maps can of course be
+ranged over directly. See
+[its documentation](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/maprange)
+for details).
+
+# Code transformation features
+
+## Rename method receivers
+
+<!-- golang/go#41892 -->
+
+The Rename operation, when applied to the declaration of a method
+receiver, now also attempts to rename the receivers of all other
+methods associated with the same named type. Each other receiver that
+cannot be fully renamed is quietly skipped.
+
+Renaming a _use_ of a method receiver continues to affect only that
+variable.
+
+```go
+type Counter struct { x int }
+
+                 Rename here to affect only this method
+                          ↓
+func (c *Counter) Inc() { c.x++ }
+func (c *Counter) Dec() { c.x++ }
+      ↑
+  Rename here to affect all methods
+```
+
+## "Eliminate dot import" code action
+
+<!-- golang/go#70319 -->
+
+This code action, available on a dotted import, will offer to replace
+the import with a regular one and qualify each use of the package
+with its name.
+
 ## Add/remove tags from struct fields
 
 Gopls now provides two new code actions, available on an entire struct
@@ -156,6 +185,8 @@
 
 ## Inline local variable
 
+<!-- golang/go#70085 -->
+
 The new `refactor.inline.variable` code action replaces a reference to
 a local variable by that variable's initializer expression. For
 example, when applied to `s` in `println(s)`:
@@ -173,3 +204,7 @@
 	println(fmt.Sprintf("+%d", x))
 }
 ```
+
+Only a single reference is replaced; issue https://go.dev/issue/70085
+tracks the feature to "inline all" uses of the variable and eliminate
+it.
diff --git a/gopls/internal/cache/snapshot.go b/gopls/internal/cache/snapshot.go
index e78c1bb..49707d0 100644
--- a/gopls/internal/cache/snapshot.go
+++ b/gopls/internal/cache/snapshot.go
@@ -1421,7 +1421,7 @@
 					fix = `This file may be excluded due to its build tags; try adding "-tags=<build tag>" to your gopls "buildFlags" configuration
 See the documentation for more information on working with build tags:
 https://github.com/golang/tools/blob/master/gopls/doc/settings.md#buildflags.`
-				} else if strings.Contains(filepath.Base(fh.URI().Path()), "_") {
+				} else if strings.Contains(fh.URI().Base(), "_") {
 					fix = `This file may be excluded due to its GOOS/GOARCH, or other build constraints.`
 				} else {
 					fix = `This file is ignored by your gopls build.` // we don't know why
diff --git a/gopls/internal/cache/workspace.go b/gopls/internal/cache/workspace.go
index 0621d17..6b2291e 100644
--- a/gopls/internal/cache/workspace.go
+++ b/gopls/internal/cache/workspace.go
@@ -18,7 +18,7 @@
 
 // isGoWork reports if uri is a go.work file.
 func isGoWork(uri protocol.DocumentURI) bool {
-	return filepath.Base(uri.Path()) == "go.work"
+	return uri.Base() == "go.work"
 }
 
 // goWorkModules returns the URIs of go.mod files named by the go.work file.
@@ -63,7 +63,7 @@
 
 // isGoMod reports if uri is a go.mod file.
 func isGoMod(uri protocol.DocumentURI) bool {
-	return filepath.Base(uri.Path()) == "go.mod"
+	return uri.Base() == "go.mod"
 }
 
 // isWorkspaceFile reports if uri matches a set of globs defined in workspaceFiles
diff --git a/gopls/internal/cmd/cmd.go b/gopls/internal/cmd/cmd.go
index f4cfd99..d057698 100644
--- a/gopls/internal/cmd/cmd.go
+++ b/gopls/internal/cmd/cmd.go
@@ -903,7 +903,7 @@
 	// case-sensitive directories. The authoritative answer
 	// requires querying the file system, and we don't want
 	// to do that.
-	if !strings.EqualFold(filepath.Base(string(f.mapper.URI)), filepath.Base(string(s.URI()))) {
+	if !strings.EqualFold(f.mapper.URI.Base(), s.URI().Base()) {
 		return protocol.Range{}, bugpkg.Errorf("mapper is for file %q instead of %q", f.mapper.URI, s.URI())
 	}
 	start, err := pointPosition(f.mapper, s.Start())
diff --git a/gopls/internal/golang/addtest.go b/gopls/internal/golang/addtest.go
index 73665ce..dfd7831 100644
--- a/gopls/internal/golang/addtest.go
+++ b/gopls/internal/golang/addtest.go
@@ -265,7 +265,7 @@
 		return nil, err
 	}
 
-	testBase := strings.TrimSuffix(filepath.Base(loc.URI.Path()), ".go") + "_test.go"
+	testBase := strings.TrimSuffix(loc.URI.Base(), ".go") + "_test.go"
 	goTestFileURI := protocol.URIFromPath(filepath.Join(loc.URI.DirPath(), testBase))
 
 	testFH, err := snapshot.ReadFile(ctx, goTestFileURI)
diff --git a/gopls/internal/golang/call_hierarchy.go b/gopls/internal/golang/call_hierarchy.go
index 1193d7e..00bc021 100644
--- a/gopls/internal/golang/call_hierarchy.go
+++ b/gopls/internal/golang/call_hierarchy.go
@@ -11,7 +11,6 @@
 	"go/ast"
 	"go/token"
 	"go/types"
-	"path/filepath"
 
 	"golang.org/x/tools/go/ast/astutil"
 	"golang.org/x/tools/go/types/typeutil"
@@ -59,7 +58,7 @@
 		Name:           obj.Name(),
 		Kind:           protocol.Function,
 		Tags:           []protocol.SymbolTag{},
-		Detail:         fmt.Sprintf("%s • %s", obj.Pkg().Path(), filepath.Base(declLoc.URI.Path())),
+		Detail:         fmt.Sprintf("%s • %s", obj.Pkg().Path(), declLoc.URI.Base()),
 		URI:            declLoc.URI,
 		Range:          rng,
 		SelectionRange: rng,
@@ -182,7 +181,7 @@
 		Name:           name,
 		Kind:           kind,
 		Tags:           []protocol.SymbolTag{},
-		Detail:         fmt.Sprintf("%s • %s", pkgPath, filepath.Base(fh.URI().Path())),
+		Detail:         fmt.Sprintf("%s • %s", pkgPath, fh.URI().Base()),
 		URI:            loc.URI,
 		Range:          rng,
 		SelectionRange: rng,
@@ -283,7 +282,7 @@
 					Name:           obj.Name(),
 					Kind:           protocol.Function,
 					Tags:           []protocol.SymbolTag{},
-					Detail:         fmt.Sprintf("%s • %s", obj.Pkg().Path(), filepath.Base(loc.URI.Path())),
+					Detail:         fmt.Sprintf("%s • %s", obj.Pkg().Path(), loc.URI.Base()),
 					URI:            loc.URI,
 					Range:          loc.Range,
 					SelectionRange: loc.Range,
diff --git a/gopls/internal/golang/codeaction.go b/gopls/internal/golang/codeaction.go
index 703b06b..7a92127 100644
--- a/gopls/internal/golang/codeaction.go
+++ b/gopls/internal/golang/codeaction.go
@@ -11,12 +11,12 @@
 	"go/ast"
 	"go/token"
 	"go/types"
-	"path/filepath"
 	"reflect"
 	"slices"
 	"strings"
 
 	"golang.org/x/tools/go/ast/astutil"
+	"golang.org/x/tools/go/ast/edge"
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/gopls/internal/analysis/fillstruct"
 	"golang.org/x/tools/gopls/internal/analysis/fillswitch"
@@ -733,11 +733,15 @@
 			continue
 		}
 
-		// Only qualify unqualified identifiers (due to dot imports).
+		// Only qualify unqualified identifiers (due to dot imports)
+		// that reference package-level symbols.
 		// All other references to a symbol imported from another package
 		// are nested within a select expression (pkg.Foo, v.Method, v.Field).
-		if is[*ast.SelectorExpr](curId.Parent().Node()) {
-			continue
+		if ek, _ := curId.ParentEdge(); ek == edge.SelectorExpr_Sel {
+			continue // qualified identifier (pkg.X) or selector (T.X or e.X)
+		}
+		if !typesinternal.IsPackageLevel(use) {
+			continue // unqualified field reference T{X: ...}
 		}
 
 		// Make sure that the package name will not be shadowed by something else in scope.
@@ -1103,7 +1107,7 @@
 
 		title := fmt.Sprintf("%s compiler optimization details for %q",
 			cond(req.snapshot.WantCompilerOptDetails(dir), "Hide", "Show"),
-			filepath.Base(dir.Path()))
+			dir.Base())
 		cmd := command.NewGCDetailsCommand(title, req.fh.URI())
 		req.addCommandAction(cmd, false)
 	}
diff --git a/gopls/internal/golang/completion/unimported.go b/gopls/internal/golang/completion/unimported.go
index e562f5c..f4dbcde 100644
--- a/gopls/internal/golang/completion/unimported.go
+++ b/gopls/internal/golang/completion/unimported.go
@@ -52,21 +52,6 @@
 		}
 	}
 	// do the stdlib next.
-	// For now, use the workspace version of stdlib packages
-	// to get function snippets. CL 665335 will fix this.
-	var x []metadata.PackageID
-	for _, mp := range stdpkgs {
-		if slices.Contains(wsIDs, metadata.PackageID(mp)) {
-			x = append(x, metadata.PackageID(mp))
-		}
-	}
-	if len(x) > 0 {
-		items := c.pkgIDmatches(ctx, x, pkgname, prefix)
-		if c.scoreList(items) {
-			return
-		}
-	}
-	// just use the stdlib
 	items := c.stdlibMatches(stdpkgs, pkgname, prefix)
 	if c.scoreList(items) {
 		return
@@ -164,7 +149,7 @@
 					}
 					kind = protocol.FunctionCompletion
 					detail = fmt.Sprintf("func (from %q)", pkg.PkgPath)
-				case protocol.Variable:
+				case protocol.Variable, protocol.Struct:
 					kind = protocol.VariableCompletion
 					detail = fmt.Sprintf("var (from %q)", pkg.PkgPath)
 				case protocol.Constant:
@@ -264,6 +249,9 @@
 		case modindex.Const:
 			kind = protocol.ConstantCompletion
 			detail = fmt.Sprintf("const (from %s)", cand.ImportPath)
+		case modindex.Type: // might be a type alias
+			kind = protocol.VariableCompletion
+			detail = fmt.Sprintf("type (from %s)", cand.ImportPath)
 		default:
 			continue
 		}
diff --git a/gopls/internal/golang/format.go b/gopls/internal/golang/format.go
index ef98580..fc3b2e3 100644
--- a/gopls/internal/golang/format.go
+++ b/gopls/internal/golang/format.go
@@ -40,11 +40,6 @@
 		return nil, err
 	}
 
-	// Generated files shouldn't be edited. So, don't format them.
-	if ast.IsGenerated(pgf.File) {
-		return nil, fmt.Errorf("can't format %q: file is generated", fh.URI().Path())
-	}
-
 	// Even if this file has parse errors, it might still be possible to format it.
 	// Using format.Node on an AST with errors may result in code being modified.
 	// Attempt to format the source of this file instead.
diff --git a/gopls/internal/mcp/context.go b/gopls/internal/mcp/context.go
index 0691191..83c7633 100644
--- a/gopls/internal/mcp/context.go
+++ b/gopls/internal/mcp/context.go
@@ -13,7 +13,6 @@
 	"fmt"
 	"go/ast"
 	"go/token"
-	"path/filepath"
 	"slices"
 	"strings"
 
@@ -58,7 +57,7 @@
 	fmt.Fprintf(&result, "Current package %q (package %s) declares the following symbols:\n\n", pkg.Metadata().PkgPath, pkg.Metadata().Name)
 	// Write context of the current file.
 	{
-		fmt.Fprintf(&result, "%s (current file):\n", filepath.Base(pgf.URI.Path()))
+		fmt.Fprintf(&result, "%s (current file):\n", pgf.URI.Base())
 		result.WriteString("--->\n")
 		if err := writeFileSummary(ctx, snapshot, pgf.URI, &result, false); err != nil {
 			return nil, err
@@ -73,7 +72,7 @@
 				continue
 			}
 
-			fmt.Fprintf(&result, "%s:\n", filepath.Base(file.URI.Path()))
+			fmt.Fprintf(&result, "%s:\n", file.URI.Base())
 			result.WriteString("--->\n")
 			if err := writeFileSummary(ctx, snapshot, file.URI, &result, false); err != nil {
 				return nil, err
@@ -86,7 +85,7 @@
 	if len(pgf.File.Imports) > 0 {
 		// Write import decls of the current file.
 		{
-			fmt.Fprintf(&result, "Current file %q contains this import declaration:\n", filepath.Base(pgf.URI.Path()))
+			fmt.Fprintf(&result, "Current file %q contains this import declaration:\n", pgf.URI.Base())
 			result.WriteString("--->\n")
 			// Add all import decl to output including all floating comment by
 			// using GenDecl's start and end position.
@@ -127,7 +126,7 @@
 
 				fmt.Fprintf(&result, "%q (package %s)\n", importPath, impMetadata.Name)
 				for _, f := range impMetadata.CompiledGoFiles {
-					fmt.Fprintf(&result, "%s:\n", filepath.Base(f.Path()))
+					fmt.Fprintf(&result, "%s:\n", f.Base())
 					result.WriteString("--->\n")
 					if err := writeFileSummary(ctx, snapshot, f, &result, true); err != nil {
 						return nil, err
diff --git a/gopls/internal/protocol/uri.go b/gopls/internal/protocol/uri.go
index 361bc44..6615210 100644
--- a/gopls/internal/protocol/uri.go
+++ b/gopls/internal/protocol/uri.go
@@ -91,6 +91,11 @@
 	return filepath.FromSlash(filename)
 }
 
+// Base returns the base name of the file path of the given URI.
+func (uri DocumentURI) Base() string {
+	return filepath.Base(uri.Path())
+}
+
 // Dir returns the URI for the directory containing the receiver.
 func (uri DocumentURI) Dir() DocumentURI {
 	// This function could be more efficiently implemented by avoiding any call
diff --git a/gopls/internal/server/code_action.go b/gopls/internal/server/code_action.go
index e37cfc9..e0fcd3e 100644
--- a/gopls/internal/server/code_action.go
+++ b/gopls/internal/server/code_action.go
@@ -180,10 +180,16 @@
 		}
 		actions = append(actions, moreActions...)
 
-		// Don't suggest fixes for generated files, since they are generally
+		// Don't suggest most fixes for generated files, since they are generally
 		// not useful and some editors may apply them automatically on save.
 		// (Unfortunately there's no reliable way to distinguish fixes from
 		// queries, so we must list all kinds of queries here.)
+		//
+		// We make an exception for OrganizeImports, because
+		// (a) it is needed when making temporary experimental
+		//     changes (e.g. adding logging) in generated files, and
+		// (b) it doesn't report diagnostics on well-formed code, and
+		//     unedited generated files must be well formed.
 		if golang.IsGenerated(ctx, snapshot, uri) {
 			actions = slices.DeleteFunc(actions, func(a protocol.CodeAction) bool {
 				switch a.Kind {
@@ -194,6 +200,8 @@
 					settings.GoplsDocFeatures,
 					settings.GoToggleCompilerOptDetails:
 					return false // read-only query
+				case settings.OrganizeImports:
+					return false // fix allowed in generated files (see #73959)
 				}
 				return true // potential write operation
 			})
diff --git a/gopls/internal/server/command.go b/gopls/internal/server/command.go
index 41de2cd..5c7427f 100644
--- a/gopls/internal/server/command.go
+++ b/gopls/internal/server/command.go
@@ -91,7 +91,7 @@
 			return false // "can't happen" (see prior Encloses check)
 		}
 
-		assert(filepath.Base(goMod.Path()) == "go.mod", fmt.Sprintf("invalid go.mod path: want go.mod, got %q", goMod.Path()))
+		assert(goMod.Base() == "go.mod", fmt.Sprintf("invalid go.mod path: want go.mod, got %q", goMod.Path()))
 
 		// Invariant: rel is a relative path without "../" segments and the last
 		// segment is "go.mod"
diff --git a/gopls/internal/server/text_synchronization.go b/gopls/internal/server/text_synchronization.go
index 982d0e7..a993598 100644
--- a/gopls/internal/server/text_synchronization.go
+++ b/gopls/internal/server/text_synchronization.go
@@ -156,18 +156,19 @@
 		return nil
 	}
 
-	// Ideally, we should be able to specify that a generated file should
-	// be opened as read-only. Tell the user that they should not be
-	// editing a generated file.
+	// Warn the user that they are editing a generated file, but
+	// don't try to stop them: there are often good reasons to do
+	// so, such as adding temporary logging, or evaluating changes
+	// to the generated code without the trouble of modifying the
+	// generator logic (see #73959).
 	snapshot, release, err := s.session.SnapshotOf(ctx, uri)
 	if err != nil {
 		return err
 	}
 	isGenerated := golang.IsGenerated(ctx, snapshot, uri)
 	release()
-
 	if isGenerated {
-		msg := fmt.Sprintf("Do not edit this file! %s is a generated file.", uri.Path())
+		msg := fmt.Sprintf("Warning: editing %s, a generated file.", uri.Base())
 		showMessage(ctx, s.client, protocol.Warning, msg)
 	}
 	return nil
diff --git a/gopls/internal/settings/default.go b/gopls/internal/settings/default.go
index 70adc1a..744e8d5 100644
--- a/gopls/internal/settings/default.go
+++ b/gopls/internal/settings/default.go
@@ -39,7 +39,7 @@
 				DynamicWatchedFilesSupported:               true,
 				LineFoldingOnly:                            false,
 				HierarchicalDocumentSymbolSupport:          true,
-				ImportsSource:                              ImportsSourceGoimports,
+				ImportsSource:                              ImportsSourceGopls,
 			},
 			ServerOptions: ServerOptions{
 				SupportedCodeActions: map[file.Kind]map[protocol.CodeActionKind]bool{
diff --git a/gopls/internal/test/integration/bench/unimported_test.go b/gopls/internal/test/integration/bench/unimported_test.go
new file mode 100644
index 0000000..9d7139b
--- /dev/null
+++ b/gopls/internal/test/integration/bench/unimported_test.go
@@ -0,0 +1,161 @@
+// Copyright 2025 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package bench
+
+import (
+	"context"
+	"fmt"
+	"go/token"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	. "golang.org/x/tools/gopls/internal/test/integration"
+	"golang.org/x/tools/gopls/internal/test/integration/fake"
+	"golang.org/x/tools/internal/modindex"
+)
+
+// experiments show the new code about 15 times faster than the old,
+// and the old code sometimes fails to find the completion
+func BenchmarkLocalModcache(b *testing.B) {
+	budgets := []string{"0s", "100ms", "200ms", "500ms", "1s", "5s"}
+	sources := []string{"gopls", "goimports"}
+	for _, budget := range budgets {
+		b.Run(fmt.Sprintf("budget=%s", budget), func(b *testing.B) {
+			for _, source := range sources {
+				b.Run(fmt.Sprintf("source=%s", source), func(b *testing.B) {
+					runModcacheCompletion(b, budget, source)
+				})
+			}
+		})
+	}
+}
+
+func runModcacheCompletion(b *testing.B, budget, source string) {
+	// First set up the program to be edited
+	gomod := `
+module mod.com
+
+go 1.21
+`
+	pat := `
+package main
+var _ = %s.%s
+`
+	pkg, name, modcache := findSym(b)
+	name, _, _ = strings.Cut(name, " ")
+	mainfile := fmt.Sprintf(pat, pkg, name)
+	// Second, create the Env and start gopls
+	dir := getTempDir()
+	if err := os.Mkdir(dir, 0750); err != nil {
+		if !os.IsExist(err) {
+			b.Fatal(err)
+		}
+	}
+	defer os.RemoveAll(dir) // is this right? needed?
+	if err := os.WriteFile(filepath.Join(dir, "go.mod"), []byte(gomod), 0644); err != nil {
+		b.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(dir, "main.go"), []byte(mainfile), 0644); err != nil {
+		b.Fatal(err)
+	}
+	ts, err := newGoplsConnector(nil)
+	if err != nil {
+		b.Fatal(err)
+	}
+	// PJW: put better EditorConfig here
+	envvars := map[string]string{
+		"GOMODCACHE": modcache,
+		//"GOPATH":     sandbox.GOPATH(), // do we need a GOPATH?
+	}
+	fc := fake.EditorConfig{
+		Env: envvars,
+		Settings: map[string]any{
+			"completeUnimported": true,
+			"completionBudget":   budget, // "0s", "100ms"
+			"importsSource":      source, // "gopls" or "goimports"
+		},
+	}
+	sandbox, editor, awaiter, err := connectEditor(dir, fc, ts)
+	if err != nil {
+		b.Fatal(err)
+	}
+	defer sandbox.Close()
+	defer editor.Close(context.Background())
+	if err := awaiter.Await(context.Background(), InitialWorkspaceLoad); err != nil {
+		b.Fatal(err)
+	}
+	env := &Env{
+		TB:      b,
+		Ctx:     context.Background(),
+		Editor:  editor,
+		Sandbox: sandbox,
+		Awaiter: awaiter,
+	}
+	// Check that completion works as expected
+	env.CreateBuffer("main.go", mainfile)
+	env.AfterChange()
+	if false { // warm up? or not?
+		loc := env.RegexpSearch("main.go", name)
+		completions := env.Completion(loc)
+		if len(completions.Items) == 0 {
+			b.Fatal("no completions")
+		}
+	}
+
+	// run benchmark
+	for b.Loop() {
+		loc := env.RegexpSearch("main.go", name)
+		env.Completion(loc)
+	}
+}
+
+// find some symbol in the module cache
+func findSym(t testing.TB) (pkg, name, gomodcache string) {
+	initForTest(t)
+	cmd := exec.Command("go", "env", "GOMODCACHE")
+	out, err := cmd.Output()
+	if err != nil {
+		t.Fatal(err)
+	}
+	modcache := strings.TrimSpace(string(out))
+	ix, err := modindex.ReadIndex(modcache)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ix == nil {
+		t.Fatal("no index")
+	}
+	if len(ix.Entries) == 0 {
+		t.Fatal("no entries")
+	}
+	nth := 100 // or something
+	for _, e := range ix.Entries {
+		if token.IsExported(e.PkgName) || strings.HasPrefix(e.PkgName, "_") {
+			continue // weird stuff in module cache
+		}
+
+		for _, nm := range e.Names {
+			nth--
+			if nth == 0 {
+				return e.PkgName, nm, modcache
+			}
+		}
+	}
+	t.Fatalf("index doesn't have enough usable names, need another %d", nth)
+	return "", "", modcache
+}
+
+// Set IndexDir, avoiding the special case for tests,
+func initForTest(t testing.TB) {
+	dir, err := os.UserCacheDir()
+	if err != nil {
+		t.Fatalf("os.UserCacheDir: %v", err)
+	}
+	dir = filepath.Join(dir, "go", "imports")
+	modindex.IndexDir = dir
+}
diff --git a/gopls/internal/test/integration/misc/definition_test.go b/gopls/internal/test/integration/misc/definition_test.go
index 8a9f27d..d53e658 100644
--- a/gopls/internal/test/integration/misc/definition_test.go
+++ b/gopls/internal/test/integration/misc/definition_test.go
@@ -637,7 +637,7 @@
 		env.OpenFile("a.go")
 
 		locString := func(loc protocol.Location) string {
-			return fmt.Sprintf("%s:%s", filepath.Base(loc.URI.Path()), loc.Range)
+			return fmt.Sprintf("%s:%s", loc.URI.Base(), loc.Range)
 		}
 
 		// Definition at the call"foo(123)" takes us to the Go declaration.
diff --git a/gopls/internal/test/integration/misc/formatting_test.go b/gopls/internal/test/integration/misc/formatting_test.go
index a0f86d3..190833c 100644
--- a/gopls/internal/test/integration/misc/formatting_test.go
+++ b/gopls/internal/test/integration/misc/formatting_test.go
@@ -268,39 +268,6 @@
 	}
 }
 
-func TestFormattingOfGeneratedFile_Issue49555(t *testing.T) {
-	const input = `
--- main.go --
-// Code generated by generator.go. DO NOT EDIT.
-
-package main
-
-import "fmt"
-
-func main() {
-
-
-
-
-	fmt.Print("hello")
-}
-`
-
-	Run(t, input, func(t *testing.T, env *Env) {
-		wantErrSuffix := "file is generated"
-
-		env.OpenFile("main.go")
-		err := env.Editor.FormatBuffer(env.Ctx, "main.go")
-		if err == nil {
-			t.Fatal("expected error, got nil")
-		}
-		// Check only the suffix because an error contains a dynamic path to main.go
-		if !strings.HasSuffix(err.Error(), wantErrSuffix) {
-			t.Fatalf("unexpected error %q, want suffix %q", err.Error(), wantErrSuffix)
-		}
-	})
-}
-
 func TestGofumptFormatting(t *testing.T) {
 	// Exercise some gofumpt formatting rules:
 	//  - No empty lines following an assignment operator
diff --git a/gopls/internal/test/integration/misc/generate_test.go b/gopls/internal/test/integration/misc/generate_test.go
index 548f3bd..f5fe226 100644
--- a/gopls/internal/test/integration/misc/generate_test.go
+++ b/gopls/internal/test/integration/misc/generate_test.go
@@ -103,3 +103,33 @@
 			env.RunGenerate("./")
 		})
 }
+
+func TestEditingGeneratedFileWarning(t *testing.T) {
+	const src = `
+-- go.mod --
+module example.com
+go 1.21
+
+-- a/a.go --
+// Code generated by me. DO NOT EDIT.
+
+package a
+
+var x = 1
+`
+	Run(t, src, func(t *testing.T, env *Env) {
+		env.OpenFile("a/a.go")
+		env.RegexpReplace("a/a.go", "var", "const")
+		collectMessages := env.Awaiter.ListenToShownMessages()
+		env.Await(env.DoneWithChange())
+		messages := collectMessages()
+
+		const want = "Warning: editing a.go, a generated file."
+		if len(messages) != 1 || messages[0].Message != want {
+			for _, message := range messages {
+				t.Errorf("got message %q", message.Message)
+			}
+			t.Errorf("no %q warning", want)
+		}
+	})
+}
diff --git a/gopls/internal/test/integration/modfile/modfile_test.go b/gopls/internal/test/integration/modfile/modfile_test.go
index 36ed9cf..c6639db 100644
--- a/gopls/internal/test/integration/modfile/modfile_test.go
+++ b/gopls/internal/test/integration/modfile/modfile_test.go
@@ -6,7 +6,6 @@
 
 import (
 	"os"
-	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
@@ -870,7 +869,7 @@
 		// Confirm that we still have metadata with only on-disk edits.
 		env.OpenFile("main.go")
 		loc := env.FirstDefinition(env.RegexpSearch("main.go", "hello"))
-		if filepath.Base(string(loc.URI)) != "hello.go" {
+		if loc.URI.Base() != "hello.go" {
 			t.Fatalf("expected definition in hello.go, got %s", loc.URI)
 		}
 		// Confirm that we no longer have metadata when the file is saved.
diff --git a/gopls/internal/test/marker/testdata/codeaction/eliminate_dot_import.txt b/gopls/internal/test/marker/testdata/codeaction/eliminate_dot_import.txt
index e72d8bd..f2e4d58 100644
--- a/gopls/internal/test/marker/testdata/codeaction/eliminate_dot_import.txt
+++ b/gopls/internal/test/marker/testdata/codeaction/eliminate_dot_import.txt
@@ -1,7 +1,7 @@
 This test checks the behavior of the 'remove dot import' code action.
 
 -- go.mod --
-module golang.org/lsptests/removedotimport
+module example.com
 
 go 1.18
 
@@ -13,6 +13,7 @@
 import (
 	. "fmt" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a1)
 	. "bytes" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a2)
+	. "time" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a3)
 )
 
 var _ = a
@@ -22,19 +23,28 @@
 
 	buf := NewBuffer(nil)
 	buf.Grow(10)
+
+	_ = Ticker{C: nil}
 }
 
 -- @a1/a.go --
 @@ -6 +6 @@
 -	. "fmt" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a1)
 +	"fmt" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a1)
-@@ -13 +13 @@
+@@ -14 +14 @@
 -	Println("hello")
 +	fmt.Println("hello")
 -- @a2/a.go --
 @@ -7 +7 @@
 -	. "bytes" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a2)
 +	"bytes" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a2)
-@@ -15 +15 @@
+@@ -16 +16 @@
 -	buf := NewBuffer(nil)
 +	buf := bytes.NewBuffer(nil)
+-- @a3/a.go --
+@@ -8 +8 @@
+-	. "time" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a3)
++	"time" //@codeaction(`.`, "refactor.rewrite.eliminateDotImport", edit=a3)
+@@ -19 +19 @@
+-	_ = Ticker{C: nil}
++	_ = time.Ticker{C: nil}
diff --git a/gopls/internal/test/marker/testdata/codeaction/imports-generated.txt b/gopls/internal/test/marker/testdata/codeaction/imports-generated.txt
new file mode 100644
index 0000000..879ea6c
--- /dev/null
+++ b/gopls/internal/test/marker/testdata/codeaction/imports-generated.txt
@@ -0,0 +1,27 @@
+This test verifies that the 'source.organizeImports' code action
+is offered in generated files (see #73959).
+
+-- go.mod --
+module example.com
+go 1.21
+
+-- a.go --
+// Code generated by me. DO NOT EDIT.
+
+package a //@codeaction("a", "source.organizeImports", result=out)
+
+func _() {
+	fmt.Println("hello") //@diag("fmt", re"undefined")
+}
+
+-- @out/a.go --
+// Code generated by me. DO NOT EDIT.
+
+package a //@codeaction("a", "source.organizeImports", result=out)
+
+import "fmt"
+
+func _() {
+	fmt.Println("hello") //@diag("fmt", re"undefined")
+}
+
diff --git a/gopls/internal/test/marker/testdata/completion/issue62676.txt b/gopls/internal/test/marker/testdata/completion/issue62676.txt
index af4c3b6..6251e94 100644
--- a/gopls/internal/test/marker/testdata/completion/issue62676.txt
+++ b/gopls/internal/test/marker/testdata/completion/issue62676.txt
@@ -53,7 +53,7 @@
 
 func _() {
 	// This uses goimports-based completion; TODO: this should insert snippets.
-	os.Open //@acceptcompletion(re"Open()", "Open", open)
+	os.Open(${1:}) //@acceptcompletion(re"Open()", "Open", open)
 }
 
 func _() {
diff --git a/gopls/internal/test/marker/testdata/format/generated.txt b/gopls/internal/test/marker/testdata/format/generated.txt
new file mode 100644
index 0000000..3d571e0
--- /dev/null
+++ b/gopls/internal/test/marker/testdata/format/generated.txt
@@ -0,0 +1,29 @@
+This test checks that formatting includes generated files too
+(reversing https://go.dev/cl/365295 to address issue #49555).
+
+See https://github.com/golang/go/issues/73959.
+
+-- flags --
+-ignore_extra_diags
+
+-- go.mod --
+module example.com
+go 1.21
+
+-- a/a.go --
+// Code generated by me. DO NOT EDIT.
+
+package a; import "fmt"; func main() { fmt.Println("hello") }
+
+//@format(out)
+
+-- @out --
+// Code generated by me. DO NOT EDIT.
+
+package a
+
+import "fmt"
+
+func main() { fmt.Println("hello") }
+
+//@format(out)
diff --git a/internal/mcp/jsonschema/infer.go b/internal/mcp/jsonschema/infer.go
index 4ce270e..f044996 100644
--- a/internal/mcp/jsonschema/infer.go
+++ b/internal/mcp/jsonschema/infer.go
@@ -42,15 +42,21 @@
 //   - complex numbers
 //   - unsafe pointers
 //
+// The cannot be any cycles in the types.
 // TODO(rfindley): we could perhaps just skip these incompatible fields.
 func ForType(t reflect.Type) (*Schema, error) {
 	return typeSchema(t)
 }
 
 func typeSchema(t reflect.Type) (*Schema, error) {
-	if t.Kind() == reflect.Pointer {
+	// Follow pointers: the schema for *T is almost the same as for T, except that
+	// an explicit JSON "null" is allowed for the pointer.
+	allowNull := false
+	for t.Kind() == reflect.Pointer {
+		allowNull = true
 		t = t.Elem()
 	}
+
 	var (
 		s   = new(Schema)
 		err error
@@ -121,6 +127,10 @@
 	default:
 		return nil, fmt.Errorf("type %v is unsupported by jsonschema", t)
 	}
+	if allowNull && s.Type != "" {
+		s.Types = []string{"null", s.Type}
+		s.Type = ""
+	}
 	return s, nil
 }
 
diff --git a/internal/mcp/jsonschema/infer_test.go b/internal/mcp/jsonschema/infer_test.go
index 150824c..b695d21 100644
--- a/internal/mcp/jsonschema/infer_test.go
+++ b/internal/mcp/jsonschema/infer_test.go
@@ -57,7 +57,7 @@
 				Properties: map[string]*schema{
 					"f":      {Type: "integer"},
 					"G":      {Type: "array", Items: &schema{Type: "number"}},
-					"P":      {Type: "boolean"},
+					"P":      {Types: []string{"null", "boolean"}},
 					"NoSkip": {Type: "string"},
 				},
 				Required:             []string{"f", "G", "P"},
diff --git a/internal/mcp/jsonschema/resolve.go b/internal/mcp/jsonschema/resolve.go
index 1754135..0f913d8 100644
--- a/internal/mcp/jsonschema/resolve.go
+++ b/internal/mcp/jsonschema/resolve.go
@@ -27,6 +27,10 @@
 	resolvedURIs map[string]*Schema
 }
 
+// Schema returns the schema that was resolved.
+// It must not be modified.
+func (r *Resolved) Schema() *Schema { return r.root }
+
 // A Loader reads and unmarshals the schema at uri, if any.
 type Loader func(uri *url.URL) (*Schema, error)
 
diff --git a/internal/mcp/jsonschema/util.go b/internal/mcp/jsonschema/util.go
index 58c11ff..5507002 100644
--- a/internal/mcp/jsonschema/util.go
+++ b/internal/mcp/jsonschema/util.go
@@ -270,7 +270,7 @@
 		return "string", true
 	case reflect.Slice, reflect.Array:
 		return "array", true
-	case reflect.Map:
+	case reflect.Map, reflect.Struct:
 		return "object", true
 	default:
 		return "", false
diff --git a/internal/mcp/jsonschema/validate.go b/internal/mcp/jsonschema/validate.go
index 466e345..9068ae6 100644
--- a/internal/mcp/jsonschema/validate.go
+++ b/internal/mcp/jsonschema/validate.go
@@ -662,8 +662,8 @@
 	case reflect.Struct:
 		props := structPropertiesOf(v.Type())
 		// Ignore nonexistent properties.
-		if index, ok := props[name]; ok {
-			return v.FieldByIndex(index)
+		if sf, ok := props[name]; ok {
+			return v.FieldByIndex(sf.Index)
 		}
 		return reflect.Value{}
 	default:
@@ -673,6 +673,8 @@
 
 // properties returns an iterator over the names and values of all properties
 // in v, which must be a map or a struct.
+// If a struct, zero-valued properties that are marked omitempty or omitzero
+// are excluded.
 func properties(v reflect.Value) iter.Seq2[string, reflect.Value] {
 	return func(yield func(string, reflect.Value) bool) {
 		switch v.Kind() {
@@ -683,8 +685,14 @@
 				}
 			}
 		case reflect.Struct:
-			for name, index := range structPropertiesOf(v.Type()) {
-				if !yield(name, v.FieldByIndex(index)) {
+			for name, sf := range structPropertiesOf(v.Type()) {
+				val := v.FieldByIndex(sf.Index)
+				if val.IsZero() {
+					if tag, ok := sf.Tag.Lookup("json"); ok && (strings.Contains(tag, "omitempty") || strings.Contains(tag, "omitzero")) {
+						continue
+					}
+				}
+				if !yield(name, val) {
 					return
 				}
 			}
@@ -707,8 +715,8 @@
 	case reflect.Struct:
 		sp := structPropertiesOf(v.Type())
 		min := 0
-		for prop, index := range sp {
-			if !v.FieldByIndex(index).IsZero() || isRequired[prop] {
+		for prop, sf := range sp {
+			if !v.FieldByIndex(sf.Index).IsZero() || isRequired[prop] {
 				min++
 			}
 		}
@@ -719,7 +727,7 @@
 }
 
 // A propertyMap is a map from property name to struct field index.
-type propertyMap = map[string][]int
+type propertyMap = map[string]reflect.StructField
 
 var structProperties sync.Map // from reflect.Type to propertyMap
 
@@ -730,10 +738,10 @@
 	if props, ok := structProperties.Load(t); ok {
 		return props.(propertyMap)
 	}
-	props := map[string][]int{}
+	props := map[string]reflect.StructField{}
 	for _, sf := range reflect.VisibleFields(t) {
 		if name, ok := jsonName(sf); ok {
-			props[name] = sf.Index
+			props[name] = sf
 		}
 	}
 	structProperties.Store(t, props)
diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go
index 96a3dd7..2ab5b87 100644
--- a/internal/mcp/mcp_test.go
+++ b/internal/mcp/mcp_test.go
@@ -709,4 +709,5 @@
 	}
 }
 
+// A function, because schemas must form a tree (they have hidden state).
 func falseSchema() *jsonschema.Schema { return &jsonschema.Schema{Not: &jsonschema.Schema{}} }
diff --git a/internal/mcp/prompt.go b/internal/mcp/prompt.go
index f57ccfb..97aed98 100644
--- a/internal/mcp/prompt.go
+++ b/internal/mcp/prompt.go
@@ -42,6 +42,10 @@
 	if schema.Type != "object" || !reflect.DeepEqual(schema.AdditionalProperties, &jsonschema.Schema{Not: &jsonschema.Schema{}}) {
 		panic(fmt.Sprintf("handler request type must be a struct"))
 	}
+	resolved, err := schema.Resolve(nil)
+	if err != nil {
+		panic(err)
+	}
 	prompt := &ServerPrompt{
 		Prompt: &Prompt{
 			Name:        name,
@@ -70,7 +74,7 @@
 			return nil, err
 		}
 		var v TReq
-		if err := unmarshalSchema(rawArgs, schema, &v); err != nil {
+		if err := unmarshalSchema(rawArgs, resolved, &v); err != nil {
 			return nil, err
 		}
 		return handler(ctx, ss, v, params)
diff --git a/internal/mcp/server.go b/internal/mcp/server.go
index 0ecb3cd..fa55c33 100644
--- a/internal/mcp/server.go
+++ b/internal/mcp/server.go
@@ -291,7 +291,7 @@
 	return res, nil
 }
 
-// FileResourceHandler returns a ReadResourceHandler that reads paths using dir as
+// fileResourceHandler returns a ReadResourceHandler that reads paths using dir as
 // a base directory.
 // It honors client roots and protects against path traversal attacks.
 //
@@ -303,7 +303,7 @@
 // Lexical path traversal attacks, where the path has ".." elements that escape dir,
 // are always caught. Go 1.24 and above also protects against symlink-based attacks,
 // where symlinks under dir lead out of the tree.
-func (s *Server) FileResourceHandler(dir string) ResourceHandler {
+func (s *Server) fileResourceHandler(dir string) ResourceHandler {
 	return fileResourceHandler(dir)
 }
 
diff --git a/internal/mcp/tool.go b/internal/mcp/tool.go
index 099321f..6e8b3cf 100644
--- a/internal/mcp/tool.go
+++ b/internal/mcp/tool.go
@@ -5,8 +5,10 @@
 package mcp
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"slices"
 
 	"golang.org/x/tools/internal/mcp/jsonschema"
@@ -39,10 +41,17 @@
 	if err != nil {
 		panic(err)
 	}
+	// We must resolve the schema after the ToolOptions have had a chance to update it.
+	// But the handler needs access to the resolved schema, and the options may change
+	// the handler too.
+	// The best we can do is use the resolved schema in our own wrapped handler,
+	// and hope that no ToolOption replaces it.
+	// TODO(jba): at a minimum, document this.
+	var resolved *jsonschema.Resolved
 	wrapped := func(ctx context.Context, cc *ServerSession, params *CallToolParams[json.RawMessage]) (*CallToolResult, error) {
 		var params2 CallToolParams[TReq]
 		if params.Arguments != nil {
-			if err := unmarshalSchema(params.Arguments, schema, &params2.Arguments); err != nil {
+			if err := unmarshalSchema(params.Arguments, resolved, &params2.Arguments); err != nil {
 				return nil, err
 			}
 		}
@@ -68,15 +77,38 @@
 	for _, opt := range opts {
 		opt.set(t)
 	}
+	if schema := t.Tool.InputSchema; schema != nil {
+		// Resolve the schema, with no base URI. We don't expect tool schemas to
+		// refer outside of themselves.
+		resolved, err = schema.Resolve(nil)
+		if err != nil {
+			panic(fmt.Errorf("resolving input schema %s: %w", schemaJSON(schema), err))
+		}
+	}
 	return t
 }
 
 // unmarshalSchema unmarshals data into v and validates the result according to
-// the given schema.
-func unmarshalSchema(data json.RawMessage, _ *jsonschema.Schema, v any) error {
+// the given resolved schema.
+func unmarshalSchema(data json.RawMessage, resolved *jsonschema.Resolved, v any) error {
 	// TODO: use reflection to create the struct type to unmarshal into.
 	// Separate validation from assignment.
-	return json.Unmarshal(data, v)
+
+	// Disallow unknown fields.
+	// Otherwise, if the tool was built with a struct, the client could send extra
+	// fields and json.Unmarshal would ignore them, so the schema would never get
+	// a chance to declare the extra args invalid.
+	dec := json.NewDecoder(bytes.NewReader(data))
+	dec.DisallowUnknownFields()
+	if err := dec.Decode(v); err != nil {
+		return fmt.Errorf("unmarshaling: %w", err)
+	}
+	if resolved != nil {
+		if err := resolved.Validate(v); err != nil {
+			return fmt.Errorf("validating\n\t%s\nagainst\n\t %s:\n %w", data, schemaJSON(resolved.Schema()), err)
+		}
+	}
+	return nil
 }
 
 // A ToolOption configures the behavior of a Tool.
@@ -177,3 +209,12 @@
 		*s = *schema
 	})
 }
+
+// schemaJSON returns the JSON value for s as a string, or a string indicating an error.
+func schemaJSON(s *jsonschema.Schema) string {
+	m, err := json.Marshal(s)
+	if err != nil {
+		return fmt.Sprintf("<!%s>", err)
+	}
+	return string(m)
+}
diff --git a/internal/mcp/tool_test.go b/internal/mcp/tool_test.go
index 646e9b3..077ea39 100644
--- a/internal/mcp/tool_test.go
+++ b/internal/mcp/tool_test.go
@@ -6,6 +6,8 @@
 
 import (
 	"context"
+	"encoding/json"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -88,3 +90,76 @@
 		}
 	}
 }
+
+func TestNewToolValidate(t *testing.T) {
+	// Check that the tool returned from NewTool properly validates its input schema.
+
+	type req struct {
+		I int
+		B bool
+		S string `json:",omitempty"`
+		P *int   `json:",omitempty"`
+	}
+
+	dummyHandler := func(context.Context, *mcp.ServerSession, *mcp.CallToolParams[req]) (*mcp.CallToolResult, error) {
+		return nil, nil
+	}
+
+	tool := mcp.NewTool("test", "test", dummyHandler)
+	for _, tt := range []struct {
+		desc string
+		args map[string]any
+		want string // error should contain this string; empty for success
+	}{
+		{
+			"both required",
+			map[string]any{"I": 1, "B": true},
+			"",
+		},
+		{
+			"optional",
+			map[string]any{"I": 1, "B": true, "S": "foo"},
+			"",
+		},
+		{
+			"wrong type",
+			map[string]any{"I": 1.5, "B": true},
+			"cannot unmarshal",
+		},
+		{
+			"extra property",
+			map[string]any{"I": 1, "B": true, "C": 2},
+			"unknown field",
+		},
+		{
+			"value for pointer",
+			map[string]any{"I": 1, "B": true, "P": 3},
+			"",
+		},
+		{
+			"null for pointer",
+			map[string]any{"I": 1, "B": true, "P": nil},
+			"",
+		},
+	} {
+		t.Run(tt.desc, func(t *testing.T) {
+			raw, err := json.Marshal(tt.args)
+			if err != nil {
+				t.Fatal(err)
+			}
+			_, err = tool.Handler(context.Background(), nil,
+				&mcp.CallToolParams[json.RawMessage]{Arguments: json.RawMessage(raw)})
+			if err == nil && tt.want != "" {
+				t.Error("got success, wanted failure")
+			}
+			if err != nil {
+				if tt.want == "" {
+					t.Fatalf("failed with:\n%s\nwanted success", err)
+				}
+				if !strings.Contains(err.Error(), tt.want) {
+					t.Fatalf("got:\n%s\nwanted to contain %q", err, tt.want)
+				}
+			}
+		})
+	}
+}
diff --git a/internal/refactor/inline/inline.go b/internal/refactor/inline/inline.go
index 7e1c999..0697f3a 100644
--- a/internal/refactor/inline/inline.go
+++ b/internal/refactor/inline/inline.go
@@ -330,20 +330,35 @@
 			}
 		}
 		// Add new imports.
+		// Set their position to after the last position of the old imports, to keep
+		// comments on the old imports from moving.
+		lastPos := token.NoPos
+		if lastSpec := last(importDecl.Specs); lastSpec != nil {
+			lastPos = lastSpec.Pos()
+			if c := lastSpec.(*ast.ImportSpec).Comment; c != nil {
+				lastPos = c.Pos()
+			}
+		}
 		for _, imp := range newImports {
 			// Check that the new imports are accessible.
 			path, _ := strconv.Unquote(imp.spec.Path.Value)
 			if !analysisinternal.CanImport(caller.Types.Path(), path) {
 				return nil, fmt.Errorf("can't inline function %v as its body refers to inaccessible package %q", callee, path)
 			}
+			if lastPos.IsValid() {
+				lastPos++
+				imp.spec.Path.ValuePos = lastPos
+			}
 			importDecl.Specs = append(importDecl.Specs, imp.spec)
 		}
+
 		var out bytes.Buffer
 		out.Write(before)
 		commented := &printer.CommentedNode{
 			Node:     importDecl,
 			Comments: comments,
 		}
+
 		if err := format.Node(&out, fset, commented); err != nil {
 			logf("failed to format new importDecl: %v", err) // debugging
 			return nil, err
@@ -354,7 +369,6 @@
 			return nil, err
 		}
 	}
-
 	// Delete imports referenced only by caller.Call.Fun.
 	for _, oldImport := range res.oldImports {
 		specToDelete := oldImport.spec
diff --git a/internal/refactor/inline/testdata/import-comments.txtar b/internal/refactor/inline/testdata/import-comments.txtar
index d4a4122..b5319e4 100644
--- a/internal/refactor/inline/testdata/import-comments.txtar
+++ b/internal/refactor/inline/testdata/import-comments.txtar
@@ -28,7 +28,7 @@
 	"io"
 
 	// This is an import of c.
-	"testdata/c"
+	"testdata/c" // yes, of c
 )
 
 var (
@@ -52,7 +52,7 @@
 
 	// This is an import of c.
 	"testdata/b"
-	"testdata/c"
+	"testdata/c" // yes, of c
 )
 
 var (