chore: remove duplicated code from LSP (#5116)

# Description ## Problem\* Resolves  ## Summary\* We've got some copy pasted code here so I've added a helper function for converting a "position" to a "location". ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher <[email protected]>
noir-lang · May 28, 2024 · f03f8ae · f03f8ae
1 parent 7d8c0a3
commit f03f8ae
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 58 deletions.
diff --git a/tooling/lsp/src/requests/goto_declaration.rs b/tooling/lsp/src/requests/goto_declaration.rs
@@ -5,12 +5,13 @@ use crate::LspState;
 use crate::{parse_diff, resolve_workspace_for_source_path};
 use async_lsp::{ErrorCode, ResponseError};
 
+use fm::PathString;
 use lsp_types::request::{GotoDeclarationParams, GotoDeclarationResponse};
 
 use nargo::insert_all_files_for_workspace_into_file_manager;
 use noirc_driver::file_manager_with_stdlib;
 
-use super::{position_to_byte_index, to_lsp_location};
+use super::{position_to_location, to_lsp_location};
 
 pub(crate) fn on_goto_declaration_request(
     state: &mut LspState,
@@ -32,49 +33,32 @@ fn on_goto_definition_inner(
     let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap();
     let package = workspace.members.first().unwrap();
 
-    let package_root_path: String = package.root_dir.as_os_str().to_string_lossy().into();
-
     let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
     insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
     let parsed_files = parse_diff(&workspace_file_manager, state);
 
     let (mut context, crate_id) =
         nargo::prepare_package(&workspace_file_manager, &parsed_files, package);
 
-    let interner;
-    if let Some(def_interner) = state.cached_definitions.get(&package_root_path) {
-        interner = def_interner;
+    let package_root_path = package.root_dir.as_os_str().to_string_lossy().into_owned();
+    let interner = if let Some(def_interner) = state.cached_definitions.get(&package_root_path) {
+        def_interner
     } else {
         // We ignore the warnings and errors produced by compilation while resolving the definition
         let _ = noirc_driver::check_crate(&mut context, crate_id, false, false, false);
-        interner = &context.def_interner;
-    }
-
-    let files = context.file_manager.as_file_map();
-    let file_id = context.file_manager.name_to_id(file_path.clone()).ok_or(ResponseError::new(
-        ErrorCode::REQUEST_FAILED,
-        format!("Could not find file in file manager. File path: {:?}", file_path),
-    ))?;
-    let byte_index =
-        position_to_byte_index(files, file_id, &params.text_document_position_params.position)
-            .map_err(|err| {
-                ResponseError::new(
-                    ErrorCode::REQUEST_FAILED,
-                    format!("Could not convert position to byte index. Error: {:?}", err),
-                )
-            })?;
-
-    let search_for_location = noirc_errors::Location {
-        file: file_id,
-        span: noirc_errors::Span::single_char(byte_index as u32),
+        &context.def_interner
     };
 
+    let files = workspace_file_manager.as_file_map();
+    let file_path = PathString::from(file_path);
+    let search_for_location =
+        position_to_location(files, &file_path, &params.text_document_position_params.position)?;
+
     let goto_declaration_response =
         interner.get_declaration_location_from(search_for_location).and_then(|found_location| {
             let file_id = found_location.file;
             let definition_position = to_lsp_location(files, file_id, found_location.span)?;
-            let response: GotoDeclarationResponse =
-                GotoDeclarationResponse::from(definition_position).to_owned();
+            let response = GotoDeclarationResponse::from(definition_position).to_owned();
             Some(response)
         });
 

diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs
@@ -4,12 +4,13 @@ use crate::{parse_diff, resolve_workspace_for_source_path};
 use crate::{types::GotoDefinitionResult, LspState};
 use async_lsp::{ErrorCode, ResponseError};
 
+use fm::PathString;
 use lsp_types::request::GotoTypeDefinitionParams;
 use lsp_types::{GotoDefinitionParams, GotoDefinitionResponse};
 use nargo::insert_all_files_for_workspace_into_file_manager;
 use noirc_driver::file_manager_with_stdlib;
 
-use super::{position_to_byte_index, to_lsp_location};
+use super::{position_to_location, to_lsp_location};
 
 pub(crate) fn on_goto_definition_request(
     state: &mut LspState,
@@ -40,50 +41,33 @@ fn on_goto_definition_inner(
     let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap();
     let package = workspace.members.first().unwrap();
 
-    let package_root_path: String = package.root_dir.as_os_str().to_string_lossy().into();
-
     let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
     insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
     let parsed_files = parse_diff(&workspace_file_manager, state);
 
     let (mut context, crate_id) =
         nargo::prepare_package(&workspace_file_manager, &parsed_files, package);
 
-    let interner;
-    if let Some(def_interner) = state.cached_definitions.get(&package_root_path) {
-        interner = def_interner;
+    let package_root_path = package.root_dir.as_os_str().to_string_lossy().into_owned();
+    let interner = if let Some(def_interner) = state.cached_definitions.get(&package_root_path) {
+        def_interner
     } else {
         // We ignore the warnings and errors produced by compilation while resolving the definition
         let _ = noirc_driver::check_crate(&mut context, crate_id, false, false, false);
-        interner = &context.def_interner;
-    }
-
-    let files = context.file_manager.as_file_map();
-    let file_id = context.file_manager.name_to_id(file_path.clone()).ok_or(ResponseError::new(
-        ErrorCode::REQUEST_FAILED,
-        format!("Could not find file in file manager. File path: {:?}", file_path),
-    ))?;
-    let byte_index =
-        position_to_byte_index(files, file_id, &params.text_document_position_params.position)
-            .map_err(|err| {
-                ResponseError::new(
-                    ErrorCode::REQUEST_FAILED,
-                    format!("Could not convert position to byte index. Error: {:?}", err),
-                )
-            })?;
-
-    let search_for_location = noirc_errors::Location {
-        file: file_id,
-        span: noirc_errors::Span::single_char(byte_index as u32),
+        &context.def_interner
     };
 
+    let files = workspace_file_manager.as_file_map();
+    let file_path = PathString::from(file_path);
+    let search_for_location =
+        position_to_location(files, &file_path, &params.text_document_position_params.position)?;
+
     let goto_definition_response = interner
         .get_definition_location_from(search_for_location, return_type_location_instead)
         .and_then(|found_location| {
             let file_id = found_location.file;
             let definition_position = to_lsp_location(files, file_id, found_location.span)?;
-            let response: GotoDefinitionResponse =
-                GotoDefinitionResponse::from(definition_position).to_owned();
+            let response = GotoDefinitionResponse::from(definition_position).to_owned();
             Some(response)
         });
 

diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs
@@ -1,8 +1,8 @@
 use std::future::Future;
 
 use crate::types::{CodeLensOptions, InitializeParams};
-use async_lsp::ResponseError;
-use fm::codespan_files::Error;
+use async_lsp::{ErrorCode, ResponseError};
+use fm::{codespan_files::Error, FileMap, PathString};
 use lsp_types::{
     DeclarationCapability, Location, Position, TextDocumentSyncCapability, TextDocumentSyncKind,
     TypeDefinitionProviderCapability, Url,
@@ -171,6 +171,30 @@ where
     }
 }
 
+fn position_to_location(
+    files: &FileMap,
+    file_path: &PathString,
+    position: &Position,
+) -> Result<noirc_errors::Location, ResponseError> {
+    let file_id = files.get_file_id(file_path).ok_or(ResponseError::new(
+        ErrorCode::REQUEST_FAILED,
+        format!("Could not find file in file manager. File path: {:?}", file_path),
+    ))?;
+    let byte_index = position_to_byte_index(files, file_id, position).map_err(|err| {
+        ResponseError::new(
+            ErrorCode::REQUEST_FAILED,
+            format!("Could not convert position to byte index. Error: {:?}", err),
+        )
+    })?;
+
+    let location = noirc_errors::Location {
+        file: file_id,
+        span: noirc_errors::Span::single_char(byte_index as u32),
+    };
+
+    Ok(location)
+}
+
 fn character_to_line_offset(line: &str, character: u32) -> Result<usize, Error> {
     let line_len = line.len();
     let mut character_offset = 0;