search

smithy-lang / smithy-language-server

A Language Server Protocol implementation for the Smithy IDL
https://smithy.io
Apache License 2.0
33 stars 19 forks source link

More verbosity in logs/output panel in case of errors #105

Closed kubukoz closed 1 year ago

kubukoz commented 1 year ago

Description of changes:

While fighting some issues in a project not loading (recently seeing some similar to #100), I found that we were discarding some useful information, such as stack traces, which makes it more difficult to debug issues on remote machines (such as my colleagues').

This PR adds more e.printStackTrace() in cases where we only printed the exception messages. While this isn't perfect, and it's far from ideal UX when you see potentially duplicate stack traces, I find that in this case more is better than none.

Many of the issues I'm seeing anytime something goes wrong in loading a project are NPEs on the project field in SmithyTextDocumentService, so I also marked that as nullable to make it clearer that it's not a safe field to use in all cases. We can amend the usages of that field in future PRs so that they have reasonable fallbacks.

Example of what would happen after this change given some issue in FileCachingCollector (responsible for things like #100):

image

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

milesziemer commented 1 year ago

Looking at this PR makes me think we need to change the way SmithyProject is being used inside SmithyTextDocumentService. I feel like the NPEs are coming from the fact that createProject won't set the project property if the project fails to load, so anywhere else we use project will just get an NPE, and then we have try-catch everywhere. We should think about what functionality is offered if the SmithyProject fails to load, and maybe split that up somehow so we have more deterministic behavior.

kubukoz commented 1 year ago

I agree in principle, but that's a bigger change and I wanted to provide users with a little bit more context for troubleshooting in the meantime.

Plus, the try-catch captures more than the NPEs - any kind of IOOBEs, for example. I'm not sure we can get rid of these blocks that easily. It would still be nice to avoid the NPEs, though.