Merge pull request #856 from alexheretic/pre-initialize-request

nrc · web-flow · commit dab46fa70362 · 2018-05-09T13:29:10.000+12:00
Handle pre-initialize requests
diff --git a/src/server/dispatch.rs b/src/server/dispatch.rs
@@ -11,13 +11,13 @@
 use super::requests::*;
 use actions::work_pool;
 use actions::work_pool::WorkDescription;
-use jsonrpc_core as jsonrpc;
+use actions::InitActionContext;
 use jsonrpc_core::types::ErrorCode;
+use lsp_data::LSPRequest;
 use server;
-use server::{Request, Response};
 use server::io::Output;
-use actions::InitActionContext;
-use lsp_data::LSPRequest;
+use server::message::ResponseError;
+use server::{Request, Response};
 use std::sync::mpsc;
 use std::thread;
 use std::time::Duration;
@@ -183,18 +183,3 @@ pub trait RequestAction: LSPRequest {
         params: Self::Params,
     ) -> Result<Self::Response, ResponseError>;
 }
-
-/// Wrapper for a response error
-#[derive(Debug)]
-pub enum ResponseError {
-    /// Error with no special response to the client
-    Empty,
-    /// Error with a response to the client
-    Message(jsonrpc::ErrorCode, String),
-}
-
-impl From<()> for ResponseError {
-    fn from(_: ()) -> Self {
-        ResponseError::Empty
-    }
-}
diff --git a/src/server/message.rs b/src/server/message.rs
@@ -49,6 +49,21 @@ impl<R: ::serde::Serialize + fmt::Debug> Response for R {
     }
 }
 
+/// Wrapper for a response error
+#[derive(Debug)]
+pub enum ResponseError {
+    /// Error with no special response to the client
+    Empty,
+    /// Error with a response to the client
+    Message(jsonrpc::ErrorCode, String),
+}
+
+impl From<()> for ResponseError {
+    fn from(_: ()) -> Self {
+        ResponseError::Empty
+    }
+}
+
 /// An action taken in response to some notification from the client.
 /// Blocks stdin whilst being handled.
 pub trait BlockingNotificationAction: LSPNotification {
@@ -60,13 +75,12 @@ pub trait BlockingNotificationAction: LSPNotification {
 pub trait BlockingRequestAction: LSPRequest {
     type Response: Response + fmt::Debug;
 
-    /// Handle request and send its response back along the given output.
+    /// Handle request and return its response. Output is also provided for additional messaging.
     fn handle<O: Output>(
-        id: usize,
         params: Self::Params,
         ctx: &mut ActionContext,
         out: O,
-    ) -> Result<Self::Response, ()>;
+    ) -> Result<Self::Response, ResponseError>;
 }
 
 /// A request that gets JSON serialized in the language server protocol.
@@ -166,10 +180,8 @@ impl<A: BlockingRequestAction> Request<A> {
         self,
         ctx: &mut ActionContext,
         out: &O,
-    ) -> Result<A::Response, ()> {
-        let result = A::handle(self.id, self.params, ctx, out.clone())?;
-        result.send(self.id, out);
-        Ok(result)
+    ) -> Result<A::Response, ResponseError> {
+        A::handle(self.params, ctx, out.clone())
     }
 }
 
diff --git a/src/server/mod.rs b/src/server/mod.rs
@@ -15,7 +15,7 @@
 use actions::{notifications, requests, ActionContext};
 use analysis::AnalysisHost;
 use config::Config;
-use jsonrpc_core::{self as jsonrpc, Id};
+use jsonrpc_core::{self as jsonrpc, Id, types::error::ErrorCode};
 pub use ls_types::notification::Exit as ExitNotification;
 pub use ls_types::request::Initialize as InitializeRequest;
 pub use ls_types::request::Shutdown as ShutdownRequest;
@@ -27,13 +27,13 @@ use lsp_data;
 use lsp_data::{InitializationOptions, LSPNotification, LSPRequest};
 use serde_json;
 use server::dispatch::Dispatcher;
