From 4de9c7b23804d07ab917ac427ae1ef8e4ea24593 Mon Sep 17 00:00:00 2001 From: Adam Welc Date: Tue, 13 Feb 2024 16:50:26 -0800 Subject: [PATCH] [move-ide] Fixed module id name construction (#16220) ## Description This PR fixes a problem related to module ID strings between parsed AST and typing AST not being the same. In the typing AST name, if the module has both the package address and the package name, the ID string will be `(pkg_name=pkg_addres)::mod_name` whereas the parsing AST it only has the name (as address is not resolved). When using module ID as a map key this causes discrepancy - module info inserted with one ID is unavailable when queried with the other ID. This PR unifies both IDs. This PR also adds back parallelism accidentally removed in https://github.com/MystenLabs/sui/pull/16178 (move-analyzer does not work without this as the requests are served in separate threads). ## Test Plan Tried to create a unit test for the module ID string fix but was not successful. Nevertheless, without this fix, opening `sui/sui_programmability/examples/capy/` in the IDE crashes move-analyzer and with this fix everything works correctly. --- .../move-analyzer/src/bin/move-analyzer.rs | 19 +- .../move/crates/move-analyzer/src/symbols.rs | 197 +++++++++++------- .../tests/mod-ident-uniform/Move.toml | 10 + .../tests/mod-ident-uniform/sources/M1.move | 2 + 4 files changed, 135 insertions(+), 93 deletions(-) create mode 100644 external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/Move.toml create mode 100644 external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/sources/M1.move diff --git a/external-crates/move/crates/move-analyzer/src/bin/move-analyzer.rs b/external-crates/move/crates/move-analyzer/src/bin/move-analyzer.rs index 39f67275714c4..1a3b0e51b5047 100644 --- a/external-crates/move/crates/move-analyzer/src/bin/move-analyzer.rs +++ b/external-crates/move/crates/move-analyzer/src/bin/move-analyzer.rs @@ -15,7 +15,6 @@ use std::{ collections::BTreeMap, path::PathBuf, sync::{Arc, Mutex}, - thread, }; use move_analyzer::{ @@ -135,20 +134,10 @@ fn main() { // to be available right after the client is initialized. if let Some(uri) = initialize_params.root_uri { if let Some(p) = symbols::SymbolicatorRunner::root_dir(&uri.to_file_path().unwrap()) { - // need to evaluate in a separate thread to allow for a larger stack size (needed on - // Windows) - thread::Builder::new() - .stack_size(symbols::STACK_SIZE_BYTES) - .spawn(move || { - if let Ok((Some(new_symbols), _)) = symbols::get_symbols(p.as_path(), lint) - { - let mut old_symbols = symbols.lock().unwrap(); - (*old_symbols).merge(new_symbols); - } - }) - .unwrap() - .join() - .unwrap(); + if let Ok((Some(new_symbols), _)) = symbols::get_symbols(p.as_path(), lint) { + let mut old_symbols = symbols.lock().unwrap(); + (*old_symbols).merge(new_symbols); + } } } }; diff --git a/external-crates/move/crates/move-analyzer/src/symbols.rs b/external-crates/move/crates/move-analyzer/src/symbols.rs index 76f7a3039bba9..baa580a3b5529 100644 --- a/external-crates/move/crates/move-analyzer/src/symbols.rs +++ b/external-crates/move/crates/move-analyzer/src/symbols.rs @@ -77,6 +77,7 @@ use std::{ fmt, path::{Path, PathBuf}, sync::{Arc, Condvar, Mutex}, + thread, }; use tempfile::tempdir; use url::Url; @@ -84,7 +85,7 @@ use url::Url; use move_command_line_common::files::FileHash; use move_compiler::{ editions::Flavor, - expansion::ast::{Fields, ModuleIdent, ModuleIdent_, Value, Value_, Visibility}, + expansion::ast::{self as E, Fields, ModuleIdent, ModuleIdent_, Value, Value_, Visibility}, naming::ast::{StructDefinition, StructFields, TParam, Type, TypeName_, Type_, UseFuns}, parser::ast::{self as P, StructName}, shared::{Identifier, Name}, @@ -101,9 +102,6 @@ use move_symbol_pool::Symbol; /// Enabling/disabling the language server reporting readiness to support go-to-def and /// go-to-references to the IDE. pub const DEFS_AND_REFS_SUPPORT: bool = true; -/// Building Move code requires a larger stack size on Windows (16M has been chosen somewhat -/// arbitrarily) -pub const STACK_SIZE_BYTES: usize = 16 * 1024 * 1024; #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Copy)] /// Location of a definition's identifier @@ -662,84 +660,88 @@ impl SymbolicatorRunner { let thread_mtx_cvar = mtx_cvar.clone(); let runner = SymbolicatorRunner { mtx_cvar }; - let (mtx, cvar) = &*thread_mtx_cvar; - // Locations opened in the IDE (files or directories) for which manifest file is missing - let mut missing_manifests = BTreeSet::new(); - // infinite loop to wait for symbolication requests - eprintln!("starting symbolicator runner loop"); - loop { - let starting_path_opt = { - // hold the lock only as long as it takes to get the data, rather than through - // the whole symbolication process (hence a separate scope here) - let mut symbolicate = mtx.lock().unwrap(); - match symbolicate.clone() { - RunnerState::Quit => break, - RunnerState::Run(root_dir) => { - *symbolicate = RunnerState::Wait; - Some(root_dir) - } - RunnerState::Wait => { - // wait for next request - symbolicate = cvar.wait(symbolicate).unwrap(); + thread::Builder::new() + .spawn(move || { + let (mtx, cvar) = &*thread_mtx_cvar; + // Locations opened in the IDE (files or directories) for which manifest file is missing + let mut missing_manifests = BTreeSet::new(); + // infinite loop to wait for symbolication requests + eprintln!("starting symbolicator runner loop"); + loop { + let starting_path_opt = { + // hold the lock only as long as it takes to get the data, rather than through + // the whole symbolication process (hence a separate scope here) + let mut symbolicate = mtx.lock().unwrap(); match symbolicate.clone() { RunnerState::Quit => break, RunnerState::Run(root_dir) => { *symbolicate = RunnerState::Wait; Some(root_dir) } - RunnerState::Wait => None, + RunnerState::Wait => { + // wait for next request + symbolicate = cvar.wait(symbolicate).unwrap(); + match symbolicate.clone() { + RunnerState::Quit => break, + RunnerState::Run(root_dir) => { + *symbolicate = RunnerState::Wait; + Some(root_dir) + } + RunnerState::Wait => None, + } + } } - } - } - }; - if let Some(starting_path) = starting_path_opt { - let root_dir = Self::root_dir(&starting_path); - if root_dir.is_none() && !missing_manifests.contains(&starting_path) { - eprintln!("reporting missing manifest"); - - // report missing manifest file only once to avoid cluttering IDE's UI in - // cases when developer indeed intended to open a standalone file that was - // not meant to compile - missing_manifests.insert(starting_path); - if let Err(err) = sender.send(Err(anyhow!( - "Unable to find package manifest. Make sure that + }; + if let Some(starting_path) = starting_path_opt { + let root_dir = Self::root_dir(&starting_path); + if root_dir.is_none() && !missing_manifests.contains(&starting_path) { + eprintln!("reporting missing manifest"); + + // report missing manifest file only once to avoid cluttering IDE's UI in + // cases when developer indeed intended to open a standalone file that was + // not meant to compile + missing_manifests.insert(starting_path); + if let Err(err) = sender.send(Err(anyhow!( + "Unable to find package manifest. Make sure that the source files are located in a sub-directory of a package containing a Move.toml file. " - ))) { - eprintln!("could not pass missing manifest error: {:?}", err); - } - continue; - } - eprintln!("symbolication started"); - match get_symbols(root_dir.unwrap().as_path(), lint) { - Ok((symbols_opt, lsp_diagnostics)) => { - eprintln!("symbolication finished"); - if let Some(new_symbols) = symbols_opt { - // merge the new symbols with the old ones to support a - // (potentially) new project/package that symbolication information - // was built for - // - // TODO: we may consider "unloading" symbolication information when - // files/directories are being closed but as with other performance - // optimizations (e.g. incrementalizatino of the vfs), let's wait - // until we know we actually need it - let mut old_symbols = symbols.lock().unwrap(); - (*old_symbols).merge(new_symbols); - } - // set/reset (previous) diagnostics - if let Err(err) = sender.send(Ok(lsp_diagnostics)) { - eprintln!("could not pass diagnostics: {:?}", err); + ))) { + eprintln!("could not pass missing manifest error: {:?}", err); + } + continue; } - } - Err(err) => { - eprintln!("symbolication failed: {:?}", err); - if let Err(err) = sender.send(Err(err)) { - eprintln!("could not pass compiler error: {:?}", err); + eprintln!("symbolication started"); + match get_symbols(root_dir.unwrap().as_path(), lint) { + Ok((symbols_opt, lsp_diagnostics)) => { + eprintln!("symbolication finished"); + if let Some(new_symbols) = symbols_opt { + // merge the new symbols with the old ones to support a + // (potentially) new project/package that symbolication information + // was built for + // + // TODO: we may consider "unloading" symbolication information when + // files/directories are being closed but as with other performance + // optimizations (e.g. incrementalizatino of the vfs), let's wait + // until we know we actually need it + let mut old_symbols = symbols.lock().unwrap(); + (*old_symbols).merge(new_symbols); + } + // set/reset (previous) diagnostics + if let Err(err) = sender.send(Ok(lsp_diagnostics)) { + eprintln!("could not pass diagnostics: {:?}", err); + } + } + Err(err) => { + eprintln!("symbolication failed: {:?}", err); + if let Err(err) = sender.send(Err(err)) { + eprintln!("could not pass compiler error: {:?}", err); + } + } } } } - } - } + }) + .unwrap(); runner } @@ -1070,7 +1072,7 @@ pub fn get_symbols( let mut def_info = BTreeMap::new(); for (pos, module_ident, module_def) in typed_modules { - let mod_ident_str = format!("{}", module_ident); + let mod_ident_str = expansion_mod_ident_to_map_key(module_ident); let (defs, symbols) = get_mod_outer_defs( &pos, &sp(pos, *module_ident), @@ -1128,7 +1130,7 @@ pub fn get_symbols( }; for (pos, module_ident, module_def) in typed_modules { - let mod_ident_str = format!("{}", module_ident); + let mod_ident_str = expansion_mod_ident_to_map_key(module_ident); typing_symbolicator.use_defs = mod_use_defs.remove(&mod_ident_str).unwrap(); typing_symbolicator.alias_lengths = mod_to_alias_lengths.get(&mod_ident_str).unwrap(); typing_symbolicator.mod_symbols(module_def); @@ -1160,6 +1162,26 @@ pub fn get_symbols( Ok((Some(symbols), ide_diagnostics)) } +/// Produces module ident string of the form pkg_name::module_name to be used as a map key. +/// It's important that these are consistent between parsing AST and typed AST, +fn parsing_mod_ident_to_map_key(mod_ident: &P::ModuleIdent_) -> String { + format!("{}", mod_ident).to_string() +} + +/// Produces module ident string of the form pkg_name::module_name to be used as a map key +/// It's important that these are consistent between parsing AST and typed AST, +fn expansion_mod_ident_to_map_key(mod_ident: &E::ModuleIdent_) -> String { + use E::Address as A; + match mod_ident.address { + A::Numerical { + name: None, value, .. + } => format!("{value}::{}", mod_ident.module).to_string(), + A::Numerical { name: Some(n), .. } | A::NamedUnassigned(n) => { + format!("{n}::{}", mod_ident.module).to_string() + } + } +} + /// Get empty symbols pub fn empty_symbols() -> Symbols { Symbols { @@ -1371,7 +1393,7 @@ fn get_mod_outer_defs( // insert use of the module name in the definition itself let mod_name = ident.module; if let Some(mod_name_start) = get_start_loc(&mod_name.loc(), files, file_id_mapping) { - let mod_ident_str = format!("{}", ident); + let mod_ident_str = expansion_mod_ident_to_map_key(&ident); use_def_map.insert( mod_name_start.line, UseDef::new( @@ -1619,7 +1641,7 @@ impl<'a> ParsingSymbolicator<'a> { fn use_decl_symbols(&mut self, use_decl: &P::UseDecl) { match &use_decl.use_ { P::Use::ModuleUse(mod_ident, mod_use) => { - let mod_ident_str = format!("{}", mod_ident); + let mod_ident_str = parsing_mod_ident_to_map_key(&mod_ident.value); let Some(mod_defs) = self.mod_outer_defs.get(&mod_ident_str) else { return; }; @@ -2275,7 +2297,7 @@ impl<'a> TypingSymbolicator<'a> { let mod_ident = mod_call.module; let mod_def = self .mod_outer_defs - .get(&format!("{}", mod_ident.value)) + .get(&expansion_mod_ident_to_map_key(&mod_ident.value)) .unwrap(); if mod_def.functions.get(&mod_call.name.value()).is_none() { @@ -2364,7 +2386,7 @@ impl<'a> TypingSymbolicator<'a> { /// Add use of a const identifier fn add_const_use_def(&mut self, module_ident: &ModuleIdent, use_name: &Symbol, use_pos: &Loc) { - let mod_ident_str = format!("{}", module_ident.value); + let mod_ident_str = expansion_mod_ident_to_map_key(&module_ident.value); let Some(mod_defs) = self.mod_outer_defs.get(&mod_ident_str) else { return; }; @@ -2435,7 +2457,7 @@ impl<'a> TypingSymbolicator<'a> { use_name: &Symbol, use_pos: &Loc, ) { - let mod_ident_str = format!("{}", module_ident.value); + let mod_ident_str = expansion_mod_ident_to_map_key(&module_ident.value); let Some(mod_defs) = self.mod_outer_defs.get(&mod_ident_str) else { return; }; @@ -2484,7 +2506,7 @@ impl<'a> TypingSymbolicator<'a> { /// Add use of a struct identifier fn add_struct_use_def(&mut self, module_ident: &ModuleIdent, use_name: &Symbol, use_pos: &Loc) { - let mod_ident_str = format!("{}", module_ident); + let mod_ident_str = expansion_mod_ident_to_map_key(&module_ident.value); let Some(mod_defs) = self.mod_outer_defs.get(&mod_ident_str) else { return; }; @@ -2538,7 +2560,7 @@ impl<'a> TypingSymbolicator<'a> { use_name: &Symbol, use_pos: &Loc, ) { - let mod_ident_str = format!("{}", module_ident); + let mod_ident_str = expansion_mod_ident_to_map_key(module_ident); let Some(name_start) = get_start_loc(use_pos, self.files, self.file_id_mapping) else { debug_assert!(false); return; @@ -5932,3 +5954,22 @@ fn dot_call_test() { None, ); } + +#[test] +/// Checks if module identifiers used during symbolication process at both parsing and typing are +/// the same. They are used as a key to a map and if they look differently, it may lead to a crash +/// due to keys used for insertion/ retrieval being different. +fn mod_ident_uniform_test() { + let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + + path.push("tests/mod-ident-uniform"); + + let (symbols_opt, _) = get_symbols(path.as_path(), false).unwrap(); + let symbols = symbols_opt.unwrap(); + + let mut fpath = path.clone(); + fpath.push("sources/M1.move"); + let cpath = dunce::canonicalize(&fpath).unwrap(); + + symbols.file_use_defs.get(&cpath).unwrap(); +} diff --git a/external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/Move.toml b/external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/Move.toml new file mode 100644 index 0000000000000..4d4ae972dec2a --- /dev/null +++ b/external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/Move.toml @@ -0,0 +1,10 @@ +[package] +name = "ModIdentUniform" +version = "0.0.1" + +[dependencies] +MoveStdlib = { local = "../../../move-stdlib/", addr_subst = { "std" = "0x1" } } + +[addresses] +ModIdentUniform = "0xCAFE" +ConflicitingAddress = "0xCAFE" diff --git a/external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/sources/M1.move b/external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/sources/M1.move new file mode 100644 index 0000000000000..b294ed9ba2cbb --- /dev/null +++ b/external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/sources/M1.move @@ -0,0 +1,2 @@ +module ModIdentUniform::M1 { +}