From 5210e0ca159383b30b65329a9e96e03da956a2c7 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Wed, 23 Feb 2022 11:34:11 -0500 Subject: [PATCH] gopls: wire in LangVersion and ModulePath for gofumpt formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If available, pass the LangVersion and ModulePath provided by go/packages to gofumpt. This allows gofumpt to apply additional formatting rules that require this information. Also add a regression test for gofumpt formatting. Fixes golang/go#51327 Change-Id: I47c8c96d595d62e1c444285ce69ce6a4e61fa74c Reviewed-on: https://go-review.googlesource.com/c/tools/+/387634 Trust: Robert Findley Run-TryBot: Robert Findley Trust: Daniel Martí gopls-CI: kokoro Reviewed-by: Hyang-Ah Hana Kim TryBot-Result: Gopher Robot --- gopls/internal/hooks/hooks.go | 10 +-- .../internal/regtest/misc/formatting_test.go | 66 +++++++++++++++++++ internal/lsp/cache/metadata.go | 5 ++ internal/lsp/cache/snapshot.go | 6 +- internal/lsp/source/format.go | 20 +++++- internal/lsp/source/options.go | 15 +++-- internal/lsp/source/view.go | 4 ++ 7 files changed, 111 insertions(+), 15 deletions(-) diff --git a/gopls/internal/hooks/hooks.go b/gopls/internal/hooks/hooks.go index 3ca21e5039..487fb60821 100644 --- a/gopls/internal/hooks/hooks.go +++ b/gopls/internal/hooks/hooks.go @@ -22,14 +22,10 @@ func Options(options *source.Options) { options.ComputeEdits = ComputeEdits } options.URLRegexp = relaxedFullWord - options.GofumptFormat = func(ctx context.Context, src []byte) ([]byte, error) { + options.GofumptFormat = func(ctx context.Context, langVersion, modulePath string, src []byte) ([]byte, error) { return format.Source(src, format.Options{ - // TODO: fill LangVersion and ModulePath from the current go.mod. - // The information is availabe as loaded by go/packages via the - // Module type, but it needs to be wired up all the way here. - // We likely want to change the GofumptFormat field type to take - // extra parameters, to then be used in format.Options. - // See https://pkg.go.dev/mvdan.cc/gofumpt@v0.3.0/format#Options. + LangVersion: langVersion, + ModulePath: modulePath, }) } updateAnalyzers(options) diff --git a/gopls/internal/regtest/misc/formatting_test.go b/gopls/internal/regtest/misc/formatting_test.go index 67e7939134..75d8f62245 100644 --- a/gopls/internal/regtest/misc/formatting_test.go +++ b/gopls/internal/regtest/misc/formatting_test.go @@ -301,3 +301,69 @@ func main() { } }) } + +func TestGofumptFormatting(t *testing.T) { + + // Exercise some gofumpt formatting rules: + // - No empty lines following an assignment operator + // - Octal integer literals should use the 0o prefix on modules using Go + // 1.13 and later. Requires LangVersion to be correctly resolved. + // - std imports must be in a separate group at the top. Requires ModulePath + // to be correctly resolved. + const input = ` +-- go.mod -- +module foo + +go 1.17 +-- foo.go -- +package foo + +import ( + "foo/bar" + "fmt" +) + +const perm = 0755 + +func foo() { + foo := + "bar" + fmt.Println(foo, bar.Bar) +} +-- foo.go.formatted -- +package foo + +import ( + "fmt" + + "foo/bar" +) + +const perm = 0o755 + +func foo() { + foo := "bar" + fmt.Println(foo, bar.Bar) +} +-- bar/bar.go -- +package bar + +const Bar = 42 +` + + WithOptions( + EditorConfig{ + Settings: map[string]interface{}{ + "gofumpt": true, + }, + }, + ).Run(t, input, func(t *testing.T, env *Env) { + env.OpenFile("foo.go") + env.FormatBuffer("foo.go") + got := env.Editor.BufferText("foo.go") + want := env.ReadWorkspaceFile("foo.go.formatted") + if got != want { + t.Errorf("unexpected formatting result:\n%s", tests.Diff(t, want, got)) + } + }) +} diff --git a/internal/lsp/cache/metadata.go b/internal/lsp/cache/metadata.go index bef7bf8e70..618578dd89 100644 --- a/internal/lsp/cache/metadata.go +++ b/internal/lsp/cache/metadata.go @@ -56,6 +56,11 @@ func (m *Metadata) PackagePath() string { return string(m.PkgPath) } +// ModuleInfo implements the source.Metadata interface. +func (m *Metadata) ModuleInfo() *packages.Module { + return m.Module +} + // KnownMetadata is a wrapper around metadata that tracks its validity. type KnownMetadata struct { *Metadata diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index b9cd36c745..e786c02afb 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1025,7 +1025,11 @@ func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]source. var mds []source.Metadata for _, id := range knownIDs { md := s.getMetadata(id) - mds = append(mds, md) + // TODO(rfindley): knownIDs and metadata should be in sync, but existing + // code is defensive of nil metadata. + if md != nil { + mds = append(mds, md) + } } return mds, nil } diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index 4f02e4f34e..79da0b3adb 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -63,8 +63,24 @@ func Format(ctx context.Context, snapshot Snapshot, fh FileHandle) ([]protocol.T // Apply additional formatting, if any is supported. Currently, the only // supported additional formatter is gofumpt. - if format := snapshot.View().Options().Hooks.GofumptFormat; snapshot.View().Options().Gofumpt && format != nil { - b, err := format(ctx, buf.Bytes()) + if format := snapshot.View().Options().GofumptFormat; snapshot.View().Options().Gofumpt && format != nil { + // gofumpt can customize formatting based on language version and module + // path, if available. + // + // Try to derive this information, but fall-back on the default behavior. + // + // TODO: under which circumstances can we fail to find module information? + // Can this, for example, result in inconsistent formatting across saves, + // due to pending calls to packages.Load? + var langVersion, modulePath string + mds, err := snapshot.MetadataForFile(ctx, fh.URI()) + if err == nil && len(mds) > 0 { + if mi := mds[0].ModuleInfo(); mi != nil { + langVersion = mi.GoVersion + modulePath = mi.Path + } + } + b, err := format(ctx, langVersion, modulePath, buf.Bytes()) if err != nil { return nil, err } diff --git a/internal/lsp/source/options.go b/internal/lsp/source/options.go index c7c6228ad2..b1df290595 100644 --- a/internal/lsp/source/options.go +++ b/internal/lsp/source/options.go @@ -462,11 +462,16 @@ func (u *UserOptions) SetEnvSlice(env []string) { // Hooks contains configuration that is provided to the Gopls command by the // main package. type Hooks struct { - LicensesText string - GoDiff bool - ComputeEdits diff.ComputeEdits - URLRegexp *regexp.Regexp - GofumptFormat func(ctx context.Context, src []byte) ([]byte, error) + LicensesText string + GoDiff bool + ComputeEdits diff.ComputeEdits + URLRegexp *regexp.Regexp + + // GofumptFormat allows the gopls module to wire-in a call to + // gofumpt/format.Source. langVersion and modulePath are used for some + // Gofumpt formatting rules -- see the Gofumpt documentation for details. + GofumptFormat func(ctx context.Context, langVersion, modulePath string, src []byte) ([]byte, error) + DefaultAnalyzers map[string]*Analyzer TypeErrorAnalyzers map[string]*Analyzer ConvenienceAnalyzers map[string]*Analyzer diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index dba45f76a4..9e9b035753 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -18,6 +18,7 @@ import ( "golang.org/x/mod/modfile" "golang.org/x/mod/module" "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/gocommand" "golang.org/x/tools/internal/imports" "golang.org/x/tools/internal/lsp/progress" @@ -319,6 +320,9 @@ type Metadata interface { // PackagePath is the package path. PackagePath() string + + // ModuleInfo returns the go/packages module information for the given package. + ModuleInfo() *packages.Module } // Session represents a single connection from a client.