Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tentative implementation of initializationOptions at RLS startup. #183

Conversation

norru
Copy link
Contributor

@norru norru commented Dec 1, 2018

Tentative implementation of initializationOptions at RLS startup.

(squash + signoff of #182)

  • adds the Rls config settings to the Rust paths, which allows to specify a path containing the initialization settings

image

  • if file at the path above exists, the plugin attempts to load and parse it as as a Json object, then forwards its whole content to the initialize RLS method as the payload of the initializationOptions field:
/home/nico/.cargo/rls.conf
{
	"settings": {
		"rust": {
			"goto_def_racer_fallback": true,
			"clippy_preference": "on"
		}
	}
}

Here how the above looks in the LSP4E log:

LSP4E to org.eclipse.corrosion.rls:{"jsonrpc":"2.0","id":"1","method":"initialize","params":{"processId":29689,"rootPath":"/home/nico/Workspaces/runtime-EclipseApplication(1)/new_rust_project/","rootUri":"file:///home/nico/Workspaces/runtime-EclipseApplication(1)/new_rust_project/","initializationOptions":{"settings":{"rust":{"goto_def_racer_fallback":true,"clippy_preference":"on"}}},"capabilities":{"workspace":{"applyEdit":true,"symbol":{},"executeCommand":{"dynamicRegistration":true},"workspaceFolders":true},"textDocument":{"synchronization":{"willSave":true,"willSaveWaitUntil":true,"didSave":true},"completion":{"completionItem":{"snippetSupport":true}},"hover":{},"signatureHelp":{},"references":{},"documentHighlight":{},"documentSymbol":{},"formatting":{},"rangeFormatting":{},"definition":{},"codeAction":{"codeActionLiteralSupport":{}},"codeLens":{},"documentLink":{},"colorProvider":{},"rename":{}}},"clientName":"Eclipse SDK","trace":"off"}}

Caveats

  • missing unit tests (I have no idea where/how to build one for this integration point)
  • changes to the settings file require a restart of RLS (which mostly means the entire IDE)
  • RLS as for 2018-12-01 does not seem to be parsing the initialization settings, probably at least until Support InitializationOptions.settings rust-lang/rls#1150 filters through to nightly
  • the initializationOptions.settings in RLS doesn't appear to be documented @nrc @mickaelistria however the example above should be valid syntax (according to @alexheretic).

Signed-off-by: Nicola Orru [email protected]

    Added signed-off-by and Contributor lines
    MVP for initializationOptions
    Added rlsConfiguration basic

Signed-off-by: Nicola Orru <[email protected]>
@norru
Copy link
Contributor Author

norru commented Dec 1, 2018

@mickaelistria I'd be happy to have rls.toml supported as well (with some sort of mime-type autodetection of json and toml). I didn't try that because there doesn't seem to be a Toml parser integrated in the plugin.

My suggestion is to go with .conf/.json first because of Gson being already integrated, and then apply another patch to implement other formats (.toml will definitely help for multiple IDE users).

@mickaelistria
Copy link
Contributor

Thanks @norru , that looks good! I suggest we wait for RLS to be ready for testing this before we merge. It would also be great if we could have Corrosion providing a best default rls.conf file (best default are of high importance); and it would be also nice to have an "Edit" button beside the RLS config entry (lower importance).
Note that maybe if we provide good defaults, it's not necessary to give the option. What do you think?

@norru
Copy link
Contributor Author

norru commented Dec 3, 2018

Hi @mickaelistria - Having good defaults would be great, not being able to change them not so much. If we could to stick to the RLS "defaults are best" we wouldn't have the problem in first place because RLS would provide those out of the box. Even if they did, I am pretty sure that their "best defaults" wouldn't always match my "best defaults".

Said that, if you have an idea of the "best defaults", we could build-in a "default fallback" as a jar resource which would kick off when the external file is not found (or even a small hardcoded Hashmap).

Also +1 for waiting until RLS ships with this feature so we can tweak.

Not sure how to implement the "Edit" function - is it just "open the file in the editor" (Can you do that from the Preferences dialog?) or you're thinking about a custom view (similar to the "Environment variables" one)? However, if there is "a way" to edit the configuration, that'll be good enough a baseline for what I am concerned.

@mickaelistria
Copy link
Contributor

Said that, if you have an idea of the "best defaults", we could build-in a "default fallback" as a jar resource which would kick off when the external file is not found (or even a small hardcoded Hashmap).

Yes, we should do something like that. Please add your "best defaults" in a form or another (JSON file as a Java resource in the plugin, or a HashMap, whatever), you'll be our reference here at the moment.

Not sure how to implement the "Edit" function - is it just "open the file in the editor" (Can you do that from the Preferences dialog?) or you're thinking about a custom view (similar to the "Environment variables" one)? However, if there is "a way" to edit the configuration, that'll be good enough a baseline for what I am concerned.

Let's not both about the edit at the moment. We can have a look at it later when we have more concrete use-case showing how it's necessary.

@norru
Copy link
Contributor Author

norru commented Dec 3, 2018

Latest RLS (2018-12-01) breaks LSP4E:

java.util.concurrent.ExecutionException: org.eclipse.lsp4j.jsonrpc.ResponseErrorException: missing field `codeActionKind`
	at java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:357)
	at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1915)
	at org.eclipse.lsp4e.LanguageServerWrapper.getServerCapabilities(LanguageServerWrapper.java:573)
	at org.eclipse.lsp4e.LanguageServiceAccessor.getLSWrappers(LanguageServiceAccessor.java:301)
	at org.eclipse.lsp4e.LanguageServiceAccessor.getLSPDocumentInfosFor(LanguageServiceAccessor.java:444)
	at org.eclipse.lsp4e.ConnectDocumentToLanguageServerSetupParticipant$1.run(ConnectDocumentToLanguageServerSetupParticipant.java:75)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
Caused by: org.eclipse.lsp4j.jsonrpc.ResponseErrorException: missing field `codeActionKind`
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleResponse(RemoteEndpoint.java:209)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.consume(RemoteEndpoint.java:193)
	at org.eclipse.lsp4e.LanguageServerWrapper.lambda$1(LanguageServerWrapper.java:219)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.handleMessage(StreamMessageProducer.java:192)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:94)
	at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:99)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

@norru
Copy link
Contributor Author

norru commented Dec 3, 2018

Thanks @mickaelistria for fixing the bug above - I have now latest RLS running against this patch and it does appear that it's parsing the custom configuration correctly

@norru
Copy link
Contributor Author

norru commented Dec 3, 2018

I have added a simple default initialization option which allows clippy and Racer navigation. Defaults are loaded if the custom file doesn't exist or is malformed.

rust-lang/rls#1152

@norru
Copy link
Contributor Author

norru commented Dec 3, 2018

@mickaelistria Once you get this I suggest you bump the version number up to 0.4.0 (interface change)

@norru norru mentioned this pull request Dec 3, 2018
@mickaelistria
Copy link
Contributor

0.3.0 was not released yet (it's a snapshot) and Corrosion doesn't publish any of its code as API, so we can stick to 0.3.0.

@mickaelistria mickaelistria merged commit 251108b into eclipse-corrosion:master Dec 3, 2018
@mickaelistria
Copy link
Contributor

This patch looks perfect to me, thanks a lot @norru for this great first code contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants