From a06d06952b52356c98880ebe1636449551c2e8f5 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 30 Apr 2025 15:24:59 +0200 Subject: [PATCH 1/7] Handle parsing of responses correctly Signed-off-by: Jonatan Waern --- src/server/message.rs | 178 ++++++++++++++++++++++++++++++++++++++---- src/server/mod.rs | 18 ++--- 2 files changed, 170 insertions(+), 26 deletions(-) diff --git a/src/server/message.rs b/src/server/message.rs index fa7a4de..ee730ac 100644 --- a/src/server/message.rs +++ b/src/server/message.rs @@ -357,24 +357,15 @@ impl RawMessage { Ok(Notification { params, _action: PhantomData }) } - pub(crate) fn try_parse(msg: &str) - -> Result, jsonrpc::Error> { - // Parse the message. - let ls_command: serde_json::Value = - serde_json::from_str(msg) - .map_err(|e| rpc_error(ParseError, e))?; - + pub(crate) fn try_parse(ls_command: serde_json::Value) + -> Result { // Per JSON-RPC/LSP spec, Requests must have ID, whereas Notifications cannot. let id = ls_command .get("id") .map_or(Value::Null, |id| serde_json::from_value(id.to_owned()).unwrap()); - let method = match ls_command.get("method") { - Some(method) => method, - // No method means this is a response to one of our requests. - // FIXME: we should confirm these, but currently just ignore them. - None => return Ok(None), - }; + // Guaranteed by caller + let method = ls_command.get("method").unwrap(); let method = method.as_str().ok_or_else(|| rpc_error(InvalidRequest, "Method is not a string"))? @@ -394,7 +385,7 @@ impl RawMessage { format!("Unsupported parameter type: {v}"))), }; - Ok(Some(RawMessage { method, id, params })) + Ok(RawMessage { method, id, params }) } } @@ -420,6 +411,99 @@ impl Serialize for RawMessage { } } +#[derive(Debug, PartialEq, Deserialize)] +pub struct ErrorResponse { + code: i32, + message: String, + data: Option, +} + +#[derive(Debug, PartialEq)] +pub enum ResultOrError { + Result(Value), + Error(ErrorResponse), +} + +#[derive(Debug, PartialEq)] +pub struct RawResponse { + pub id: Value, + pub result: ResultOrError, +} + +impl RawResponse { + pub(crate) fn try_parse(mut ls_command: serde_json::Value) + -> Result { + // Get the ID (required in responses) + let id = ls_command.get("id") + .ok_or_else(||rpc_error(InvalidRequest, "No ID in response"))? + .to_owned(); + + let result = ls_command.get_mut("result").map( + |r|r.take()); + let error = ls_command.get_mut("error").map( + |val|ErrorResponse::deserialize(val.take())); + let resultorerror = match (result, error) { + (Some(_), Some(_)) => + return Err(rpc_error(InvalidRequest, + "Both 'result' and 'error' \ + specified in response")), + (None, None) => + return Err(rpc_error(InvalidRequest, + "Neither 'result' and 'error' \ + specified in response")), + (Some(result), None) => + ResultOrError::Result(result), + (None, Some(error)) => + ResultOrError::Error(error.map_err( + |e|rpc_error(ParseError, e))?), + }; + + Ok(RawResponse { id, result: resultorerror }) + } +} + +pub enum RawMessageOrResponse { + Message(RawMessage), + Response(RawResponse), +} + +impl RawMessageOrResponse { + pub(crate) fn try_parse(msg: &str) -> + Result { + let ls_command: serde_json::Value = + serde_json::from_str(msg) + .map_err(|e| rpc_error(ParseError, e))?; + if ls_command.get("method").is_some() { + RawMessage::try_parse(ls_command) + .map(|raw|RawMessageOrResponse::Message(raw)) + } else { + RawResponse::try_parse(ls_command) + .map(|raw|RawMessageOrResponse::Response(raw)) + } + } + + pub fn is_message(&self) -> bool { + matches!(self, RawMessageOrResponse::Message(_)) + } + + pub fn is_response(&self) -> bool { + matches!(self, RawMessageOrResponse::Response(_)) + } + + pub(crate) fn as_message(self) -> Option { + match self { + RawMessageOrResponse::Message(mess) => Some(mess), + _ => None, + } + } + pub(crate) fn as_response(self) -> Option { + match self { + RawMessageOrResponse::Response(resp) => Some(resp), + _ => None, + } + } +} + #[cfg(test)] mod test { use super::*; @@ -459,7 +543,10 @@ mod test { // Missing parameters are represented internally as Null. params: serde_json::Value::Null, }; - assert_eq!(expected_msg, RawMessage::try_parse(&raw_json).unwrap().unwrap()); + + let parsed = RawMessageOrResponse::try_parse(&raw_json) + .unwrap().as_message().unwrap(); + assert_eq!(expected_msg, parsed); } #[test] @@ -477,7 +564,9 @@ mod test { // Missing parameters are represented internally as Null. params: serde_json::Value::Null, }; - assert_eq!(expected_msg, RawMessage::try_parse(&raw_json).unwrap().unwrap()); + let parsed = RawMessageOrResponse::try_parse(&raw_json) + .unwrap().as_message().unwrap(); + assert_eq!(expected_msg, parsed); } #[test] @@ -546,7 +635,62 @@ mod test { #[test] fn deserialize_message_empty_params() { let msg = r#"{"jsonrpc":"2.0","method":"initialized","params":{}}"#; - let parsed = RawMessage::try_parse(msg).unwrap().unwrap(); + let parsed = RawMessageOrResponse::try_parse(msg) + .unwrap().as_message().unwrap(); parsed.parse_as_notification::().unwrap(); } + + #[test] + fn simple_request_response() { + let raw_json = json!({ + "jsonrpc": "2.0", + "id": "abc", + "result": { + "this" : 0, + "really" : "could", + "be" : ["anything"] + } + }) + .to_string(); + + let expected_resp = RawResponse { + id: Value::from("abc"), + result: ResultOrError::Result(json!({ + "this" : 0, + "really" : "could", + "be" : ["anything"] + })) + }; + + let parsed = RawMessageOrResponse::try_parse(&raw_json) + .unwrap().as_response().unwrap(); + assert_eq!(expected_resp, parsed); + } + #[test] + fn simple_request_error() { + let raw_json = json!({ + "jsonrpc": "2.0", + "id": "abc", + "error": { + "code" : 0, + "message" : "oh no!", + } + }) + .to_string(); + + let expected_resp = RawResponse { + id: Value::from("abc"), + result: ResultOrError::Error( + ErrorResponse { + code: 0, + message: "oh no!".to_string(), + data: None, + } + ), + }; + + let parsed = RawMessageOrResponse::try_parse(&raw_json) + .unwrap().as_response().unwrap(); + assert_eq!(expected_resp, parsed); + } } diff --git a/src/server/mod.rs b/src/server/mod.rs index ce999ff..a57f90d 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -17,7 +17,7 @@ use crate::server::dispatch::Dispatcher; pub use crate::server::dispatch::{RequestAction, DEFAULT_REQUEST_TIMEOUT}; pub use crate::server::io::{MessageReader, Output}; use crate::server::io::{StdioMsgReader, StdioOutput}; -use crate::server::message::RawMessage; +use crate::server::message::{RawMessage, RawMessageOrResponse}; pub use crate::server::message::{ Ack, BlockingNotificationAction, BlockingRequestAction, NoResponse, Notification, Request, @@ -326,12 +326,15 @@ impl LsService { } }; trace!("Read a message `{}`", msg_string); - match RawMessage::try_parse(&msg_string) { - Ok(Some(rm)) => { + match RawMessageOrResponse::try_parse(&msg_string) { + Ok(RawMessageOrResponse::Message(rm)) => { debug!("Parsed a message: {}", rm.method); send.send(ServerToHandle::ClientMessage(rm)).ok(); }, - Ok(None) => (), + Ok(RawMessageOrResponse::Response(rr)) => { + // TODO + debug!("Parsed a response: {}", rr.id); + }, Err(e) => { error!("parsing error, {:?}", e); output.custom_failure( @@ -797,12 +800,9 @@ mod test { /// Some clients send empty object params for void params requests (see issue #1038). #[test] fn parse_shutdown_object_params() { - let raw = RawMessage::try_parse( + let raw = RawMessageOrResponse::try_parse( r#"{"jsonrpc": "2.0", "id": 2, "method": "shutdown", "params": {}}"#, - ) - .ok() - .and_then(|x| x) - .expect("raw parse failed"); + ).unwrap().as_message().unwrap(); let _request: Request = raw.parse_as_request().expect("Boring validation is happening"); From 562d13b02e5fa92821103d4ff842fc2b6f3bb07e Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 30 Apr 2025 15:58:37 +0200 Subject: [PATCH 2/7] Support the sending of requests that require responses and implement this for workspace/configuration Signed-off-by: Jonatan Waern --- clients.md | 15 +++- src/actions/hover.rs | 6 +- src/actions/mod.rs | 106 ++++++++++++++++--------- src/actions/notifications.rs | 58 ++++++++++---- src/actions/requests.rs | 147 +++++++++++++++++++++++++---------- src/cmd.rs | 12 +-- src/config.rs | 48 ++++++++---- src/lsp_data.rs | 28 +++++-- src/server/dispatch.rs | 59 +++++++++++--- src/server/message.rs | 93 ++++++++++++---------- src/server/mod.rs | 28 ++++--- 11 files changed, 412 insertions(+), 188 deletions(-) diff --git a/clients.md b/clients.md index f8ca5cd..e845597 100644 --- a/clients.md +++ b/clients.md @@ -30,7 +30,11 @@ Once you have this basic support in place, the hard work begins: * Implement [extensions to the protocol](clients.md#extensions-to-the-language-server-protocol) * Client-side configuration. - You'll need to send the `workspace/didChangeConfiguration` notification when - configuration changes. + configuration changes. You can either send the entire configuration in the + notification, or send "null" if your client supports pull-style configuration updates + (note that you need to support dynamic registration of + "didChangeConfiguration" and support the "workspace/configuration" request on the client + for pull-style updates) - For the config options, see [config.rs](./src/config.rs#L99-L111) * Check for and install the DLS - Download the latest [binary](https://github.com/intel/dml-language-server/actions/workflows/rust.yml). @@ -90,6 +94,7 @@ From Server to client: * `client/registerCapability` * `client/unregisterCapability` * `textDocument/publishDiagnostics` +* `workspace/configuration` (if using pull-style configuration updates) ### Extensions to the Language Server Protocol @@ -165,6 +170,14 @@ From Server to Client, these notifcations have a specified data value: with the title "Analysing" when it starts on any long-term work, and a 'WorkDoneProgressEnd' with the corresponding id when it finishes all such work. +* `workspace/configuration` + The server sends a single ConfigurationItem with no scope and a + "simics-modeling.dls" section. The client response value should be the + an array containing one element which is an object with the full + configuration structure for the server. +* `client/registerCapability` + The server may try to register on 'DidChangeWatchedFiles' or 'DidChangeConfiguration', + the latter being required for pull-style configuration updates. ## Resources diff --git a/src/actions/hover.rs b/src/actions/hover.rs index f1a27fc..7fddb36 100644 --- a/src/actions/hover.rs +++ b/src/actions/hover.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; use crate::actions::InitActionContext; use crate::lsp_data::*; -use crate::server::ResponseError; +use crate::server::{Output, ResponseError}; #[derive(Debug, Deserialize, Serialize, PartialEq, Eq)] pub struct Tooltip { @@ -16,8 +16,8 @@ pub struct Tooltip { } // Build a hover tooltip -pub fn tooltip( - ctx: &mut InitActionContext, +pub fn tooltip( + ctx: &mut InitActionContext, params: &TextDocumentPositionParams, ) -> Result { let hover_file_path = parse_file_path!(¶ms.text_document.uri, "hover")?; diff --git a/src/actions/mod.rs b/src/actions/mod.rs index f63aed4..6219c2a 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -3,7 +3,7 @@ //! Actions that the DLS can perform: responding to requests, watching files, //! etc. -use log::{debug, trace, error, warn}; +use log::{debug, info, trace, error, warn}; use thiserror::Error; use crossbeam::channel; use serde::Deserialize; @@ -31,7 +31,10 @@ use crate::file_management::{PathResolver, CanonPath}; use crate::lint::{LintCfg, maybe_parse_lint_cfg}; use crate::lsp_data; use crate::lsp_data::*; -use crate::server::{Output, ServerToHandle, error_message}; +use crate::server::{Output, ServerToHandle, error_message, + Request, RequestId, SentRequest}; +use crate::server::message::RawResponse; +use crate::server::dispatch::HandleResponseType; use crate::Span; use crate::span; use crate::span::{ZeroIndexed, FilePosition}; @@ -64,9 +67,9 @@ pub mod progress; pub mod work_pool; /// Persistent context shared across all requests and notifications. -pub enum ActionContext { +pub enum ActionContext { /// Context after server initialization. - Init(InitActionContext), + Init(InitActionContext), /// Context before initialization. Uninit(UninitActionContext), } @@ -81,20 +84,20 @@ impl From<()> for InitError { } } -impl ActionContext { +impl ActionContext { /// Construct a new, uninitialized context. pub fn new( vfs: Arc, config: Arc>, notify: channel::Sender, - ) -> ActionContext { + ) -> ActionContext { ActionContext::Uninit(UninitActionContext::new( Arc::new(Mutex::new(AnalysisStorage::init(notify))), vfs, config)) } /// Initialize this context, returns `Err(())` if it has already been initialized. - pub fn init( + pub fn init( &mut self, init_options: InitializationOptions, client_capabilities: lsp_data::ClientCapabilities, @@ -130,7 +133,7 @@ impl ActionContext { /// Returns an initialiased wrapped context, /// or `Err(())` if not initialised. - pub fn inited(&self) -> Result { + pub fn inited(&self) -> Result, InitError> { match *self { ActionContext::Uninit(_) => Err(().into()), ActionContext::Init(ref ctx) => Ok(ctx.clone()), @@ -218,7 +221,7 @@ pub struct AnalysisStateWaitDefinition { // (not concurrent), so make sure shared info is behind Arcs, and that no overly // large data structures are stored. #[derive(Clone)] -pub struct InitActionContext { +pub struct InitActionContext { pub analysis: Arc>, vfs: Arc, // Queues analysis jobs so that we don't over-use the CPU. @@ -246,6 +249,9 @@ pub struct InitActionContext { active_waits: Arc>>, + outstanding_requests: Arc>>>>, + pub config: Arc>, pub lint_config: Arc>, pub sent_warnings: Arc>>, @@ -301,7 +307,7 @@ impl ContextDefinition { pub type ActiveDeviceContexts = HashSet; -impl InitActionContext { +impl InitActionContext { fn new( analysis: Arc>, vfs: Arc, @@ -309,7 +315,7 @@ impl InitActionContext { client_capabilities: lsp_data::ClientCapabilities, pid: u32, _client_supports_cmd_run: bool, - ) -> InitActionContext { + ) -> InitActionContext { let lint_config = Arc::new(Mutex::new( config.lock().unwrap().lint_cfg_path.clone() .and_then(maybe_parse_lint_cfg) @@ -329,6 +335,7 @@ impl InitActionContext { has_notified_missing_builtins: false, //client_supports_cmd_run, active_waits: Arc::default(), + outstanding_requests: Arc::default(), shut_down: Arc::new(AtomicBool::new(false)), pid, workspace_roots: Arc::default(), @@ -352,9 +359,9 @@ impl InitActionContext { } } - fn init(&mut self, - _init_options: InitializationOptions, - out: O) { + fn init(&mut self, + _init_options: InitializationOptions, + out: O) { self.update_compilation_info(&out) } @@ -368,7 +375,7 @@ impl InitActionContext { } } - fn update_linter_config(&mut self, _out: &O) { + fn update_linter_config(&mut self, _out: &O) { trace!("Updating linter config"); if let Ok(config) = self.config.lock() { *self.lint_config.lock().unwrap() = @@ -378,7 +385,7 @@ impl InitActionContext { } } - pub fn update_compilation_info(&mut self, out: &O) { + pub fn update_compilation_info(&mut self, out: &O) { trace!("Updating compile info"); if let Ok(config) = self.config.lock() { if let Some(compile_info) = &config.compile_info_path { @@ -424,7 +431,7 @@ impl InitActionContext { } } - pub fn report_errors(&mut self, output: &O) { + pub fn report_errors(&mut self, output: &O) { self.update_analysis(); let filter = Some(self.device_active_contexts.lock().unwrap().clone()); let (isolated, device, lint) = @@ -528,9 +535,7 @@ impl InitActionContext { .update_analysis(&self.construct_resolver()); } - pub fn trigger_device_analysis(&mut self, - file: &Path, - out: &O) { + pub fn trigger_device_analysis(&mut self, file: &Path, out: &O) { let canon_path: CanonPath = file.to_path_buf().into(); debug!("triggering devices dependant on {}", canon_path.as_str()); self.update_analysis(); @@ -561,14 +566,14 @@ impl InitActionContext { // Called when config might have changed, re-update include paths // and similar - pub fn maybe_changed_config(&mut self, out: &O) { + pub fn maybe_changed_config(&mut self, out: &O) { trace!("Compilation info might have changed"); self.update_compilation_info(out); self.update_linter_config(out); } // Call before adding new analysis - pub fn maybe_start_progress(&mut self, out: &O) { + pub fn maybe_start_progress(&mut self, out: &O) { let mut notifier = self.current_notifier.lock().unwrap(); @@ -580,7 +585,7 @@ impl InitActionContext { new_notifier.notify_begin_progress(); } } - pub fn maybe_end_progress(&mut self, out: &O) { + pub fn maybe_end_progress(&mut self, out: &O) { if !self.analysis_queue.has_work() { // Need the scope here to succesfully drop the guard lock before // going into maybe_warn_missing_builtins below @@ -598,7 +603,7 @@ impl InitActionContext { } } - fn maybe_warn_missing_builtins(&mut self, out: &O) { + fn maybe_warn_missing_builtins(&mut self, out: &O) { if !self.has_notified_missing_builtins && !self.analysis.lock().unwrap().has_client_file( &PathBuf::from("dml-builtins.dml")) { @@ -628,10 +633,10 @@ impl InitActionContext { toret } - pub fn isolated_analyze(&mut self, - client_path: &Path, - context: Option, - out: &O) { + pub fn isolated_analyze(&mut self, + client_path: &Path, + context: Option, + out: &O) { debug!("Wants isolated analysis of {:?}{}", client_path, context.as_ref().map(|s|format!(" under context {}", s.as_str())) @@ -657,7 +662,7 @@ impl InitActionContext { &self.vfs, context, path, client_path.to_path_buf(), token); } - fn device_analyze(&mut self, device: &CanonPath, out: &O) { + fn device_analyze(&mut self, device: &CanonPath, out: &O) { debug!("Wants device analysis of {:?}", device); self.maybe_start_progress(out); self.maybe_add_device_context(device); @@ -839,9 +844,7 @@ impl InitActionContext { } } - pub fn maybe_trigger_lint_analysis(&mut self, - file: &Path, - out: &O) { + pub fn maybe_trigger_lint_analysis(&mut self, file: &Path, out: &O) { if !self.config.lock().unwrap().linting_enabled { return; } @@ -860,11 +863,11 @@ impl InitActionContext { out); } - fn lint_analyze(&mut self, - file: &Path, - context: Option, - cfg: LintCfg, - out: &O) { + fn lint_analyze(&mut self, + file: &Path, + context: Option, + cfg: LintCfg, + out: &O) { debug!("Wants to lint {:?}", file); self.maybe_start_progress(out); let path = if let Some(p) = @@ -1117,6 +1120,33 @@ impl InitActionContext { true }); } + + pub fn send_request(&self, params: R::Params, out: &O) + where + R: SentRequest, + ::Params: std::fmt::Debug + { + let id = out.provide_id(); + info!("Sending a request {} with params {:?} and id {}", + ::METHOD, params, id); + let request = Request::::new(id.clone(), params); + self.outstanding_requests.lock().unwrap().insert( + id, Box::new(::handle_response)); + out.request(request); + } + + pub fn handle_request_response(&mut self, resp: RawResponse, out: &O) { + info!("Got a response to some request: {:?}", resp); + let maybe_req_fn = { self.outstanding_requests + .lock().unwrap().remove(&resp.id) }; + if let Some(request_fn) = maybe_req_fn { + request_fn(self, resp, out); + } else { + info!("Got response for request id {:?} but it was not tracked\ + (either timed out or was never sent)", + resp.id); + } + } } /// Some notifications come with sequence numbers, we check that these are in @@ -1177,7 +1207,7 @@ pub struct FileWatch { impl FileWatch { /// Construct a new `FileWatch`. - pub fn new(ctx: &InitActionContext) -> Option { + pub fn new(ctx: &InitActionContext) -> Option { match ctx.config.lock() { Ok(config) => { config.compile_info_path.as_ref().map( diff --git a/src/actions/notifications.rs b/src/actions/notifications.rs index 01b4384..500fe42 100644 --- a/src/actions/notifications.rs +++ b/src/actions/notifications.rs @@ -5,7 +5,6 @@ use crate::actions::{FileWatch, InitActionContext, VersionOrdering, ContextDefinition}; use crate::file_management::CanonPath; -use crate::server::message::Request; use crate::span::{Span}; use crate::vfs::{Change, VfsSpan}; use crate::lsp_data::*; @@ -34,14 +33,30 @@ impl BlockingNotificationAction for Initialized { // Respond to the `initialized` notification. fn handle( _params: Self::Params, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O, ) -> Result<(), ResponseError> { - // TODO: register any dynamic capabilities + // These are the requirements for the pull-variant of + // configuration update + // - Dynamically registered DidChangeConfiguration + // - ConfigurationRequest support + if ctx.client_capabilities.did_change_configuration_support() + == (true, true) + && ctx.client_capabilities.configuration_support() { + const CONFIG_ID: &str = "dls-config"; + let reg_params = RegistrationParams { + registrations: vec![Registration { + id: CONFIG_ID.to_owned(), + method: + ::METHOD.to_owned(), + register_options: None, + }], + }; + ctx.send_request::(reg_params, &out); + } // Register files we watch for changes based on config const WATCH_ID: &str = "dls-watch"; - let id = out.provide_id(); let reg_params = RegistrationParams { registrations: vec![Registration { id: WATCH_ID.to_owned(), @@ -51,9 +66,7 @@ impl BlockingNotificationAction for Initialized { |fw|fw.watchers_config()), }], }; - - let request = Request::::new(id, reg_params); - out.request(request); + ctx.send_request::(reg_params, &out); Ok(()) } } @@ -61,7 +74,7 @@ impl BlockingNotificationAction for Initialized { impl BlockingNotificationAction for DidOpenTextDocument { fn handle( params: Self::Params, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O, ) -> Result<(), ResponseError> { debug!("on_open: {:?}", params.text_document.uri); @@ -80,7 +93,7 @@ impl BlockingNotificationAction for DidOpenTextDocument { impl BlockingNotificationAction for DidCloseTextDocument { fn handle( params: Self::Params, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O, ) -> Result<(), ResponseError> { debug!("on_close: {:?}", params.text_document.uri); @@ -94,7 +107,7 @@ impl BlockingNotificationAction for DidCloseTextDocument { impl BlockingNotificationAction for DidChangeTextDocument { fn handle( params: Self::Params, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O, ) -> Result<(), ResponseError> { debug!("on_change: {:?}, thread: {:?}", params, thread::current().id()); @@ -152,7 +165,7 @@ impl BlockingNotificationAction for DidChangeTextDocument { impl BlockingNotificationAction for Cancel { fn handle( _params: CancelParams, - _ctx: &mut InitActionContext, + _ctx: &mut InitActionContext, _out: O, ) -> Result<(), ResponseError> { // Nothing to do. @@ -163,10 +176,23 @@ impl BlockingNotificationAction for Cancel { impl BlockingNotificationAction for DidChangeConfiguration { fn handle( params: DidChangeConfigurationParams, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O, ) -> Result<(), ResponseError> { debug!("config change: {:?}", params.settings); + // New style config update, send re-config request + if params.settings.is_null() || params.settings.as_object(). + map_or(false, |o|o.is_empty()) { + let config_params = lsp_types::ConfigurationParams { + items: vec![lsp_types::ConfigurationItem { + scope_uri: None, + section: Some("simics-modeling.dls".to_string()), + }], + }; + ctx.send_request::( + config_params, &out); + return Ok(()); + } use std::collections::HashMap; let mut dups = HashMap::new(); let mut unknowns = vec![]; @@ -199,7 +225,7 @@ impl BlockingNotificationAction for DidChangeConfiguration { impl BlockingNotificationAction for DidSaveTextDocument { fn handle( params: DidSaveTextDocumentParams, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O, ) -> Result<(), ResponseError> { let file_path = parse_file_path!(¶ms.text_document.uri, "on_save")?; @@ -217,7 +243,7 @@ impl BlockingNotificationAction for DidSaveTextDocument { impl BlockingNotificationAction for DidChangeWatchedFiles { fn handle( params: DidChangeWatchedFilesParams, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O, ) -> Result<(), ResponseError> { if let Some(file_watch) = FileWatch::new(ctx) { @@ -233,7 +259,7 @@ impl BlockingNotificationAction for DidChangeWorkspaceFolders { // Respond to the `initialized` notification. fn handle( params: DidChangeWorkspaceFoldersParams, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, _out: O, ) -> Result<(), ResponseError> { let added = params.event.added; @@ -286,7 +312,7 @@ impl LSPNotification for ChangeActiveContexts { impl BlockingNotificationAction for ChangeActiveContexts { fn handle( params: ChangeActiveContextsParams, - ctx: &mut InitActionContext, + ctx: &mut InitActionContext, out: O) -> Result<(), ResponseError> { debug!("ChangeActiveContexts: {:?}", params); let contexts: Vec = params.active_contexts diff --git a/src/actions/requests.rs b/src/actions/requests.rs index 79478e6..eef48c1 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -3,7 +3,7 @@ //! Requests that the DLS can respond to. use jsonrpc::error::{StandardError, standard_error}; -use log::{debug, error, trace, warn}; +use log::{debug, error, info, trace, warn}; use serde::{Deserialize, Serialize}; use serde_json::Value; use std::collections::HashSet; @@ -18,6 +18,7 @@ use crate::actions::notifications::ContextDefinitionKindParam; use crate::analysis::{ZeroSpan, ZeroFilePosition, SymbolRef}; use crate::analysis::reference::ReferenceKind; use crate::analysis::symbols::SimpleSymbol; +use crate::config::Config; pub use crate::lsp_data::request::{ ApplyWorkspaceEdit, @@ -34,8 +35,10 @@ pub use crate::lsp_data::request::{ HoverRequest, RangeFormatting, References, + RegisterCapability, Rename, ResolveCompletionItem as ResolveCompletion, + WorkspaceConfiguration, WorkspaceSymbolRequest, }; @@ -50,7 +53,7 @@ use crate::actions::analysis_storage::AnalysisLookupError; use crate::config::WarningFrequency; use crate::file_management::CanonPath; use crate::server; -use crate::server::{Ack, Output, Request, RequestAction, +use crate::server::{Ack, Output, Request, RequestAction, SentRequest, Response, ResponseError, ResponseWithMessage}; // Gives a slightly-better error message than the short-form one specified by @@ -84,11 +87,11 @@ fn warn_miss_lookup(error: AnalysisLookupError, file: Option<&str>) { } } -fn response_maybe_with_limitations( +fn response_maybe_with_limitations( path: &Path, response: R, limitations: I, - ctx: &InitActionContext) + ctx: &InitActionContext) -> ResponseWithMessage where I: IntoIterator, @@ -131,10 +134,12 @@ pub const TYPE_SEMANTIC_LIMITATION: DLSLimitation = DLSLimitation { }; // TODO: This function is getting bloated, refactor into several smaller ones -fn fp_to_symbol_refs(fp: &ZeroFilePosition, - ctx: &InitActionContext, - relevant_limitations: &mut HashSet) - -> Result, AnalysisLookupError> { +fn fp_to_symbol_refs + (fp: &ZeroFilePosition, + ctx: &InitActionContext, + relevant_limitations: &mut HashSet) + -> Result, AnalysisLookupError> +{ let analysis = ctx.analysis.lock().unwrap(); // This step-by-step approach could be folded into analysis_storage, // but I keep it as separate here so that we could, perhaps, @@ -218,9 +223,11 @@ fn fp_to_symbol_refs(fp: &ZeroFilePosition, Ok(definitions) } -fn handle_default_remapping(ctx: &InitActionContext, - symbols: Vec, - fp: &ZeroFilePosition) -> HashSet { +fn handle_default_remapping + (ctx: &InitActionContext, + symbols: Vec, + fp: &ZeroFilePosition) -> HashSet +{ let analysis = ctx.analysis.lock().unwrap(); let refr_opt = analysis.reference_at_pos(fp); // NOTE: Because the call to this is preceded by a symbol lookup, @@ -403,8 +410,8 @@ impl RequestAction for WorkspaceSymbolRequest { Ok(None) } - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result { let analysis = ctx.analysis.lock().unwrap(); @@ -428,8 +435,8 @@ impl RequestAction for DocumentSymbolRequest { Ok(None) } - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result { debug!("Handing doc symbol request {:?}", params); @@ -463,7 +470,7 @@ impl RequestAction for HoverRequest { range: None }) } - fn handle(mut ctx: InitActionContext, + fn handle(mut ctx: InitActionContext, params: Self::Params, ) -> Result { debug!("handling hover ({:?})", params); @@ -488,8 +495,8 @@ impl RequestAction for GotoImplementation { Ok(None.into()) } - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result { debug!("Requesting implementations with params {:?}", params); @@ -552,8 +559,8 @@ impl RequestAction for GotoDeclaration { Ok(None.into()) } - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result { debug!("Requesting declarations with params {:?}", params); @@ -610,8 +617,8 @@ impl RequestAction for GotoDefinition { Ok(None.into()) } - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result { debug!("Requesting definitions with params {:?}", params); @@ -668,8 +675,8 @@ impl RequestAction for References { Ok(vec![].into()) } - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result { debug!("Requesting references with params {:?}", params); @@ -721,8 +728,8 @@ impl RequestAction for Completion { Ok(vec![]) } - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: Self::Params, ) -> Result { // TODO: Acquire completions for location @@ -737,8 +744,8 @@ impl RequestAction for DocumentHighlightRequest { Ok(vec![]) } - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: Self::Params, ) -> Result { // TODO: Acquire highlighting info for file and span @@ -756,8 +763,8 @@ impl RequestAction for Rename { change_annotations: None })) } - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: Self::Params, ) -> Result { // TODO: Perform a rename @@ -800,8 +807,8 @@ impl RequestAction for ExecuteCommand { } /// Currently, no support for this - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: ExecuteCommandParams, ) -> Result { // TODO: handle specialized commands. or if no such commands, remove @@ -820,8 +827,8 @@ impl RequestAction for CodeActionRequest { Ok(vec![]) } - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: Self::Params, ) -> Result { // TODO: figure out if we want to use this @@ -840,8 +847,8 @@ impl RequestAction for Formatting { )) } - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: Self::Params, ) -> Result { // TODO: format document @@ -859,8 +866,8 @@ impl RequestAction for RangeFormatting { )) } - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: Self::Params, ) -> Result { // TODO: format range @@ -875,7 +882,7 @@ impl RequestAction for ResolveCompletion { Err(ResponseError::Empty) } - fn handle(_: InitActionContext, _params: Self::Params) -> Result { + fn handle(_: InitActionContext, _params: Self::Params) -> Result { // TODO: figure out if we want to use this Self::fallback_response() } @@ -888,8 +895,8 @@ impl RequestAction for CodeLensRequest { Err(ResponseError::Empty) } - fn handle( - _ctx: InitActionContext, + fn handle( + _ctx: InitActionContext, _params: Self::Params, ) -> Result { // TODO: figure out if we want to use this @@ -931,8 +938,8 @@ impl RequestAction for GetKnownContextsRequest { Err(ResponseError::Empty) } - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result { let for_these_paths: Vec = @@ -982,3 +989,57 @@ impl RequestAction for GetKnownContextsRequest { ) } } + +/// Server-to-client requests +impl SentRequest for RegisterCapability { + type Response = ::Result; + fn on_response + (_ctx: &mut InitActionContext, _response: Self::Response, _out: &O) { + info!("Successful registration on some capability"); + } +} + +impl SentRequest for WorkspaceConfiguration { + type Response = ::Result; + fn on_response + (ctx: &mut InitActionContext, response: Self::Response, out: &O) + { + info!("Acquired configuration updates {:?}", response); + + let dml_object = if response.len() == 1 { + &response[0] + } else { + error!("Received incorrectly formatted response to \ + 'workspace/configuration' from client (array length \ + should be 1)\ + : {:?}", + response); + return; + }; + use std::collections::HashMap; + let mut dups = HashMap::new(); + let mut unknowns = vec![]; + let mut deprecated = vec![]; + let settings = Config::try_deserialize( + dml_object, + &mut dups, + &mut unknowns, + &mut deprecated, + ); + crate::server::maybe_notify_unknown_configs(out, &unknowns); + crate::server::maybe_notify_deprecated_configs(out, &deprecated); + crate::server::maybe_notify_duplicated_configs(out, &dups); + + let new_config = match settings { + Ok(value) => value, + Err(err) => { + warn!("Received unactionable config: {:?} (error: {:?})", + &response, err); + return; + } + }; + + ctx.config.lock().unwrap().update(new_config); + ctx.maybe_changed_config(out); + } +} diff --git a/src/cmd.rs b/src/cmd.rs index 353353a..1ea4cdd 100644 --- a/src/cmd.rs +++ b/src/cmd.rs @@ -156,7 +156,7 @@ fn def(file_name: &str, row: &str, col: &str) partial_result_token: None, }, }; - Request { id: next_id(), params, received: Instant::now(), _action: PhantomData } + Request { id: next_id(), params, received: Instant::now(), action: PhantomData } } fn workspace_symbol(query: &str) -> Request { @@ -169,7 +169,7 @@ fn workspace_symbol(query: &str) -> Request { work_done_token: None, }, }; - Request { id: next_id(), params, received: Instant::now(), _action: PhantomData } + Request { id: next_id(), params, received: Instant::now(), action: PhantomData } } fn document_symbol(file_name: &str) @@ -183,11 +183,11 @@ fn document_symbol(file_name: &str) work_done_token: None, }, }; - Request { id: next_id(), params, received: Instant::now(), _action: PhantomData } + Request { id: next_id(), params, received: Instant::now(), action: PhantomData } } pub fn shutdown() -> Request { - Request { id: next_id(), params: (), received: Instant::now(), _action: PhantomData } + Request { id: next_id(), params: (), received: Instant::now(), action: PhantomData } } pub fn exit() -> Notification { @@ -224,7 +224,7 @@ pub fn initialize(root_path: String) -> Request { work_done_token: None, }, }; - Request { id: next_id(), params, received: Instant::now(), _action: PhantomData } + Request { id: next_id(), params, received: Instant::now(), action: PhantomData } } fn open(file_name: &str) @@ -303,7 +303,7 @@ pub fn get_contexts(paths: Vec) -> Request, dups: &mut std::collections::HashMap>, unknowns: &mut Vec, deprecated: &mut Vec, @@ -249,9 +247,9 @@ impl Config { } } - if let serde_json::Value::Object(map) = val { - let seq = serde::de::value::MapDeserializer::new(map.iter().filter_map(|(k, v)| { - use heck::ToSnakeCase; + use heck::ToSnakeCase; + let seq = serde::de::value::MapDeserializer::new( + vals.iter().filter_map(|(k, v)| { let snake_case = k.to_snake_case(); let vec = dups.entry(snake_case.clone()).or_default(); vec.push(k.to_string()); @@ -266,19 +264,37 @@ impl Config { None } })); - match serde_ignored::deserialize(seq, |path| unknowns.push(path.to_string())) { - Ok(conf) => { - dups.retain(|_, v| v.len() > 1); - return Ok(conf); - } - _ => { - dups.retain(|_, v| v.len() > 1); - } + match serde_ignored::deserialize( + seq, |path| unknowns.push(path.to_string())) { + Ok(conf) => { + dups.retain(|_, v| v.len() > 1); + return Ok(conf); + } + _ => { + dups.retain(|_, v| v.len() > 1); } } Err(().into()) } + /// try to deserialize a Config from a json value, val is expected to be a + /// Value::Object, all first level keys of val are converted to snake_case, + /// duplicated and unknown keys are reported + pub fn try_deserialize( + val: &serde_json::value::Value, + dups: &mut std::collections::HashMap>, + unknowns: &mut Vec, + deprecated: &mut Vec, + ) -> Result { + if let serde_json::Value::Object(map) = val { + return Self::try_deserialize_vec( + &map.iter().map(|(k,v)|(k.clone(), v.clone())).collect(), + dups, unknowns, deprecated + ); + } + Err(().into()) + } + /// Join this configuration with the new config. pub fn update(&mut self, mut new: Config) { if let Some(new_retain) = new.analysis_retain_duration { diff --git a/src/lsp_data.rs b/src/lsp_data.rs index b144d80..e389719 100644 --- a/src/lsp_data.rs +++ b/src/lsp_data.rs @@ -316,7 +316,7 @@ impl InitializationOptions { #[serde(default)] pub struct ClientCapabilities { pub root: Option, - pub workspace_folders: bool, + pub capabilities: lsp_types::ClientCapabilities, } impl ClientCapabilities { @@ -327,14 +327,32 @@ impl ClientCapabilities { root: params.root_uri.as_ref().and_then( |uri|parse_file_path!(uri, "decode root").ok()), - workspace_folders: params.capabilities.workspace.as_ref().and_then( - |workspace|workspace.workspace_folders.and_then( - |folders|if folders { Some(folders)} else { None }) - ).is_some(), + capabilities: params.capabilities.clone(), } } } +// Fast-access to fields we care about +impl ClientCapabilities { + pub fn workspace_folder_support(&self) -> bool { + self.capabilities.workspace.as_ref() + .map_or(false, |w|w.workspace_folders.unwrap_or(false)) + } + // (supported, dynamic registration supported) + pub fn did_change_configuration_support(&self) -> (bool, bool) { + if let Some(dcc) = self.capabilities.workspace.as_ref() + .and_then(|w|w.did_change_configuration) { + (true, dcc.dynamic_registration.unwrap_or(false)) + } else { + (false, false) + } + } + pub fn configuration_support(&self) -> bool { + self.capabilities.workspace.as_ref() + .map_or(false, |w|w.configuration.unwrap_or(false)) + } +} + #[cfg(test)] mod test { use super::*; diff --git a/src/server/dispatch.rs b/src/server/dispatch.rs index 29e0fe0..2054f1c 100644 --- a/src/server/dispatch.rs +++ b/src/server/dispatch.rs @@ -5,7 +5,7 @@ use std::thread; use std::time::Duration; use jsonrpc::error::StandardError::InternalError; -use log::debug; +use log::{info, error, debug}; use crate::actions::work_pool; use crate::actions::work_pool::WorkDescription; @@ -14,7 +14,7 @@ use crate::concurrency::{ConcurrentJob, JobToken}; use crate::lsp_data::LSPRequest; use crate::server; use crate::server::io::Output; -use crate::server::message::ResponseError; +use crate::server::message::{RawResponse, ResponseError}; use crate::server::{Request, Response}; use crate::actions::requests::*; @@ -48,7 +48,7 @@ macro_rules! define_dispatch_request_enum { )* impl DispatchRequest { - fn handle(self, ctx: InitActionContext, out: &O) { + fn handle(self, ctx: InitActionContext, out: &O) { match self { $( DispatchRequest::$request_type(req) => { @@ -110,14 +110,14 @@ define_dispatch_request_enum!( /// handle the requests sequentially, without blocking stdin. /// Requests dispatched this way are automatically timed out & avoid /// processing if have already timed out before starting. -pub(crate) struct Dispatcher { - sender: mpsc::Sender<(DispatchRequest, InitActionContext, JobToken)>, +pub(crate) struct Dispatcher { + sender: mpsc::Sender<(DispatchRequest, InitActionContext, JobToken)>, } -impl Dispatcher { +impl Dispatcher { /// Creates a new `Dispatcher` starting a new thread and channel. - pub(crate) fn new(out: O) -> Self { - let (sender, receiver) = mpsc::channel::<(DispatchRequest, InitActionContext, JobToken)>(); + pub(crate) fn new(out: O) -> Self { + let (sender, receiver) = mpsc::channel::<(DispatchRequest, InitActionContext, JobToken)>(); thread::Builder::new() .name("dispatch-worker".into()) @@ -136,7 +136,7 @@ impl Dispatcher { pub(crate) fn dispatch>( &mut self, request: R, - ctx: InitActionContext, + ctx: InitActionContext, ) { let (job, token) = ConcurrentJob::new(); ctx.add_job(job); @@ -161,8 +161,45 @@ pub trait RequestAction: LSPRequest { fn fallback_response() -> Result; /// Request processing logic. - fn handle( - ctx: InitActionContext, + fn handle( + ctx: InitActionContext, params: Self::Params, ) -> Result; } + +pub type HandleResponseType = fn (&mut InitActionContext, + RawResponse, + &O); +// Request sent from server to client +pub trait SentRequest: LSPRequest { + /// Expected deserializable response type + type Response: for <'a> serde::Deserialize<'a>; + + /// Max duration this request should finish within + fn timeout() -> Duration { + DEFAULT_REQUEST_TIMEOUT + } + + fn handle_response + (ctx: &mut InitActionContext, response: RawResponse, out: &O) + { + match response.result { + Ok(val) => { + match serde_json::from_value(val) { + Ok(response_value) => Self::on_response( + ctx, response_value, out), + Err(response_err) => + error!("Unexpected response value to request: {:?} \ + (wanted {})", + response_err, + std::any::type_name::()), + } + }, + Err(error) => { + info!("Received an error to request on client: {:?}", error); + } + } + } + fn on_response + (ctx: &mut InitActionContext, response: Self::Response, out: &O); +} diff --git a/src/server/message.rs b/src/server/message.rs index ee730ac..054b43b 100644 --- a/src/server/message.rs +++ b/src/server/message.rs @@ -106,7 +106,7 @@ where R: Response + std::fmt::Debug + serde::Serialize { /// Blocks stdin whilst being handled. pub trait BlockingNotificationAction: LSPNotification { /// Handles this notification. - fn handle(_: Self::Params, _: &mut InitActionContext, _: O) + fn handle(_: Self::Params, _: &mut InitActionContext, _: O) -> Result<(), ResponseError>; } @@ -118,7 +118,7 @@ pub trait BlockingRequestAction: LSPRequest { fn handle( id: RequestId, params: Self::Params, - ctx: &mut ActionContext, + ctx: &mut ActionContext, out: O, ) -> Result; } @@ -175,6 +175,12 @@ impl From for RequestId { } } +impl From for RequestId { + fn from(value: String) -> Self { + RequestId::Str(value) + } +} + /// A request that gets JSON serialized in the language server protocol. pub struct Request { /// The unique request ID. @@ -184,13 +190,13 @@ pub struct Request { /// The extra action-specific parameters. pub params: A::Params, /// This request's handler action. - pub _action: PhantomData, + pub action: PhantomData, } impl Request { /// Creates a server `Request` structure with given `params`. pub fn new(id: RequestId, params: A::Params) -> Request { - Request { id, received: Instant::now(), params, _action: PhantomData } + Request { id, received: Instant::now(), params, action: PhantomData } } } @@ -253,7 +259,7 @@ where impl Request { pub fn blocking_dispatch( self, - ctx: &mut ActionContext, + ctx: &mut ActionContext, out: &O, ) -> Result { A::handle(self.id, self.params, ctx, out.clone()) @@ -261,7 +267,7 @@ impl Request { } impl Notification { - pub fn dispatch(self, ctx: &mut InitActionContext, out: O) + pub fn dispatch(self, ctx: &mut InitActionContext, out: O) -> Result<(), ResponseError> { A::handle(self.params, ctx, out)?; Ok(()) @@ -335,9 +341,9 @@ impl RawMessage { Value::String(ref s) => Some(RequestId::Str(s.to_string())), _ => None, }; - - match parsed_id { - Some(id) => Ok(Request { id, params, received: Instant::now(), _action: PhantomData }), + + match parsed_id { + Some(id) => Ok(Request { id, params, received: Instant::now(), action: PhantomData }), None => Err(rpc_error(InvalidRequest, "Failed to parse Id")), } } @@ -418,15 +424,11 @@ pub struct ErrorResponse { data: Option, } -#[derive(Debug, PartialEq)] -pub enum ResultOrError { - Result(Value), - Error(ErrorResponse), -} +pub type ResultOrError = Result; #[derive(Debug, PartialEq)] pub struct RawResponse { - pub id: Value, + pub id: RequestId, pub result: ResultOrError, } @@ -438,6 +440,18 @@ impl RawResponse { .ok_or_else(||rpc_error(InvalidRequest, "No ID in response"))? .to_owned(); + let parsed_id = match id { + Value::Number(n) if n.is_u64() => + Some(RequestId::Num(n.as_u64().unwrap())), + Value::String(ref s) => Some(RequestId::Str(s.to_string())), + _ => None, + }; + + let parsed_id = match parsed_id { + Some(id) => id, + None => return Err(rpc_error(InvalidRequest, "Failed to parse Id")), + }; + let result = ls_command.get_mut("result").map( |r|r.take()); let error = ls_command.get_mut("error").map( @@ -452,13 +466,12 @@ impl RawResponse { "Neither 'result' and 'error' \ specified in response")), (Some(result), None) => - ResultOrError::Result(result), + Ok(result), (None, Some(error)) => - ResultOrError::Error(error.map_err( - |e|rpc_error(ParseError, e))?), + Err(error.map_err(|e|rpc_error(ParseError, e))?), }; - Ok(RawResponse { id, result: resultorerror }) + Ok(RawResponse { id: parsed_id, result: resultorerror }) } } @@ -490,13 +503,15 @@ impl RawMessageOrResponse { matches!(self, RawMessageOrResponse::Response(_)) } - pub(crate) fn as_message(self) -> Option { + #[allow(dead_code)] + pub(crate) fn as_message(&self) -> Option<&RawMessage> { match self { RawMessageOrResponse::Message(mess) => Some(mess), _ => None, } } - pub(crate) fn as_response(self) -> Option { + #[allow(dead_code)] + pub(crate) fn as_response(&self) -> Option<&RawResponse> { match self { RawMessageOrResponse::Response(resp) => Some(resp), _ => None, @@ -544,9 +559,9 @@ mod test { params: serde_json::Value::Null, }; - let parsed = RawMessageOrResponse::try_parse(&raw_json) - .unwrap().as_message().unwrap(); - assert_eq!(expected_msg, parsed); + let raw = RawMessageOrResponse::try_parse(&raw_json).unwrap(); + let parsed = raw.as_message().unwrap(); + assert_eq!(&expected_msg, parsed); } #[test] @@ -564,9 +579,9 @@ mod test { // Missing parameters are represented internally as Null. params: serde_json::Value::Null, }; - let parsed = RawMessageOrResponse::try_parse(&raw_json) - .unwrap().as_message().unwrap(); - assert_eq!(expected_msg, parsed); + let raw = RawMessageOrResponse::try_parse(&raw_json).unwrap(); + let parsed = raw.as_message().unwrap(); + assert_eq!(&expected_msg, parsed); } #[test] @@ -635,8 +650,8 @@ mod test { #[test] fn deserialize_message_empty_params() { let msg = r#"{"jsonrpc":"2.0","method":"initialized","params":{}}"#; - let parsed = RawMessageOrResponse::try_parse(msg) - .unwrap().as_message().unwrap(); + let raw = RawMessageOrResponse::try_parse(msg).unwrap(); + let parsed = raw.as_message().unwrap(); parsed.parse_as_notification::().unwrap(); } @@ -654,17 +669,17 @@ mod test { .to_string(); let expected_resp = RawResponse { - id: Value::from("abc"), - result: ResultOrError::Result(json!({ + id: RequestId::from("abc".to_string()), + result: ResultOrError::Ok(json!({ "this" : 0, "really" : "could", "be" : ["anything"] })) }; - let parsed = RawMessageOrResponse::try_parse(&raw_json) - .unwrap().as_response().unwrap(); - assert_eq!(expected_resp, parsed); + let raw = RawMessageOrResponse::try_parse(&raw_json).unwrap(); + let parsed = raw.as_response().unwrap(); + assert_eq!(&expected_resp, parsed); } #[test] fn simple_request_error() { @@ -679,8 +694,8 @@ mod test { .to_string(); let expected_resp = RawResponse { - id: Value::from("abc"), - result: ResultOrError::Error( + id: RequestId::from("abc".to_string()), + result: ResultOrError::Err( ErrorResponse { code: 0, message: "oh no!".to_string(), @@ -689,8 +704,8 @@ mod test { ), }; - let parsed = RawMessageOrResponse::try_parse(&raw_json) - .unwrap().as_response().unwrap(); - assert_eq!(expected_resp, parsed); + let raw = RawMessageOrResponse::try_parse(&raw_json).unwrap(); + let parsed = raw.as_response().unwrap(); + assert_eq!(&expected_resp, parsed); } } diff --git a/src/server/mod.rs b/src/server/mod.rs index a57f90d..47a3f50 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -14,10 +14,11 @@ use crate::lsp_data::{ ShowMessageParams, Workspace, }; use crate::server::dispatch::Dispatcher; -pub use crate::server::dispatch::{RequestAction, DEFAULT_REQUEST_TIMEOUT}; +pub use crate::server::dispatch::{RequestAction, SentRequest, + DEFAULT_REQUEST_TIMEOUT}; pub use crate::server::io::{MessageReader, Output}; use crate::server::io::{StdioMsgReader, StdioOutput}; -use crate::server::message::{RawMessage, RawMessageOrResponse}; +use crate::server::message::{RawMessage, RawMessageOrResponse, RawResponse}; pub use crate::server::message::{ Ack, BlockingNotificationAction, BlockingRequestAction, NoResponse, Notification, Request, @@ -88,7 +89,7 @@ impl BlockingRequestAction for ShutdownRequest { fn handle( _id: RequestId, _params: Self::Params, - ctx: &mut ActionContext, + ctx: &mut ActionContext, _out: O, ) -> Result { if let Ok(ctx) = ctx.inited() { @@ -192,7 +193,7 @@ impl BlockingRequestAction for InitializeRequest { fn handle( id: RequestId, mut params: Self::Params, - ctx: &mut ActionContext, + ctx: &mut ActionContext, out: O, ) -> Result { let mut dups = std::collections::HashMap::new(); @@ -276,8 +277,8 @@ pub struct LsService { output: O, server_send: channel::Sender, server_receive: channel::Receiver, - ctx: ActionContext, - dispatcher: Dispatcher, + ctx: ActionContext, + dispatcher: Dispatcher, } impl LsService { @@ -332,8 +333,8 @@ impl LsService { send.send(ServerToHandle::ClientMessage(rm)).ok(); }, Ok(RawMessageOrResponse::Response(rr)) => { - // TODO debug!("Parsed a response: {}", rr.id); + send.send(ServerToHandle::ClientResponse(rr)).ok(); }, Err(e) => { error!("parsing error, {:?}", e); @@ -368,6 +369,11 @@ impl LsService { ServerStateChange::Break { exit_code } => return exit_code, }, + ServerToHandle::ClientResponse(resp) => { + if let ActionContext::Init(ctx) = &mut self.ctx { + ctx.handle_request_response(resp, &self.output); + } + }, ServerToHandle::ExitCode(code) => return code, ServerToHandle::IsolatedAnalysisDone(path, context, requests) => { @@ -622,6 +628,7 @@ impl LsService { #[derive(PartialEq, Debug)] pub enum ServerToHandle { ClientMessage(RawMessage), + ClientResponse(RawResponse), ExitCode(i32), IsolatedAnalysisDone(CanonPath, Option, Vec), DeviceAnalysisDone(CanonPath), @@ -650,7 +657,7 @@ fn experimental_caps() -> Value { }).unwrap() } -fn server_caps(_ctx: &ActionContext) -> ServerCapabilities { +fn server_caps(_ctx: &ActionContext) -> ServerCapabilities { ServerCapabilities { call_hierarchy_provider: None, declaration_provider: Some(DeclarationCapability::Simple(true)), @@ -802,9 +809,10 @@ mod test { fn parse_shutdown_object_params() { let raw = RawMessageOrResponse::try_parse( r#"{"jsonrpc": "2.0", "id": 2, "method": "shutdown", "params": {}}"#, - ).unwrap().as_message().unwrap(); + ).unwrap(); + let parsed = raw.as_message().unwrap(); let _request: Request = - raw.parse_as_request().expect("Boring validation is happening"); + parsed.parse_as_request().expect("Boring validation is happening"); } } From 75832af8040c645cba94f27fd4555ac38a8ece15 Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 28 May 2025 14:08:30 +0200 Subject: [PATCH 3/7] Dont bother the server with unknown configs When we get vscode to only send the relevant-for-dls configs (whether that is by specifying a new domain for them, or by filtering manually) we can revert this Signed-off-by: Jonatan Waern --- src/server/mod.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/server/mod.rs b/src/server/mod.rs index 47a3f50..95ba735 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -106,7 +106,7 @@ impl BlockingRequestAction for ShutdownRequest { } } -pub(crate) fn maybe_notify_unknown_configs(out: &O, unknowns: &[String]) { +pub(crate) fn maybe_notify_unknown_configs(_out: &O, unknowns: &[String]) { use std::fmt::Write; if unknowns.is_empty() { return; @@ -117,10 +117,11 @@ pub(crate) fn maybe_notify_unknown_configs(out: &O, unknowns: &[Strin write!(msg, "{}`{}` ", if first { ' ' } else { ',' }, key).unwrap(); first = false; } - out.notify(Notification::::new(ShowMessageParams { - typ: MessageType::WARNING, - message: msg, - })); + warn!("{}", msg); + // out.notify(Notification::::new(ShowMessageParams { + // typ: MessageType::WARNING, + // message: msg, + // })); } #[allow(dead_code)] From 0dd212257214152b333f93cd31ffe4ec9b41d09f Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Tue, 3 Jun 2025 09:50:04 +0200 Subject: [PATCH 4/7] Track configuration changes when updated Signed-off-by: Jonatan Waern --- src/actions/mod.rs | 17 ++++++++++++----- src/actions/notifications.rs | 6 +++--- src/actions/requests.rs | 4 ++-- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/actions/mod.rs b/src/actions/mod.rs index 6219c2a..493636d 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -375,7 +375,7 @@ impl InitActionContext { } } - fn update_linter_config(&mut self, _out: &O) { + fn update_linter_config(&self, _out: &O) { trace!("Updating linter config"); if let Ok(config) = self.config.lock() { *self.lint_config.lock().unwrap() = @@ -385,7 +385,7 @@ impl InitActionContext { } } - pub fn update_compilation_info(&mut self, out: &O) { + pub fn update_compilation_info(&self, out: &O) { trace!("Updating compile info"); if let Ok(config) = self.config.lock() { if let Some(compile_info) = &config.compile_info_path { @@ -566,10 +566,17 @@ impl InitActionContext { // Called when config might have changed, re-update include paths // and similar - pub fn maybe_changed_config(&mut self, out: &O) { + pub fn maybe_changed_config(&mut self, + old_config: Config, + out: &O) { trace!("Compilation info might have changed"); - self.update_compilation_info(out); - self.update_linter_config(out); + let config = self.config.lock().unwrap(); + if config.compile_info_path != old_config.compile_info_path { + self.update_compilation_info(out); + } + if config.lint_cfg_path != old_config.lint_cfg_path { + self.update_linter_config(out); + } } // Call before adding new analysis diff --git a/src/actions/notifications.rs b/src/actions/notifications.rs index 500fe42..7dcb4e8 100644 --- a/src/actions/notifications.rs +++ b/src/actions/notifications.rs @@ -214,9 +214,9 @@ impl BlockingNotificationAction for DidChangeConfiguration { return Err(().into()); } }; - + let old = ctx.config.lock().unwrap().clone(); ctx.config.lock().unwrap().update(new_config); - ctx.maybe_changed_config(&out); + ctx.maybe_changed_config(old, &out); Ok(()) } @@ -248,7 +248,7 @@ impl BlockingNotificationAction for DidChangeWatchedFiles { ) -> Result<(), ResponseError> { if let Some(file_watch) = FileWatch::new(ctx) { if params.changes.iter().any(|c| file_watch.is_relevant(c)) { - ctx.maybe_changed_config(&out); + ctx.update_compilation_info(&out); } } Ok(()) diff --git a/src/actions/requests.rs b/src/actions/requests.rs index eef48c1..b557e0c 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -1038,8 +1038,8 @@ impl SentRequest for WorkspaceConfiguration { return; } }; - + let old = ctx.config.lock().unwrap().clone(); ctx.config.lock().unwrap().update(new_config); - ctx.maybe_changed_config(out); + ctx.maybe_changed_config(old, out); } } From 3dd44138405e4000eb414b457efb0bdd6811e71b Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Tue, 3 Jun 2025 12:03:32 +0200 Subject: [PATCH 5/7] Clean out 'mut' from self arguments in InitActionContext methods Imposes unnecessary restrictions on code that can be written on them, and incorrectly gives the impression that modifying the InitActionContext directly is valid (it's usually not) Signed-off-by: Jonatan Waern --- src/actions/mod.rs | 41 ++++++++++++++++++++++------------------- src/actions/requests.rs | 4 ++-- src/server/dispatch.rs | 6 +++--- 3 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/actions/mod.rs b/src/actions/mod.rs index 493636d..e457074 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -346,12 +346,12 @@ impl InitActionContext { } } - fn add_direct_open(&mut self, path: PathBuf) { + fn add_direct_open(&self, path: PathBuf) { let canon_path: CanonPath = path.into(); self.direct_opens.lock().unwrap().insert(canon_path); } - fn remove_direct_open(&mut self, path: PathBuf) { + fn remove_direct_open(&self, path: PathBuf) { let canon_path: CanonPath = path.into(); if !self.direct_opens.lock().unwrap().remove(&canon_path) { debug!("Tried to remove a directly opened file ({:?}) \ @@ -365,7 +365,7 @@ impl InitActionContext { self.update_compilation_info(&out) } - pub fn update_workspaces(&mut self, + pub fn update_workspaces(&self, mut add: Vec, remove: Vec) { if let Ok(mut workspaces) = self.workspace_roots.lock() { @@ -431,7 +431,7 @@ impl InitActionContext { } } - pub fn report_errors(&mut self, output: &O) { + pub fn report_errors(&self, output: &O) { self.update_analysis(); let filter = Some(self.device_active_contexts.lock().unwrap().clone()); let (isolated, device, lint) = @@ -530,12 +530,12 @@ impl InitActionContext { Ok(new_compinfo) } - pub fn update_analysis(&mut self) { + pub fn update_analysis(&self) { self.analysis.lock().unwrap() .update_analysis(&self.construct_resolver()); } - pub fn trigger_device_analysis(&mut self, file: &Path, out: &O) { + pub fn trigger_device_analysis(&self, file: &Path, out: &O) { let canon_path: CanonPath = file.to_path_buf().into(); debug!("triggering devices dependant on {}", canon_path.as_str()); self.update_analysis(); @@ -566,7 +566,7 @@ impl InitActionContext { // Called when config might have changed, re-update include paths // and similar - pub fn maybe_changed_config(&mut self, + pub fn maybe_changed_config(&self, old_config: Config, out: &O) { trace!("Compilation info might have changed"); @@ -580,7 +580,7 @@ impl InitActionContext { } // Call before adding new analysis - pub fn maybe_start_progress(&mut self, out: &O) { + pub fn maybe_start_progress(&self, out: &O) { let mut notifier = self.current_notifier.lock().unwrap(); @@ -610,6 +610,9 @@ impl InitActionContext { } } + // NOTE: Do not call this method from outside the main server loop, + // as notification/request handlers obtain _copies_ of the context, + // and not the context itself fn maybe_warn_missing_builtins(&mut self, out: &O) { if !self.has_notified_missing_builtins && !self.analysis.lock().unwrap().has_client_file( @@ -640,7 +643,7 @@ impl InitActionContext { toret } - pub fn isolated_analyze(&mut self, + pub fn isolated_analyze(&self, client_path: &Path, context: Option, out: &O) { @@ -669,7 +672,7 @@ impl InitActionContext { &self.vfs, context, path, client_path.to_path_buf(), token); } - fn device_analyze(&mut self, device: &CanonPath, out: &O) { + fn device_analyze(&self, device: &CanonPath, out: &O) { debug!("Wants device analysis of {:?}", device); self.maybe_start_progress(out); self.maybe_add_device_context(device); @@ -851,7 +854,7 @@ impl InitActionContext { } } - pub fn maybe_trigger_lint_analysis(&mut self, file: &Path, out: &O) { + pub fn maybe_trigger_lint_analysis(&self, file: &Path, out: &O) { if !self.config.lock().unwrap().linting_enabled { return; } @@ -870,7 +873,7 @@ impl InitActionContext { out); } - fn lint_analyze(&mut self, + fn lint_analyze(&self, file: &Path, context: Option, cfg: LintCfg, @@ -1142,17 +1145,17 @@ impl InitActionContext { out.request(request); } - pub fn handle_request_response(&mut self, resp: RawResponse, out: &O) { + pub fn handle_request_response(&self, resp: RawResponse, out: &O) { info!("Got a response to some request: {:?}", resp); let maybe_req_fn = { self.outstanding_requests .lock().unwrap().remove(&resp.id) }; if let Some(request_fn) = maybe_req_fn { - request_fn(self, resp, out); - } else { - info!("Got response for request id {:?} but it was not tracked\ - (either timed out or was never sent)", - resp.id); - } + request_fn(self, resp, out); + } else { + info!("Got response for request id {:?} but it was not tracked\ + (either timed out or was never sent)", + resp.id); + } } } diff --git a/src/actions/requests.rs b/src/actions/requests.rs index b557e0c..ff699cb 100644 --- a/src/actions/requests.rs +++ b/src/actions/requests.rs @@ -994,7 +994,7 @@ impl RequestAction for GetKnownContextsRequest { impl SentRequest for RegisterCapability { type Response = ::Result; fn on_response - (_ctx: &mut InitActionContext, _response: Self::Response, _out: &O) { + (_ctx: &InitActionContext, _response: Self::Response, _out: &O) { info!("Successful registration on some capability"); } } @@ -1002,7 +1002,7 @@ impl SentRequest for RegisterCapability { impl SentRequest for WorkspaceConfiguration { type Response = ::Result; fn on_response - (ctx: &mut InitActionContext, response: Self::Response, out: &O) + (ctx: &InitActionContext, response: Self::Response, out: &O) { info!("Acquired configuration updates {:?}", response); diff --git a/src/server/dispatch.rs b/src/server/dispatch.rs index 2054f1c..e3efacb 100644 --- a/src/server/dispatch.rs +++ b/src/server/dispatch.rs @@ -167,7 +167,7 @@ pub trait RequestAction: LSPRequest { ) -> Result; } -pub type HandleResponseType = fn (&mut InitActionContext, +pub type HandleResponseType = fn (&InitActionContext, RawResponse, &O); // Request sent from server to client @@ -181,7 +181,7 @@ pub trait SentRequest: LSPRequest { } fn handle_response - (ctx: &mut InitActionContext, response: RawResponse, out: &O) + (ctx: &InitActionContext, response: RawResponse, out: &O) { match response.result { Ok(val) => { @@ -201,5 +201,5 @@ pub trait SentRequest: LSPRequest { } } fn on_response - (ctx: &mut InitActionContext, response: Self::Response, out: &O); + (ctx: &InitActionContext, response: Self::Response, out: &O); } From b3fc30a12b60243076f86e1848f515a8683a0afc Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 28 May 2025 16:34:34 +0200 Subject: [PATCH 6/7] Update lint reporting when config changes Signed-off-by: Jonatan Waern --- src/actions/analysis_storage.rs | 4 ++ src/actions/mod.rs | 98 ++++++++++++++++++++++++++++++--- 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/src/actions/analysis_storage.rs b/src/actions/analysis_storage.rs index 5c2b724..c320094 100644 --- a/src/actions/analysis_storage.rs +++ b/src/actions/analysis_storage.rs @@ -249,6 +249,10 @@ impl AnalysisStorage { self.isolated_analysis.contains_key(path) } + pub fn has_lint_analysis(&self, path: &CanonPath) -> bool { + self.lint_analysis.contains_key(path) + } + pub fn all_dependencies(&self, path: &CanonPath, context: Option<&CanonPath>) diff --git a/src/actions/mod.rs b/src/actions/mod.rs index e457074..5ebd8ad 100644 --- a/src/actions/mod.rs +++ b/src/actions/mod.rs @@ -434,17 +434,22 @@ impl InitActionContext { pub fn report_errors(&self, output: &O) { self.update_analysis(); let filter = Some(self.device_active_contexts.lock().unwrap().clone()); - let (isolated, device, lint) = + let (isolated, device, mut lint) = self.analysis.lock().unwrap().gather_errors(filter.as_ref()); let notifier = AnalysisDiagnosticsNotifier::new("indexing".to_string(), output.clone()); notifier.notify_begin_diagnostics(); + let config = self.config.lock().unwrap(); + if !config.linting_enabled { + // A slightly hacky way to not report linting errors + // when we have done linting but then turned off the setting + lint.clear(); + } let files: HashSet<&PathBuf> = isolated.keys() .chain(device.keys()) .chain(lint.keys()) .collect(); - let config = self.config.lock().unwrap(); let direct_opens = self.direct_opens.lock().unwrap(); for file in files { let mut sorted_errors: Vec = @@ -570,12 +575,91 @@ impl InitActionContext { old_config: Config, out: &O) { trace!("Compilation info might have changed"); - let config = self.config.lock().unwrap(); - if config.compile_info_path != old_config.compile_info_path { - self.update_compilation_info(out); + enum LintReissueRequirement { + None, + ReReport, + AnalyzeMissing, + AnalyzeAll, + } + impl LintReissueRequirement { + fn upgrade_to(self, to: LintReissueRequirement) -> + LintReissueRequirement { + match (self, to) { + (Self::None, o) => o, + (o, Self::None) => o, + (Self::AnalyzeAll, _) => Self::AnalyzeAll, + (_, Self::AnalyzeAll) => Self::AnalyzeAll, + (Self::AnalyzeMissing, _) => Self::AnalyzeMissing, + (_, Self::AnalyzeMissing) => Self::AnalyzeMissing, + _ => Self::ReReport, + } + } + } + let mut lint_reissue = LintReissueRequirement::None; + { + let config = self.config.lock().unwrap(); + if config.compile_info_path != old_config.compile_info_path { + self.update_compilation_info(out); + } + if config.linting_enabled != old_config.linting_enabled { + if config.linting_enabled { + lint_reissue = lint_reissue.upgrade_to( + LintReissueRequirement::AnalyzeMissing); + } else { + lint_reissue = lint_reissue.upgrade_to( + LintReissueRequirement::ReReport); + } + } + if config.suppress_imports != old_config.suppress_imports { + if config.suppress_imports { + lint_reissue = lint_reissue.upgrade_to( + LintReissueRequirement::ReReport); + } else { + lint_reissue = lint_reissue.upgrade_to( + LintReissueRequirement::AnalyzeMissing); + } + } + if config.lint_direct_only != old_config.lint_direct_only { + if config.lint_direct_only { + lint_reissue = lint_reissue.upgrade_to( + LintReissueRequirement::ReReport); + } else { + lint_reissue = lint_reissue.upgrade_to( + LintReissueRequirement::AnalyzeMissing); + } + } + if config.lint_cfg_path != old_config.lint_cfg_path { + self.update_linter_config(out); + lint_reissue = LintReissueRequirement::AnalyzeAll; + } } - if config.lint_cfg_path != old_config.lint_cfg_path { - self.update_linter_config(out); + match lint_reissue { + LintReissueRequirement::None => (), + LintReissueRequirement::ReReport => + self.report_errors(out), + LintReissueRequirement::AnalyzeMissing => { + // Because lint_analyze re-locks self.analysis, we need + // to copy out the paths before iterating here + let paths: Vec<_> = { + let storage = self.analysis.lock().unwrap(); + storage.isolated_analysis.keys().filter( + |p|!storage.has_lint_analysis(p)).cloned().collect() + }; + for path in paths { + self.maybe_trigger_lint_analysis(path.as_path(), out); + } + self.report_errors(out); + }, + LintReissueRequirement::AnalyzeAll => { + let paths: Vec<_> = { + self.analysis.lock().unwrap() + .isolated_analysis.keys().cloned().collect() + }; + for path in paths { + self.maybe_trigger_lint_analysis(path.as_path(), out); + } + self.report_errors(out); + }, } } From 1518e9827a3aea9b6b196c38d94aefc4260babed Mon Sep 17 00:00:00 2001 From: Jonatan Waern Date: Wed, 28 May 2025 16:46:52 +0200 Subject: [PATCH 7/7] Update changelog Signed-off-by: Jonatan Waern --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d699978..424e988 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ ## 0.9.10 - Added indentation-style rules to the lint module with configurable indent size as specified in [example_lint_cfg.json](./example_files/example_lint_cfg.README) +- Configuration changes are now correctly tracked with the pull-model, and the + server should correctly update most settings without a restart on config + change. ## 0.9.9 - Fixed an issue that could cause analysis thread crash when an object was declared both