[sui-pkg-alt] move-analyzer fixes (#24493)

## Description 

This PR brings a few fixes to move-analyzer and a small optimization to
reuse a regex rather than creating a new one every time.

## Test plan 

Existing tests.

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] gRPC:
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] Indexing Framework:

Author: stefan-mysten

external-crates/move/crates/move-analyzer/src/symbols/compilation.rs

@@ -44,7 +44,7 @@ use move_compiler::{
 };
 use move_ir_types::location::Loc;
 
-use move_package_alt::{flavor::MoveFlavor, package::RootPackage, schema::PackageName};
+use move_package_alt::{flavor::MoveFlavor, package::RootPackage};
 use move_package_alt_compilation::{
     build_config::BuildConfig,
     build_plan::BuildPlan,
@@ -339,13 +339,19 @@ pub fn get_compiled_pkg<F: MoveFlavor>(
 
     let root_pkg = load_root_pkg::<F>(&build_config, pkg_path)?;
     let root_pkg_name = Symbol::from(root_pkg.name().to_string());
+    // the package's transitive dependencies
+    let mut dependencies: Vec<_> = root_pkg
+        .packages()
+        .into_iter()
+        .filter(|x| !x.is_root())
+        .collect();
     let build_plan =
         BuildPlan::create(&root_pkg, &build_config)?.set_compiler_vfs_root(overlay_fs_root.clone());
 
-    let mapped_files_data =
-        compute_mapped_files(&root_pkg, &build_config, overlay_fs_root.clone())?;
     // Hash dependencies so we can check if something has changed.
     // TODO: do we still need this?
+    let mapped_files_data =
+        compute_mapped_files(&root_pkg, &build_config, overlay_fs_root.clone())?;
     let file_paths: Arc<BTreeMap<FileHash, PathBuf>> = Arc::new(
         mapped_files_data
             .files
@@ -361,12 +367,9 @@ pub fn get_compiled_pkg<F: MoveFlavor>(
     let mut compiler_analysis_info_opt = None;
     let mut compiler_autocomplete_info_opt = None;
 
-    // TODO: we need to rework on loading the root pkg only once.
-    let dependencies = root_pkg.packages();
-
     let compiler_flags = compiler_flags(&build_config);
     let (mut caching_result, other_diags) = if let Ok(deps_package_paths) =
-        make_deps_for_compiler(&mut Vec::new(), dependencies, &build_config)
+        make_deps_for_compiler(&mut Vec::new(), dependencies.clone(), &build_config)
     {
         let src_deps: BTreeMap<Symbol, PackagePaths> = deps_package_paths
             .into_iter()
@@ -414,8 +417,13 @@ pub fn get_compiled_pkg<F: MoveFlavor>(
 
         let caching_result = match cached_pkg_info_opt {
             Some(cached_pkg_info) => {
-                // TODO: do we need to do anything here?
-                // dependencies.remove_deps(cached_pkg_info.dep_names.clone());
+                // remove dependencies that are already included in the cached package info to
+                // avoid recompiling them
+                dependencies.retain(|d| {
+                    !cached_pkg_info
+                        .dep_names
+                        .contains(&Symbol::from(d.id().to_string()))
+                });
 
                 let deps = cached_pkg_info.deps.clone();
                 let analyzed_pkg_info = AnalyzedPkgInfo::new(
@@ -435,8 +443,15 @@ pub fn get_compiled_pkg<F: MoveFlavor>(
                 )
             }
             None => {
-                let sorted_deps = root_pkg.sorted_deps();
-                let sorted_deps: Vec<PackageName> = sorted_deps.into_iter().cloned().collect();
+                // get the topologically sorted dependencies, but use the package ids instead of
+                // package names. In the new pkg system, multiple packages with the same name can
+                // exist as the package system will assign unique package ids to them, before
+                // passing them to the compiler.
+                let sorted_deps: Vec<Symbol> = root_pkg
+                    .sorted_deps_ids()
+                    .into_iter()
+                    .map(|x| Symbol::from(x.to_string()))
+                    .collect();
                 if let Some((program_deps, dep_names)) = compute_pre_compiled_dep_data(
                     &mut cached_packages.compiled_dep_pkgs,
                     mapped_files_data.dep_pkg_paths,
@@ -508,8 +523,8 @@ pub fn get_compiled_pkg<F: MoveFlavor>(
     // (that no longer have failures/warnings) are reset
     let mut ide_diags = lsp_empty_diagnostics(mapped_files_data.files.file_name_mapping());
     if full_compilation || !files_to_compile.is_empty() {
-        build_plan.compile_with_driver(
-            // dependencies,
+        build_plan.compile_with_driver_and_deps(
+            dependencies.into_iter().map(|x| x.id()).cloned().collect(),
             &mut std::io::sink(),
             |compiler| {
                 let compiler = compiler.set_ide_mode();
@@ -548,7 +563,6 @@ pub fn get_compiled_pkg<F: MoveFlavor>(
                         let failure = true;
                         diagnostics = Some((diags, failure));
                         eprintln!("typed AST compilation failed");
-                        eprintln!("diagnostics: {:#?}", diagnostics);
                         return Ok((files, vec![]));
                     }
                 };
@@ -574,7 +588,6 @@ pub fn get_compiled_pkg<F: MoveFlavor>(
                 });
                 caching_result.edition =
                     Some(compiler.compilation_env().edition(Some(root_pkg_name)));
-
                 // compile to CFGIR for accurate diags
                 eprintln!("compiling to CFGIR");
                 let compilation_result = compiler.at_typing(typed_program).run::<PASS_CFGIR>();
@@ -696,23 +709,24 @@ fn compute_pre_compiled_dep_data(
     mut dep_paths: BTreeMap<Symbol, PathBuf>,
     mut src_deps: BTreeMap<Symbol, PackagePaths>,
     root_package_name: Symbol,
-    topological_order: &[PackageName],
+    topological_order: &[Symbol],
     compiler_flags: Flags,
     vfs_root: VfsPath,
 ) -> Option<(Arc<PreCompiledProgramInfo>, BTreeSet<Symbol>)> {
     let mut pre_compiled_modules = BTreeMap::new();
     let mut pre_compiled_names = BTreeSet::new();
     for pkg_name in topological_order.iter().rev() {
-        let pkg_name = Symbol::from(pkg_name.to_string());
-        if pkg_name == root_package_name {
+        // both pkg_name and root_package_name are actually PackageIDs and generated by the pkg
+        // system
+        if pkg_name == &root_package_name {
             continue;
         }
-        let Some(dep_path) = dep_paths.remove(&pkg_name) else {
+        let Some(dep_path) = dep_paths.remove(pkg_name) else {
             eprintln!("no dep path for {pkg_name}, no caching");
             // do non-cached path
             return None;
         };
-        let Some(dep_info) = src_deps.remove(&pkg_name) else {
+        let Some(dep_info) = src_deps.remove(pkg_name) else {
             eprintln!("no dep info for {pkg_name}, no caching");
             // do non-cached path
             return None;
@@ -827,10 +841,7 @@ fn compute_mapped_files<F: MoveFlavor>(
             if is_dep {
                 hasher.update(fhash.0);
                 dep_hashes.push(fhash);
-                dep_pkg_paths.insert(
-                    rpkg.name().as_str().into(),
-                    rpkg.path().path().to_path_buf(),
-                );
+                dep_pkg_paths.insert(rpkg.id().clone().into(), rpkg.path().path().to_path_buf());
             }
             // write to top layer of the overlay file system so that the content
             // is immutable for the duration of compilation and symbolication

external-crates/move/crates/move-package-alt-compilation/src/build_plan.rs

@@ -27,7 +27,7 @@ use move_package_alt::{
     errors::PackageResult,
     flavor::MoveFlavor,
     package::{RootPackage, layout::SourcePackageLayout},
-    schema::PackageName,
+    schema::PackageID,
 };
 use move_symbol_pool::Symbol;
 use toml_edit::{DocumentMut, value};
@@ -36,14 +36,18 @@ const EDITION_NAME: &str = "edition";
 
 pub struct BuildPlan<'a, F: MoveFlavor> {
     root_pkg: &'a RootPackage<F>,
+    sorted_deps_ids: Vec<&'a PackageID>,
     compiler_vfs_root: Option<VfsPath>,
     build_config: BuildConfig,
 }
 
 impl<'a, F: MoveFlavor> BuildPlan<'a, F> {
     pub fn create(root_pkg: &'a RootPackage<F>, build_config: &BuildConfig) -> PackageResult<Self> {
+        let mut sorted_deps_ids = root_pkg.sorted_deps_ids();
+        sorted_deps_ids.reverse();
         Ok(Self {
             root_pkg,
+            sorted_deps_ids,
             build_config: build_config.clone(),
             compiler_vfs_root: None,
         })
@@ -73,12 +77,52 @@ impl<'a, F: MoveFlavor> BuildPlan<'a, F> {
             Compiler,
         )
             -> anyhow::Result<(MappedFiles, Vec<AnnotatedCompiledUnit>)>,
+    ) -> anyhow::Result<CompiledPackage> {
+        let program_info_hook = SaveHook::new([SaveFlag::TypingInfo]);
+        let dependencies: BTreeSet<PackageID> = self
+            .root_pkg
+            .packages()
+            .into_iter()
+            .filter(|x| !x.is_root())
+            .map(|x| x.id().to_string())
+            .collect();
+        let compiled = build_all::<W, F>(
+            writer,
+            self.compiler_vfs_root.clone(),
+            self.root_pkg,
+            dependencies,
+            &self.build_config,
+            |compiler| {
+                let compiler = compiler.add_save_hook(&program_info_hook);
+                compiler_driver(compiler)
+            },
+        )?;
+
+        let project_root = self.root_pkg.package_path();
+
+        self.clean(
+            &project_root.join(CompiledPackageLayout::Root.path()),
+            self.sorted_deps_ids.clone(),
+        )?;
+
+        Ok(compiled)
+    }
+
+    pub fn compile_with_driver_and_deps<W: Write + Send>(
+        &self,
+        dependencies: BTreeSet<PackageID>,
+        writer: &mut W,
+        compiler_driver: impl FnOnce(
+            Compiler,
+        )
+            -> anyhow::Result<(MappedFiles, Vec<AnnotatedCompiledUnit>)>,
     ) -> anyhow::Result<CompiledPackage> {
         let program_info_hook = SaveHook::new([SaveFlag::TypingInfo]);
         let compiled = build_all::<W, F>(
             writer,
             self.compiler_vfs_root.clone(),
             self.root_pkg,
+            dependencies,
             &self.build_config,
             |compiler| {
                 let compiler = compiler.add_save_hook(&program_info_hook);
@@ -87,11 +131,10 @@ impl<'a, F: MoveFlavor> BuildPlan<'a, F> {
         )?;
 
         let project_root = self.root_pkg.package_path();
-        let sorted_deps = self.root_pkg.sorted_deps().into_iter().cloned().collect();
 
         self.clean(
             &project_root.join(CompiledPackageLayout::Root.path()),
-            sorted_deps,
+            self.sorted_deps_ids.clone(),
         )?;
 
         Ok(compiled)
@@ -134,7 +177,7 @@ impl<'a, F: MoveFlavor> BuildPlan<'a, F> {
 
     // Clean out old packages that are no longer used, or no longer used under the current
     // compilation flags
-    fn clean(&self, project_root: &Path, keep_paths: BTreeSet<PackageName>) -> anyhow::Result<()> {
+    fn clean(&self, project_root: &Path, keep_paths: Vec<&PackageID>) -> anyhow::Result<()> {
         // Compute the actual build directory based on install_dir configuration
         let build_root = shared::get_build_output_path(project_root, &self.build_config);
 
@@ -159,11 +202,18 @@ impl<'a, F: MoveFlavor> BuildPlan<'a, F> {
     /// Migrate the package from legacy to Move 2024 edition, if possible.
     pub fn migrate<W: Write + Send>(&self, writer: &mut W) -> anyhow::Result<Option<Migration>> {
         let root_name = Symbol::from(self.root_pkg.name().to_string());
+        let dependencies: BTreeSet<_> = self
+            .root_pkg
+            .sorted_deps_ids()
+            .into_iter()
+            .cloned()
+            .collect();
         let (files, res) = build_for_driver(
             writer,
             None,
             &self.build_config,
             self.root_pkg,
+            dependencies,
             |compiler| compiler.generate_migration_patch(&root_name),
         )?;
         let migration = match res {
@@ -183,11 +233,10 @@ impl<'a, F: MoveFlavor> BuildPlan<'a, F> {
         };
 
         let project_root = self.root_pkg.package_path();
-        let sorted_deps = self.root_pkg.sorted_deps().into_iter().cloned().collect();
 
         self.clean(
             &project_root.join(CompiledPackageLayout::Root.path()),
-            sorted_deps,
+            self.sorted_deps_ids.clone(),
         )?;
 
         Ok(migration)

external-crates/move/crates/move-package-alt-compilation/src/compilation.rs

@@ -32,7 +32,10 @@ use move_compiler::{
 };
 use move_docgen::DocgenFlags;
 use move_package_alt::{
-    flavor::MoveFlavor, graph::PackageInfo, package::RootPackage, schema::Environment,
+    flavor::MoveFlavor,
+    graph::PackageInfo,
+    package::RootPackage,
+    schema::{Environment, PackageID},
 };
 use move_symbol_pool::Symbol;
 use std::{collections::BTreeMap, io::Write, path::PathBuf, str::FromStr};
@@ -76,17 +79,24 @@ pub fn build_all<W: Write + Send, F: MoveFlavor>(
     w: &mut W,
     vfs_root: Option<VfsPath>,
     root_pkg: &RootPackage<F>,
+    dependencies: BTreeSet<PackageID>,
     build_config: &BuildConfig,
     compiler_driver: impl FnOnce(Compiler) -> Result<(MappedFiles, Vec<AnnotatedCompiledUnit>)>,
 ) -> Result<CompiledPackage> {
     let project_root = root_pkg.package_path().to_path_buf();
     let program_info_hook = SaveHook::new([SaveFlag::TypingInfo]);
     let package_name = Symbol::from(root_pkg.name().as_str());
-    let (file_map, all_compiled_units) =
-        build_for_driver(w, vfs_root, build_config, root_pkg, |compiler| {
+    let (file_map, all_compiled_units) = build_for_driver(
+        w,
+        vfs_root,
+        build_config,
+        root_pkg,
+        dependencies,
+        |compiler| {
             let compiler = compiler.add_save_hook(&program_info_hook);
             compiler_driver(compiler)
-        })?;
+        },
+    )?;
 
     let mut all_compiled_units_vec = vec![];
     let mut root_compiled_units = vec![];
@@ -195,9 +205,15 @@ pub fn build_for_driver<W: Write + Send, T, F: MoveFlavor>(
     vfs_root: Option<VfsPath>,
     build_config: &BuildConfig,
     root_pkg: &RootPackage<F>,
+    dependencies: BTreeSet<PackageID>,
     compiler_driver: impl FnOnce(Compiler) -> Result<T>,
 ) -> Result<T> {
+    // TODO: pkg-alt this seems to add more packages for the compiler than necessary.
     let packages = root_pkg.packages();
+    let packages = packages
+        .into_iter()
+        .filter(|p| p.is_root() || dependencies.contains(p.id()))
+        .collect::<Vec<_>>();
     let package_paths = make_deps_for_compiler(w, packages, build_config)?;
 
     debug!("Package paths {:#?}", package_paths);
@@ -212,7 +228,6 @@ pub fn build_for_driver<W: Write + Send, T, F: MoveFlavor>(
     let lint_level = build_config.lint_flag.get();
     let sui_mode = build_config.default_flavor == Some(Flavor::Sui);
     let flags = compiler_flags(build_config);
-
     let mut compiler = Compiler::from_package_paths(vfs_root, package_paths, vec![])
         .unwrap()
         .set_flags(flags);
@@ -372,7 +387,7 @@ pub fn make_deps_for_compiler<W: Write + Send, F: MoveFlavor>(
             warning_filter: WarningFiltersBuilder::new_for_source(),
         };
 
-        // TODO: improve/rework this? Renaming the root pkg to have a unique name for the compiler
+        // Assign a unique name for the compiler for each package.
         let safe_name = Symbol::from(pkg.id().clone());
 
         debug!("Package name {:?} -- Safe name {:?}", name, safe_name);

external-crates/move/crates/move-package-alt/src/compatibility/mod.rs

@@ -8,6 +8,7 @@ use std::path::{Path, PathBuf};
 
 use anyhow::{Context, Result, bail};
 use move_core_types::account_address::{AccountAddress, AccountAddressParseError};
+use once_cell::sync::Lazy;
 use regex::Regex;
 use tracing::debug;
 
@@ -34,6 +35,9 @@ pub enum LegacySubstOrRename {
 /// The regex to detect `module <name>::<module_name>` on its different forms.
 const MODULE_REGEX: &str = r"\bmodule\s+([a-zA-Z_][\w]*)::([a-zA-Z_][\w]*)";
 
+// Compile regex once at program startup
+static MODULE_REGEX_COMPILED: Lazy<Regex> = Lazy::new(|| Regex::new(MODULE_REGEX).unwrap());
+
 /// This is a naive way to detect all module names that are part of the source code
 /// for a given package.
 ///
@@ -111,18 +115,15 @@ fn find_files(files: &mut Vec<PathBuf>, dir: &Path, extension: &str, max_depth:
 // Consider supporting the legacy `address { module {} }` format.
 fn parse_module_names(contents: &str) -> Result<HashSet<String>> {
     let clean = strip_comments(contents);
-    let mut set = HashSet::new();
+
     // This matches `module a::b {}`, and `module a::b;` cases.
     // In both cases, the match is the 2nd group (so `match.get(1)`)
-    let regex = Regex::new(MODULE_REGEX).unwrap();
-
-    for cap in regex.captures_iter(&clean) {
-        set.insert(cap[1].to_string());
-    }
-
-    Ok(set
-        .into_iter()
-        .filter(|name| !is_address_like(name.as_str()))
+    Ok(MODULE_REGEX_COMPILED
+        .captures_iter(&clean)
+        .filter_map(|cap| {
+            let name = &cap[1];
+            (!is_address_like(name)).then(|| name.to_string())
+        })
         .collect())
 }