Return errors instead of panics #638
Conversation
|
@microsoft-github-policy-service agree |
| program := l.GetProgram() | ||
| file := program.GetSourceFile(fileName) | ||
| if file == nil { | ||
| panic("file not found") |
There was a problem hiding this comment.
My understanding is that these are mainly invariants that shouldn't be broken, such that we'd want to know about them. But, maybe we need to surface these differently (like gopls' bug.Report), or as a general panic handler (sort of like old TS with exceptions, which we also did not use except in tragic cases).
There was a problem hiding this comment.
Yeah, but it happened in the mentioned issue, and I'm not sure it should crash the LSP server.
I don't have any strong opinion, because it's always up to the team and the project. But in general, panics are for unexpected errors, this one (even if it should never happen in real life) is kinda expected.
And I'm not a big fan of global recovery exactly because if some panic happens, we'd want to know about it.
I think the same problem would be reported in issue even without panic anyway, since it's a bug (at least looks like the one).
There was a problem hiding this comment.
I agree, lsp servers shouldn't crash for file not found cases. At least the ts lsp , go lsp doesn't crash. I think we need an error channel to manage errors async. The Current version #645 seems to be halting at every error.
tsgo heavily overuses panics, that can lead to the situations mentioned in #631:
PR is intended to start moving the codebase towards more idiomatic error handling.
Atm, I've changed 2 panics and we probably need to agree on the approach: I can keep adding changes to this PR, but it'll be big and hard to follow, or I can create a separate PRs to keep them small and simple.