diff --git a/lsp/lsp-harness/src/jsonrpc.rs b/lsp/lsp-harness/src/jsonrpc.rs index a2a68d06db..bc11942d25 100644 --- a/lsp/lsp-harness/src/jsonrpc.rs +++ b/lsp/lsp-harness/src/jsonrpc.rs @@ -90,11 +90,11 @@ pub struct Response { } impl Server { - /// Launch a language server by running the given command. - /// - /// The command's stdin and stdout will be overridden to "piped" (because - /// that's what LSes do). - pub fn new(mut cmd: std::process::Command) -> Result { + /// Similar to `new`, but allows passing custom stuff + pub fn new_with_options( + mut cmd: std::process::Command, + initialization_options: Option, + ) -> Result { let lsp = cmd.stdin(Stdio::piped()).stdout(Stdio::piped()).spawn()?; let mut lsp = Server { @@ -104,11 +104,19 @@ impl Server { id: 0, }; - lsp.initialize()?; + lsp.initialize(initialization_options)?; Ok(lsp) } + /// Launch a language server by running the given command. + /// + /// The command's stdin and stdout will be overridden to "piped" (because + /// that's what LSes do). + pub fn new(cmd: std::process::Command) -> Result { + Server::new_with_options(cmd, None) + } + /// Make the language server aware of a file. pub fn send_file(&mut self, uri: Url, contents: &str) -> Result<()> { self.send_notification::(DidOpenTextDocumentParams { @@ -151,7 +159,7 @@ impl Server { self.send_notification::(()) } - fn initialize(&mut self) -> Result<()> { + fn initialize(&mut self, initialization_options: Option) -> Result<()> { // `root_path` is deprecated, but we need ot initialize the struct // somehow. There is no `Default` implementation for `InitilizeParams` // in versions of `lsp-types` compatible with `codespan-lsp` @@ -160,7 +168,7 @@ impl Server { process_id: None, root_path: None, root_uri: None, - initialization_options: None, + initialization_options, capabilities: ClientCapabilities::default(), trace: None, workspace_folders: None, diff --git a/lsp/lsp-harness/src/lib.rs b/lsp/lsp-harness/src/lib.rs index 954d21df1d..5c99a57fff 100644 --- a/lsp/lsp-harness/src/lib.rs +++ b/lsp/lsp-harness/src/lib.rs @@ -126,14 +126,17 @@ impl Default for TestHarness { } impl TestHarness { - pub fn new() -> Self { + pub fn new_with_options(initialization_options: Option) -> Self { let cmd = std::process::Command::cargo_bin("nls").unwrap(); - let srv = Server::new(cmd).unwrap(); + let srv = Server::new_with_options(cmd, initialization_options).unwrap(); Self { srv, out: Vec::new(), } } + pub fn new() -> Self { + Self::new_with_options(None) + } pub fn request(&mut self, params: T::Params) where diff --git a/lsp/nls/src/background.rs b/lsp/nls/src/background.rs index 3beacacd9b..1a7b10a351 100644 --- a/lsp/nls/src/background.rs +++ b/lsp/nls/src/background.rs @@ -16,14 +16,12 @@ use nickel_lang_core::{ use serde::{Deserialize, Serialize}; use crate::{ - cache::CacheExt as _, diagnostic::SerializableDiagnostic, files::uri_to_path, world::World, + cache::CacheExt as _, config, diagnostic::SerializableDiagnostic, files::uri_to_path, + world::World, }; -const EVAL_TIMEOUT: Duration = Duration::from_secs(1); -const RECURSION_LIMIT: usize = 128; -// The duration during which a file causing the evaluator to timeout will be blacklisted from further -// evaluations -const BLACKLIST_DURATION: Duration = Duration::from_secs(30); +// Environment variable used to pass the recursion limit value to the child worker +const RECURSION_LIMIT_ENV_VAR_NAME: &str = "NICKEL_NLS_RECURSION_LIMIT"; #[derive(Debug, Serialize, Deserialize)] enum Command { @@ -106,7 +104,8 @@ pub fn worker_main() -> anyhow::Result<()> { // We've already checked that parsing and typechecking are successful, so we // don't expect further errors. let rt = vm.prepare_eval(file_id).unwrap(); - let errors = vm.eval_permissive(rt, RECURSION_LIMIT); + let recursion_limit = std::env::var(RECURSION_LIMIT_ENV_VAR_NAME)?.parse::()?; + let errors = vm.eval_permissive(rt, recursion_limit); diagnostics.extend( errors .into_iter() @@ -142,12 +141,18 @@ struct SupervisorState { eval_stack: Vec, // If evaluating a file causes the worker to time out or crash, we blacklist that file - // and refuse to evaluate it for `BLACKLIST_DURATION` + // and refuse to evaluate it for `self.config.blacklist_duration` banned_files: HashMap, + + config: config::LspEvalConfig, } impl SupervisorState { - fn new(cmd_rx: Receiver, response_tx: Sender) -> anyhow::Result { + fn new( + cmd_rx: Receiver, + response_tx: Sender, + config: config::LspEvalConfig, + ) -> anyhow::Result { Ok(Self { cmd_rx, response_tx, @@ -155,6 +160,7 @@ impl SupervisorState { deps: HashMap::new(), banned_files: HashMap::new(), eval_stack: Vec::new(), + config, }) } @@ -182,6 +188,10 @@ impl SupervisorState { fn eval(&self, uri: &Url) -> anyhow::Result { let path = std::env::current_exe()?; let mut child = std::process::Command::new(path) + .env( + RECURSION_LIMIT_ENV_VAR_NAME, + self.config.eval_limits.recursion_limit.to_string(), + ) .arg("--background-eval") .stdout(std::process::Stdio::piped()) .stdin(std::process::Stdio::piped()) @@ -214,7 +224,10 @@ impl SupervisorState { }; bincode::serialize_into(&mut tx, &eval)?; - let result = run_with_timeout(move || bincode::deserialize_from(rx), EVAL_TIMEOUT); + let result = run_with_timeout( + move || bincode::deserialize_from(rx), + self.config.eval_limits.timeout, + ); Ok(result??) } @@ -230,7 +243,8 @@ impl SupervisorState { } Command::EvalFile { uri } => { match self.banned_files.get(&uri) { - Some(blacklist_time) if blacklist_time.elapsed() < BLACKLIST_DURATION => {} + Some(blacklist_time) + if blacklist_time.elapsed() < self.config.blacklist_duration => {} _ => { // If we re-request an evaluation, remove the old one. (This is quadratic in the // size of the eval stack, but it only contains unique entries so we don't expect it @@ -285,10 +299,10 @@ impl SupervisorState { } impl BackgroundJobs { - pub fn new() -> Self { + pub fn new(config: config::LspEvalConfig) -> Self { let (cmd_tx, cmd_rx) = crossbeam::channel::unbounded(); let (diag_tx, diag_rx) = crossbeam::channel::unbounded(); - match SupervisorState::new(cmd_rx, diag_tx) { + match SupervisorState::new(cmd_rx, diag_tx, config) { Ok(mut sup) => { std::thread::spawn(move || { sup.run(); diff --git a/lsp/nls/src/config.rs b/lsp/nls/src/config.rs new file mode 100644 index 0000000000..47ae77f0f3 --- /dev/null +++ b/lsp/nls/src/config.rs @@ -0,0 +1,52 @@ +//! Configuration for the Nickel Language Server +use serde::{Deserialize, Serialize}; + +use std::time::Duration; + +/** +Limits to appy to the LSP background evaluator. +If an evaluation reaches one of these limits, it will be canceled and the offending file will be +temporarily blacklisted. +*/ +#[derive(Debug, Deserialize, Serialize)] +#[serde(default)] +pub struct LspEvalLimits { + /// Time out at which to cancel the background evaluation + pub timeout: Duration, + /// The maximum recursion level to allow in the background evaluator + pub recursion_limit: usize, +} +impl Default for LspEvalLimits { + fn default() -> Self { + LspEvalLimits { + timeout: Duration::from_secs(1), + recursion_limit: 128, + } + } +} + +/// The configuration of the LSP evaluator +#[derive(Debug, Deserialize, Serialize)] +#[serde(default)] +pub struct LspEvalConfig { + pub eval_limits: LspEvalLimits, + /// The duration during which a file that broke the background evaluator will be blacklisted + /// from it + pub blacklist_duration: Duration, +} + +impl Default for LspEvalConfig { + fn default() -> Self { + LspEvalConfig { + eval_limits: Default::default(), + blacklist_duration: Duration::from_secs(30), + } + } +} + +#[derive(Debug, Deserialize, Serialize, Default)] +#[serde(default)] +pub struct LspConfig { + /// Configuration for the background evaluator in the LSP + pub eval_config: LspEvalConfig, +} diff --git a/lsp/nls/src/main.rs b/lsp/nls/src/main.rs index 4908d28399..39380d03a3 100644 --- a/lsp/nls/src/main.rs +++ b/lsp/nls/src/main.rs @@ -12,6 +12,7 @@ mod background; mod cache; mod codespan_lsp; mod command; +mod config; mod diagnostic; mod error; mod field_walker; @@ -29,7 +30,7 @@ mod usage; mod utils; mod world; -use crate::trace::Trace; +use crate::{config::LspConfig, trace::Trace}; #[derive(clap::Parser, Debug)] /// The language server of the Nickel language. @@ -83,9 +84,17 @@ fn main() -> Result<()> { let capabilities = Server::capabilities(); - connection.initialize(serde_json::to_value(capabilities)?)?; + let initialize_params = connection.initialize(serde_json::to_value(capabilities)?)?; - let _server = Server::new(connection).run(); + debug!("Raw InitializeParams: {:?}", initialize_params); + let config = match initialize_params.get("initializationOptions") { + Some(opts) => serde_json::from_value::(opts.clone())?, + None => LspConfig::default(), + }; + + debug!("Parsed InitializeParams: {:?}", config); + + let _server = Server::new(connection, config).run(); Ok(()) } diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index 0ef2297517..7bf9ae98e0 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -19,6 +19,7 @@ use crate::{ actions, background::BackgroundJobs, command, + config::LspConfig, requests::{completion, formatting, goto, hover, rename, symbols}, trace::Trace, world::World, @@ -73,11 +74,11 @@ impl Server { } } - pub fn new(connection: Connection) -> Server { + pub fn new(connection: Connection, config: LspConfig) -> Server { Server { connection, world: World::default(), - background_jobs: BackgroundJobs::new(), + background_jobs: BackgroundJobs::new(config.eval_config), } } diff --git a/lsp/nls/tests/main.rs b/lsp/nls/tests/main.rs index 442245f717..173610e4ee 100644 --- a/lsp/nls/tests/main.rs +++ b/lsp/nls/tests/main.rs @@ -1,4 +1,6 @@ use nickel_lang_utils::project_root::project_root; +use pretty_assertions::assert_eq; +use serde_json::json; use test_generator::test_resources; use lsp_harness::{TestFixture, TestHarness}; @@ -111,3 +113,32 @@ fn reload_broken_imports() { } } } + +#[test] +fn apply_client_options() { + let _ = env_logger::try_init(); + let lsp_options = json!({ + "eval_config": { + "eval_limits": { + "recursion_limit": 1 + } + } + }); + let mut harness = TestHarness::new_with_options(Some(lsp_options)); + let url = |s: &str| lsp_types::Url::from_file_path(s).unwrap(); + harness.send_file( + url("/test.ncl"), + "{ C = fun n => if n == 0 then String else C (n - 1), res = 2 | C 5 }", + ); + + // Typecheck diagnostics. Empty because there's nothing to error on + let diags = harness.wait_for_diagnostics(); + assert!(diags.diagnostics.is_empty()); + + // Evaluator diagnostics. + // These shouldn't be empty (because `C 5 == String` so `2 | C 5` is a contract + // violation), but they are because `recursion_limit` is too low for the evaluator to be able + // to compute that. + let diags = harness.wait_for_diagnostics(); + assert!(diags.diagnostics.is_empty()); +}