all: merge master (5b4d426) into gopls-release-branch.0.13
For golang/go#61675
Merge List:
+ 2023-08-01 5b4d42666 gopls/internal/hooks: clean language version before passing to gofumpt
+ 2023-08-01 2160c5f15 gopls/internal/lsp/analysis: fix stubmethods with variadic parameters
Change-Id: I5a27fc0307d7f032b6f06d677a29a757a3c7f758
diff --git a/gopls/internal/hooks/gofumpt_118.go b/gopls/internal/hooks/gofumpt_118.go
index 4eb5232..bf0ba41 100644
--- a/gopls/internal/hooks/gofumpt_118.go
+++ b/gopls/internal/hooks/gofumpt_118.go
@@ -9,6 +9,7 @@
import (
"context"
+ "fmt"
"golang.org/x/tools/gopls/internal/lsp/source"
"mvdan.cc/gofumpt/format"
@@ -16,9 +17,62 @@
func updateGofumpt(options *source.Options) {
options.GofumptFormat = func(ctx context.Context, langVersion, modulePath string, src []byte) ([]byte, error) {
+ fixedVersion, err := fixLangVersion(langVersion)
+ if err != nil {
+ return nil, err
+ }
return format.Source(src, format.Options{
- LangVersion: langVersion,
+ LangVersion: fixedVersion,
ModulePath: modulePath,
})
}
}
+
+// fixLangVersion function cleans the input so that gofumpt doesn't panic. It is
+// rather permissive, and accepts version strings that aren't technically valid
+// in a go.mod file.
+//
+// More specifically, it looks for an optional 'v' followed by 1-3
+// '.'-separated numbers. The resulting string is stripped of any suffix beyond
+// this expected version number pattern.
+//
+// See also golang/go#61692: gofumpt does not accept the new language versions
+// appearing in go.mod files (e.g. go1.21rc3).
+func fixLangVersion(input string) (string, error) {
+ bad := func() (string, error) {
+ return "", fmt.Errorf("invalid language version syntax %q", input)
+ }
+ if input == "" {
+ return input, nil
+ }
+ i := 0
+ if input[0] == 'v' { // be flexible about 'v'
+ i++
+ }
+ // takeDigits consumes ascii numerals 0-9 and reports if at least one was
+ // consumed.
+ takeDigits := func() bool {
+ found := false
+ for ; i < len(input) && '0' <= input[i] && input[i] <= '9'; i++ {
+ found = true
+ }
+ return found
+ }
+ if !takeDigits() { // versions must start with at least one number
+ return bad()
+ }
+
+ // Accept optional minor and patch versions.
+ for n := 0; n < 2; n++ {
+ if i < len(input) && input[i] == '.' {
+ // Look for minor/patch version.
+ i++
+ if !takeDigits() {
+ i--
+ break
+ }
+ }
+ }
+ // Accept any suffix.
+ return input[:i], nil
+}
diff --git a/gopls/internal/hooks/gofumpt_118_test.go b/gopls/internal/hooks/gofumpt_118_test.go
new file mode 100644
index 0000000..838ce73
--- /dev/null
+++ b/gopls/internal/hooks/gofumpt_118_test.go
@@ -0,0 +1,53 @@
+// Copyright 2022 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.
+
+//go:build go1.18
+// +build go1.18
+
+package hooks
+
+import "testing"
+
+func TestFixLangVersion(t *testing.T) {
+ tests := []struct {
+ input, want string
+ wantErr bool
+ }{
+ {"", "", false},
+ {"1.18", "1.18", false},
+ {"v1.18", "v1.18", false},
+ {"1.21", "1.21", false},
+ {"1.21rc3", "1.21", false},
+ {"1.21.0", "1.21.0", false},
+ {"1.21.1", "1.21.1", false},
+ {"v1.21.1", "v1.21.1", false},
+ {"v1.21.0rc1", "v1.21.0", false}, // not technically valid, but we're flexible
+ {"v1.21.0.0", "v1.21.0", false}, // also technically invalid
+ {"1.1", "1.1", false},
+ {"v1", "v1", false},
+ {"1", "1", false},
+ {"v1.21.", "v1.21", false}, // also invalid
+ {"1.21.", "1.21", false},
+
+ // Error cases.
+ {"rc1", "", true},
+ {"x1.2.3", "", true},
+ }
+
+ for _, test := range tests {
+ got, err := fixLangVersion(test.input)
+ if test.wantErr {
+ if err == nil {
+ t.Errorf("fixLangVersion(%q) succeeded unexpectedly", test.input)
+ }
+ continue
+ }
+ if err != nil {
+ t.Fatalf("fixLangVersion(%q) failed: %v", test.input, err)
+ }
+ if got != test.want {
+ t.Errorf("fixLangVersion(%q) = %s, want %s", test.input, got, test.want)
+ }
+ }
+}
diff --git a/gopls/internal/lsp/analysis/stubmethods/stubmethods.go b/gopls/internal/lsp/analysis/stubmethods/stubmethods.go
index 930cb99..f5b2ac5 100644
--- a/gopls/internal/lsp/analysis/stubmethods/stubmethods.go
+++ b/gopls/internal/lsp/analysis/stubmethods/stubmethods.go
@@ -174,8 +174,19 @@
if !ok {
return nil
}
- sigVar := sig.Params().At(paramIdx)
- iface := ifaceObjFromType(sigVar.Type())
+ var paramType types.Type
+ if sig.Variadic() && paramIdx >= sig.Params().Len()-1 {
+ v := sig.Params().At(sig.Params().Len() - 1)
+ if s, _ := v.Type().(*types.Slice); s != nil {
+ paramType = s.Elem()
+ }
+ } else if paramIdx < sig.Params().Len() {
+ paramType = sig.Params().At(paramIdx).Type()
+ }
+ if paramType == nil {
+ return nil // A type error prevents us from determining the param type.
+ }
+ iface := ifaceObjFromType(paramType)
if iface == nil {
return nil
}
diff --git a/gopls/internal/lsp/code_action.go b/gopls/internal/lsp/code_action.go
index ff5e2a9..69df978 100644
--- a/gopls/internal/lsp/code_action.go
+++ b/gopls/internal/lsp/code_action.go
@@ -12,6 +12,7 @@
"strings"
"golang.org/x/tools/go/ast/inspector"
+ "golang.org/x/tools/gopls/internal/bug"
"golang.org/x/tools/gopls/internal/lsp/analysis/fillstruct"
"golang.org/x/tools/gopls/internal/lsp/analysis/infertypeargs"
"golang.org/x/tools/gopls/internal/lsp/analysis/stubmethods"
@@ -198,26 +199,46 @@
if err != nil {
return nil, err
}
- for _, pd := range diagnostics {
+ for _, pd := range stubMethodsDiagnostics {
start, end, err := pgf.RangePos(pd.Range)
if err != nil {
return nil, err
}
- if d, ok := stubmethods.DiagnosticForError(pkg.FileSet(), pgf.File, start, end, pd.Message, pkg.GetTypesInfo()); ok {
+ action, ok, err := func() (_ protocol.CodeAction, _ bool, rerr error) {
+ // golang/go#61693: code actions were refactored to run outside of the
+ // analysis framework, but as a result they lost their panic recovery.
+ //
+ // Stubmethods "should never fail"", but put back the panic recovery as a
+ // defensive measure.
+ defer func() {
+ if r := recover(); r != nil {
+ rerr = bug.Errorf("stubmethods panicked: %v", r)
+ }
+ }()
+ d, ok := stubmethods.DiagnosticForError(pkg.FileSet(), pgf.File, start, end, pd.Message, pkg.GetTypesInfo())
+ if !ok {
+ return protocol.CodeAction{}, false, nil
+ }
cmd, err := command.NewApplyFixCommand(d.Message, command.ApplyFixArgs{
URI: protocol.URIFromSpanURI(pgf.URI),
Fix: source.StubMethods,
Range: pd.Range,
})
if err != nil {
- return nil, err
+ return protocol.CodeAction{}, false, err
}
- actions = append(actions, protocol.CodeAction{
+ return protocol.CodeAction{
Title: d.Message,
Kind: protocol.QuickFix,
Command: &cmd,
Diagnostics: []protocol.Diagnostic{pd},
- })
+ }, true, nil
+ }()
+ if err != nil {
+ return nil, err
+ }
+ if ok {
+ actions = append(actions, action)
}
}
@@ -387,7 +408,17 @@
return actions, nil
}
-func refactorRewrite(ctx context.Context, snapshot source.Snapshot, pkg source.Package, pgf *source.ParsedGoFile, fh source.FileHandle, rng protocol.Range) ([]protocol.CodeAction, error) {
+func refactorRewrite(ctx context.Context, snapshot source.Snapshot, pkg source.Package, pgf *source.ParsedGoFile, fh source.FileHandle, rng protocol.Range) (_ []protocol.CodeAction, rerr error) {
+ // golang/go#61693: code actions were refactored to run outside of the
+ // analysis framework, but as a result they lost their panic recovery.
+ //
+ // These code actions should never fail, but put back the panic recovery as a
+ // defensive measure.
+ defer func() {
+ if r := recover(); r != nil {
+ rerr = bug.Errorf("refactor.rewrite code actions panicked: %v", r)
+ }
+ }()
start, end, err := pgf.RangePos(rng)
if err != nil {
return nil, err
diff --git a/gopls/internal/regtest/marker/testdata/stubmethods/issue61693.txt b/gopls/internal/regtest/marker/testdata/stubmethods/issue61693.txt
new file mode 100644
index 0000000..8dda662
--- /dev/null
+++ b/gopls/internal/regtest/marker/testdata/stubmethods/issue61693.txt
@@ -0,0 +1,35 @@
+This test exercises stub methods functionality with variadic parameters.
+
+In golang/go#61693 stubmethods was panicking in this case.
+
+-- go.mod --
+module mod.com
+
+go 1.18
+-- main.go --
+package main
+
+type C int
+
+func F(err ...error) {}
+
+func _() {
+ var x error
+ F(x, C(0)) //@suggestedfix(re"C.0.", re"missing method Error", "quickfix", stub)
+}
+-- @stub/main.go --
+package main
+
+type C int
+
+// Error implements error.
+func (C) Error() string {
+ panic("unimplemented")
+}
+
+func F(err ...error) {}
+
+func _() {
+ var x error
+ F(x, C(0)) //@suggestedfix(re"C.0.", re"missing method Error", "quickfix", stub)
+}
diff --git a/gopls/internal/regtest/misc/formatting_test.go b/gopls/internal/regtest/misc/formatting_test.go
index ee8098c..1556bb7 100644
--- a/gopls/internal/regtest/misc/formatting_test.go
+++ b/gopls/internal/regtest/misc/formatting_test.go
@@ -366,3 +366,30 @@
}
})
}
+
+func TestGofumpt_Issue61692(t *testing.T) {
+ testenv.NeedsGo1Point(t, 21)
+
+ const input = `
+-- go.mod --
+module foo
+
+go 1.21rc3
+-- foo.go --
+package foo
+
+func _() {
+ foo :=
+ "bar"
+}
+`
+
+ WithOptions(
+ Settings{
+ "gofumpt": true,
+ },
+ ).Run(t, input, func(t *testing.T, env *Env) {
+ env.OpenFile("foo.go")
+ env.FormatBuffer("foo.go") // golang/go#61692: must not panic
+ })
+}
