Updated configuration loading during initialization

[jmhymas]

I removed the code that attempted to load a global ols.json. The value returned from that code path was never used, and I couldn't find a global ols.json in the repository or any mention of one in the documentation. I assumed this was an artifact of an older config loading approach, but I'm happy to restore it if that isn't the case.

I also updated the ols.json parsing logic. The two main changes were: (1) Separating file I/O from parsing so the config-loading helper only has a single failure path. (2) Returning an error when a user-provided ols.json cannot be parsed.

This means OLS now fails to initialize if the project contains an invalid ols.json. That seemed like an expected behavior to me, if the user explicitly provides configuration that can't be parsed, fail fast and loud so they're aware of the issue. However, if that isn't in line with the project's goals, I'm happy to change it.

I also looked for a way to report the parsing error without failing initialization. It appeared that send_error is the intended way to respond with LSP errors, but I only found it being called from the request dispatch function. I didn't want to just starting calling it from other places if it's not intended to be used that way.

[BradLewis]

Thanks! This causes nvim to report an error for me when I start the LSP

I think rather than returning an error when failing to parse the json, we should instead report a diagnostic using add_diagnostics, ideally reporting the error at the location that the json parsing failed. This will make the LSP start correctly but report a red squiggle at the offending place.

[jmhymas]

Sure, I can make that changes and update the PR. Thank you for letting me know the proper method to use :)

[jmhymas]

Hey @BradLewis, thanks for the reply. I made the change to use add_diagostic, but I don't think this is accomplishing what I set out to do. The original issue I was trying to resolve is OLS failing quietly when it gets malformed JSON from the user. The error you got was actually kind of the point. Without that error the user has absolutely no way to know OLS failed to load their ols.json file without digging through logs (unless there is something I'm unaware of). Since OLS loads default configs in the event of a failure, it's not obvious to the user anything is wrong.

There was a conversation on the Odin discord regarding this.

Edit: My understanding is that add_diagnostic sends a diagnostic that can be visible in the editor, but there is no where to display this diagnostic, since the error is coming from the ols.json at startup. Please correct me if I'm wrong.

Edit2: Sorry, learning as I go here. I see where I got turned around. I misunderstood the way error was being used in this context. I did some more digging and found send_notification, which showed me how window/logMessage gets sent. On my branch I added a hacky solution to call send_notification directly with a Notification containing window/showMessage, and that worked to display a message in nvim without throwing an error. But there doesn't currently seem to be any infrastructure to send window/showMessage in a way consistent with how the logger works. I'd be happy to look into adding that, but I'd probably need some guidance.

[BradLewis]

No worries! The main problem with the first approach is this is more of a 'we failed to parse the lsp request' rather than 'we parsed the request but couldn't parse something beside it' I think there are a couple of different avenues we can pursue for this.

Edit: My understanding is that add_diagnostic sends a diagnostic that can be visible in the editor, but there is no where to display this diagnostic, since the error is coming from the ols.json at startup. Please correct me if I'm wrong.

In this case the error can just be pointing to the ols.json file itself that it's failing to parse. This would be a nice way to show the user "hey, we failed to parse the ols.json file at ". Ideally we'd be able to point to where it failed in the json file but I don't think the odin json parser gives us that information at the moment.

More inline with what you've been trying to accomplish is windows/showMessage https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showMessage . I don't think we use this currently but it would work very similarly to how push_diagnostics() calls send_notification. This would just be a new type of notification. I think if we do this we should still do the diagnostic I mentioned above as well.

Let me know if you have any questions about this, happy to help!

[Carlyle-Foster]

I removed the code that attempted to load a global ols.json. The value returned from that code path was never used, and I couldn't find a global ols.json in the repository or any mention of one in the documentation. I assumed this was an artifact of an older config loading approach, but I'm happy to restore it if that isn't the case.

the value returned by read_ols_config(global_ols_config_path, config, uri) is an error code, the global config is loaded as a side-effect. read_ols_config(ols_config_path, config, uri) doesn't even assign the error code at all, both of those should probably be fixed

the global ols.json is used, the vscode extension prompts you to create an ols.json when it can't find one and if you accept it asks whether it should be project-scoped or global. there's also a command to edit the global config from the vscode command palette (and create it lazily if necessary, to mirror how the per-project one works). maybe there could be more documentation of it though?

thanks for bringing this up btw, it made me realize there's a subtle bug where the vscode extension doesn't try to load the global config if it finds the per-project config, whereas the server overlays the two configs with the per-project one overriding the global one, i should really fix that

[Carlyle-Foster]

thanks for bringing this up btw, it made me realize there's a subtle bug where the vscode extension doesn't try to load the global config if it finds the per-project config, whereas the server overlays the two configs with the per-project one overriding the global one, i should really fix that

there's a somewhat less subtle bug where the extension is hardcoded to read it's configuration values from the per-project ols.json, it managed to escape notice because the extension itself only reads 2 fields and the rest are for the server (the only things the extension wants to know are "what is the custom odin command, if any" and "what collections should i know about")