tests.nixpkgs-check-by-name: Minor code improvements

- Renames EmptyNonAutoCalled to ManualDefinition and add some docs to
  better explain what it's for
- Don't conflate the Ratchet type with the Context type, keep them
  apart, making the code a bit cleaner, but also allows adding
  additional context for a Tight ratchet in the future
- Change the signature of to_nixpkgs_problem to align with the other
  ratchet function signatures

pkgs/test/nixpkgs-check-by-name/src/eval.rs

@@ -191,13 +191,13 @@ pub fn check_values(
                         CallPackage(CallPackageInfo {
                             is_derivation: true,
                             call_package_variant: Manual { path, empty_arg },
-                        }) => Success(Loose(ratchet::UsesByName {
+                        }) => Success(Loose(ratchet::CouldUseByName {
                             call_package_path: path,
                             empty_arg,
                         })),
                     };
                     uses_by_name.map(|x| ratchet::Package {
-                        empty_non_auto_called: Tight,
+                        manual_definition: Tight,
                         uses_by_name: x,
                     })
                 }
@@ -213,7 +213,7 @@ pub fn check_values(
                     // We choose the latter, since we want to move towards pkgs/by-name, not away
                     // from it
                     Success(ratchet::Package {
-                        empty_non_auto_called: Tight,
+                        manual_definition: Tight,
                         uses_by_name: Tight,
                     })
                 }
