gopls/internal/analysis/stubmethods: merge into CodeAction

This change removes the stubmethods analyzer, a roundabout
implementation, and instead computes it directly from the
protocol.QuickFix code action producer.

This is simpler, more efficient, and has noticeably lower
latency (being type-based not analysis based).
We should consider this for the other type-error analyzers.
However, the documentation, formerly generated to analyzers.md,
is now maintained in the Quick Fixes section of diagnostics.md.
More importantly, the `analyzers[stubmethods]` config setting
no longer exists.

Also:
- addEditAction et al: pass Diagnostics as a parameter
  instead of returning a pointer to a mutable CodeAction.
- protocol.Intersect: clarify how its treatment differs from
  mathematical convention in its handling of empty ranges,
  and fix a bug where by [1,2) and [2,3) were considered
  to intersect. (Only abutting empty ranges intersect by
  our definition.)
- Upgrade a marker test from @diag to @suggestedfixerr,
  now that the latter exists.

Change-Id: I010b2d4730596cac6f376c631839bfda159bf433
Reviewed-on: https://go-review.googlesource.com/c/tools/+/617035
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: adonovan

gopls/doc/analyzers.md

@@ -796,42 +796,6 @@ Default: on.
 
 Package documentation: [structtag](https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/structtag)
 
-<a id='stubmethods'></a>
-## `stubmethods`: detect missing methods and fix with stub implementations
-
-
-This analyzer detects type-checking errors due to missing methods
-in assignments from concrete types to interface types, and offers
-a suggested fix that will create a set of stub methods so that
-the concrete type satisfies the interface.
-
-For example, this function will not compile because the value
-NegativeErr{} does not implement the "error" interface:
-
-	func sqrt(x float64) (float64, error) {
-		if x < 0 {
-			return 0, NegativeErr{} // error: missing method
-		}
-		...
-	}
-
-	type NegativeErr struct{}
-
-This analyzer will suggest a fix to declare this method:
-
-	// Error implements error.Error.
-	func (NegativeErr) Error() string {
-		panic("unimplemented")
-	}
-
-(At least, it appears to behave that way, but technically it
-doesn't use the SuggestedFix mechanism and the stub is created by
-logic in gopls's golang.stub function.)
-
-Default: on.
-
-Package documentation: [stubmethods](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/stubmethods)
-
 <a id='testinggoroutine'></a>
 ## `testinggoroutine`: report calls to (*testing.T).Fatal from goroutines started by a test
 

gopls/doc/features/diagnostics.md

@@ -119,6 +119,52 @@ Client support:
 - **Vim + coc.nvim**: ??
 - **CLI**: `gopls check file.go`
 
+
+<!-- Below we list any quick fixes (by their internal fix name)
+     that aren't analyzers. -->
+
+### `stubMethods`: Declare missing methods of type
+
+When a value of a concrete type is assigned to a variable of an
+interface type, but the concrete type does not possess all the
+necessary methods, the type checker will report a "missing method"
+error.
+
+In this situation, gopls offers a quick fix to add stub declarations
+of all the missing methods to the concrete type so that it implements
+the interface.
+
+For example, this function will not compile because the value
+`NegativeErr{}` does not implement the "error" interface:
+
+```go
+func sqrt(x float64) (float64, error) {
+	if x < 0 {
+		return 0, NegativeErr{} // error: missing method
+	}
+	...
+}
+
+type NegativeErr struct{}
+```
+
+Gopls will offer a quick fix to declare this method:
+
+```go
+
+// Error implements error.Error.
+func (NegativeErr) Error() string {
+	panic("unimplemented")
+}
+```
+
+Beware that the new declarations appear alongside the concrete type,
+which may be in a different file or even package from the cursor
+position.
+(Perhaps gopls should send a `showDocument` request to navigate the
+client there, or a progress notification indicating that something
+happened.)
+
 <!--
 
 dorky details and deletia:

gopls/doc/features/transformation.md

@@ -275,9 +275,6 @@ The `refactor.extract` family of code actions all return commands that
 replace the selected expression or statements with a reference to a
 newly created declaration that contains the selected code:
 
-<!-- See TODO comments in settings/codeactionkind.go about splitting
-     up "refactor.extract" into finer grained categories. -->
-
 - **`refactor.extract.function`** replaces one or more complete statements by a
   call to a new function named `newFunction` whose body contains the
   statements. The selection must enclose fewer statements than the

gopls/internal/analysis/stubmethods/doc.go

@@ -4,101 +4,16 @@
 
 package stubmethods
 
+// TODO(adonovan): rename package to golang/stubmethods or move
+// functions to golang/stub.go.
+
 import (
-	"bytes"
-	_ "embed"
 	"fmt"
 	"go/ast"
-	"go/format"
 	"go/token"
 	"go/types"
-	"strings"
-
-	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/go/ast/astutil"
-	"golang.org/x/tools/gopls/internal/util/typesutil"
-	"golang.org/x/tools/internal/analysisinternal"
-	"golang.org/x/tools/internal/typesinternal"
 )
 
-//go:embed doc.go
-var doc string
-
-var Analyzer = &analysis.Analyzer{
-	Name:             "stubmethods",
-	Doc:              analysisinternal.MustExtractDoc(doc, "stubmethods"),
-	Run:              run,
-	RunDespiteErrors: true,
-	URL:              "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/stubmethods",
-}
-
-// TODO(rfindley): remove this thin wrapper around the stubmethods refactoring,
-// and eliminate the stubmethods analyzer.
-//
-// Previous iterations used the analysis framework for computing refactorings,
-// which proved inefficient.
-func run(pass *analysis.Pass) (interface{}, error) {
-	for _, err := range pass.TypeErrors {
-		var file *ast.File
-		for _, f := range pass.Files {
-			if f.Pos() <= err.Pos && err.Pos < f.End() {
-				file = f
-				break
-			}
-		}
-		// Get the end position of the error.
-		_, _, end, ok := typesinternal.ReadGo116ErrorData(err)
-		if !ok {
-			var buf bytes.Buffer
-			if err := format.Node(&buf, pass.Fset, file); err != nil {
-				continue
-			}
-			end = analysisinternal.TypeErrorEndPos(pass.Fset, buf.Bytes(), err.Pos)
-		}
-		if diag, ok := DiagnosticForError(pass.Fset, file, err.Pos, end, err.Msg, pass.TypesInfo); ok {
-			pass.Report(diag)
-		}
-	}
-
-	return nil, nil
-}
-
-// MatchesMessage reports whether msg matches the error message sought after by
-// the stubmethods fix.
-func MatchesMessage(msg string) bool {
-	return strings.Contains(msg, "missing method") || strings.HasPrefix(msg, "cannot convert") || strings.Contains(msg, "not implement")
-}
-
-// DiagnosticForError computes a diagnostic suggesting to implement an
-// interface to fix the type checking error defined by (start, end, msg).
-//
-// If no such fix is possible, the second result is false.
-func DiagnosticForError(fset *token.FileSet, file *ast.File, start, end token.Pos, msg string, info *types.Info) (analysis.Diagnostic, bool) {
-	if !MatchesMessage(msg) {
-		return analysis.Diagnostic{}, false
-	}
-
-	path, _ := astutil.PathEnclosingInterval(file, start, end)
-	si := GetStubInfo(fset, info, path, start)
-	if si == nil {
-		return analysis.Diagnostic{}, false
-	}
-	qf := typesutil.FileQualifier(file, si.Concrete.Obj().Pkg(), info)
-	iface := types.TypeString(si.Interface.Type(), qf)
-	return analysis.Diagnostic{
-		Pos:      start,
-		End:      end,
-		Message:  msg,
-		Category: FixCategory,
-		SuggestedFixes: []analysis.SuggestedFix{{
-			Message: fmt.Sprintf("Declare missing methods of %s", iface),
-			// No TextEdits => computed later by gopls.
-		}},
-	}, true
-}
-
-const FixCategory = "stubmethods" // recognized by gopls ApplyFix
-
 // StubInfo represents a concrete type
 // that wants to stub out an interface type
 type StubInfo struct {

gopls/internal/analysis/stubmethods/stubmethods.go

@@ -567,11 +567,6 @@
 							"Doc": "check that struct field tags conform to reflect.StructTag.Get\n\nAlso report certain struct tags (json, xml) used with unexported fields.",
 							"Default": "true"
 						},
-						{
-							"Name": "\"stubmethods\"",
-							"Doc": "detect missing methods and fix with stub implementations\n\nThis analyzer detects type-checking errors due to missing methods\nin assignments from concrete types to interface types, and offers\na suggested fix that will create a set of stub methods so that\nthe concrete type satisfies the interface.\n\nFor example, this function will not compile because the value\nNegativeErr{} does not implement the \"error\" interface:\n\n\tfunc sqrt(x float64) (float64, error) {\n\t\tif x \u003c 0 {\n\t\t\treturn 0, NegativeErr{} // error: missing method\n\t\t}\n\t\t...\n\t}\n\n\ttype NegativeErr struct{}\n\nThis analyzer will suggest a fix to declare this method:\n\n\t// Error implements error.Error.\n\tfunc (NegativeErr) Error() string {\n\t\tpanic(\"unimplemented\")\n\t}\n\n(At least, it appears to behave that way, but technically it\ndoesn't use the SuggestedFix mechanism and the stub is created by\nlogic in gopls's golang.stub function.)",
-							"Default": "true"
-						},
 						{
 							"Name": "\"testinggoroutine\"",
 							"Doc": "report calls to (*testing.T).Fatal from goroutines started by a test\n\nFunctions that abruptly terminate a test, such as the Fatal, Fatalf, FailNow, and\nSkip{,f,Now} methods of *testing.T, must be called from the test goroutine itself.\nThis checker detects calls to these functions that occur within a goroutine\nstarted by the test. For example:\n\n\tfunc TestFoo(t *testing.T) {\n\t    go func() {\n\t        t.Fatal(\"oops\") // error: (*T).Fatal called from non-test goroutine\n\t    }()\n\t}",
@@ -1238,12 +1233,6 @@
 			"URL": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/structtag",
 			"Default": true
 		},
-		{
-			"Name": "stubmethods",
-			"Doc": "detect missing methods and fix with stub implementations\n\nThis analyzer detects type-checking errors due to missing methods\nin assignments from concrete types to interface types, and offers\na suggested fix that will create a set of stub methods so that\nthe concrete type satisfies the interface.\n\nFor example, this function will not compile because the value\nNegativeErr{} does not implement the \"error\" interface:\n\n\tfunc sqrt(x float64) (float64, error) {\n\t\tif x \u003c 0 {\n\t\t\treturn 0, NegativeErr{} // error: missing method\n\t\t}\n\t\t...\n\t}\n\n\ttype NegativeErr struct{}\n\nThis analyzer will suggest a fix to declare this method:\n\n\t// Error implements error.Error.\n\tfunc (NegativeErr) Error() string {\n\t\tpanic(\"unimplemented\")\n\t}\n\n(At least, it appears to behave that way, but technically it\ndoesn't use the SuggestedFix mechanism and the stub is created by\nlogic in gopls's golang.stub function.)",
-			"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/stubmethods",
-			"Default": true
-		},
 		{
 			"Name": "testinggoroutine",
 			"Doc": "report calls to (*testing.T).Fatal from goroutines started by a test\n\nFunctions that abruptly terminate a test, such as the Fatal, Fatalf, FailNow, and\nSkip{,f,Now} methods of *testing.T, must be called from the test goroutine itself.\nThis checker detects calls to these functions that occur within a goroutine\nstarted by the test. For example:\n\n\tfunc TestFoo(t *testing.T) {\n\t    go func() {\n\t        t.Fatal(\"oops\") // error: (*T).Fatal called from non-test goroutine\n\t    }()\n\t}",

gopls/internal/analysis/stubmethods/stubmethods_test.go

@@ -68,5 +68,5 @@ func convertStringLiteral(req *codeActionsRequest) {
 		bug.Reportf("failed to convert diff.Edit to protocol.TextEdit:%v", err)
 		return
 	}
-	req.addEditAction(title, protocol.DocumentChangeEdit(req.fh, textedits))
+	req.addEditAction(title, nil, protocol.DocumentChangeEdit(req.fh, textedits))
 }