Two issues with incomplete input #1550
Conversation
This fixes an off-by-one error in the handling of incomplete input. It also makes import resolution in incomplete input more reliable -- previously it would only work when the file being opened is in the current working directory.
| // is the same as the real file. | ||
| let mut path = PathBuf::from(server.cache.files().name(range.src_id)); | ||
| path.pop(); | ||
| path.push("<incomplete-input>"); |
There was a problem hiding this comment.
Maybe this isn't worth worrying about, but is it possible to have a file whose name is <incomplete-input>?
There was a problem hiding this comment.
Yeah, it's a fair point. We have various other inputs (like "") following the same pattern, but I'll check how much effort it is to have an enum separating synthetic inputs from real files.
| // The way indexing works here is that if the input file is | ||
| // | ||
| // foo‸ | ||
| // | ||
| // then the cursor (denoted by ‸) is at index 3 and the span of foo is [0, | ||
| // 3), which does not contain the cursor. For most purposes we're interested | ||
| // in querying information about foo, so to do that we use the position just | ||
| // *before* the cursor. |
There was a problem hiding this comment.
does this mean that if the cursor is over the first letter of a variable name, we won't get completion for that variable (unless it's the first character of the line)?
There was a problem hiding this comment.
I think it should be fine, basically because we're using the surrounding context to provide completion; we don't actually care about the identifier under the cursor.
To be specific, In the context of a record path you should get completion, because if the input is foo.bar and the cursor is "over" the b then you'll get the list of completions implied by foo.. (This is the usual behavior for LSP; the editor is in charge of doing any filtering based on prefixes etc.)
In the context of a "bare" variable name, like the foo in { foo = 1 } | { foobar | Number } it should also be fine, because we'll see that we're inside a record and use that record's fields and annotations for completion.
|
|
||
| let term = server.lookup_term_by_position(pos)?.cloned(); | ||
| let sanitized_term = term | ||
| .as_ref() | ||
| .and_then(|rt| sanitize_term_for_completion(rt, pos, server)); | ||
| .and_then(|rt| sanitize_term_for_completion(rt, cursor, server)); |
There was a problem hiding this comment.
why is this using cursor rather than pos?
There was a problem hiding this comment.
This was the off-by-one error. The sanitize_term_for_completion function passes the range [error_start, cursor) to get reparsed. It's an exclusive end (like most ranges in rust), and so using pos instead of cursor drops one character too many.
| ### position = { line = 12, character = 30 } | ||
| ### position = { line = 12, character = 26 } |
There was a problem hiding this comment.
are we just changing what these test are testing for here? should we add new tests rather than replacing these ones, or are these ones not serving a useful purpose?
There was a problem hiding this comment.
The change shouldn't have an effect: it's moving the cursor test location from (import "included.ncl").lala‸ to (import "included.ncl").‸lala to check that off-by-one behavior. One could argue that we should test both, but most of the test cases have the cursor at the end so that case is pretty well covered already...
This fixes an off-by-one error in the handling of incomplete input. It also makes import resolution in incomplete input more reliable -- previously it would only work when the file being opened is in the current working directory.