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

CONTRIBUTING.md

@@ -47,31 +47,32 @@ The most important tools and commands in this environment are:
 ### Integration tests
 
 Integration tests are declared in [`./tests`](./tests) as subdirectories imitating Nixpkgs with these files:
-- `default.nix`:
-  Always contains
-  ```nix
-  import <test-nixpkgs> { root = ./.; }
-  ```
-  which makes
-  ```
-  nix-instantiate <subdir> --eval -A <attr> --arg overlays <overlays>
-  ```
-  work very similarly to the real Nixpkgs, just enough for the program to be able to test it.
-- `pkgs/by-name`:
-  The `pkgs/by-name` directory to check.
-
-- `pkgs/top-level/all-packages.nix` (optional):
-  Contains an overlay of the form
-  ```nix
-  self: super: {
-    # ...
-  }
-  ```
-  allowing the simulation of package overrides to the real [`pkgs/top-level/all-packages.nix`](https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/all-packages.nix).
-  The default is an empty overlay.
+- `main`: A Nixpkgs root directory with:
+  - `default.nix`:
+    Always contains
+    ```nix
+    import <test-nixpkgs> { root = ./.; }
+    ```
+    which makes
+    ```
+    nix-instantiate <subdir> --eval -A <attr> --arg overlays <overlays>
+    ```
+    work very similarly to the real Nixpkgs, just enough for the program to be able to test it.
+  - `pkgs/by-name`:
+    The `pkgs/by-name` directory to check.
+
+  - `pkgs/top-level/all-packages.nix` (optional):
+    Contains an overlay of the form
+    ```nix
+    self: super: {
+      # ...
+    }
+    ```
+    allowing the simulation of package overrides to the real [`pkgs/top-level/all-packages.nix`](https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/all-packages.nix).
+    The default is an empty overlay.
 
 - `base` (optional):
-  Contains another subdirectory imitating Nixpkgs with potentially any of the above structures.
+  Contains another Nixpkgs root directory with potentially any of the above structures.
   This is used to test [ratchet checks](./README.md#ratchet-checks).
 
 - `expected` (optional):

src/eval.rs

@@ -1,3 +1,4 @@
+use std::collections::BTreeMap;
 use std::path::{Path, PathBuf};
 use std::{env, fs, process};
 
@@ -156,7 +157,7 @@ pub fn check_values(
     nixpkgs_path: &Path,
     nix_file_store: &mut NixFileStore,
     package_names: &[String],
-) -> validation::Result<ratchet::Nixpkgs> {
+) -> validation::Result<BTreeMap<String, ratchet::Package>> {
     let work_dir = tempfile::Builder::new()
         .prefix("nixpkgs-vet")
         .tempdir()
@@ -255,9 +256,7 @@ pub fn check_values(
             .collect_vec()?,
     );
 
-    Ok(check_result.map(|elems| ratchet::Nixpkgs {
-        packages: elems.into_iter().collect(),
-    }))
+    Ok(check_result.map(|elems| elems.into_iter().collect()))
 }
 
 /// Handle the evaluation result for an attribute in `pkgs/by-name`, making it a validation result.

src/files.rs

@@ -0,0 +1,93 @@
+use relative_path::RelativePath;
+use relative_path::RelativePathBuf;
+use std::collections::BTreeMap;
+use std::path::Path;
+
+use crate::nix_file::NixFileStore;
+use crate::validation::ResultIteratorExt;
+use crate::validation::Validation::Success;
+use crate::{nix_file, ratchet, structure, validation};
+
+/// Runs check on all Nix files, returning a ratchet result for each
+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| {
+        // 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 }))
+    })
+}
+
+/// Processes all Nix files in a Nixpkgs directory according to a given function `f`, collecting the
+/// results into a mapping from each file to a ratchet value.
+fn process_nix_files(
+    nixpkgs_path: &Path,
+    nix_file_store: &mut NixFileStore,
+    f: impl Fn(&nix_file::NixFile) -> validation::Result<ratchet::File>,
+) -> validation::Result<BTreeMap<RelativePathBuf, ratchet::File>> {
+    // Get all Nix files
+    let files = {
+        let mut files = vec![];
+        collect_nix_files(nixpkgs_path, &RelativePathBuf::new(), &mut files)?;
+        files
+    };
+
+    let results = files
+        .into_iter()
+        .map(|path| {
+            // Get the (optionally-cached) parsed Nix file
+            let nix_file = nix_file_store.get(&path.to_path(nixpkgs_path))?;
+            let result = f(nix_file)?;
+            let val = result.map(|ratchet| (path, ratchet));
+            Ok::<_, anyhow::Error>(val)
+        })
+        .collect_vec()?;
+
+    Ok(validation::sequence(results).map(|entries| {
+        // Convert the Vec to a BTreeMap
+        entries.into_iter().collect()
+    }))
+}
+
+/// Recursively collects all Nix files in the relative `dir` within `base`
+/// into the `files` `Vec`.
+fn collect_nix_files(
+    base: &Path,
+    dir: &RelativePath,
+    files: &mut Vec<RelativePathBuf>,
+) -> anyhow::Result<()> {
+    for entry in structure::read_dir_sorted(&dir.to_path(base))? {
+        let mut relative_path = dir.to_relative_path_buf();
+        relative_path.push(entry.file_name().to_string_lossy().into_owned());
+
+        let absolute_path = entry.path();
+
+        // We'll get to every file based on directory recursion, no need to follow symlinks.
+        if absolute_path.is_symlink() {
+            continue;
+        }
+        if absolute_path.is_dir() {
+            collect_nix_files(base, &relative_path, files)?
+        } else if absolute_path.extension().is_some_and(|x| x == "nix") {
+            files.push(relative_path)
+        }
+    }
+    Ok(())
+}

