More aggressive type/contract deduplication on hover #1984
Conversation
Deduplicate not just the type annotations, but also the contract annotations. Also, try to report Dyn less often.
| // There's no point in repeating the static type in the annotations. | ||
| if let Some(idx) = annotations.iter().position(|a| a == &ty) { | ||
| annotations.remove(idx); | ||
| } |
There was a problem hiding this comment.
I don't understand why we remove the static type from the annotations. annotations is just the list of the type, followed by all contracts, sorted and deduped, right? So, I would expect to find the static type in it, and that it's intentional. Why remove it?
There was a problem hiding this comment.
Ok, I get it. You include the static type in the annotations so that it's deduped (if there is the same as a contract), but you still keep the distinction static type/contracts, so you display the static type separately, and don't want to repeat it in the annotations.
Which I think is the right behavior (the static type is special in that it's the one selected e.g. by the typechecker). Maybe you could add a short mention explaining this (we don't distinguish between types and contracts for the dedup but we will in the LSP answer) to the comment attached to annotations ?
There was a problem hiding this comment.
Yes, exactly. I've added a comment so hopefully it's less mysterious now
lsp/nls/src/requests/hover.rs
Outdated
| annotations.sort(); | ||
| annotations.dedup(); |
There was a problem hiding this comment.
I'm not entirely sure it's ok to sort the annotations. The order of contracts is meaningful in Nickel (different order of application might give a different result). Additionally, if there's no static type, the first contract of the list is taken to be the static type, which also makes a difference statically, and not only at runtime.
There was a problem hiding this comment.
Ok, I'm deduping them without sorting now.
Fixes #1972.
While testing this, I noticed that some field documentation was missed when hovering over the exact field that was annotated, so I fixed that too.