@@ -248,7 +248,7 @@ pub fn check_values(
 
                     check_result.and(match &call_package_variant {
                         Auto => Success(ratchet::Package {
-                            empty_non_auto_called: Tight,
+                            manual_definition: Tight,
                             uses_by_name: Tight,
                         }),
                         Manual { path, empty_arg } => {
@@ -261,11 +261,7 @@ pub fn check_values(
                             if correct_file {
                                 Success(ratchet::Package {
                                     // Empty arguments for non-auto-called packages are not allowed anymore.
-                                    empty_non_auto_called: if *empty_arg {
+                                    manual_definition: if *empty_arg { Loose(()) } else { Tight },
-                                        Loose(ratchet::EmptyNonAutoCalled)
-                                    } else {
-                                        Tight
-                                    },
                                     uses_by_name: Tight,
                                 })
                             } else {

pkgs/test/nixpkgs-check-by-name/src/ratchet.rs

@@ -33,7 +33,7 @@ impl Nixpkgs {
 /// The ratchet value for a top-level package
 pub struct Package {
     /// The ratchet value for the check for non-auto-called empty arguments
-    pub empty_non_auto_called: RatchetState<EmptyNonAutoCalled>,
+    pub manual_definition: RatchetState<ManualDefinition>,
 
     /// The ratchet value for the check for new packages using pkgs/by-name
     pub uses_by_name: RatchetState<UsesByName>,
@@ -43,10 +43,10 @@ impl Package {
     /// Validates the ratchet checks for a top-level package
     pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
         validation::sequence_([
-            RatchetState::<EmptyNonAutoCalled>::compare(
+            RatchetState::<ManualDefinition>::compare(
                 name,
-                optional_from.map(|x| &x.empty_non_auto_called),
+                optional_from.map(|x| &x.manual_definition),
-                &to.empty_non_auto_called,
+                &to.manual_definition,
             ),
             RatchetState::<UsesByName>::compare(
                 name,
@@ -58,13 +58,13 @@ impl Package {
 }
 
 /// The ratchet state of a generic ratchet check.
-pub enum RatchetState<Context> {
+pub enum RatchetState<Ratchet: ToNixpkgsProblem> {
     /// 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),
+    Loose(Ratchet::ToContext),
     /// 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.
@@ -73,40 +73,63 @@ pub enum RatchetState<Context> {
 
 /// A trait that can convert an attribute-specific error context into a NixpkgsProblem
 pub trait ToNixpkgsProblem {
+    /// Context relating to the Nixpkgs that is being transitioned _to_
+    type ToContext;
+
     /// How to convert an attribute-specific error context into a NixpkgsProblem
-    fn to_nixpkgs_problem(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem;
+    fn to_nixpkgs_problem(
+        name: &str,
+        optional_from: Option<()>,
+        to: &Self::ToContext,
+    ) -> 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
+        match (optional_from, to) {
-        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)) => {
+            (Some(RatchetState::Tight), RatchetState::Loose(loose_context)) => {
-                Context::to_nixpkgs_problem(name, context, optional_from.is_some()).into()
+                Context::to_nixpkgs_problem(name, Some(()), loose_context).into()
             }
+
+            // Introducing a loose ratchet is also not allowed
+            (None, RatchetState::Loose(loose_context)) => {
+                Context::to_nixpkgs_problem(name, None, loose_context).into()
+            }
+
+            // Everything else is allowed, including:
+            // - Loose -> Loose (grandfathering policy for a loose ratchet)
+            // - -> Tight (always okay to keep or make the ratchet tight)
+            _ => Success(()),
         }
     }
 }
 
-/// The ratchet value of an attribute
+/// The ratchet to check whether a top-level attribute has/needs
-/// for the non-auto-called empty argument check of a single.
+/// a manual definition, e.g. in all-packages.nix.
 ///
-/// This checks that packages defined in `pkgs/by-name` cannot be overridden
+/// This ratchet is only tight for attributes that:
-/// with an empty second argument like `callPackage ... { }`.
+/// - Are not defined in `pkgs/by-name`, and rely on a manual definition
-pub struct EmptyNonAutoCalled;
+/// - Are defined in `pkgs/by-name` without any manual definition,
+///   (no custom argument overrides)
+/// - Are defined with `pkgs/by-name` with a manual definition that can't be removed
+///   because it provides custom argument overrides
+///
+/// In comparison, this ratchet is loose for attributes that:
+/// - Are defined in `pkgs/by-name` with a manual definition
+///   that doesn't have any custom argument overrides
+pub enum ManualDefinition {}
 
-impl ToNixpkgsProblem for EmptyNonAutoCalled {
+impl ToNixpkgsProblem for ManualDefinition {
-    fn to_nixpkgs_problem(name: &str, _context: &Self, _existed_before: bool) -> NixpkgsProblem {
+    type ToContext = ();
+
+    fn to_nixpkgs_problem(
+        name: &str,
+        _optional_from: Option<()>,
+        _to: &Self::ToContext,
+    ) -> NixpkgsProblem {
         NixpkgsProblem::WrongCallPackage {
             relative_package_file: structure::relative_file_for_package(name),
             package_name: name.to_owned(),
@@ -119,8 +142,10 @@ impl ToNixpkgsProblem for EmptyNonAutoCalled {
 ///
 /// This checks that all new package defined using callPackage must be defined via pkgs/by-name
 /// It also checks that once a package uses pkgs/by-name, it can't switch back to all-packages.nix
+pub enum UsesByName {}
+
 #[derive(Clone)]
-pub struct UsesByName {
+pub struct CouldUseByName {
     /// The first callPackage argument, used for better errors
     pub call_package_path: Option<PathBuf>,
     /// Whether the second callPackage argument is empty, used for better errors
@@ -128,18 +153,24 @@ pub struct UsesByName {
 }
 
 impl ToNixpkgsProblem for UsesByName {
-    fn to_nixpkgs_problem(name: &str, a: &Self, existed_before: bool) -> NixpkgsProblem {
+    type ToContext = CouldUseByName;
-        if existed_before {
+
+    fn to_nixpkgs_problem(
+        name: &str,
+        optional_from: Option<()>,
+        to: &Self::ToContext,
+    ) -> NixpkgsProblem {
+        if let Some(()) = optional_from {
             NixpkgsProblem::MovedOutOfByName {
                 package_name: name.to_owned(),
-                call_package_path: a.call_package_path.clone(),
+                call_package_path: to.call_package_path.clone(),
-                empty_arg: a.empty_arg,
+                empty_arg: to.empty_arg,
             }
         } else {
             NixpkgsProblem::NewPackageNotUsingByName {
                 package_name: name.to_owned(),
-                call_package_path: a.call_package_path.clone(),
+                call_package_path: to.call_package_path.clone(),
-                empty_arg: a.empty_arg,
+                empty_arg: to.empty_arg,
             }
         }
     }