-pub use server::dispatch::{RequestAction, ResponseError, DEFAULT_REQUEST_TIMEOUT};
+pub use server::dispatch::{RequestAction, DEFAULT_REQUEST_TIMEOUT};
 pub use server::io::{MessageReader, Output};
 use server::io::{StdioMsgReader, StdioOutput};
 use server::message::RawMessage;
 pub use server::message::{
     Ack, BlockingNotificationAction, BlockingRequestAction, NoResponse, Notification, Request,
-    Response,
+    Response, ResponseError
 };
 use std::path::PathBuf;
 use std::sync::atomic::Ordering;
@@ -45,6 +45,8 @@ mod dispatch;
 mod io;
 mod message;
 
+const NOT_INITIALIZED_CODE: ErrorCode = ErrorCode::ServerError(-32002);
+
 /// Run the Rust Language Server.
 pub fn run_server(analysis: Arc<AnalysisHost>, vfs: Arc<Vfs>) {
     debug!("Language Server starting up. Version: {}", version());
@@ -63,18 +65,21 @@ impl BlockingRequestAction for ShutdownRequest {
     type Response = Ack;
 
     fn handle<O: Output>(
-        _id: usize,
         _params: Self::Params,
         ctx: &mut ActionContext,
         _out: O,
-    ) -> Result<Self::Response, ()> {
-        // Currently we don't perform an explicit cleanup, other than storing state
-        ctx.inited()
-            .expect("initialized context: todo -32002 response")
-            .shut_down
-            .store(true, Ordering::SeqCst);
-
-        Ok(Ack)
+    ) -> Result<Self::Response, ResponseError> {
+        if let Ok(ctx) = ctx.inited() {
+            // Currently we don't perform an explicit cleanup, other than storing state
+            ctx.shut_down.store(true, Ordering::SeqCst);
+            Ok(Ack)
+        }
+        else {
+            Err(ResponseError::Message(
+                NOT_INITIALIZED_CODE,
+                "not yet received `initialize` request".to_owned(),
+            ))
+        }
     }
 }
 
@@ -87,14 +92,13 @@ fn handle_exit_notification(ctx: &mut ActionContext) -> ! {
 }
 
 impl BlockingRequestAction for InitializeRequest {
-    type Response = NoResponse;
+    type Response = InitializeResult;
 
     fn handle<O: Output>(
-        id: usize,
         params: Self::Params,
         ctx: &mut ActionContext,
         out: O,
-    ) -> Result<NoResponse, ()> {
+    ) -> Result<InitializeResult, ResponseError> {
         let init_options: InitializationOptions = params
             .initialization_options
             .as_ref()
@@ -106,13 +110,16 @@ impl BlockingRequestAction for InitializeRequest {
         let result = InitializeResult {
             capabilities: server_caps(),
         };
-        out.success(id, &result);
 
         let capabilities = lsp_data::ClientCapabilities::new(&params);
-        ctx.init(get_root_path(&params), &init_options, capabilities, &out)
-            .expect("context already initialized");
-
-        Ok(NoResponse)
+        match ctx.init(get_root_path(&params), &init_options, capabilities, &out) {
+            Ok(_) => Ok(result),
+            Err(_) => Err(ResponseError::Message(
+                // No code in the spec, just use some number
+                ErrorCode::ServerError(123),
+                "Already received an initialize request".to_owned(),
+            )),
+        }
     }
 }
 
@@ -169,7 +176,7 @@ impl<O: Output> LsService<O> {
                         }
                         else {
                             warn!(
-                                "Server has not yet received an `initialize` request, ignoring {}", <$n_action as LSPNotification>::METHOD,
+                                "Server has not yet received an `initialize` request, ignoring {}", $method,
                             );
                         }
                     }
@@ -182,20 +189,41 @@ impl<O: Output> LsService<O> {
                         // block until all nonblocking requests have been handled ensuring ordering
                         self.dispatcher.await_all_dispatched();
 
-                        if request.blocking_dispatch(&mut self.ctx, &self.output).is_err() {
-                            debug!("Error handling request: {:?}", msg);
+                        let req_id = request.id;
+                        match request.blocking_dispatch(&mut self.ctx, &self.output) {
+                            Ok(res) => res.send(req_id, &self.output),
+                            Err(ResponseError::Empty) => {
+                                debug!("error handling {}", $method);
+                                self.output.failure_message(
+                                    req_id,
+                                    ErrorCode::InternalError,
+                                    "An unknown error occurred"
+                                )
+                            }
+                            Err(ResponseError::Message(code, msg)) => {
+                                debug!("error handling {}: {}", $method, msg);
+                                self.output.failure_message(req_id, code, msg)
+                            }
                         }
                     }
                 )*
 
                 $(
                     <$request as LSPRequest>::METHOD => {
                         let request: Request<$request> = msg.parse_as_request()?;
-                        let request = (
-                            request,
-                            self.ctx.inited().expect("initialized context: todo -32002 response"),
-                        );
-                        self.dispatcher.dispatch(request);
+                        if let Ok(ctx) = self.ctx.inited() {
+                            self.dispatcher.dispatch((request, ctx));
+                        }
+                        else {
+                            warn!(
+                                "Server has not yet received an `initialize` request, cannot handle {}", $method,
+                            );
+                            self.output.failure_message(
+                                request.id,
+                                NOT_INITIALIZED_CODE,
+                                "not yet received `initialize` request".to_owned(),
+                            );
+                        }
                     }
                 )*
                     // exit notification can uniquely handle pre `initialize` request state
diff --git a/src/test/mod.rs b/src/test/mod.rs
@@ -1569,7 +1569,8 @@ fn test_all_targets() {
     );
 }
 
