From 75176c78c15d368f26e03002cc7d8216a54d685a Mon Sep 17 00:00:00 2001 From: Michael George Date: Thu, 8 May 2025 11:01:17 -0400 Subject: [PATCH] Review of package-alt --- .../move/crates/move-package-alt/TODO.md | 181 ++++++++++++++++++ .../src/dependency/dependency_set.rs | 9 + .../src/dependency/external.rs | 3 + .../move-package-alt/src/dependency/git.rs | 23 ++- .../move-package-alt/src/dependency/local.rs | 1 + .../move-package-alt/src/dependency/mod.rs | 42 +++- .../move-package-alt/src/jsonrpc/client.rs | 1 + .../move-package-alt/src/package/manifest.rs | 3 + .../move-package-alt/src/package/mod.rs | 2 + 9 files changed, 254 insertions(+), 11 deletions(-) create mode 100644 external-crates/move/crates/move-package-alt/TODO.md diff --git a/external-crates/move/crates/move-package-alt/TODO.md b/external-crates/move/crates/move-package-alt/TODO.md new file mode 100644 index 0000000000000..c7a9e0ee82fca --- /dev/null +++ b/external-crates/move/crates/move-package-alt/TODO.md @@ -0,0 +1,181 @@ +TODO +==== + - in many instances we've wanted to do some amount of tranformation when converting from serialized + stuff into our data structures. I've been thinking for a while that we may want to just have a + separate parsing module that contains only the parsing data structures and + serialization/deserialization code. We've been avoiding this for a while and I think it's + causing us headaches + + - + +Needs review +============ + - [X] dependency/dependency_set.rs + - [X] dependency/mod.rs + - [X] dependency/local.rs + - [X] dependency/git.rs - will rereview with cleanup PR + + - [ ] dependency/external.rs + - [ ] mocks/mock-resolver.rs + - [ ] jsonrpc/types.rs + - [ ] jsonrpc/client.rs + - [ ] jsonrpc/mod.rs + - [ ] tests/jsonrpc_tests.rs + + + - [ ] errors/manifest_error.rs + - [ ] errors/git_error.rs + - [ ] errors/files.rs + - [ ] errors/mod.rs + - [ ] errors/located.rs + - [ ] errors/lockfile_error.rs + - [ ] errors/resolver_error.rs + - [ ] lib.rs + + - [ ] main.rs + - [ ] cli/build.rs + - [ ] cli/mod.rs + - [ ] cli/parse.rs + + - [ ] package/mod.rs + - [ ] flavor/vanilla.rs + - [ ] flavor/mod.rs + + - [ ] package/lockfile.rs + - [ ] package/manifest.rs + + - [ ] tests/test_runner.rs + - [ ] tests/data/flavor_sui/Move@parsed.snap + - [ ] tests/data/flavor_sui/Move.parsed + - [ ] tests/data/flavor_sui/Move.toml + - [ ] tests/data/edition_unknown/Move@parsed.snap + - [ ] tests/data/edition_unknown/Move.parsed + - [ ] tests/data/edition_unknown/Move.toml + - [ ] tests/data/edition_2024_alpha/Move@parsed.snap + - [ ] tests/data/edition_2024_alpha/Move.parsed + - [ ] tests/data/edition_2024_alpha/Move.toml + - [ ] tests/data/external_returns_external/Move.pinned + - [ ] tests/data/external_returns_external/Move@pinned.snap + - [ ] tests/data/external_returns_external/Move.toml + - [ ] tests/data/too_many_entries_renaming_instantiation/Move@parsed.snap + - [ ] tests/data/too_many_entries_renaming_instantiation/Move.parsed + - [ ] tests/data/too_many_entries_renaming_instantiation/Move.toml + - [ ] tests/data/invalid_lockfile_parsing/Move.snap + - [ ] tests/data/no_path_set_for_dependency/Move@parsed.snap + - [ ] tests/data/no_path_set_for_dependency/Move.parsed + - [ ] tests/data/no_path_set_for_dependency/Move.toml + - [ ] tests/data/flavor_unknown/Move@parsed.snap + - [ ] tests/data/flavor_unknown/Move.parsed + - [ ] tests/data/flavor_unknown/Move.toml + - [ ] tests/data/invalid_identifier_package_name/Move@parsed.snap + - [ ] tests/data/invalid_identifier_package_name/Move.parsed + - [ ] tests/data/invalid_identifier_package_name/Move.toml + - [ ] tests/data/missing_required_package_segment/Move@parsed.snap + - [ ] tests/data/missing_required_package_segment/Move.parsed + - [ ] tests/data/missing_required_package_segment/Move.toml + - [ ] tests/data/missing_minimal_required_field_name/Move@parsed.snap + - [ ] tests/data/missing_minimal_required_field_name/Move.parsed + - [ ] tests/data/missing_minimal_required_field_name/Move.toml + - [ ] tests/data/basic_implicit_deps/Move@parsed.snap + - [ ] tests/data/basic_implicit_deps/Move.parsed + - [ ] tests/data/basic_implicit_deps/Move.toml + - [ ] tests/data/invalid_authors/Move@parsed.snap + - [ ] tests/data/invalid_authors/Move.parsed + - [ ] tests/data/invalid_authors/Move.toml + - [ ] tests/data/lockfile_parsing_valid/Move.toml + - [ ] tests/data/lockfile_parsing_valid/Move@locked.snap + - [ ] tests/data/lockfile_parsing_valid/Move.lock + - [ ] tests/data/lockfile_parsing_valid/Move.locked + - [ ] tests/data/edition_unknown_suffix/Move@parsed.snap + - [ ] tests/data/edition_unknown_suffix/Move.parsed + - [ ] tests/data/edition_unknown_suffix/Move.toml + - [ ] tests/data/flavor_global_storage/Move@parsed.snap + - [ ] tests/data/flavor_global_storage/Move.parsed + - [ ] tests/data/flavor_global_storage/Move.toml + - [ ] tests/data/missing_required_envs_segment/Move.snap + - [ ] tests/data/missing_required_envs_segment/Move.toml + - [ ] tests/data/external_multiple_resolvers/Move.pinned + - [ ] tests/data/external_multiple_resolvers/Move@pinned.snap + - [ ] tests/data/external_multiple_resolvers/Move.toml + - [ ] tests/data/non_identifier_address_name_in_subst/Move@parsed.snap + - [ ] tests/data/non_identifier_address_name_in_subst/Move.parsed + - [ ] tests/data/non_identifier_address_name_in_subst/Move.toml + - [ ] tests/data/basic/Move@parsed.snap + - [ ] tests/data/basic/Move.parsed + - [ ] tests/data/basic/Move.toml + - [ ] tests/data/external_missing_keys/Move.pinned + - [ ] tests/data/external_missing_keys/Move@pinned.snap + - [ ] tests/data/external_missing_keys/Move.toml + - [ ] tests/data/package_name_empty/Move@parsed.snap + - [ ] tests/data/package_name_empty/Move.parsed + - [ ] tests/data/package_name_empty/Move.toml + - [ ] tests/data/external_empty_output/Move.pinned + - [ ] tests/data/external_empty_output/Move@pinned.snap + - [ ] tests/data/external_empty_output/Move.toml + - [ ] tests/data/edition_legacy/Move@parsed.snap + - [ ] tests/data/edition_legacy/Move.parsed + - [ ] tests/data/edition_legacy/Move.toml + - [ ] tests/data/invalid_environment2/Move@parsed.snap + - [ ] tests/data/invalid_environment2/Move.parsed + - [ ] tests/data/invalid_environment2/Move.toml + - [ ] tests/data/external_basic/Move@pinned.snap + - [ ] tests/data/external_basic/Move.toml + - [ ] tests/data/unknown_toplevel_field/Move@parsed.snap + - [ ] tests/data/unknown_toplevel_field/Move.parsed + - [ ] tests/data/unknown_toplevel_field/Move.toml + - [ ] tests/data/edition_2024_beta/Move@parsed.snap + - [ ] tests/data/edition_2024_beta/Move.parsed + - [ ] tests/data/edition_2024_beta/Move.toml + - [ ] tests/data/non_string_package_name/Move@parsed.snap + - [ ] tests/data/non_string_package_name/Move.parsed + - [ ] tests/data/non_string_package_name/Move.toml + - [ ] tests/data/external_errorcode/Move.pinned + - [ ] tests/data/external_errorcode/Move@pinned.snap + - [ ] tests/data/external_errorcode/Move.toml + - [ ] tests/data/invalid_environment/Move@parsed.snap + - [ ] tests/data/invalid_environment/Move.parsed + - [ ] tests/data/invalid_environment/Move.toml + - [ ] tests/data/external_non_json/Move@pinned.snap + - [ ] tests/data/external_non_json/Move.toml + - [ ] tests/data/external_bad_schema/Move.pinned + - [ ] tests/data/external_bad_schema/Move@pinned.snap + - [ ] tests/data/external_bad_schema/Move.toml + - [ ] tests/data/basic_with_bar/Move@parsed.snap + - [ ] tests/data/basic_with_bar/Move.parsed + - [ ] tests/data/basic_with_bar/Move.toml + - [ ] tests/data/basic_with_bar/bar/Move@parsed.snap + - [ ] tests/data/basic_with_bar/bar/Move.parsed + - [ ] tests/data/basic_with_bar/bar/Move.toml + - [ ] tests/data/edition_2024/Move@parsed.snap + - [ ] tests/data/edition_2024/Move.parsed + - [ ] tests/data/edition_2024/Move.toml + - [ ] tests/data/basic_missing_git_dep/Move@parsed.snap + - [ ] tests/data/basic_missing_git_dep/Move.parsed + - [ ] tests/data/basic_missing_git_dep/Move.toml + - [ ] tests/data/non_string_local_dep_path/Move@parsed.snap + - [ ] tests/data/non_string_local_dep_path/Move.parsed + - [ ] tests/data/non_string_local_dep_path/Move.toml + - [ ] tests/data/integer_subst_in_dependency/Move@parsed.snap + - [ ] tests/data/integer_subst_in_dependency/Move.parsed + - [ ] tests/data/integer_subst_in_dependency/Move.toml + - [ ] tests/data/basic_move_project/config + - [ ] tests/data/basic_move_project/pkg_a/Move.toml + - [ ] tests/data/basic_move_project/pkg_b/Move.toml + - [ ] tests/data/invalid_hex_address_in_subst/Move@parsed.snap + - [ ] tests/data/invalid_hex_address_in_subst/Move.parsed + - [ ] tests/data/invalid_hex_address_in_subst/Move.toml + - [ ] tests/data/basic_no_dep_override_git/Move@parsed.snap + - [ ] tests/data/basic_no_dep_override_git/Move.parsed + - [ ] tests/data/basic_no_dep_override_git/Move.toml + - [ ] tests/data/duplicate_top_level_field/Move@parsed.snap + - [ ] tests/data/duplicate_top_level_field/Move.parsed + - [ ] tests/data/duplicate_top_level_field/Move.toml + - [ ] tests/data/invalid_environment1/Move@parsed.snap + - [ ] tests/data/invalid_environment1/Move.parsed + - [ ] tests/data/invalid_environment1/Move.toml + - [ ] tests/data/invalid_author/Move@parsed.snap + - [ ] tests/data/invalid_author/Move.parsed + - [ ] tests/data/invalid_author/Move.toml + - [ ] tests/data/basic_external_resolver/Move@parsed.snap + - [ ] tests/data/basic_external_resolver/Move.parsed + - [ ] tests/data/basic_external_resolver/Move.toml diff --git a/external-crates/move/crates/move-package-alt/src/dependency/dependency_set.rs b/external-crates/move/crates/move-package-alt/src/dependency/dependency_set.rs index 97c8d847c9478..954d9e648c709 100644 --- a/external-crates/move/crates/move-package-alt/src/dependency/dependency_set.rs +++ b/external-crates/move/crates/move-package-alt/src/dependency/dependency_set.rs @@ -14,6 +14,12 @@ use serde::{Deserialize, Serialize}; use crate::package::{EnvironmentName, PackageName}; +// TODO (potential refactor): using [Option] here and representing the default environment as +// [None] has led to some confusion, it might be better to make a specific enumeration, so that +// [DependencySet] becomes closer to a vanilla map. In fact, in the dependency graph we'll have +// `Option, PackageName` edges - this might be a good type to encapsulate; then +// DependencySet just becomes a map from edges to T. A little curry can go a long way + /// A set of default dependencies and dep replacements. Within each environment, package names are /// unique. /// @@ -77,6 +83,7 @@ impl DependencySet { /// Convenience method to return either [default_deps] or [deps_for_env] depending on [env]; an /// [env] of [None] indicates a request for the default dependencies. + /// TODO rename to deps pub fn deps_for(&self, env: Option<&EnvironmentName>) -> BTreeMap { match env { Some(env) => self.deps_for_env(env), @@ -127,6 +134,7 @@ impl DependencySet { /// A copy of [self] expanded with an entry (package name, env, dep) for all /// packages in [self] and environments in [envs]. + /// TODO: rename to expand or extend pub fn explode(&mut self, envs: impl IntoIterator) where T: Clone, @@ -263,6 +271,7 @@ impl FromIterator<(Option, PackageName, T)> for DependencySe } } +// TODO: maybe this can be derived with our fancy derive macros? // Note: can't be derived because that adds a spurious T: Default bound impl Default for DependencySet { /// The empty dependency set diff --git a/external-crates/move/crates/move-package-alt/src/dependency/external.rs b/external-crates/move/crates/move-package-alt/src/dependency/external.rs index 1fbfa5e3ffbfa..7fd18cd208971 100644 --- a/external-crates/move/crates/move-package-alt/src/dependency/external.rs +++ b/external-crates/move/crates/move-package-alt/src/dependency/external.rs @@ -60,6 +60,7 @@ struct RField { } /// Requests from the package mananger to the external resolver +// TODO: fix typo above #[derive(Serialize, Debug)] struct ResolveRequest { #[serde(default)] @@ -135,6 +136,8 @@ impl ExternalDependency { } } +// TODO: CLEANUP +// Explain this conversion impl TryFrom for ExternalDependency { type Error = PackageError; diff --git a/external-crates/move/crates/move-package-alt/src/dependency/git.rs b/external-crates/move/crates/move-package-alt/src/dependency/git.rs index c9eebcf379145..dcb64625bb696 100644 --- a/external-crates/move/crates/move-package-alt/src/dependency/git.rs +++ b/external-crates/move/crates/move-package-alt/src/dependency/git.rs @@ -6,6 +6,7 @@ //! //! Git dependencies are cached in `~/.move`, which has the following structure: //! +//! TODO: this doesn't match the implementation below: //! ```ignore //! .move/ //! git/ @@ -34,8 +35,17 @@ use crate::errors::{GitError, GitErrorKind, Located, PackageError, PackageResult use super::{DependencySet, Pinned, Unpinned}; +// TODO: (potential refactor): it might be good to separate out a separate module that is just git +// stuff and another that uses that git stuff to implement the dependency operations (like +// the jsonrpc / dependency::external split). + +// TODO: curious about the benefit of using String instead of wrapping it. The advantage of +// wrapping it is that we have invariants (with the type alias, nothing prevents us from +// writing `let x : Sha = ""` (whereas `let x = Sha::new("")` can fail) type Sha = String; +/// TODO keep same style around all types +/// /// A git dependency that is unpinned. The `rev` field can be either empty, a branch, or a sha. To /// resolve this into a [`PinnedGitDependency`], call `pin_one` function. #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] @@ -60,7 +70,6 @@ pub struct PinnedGitDependency { repo: String, /// The exact sha for the revision - // rev: Sha, #[serde(deserialize_with = "deserialize_sha")] rev: Sha, @@ -71,6 +80,7 @@ pub struct PinnedGitDependency { /// Helper struct that represents a Git repository, with extra information about which folder to /// checkout. +// TODO: how is this different from [UnpinnedGitDependency]? #[derive(Clone, Debug)] pub struct GitRepo { /// Repository URL @@ -81,7 +91,8 @@ pub struct GitRepo { path: PathBuf, } -// Custom error type for SHA validation +/// Custom error type for SHA validation +// TODO: derive(Error)? #[derive(Debug)] pub enum ShaError { InvalidLength(usize), @@ -133,6 +144,7 @@ impl GitRepo { self.rev.as_deref() } + // TODO: needs a comment pub fn package_set_path(&self) -> &PathBuf { &self.path } @@ -163,6 +175,7 @@ impl GitRepo { } /// Used for testing to be able to specify which folder to fetch to. Use `fetch` for all other needs. + // TODO: should be non-pub pub async fn fetch_to_folder(&self, fetch_to_folder: PathBuf) -> PackageResult { self.fetch_impl(Some(fetch_to_folder)).await } @@ -170,6 +183,8 @@ impl GitRepo { /// Checkout the repository using a sparse checkout. It will try to clone without checkout, set /// sparse checkout directory, and then checkout the folder specified by `self.path` at the /// given sha. + /// + // TODO think more about debug statements and what information to log async fn checkout_repo(&self, repo_fs_path: &PathBuf, sha: &str) -> PackageResult<()> { // Checkout repo if it does not exist already if !repo_fs_path.exists() { @@ -267,6 +282,7 @@ impl GitRepo { } /// Runs a git command from the provided arguments. + // TODO: check for error codes here pub async fn run_git_cmd_with_args( &self, args: &[&str], @@ -433,6 +449,8 @@ pub fn format_repo_to_fs_path(repo: &str, sha: &str, root_path: Option) /// Check if the given string is a valid commit SHA, i.e., 40 character long with only /// lowercase letters and digits +/// +// TODO: rename this function to is_sha fn check_is_commit_sha(input: &str) -> bool { input.len() == 40 && input @@ -448,6 +466,7 @@ fn url_to_file_name(url: &str) -> String { .to_string() } +// TODO: add more tests #[cfg(test)] mod tests { use super::*; diff --git a/external-crates/move/crates/move-package-alt/src/dependency/local.rs b/external-crates/move/crates/move-package-alt/src/dependency/local.rs index a017eba38e787..8d44f826eff1f 100644 --- a/external-crates/move/crates/move-package-alt/src/dependency/local.rs +++ b/external-crates/move/crates/move-package-alt/src/dependency/local.rs @@ -16,6 +16,7 @@ pub struct LocalDependency { local: PathBuf, } +// TODO: dead code impl TryFrom<(&Path, toml_edit::Value)> for LocalDependency { type Error = anyhow::Error; // TODO diff --git a/external-crates/move/crates/move-package-alt/src/dependency/mod.rs b/external-crates/move/crates/move-package-alt/src/dependency/mod.rs index 9fc1f391e3625..4f73be54a5431 100644 --- a/external-crates/move/crates/move-package-alt/src/dependency/mod.rs +++ b/external-crates/move/crates/move-package-alt/src/dependency/mod.rs @@ -2,13 +2,6 @@ // Copyright (c) The Move Contributors // SPDX-License-Identifier: Apache-2.0 -mod dependency_set; -// TODO: this shouldn't be pub; need to move resolver error into resolver module -pub mod external; -mod git; -mod local; - -pub use dependency_set::DependencySet; use std::{ collections::BTreeMap, fmt::{self, Debug}, @@ -16,7 +9,6 @@ use std::{ path::PathBuf, process::{Command, Stdio}, }; -use tracing::debug; use derive_where::derive_where; use serde::{ @@ -24,6 +16,8 @@ use serde::{ de::{self, MapAccess, SeqAccess, Visitor}, }; +use tracing::debug; + use crate::{ errors::{GitError, PackageError, PackageResult, ResolverError}, flavor::MoveFlavor, @@ -34,6 +28,24 @@ use external::ExternalDependency; use git::{GitRepo, PinnedGitDependency, UnpinnedGitDependency}; use local::LocalDependency; +mod dependency_set; +// TODO: this shouldn't be pub; need to move resolver error into resolver module +pub mod external; +mod git; +mod local; + +pub use dependency_set::DependencySet; + +// TODO (potential refactor): consider using objects for manifest dependencies (i.e. `Box`). +// part of the complexity here would be deserialization - probably need a flavor-specific +// function that converts a toml value to a Box +// +// resolution would also be interesting because of batch resolution. Would probably need a +// trait method to return a resolver object, and then a method on the resolver object to +// resolve a bunch of dependencies (resolvers could implement Eq) +// +// TODO: maybe rename ManifestDependencyInfo to UnpinnedDependency + /// Phantom type to represent pinned dependencies (see [PinnedDependency]) #[derive(Debug, PartialEq, Eq)] pub struct Pinned; @@ -45,6 +57,7 @@ pub struct Unpinned; /// [ManifestDependencyInfo]s contain the dependency-type-specific things that users write in their /// Move.toml files in the `dependencies` section. /// +/// TODO: this paragraph will change with upcoming design changes: /// There are additional general fields in the manifest format (like `override` or `rename-from`) /// that are not part of the ManifestDependencyInfo. We separate these partly because these things /// are not serialized to the Lock file. See [crate::package::manifest] for the full representation @@ -80,6 +93,8 @@ pub enum PinnedDependencyInfo { FlavorSpecific(F::FlavorDependency), } +// TODO: these should be moved down. + // UNPINNED impl<'de, F> Deserialize<'de> for ManifestDependencyInfo where @@ -107,7 +122,10 @@ where Ok(ManifestDependencyInfo::Local(dep)) } else { // TODO: maybe this could be prettier. The problem is that we don't know how to - // tell if something is a flavor dependency + // tell if something is a flavor dependency. One option might be to add a method to + // [MoveFlavor] that gives the list of flavor dependency tags. Another approach + // worth considering is removing flavor dependencies entirely and just having + // on-chain dependencies (with the flavor being used to resolve them). let dep = toml::Value::try_from(data) .map_err(de::Error::custom)? .try_into() @@ -184,6 +202,7 @@ fn split( (gits, exts, locs, flav) } +// TODO: this will change with upcoming design changes: /// Replace all dependencies with their pinned versions. The returned set may have a different set /// of keys than the input, for example if new implicit dependencies are added or if external /// resolvers resolve default deps to dep-replacements, or if dep-replacements are identical to the @@ -231,6 +250,7 @@ pub async fn pin( ])) } +// TODO: this will change with the upcoming design changes: /// For each environment, if none of the implicit dependencies are present in [deps] (or the /// default environment), then they are all added. // TODO: what's the notion of identity used here? @@ -250,3 +270,7 @@ fn fetch( todo!() } + +// TODO: unit tests +#[cfg(test)] +mod tests {} diff --git a/external-crates/move/crates/move-package-alt/src/jsonrpc/client.rs b/external-crates/move/crates/move-package-alt/src/jsonrpc/client.rs index 0d8fba74d140c..e359aa9e6900e 100644 --- a/external-crates/move/crates/move-package-alt/src/jsonrpc/client.rs +++ b/external-crates/move/crates/move-package-alt/src/jsonrpc/client.rs @@ -18,6 +18,7 @@ pub struct Endpoint { sqn: RequestID, } +// TODO: think if we keep errors here or move them in their own error module #[derive(Error, Debug)] pub enum JsonRpcError { #[error(transparent)] diff --git a/external-crates/move/crates/move-package-alt/src/package/manifest.rs b/external-crates/move/crates/move-package-alt/src/package/manifest.rs index f7abd2e8f58f5..0c58e6948ceec 100644 --- a/external-crates/move/crates/move-package-alt/src/package/manifest.rs +++ b/external-crates/move/crates/move-package-alt/src/package/manifest.rs @@ -19,6 +19,7 @@ use crate::{ use super::*; +// TODO: add 2025 edition const ALLOWED_EDITIONS: &[&str] = &["2024", "2024.beta", "legacy"]; // Note: [Manifest] objects are immutable and should not implement [serde::Serialize]; any tool @@ -96,6 +97,8 @@ impl Manifest { } /// Validate the manifest contents, after deserialization. + /// + // TODO: add more validation pub fn validate_manifest(&self, handle: FileHandle) -> PackageResult<()> { // Validate package name if self.package.name.get_ref().is_empty() { diff --git a/external-crates/move/crates/move-package-alt/src/package/mod.rs b/external-crates/move/crates/move-package-alt/src/package/mod.rs index eb180775bd4f5..4130ba5d1bbb1 100644 --- a/external-crates/move/crates/move-package-alt/src/package/mod.rs +++ b/external-crates/move/crates/move-package-alt/src/package/mod.rs @@ -22,6 +22,8 @@ use lockfile::{Lockfile, Publication}; use manifest::Manifest; use tracing::debug; +// TODO: we might want to use [move_core_types::Identifier] here, particularly for `PackageName`. +// This will force us to maintain invariants. pub type EnvironmentName = String; pub type PackageName = String;