gopls/internal/lsp/source: stub: don't panic when encountering 'error'
The built-in error interface is the only non-trivial named interface
type with no package. This caused v0.11.0 of gopls to panic when
the ApplyFix stub operation was applied to an interface that embedded
'error'. The crash was inadvertently fixed by recent changes, but still
the operation failed with an error that mentioned the filename "".
This change causes the operation to succeed with the proper result.
It also adds a test.
I think there's a simpler implementation of this operation following
the approach described in the existing comment that would obviate
the special-case logic added in this change.
Fixes golang/vscode-go#2606
Change-Id: I544de5f828055e999344117e4f8b2579bde21cfb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/463035
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/source/stub.go b/gopls/internal/lsp/source/stub.go
index cf360dd..7b6f4f4 100644
--- a/gopls/internal/lsp/source/stub.go
+++ b/gopls/internal/lsp/source/stub.go
@@ -255,6 +255,20 @@
if !ok {
return nil, fmt.Errorf("expected %v to be an interface but got %T", iface, ifaceObj.Type().Underlying())
}
+
+ // The built-in error interface is special.
+ if ifaceObj.Pkg() == nil && ifaceObj.Name() == "error" {
+ var missingInterfaces []*missingInterface
+ if concMS.Lookup(nil, "Error") == nil {
+ errorMethod, _, _ := types.LookupFieldOrMethod(iface, false, nil, "Error")
+ missingInterfaces = append(missingInterfaces, &missingInterface{
+ iface: ifaceObj,
+ missing: []*types.Func{errorMethod.(*types.Func)},
+ })
+ }
+ return missingInterfaces, nil
+ }
+
// Parse the imports from the file that declares the interface.
ifaceFilename := safetoken.StartPosition(snapshot.FileSet(), ifaceObj.Pos()).Filename
ifaceFH, err := snapshot.GetFile(ctx, span.URIFromPath(ifaceFilename))
@@ -266,18 +280,15 @@
return nil, fmt.Errorf("error parsing imports from interface file: %w", err)
}
- mi := &missingInterface{
- iface: ifaceObj,
- imports: ifaceFile.File.Imports,
- }
+ var missing []*types.Func
- // Add all the interface methods not defined by the concrete type to mi.missing.
+ // Add all the interface methods not defined by the concrete type to missing.
for i := 0; i < iface.NumExplicitMethods(); i++ {
method := iface.ExplicitMethod(i)
if sel := concMS.Lookup(concPkg, method.Name()); sel == nil {
// Concrete type does not have the interface method.
if _, ok := visited[method.Name()]; !ok {
- mi.missing = append(mi.missing, method)
+ missing = append(missing, method)
visited[method.Name()] = struct{}{}
}
} else {
@@ -297,27 +308,31 @@
// sets of the interface and concrete types. Once the set
// difference (missing methods) is computed, the imports
// from the declaring file(s) could be loaded as needed.
- var missing []*missingInterface
+ var missingInterfaces []*missingInterface
for i := 0; i < iface.NumEmbeddeds(); i++ {
eiface := iface.Embedded(i).Obj()
em, err := missingMethods(ctx, snapshot, concMS, concPkg, eiface, visited)
if err != nil {
return nil, err
}
- missing = append(missing, em...)
+ missingInterfaces = append(missingInterfaces, em...)
}
// The type checker is deterministic, but its choice of
// ordering of embedded interfaces varies with Go version
// (e.g. go1.17 was sorted, go1.18 was lexical order).
// Sort to ensure test portability.
- sort.Slice(missing, func(i, j int) bool {
- return missing[i].iface.Id() < missing[j].iface.Id()
+ sort.Slice(missingInterfaces, func(i, j int) bool {
+ return missingInterfaces[i].iface.Id() < missingInterfaces[j].iface.Id()
})
- if len(mi.missing) > 0 {
- missing = append(missing, mi)
+ if len(missing) > 0 {
+ missingInterfaces = append(missingInterfaces, &missingInterface{
+ iface: ifaceObj,
+ imports: ifaceFile.File.Imports,
+ missing: missing,
+ })
}
- return missing, nil
+ return missingInterfaces, nil
}
// missingInterface represents an interface
diff --git a/gopls/internal/lsp/testdata/stub/stub_issue2606.go b/gopls/internal/lsp/testdata/stub/stub_issue2606.go
new file mode 100644
index 0000000..66ef2b2
--- /dev/null
+++ b/gopls/internal/lsp/testdata/stub/stub_issue2606.go
@@ -0,0 +1,7 @@
+package stub
+
+type I interface{ error }
+
+type C int
+
+var _ I = C(0) //@suggestedfix("C", "refactor.rewrite", "")
diff --git a/gopls/internal/lsp/testdata/stub/stub_issue2606.go.golden b/gopls/internal/lsp/testdata/stub/stub_issue2606.go.golden
new file mode 100644
index 0000000..4db2663
--- /dev/null
+++ b/gopls/internal/lsp/testdata/stub/stub_issue2606.go.golden
@@ -0,0 +1,14 @@
+-- suggestedfix_stub_issue2606_7_11 --
+package stub
+
+type I interface{ error }
+
+type C int
+
+// Error implements I
+func (C) Error() string {
+ panic("unimplemented")
+}
+
+var _ I = C(0) //@suggestedfix("C", "refactor.rewrite", "")
+
diff --git a/gopls/internal/lsp/testdata/summary.txt.golden b/gopls/internal/lsp/testdata/summary.txt.golden
index e098241..478b6a6 100644
--- a/gopls/internal/lsp/testdata/summary.txt.golden
+++ b/gopls/internal/lsp/testdata/summary.txt.golden
@@ -13,7 +13,7 @@
FormatCount = 6
ImportCount = 8
SemanticTokenCount = 3
-SuggestedFixCount = 64
+SuggestedFixCount = 65
FunctionExtractionCount = 27
MethodExtractionCount = 6
DefinitionsCount = 99
diff --git a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden
index b402eef..735259e 100644
--- a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden
+++ b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden
@@ -13,7 +13,7 @@
FormatCount = 6
ImportCount = 8
SemanticTokenCount = 3
-SuggestedFixCount = 70
+SuggestedFixCount = 71
FunctionExtractionCount = 27
MethodExtractionCount = 6
DefinitionsCount = 110