-/// Test hover continues to work after the source has moved line
+/// Handle receiving a notification before the `initialize` request by ignoring and
+/// continuing to run
 #[test]
 fn ignore_uninitialized_notification() {
     let mut env = Environment::new("common");
@@ -1626,3 +1627,64 @@ fn ignore_uninitialized_notification() {
         ],
     );
 }
+
+/// Handle receiving requests before the `initialize` request by returning an error response
+/// and continuing to run
+#[test]
+fn fail_uninitialized_request() {
+    let mut env = Environment::new("common");
+
+    let source_file_path = Path::new("src").join("main.rs");
+    let root_path = env.cache.abs_path(Path::new("."));
+    let url = Url::from_file_path(env.cache.abs_path(&source_file_path))
+        .expect("couldn't convert file path to URL");
+
+    let messages = vec![
+        request::<requests::Definition>(
+            0,
+            TextDocumentPositionParams {
+                text_document: TextDocumentIdentifier::new(url),
+                position: env.cache
+                    .mk_ls_position(src(&source_file_path, 22, "world")),
+            },
+        ).to_string(),
+        initialize(1, root_path.as_os_str().to_str().map(|x| x.to_owned())).to_string(),
+    ];
+
+    let (mut server, results) = env.mock_server(messages);
+
+    // Return error response to pre `initialize` request, keep running.
+    assert_eq!(
+        ls_server::LsService::handle_message(&mut server),
+        ls_server::ServerStateChange::Continue
+    );
+    {
+        wait_for_n_results!(1, results);
+        let response = json::parse(&results.lock().unwrap().remove(0)).unwrap();
+        assert_eq!(response["id"], 0);
+        assert_eq!(response["error"]["code"], -32002);
+        let message = response["error"]["message"].as_str().unwrap();
+        assert!(
+            message.to_lowercase().contains("initialize"),
+            "Unexpected error.message `{}`",
+            message,
+        );
+    }
+
+    // Initialize and build.
+    assert_eq!(
+        ls_server::LsService::handle_message(&mut server),
+        ls_server::ServerStateChange::Continue
+    );
+    expect_messages(
+        results.clone(),
+        &[
+            ExpectedMessage::new(Some(1)).expect_contains("capabilities"),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#"title":"Building""#),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains("completion"),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#""done":true"#),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#"title":"Indexing""#),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#""done":true"#),
+        ],
+    );
+}
