Skip to content

Commit

Permalink
[move-ide] Fixed module id name construction (#16220)
Browse files Browse the repository at this point in the history 
## 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
#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.
  • Loading branch information
@awelc
awelc authored Feb 14, 2024
1 parent 730cc67 commit 4de9c7b
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 93 deletions.
19 changes: 4 additions & 15 deletions external-crates/move/crates/move-analyzer/src/bin/move-analyzer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ use std::{
collections::BTreeMap,
path::PathBuf,
sync::{Arc, Mutex},
thread,
};

use move_analyzer::{
Expand Down Expand Up @@ -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);
}
}
}
};
Expand Down
197 changes: 119 additions & 78 deletions external-crates/move/crates/move-analyzer/src/symbols.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,15 @@ use std::{
fmt,
path::{Path, PathBuf},
sync::{Arc, Condvar, Mutex},
thread,
};
use tempfile::tempdir;
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},
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}
10 changes: 10 additions & 0 deletions external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/Move.toml
View file
Original file line number Diff line number Diff line change
@@ -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"
2 changes: 2 additions & 0 deletions external-crates/move/crates/move-analyzer/tests/mod-ident-uniform/sources/M1.move
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module ModIdentUniform::M1 {
}

0 comments on commit 4de9c7b

Please sign in to comment.