Prevent failure while creating BuildPlan from lock file in the case of a dependency removal

forc-pkg/src/manifest.rs

@@ -334,6 +334,13 @@ impl Manifest {
             .and_then(|deps| deps.get(dep_name))
     }
 
+    /// Retrieve the listed patches for the given name.
+    pub fn patch(&self, patch_name: &str) -> Option<&BTreeMap<std::string::String, Dependency>> {
+        self.patch
+            .as_ref()
+            .and_then(|patches| patches.get(patch_name))
+    }
+
     /// Finds and returns the name of the dependency associated with a package of the specified
     /// name if there is one.
     ///

forc-pkg/src/pkg.rs

@@ -5,7 +5,7 @@ use crate::{
 use anyhow::{anyhow, bail, Context, Error, Result};
 use forc_util::{
     find_file_name, git_checkouts_directory, kebab_to_snake_case, print_on_failure,
-    print_on_success, print_on_success_library, println_yellow_err,
+    print_on_success, print_on_success_library, println_red, println_yellow_err,
 };
 use fuel_tx::StorageSlot;
 use fuels_types::JsonABI;
@@ -242,8 +242,13 @@ impl BuildPlan {
         sway_git_tag: &str,
     ) -> Result<Self> {
         let lock_path = forc_util::lock_path(manifest.dir());
-        let plan_result = BuildPlan::from_lock_file(&lock_path, sway_git_tag);
+        let from_lock = BuildPlan::from_lock_file(&lock_path, sway_git_tag);
+        let removed_deps = from_lock
+            .as_ref()
+            .map(|from_lock| from_lock.1.clone())
+            .unwrap_or_default();
 
+        let plan_result = from_lock.map(|from_lock| from_lock.0);
         // Retrieve the old lock file state so we can produce a diff.
         let old_lock = plan_result
             .as_ref()
@@ -267,10 +272,10 @@ impl BuildPlan {
         // If there are no issues with the BuildPlan generated from the lock file
         // Check and apply the diff.
         if new_lock_cause.is_none() {
-            let diff = plan.validate(manifest, sway_git_tag)?;
-            if !diff.added.is_empty() || !diff.removed.is_empty() {
+            let diff = plan.generate_diff(manifest)?;
+            if !diff.added.is_empty() || !diff.removed.is_empty() || !removed_deps.is_empty() {
                 new_lock_cause = Some(anyhow!("lock file did not match manifest `diff`"));
-                plan = plan.apply_pkg_diff(diff, sway_git_tag, offline)?;
+                plan = plan.apply_pkg_diff(&diff, sway_git_tag, offline)?;
             }
         }
 
@@ -293,12 +298,13 @@ impl BuildPlan {
     }
 
     /// Create a new build plan from an existing one. Needs the difference with the existing plan with the lock.
-    pub fn apply_pkg_diff(
+    fn apply_pkg_diff(
         &self,
-        pkg_diff: PkgDiff,
+        pkg_diff: &PkgDiff,
         sway_git_tag: &str,
         offline_mode: bool,
     ) -> Result<Self> {
+        self.validate(sway_git_tag)?;
         let mut graph = self.graph.clone();
         let mut path_map = self.path_map.clone();
 
@@ -307,7 +313,7 @@ impl BuildPlan {
             .last()
             .ok_or_else(|| anyhow!("Invalid Graph"))?;
         let PkgDiff { added, removed } = pkg_diff;
-        remove_deps(&mut graph, &path_map, proj_node, &removed);
+        remove_deps(&mut graph, &path_map, proj_node, removed);
 
         let mut visited_map: HashMap<Pinned, NodeIx> = graph
             .node_references()
@@ -319,12 +325,13 @@ impl BuildPlan {
             &mut graph,
             &mut path_map,
             &self.compilation_order,
-            &added,
+            added,
             sway_git_tag,
             offline_mode,
             &mut visited_map,
         )?;
         let compilation_order = compilation_order(&graph)?;
+        self.validate(sway_git_tag)?;
         Ok(Self {
             graph,
             path_map,
@@ -333,26 +340,58 @@ impl BuildPlan {
     }
 
     /// Attempt to load the build plan from the `Lock`.
-    pub fn from_lock(proj_path: &Path, lock: &Lock, sway_git_tag: &str) -> Result<Self> {
-        let graph = lock.to_graph()?;
+    /// Returns the best effort BuildPlan and the packages removed
+    /// from project's manifest file after the lock file is generated.
+    ///
+    /// The returned removed dependencies are already removed from the dependency graph.
+    fn from_lock(
+        proj_path: &Path,
+        lock: &Lock,
+        sway_git_tag: &str,
+    ) -> Result<(Self, Vec<DependencyName>)> {
+        let mut graph = lock.to_graph()?;
+        let tmp_compilation_order = compilation_order(&graph)?;
+        let (path_map, removed_deps) =
+            graph_to_path_map(proj_path, &graph, &tmp_compilation_order, sway_git_tag)?;
+        let removed_deps_vec: Vec<DependencyName> = removed_deps
+            .clone()
+            .into_iter()
+            .map(|dep| graph[dep].name.clone())
+            .collect();
+
+        // Remove the unresolveable path dependencies from the graph
+        apply_diff_after_lock(removed_deps, &mut graph)?;
+
+        // Since apply_diff_after_lock removes removed dependencies from the graph compilation order may change
         let compilation_order = compilation_order(&graph)?;
-        let path_map = graph_to_path_map(proj_path, &graph, &compilation_order, sway_git_tag)?;
-        Ok(Self {
-            graph,
-            path_map,
-            compilation_order,
-        })
+        Ok((
+            Self {
+                graph,
+                path_map,
+                compilation_order,
+            },
+            removed_deps_vec,
+        ))
     }
 
     /// Attempt to load the build plan from the `Forc.lock` file.
-    pub fn from_lock_file(lock_path: &Path, sway_git_tag: &str) -> Result<Self> {
+    /// Returns the best effort BuildPlan and the packages removed  
+    /// from project's manifest file after the lock file is generated.
+    ///
+    /// The returned removed dependencies are already removed from the dependency graph.
+    pub fn from_lock_file(
+        lock_path: &Path,
+        sway_git_tag: &str,
+    ) -> Result<(Self, Vec<DependencyName>)> {
         let proj_path = lock_path.parent().unwrap();
         let lock = Lock::from_path(lock_path)?;
         Self::from_lock(proj_path, &lock, sway_git_tag)
     }
 
-    /// Ensure that the build plan is valid for the given manifest.
-    pub fn validate(&self, manifest: &Manifest, sway_git_tag: &str) -> Result<PkgDiff> {
+    /// Attempt to find the difference between the graph constructed from the lock file
+    /// and the graph that is described by the manifest file. The difference is used to
+    /// update BuildPlan.
+    pub fn generate_diff(&self, manifest: &Manifest) -> Result<PkgDiff> {
         let mut added = vec![];
         let mut removed = vec![];
         // Retrieve project's graph node.
@@ -383,7 +422,7 @@ impl BuildPlan {
                     if det.version.is_some() {
                         println_yellow_err(&format!(
                             "  WARNING! Dependency \"{}\" specifies the unused `version` field: \
-                            consider using `branch` or `tag` instead",
+                                consider using `branch` or `tag` instead",
                             dep_name
                         ));
                     }
@@ -411,6 +450,11 @@ impl BuildPlan {
                 .collect();
         }
 
+        Ok(PkgDiff { added, removed })
+    }
+
+    /// Ensure that the build plan is valid for the given manifest.
+    fn validate(&self, sway_git_tag: &str) -> Result<()> {
         // Ensure the pkg names of all nodes match their associated manifests.
         for node in self.graph.node_indices() {
             let pkg = &self.graph[node];
@@ -425,7 +469,7 @@ impl BuildPlan {
                 );
             }
         }
-        Ok(PkgDiff { added, removed })
+        Ok(())
     }
 
     /// View the build plan's compilation graph.
@@ -809,20 +853,58 @@ pub fn compilation_order(graph: &Graph) -> Result<Vec<NodeIx>> {
     })
 }
 
+/// Applies the differences between the graph that is described by the lock file and the graph that
+/// is actually possible to build.
+///
+/// `deps_to_remove` : Indices of path dependencies that are removed from their parent's manifest file after the lock file is generated.
+///
+/// There might be some differences between the graph that is described by the lock file and
+/// the graph that can actually possible to build. This is happening in the case of a dependency
+/// (described by path) removal after the lock file is generated. Since dependency is removed
+/// graph_to_path_map cannot find the required path for the dependency entry in the lock file.
+/// This requires us to remove those removed nodes from the graph so that we can have a BuildPlan
+/// generated from lock file. After the BuildPlan is generated it can be used to apply diffs by
+/// apply_pkg_diff().
+fn apply_diff_after_lock(deps_to_remove: HashSet<NodeIx>, graph: &mut Graph) -> Result<()> {
+    for dep in deps_to_remove {
+        let dep_name = &graph[dep].name.clone();
+        println_red(&format!("  Removing {}", dep_name));
+        // Remove the deleted node
+        graph.remove_node(dep);
+    }
+    Ok(())
+}
+/// Search dependencies and patches for the details for a pinned dependency
+fn check_manifest_for_pinned(manifest: &ManifestFile, pinned_dep: &Pinned) -> bool {
+    manifest.dep(&pinned_dep.name).is_none()
+        && !manifest
+            .deps()
+            .filter_map(|dep| dep.1.package())
+            .any(|package| package == pinned_dep.name)
+        && manifest.patch(&pinned_dep.name).is_none()
+        && manifest
+            .patches()
+            .find_map(|patches| patches.1.get(&pinned_dep.name))
+            .is_none()
+}
+
 /// Given graph of pinned dependencies and the directory for the root node, produce a path map
 /// containing the path to the local source for every node in the graph.
+///
+/// Returns the generated path map and the nodes that should be removed from the graph (the nodes
+/// that are removed from their parent's manifest file).
 pub fn graph_to_path_map(
     proj_manifest_dir: &Path,
     graph: &Graph,
     compilation_order: &[NodeIx],
     sway_git_tag: &str,
-) -> Result<PathMap> {
+) -> Result<(PathMap, HashSet<NodeIx>)> {
     let mut path_map = PathMap::new();
+    let mut removed_deps = HashSet::new();
 
     // We resolve all paths in reverse compilation order.
     // That is, we follow paths starting from the project root.
     let mut path_resolve_order = compilation_order.iter().cloned().rev();
-
     // Add the project's package to the map.
     let proj_node = path_resolve_order
         .next()
@@ -869,6 +951,13 @@ pub fn graph_to_path_map(
                 // Construct the path relative to the parent's path.
                 let parent_path = &path_map[&parent.id()];
                 let parent_manifest = ManifestFile::from_dir(parent_path, sway_git_tag)?;
+                // Check if parent's manifest file includes this dependency.
+                // If not this is a removed dependency and we should continue with the following dep
+                // Note that we need to check from package name as well since dependency name and package name is not the same thing
+                if check_manifest_for_pinned(&parent_manifest, dep) {
+                    removed_deps.insert(dep_node);
+                    continue;
+                }
                 let detailed = parent_manifest
                     .dependencies
                     .as_ref()
@@ -916,7 +1005,7 @@ pub fn graph_to_path_map(
         path_map.insert(dep.id(), dep_path.canonicalize()?);
     }
 
-    Ok(path_map)
+    Ok((path_map, removed_deps))
 }
 
 /// Given a `graph`, the node index of a path dependency within that `graph`, and the supposed

sway-lsp/src/core/document.rs

@@ -162,7 +162,7 @@ impl TextDocument {
             pkg::ManifestFile::from_dir(&manifest_dir, forc::utils::SWAY_GIT_TAG).unwrap();
         let lock_path = forc_util::lock_path(manifest.dir());
         let plan = pkg::BuildPlan::from_lock_file(&lock_path, forc::utils::SWAY_GIT_TAG).unwrap();
-        let res = pkg::check(&plan, silent_mode, forc::utils::SWAY_GIT_TAG).unwrap();
+        let res = pkg::check(&plan.0, silent_mode, forc::utils::SWAY_GIT_TAG).unwrap();
 
         match res {
             CompileAstResult::Failure { .. } => None,