src/main.rs

@@ -10,6 +10,7 @@
 // #![allow(clippy::missing_const_for_fn)]
 
 mod eval;
+mod files;
 mod location;
 mod nix_file;
 mod problem;
@@ -21,6 +22,7 @@ mod validation;
 
 use anyhow::Context as _;
 use clap::Parser;
+use std::collections::BTreeMap;
 use std::path::{Path, PathBuf};
 use std::process::ExitCode;
 use std::{panic, thread};
@@ -113,20 +115,30 @@ fn check_nixpkgs(nixpkgs_path: &Path) -> validation::Result<ratchet::Nixpkgs> {
         )
     })?;
 
-    if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() {
-        // No pkgs/by-name directory, always valid
-        return Ok(Success(ratchet::Nixpkgs::default()));
-    }
-
     let mut nix_file_store = NixFileStore::default();
-    let structure = check_structure(&nixpkgs_path, &mut nix_file_store)?;
 
-    // Only if we could successfully parse the structure, we do the evaluation checks
-    let result = structure.result_map(|package_names| {
-        eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names.as_slice())
-    })?;
+    let package_result = {
+        if !nixpkgs_path.join(structure::BASE_SUBPATH).exists() {
+            // No pkgs/by-name directory, always valid
+            Success(BTreeMap::new())
+        } else {
+            let structure = check_structure(&nixpkgs_path, &mut nix_file_store)?;
+
+            // Only if we could successfully parse the structure, we do the evaluation checks
+            structure.result_map(|package_names| {
+                eval::check_values(&nixpkgs_path, &mut nix_file_store, package_names.as_slice())
+            })?
+        }
+    };
+
+    let file_result = files::check_files(&nixpkgs_path, &mut nix_file_store)?;
 
-    Ok(result)
+    Ok(
+        package_result.and(file_result, |packages, files| ratchet::Nixpkgs {
+            packages,
+            files,
+        }),
+    )
 }
 
 #[cfg(test)]
@@ -178,7 +190,7 @@ mod tests {
             return Ok(());
         }
 
-        let base = path.join(BASE_SUBPATH);
+        let base = path.join("main").join(BASE_SUBPATH);
 
         fs::create_dir_all(base.join("fo/foo"))?;
         fs::write(base.join("fo/foo/package.nix"), "{ someDrv }: someDrv")?;
@@ -237,7 +249,7 @@ mod tests {
             .build()
             .expect("valid regex");
 
-        let path = path.to_owned();
+        let main_path = path.join("main");
         let base_path = path.join("base");
         let base_nixpkgs = if base_path.exists() {
             base_path
@@ -251,7 +263,7 @@ mod tests {
         let nix_conf_dir = nix_conf_dir.path().as_os_str();
 
         let status = temp_env::with_var("NIX_CONF_DIR", Some(nix_conf_dir), || {
-            process(base_nixpkgs, &path)
+            process(base_nixpkgs, &main_path)
         });
 
         let actual_errors = format!("{status}\n");

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

@@ -2,6 +2,7 @@
 //!
 //! Each type has a `compare` method that validates the ratchet checks for that item.
 
+use relative_path::RelativePath;
 use std::collections::BTreeMap;
 
 use relative_path::RelativePathBuf;
@@ -15,6 +16,7 @@ use crate::validation::{self, Validation, Validation::Success};
 pub struct Nixpkgs {
     /// The ratchet values for all packages
     pub packages: BTreeMap<String, Package>,
+    pub files: BTreeMap<RelativePathBuf, File>,
 }
 
 impl Nixpkgs {
@@ -27,6 +29,9 @@ impl Nixpkgs {
                 .into_iter()
                 .map(|(name, pkg)| Package::compare(&name, from.packages.get(&name), &pkg)),
         )
+        .and_(validation::sequence_(to.files.into_iter().map(
+            |(name, file)| File::compare(&name, from.files.get(&name), &file),
+        )))
     }
 }
 
@@ -57,6 +62,22 @@ impl Package {
     }
 }
 
+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<()> {
+        // 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,
+        )
+    }
+}
+
 /// The ratchet state of a generic ratchet check.
 pub enum RatchetState<Ratchet: ToProblem> {
     /// The ratchet is loose. It can be tightened more. In other words, this is the legacy state
@@ -108,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/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/lib/minver.nix

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

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/lib/minver.nix

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

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/lib/minver.nix

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

tests/success/main/default.nix

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

tests/symlink-escape/main/default.nix

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

tests/symlink-invalid/main/default.nix

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

tests/unknown-location/main/default.nix

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

tests/uppercase/main/default.nix

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