Make the LSP configurable (#1974)

* Make the LSP options configurable

Take into account the configuration sent by the client through
[`InitializeParams.initializationOptions`](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initializeParams).

This is currently used to set the limits for the background evaluation,
although it also creates the infrastructure for setting more things.

* Fix the doc comments for the config

* Remove a useless explicit Default instance

Replace it by the derived one (thanks clippy)

* Comment fixes

Co-Authored-By: Yann Hamdaoui <yann.hamdaoui@tweag.io>

* Simplify the nls config definition

`#[serde(default)]` applied to a whole struct is enough for it to do the
right thing with the `Default` trait. Makes the module significantly
nicer to read.

---------

Co-authored-by: Yann Hamdaoui <yann.hamdaoui@tweag.io>

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<Server> {
+    /// Similar to `new`, but allows passing custom stuff
+    pub fn new_with_options(
+        mut cmd: std::process::Command,
+        initialization_options: Option<serde_json::Value>,
+    ) -> Result<Server> {
         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> {
+        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::<DidOpenTextDocument>(DidOpenTextDocumentParams {
@@ -151,7 +159,7 @@ impl Server {
         self.send_notification::<Exit>(())
     }
 
-    fn initialize(&mut self) -> Result<()> {
+    fn initialize(&mut self, initialization_options: Option<serde_json::Value>) -> 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,

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<serde_json::Value>) -> 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<T: LspRequest>(&mut self, params: T::Params)
     where

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::<usize>()?;
+            let errors = vm.eval_permissive(rt, recursion_limit);
             diagnostics.extend(
                 errors
                     .into_iter()
@@ -142,19 +141,26 @@ struct SupervisorState {
     eval_stack: Vec<Url>,
 
     // 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<Url, Instant>,
+
+    config: config::LspEvalConfig,
 }
 
 impl SupervisorState {
-    fn new(cmd_rx: Receiver<Command>, response_tx: Sender<Diagnostics>) -> anyhow::Result<Self> {
+    fn new(
+        cmd_rx: Receiver<Command>,
+        response_tx: Sender<Diagnostics>,
+        config: config::LspEvalConfig,
+    ) -> anyhow::Result<Self> {
         Ok(Self {
             cmd_rx,
             response_tx,
             contents: HashMap::new(),
             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<Diagnostics> {
         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();

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,
+}

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::<LspConfig>(opts.clone())?,
+        None => LspConfig::default(),
+    };
+
+    debug!("Parsed InitializeParams: {:?}", config);
+
+    let _server = Server::new(connection, config).run();
 
     Ok(())
 }

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),
         }
     }
 

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());
+}