Skip to content

Commit

Permalink
internal/lsp: refactor workspace Symbol method
Browse files Browse the repository at this point in the history
This is based on Paul Jolly's CL 228760, updated to use the new cache
API, support the symbolStyle configuration option, and lift up the
concept of symbol score for later improvement.

From that CL:

There are a number of issues with the current implementation:

* test variant packages are not handled correctly, meaning duplicate
  symbols are returned
* fuzzy results are not ordered by score

We refactor the implementation of workspace symbol to use a
symbolCollector that carries context during the walk for symbols. As
part of resolving the test variant issue, we first determine a list of
packages to walk.

(*symbolCollector).collectPackages gathers the packages we are going to
inspect for symbols. This pre-step is required in order to filter out
any "duplicate" *types.Package. The duplicates arise for packages that
have test variants.  For example, if package mod.com/p has test files,
then we will visit two packages that have the PkgPath() mod.com/p: the
first is the actual package mod.com/p, the second is a special version
that includes the non-XTest _test.go files. If we were to walk both of
of these packages, then we would get duplicate matching symbols and we
would waste effort. Therefore where test variants exist we walk those
(because they include any symbols defined in non-XTest _test.go files).

One further complication is that even after this filtering, packages
between views might not be "identical" because they can be built using
different build constraints (via the "env" config option). Therefore on
a per view basis we first build up a map of PkgPath() -> *types.Package
preferring the test variants if they exist. Then we merge the results
between views, de-duping by *types.Package.

Finally, when we come to walk these packages and start gathering
symbols, we ignore any files we have already seen (due to different
*types.Package for the same import path as a result of different build
constraints), keeping track of those symbols via symbolCollector.

Then we walk that list of packages in much the same way as before.

For golang/go#40548

Co-authored-by: Paul Jolly <paul@myitcv.io>
Change-Id: I8af5bdedbd4a6c3631a213d73a735aea556a13ae
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247818
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
  • Loading branch information
findleyr and myitcv committed Aug 27, 2020
1 parent 84ab570 commit 88346e9
Show file tree
Hide file tree
Showing 11 changed files with 419 additions and 190 deletions.
503 changes: 324 additions & 179 deletions internal/lsp/source/workspace_symbol.go

Large diffs are not rendered by default.

60 changes: 60 additions & 0 deletions internal/lsp/source/workspace_symbol_test.go
@@ -0,0 +1,60 @@
// Copyright 2020 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 source

import (
"strings"
"testing"
)

func TestBestMatch(t *testing.T) {
tests := []struct {
desc string
symbol string
matcher matcherFunc
wantMatch string
wantScore float64
}{
{
desc: "shortest match",
symbol: "foo/bar/baz.quux",
matcher: func(string) float64 { return 1.0 },
wantMatch: "quux",
wantScore: 1.0,
},
{
desc: "partial match",
symbol: "foo/bar/baz.quux",
matcher: func(s string) float64 {
if strings.HasPrefix(s, "bar") {
return 1.0
}
return 0.0
},
wantMatch: "bar/baz.quux",
wantScore: 1.0,
},
{
desc: "longest match",
symbol: "foo/bar/baz.quux",
matcher: func(s string) float64 {
parts := strings.Split(s, "/")
return float64(len(parts))
},
wantMatch: "foo/bar/baz.quux",
wantScore: 3.0,
},
}

for _, test := range tests {
test := test
t.Run(test.desc, func(t *testing.T) {
gotMatch, gotScore := bestMatch(test.symbol, test.matcher)
if gotMatch != test.wantMatch || gotScore != test.wantScore {
t.Errorf("bestMatch(%q, matcher) = (%q, %.2g), want (%q, %.2g)", test.symbol, gotMatch, gotScore, test.wantMatch, test.wantScore)
}
})
}
}
@@ -0,0 +1,3 @@
package a

var RandomGopherTestVariableA = "a" //@symbol("RandomGopherTestVariableA", "RandomGopherTestVariableA", "Variable", "", "a.RandomGopherTestVariableA")
@@ -0,0 +1,3 @@
-- symbols --
RandomGopherTestVariableA Variable 3:5-3:30

@@ -0,0 +1,3 @@
package a_test

