Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: fuzzy matching in unimported completion interferes with "good" (prefix/substring) choices #36591

Closed
zikaeroh opened this issue Jan 16, 2020 · 14 comments
Assignees
Labels

Comments

@zikaeroh
Copy link

@zikaeroh zikaeroh commented Jan 16, 2020

Please answer these questions before submitting your issue. Thanks!

What did you do?

In my project (https://github.com/hortbot/hortbot, relatively large overall dependency set), add a new test file with no imports, then make a new test and complete on unimported packages.

What did you expect to see?

Typing something like errors.New or assert.NilError gives me a resonable set of choices. I.e. I want to see errors.New, or pkg/errors.New, etc, at the top as they are perfect matches.

What did you see instead?

For errors.New, I get New from a few packages, then a bunch of other results I don't care about that contain the letters "NEW" in order (NewResourceCreationErrorEnum, LabelErrorEnum_CANNOT_ATTACH_NON_MANAGER_LABEL_TO_CUSTOMER), then more exact New matches, and them more fuzzy matches, and so on.

Logs: https://gist.github.com/zikaeroh/385ca2756bffd1af9ab49e199c526a2b

Build info

golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@v0.1.8-0.20200116011002-7042ee646e79 h1:DR0yHhyXch/edNKuX31OXiMcMp1SLwPZnQOx3WwNprI=
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/sergi/go-diff@v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
    golang.org/x/mod@v0.1.1-0.20191105210325-c90efee705ee h1:WG0RUwxtNT4qqaXX3DPA8zHFNm/D9xaBpxzHt1WcA/E=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20200116011002-7042ee646e79 h1:vUYHqXKakw93ZB7hlBqmqr5mOVwQaMWS38qZgBHpdIQ=
    golang.org/x/xerrors@v0.0.0-20191011141410-1b5146add898 h1:/atklqdjdhuosWIl6AIbOeHJjicWYPqR9bpxqxYG2pA=
    honnef.co/go/tools@v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=
    mvdan.cc/xurls/v2@v2.1.0 h1:KaMb5GLhlcSX+e+qhbRJODnUUBvlw01jt4yrjFIHAuA=

Go info

go version go1.13.6 linux/amd64

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jake/.cache/go-build"
GOENV="/home/jake/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jake/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jake/zikaeroh/hortbot/hortbot/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build966422196=/tmp/go-build -gno-record-gcc-switches"
@gopherbot gopherbot added this to the Unreleased milestone Jan 16, 2020
@gopherbot gopherbot added the Tools label Jan 16, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 16, 2020

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@gopherbot gopherbot added the gopls label Jan 16, 2020
@heschik heschik self-assigned this Jan 16, 2020
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 16, 2020

cc @muirmanders for fuzzy matching issues, but I'm confident there are unimported completion scoring issues in here too so I'll take it for now.

@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Jan 16, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 16, 2020

Change https://golang.org/cl/215023 mentions this issue: internal/lsp/source: score in-memory unimported candidates

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jan 16, 2020

It appears the fuzzy matcher gives the same (perfect) score to candidates "New" and "NewFoo" when searching on "New". We can either change the fuzzy matcher to add a penalty for non-exact matches, or we can change gopls to prefer shorter candidates when the scores are otherwise identical. I would guess the latter is a better approach, especially considering that there are different matchers beyond fuzzy.

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jan 16, 2020

Actually, the existing sort by candidate label should already order shorter candidates first when scores are equal. If "NewFoo" shows up before "New", it must be because "NewFoo" has a higher score than "New".

gopherbot pushed a commit to golang/tools that referenced this issue Jan 17, 2020
We were assuming that all in-memory packages were equally useful. That's
not true for projects with a large dependency tree. Call into the
imports code to score them.

While I'm here, score the main module above direct deps.

Updates golang/go#36591.

Change-Id: I07c56dd3ff7338e76f3643e18d35abc1b52d6763
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215023
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 17, 2020

You're right, something is giving the worse candidates a much higher score than I set by a factor of 10. All my scores are .01 * 7 at most.

source.CompletionItem{Label:"AdErrorEnum_CANNOT_SET_FIELD_WITH_AD_ID_SET_FOR_SHARING", Detail:"(from \"google.golang.org/genproto/googleapis/ads/googleads/v1/errors\")", InsertText:"AdErrorEnum_CANNOT_SET_FIELD_WITH_AD_ID_SET_FOR_SHARING", Kind:var, AdditionalTextEdits:[]protocol.TextEdit{protocol.TextEdit{Range:1:0-1:0, NewText:"\nimport \"google.golang.org/genproto/googleapis/ads/googleads/v1/errors\"\n"}}, Depth:0, Score:0.4, snippet:(*snippet.Builder)(nil), Documentation:""}
vs
source.CompletionItem{Label:"As", Detail:"func(err error, target interface{}) bool (from \"errors\")", InsertText:"As", Kind:func, AdditionalTextEdits:[]protocol.TextEdit{protocol.TextEdit{Range:1:0-1:0, NewText:"\nimport \"errors\"\n"}}, Depth:0, Score:0.07, snippet:(*snippet.Builder)(0xc011946cc0), Documentation:"As finds the first error in err's chain that matches target, and if so, sets target to that error value and returns true."}

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 17, 2020

https://github.com/golang/tools/blob/6edc0a871e694a1c8728996c84668863c13d2b4f/internal/lsp/source/completion.go#L326

maybe? I think we should have a rule that scores only go one direction, probably down, from the ones passed in.

@heschik heschik modified the milestones: gopls unplanned, gopls/v0.3.0 Jan 17, 2020
@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jan 17, 2020

something is giving the worse candidates a much higher score

All non-typed candidates end up getting the highScore multiplier: https://github.com/golang/tools/blob/6edc0a871e694a1c8728996c84668863c13d2b4f/internal/lsp/source/completion.go#L1682

we should have a rule that scores only go one direction

The code that discovers a candidate doesn't normally need to be concerned with the quality of the candidate. If the candidate ends up being a good match, it gets the highScore multiplier. If it is a poor match, it gets the lowScore multiplier. If it is a maybe-annoying candidate it gets a more moderate score penalty. We could make it only go down, but then the caller of found() would need to call matchingCandidate() to set the starting score.

The untyped unimported candidates are tricky to rank properly relative to typed candidates. I would make typed unimported candidates use stdScore as a base (modified slightly by any relevance data), and make the untyped candidates use maybe 0.9 * stdScore as the base so they are downranked relative to typed candidates if all else is equal (but not too far so that it doesn't take much extra typing to get the untyped candidate as the top candidate). In general it doesn't seem like the unimported package members need to use such low scores. The unimported package name candidates should be downranked a bit, but they also probably don't need such a low score (e.g. 0.5 * stdScore might be sufficient).

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 17, 2020

We could make it only go down, but then the caller of found() would need to call matchingCandidate() to set the starting score.

I don't think that's true? Anywhere we multiply by 10 today, the caller could pass score*10 and we could divide by 10 everywhere we aren't multiplying now. Just so that it's more predictable what the score range of any given call could be.

I reviewed all the callers of matchingCandidate, and it looks like it will be harmless to return false instead of true -- it controls stuff like creating composite literals and taking references, none of which makes sense if you don't know the type. That resolves the immediate complaint in this bug. CL shortly.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 17, 2020

Change https://golang.org/cl/215322 mentions this issue: internal/lsp/source: fix ranking of untyped completions

gopherbot pushed a commit to golang/tools that referenced this issue Jan 17, 2020
Claiming that untyped candidates matched the type of whatever we were
looking for messed up rankings in found(). The only other places that
use it will all work better with false. Return false.

Updates golang/go#36591.

Change-Id: I5e1e8af7cc5c27422740cbb77f9a4a20edb1e447
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215322
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 17, 2020

Hopefully this is fixed enough for v0.3. @muirdm, do you think we need to make the changes you described above, or just keep them in mind in case of future issues? I don't fully understand the implications.

@heschik heschik modified the milestones: gopls/v0.3.0, gopls unplanned Jan 17, 2020
@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jan 17, 2020

We don't need to do anything now. We should keep it in mind if we get reports like "unimported candidate xyz not ranked first after typing xy" or similar unexpected ranking issues.

@zikaeroh

This comment has been minimized.

Copy link
Author

@zikaeroh zikaeroh commented Jan 17, 2020

Victory. 😄

image

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jan 17, 2020

Cool, closing.

@heschik heschik closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.