flux-lsp icon indicating copy to clipboard operation
flux-lsp copied to clipboard

Variable completion casts too wide of a net

Open rockstar opened this issue 2 years ago • 6 comments

In the server tests, we have some code that looks like this:

import "strings"
import "csv"

cal = 10
env = "prod01-us-west-2"

cool = (a) => a + 1

c

The test then checks to see what would be expanded from c. This is the current list it would expand to:

        "buckets",
        "cardinality",
        "chandeMomentumOscillator",
        "columns",
        "contains",
        "contrib/RohanSreerama5/naiveBayesClassifier",
        "contrib/anaisdg/anomalydetection",
        "contrib/bonitoo-io/servicenow",
        "contrib/bonitoo-io/tickscript",
        "contrib/bonitoo-io/victorops",
        "contrib/chobbs/discord",
        "count",
        "cov",
        "covariance",
        "csv",
        "cumulativeSum",
        "dict",
        "difference",
        "distinct",
        "duplicate",
        "experimental/csv",
        "experimental/record",
        "findColumn",
        "findRecord",
        "getColumn",
        "getRecord",
        "highestCurrent",
        "hourSelection",
        "increase",
        "influxdata/influxdb/schema",
        "influxdata/influxdb/secrets",
        "internal/location",
        "logarithmicBins",
        "lowestCurrent",
        "reduce",
        "slack",
        "socket",
        "stateCount",
        "stateTracking",
        "testing/expect",
        "truncateTimeColumn",

I haven't looked at the code, but it looks like it's expanding to anything that could possibly have a c (or even a C) in it. If I type c I expect things that start with c (and not C) to be possibly completions.

rockstar avatar May 04 '22 15:05 rockstar

Providing completion for anything that contains the identifier seems good to me, though I think it should prefer things that start with the identifier that we are completing (and thereby list those first).

It is possible to use https://docs.rs/lsp-types/0.93.0/lsp_types/struct.CompletionItem.html#structfield.sort_text to better control the order but I think VS code (and other clients) actually do fuzzy matching/sorting on what the server sends to give better UX https://github.com/microsoft/language-server-protocol/issues/898 so trying to override that may make things worse.

Marwes avatar May 04 '22 17:05 Marwes

Providing completion for anything that contains the identifier seems good to me

I'm not sure I agree, at least in the case of short identifiers. Typing c and getting buckets as a suggestion seems incorrect. The Monaco client seems to agree. It looks like it drops anything that doesn't start with c or have a camel case C in it.

image

The client seems to be hiding this issue, but I think we should suggest less as well.

rockstar avatar May 05 '22 04:05 rockstar

I sort of imagine it'd be nice for clients to get all the options and be able to apply whatever weighting they see fit.

Otherwise, I think we could calculate the levenshtein distance ourselves and send only the N strongest scores. For a single character search, things starting with that character should score better. The more characters entered, the better the results should be.

onelson avatar May 05 '22 04:05 onelson

The client seems to be hiding this issue, but I think we should suggest less as well.

I don't see it as hiding it, it just allows the client to decide how to display the completions in the UI. For that example it seems to already be doing what we want (?) so us sending "extra" completions doesn't seem to cause an issue.

Marwes avatar May 05 '22 09:05 Marwes

image

Here's an example that occurs because the client is trying to be "too smart" with what it considers a complete list. It's re-using a previous list of completables in a way that is more misleading than anything. After seeing this interaction, I'm not sure clients can be trusted to complete things in the way that makes sense, so I'm in favor of never saying that the completion items are complete.

rockstar avatar May 26 '22 14:05 rockstar

Strange, why is it reusing the completions in this case? I'd expect that typing . would trigger a new completion request automatically, and the response then would only return properties from the schema object.

Marwes avatar May 27 '22 11:05 Marwes