Combine metadata for completion items instead of choosing arbitrarily.

[jneem]

Fixes #1933

[yannham]

Sorry, I proposed this solution, but I just realize it's not correct, because you don't really know if you should actually conflate all foos or if they are logically different.

For example:

let C = { foo | Number | doc "Some documentation" } in
{
  foo | String,
  bar = {
    f<CURSOR> 
  } | C
}

In this example, the LSP incorrectly suggests that the contracts for foo are String, Number. But the foo from the enclosing scope and the one coming from C shouldn't actually be merged.

Could we have a way of computing if the external foo is actually shadowed by the inner foo (as in the example above), in which case the correct behavior is to show only the inner one? I guess this was the original behavior, right? The difficult part is to know when they aren't actually shadowing each other but are "at the same level" and are going to be merged.

I thought that another dumber solution could be to just special case the pattern of the original issue (which is common, a bit like "declaring a function argument"), but even that is incorrect:

let C = { foo | Number | doc "Some documentation" } in
{
  bar = {
    foo,

    f<CURSOR> 
  }
} | C

This solution would suggest that foo must be Number, while once, again, the two foos are different and the inner one is unconstrained.

[jneem]

Hm, I think the issue isn't just the scope, but the syntactic position of the thing we're completing, right? I mean, if you're trying to complete the value instead of the field name like this:

let C = { foo | Number | doc "Some documentation", .. } in
{
  foo | String,
  bar = {
    baz = f<CURSOR> 
  } | C
}

then you want the string one, right? The current implementation does take scoping into account (for example, in let foo = 1 in let foo = 2 in fo<CURSOR> it knows which foo is in scope), but it's combining multiple sources of completions (specifically: variables in scope, and "cousins" in record merges) without knowing which sources are appropriate for the context.

I think the mechanism for this should be somewhat similar to #1932, where we need to do filtering of completion candidates for field names.

Also, the current behavior just takes the metadata from an arbitrary completion candidate. I think that combining all valid completion candidates is the right behavior; we just need some more work to filter out invalid ones. As an example where you'd want to merge multiple candidates:

let GenericC = { foos | Array Dyn | doc "some things"} in
let SpecificC = { foos  | Array String | doc "some specific things" } in
{
  fo<CURSOR>
} | GenericC | SpecificC

(Ok, so in this example we actually want the docs from SpecificC, but this might be hard to determine so I think merging them is fine.)

[yannham]

Hm, I think the issue isn't just the scope, but the syntactic position of the thing we're completing, right? I mean, if you're trying to complete the value instead of the field name like this:

Ah, this is a good point, but I think still rather a separate one. At any location in a source file, there is a local environment where many foos can appear coming from different sources/scopes. The original issue is, some of them will actually be merged in the final result (cousins, siblings?), so you want to merge their metadata to have maximal information. I believe which one you should merge is entirely based on scoping (maybe some extended notion of scoping, because in Nickel {foo | C = ...}doesn't bring the variables ofC` into scope, but completion does - the point stands that it's purely lexical).

In your last example we should indeed merge them - the issue with merging documentation is unfortunate, but kinda beside the point. In fact we decided to just arbitrarily get rid of one docstring in combine, but we could very much concatenate documentation when they're non-empty and different.

Then, it turns out we only want to keep one suggestion, because of shadowing. So it's true that in practice this is equivalent to decide "should I get rid of this other suggestion or merge it with the current, most local one?". In the end we're probably saying the same thing in a different way 🙂

About the fact that you might want a different rationale for completion when defining a field or using a field, I think it boils down to the official Nickel scoping versus NLS completion "scoping": stuff brought into "scope" by contracts and types aren't considered to be into scope by Nickel (yet, but some people would actually expect it, see #1915 (comment) and the whole discussion). So maybe just filtering out stuff coming from types/contracts for completion in the right-hand side of ab =, while considering it as a proper part of the environment like any other sibling for the left hand side is sufficient?

At a distance it looks like the more involved part is deciding what to merge and what to drop. I imagine you'd need some sort of "level" or similar metadata attached to bindings in the environment that help you decide which declaration takes precedence, and which declarations are at the same level and should be merged. Then, when doing {...record literal...} | C, completion info coming from C should be at the same level as top-level recursive fields in the record literal; that is, if the literal is {foo = .., ..etc..} and C = {bar, baz}, then level(foo) == level(bar) == level(baz).

[jneem]

@yannham I think this is ready for another look. I basically just separated out an extra case, so now we have three cases: completing record paths like foo.bar.baz in expression position, completing record field names like { foo.bar.baz }, and everything else. Only the "everything else" looks in the environment.

There's one (pre-existing) bug that I know of, in the that incomplete parsing heuristics misclassify { foo.bar. } as the first case instead of the second. I'm not sure of a quick way to improve this...

(edit: this will also fix #1932)

[yannham]

Could be nice to write documentation for this function explaining that it combines the metadata of identical items, because I fear it's not obvious from the function's name anymore

[jneem]

I refactored it a little to make it more clear (and also added documentation)

[yannham]

@jneem Oh and I forgot: could we also have the test case that I included in my comment above:

let C = { foo | Number | doc "Some documentation" } in
{
  foo | String,
  bar = {
    f<CURSOR> 
  } | C
}

I'm not entirely convinced the issue is solved, because I don't know where foos are coming from here, in the divide field_completion/env_completion

[jneem]

The foo | String will be in the environment so it will be there if we ask for env_completion. The foo | Number will come from the "cousin" (really more like a sibling) record so it will be there if we ask for field_completion. In that cursor position it asks for field completion, so it will get only foo | Number. An example where you'd get env completion instead would be

let C = { foo | Number | doc "Some documentation" } in
{
  foo | String,
  bar = {
    f = fo<CURSOR> 
  } | C
}