influxdata / flux-lsp

Implementation of Language Server Protocol for the flux language
MIT License
26 stars 5 forks source link

Variable completion casts too wide of a net #458

Open rockstar opened 2 years ago

rockstar commented 2 years ago

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.

Marwes commented 2 years ago

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.

rockstar commented 2 years ago

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.

onelson commented 2 years ago

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.

Marwes commented 2 years ago

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.

rockstar commented 2 years ago
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.

Marwes commented 2 years ago

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.