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