From edd193a1190f64c719abcd07f957a5360d10464f Mon Sep 17 00:00:00 2001 From: GreasySlug <9619abgoni@gmail.com> Date: Sun, 4 Feb 2024 23:51:45 +0900 Subject: [PATCH 1/2] refactor: wrap and handle the py ver retrieval --- build.rs | 4 +++- crates/erg_common/config.rs | 2 +- crates/erg_common/python_util.rs | 12 ++++++------ crates/erg_compiler/codegen.rs | 7 ++++++- tests/embed.rs | 4 ++-- tests/test.rs | 4 ++-- 6 files changed, 20 insertions(+), 13 deletions(-) diff --git a/build.rs b/build.rs index 96aa2273..ada05ea8 100644 --- a/build.rs +++ b/build.rs @@ -1,7 +1,9 @@ use erg_common::python_util::{env_magic_number, env_python_version}; fn main() -> std::io::Result<()> { - let version = env_python_version(); + let Some(version) = env_python_version() else { + panic!("Failed to get python version"); + }; if version.major != 3 { panic!("Python 3 is required"); } diff --git a/crates/erg_common/config.rs b/crates/erg_common/config.rs index 1407d238..254202a8 100644 --- a/crates/erg_common/config.rs +++ b/crates/erg_common/config.rs @@ -376,7 +376,7 @@ impl ErgConfig { .parse::() .expect("the value of `-py-command` is not a valid Python command"); cfg.py_magic_num = Some(detect_magic_number(&py_command)); - cfg.target_version = Some(get_python_version(&py_command)); + cfg.target_version = get_python_version(&py_command); cfg.py_command = Some(Box::leak(py_command.into_boxed_str())); } "--hex-py-magic-num" | "--hex-python-magic-number" => { diff --git a/crates/erg_common/python_util.rs b/crates/erg_common/python_util.rs index e3cd9ba3..a6dbf91d 100644 --- a/crates/erg_common/python_util.rs +++ b/crates/erg_common/python_util.rs @@ -693,7 +693,7 @@ impl std::str::FromStr for PythonVersion { } } -pub fn get_python_version(py_command: &str) -> PythonVersion { +pub fn get_python_version(py_command: &str) -> Option { let out = if cfg!(windows) { Command::new("cmd") .arg("/C") @@ -710,19 +710,19 @@ pub fn get_python_version(py_command: &str) -> PythonVersion { .expect("cannot get the python version") }; let s_version = String::from_utf8(out.stdout).unwrap(); - let mut iter = s_version.split(' '); - let mut iter = iter.nth(1).unwrap().split('.'); + let iter = s_version.split(' ').nth(1)?; + let mut iter = iter.split('.'); let major = iter.next().and_then(|i| i.parse().ok()).unwrap_or(3); let minor = iter.next().and_then(|i| i.parse().ok()); let micro = iter.next().and_then(|i| i.trim_end().parse().ok()); - PythonVersion { + Some(PythonVersion { major, minor, micro, - } + }) } -pub fn env_python_version() -> PythonVersion { +pub fn env_python_version() -> Option { get_python_version(&which_python()) } diff --git a/crates/erg_compiler/codegen.rs b/crates/erg_compiler/codegen.rs index dcda23fd..55859fa5 100644 --- a/crates/erg_compiler/codegen.rs +++ b/crates/erg_compiler/codegen.rs @@ -228,7 +228,12 @@ pub struct PyCodeGenerator { impl PyCodeGenerator { pub fn new(cfg: ErgConfig) -> Self { Self { - py_version: cfg.target_version.unwrap_or_else(env_python_version), + py_version: cfg.target_version.unwrap_or_else(|| { + let Some(version) = env_python_version() else { + panic!("Failed to get python version"); + }; + version + }), cfg, str_cache: CacheSet::new(), prelude_loaded: false, diff --git a/tests/embed.rs b/tests/embed.rs index 9a500ec9..ff870881 100644 --- a/tests/embed.rs +++ b/tests/embed.rs @@ -69,7 +69,7 @@ while! do! c < 7, do!: #[test] fn test_transpiler_embedding3() -> Result<(), ()> { - if env_python_version().minor < Some(10) { + if env_python_version().unwrap().minor < Some(10) { println!("skipped: {}", fn_name!()); return Ok(()); } @@ -96,7 +96,7 @@ print!(i, end:=\"\") #[test] fn test_transpiler_embedding4() -> Result<(), ()> { - if env_python_version().minor < Some(10) { + if env_python_version().unwrap().minor < Some(10) { println!("skipped: {}", fn_name!()); return Ok(()); } diff --git a/tests/test.rs b/tests/test.rs index 9cd4b9ba..da562e8e 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -172,7 +172,7 @@ fn exec_fib() -> Result<(), ()> { #[test] fn exec_helloworld() -> Result<(), ()> { // HACK: When running the test with Windows, the exit code is 1 (the cause is unknown) - if cfg!(windows) && env_python_version().minor >= Some(8) { + if cfg!(windows) && env_python_version().unwrap().minor >= Some(8) { expect_end_with("examples/helloworld.er", 0, 1) } else { expect_success("examples/helloworld.er", 0) @@ -323,7 +323,7 @@ fn exec_pattern() -> Result<(), ()> { #[test] fn exec_pyimport_test() -> Result<(), ()> { // HACK: When running the test with Windows, the exit code is 1 (the cause is unknown) - if cfg!(windows) && env_python_version().minor < Some(8) { + if cfg!(windows) && env_python_version().unwrap().minor < Some(8) { expect_end_with("tests/should_ok/pyimport.er", 2, 1) } else { expect_success("tests/should_ok/pyimport.er", 2) From 30f615f3909f0ea2d37d0068cf52143c6a26f9fb Mon Sep 17 00:00:00 2001 From: Shunsuke Shibayama Date: Tue, 6 Feb 2024 01:54:55 +0900 Subject: [PATCH 2/2] fix(els): eliminate `unwrap`s --- crates/els/channels.rs | 61 +++++++++++++++++++++--------------------- crates/els/rename.rs | 42 ++++++++++++++++------------- crates/els/server.rs | 28 ++++++++++++------- crates/els/util.rs | 4 ++- 4 files changed, 76 insertions(+), 59 deletions(-) diff --git a/crates/els/channels.rs b/crates/els/channels.rs index fca7f852..5672ae34 100644 --- a/crates/els/channels.rs +++ b/crates/els/channels.rs @@ -136,33 +136,27 @@ impl SendChannels { } pub(crate) fn close(&self) { - self.completion.send(WorkerMessage::Kill).unwrap(); - self.resolve_completion.send(WorkerMessage::Kill).unwrap(); - self.goto_definition.send(WorkerMessage::Kill).unwrap(); - self.semantic_tokens_full.send(WorkerMessage::Kill).unwrap(); - self.inlay_hint.send(WorkerMessage::Kill).unwrap(); - self.inlay_hint_resolve.send(WorkerMessage::Kill).unwrap(); - self.hover.send(WorkerMessage::Kill).unwrap(); - self.references.send(WorkerMessage::Kill).unwrap(); - self.code_lens.send(WorkerMessage::Kill).unwrap(); - self.code_action.send(WorkerMessage::Kill).unwrap(); - self.code_action_resolve.send(WorkerMessage::Kill).unwrap(); - self.signature_help.send(WorkerMessage::Kill).unwrap(); - self.will_rename_files.send(WorkerMessage::Kill).unwrap(); - self.execute_command.send(WorkerMessage::Kill).unwrap(); - self.workspace_symbol.send(WorkerMessage::Kill).unwrap(); - self.document_symbol.send(WorkerMessage::Kill).unwrap(); - self.call_hierarchy_prepare - .send(WorkerMessage::Kill) - .unwrap(); - self.call_hierarchy_incoming - .send(WorkerMessage::Kill) - .unwrap(); - self.call_hierarchy_outgoing - .send(WorkerMessage::Kill) - .unwrap(); - self.folding_range.send(WorkerMessage::Kill).unwrap(); - self.health_check.send(WorkerMessage::Kill).unwrap(); + let _ = self.completion.send(WorkerMessage::Kill); + let _ = self.resolve_completion.send(WorkerMessage::Kill); + let _ = self.goto_definition.send(WorkerMessage::Kill); + let _ = self.semantic_tokens_full.send(WorkerMessage::Kill); + let _ = self.inlay_hint.send(WorkerMessage::Kill); + let _ = self.inlay_hint_resolve.send(WorkerMessage::Kill); + let _ = self.hover.send(WorkerMessage::Kill); + let _ = self.references.send(WorkerMessage::Kill); + let _ = self.code_lens.send(WorkerMessage::Kill); + let _ = self.code_action.send(WorkerMessage::Kill); + let _ = self.code_action_resolve.send(WorkerMessage::Kill); + let _ = self.signature_help.send(WorkerMessage::Kill); + let _ = self.will_rename_files.send(WorkerMessage::Kill); + let _ = self.execute_command.send(WorkerMessage::Kill); + let _ = self.workspace_symbol.send(WorkerMessage::Kill); + let _ = self.document_symbol.send(WorkerMessage::Kill); + let _ = self.call_hierarchy_prepare.send(WorkerMessage::Kill); + let _ = self.call_hierarchy_incoming.send(WorkerMessage::Kill); + let _ = self.call_hierarchy_outgoing.send(WorkerMessage::Kill); + let _ = self.folding_range.send(WorkerMessage::Kill); + let _ = self.health_check.send(WorkerMessage::Kill); } } @@ -195,7 +189,11 @@ pub struct ReceiveChannels { } pub trait Sendable { - fn send(&self, id: i64, params: R::Params); + fn send( + &self, + id: i64, + params: R::Params, + ) -> Result<(), mpsc::SendError>>; } macro_rules! impl_sendable { @@ -203,13 +201,16 @@ macro_rules! impl_sendable { impl Sendable<$Request> for Server { - fn send(&self, id: i64, params: $Params) { + fn send( + &self, + id: i64, + params: $Params, + ) -> Result<(), mpsc::SendError>> { self.channels .as_ref() .unwrap() .$receiver .send($crate::channels::WorkerMessage::Request(id, params)) - .unwrap(); } } }; diff --git a/crates/els/rename.rs b/crates/els/rename.rs index 17fd0b06..0fb2e80e 100644 --- a/crates/els/rename.rs +++ b/crates/els/rename.rs @@ -29,6 +29,7 @@ use crate::util::{self, NormalizedUrl}; impl Server { pub(crate) fn rename(&mut self, msg: &Value) -> ELSResult<()> { let params = RenameParams::deserialize(&msg["params"])?; + let id = msg["id"].as_i64().unwrap(); self.send_log(format!("rename request: {params:?}"))?; let uri = NormalizedUrl::new(params.text_document_position.text_document.uri); let pos = params.text_document_position.position; @@ -66,9 +67,7 @@ impl Server { _ => format!("this {kind} cannot be renamed"), }; let edit = WorkspaceEdit::new(changes); - self.send_stdout( - &json!({ "jsonrpc": "2.0", "id": msg["id"].as_i64().unwrap(), "result": edit }), - )?; + self.send_stdout(&json!({ "jsonrpc": "2.0", "id": id, "result": edit }))?; return self.send_error_info(error_reason); } Self::commit_change(&mut changes, &vi.def_loc, params.new_name.clone()); @@ -88,9 +87,7 @@ impl Server { } let timestamps = self.get_timestamps(changes.keys()); let edit = WorkspaceEdit::new(changes); - self.send_stdout( - &json!({ "jsonrpc": "2.0", "id": msg["id"].as_i64().unwrap(), "result": edit }), - )?; + self.send_stdout(&json!({ "jsonrpc": "2.0", "id": id, "result": edit }))?; for _ in 0..20 { self.send_log("waiting for file to be modified...")?; if self.all_changed(×tamps) { @@ -108,9 +105,7 @@ impl Server { return Ok(()); } } - self.send_stdout( - &json!({ "jsonrpc": "2.0", "id": msg["id"].as_i64().unwrap(), "result": Value::Null }), - ) + self.send_stdout(&json!({ "jsonrpc": "2.0", "id": id, "result": Value::Null })) } fn commit_change( @@ -169,7 +164,11 @@ impl Server { .ref_inner() .iter() .filter(|node| node.id == path || self_node.depends_on(&node.id)) - .map(|node| NormalizedUrl::new(Url::from_file_path(node.id.to_path_buf()).unwrap())) + .filter_map(|node| { + Some(NormalizedUrl::new( + Url::from_file_path(node.id.to_path_buf()).ok()?, + )) + }) .collect() } @@ -181,7 +180,11 @@ impl Server { .ref_inner() .iter() .filter(|node| node.depends_on(&path)) - .map(|node| NormalizedUrl::new(Url::from_file_path(node.id.to_path_buf()).unwrap())) + .filter_map(|node| { + Some(NormalizedUrl::new( + Url::from_file_path(node.id.to_path_buf()).ok()?, + )) + }) .collect() } } @@ -192,26 +195,29 @@ impl Server { old_uri: &NormalizedUrl, new_uri: &NormalizedUrl, ) -> HashMap> { + let mut changes = HashMap::new(); let old_path = util::uri_to_path(old_uri) .file_stem() - .unwrap() + .unwrap_or_default() .to_string_lossy() .to_string(); let old_path = old_path.trim_end_matches(".d"); let new_path = util::uri_to_path(new_uri) .file_stem() - .unwrap() + .unwrap_or_default() .to_string_lossy() .to_string(); + if old_path.is_empty() || new_path.is_empty() { + return changes; + } let new_path = new_path.trim_end_matches(".d"); - let mut changes = HashMap::new(); for dep in self.dependents_of(old_uri) { let imports = self.search_imports(&dep, old_path); - let edits = imports.iter().map(|lit| { - TextEdit::new( - util::loc_to_range(lit.loc()).unwrap(), + let edits = imports.iter().filter_map(|lit| { + Some(TextEdit::new( + util::loc_to_range(lit.loc())?, lit.token.content.replace(old_path, new_path), - ) + )) }); changes.insert(dep.raw(), edits.collect()); } diff --git a/crates/els/server.rs b/crates/els/server.rs index 0dcdedf0..b03bd8c7 100644 --- a/crates/els/server.rs +++ b/crates/els/server.rs @@ -43,12 +43,12 @@ use lsp_types::request::{ use lsp_types::{ CallHierarchyServerCapability, CodeActionKind, CodeActionOptions, CodeActionProviderCapability, CodeLensOptions, CompletionOptions, ConfigurationItem, ConfigurationParams, - DidChangeTextDocumentParams, DidOpenTextDocumentParams, ExecuteCommandOptions, - FoldingRangeProviderCapability, HoverProviderCapability, ImplementationProviderCapability, - InitializeParams, InitializeResult, InlayHintOptions, InlayHintServerCapabilities, OneOf, - Position, SemanticTokenType, SemanticTokensFullOptions, SemanticTokensLegend, - SemanticTokensOptions, SemanticTokensServerCapabilities, ServerCapabilities, - SignatureHelpOptions, WorkDoneProgressOptions, + DidChangeTextDocumentParams, DidOpenTextDocumentParams, DidSaveTextDocumentParams, + ExecuteCommandOptions, FoldingRangeProviderCapability, HoverProviderCapability, + ImplementationProviderCapability, InitializeParams, InitializeResult, InlayHintOptions, + InlayHintServerCapabilities, OneOf, Position, SemanticTokenType, SemanticTokensFullOptions, + SemanticTokensLegend, SemanticTokensOptions, SemanticTokensServerCapabilities, + ServerCapabilities, SignatureHelpOptions, WorkDoneProgressOptions, }; use serde::{Deserialize, Serialize}; @@ -325,6 +325,9 @@ impl Server { let msg = _self.read_message().unwrap(); if let Err(err) = _self.dispatch(msg) { lsp_log!("error: {err}"); + if err.to_string().contains("sending on a closed channel") { + _self.restart(); + }; } }, fn_name!(), @@ -333,7 +336,7 @@ impl Server { // recover from crash if handle.is_finished() { self.send_error_info("The compiler has crashed. Restarting...") - .unwrap(); + .expect("failed to send error info to client"); self.restart(); let mut _self = self.clone(); handle = spawn_new_thread( @@ -341,6 +344,9 @@ impl Server { let msg = _self.read_message().unwrap(); if let Err(err) = _self.dispatch(msg) { lsp_log!("error: {err}"); + if err.to_string().contains("sending on a closed channel") { + _self.restart(); + }; } }, fn_name!(), @@ -600,8 +606,10 @@ impl Server { })) } + /// Restart the server. Clear caches and close & reopen channels. #[allow(unused)] pub(crate) fn restart(&mut self) { + lsp_log!("restarting ELS"); self.file_cache.clear(); self.comp_cache.clear(); self.channels.as_ref().unwrap().close(); @@ -696,7 +704,7 @@ impl Server { Server: Sendable, { let params = R::Params::deserialize(&msg["params"])?; - self.send(id, params); + self.send(id, params)?; Ok(()) } @@ -801,8 +809,8 @@ impl Server { self.check_file(uri, code) } "textDocument/didSave" => { - let uri = - NormalizedUrl::parse(msg["params"]["textDocument"]["uri"].as_str().unwrap())?; + let params = DidSaveTextDocumentParams::deserialize(msg["params"].clone())?; + let uri = NormalizedUrl::new(params.text_document.uri); self.send_log(format!("{method}: {uri}"))?; let code = self.file_cache.get_entire_code(&uri)?; self.recheck_file(uri, code) diff --git a/crates/els/util.rs b/crates/els/util.rs index 5b303538..e01a2c89 100644 --- a/crates/els/util.rs +++ b/crates/els/util.rs @@ -163,7 +163,9 @@ pub(crate) fn get_token_from_stream( } pub(crate) fn get_metadata_from_uri(uri: &Url) -> ELSResult { - let path = uri.to_file_path().unwrap(); + let path = uri + .to_file_path() + .map_err(|_| "failed to convert uri to path")?; Ok(metadata(path)?) }