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

Jump to bottom

x/tools/gopls: consider disambiguating same-score completion candidates using candidate length #40262

Open
muirdm opened this issue Jul 16, 2020 · 7 comments
Open

x/tools/gopls: consider disambiguating same-score completion candidates using candidate length #40262

muirdm opened this issue Jul 16, 2020 · 7 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/unplanned

Comments

@muirdm
Copy link

muirdm commented Jul 16, 2020 •
edited

Currently completion candidates are sorted by score, and then secondarily sorted lexicographically to provide a predictable order. I propose we instead do a secondary sort by candidate length, preferring shorter candidates. My intuition is that shorter names are used more frequently than longer names, so it is a somewhat better heuristic to put shorter items first, all else being equal.

Edit: Note that this proposal only comes in to play when candidates have identical scores. Currently the tie-break sorting is alphabetical; I'm proposing we switch to a potentially better heuristic. This will have a very small impact in general.

Contrived example:

package main

type myStr struct {
	s string
}

func (m myStr) Get() string {
	return m.s
}

type foo struct {
	ID         myStr
	AardvarkID myStr
}

func main() {
	var f foo
	var _ string = f      // want "f.ID.s" but got "f.AardvarkID.s"
	var _ string = fidget // want "f.ID.Get()" but got "f.AardvarkID.Get()"
}

/cc @heschik because you had an opinion in slack
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jul 16, 2020
@gopherbot gopherbot added this to the Unreleased milestone Jul 16, 2020
@stamblerre stamblerre removed this from the Unreleased milestone Jul 16, 2020
@myitcv
Copy link
Member

myitcv commented Jul 17, 2020

(Based on my limited understanding of how this all works) I think we first need to look at whether fuzzy matching (and therefore scoring) matches people's intuition: #38380. Because my gut feeling is that, ignoring for one second whether the fuzzy matching does/doesn't match one's intuition, sorting by length/lexicographically on top of the fuzzy scoring further skews the results from an intuitive order.

@muirdm
Copy link
Author

muirdm commented Jul 17, 2020

My first example

var _ string = f      // want "f.ID.s" but got "f.AardvarkID.s"

doesn't involve fuzzy matching. I think that, in the absence of more information, defaulting to the shortest one makes more sense than sorting them lexicographically, at least for fields/methods.

For my second example that does involve fuzzy matching, you are probably right that the issue is better fixed via the fuzzy matcher's ranking.

@zephyrtronium
Copy link
Contributor

I use completion to save keystrokes. Putting shorter candidates first counteracts that.

@muirdm
Copy link
Author

muirdm commented Jul 17, 2020

I use completion to save keystrokes. Putting shorter candidates first counteracts that.

Can you provide an example where my proposal would make candidate ranking worse?

Please note that my proposed change only impacts candidates that already have identical scores, which is not that common. The existing tie-break sorting is alphabetical, which is effectively random from a ranking perspective.

@muirdm muirdm changed the title x/tools/gopls: consider sorting completion candidates by length x/tools/gopls: consider disambiguating same-score completion candidates using candidate length Jul 17, 2020
@zephyrtronium
Copy link
Contributor

Thinking about it more, for same-score completions that already have some portion filled – which was what I was thinking about in my previous comment – shortlex order is probably rarely different from the current lexicographic order. On the other hand, both seem ultimately arbitrary. Your intuition was that shorter names are used more frequently than longer ones, but I think the only place that is a convention in Go is for local variable names.

@myitcv
Copy link
Member

myitcv commented Jul 20, 2020

@muirdm

Please note that my proposed change only impacts candidates that already have identical scores

If we "fix" the fuzzy completion algorithm this problem of same-score might go away?

@muirdm
Copy link
Author

muirdm commented Jul 20, 2020

Your intuition was that shorter names are used more frequently than longer ones, but I think the only place that is a convention in Go is for local variable names.

I would guess shorter names are used more frequently for functions, methods and struct fields as well. It is easy enough to write a program to analyze code and collect some numbers, so maybe I will do that.

If we "fix" the fuzzy completion algorithm this problem of same-score might go away?

My first example var _ string = f // want "f.ID.s" but got "f.AardvarkID.s" would not be fixed because it is not related to fuzzy matcher scoring. "f" is an exact prefix of both candidates, and there are no other "f"s later in the candidates.

@stamblerre stamblerre added this to the gopls/unplanned milestone Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/unplanned
Development

No branches or pull requests
5 participants
@zephyrtronium @myitcv @stamblerre @gopherbot @muirdm