var RandomGopherXTestVariableA = "a" //@symbol("RandomGopherXTestVariableA", "RandomGopherXTestVariableA", "Variable", "", "a_test.RandomGopherXTestVariableA")
@@ -0,0 +1,3 @@
-- symbols --
RandomGopherXTestVariableA Variable 3:5-3:31

26 changes: 16 additions & 10 deletions internal/lsp/testdata/lsp/primarymod/workspacesymbol/fuzzy/fuzzy.go
Expand Up @@ -2,25 +2,31 @@ package fuzzy

/*@
workspacesymbolfuzzy("rgop",
RandomGopherVariableA,
RandomGopherConstantA,
bBar,
randomgopherinvariable,
RandomGopherVariableA,
RandomGopherXTestVariableA,
RandomGopherVariableB,
RandomGopherStructB,
bBar,
RandomGopherTestVariableA,
RandomGopherConstantA,
)
workspacesymbolfuzzy("randoma",
RandomGopherVariableA,
RandomGopherConstantA,
randomgopherinvariable,
RandomGopherVariableB,
bBar,
RandomGopherVariableB,
randomgopherinvariable,
RandomGopherXTestVariableA,
RandomGopherTestVariableA,
RandomGopherConstantA,
RandomGopherVariableA,
)
workspacesymbolfuzzy("randomb",
RandomGopherVariableA,
bBar,
randomgopherinvariable,
RandomGopherVariableB,
RandomGopherTestVariableA,
RandomGopherXTestVariableA,
RandomGopherVariableA,
RandomGopherStructB,
bBar,
RandomGopherVariableB,
)
*/
Expand Up @@ -2,5 +2,7 @@
workspacesymbol/a/a.go:3:5-26 a.RandomGopherVariableA Variable
workspacesymbol/a/a.go:5:7-28 a.RandomGopherConstantA Constant
workspacesymbol/a/a.go:8:2-24 a.randomgopherinvariable Constant
workspacesymbol/a/a_test.go:3:5-30 a.RandomGopherTestVariableA Variable
workspacesymbol/a/a_x_test.go:3:5-31 a_test.RandomGopherXTestVariableA Variable
workspacesymbol/b/b.go:3:5-26 b.RandomGopherVariableB Variable
workspacesymbol/b/b.go:6:2-5 b.RandomGopherStructB.Bar Field
@@ -1,6 +1,8 @@
-- workspace_symbol --
workspacesymbol/a/a.go:3:5-26 a.RandomGopherVariableA Variable
workspacesymbol/a/a.go:8:2-24 a.randomgopherinvariable Constant
workspacesymbol/a/a_test.go:3:5-30 a.RandomGopherTestVariableA Variable
workspacesymbol/a/a_x_test.go:3:5-31 a_test.RandomGopherXTestVariableA Variable
workspacesymbol/b/b.go:3:5-26 b.RandomGopherVariableB Variable
workspacesymbol/b/b.go:5:6-25 b.RandomGopherStructB Struct
workspacesymbol/b/b.go:6:2-5 b.RandomGopherStructB.Bar Field
Expand Up @@ -2,6 +2,8 @@
workspacesymbol/a/a.go:3:5-26 a.RandomGopherVariableA Variable
workspacesymbol/a/a.go:5:7-28 a.RandomGopherConstantA Constant
workspacesymbol/a/a.go:8:2-24 a.randomgopherinvariable Constant
workspacesymbol/a/a_test.go:3:5-30 a.RandomGopherTestVariableA Variable
workspacesymbol/a/a_x_test.go:3:5-31 a_test.RandomGopherXTestVariableA Variable
workspacesymbol/b/b.go:3:5-26 b.RandomGopherVariableB Variable
workspacesymbol/b/b.go:5:6-25 b.RandomGopherStructB Struct
workspacesymbol/b/b.go:6:2-5 b.RandomGopherStructB.Bar Field
2 changes: 1 addition & 1 deletion internal/lsp/testdata/lsp/summary.txt.golden
Expand Up @@ -20,7 +20,7 @@ HighlightsCount = 69
ReferencesCount = 11
RenamesCount = 29
PrepareRenamesCount = 7
SymbolsCount = 3
SymbolsCount = 5
WorkspaceSymbolsCount = 2
FuzzyWorkspaceSymbolsCount = 3
CaseSensitiveWorkspaceSymbolsCount = 2
Expand Down

0 comments on commit 88346e9

Please sign in to comment.