Introduce bogus ratchet check ensuring that no new Nix string files are introduced

Author: infinisil

src/files.rs

@@ -13,9 +13,25 @@ pub fn check_files(
     nixpkgs_path: &Path,
     nix_file_store: &mut NixFileStore,
 ) -> validation::Result<BTreeMap<RelativePathBuf, ratchet::File>> {
-    process_nix_files(nixpkgs_path, nix_file_store, |_nix_file| {
-        // Noop for now, only boilerplate to make it easier to add future file-based checks
-        Ok(Success(ratchet::File {}))
+    process_nix_files(nixpkgs_path, nix_file_store, |nix_file| {
+        // Bogus ratchet check towards enforcing that no files are strings
+        let file_is_str = match nix_file.syntax_root.expr() {
+            // This happens if the file can't be parsed, in which case we can't really decide
+            // whether it's a string or not
+            None => ratchet::RatchetState::NonApplicable,
+            // The expression is a string, not allowed for new files and for existing files to be
+            // changed to a string
+            Some(Str(_)) => ratchet::RatchetState::Loose(
+                npv_170::FileIsAString::new(
+                    RelativePathBuf::from_path(nix_file.path.strip_prefix(nixpkgs_path).unwrap())
+                        .unwrap(),
+                )
+                .into(),
+            ),
+            // This is good
+            Some(_) => ratchet::RatchetState::Tight,
+        };
+        Ok(Success(ratchet::File { file_is_str }))
     })
 }
 

src/problem/mod.rs

@@ -34,6 +34,8 @@ pub mod npv_161;
 pub mod npv_162;
 pub mod npv_163;
 
+pub mod npv_170;
+
 #[derive(Clone, Display, EnumFrom)]
 pub enum Problem {
     /// NPV-100: attribute is not defined but it should be defined automatically
@@ -123,6 +125,8 @@ pub enum Problem {
     NewTopLevelPackageShouldBeByNameWithCustomArgument(
         npv_163::NewTopLevelPackageShouldBeByNameWithCustomArgument,
     ),
+
+    FileIsAString(npv_170::FileIsAString),
 }
 
 fn indent_definition(column: usize, definition: &str) -> String {

src/problem/npv_170.rs

@@ -0,0 +1,23 @@
+use std::fmt;
+
+use derive_new::new;
+use indoc::writedoc;
+use relative_path::RelativePathBuf;
+
+#[derive(Clone, new)]
+pub struct FileIsAString {
+    #[new(into)]
+    file: RelativePathBuf,
+}
+
+impl fmt::Display for FileIsAString {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let Self { file } = self;
+        writedoc!(
+            f,
+            "
+            - File {file} is a string, which is not allowed anymore
+            ",
+        )
+    }
+}

src/ratchet.rs

@@ -62,16 +62,19 @@ impl Package {
     }
 }
 
-pub struct File {}
+pub struct File {
+    pub file_is_str: RatchetState<FileIsStr>,
+}
 
 impl File {
     /// Validates the ratchet checks for a top-level package
-    pub fn compare(
-        _name: &RelativePath,
-        _optional_from: Option<&Self>,
-        _to: &Self,
-    ) -> Validation<()> {
-        Success(())
+    pub fn compare(name: &RelativePath, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
+        // TODO: It's not great how RatchetState::compare requires a str, it should be more generic
+        RatchetState::<FileIsStr>::compare(
+            name.as_str(),
+            optional_from.map(|x| &x.file_is_str),
+            &to.file_is_str,
+        )
     }
 }
 
@@ -126,6 +129,16 @@ impl<Context: ToProblem> RatchetState<Context> {
     }
 }
 
+pub enum FileIsStr {}
+
+impl ToProblem for FileIsStr {
+    type ToContext = Problem;
+
+    fn to_problem(_name: &str, _optional_from: Option<()>, to: &Self::ToContext) -> Problem {
+        (*to).clone()
+    }
+}
+
 /// The ratchet to check whether a top-level attribute has/needs a manual definition, e.g. in
 /// `pkgs/top-level/all-packages.nix`.
 ///

tests/no-strings-changed/base/default.nix

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

tests/no-strings-changed/base/lib/minver.nix

@@ -0,0 +1,3 @@
+{
+  not = "a string";
+}

tests/no-strings-changed/expected

@@ -0,0 +1,3 @@
+- File lib/minver.nix is a string, which is not allowed anymore
+
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

tests/no-strings-changed/main/default.nix

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

tests/no-strings-changed/main/lib/minver.nix

@@ -0,0 +1 @@
+"1.0"

tests/no-strings-maintained/base/default.nix

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

tests/no-strings-maintained/base/lib/minver.nix

@@ -0,0 +1 @@
+"1.0"

tests/no-strings-maintained/expected

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

tests/no-strings-maintained/main/default.nix

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

tests/no-strings-maintained/main/lib/minver.nix

@@ -0,0 +1 @@
+"1.0"

tests/no-strings-new/base/default.nix

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

tests/no-strings-new/expected

@@ -0,0 +1,3 @@
+- File lib/minver.nix is a string, which is not allowed anymore
+
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

tests/no-strings-new/main/default.nix

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

tests/no-strings-new/main/lib/minver.nix

@@ -0,0 +1 @@
+"1.0"