Fix LSP server not giving completion when a non-contract meta-information is present in a declaration #991
Conversation
lsp/nls/src/requests/completion.rs
Outdated
| Some(meta_value) => { | ||
| let result = find_fields_from_meta_value(meta_value, path); | ||
| (!result.is_empty()).then_some(result) | ||
| } |
There was a problem hiding this comment.
I'm not sure to understand why this fixes the issue, but I imagine you tested the result. Is it because find_fields_from_contracts is called somewhere in a chain, and if it returns something, we keep the result, otherwise we try to find a completion by other means?
If this is case, why don't we aggregate all source of information (types, contracts, literal definition)? For example, if the contract is {foo, ..} but we know that the record is equal to {foo = 1, bar = 2}, does this mean that even after this PR, completion won't show bar?
There was a problem hiding this comment.
Is it because find_fields_from_contracts is called somewhere in a chain, and if it returns something, we keep the result, otherwise we try to find a completion by other means?
Yes.
I think aggregation is better. Thanks!
|
Yes, we have a function that removes duplicates. But we can also avoid that entirely by collecting the completion items with a Currently writing some tests will merge once that is completed. |
ebresafegaga
force-pushed
the
fix/lsp-contract
branch
from
f60fa6f
to
ec3e6b8
Compare
lsp/nls/src/requests/completion.rs
Outdated
| let mut fst = find_fields_from_contract(linearization, *body_id, path) | ||
| .unwrap_or_default(); | ||
| let snd = | ||
| find_fields_from_term_kind(linearization, id, &mut p).unwrap_or_default(); |
There was a problem hiding this comment.
Then, does it make sense anymore to have an option returned by find_fields_from if we're aggregating? Would an empty collection be enough and make sense? (getting wrap of the unwrap_or_default)
There was a problem hiding this comment.
Yes, that would be much better. Thank you!
There was a problem hiding this comment.
But on a second thought, most of these functions use the monadic ? operator on Options to make error propagation or early return much cleaner
There was a problem hiding this comment.
But do they need that at all? Couldn't you just return an empty collection instead?
There was a problem hiding this comment.
In any case, returning an option should be decided first and foremost by the semantics of the function and the callers though
|
The CI is failing because the order of completion suggestions is not the same between the expected value and the actual value in some of the tests. I guess you want to sort your vectors inside your tests (or in the LSP directly)? |
Closes #989