Opt-out of build after `initialize` by Xanewok · Pull Request #420 · rust-lang/rls

Xanewok · 2017-07-19T17:45:44Z

Fixes #419.

Introduces new InitializationOptions structure defining options that are supported via initialization_options in initialize request.
Allow for not building immediately after initialize, configurable via omit_init_build in InitializationOptions
Bump up build priority from Normal to Cargo when compilation context isn't cached yet (in case we omit first build, don't catch onDidChangeConfiguration and start a Normal build - we always need Cargo as a first to resolve and build deps etc.)

Xanewok · 2017-07-19T17:49:47Z

src/server.rs


        let root_path = params.root_path.map(PathBuf::from).expect("No root path");
+        let init_options: Option<InitializationOptions> = params.initialization_options
+                              .and_then(|options| serde_json::from_value(options)


This could be just simplified to serde_json::from_value(options).ok() and not introduce 4 more lines for a debug! but I wanted to add a message in case deserialization fails for the options. Is that okay?

Is deserialisation more likely to fail here than elsewhere? I would prefer the simpler version I think - this is kinda hard to read.

@nrc this is somewhat more user-facing and gives LSP clients freedom to pass any JSON config (which is reasonable, since the configs will wildly differ for different servers), hence why I wanted to have a debug or preferably a user warning later that passed config is invalid or unsupported by the current server. Should I simplify that for now?

I think it is fair to assume that clients will pass valid JSON. They might add options or have options missing and we should handle that gracefully, but if the send malformed JSON, then I think they are not sticking to the LSP and I hope that doesn't happen to often (and the extension developers should hit this before users do).

Xanewok · 2017-07-19T17:52:54Z

src/server.rs

+                                            }));
        self.handler.init(root_path);
-        self.handler.inited().init(self.output.clone());
+        self.handler.inited().init(init_options, self.output.clone());


Not passing init_options to .init() since it'd have to store some state to be useful for .inited().init() and the only supported option right now will be only used once on init, which means it isn't probably worth keeping around. Is that okay for now?

I'm not sure I understand, but the code looks fine to me.

Xanewok · 2017-07-19T17:54:02Z

src/actions/mod.rs

-    pub fn init<O: Output>(&self, out: O) {
-        self.build_current_project(BuildPriority::Cargo, out);
+    pub fn init<O: Output>(&self, init_options: Option<InitializationOptions>, out: O) {
+        let omit_init_build = init_options.and_then(|x| x.omit_init_build)


Should I move default values logic to an impl InitializationOptions?

Sort of. I would implement Default for InitializationOptions and take a InitializationOptions here, rather than an Option

Xanewok · 2017-07-19T18:47:04Z

x86_64-pc-windows-msvc failed with:

error: could not download file from 'https://static.rust-lang.org/dist/channel-rust-nightly.toml.sha256' to 'C:\Users\appveyor\.rustup\tmp\yfwm5cm7aj55oiqv_file'
info: caused by: error during download
info: caused by: [35] SSL connect error (schannel: next InitializeSecurityContext failed: Unknown error (0x80092013) - The revocation function was unable to check revocation because the revocation server was offline.)

Xanewok · 2017-07-26T22:18:53Z

This version has a simplified default InitializationOptions, implemented in 4fdd0b1. When fields are missing, they're filled using Default trait and handling parsing error is simplified now.

nrc · 2017-07-26T23:34:14Z

src/test/mod.rs

+         Self::initialize_with_opts(id, root_path, None)
+    }
+
+    pub fn initialize_with_opts(id: usize, root_path: Option<String>, initialization_options: Option<InitializationOptions>) -> ServerMessage {


We only seem to call this function from a test, I'd expect to see a call when we receive the init message

@nrc not sure I understand. This creates only a client-specific message. Since server itself has no use for it, it's used only in tests and once for our CLI "client". This is where we parse the additional options when the server receives the initialize request via JSON-RPC and here we're passing it to init action handler.

oh of course, sorry, I thought this was still in server.rs, I didn't see it was in the tests

Xanewok changed the title ~~Init build~~ Opt-out of build after initialize Jul 19, 2017

Xanewok commented Jul 19, 2017

View reviewed changes

Xanewok force-pushed the init-build branch from 6a858b5 to 721d006 Compare July 25, 2017 23:51

Xanewok added 2 commits July 26, 2017 18:21

Support omitting build after initialize

c2b44b8

Use Cargo build priority if compilation args/envs are not fetched yet

39f258a

Xanewok force-pushed the init-build branch from 721d006 to db3322b Compare July 26, 2017 16:30

Xanewok added 2 commits July 27, 2017 00:02

Implement Default for InitializationOptions

4fdd0b1

Add omit_init_build test

d0de844

Xanewok force-pushed the init-build branch from db3322b to d0de844 Compare July 26, 2017 22:12

nrc reviewed Jul 26, 2017

View reviewed changes

nrc merged commit 5fb0afd into rust-lang:master Jul 27, 2017

Xanewok deleted the init-build branch July 27, 2017 00:19

Conversation

Xanewok commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xanewok commented Jul 19, 2017

Uh oh!

Xanewok commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Xanewok commented Jul 19, 2017 •

edited

Loading