Fix LSP crash when a resolved import term fails to typecheck #1191
Conversation
ebresafegaga
force-pushed
the
lsp/typecheck-crash
branch
from
9efb99b
to
dd1c0fd
Compare
There was a problem hiding this comment.
I think there are two things in one: not crashing on typechecking failure, and showing a nice error message. I have reservation with respect to the second point; however, the first one is valuable in itself. So if you want to separate the two (and remove the error-reporting part), I'm fine merging the first part already.
lsp/nls/src/cache.rs
Outdated
| // We do this to get a list of the imports that have been resolved | ||
| // but don't typecheck, and then, we give an appriopiate diagnostics | ||
| let mut errors = Vec::new(); | ||
| let term = term.traverse::<_, _, ()>( |
There was a problem hiding this comment.
I don't think traversing the whole term again unconditionally (even if no import failed) is reasonable. Some terms might be huge (@vkleen's auto-generated contracts).
If you need this information, would that be possible to build it up during linearization? Keeping e.g. a list of tuples (location, import_id), or even a map HashMap<FileId, Vec<TermPos>>. I won't include the imports that are in a file which failed to parse or typecheck, but I think it's fine for now: at least, a well-typed file will show if there is an issue with its imports.
Secondly, we might want to know if the previous for succeeded, in case we can avoid to do this search in the first place.
There was a problem hiding this comment.
I won't include the imports that are in a file which failed to parse or typecheck
Why though? Isn't the point of having such map to be able to tell the exact position of the import term in the file for a particular import_id?
There was a problem hiding this comment.
Because it seems harder to do. If you build the import during linearization, but the current file fails to typecheck, you might not have visited all the terms and the import node.
Also, the need is maybe less clear cut (at least to me): if the current file doesn't even typecheck, the user has already some local red squiggles to work on anyway. Then, once the current file typechecks, we can show imports that has failed, in a second time.
So, put differently, I believe ignoring the import statements in a file which doesn't typecheck seems easier to do, and the consequence doesn't seem too bad.
|
Sure, I've made a separate pull request for the diagnostics part here #1214 |
Currently, when we open a Nickel file and some of content of one or more imports don't typecheck, the LSP crashes.
This pull request fixes that.