Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: sui-pkg-alt
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
207730e
A few fixes to move-analyzer and a small optimization to reuse a rege…
stefan-mysten Dec 2, 2025
2e63435
WIP
stefan-mysten Dec 3, 2025
836236c
Cleanup and add back the compile_with_driver_and_deps function
stefan-mysten Dec 3, 2025
af8cd8d
Cleanup
stefan-mysten Dec 3, 2025
c914813
Cleanup
stefan-mysten Dec 3, 2025
5cb210d
Cleanup
stefan-mysten Dec 3, 2025
2b461c0
Cleanup
stefan-mysten Dec 3, 2025
94e37f0
Cleanup
stefan-mysten Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 35 additions & 24 deletions external-crates/move/crates/move-analyzer/src/symbols/compilation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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();

Comment on lines -364 to -366
Copy link
Contributor Author

@stefan-mysten stefan-mysten Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved it up a bit

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()
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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![]));
}
};
Expand All @@ -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>();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
61 changes: 55 additions & 6 deletions external-crates/move/crates/move-package-alt-compilation/src/build_plan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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,
})
Expand Down Expand Up @@ -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);
Expand All @@ -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)
Expand Down Expand Up @@ -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);

Expand All @@ -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 {
Expand All @@ -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)
Expand Down
27 changes: 21 additions & 6 deletions external-crates/move/crates/move-package-alt-compilation/src/compilation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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![];
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
21 changes: 11 additions & 10 deletions external-crates/move/crates/move-package-alt/src/compatibility/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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.
///
Expand Down Expand Up @@ -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())
})
Comment on lines 117 to +126
Copy link
Contributor Author

@stefan-mysten stefan-mysten Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be slightly more optimized, particularly that we're reusing the REGEX rather than creating a new one every time.

.collect())
}

Expand Down
Loading
Loading