Merge master into staging-next

1 files changed, 57 insertions, 38 deletions

diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
index c12f1ead25402..85feb0eee62f3 100644
--- a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
+++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs
@@ -10,31 +10,20 @@ use std::collections::HashMap;
 /// The ratchet value for the entirety of Nixpkgs.
 #[derive(Default)]
 pub struct Nixpkgs {
-    /// The ratchet values for each package in `pkgs/by-name`
-    pub packages: HashMap<String, Package>,
+    /// Sorted list of attributes in package_map
+    pub package_names: Vec<String>,
+    /// The ratchet values for all packages
+    pub package_map: HashMap<String, Package>,
 }
 
 impl Nixpkgs {
     /// Validates the ratchet checks for Nixpkgs
-    pub fn compare(optional_from: Option<Self>, to: Self) -> Validation<()> {
+    pub fn compare(from: Self, to: Self) -> Validation<()> {
         validation::sequence_(
             // We only loop over the current attributes,
             // we don't need to check ones that were removed
-            to.packages.into_iter().map(|(name, attr_to)| {
-                let attr_from = if let Some(from) = &optional_from {
-                    from.packages.get(&name)
-                } else {
-                    // This pretends that if there's no base version to compare against, all
-                    // attributes existed without conforming to the new strictness check for
-                    // backwards compatibility.
-                    // TODO: Remove this case. This is only needed because the `--base`
-                    // argument is still optional, which doesn't need to be once CI is updated
-                    // to pass it.
-                    Some(&Package {
-                        empty_non_auto_called: EmptyNonAutoCalled::Invalid,
-                    })
-                };
-                Package::compare(&name, attr_from, &attr_to)
+            to.package_names.into_iter().map(|name| {
+                Package::compare(&name, from.package_map.get(&name), &to.package_map[&name])
             }),
         )
     }
@@ -43,13 +32,13 @@ impl Nixpkgs {
 /// The ratchet value for a single package in `pkgs/by-name`
 pub struct Package {
     /// The ratchet value for the check for non-auto-called empty arguments
-    pub empty_non_auto_called: EmptyNonAutoCalled,
+    pub empty_non_auto_called: RatchetState<EmptyNonAutoCalled>,
 }
 
 impl Package {
     /// Validates the ratchet checks for a single package defined in `pkgs/by-name`
     pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
-        EmptyNonAutoCalled::compare(
+        RatchetState::<EmptyNonAutoCalled>::compare(
             name,
             optional_from.map(|x| &x.empty_non_auto_called),
             &to.empty_non_auto_called,
@@ -57,29 +46,59 @@ impl Package {
     }
 }
 
-/// The ratchet value of a single package in `pkgs/by-name`
+/// The ratchet state of a generic ratchet check.
+pub enum RatchetState<Context> {
+    /// The ratchet is loose, it can be tightened more.
+    /// In other words, this is the legacy state we're trying to move away from.
+    /// Introducing new instances is not allowed but previous instances will continue to be allowed.
+    /// The `Context` is context for error messages in case a new instance of this state is
+    /// introduced
+    Loose(Context),
+    /// The ratchet is tight, it can't be tightened any further.
+    /// This is either because we already use the latest state, or because the ratchet isn't
+    /// relevant.
+    Tight,
+}
+
+/// A trait that can convert an attribute-specific error context into a NixpkgsProblem
+pub trait ToNixpkgsProblem {
+    /// How to convert an attribute-specific error context into a NixpkgsProblem
+    fn to_nixpkgs_problem(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem;
+}
+
+impl<Context: ToNixpkgsProblem> RatchetState<Context> {
+    /// Compare the previous ratchet state of an attribute to the new state.
+    /// The previous state may be `None` in case the attribute is new.
+    fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
+        // If we don't have a previous state, enforce a tight ratchet
+        let from = optional_from.unwrap_or(&RatchetState::Tight);
+        match (from, to) {
+            // Always okay to keep it tight or tighten the ratchet
+            (_, RatchetState::Tight) => Success(()),
+
+            // Grandfathering policy for a loose ratchet
+            (RatchetState::Loose { .. }, RatchetState::Loose { .. }) => Success(()),
+
+            // Loosening a ratchet is now allowed
+            (RatchetState::Tight, RatchetState::Loose(context)) => {
+                Context::to_nixpkgs_problem(name, context, optional_from.is_some()).into()
+            }
+        }
+    }
+}
+
+/// The ratchet value of an attribute
 /// for the non-auto-called empty argument check of a single.
 ///
 /// This checks that packages defined in `pkgs/by-name` cannot be overridden
 /// with an empty second argument like `callPackage ... { }`.
-#[derive(PartialEq, PartialOrd)]
-pub enum EmptyNonAutoCalled {
-    Invalid,
-    Valid,
-}
+pub struct EmptyNonAutoCalled;
 
-impl EmptyNonAutoCalled {
-    /// Validates the non-auto-called empty argument ratchet check for a single package defined in `pkgs/by-name`
-    fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
-        let from = optional_from.unwrap_or(&Self::Valid);
-        if to >= from {
-            Success(())
-        } else {
-            NixpkgsProblem::WrongCallPackage {
-                relative_package_file: structure::relative_file_for_package(name),
-                package_name: name.to_owned(),
-            }
-            .into()
+impl ToNixpkgsProblem for EmptyNonAutoCalled {
+    fn to_nixpkgs_problem(name: &str, _context: &Self, _existed_before: bool) -> NixpkgsProblem {
+        NixpkgsProblem::WrongCallPackage {
+            relative_package_file: structure::relative_file_for_package(name),
+            package_name: name.to_owned(),
         }
     }
 }