Merge pull request #283017 from tweag/by-name-fix-ratchet

tests.nixpkgs-check-by-name: Fix ratchet checks in certain cases

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

@@ -159,8 +159,8 @@ pub fn check_values(
                     let uses_by_name = match attribute_info {
                         // In these cases the package doesn't qualify for being in pkgs/by-name,
                         // so the UsesByName ratchet is already as tight as it can be
-                        NonAttributeSet => Success(Tight),
+                        NonAttributeSet => Success(NonApplicable),
-                        NonCallPackage => Success(Tight),
+                        NonCallPackage => Success(NonApplicable),
                         // This is the case when the `pkgs/by-name`-internal _internalCallByNamePackageFile
                         // is used for a package outside `pkgs/by-name`
                         CallPackage(CallPackageInfo {
@@ -176,14 +176,14 @@ pub fn check_values(
                             // In the future we could kind of abuse this behavior to have better
                             // enforcement of conditional aliases, but for now we just need to not
                             // give an error.
-                            Success(Tight)
+                            Success(NonApplicable)
                         }
                         // Only derivations can be in pkgs/by-name,
                         // so this attribute doesn't qualify
                         CallPackage(CallPackageInfo {
                             is_derivation: false,
                             ..
-                        }) => Success(Tight),
+                        }) => Success(NonApplicable),
 
                         // The case of an attribute that qualifies:
                         // - Uses callPackage
@@ -191,30 +191,35 @@ 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,
                     })
                 }
                 NonByName(EvalFailure) => {
-                    // This is a bit of an odd case: We don't even _know_ whether this attribute
+                    // We don't know anything about this attribute really
-                    // would qualify for using pkgs/by-name. We can either:
-                    // - Assume it's not using pkgs/by-name, which has the problem that if a
-                    //   package evaluation gets broken temporarily, the fix can remove it from
-                    //   pkgs/by-name again
-                    // - Assume it's using pkgs/by-name already, which has the problem that if a
-                    //   package evaluation gets broken temporarily, fixing it requires a move to
-                    //   pkgs/by-name
-                    // 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,
+                        // We'll assume that we can't remove any manual definitions, which has the
-                        uses_by_name: Tight,
+                        // minimal drawback that if there was a manual definition that could've
+                        // been removed, fixing the package requires removing the definition, no
+                        // big deal, that's a minor edit.
+                        manual_definition: Tight,
+
+                        // Regarding whether this attribute could `pkgs/by-name`, we don't really
+                        // know, so return NonApplicable, which has the effect that if a
+                        // package evaluation gets broken temporarily, the fix can remove it from
+                        // pkgs/by-name again. For now this isn't our problem, but in the future we
+                        // might have another check to enforce that evaluation must not be broken.
+                        // The alternative of assuming that it's using `pkgs/by-name` already
+                        // has the problem that if a package evaluation gets broken temporarily,
+                        // fixing it requires a move to pkgs/by-name, which could happen more
+                        // often and isn't really justified.
+                        uses_by_name: NonApplicable,
                     })
                 }
                 ByName(Missing) => NixpkgsProblem::UndefinedAttr {
@@ -248,7 +253,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 +266,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,55 +58,82 @@ 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.
     Tight,
+    /// This ratchet can't be applied.
+    /// State transitions from/to NonApplicable are always allowed
+    NonApplicable,
 }
 
 /// 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)
+            // - Anything involving NotApplicable, where we can't really make any good calls
+            _ => 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 +146,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 +157,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,
             }
         }
     }

pkgs/test/nixpkgs-check-by-name/tests/manual-definition/all-packages.nix

@@ -0,0 +1,10 @@
+self: super: {
+  nonAttributeSet = self.callPackage ({ someDrv }: someDrv) { };
+  nonCallPackage = self.callPackage ({ someDrv }: someDrv) { };
+  internalCallByName = self.callPackage ({ someDrv }: someDrv) { };
+  nonDerivation = self.callPackage ({ someDrv }: someDrv) { };
+
+  onlyMove = self.callPackage ./pkgs/by-name/on/onlyMove/package.nix { };
+
+  noEval = self.callPackage ./pkgs/by-name/no/noEval/package.nix { };
+}

pkgs/test/nixpkgs-check-by-name/tests/manual-definition/base/all-packages.nix

@@ -0,0 +1,9 @@
+self: super: {
+  nonAttributeSet = null;
+  nonCallPackage = self.someDrv;
+  internalCallByName = self._internalCallByNamePackageFile ./some-pkg.nix;
+  nonDerivation = self.callPackage ({ }: { }) { };
+
+  onlyMove = self.callPackage ({ someDrv }: someDrv) { };
+  noEval = throw "foo";
+}

pkgs/test/nixpkgs-check-by-name/tests/manual-definition/base/default.nix

@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }

pkgs/test/nixpkgs-check-by-name/tests/manual-definition/default.nix

@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }

pkgs/test/nixpkgs-check-by-name/tests/manual-definition/expected

@@ -0,0 +1,2 @@
+pkgs.noEval: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/no/noEval/package.nix { ... }` with a non-empty second argument.
+pkgs.onlyMove: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/on/onlyMove/package.nix { ... }` with a non-empty second argument.

pkgs/test/nixpkgs-check-by-name/tests/manual-definition/pkgs/by-name/no/noEval/package.nix

@@ -0,0 +1 @@
+{ someDrv }: someDrv

pkgs/test/nixpkgs-check-by-name/tests/manual-definition/pkgs/by-name/on/onlyMove/package.nix

@@ -0,0 +1 @@
+{ someDrv }: someDrv

pkgs/test/nixpkgs-check-by-name/tests/no-eval/all-packages.nix

@@ -1,3 +1,5 @@
 self: super: {
   iDontEval = throw "I don't eval";
+
+  futureEval = self.callPackage ({ someDrv }: someDrv) { };
 }

pkgs/test/nixpkgs-check-by-name/tests/no-eval/base/all-packages.nix

@@ -0,0 +1,3 @@
+self: super: {
+  futureEval = throw "foo";
+}

pkgs/test/nixpkgs-check-by-name/tests/no-eval/base/default.nix

@@ -0,0 +1 @@
+import <test-nixpkgs> { root = ./.; }

pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md

@@ -1 +0,0 @@
-(this is just here so the directory can get tracked by git)