fix: don't panic on Range::none() dependencies (#55)

* fix: don't panic on Range::none() dependencies

* fix: check packages don't depend on themselves

* style: reword ForbiddenEmptyDependency description

* style: rename newly introduced error types

Co-authored-by: Alex Tokarev <[email protected]>

Author: Eh2406

src/error.rs

@@ -43,6 +43,34 @@ pub enum PubGrubError<P: Package, V: Version> {
         source: Box<dyn std::error::Error>,
     },
 
+    /// Error arising when the implementer of
+    /// [DependencyProvider](crate::solver::DependencyProvider)
+    /// returned a dependency on an empty range.
+    /// This technically means that the package can not be selected,
+    /// but is clearly some kind of mistake.
+    #[error("Package {dependent} required by {package} {version} depends on the empty set")]
+    DependencyOnTheEmptySet {
+        /// Package whose dependencies we want.
+        package: P,
+        /// Version of the package for which we want the dependencies.
+        version: V,
+        /// The dependent package that requires us to pick from the empty set.
+        dependent: P,
+    },
+
+    /// Error arising when the implementer of
+    /// [DependencyProvider](crate::solver::DependencyProvider)
+    /// returned a dependency on the requested package.
+    /// This technically means that the package directly depends on itself,
+    /// and is clearly some kind of mistake.
+    #[error("{package} {version} depends on itself")]
+    SelfDependency {
+        /// Package whose dependencies we want.
+        package: P,
+        /// Version of the package for which we want the dependencies.
+        version: V,
+    },
+
     /// Error arising when the implementer of [DependencyProvider](crate::solver::DependencyProvider)
     /// returned an error in the method [should_cancel](crate::solver::DependencyProvider::should_cancel).
     #[error("We should cancel")]

src/solver.rs

@@ -140,7 +140,22 @@ pub fn resolve<P: Package, V: Version>(
                 });
                 continue;
             }
-            Dependencies::Known(x) => x,
+            Dependencies::Known(x) => {
+                if x.contains_key(&p) {
+                    return Err(PubGrubError::SelfDependency {
+                        package: p.clone(),
+                        version: v.clone(),
+                    });
+                }
+                if let Some((dependent, _)) = x.iter().find(|(_, r)| r == &&Range::none()) {
+                    return Err(PubGrubError::DependencyOnTheEmptySet {
+                        package: p.clone(),
+                        version: v.clone(),
+                        dependent: dependent.clone(),
+                    });
+                }
+                x
+            }
         };
 
         // Add that package and version if the dependencies are not problematic.

tests/proptest.rs

@@ -326,8 +326,8 @@ proptest! {
                     (Ok(l), Ok(r)) => assert_eq!(l, r),
                     (Err(PubGrubError::NoSolution(derivation_l)), Err(PubGrubError::NoSolution(derivation_r))) => {
                         prop_assert_eq!(
-                            DefaultStringReporter::report(&derivation_l).to_string(),
-                            DefaultStringReporter::report(&derivation_r).to_string()
+                            DefaultStringReporter::report(&derivation_l),
+                            DefaultStringReporter::report(&derivation_r)
                         )},
                     _ => panic!("not the same result")
                 }

tests/tests.rs

@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: MPL-2.0
 
+use pubgrub::error::PubGrubError;
 use pubgrub::range::Range;
 use pubgrub::solver::{resolve, OfflineDependencyProvider};
 use pubgrub::version::NumberVersion;
@@ -25,3 +26,29 @@ fn same_result_on_repeated_runs() {
         }
     }
 }
+
+#[test]
+fn should_always_find_a_satisfier() {
+    let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new();
+    dependency_provider.add_dependencies("a", 0, vec![("b", Range::none())]);
+    assert!(matches!(
+        resolve(&dependency_provider, "a", 0),
+        Err(PubGrubError::DependencyOnTheEmptySet { .. })
+    ));
+
+    dependency_provider.add_dependencies("c", 0, vec![("a", Range::any())]);
+    assert!(matches!(
+        resolve(&dependency_provider, "c", 0),
+        Err(PubGrubError::DependencyOnTheEmptySet { .. })
+    ));
+}
+
+#[test]
+fn cannot_depend_on_self() {
+    let mut dependency_provider = OfflineDependencyProvider::<_, NumberVersion>::new();
+    dependency_provider.add_dependencies("a", 0, vec![("a", Range::any())]);
+    assert!(matches!(
+        resolve(&dependency_provider, "a", 0),
+        Err(PubGrubError::SelfDependency { .. })
+    ));
+}