tweag / nickel

Better configuration for less
https://nickel-lang.org/
MIT License
2.23k stars 85 forks source link

LSP: Completion shouldn't suggest variable names for record fields #1932

Closed thufschmitt closed 2 weeks ago

thufschmitt commented 1 month ago

Describe the bug

When a completion is triggered while declaring the name of a record field, the LSP will suggest known fields based on the contract/type/merges for the record (which is awesome), but also based on the variables in scope. This is annoying since

  1. It is likely to be more noise than anything because there's no particular reason for these variables to be interesting record field names.
  2. When exposed to the final consumer of the configuration, it would be much friendlier if the completion was only suggesting valid configuration options, which isn't the case right now

To Reproduce

let foo = {} in
{
  <CURSOR>
}

Triggering the completion will suggest foo (and std), but there's no particular reason for it to be relevant.

A side-effect (motivating the 2. annoyance) is that

let foo = {} in
{
  <CURSOR>
} | { bar | Number = 1 }

will suggest foo, while putting foo here will break the contract.

Expected behavior

Only suggest based on the type/contract/merge

Environment

Additional context

jneem commented 2 weeks ago

I think this can be considered closed by #1940, which makes a separation between names in the environment and record fields.

The current situation isn't perfect because it doesn't understand the order of merges, or the difference between contracts and merges. So for example, it will still suggest foo in

({ <CURSOR> } | { bar | Number }) & { foo = 1 }