Implement package description checking

README.md

@@ -80,6 +80,11 @@ The following checks are performed when calling the binary:
 Evaluate Nixpkgs with `system` set to `x86_64-linux` and check that:
 - For each package directory, the `pkgs.${name}` attribute must be defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`.
 - For each package directory, `pkgs.lib.isDerivation pkgs.${name}` must be `true`.
+- For each top-level attribute, `meta.description` must:
+  - Start with a capital letter
+  - Not start with an article (a/an/the)
+  - Not start with the package name
+  - Not end with punctuation
 
 ### Ratchet checks
 

src/eval.nix

@@ -59,7 +59,18 @@ let
       else
         {
           AttributeSet = {
-            is_derivation = pkgs.lib.isDerivation value;
+            derivation =
+              if pkgs.lib.isDerivation value then
+                {
+                  Derivation = {
+                    pname = pkgs.lib.getName value;
+                    description = value.meta.description or null;
+                  };
+                }
+              else
+                {
+                  NonDerivation = null;
+                };
             definition_variant =
               if !value ? _callPackageVariant then
                 { ManualDefinition.is_semantic_call_package = false; }

src/eval.rs

@@ -79,12 +79,22 @@ pub enum AttributeVariant {
     NonAttributeSet,
     AttributeSet {
         /// Whether the attribute is a derivation (`lib.isDerivation`)
-        is_derivation: bool,
+        derivation: Derivation,
         /// The type of `callPackage` used.
         definition_variant: DefinitionVariant,
     },
 }
 
+/// Info about a derivation
+#[derive(Deserialize)]
+pub enum Derivation {
+    NonDerivation,
+    Derivation {
+        pname: String,
+        description: Option<String>,
+    },
+}
+
 #[derive(Deserialize)]
 pub enum DefinitionVariant {
     /// An automatic definition by the `pkgs/by-name` overlay, though it's detected using the
@@ -261,6 +271,61 @@ pub fn check_values(
     }))
 }
 
+/// Check the validity of a package description
+fn check_description(pname: &str, description: &Option<String>) -> ratchet::PackageDescription {
+    if let Some(text) = description {
+        let lowercased = text.to_lowercase();
+        ratchet::PackageDescription {
+            not_capitalised: {
+                if let Some(first) = text.chars().next() {
+                    if first.is_lowercase() {
+                        Loose(text.to_owned())
+                    } else {
+                        Tight
+                    }
+                } else {
+                    Tight
+                }
+            },
+            starts_with_article: {
+                if lowercased.starts_with("a ")
+                    || lowercased.starts_with("an ")
+                    || lowercased.starts_with("the ")
+                {
+                    Loose(text.to_owned())
+                } else {
+                    Tight
+                }
+            },
+            starts_with_package_name: {
+                if lowercased.starts_with(&pname.to_lowercase()) {
+                    Loose(text.to_owned())
+                } else {
+                    Tight
+                }
+            },
+            ends_with_punctuation: {
+                if let Some(last) = text.chars().last() {
+                    if last.is_ascii_punctuation() {
+                        Loose(text.to_owned())
+                    } else {
+                        Tight
+                    }
+                } else {
+                    Tight
+                }
+            },
+        }
+    } else {
+        ratchet::PackageDescription {
+            not_capitalised: Tight,
+            starts_with_article: Tight,
+            starts_with_package_name: Tight,
+            ends_with_punctuation: Tight,
+        }
+    }
+}
+
 /// Handle the evaluation result for an attribute in `pkgs/by-name`, making it a validation result.
 fn by_name(
     nix_file_store: &mut NixFileStore,
@@ -279,6 +344,27 @@ fn by_name(
         .into()
     };
 
+    let description_ratchet = match by_name_attribute {
+        Existing(AttributeInfo {
+            attribute_variant:
+                AttributeVariant::AttributeSet {
+                    derivation:
+                        Derivation::Derivation {
+                            pname: ref name,
+                            ref description,
+                        },
+                    ..
+                },
+            ..
+        }) => check_description(name, description),
+        _ => ratchet::PackageDescription {
+            not_capitalised: NonApplicable,
+            starts_with_article: NonApplicable,
+            starts_with_package_name: NonApplicable,
+            ends_with_punctuation: NonApplicable,
+        },
+    };
+
     // At this point we know that `pkgs/by-name/fo/foo/package.nix` has to exists.  This match
     // decides whether the attribute `foo` is defined accordingly and whether a legacy manual
     // definition could be removed.
@@ -308,16 +394,17 @@ fn by_name(
             // And it's an attribute set, which allows us to get more information about it
             attribute_variant:
                 AttributeVariant::AttributeSet {
-                    is_derivation,
+                    derivation,
                     definition_variant,
                 },
             location,
         }) => {
             // Only derivations are allowed in `pkgs/by-name`.
-            let is_derivation_result = if is_derivation {
-                Success(())
-            } else {
-                to_validation(ByNameErrorKind::NonDerivation).map(|_| ())
+            let is_derivation_result = match derivation {
+                Derivation::Derivation { .. } => Success(()),
+                Derivation::NonDerivation => {
+                    to_validation(ByNameErrorKind::NonDerivation).map(|_| ())
+                }
             };
 
             // If the definition looks correct
@@ -399,6 +486,7 @@ fn by_name(
         // once at the end with a map.
         manual_definition_result.map(|manual_definition| ratchet::Package {
             manual_definition,
+            description: description_ratchet,
             uses_by_name: Tight,
         }),
     )
@@ -477,6 +565,27 @@ fn handle_non_by_name_attribute(
     use ratchet::RatchetState::*;
     use NonByNameAttribute::*;
 
+    let description_ratchet = match non_by_name_attribute {
+        EvalSuccess(AttributeInfo {
+            attribute_variant:
+                AttributeVariant::AttributeSet {
+                    derivation:
+                        Derivation::Derivation {
+                            pname: ref name,
+                            ref description,
+                        },
+                    ..
+                },
+            ..
+        }) => check_description(name, description),
+        _ => ratchet::PackageDescription {
+            not_capitalised: NonApplicable,
+            starts_with_article: NonApplicable,
+            starts_with_package_name: NonApplicable,
+            ends_with_punctuation: NonApplicable,
+        },
+    };
+
     // The ratchet state whether this attribute uses `pkgs/by-name`.
     //
     // This is never `Tight`, because we only either:
@@ -504,7 +613,7 @@ fn handle_non_by_name_attribute(
             // are. Anything else can't be in `pkgs/by-name`.
             attribute_variant: AttributeVariant::AttributeSet {
                 // As of today, non-derivation attribute sets can't be in `pkgs/by-name`.
-                is_derivation: true,
+                derivation: Derivation::Derivation { .. },
                 // Of the two definition variants, really only the manual one makes sense here.
                 //
                 // Special cases are:
@@ -601,5 +710,6 @@ fn handle_non_by_name_attribute(
         // ourselves all the time to define `manual_definition`, just set it once at the end here.
         manual_definition: Tight,
         uses_by_name,
+        description: description_ratchet,
     }))
 }

src/nixpkgs_problem.rs

@@ -19,6 +19,7 @@ pub enum NixpkgsProblem {
     NixFile(NixFileError),
     TopLevelPackage(TopLevelPackageError),
     NixEval(NixEvalError),
+    Description(DescriptionError),
 }
 
 /// A file structure error involving a shard (e.g. `fo` is the shard in the path `pkgs/by-name/fo/foo/package.nix`)
@@ -146,6 +147,23 @@ pub struct NixEvalError {
     pub stderr: String,
 }
 
+/// An error relating to the description of a package
+#[derive(Clone)]
+pub struct DescriptionError {
+    pub package_name: String,
+    pub description: String,
+    pub kind: DescriptionErrorKind,
+}
+
+/// The kind of description problem
+#[derive(Clone)]
+pub enum DescriptionErrorKind {
+    NotCapitalised,
+    StartsWithArticle,
+    StartsWithPackageName,
+    EndsWithPunctuation,
+}
+
 impl fmt::Display for NixpkgsProblem {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
@@ -440,6 +458,15 @@ impl fmt::Display for NixpkgsProblem {
                 f.write_str(stderr)?;
                 write!(f, "- Nix evaluation failed for some package in `pkgs/by-name`, see error above")
             },
+            NixpkgsProblem::Description(DescriptionError { package_name, description, kind }) => {
+                let text = match kind {
+                    DescriptionErrorKind::NotCapitalised => "is not capitalised",
+                    DescriptionErrorKind::StartsWithArticle => "starts with an article (the/a/an)",
+                    DescriptionErrorKind::StartsWithPackageName => "starts with the package name",
+                    DescriptionErrorKind::EndsWithPunctuation => "ends with punctuation",
+                };
+                write!(f, "- Description for package {package_name} (\"{description}\") {text}")
+            },
        }
     }
 }

src/ratchet.rs

@@ -3,7 +3,8 @@
 //! Each type has a `compare` method that validates the ratchet checks for that item.
 
 use crate::nix_file::CallPackageArgumentInfo;
-use crate::nixpkgs_problem::{NixpkgsProblem, TopLevelPackageError};
+use crate::nixpkgs_problem::DescriptionErrorKind;
+use crate::nixpkgs_problem::{DescriptionError, NixpkgsProblem, TopLevelPackageError};
 use crate::validation::{self, Validation, Validation::Success};
 use relative_path::RelativePathBuf;
 use std::collections::HashMap;
@@ -37,6 +38,9 @@ pub struct Package {
 
     /// The ratchet value for the check for new packages using pkgs/by-name
     pub uses_by_name: RatchetState<UsesByName>,
+
+    /// The package description is valid
+    pub description: PackageDescription,
 }
 
 impl Package {
@@ -53,6 +57,11 @@ impl Package {
                 optional_from.map(|x| &x.uses_by_name),
                 &to.uses_by_name,
             ),
+            PackageDescription::compare(
+                name,
+                optional_from.map(|x| &x.description),
+                &to.description,
+            ),
         ])
     }
 }
@@ -166,3 +175,92 @@ impl ToNixpkgsProblem for UsesByName {
         })
     }
 }
+
+/// The ratchet value of an attribute for the check that the description is valid for new packages.
+///
+/// This checks that new packages don't use an indefinite article in `meta.description`.
+pub struct PackageDescription {
+    pub not_capitalised: RatchetState<DescriptionNotCapitalised>,
+    pub starts_with_article: RatchetState<DescriptionStartsWithArticle>,
+    pub starts_with_package_name: RatchetState<DescriptionStartsWithPackageName>,
+    pub ends_with_punctuation: RatchetState<DescriptionEndsWithPunctuation>,
+}
+
+impl PackageDescription {
+    pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
+        validation::sequence_([
+            RatchetState::<DescriptionNotCapitalised>::compare(
+                name,
+                optional_from.map(|x| &x.not_capitalised),
+                &to.not_capitalised,
+            ),
+            RatchetState::<DescriptionStartsWithArticle>::compare(
+                name,
+                optional_from.map(|x| &x.starts_with_article),
+                &to.starts_with_article,
+            ),
+            RatchetState::<DescriptionStartsWithPackageName>::compare(
+                name,
+                optional_from.map(|x| &x.starts_with_package_name),
+                &to.starts_with_package_name,
+            ),
+            RatchetState::<DescriptionEndsWithPunctuation>::compare(
+                name,
+                optional_from.map(|x| &x.ends_with_punctuation),
+                &to.ends_with_punctuation,
+            ),
+        ])
+    }
+}
+
+pub enum DescriptionNotCapitalised {}
+pub enum DescriptionStartsWithArticle {}
+pub enum DescriptionStartsWithPackageName {}
+pub enum DescriptionEndsWithPunctuation {}
+
+impl ToDescriptionErrorKind for DescriptionNotCapitalised {
+    fn to_description_error_kind() -> DescriptionErrorKind {
+        DescriptionErrorKind::NotCapitalised
+    }
+}
+
+impl ToDescriptionErrorKind for DescriptionStartsWithArticle {
+    fn to_description_error_kind() -> DescriptionErrorKind {
+        DescriptionErrorKind::StartsWithArticle
+    }
+}
+
+impl ToDescriptionErrorKind for DescriptionStartsWithPackageName {
+    fn to_description_error_kind() -> DescriptionErrorKind {
+        DescriptionErrorKind::StartsWithPackageName
+    }
+}
+
+impl ToDescriptionErrorKind for DescriptionEndsWithPunctuation {
+    fn to_description_error_kind() -> DescriptionErrorKind {
+        DescriptionErrorKind::EndsWithPunctuation
+    }
+}
+
+pub trait ToDescriptionErrorKind {
+    fn to_description_error_kind() -> DescriptionErrorKind;
+}
+
+impl<T> ToNixpkgsProblem for T
+where
+    T: ToDescriptionErrorKind,
+{
+    type ToContext = String;
+
+    fn to_nixpkgs_problem(
+        name: &str,
+        _optional_from: Option<()>,
+        description: &Self::ToContext,
+    ) -> NixpkgsProblem {
+        NixpkgsProblem::Description(DescriptionError {
+            package_name: name.to_owned(),
+            description: description.to_owned(),
+            kind: T::to_description_error_kind(),
+        })
+    }
+}

tests/description-already-nonconform/base/default.nix

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

tests/description-already-nonconform/base/expected

@@ -0,0 +1 @@
+Validated successfully