Lint cleanup (#127)

CONTRIBUTING.md

@@ -12,16 +12,33 @@ nix-shell
 ```
 
 The most important tools and commands in this environment are:
+
 - [rust-analyzer](https://rust-analyzer.github.io/) to have an IDE-like experience for your own editor.
+
 - Running tests:
+
   ```bash
   cargo test
   ```
-- Linting and formatting:
+
+- Formatting:
+
   ```bash
-  cargo clippy --all-targets
   treefmt
   ```
+
+- Linting:
+
+  ```bash
+  cargo clippy --all-targets
+  ```
+
+  Optionally, check out extra lints or uncomment them at the top of `main.rs`.
+
+  ```bash
+  cargo clippy --all-targets -- -W clippy::nursery -W clippy::pedantic
+  ```
+
 - Running the [main CI checks](./.github/workflows/main.yml) locally:
   ```bash
   nix-build -A ci

src/eval.rs

@@ -115,6 +115,7 @@ fn pass_through_environment_variables_for_nix_eval_in_nix_build(command: &mut pr
 }
 
 #[cfg(not(test))]
+#[allow(clippy::unnecessary_wraps)]
 fn mutate_nix_instatiate_arguments_based_on_cfg(
     _work_dir_path: &Path,
     command: &mut process::Command,
@@ -313,16 +314,13 @@ fn by_name(
                 // An automatic `callPackage` by the `pkgs/by-name` overlay.
                 // Though this gets detected by checking whether the internal
                 // `_internalCallByNamePackageFile` was used
-                DefinitionVariant::AutoDefinition => {
-                    if let Some(_location) = location {
-                        // Such an automatic definition should definitely not have a location.
-                        // Having one indicates that somebody is using
-                        // `_internalCallByNamePackageFile`,
-                        npv_102::ByNameInternalCallPackageUsed::new(attribute_name).into()
-                    } else {
-                        Success(Tight)
-                    }
-                }
+                DefinitionVariant::AutoDefinition => location.map_or_else(
+                    || Success(Tight),
+                    // Such an automatic definition should definitely not have a location.
+                    // Having one indicates that somebody is using
+                    // `_internalCallByNamePackageFile`,
+                    |_location| npv_102::ByNameInternalCallPackageUsed::new(attribute_name).into(),
+                ),
                 // The attribute is manually defined, e.g. in `all-packages.nix`.
                 // This means we need to enforce it to look like this:
                 //   callPackage ../pkgs/by-name/fo/foo/package.nix { ... }
@@ -454,8 +452,8 @@ fn handle_non_by_name_attribute(
     attribute_name: &str,
     non_by_name_attribute: NonByNameAttribute,
 ) -> validation::Result<ratchet::Package> {
-    use ratchet::RatchetState::*;
-    use NonByNameAttribute::*;
+    use ratchet::RatchetState::{Loose, NonApplicable, Tight};
+    use NonByNameAttribute::EvalSuccess;
 
     // The ratchet state whether this attribute uses `pkgs/by-name`.
     //

src/main.rs

@@ -1,10 +1,14 @@
+// Temporarily uncomment to view more pedantic or new nursery lints.
 // #![warn(clippy::pedantic)]
-// #![allow(clippy::uninlined_format_args)]
-// #![allow(clippy::enum_glob_use)]
-// #![allow(clippy::module_name_repetitions)]
-// #![allow(clippy::doc_markdown)]
 // #![allow(clippy::if_not_else)]
 // #![allow(clippy::ignored_unit_patterns)]
+// #![allow(clippy::module_name_repetitions)]
+// #![allow(clippy::uninlined_format_args)]
+// #![allow(clippy::unnested_or_patterns)]
+// #![warn(clippy::nursery)]
+// #![allow(clippy::use_self)]
+// #![allow(clippy::missing_const_for_fn)]
+
 mod eval;
 mod location;
 mod nix_file;
@@ -54,7 +58,7 @@ pub struct Args {
 
 fn main() -> ExitCode {
     let args = Args::parse();
-    let status: ColoredStatus = process(args.base, args.nixpkgs).into();
+    let status: ColoredStatus = process(args.base, &args.nixpkgs).into();
     eprintln!("{status}");
     status.into()
 }
@@ -64,10 +68,10 @@ fn main() -> ExitCode {
 /// # Arguments
 /// - `base_nixpkgs`: Path to the base Nixpkgs to run ratchet checks against.
 /// - `main_nixpkgs`: Path to the main Nixpkgs to check.
-fn process(base_nixpkgs: PathBuf, main_nixpkgs: PathBuf) -> Status {
+fn process(base_nixpkgs: PathBuf, main_nixpkgs: &Path) -> Status {
     // Very easy to parallelise this, since both operations are totally independent of each other.
     let base_thread = thread::spawn(move || check_nixpkgs(&base_nixpkgs));
-    let main_result = match check_nixpkgs(&main_nixpkgs) {
+    let main_result = match check_nixpkgs(main_nixpkgs) {
         Ok(result) => result,
         Err(error) => {
             return error.into();
@@ -88,7 +92,7 @@ fn process(base_nixpkgs: PathBuf, main_nixpkgs: PathBuf) -> Status {
         (Failure(..), Success(..)) => Status::BranchHealed,
         (Success(base), Success(main)) => {
             // Both base and main branch succeed. Check ratchet state between them...
-            match ratchet::Nixpkgs::compare(base, main) {
+            match ratchet::Nixpkgs::compare(&base, main) {
                 Failure(errors) => Status::DiscouragedPatternedIntroduced(errors),
                 Success(..) => Status::ValidatedSuccessfully,
             }
@@ -247,7 +251,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, &path)
         });
 
         let actual_errors = format!("{status}\n");

src/nix_file.rs

@@ -48,7 +48,7 @@ pub struct NixFile {
 }
 
 impl NixFile {
-    /// Creates a new NixFile, failing for I/O or parse errors.
+    /// Creates a new `NixFile`, failing for I/O or parse errors.
     fn new(path: impl AsRef<Path>) -> anyhow::Result<NixFile> {
         let Some(parent_dir) = path.as_ref().parent() else {
             anyhow::bail!("Could not get parent of path {}", path.as_ref().display())
@@ -78,7 +78,7 @@ impl NixFile {
 }
 
 /// Information about `callPackage` arguments.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Eq)]
 pub struct CallPackageArgumentInfo {
     /// The relative path of the first argument, or `None` if it's not a path.
     pub relative_path: Option<RelativePathBuf>,
@@ -116,7 +116,7 @@ impl NixFile {
     ///
     ///   results in `{ file = ./default.nix; line = 2; column = 3; }`
     ///
-    /// - Get the NixFile for `.file` from a `NixFileStore`
+    /// - Get the `NixFile` for `.file` from a `NixFileStore`
     ///
     /// - Call this function with `.line`, `.column` and `relative_to` as the (absolute) current
     ///   directory.
@@ -143,7 +143,7 @@ impl NixFile {
             Right(attrpath_value) => {
                 let definition = attrpath_value.to_string();
                 let attrpath_value =
-                    self.attrpath_value_call_package_argument_info(attrpath_value, relative_to)?;
+                    self.attrpath_value_call_package_argument_info(&attrpath_value, relative_to)?;
                 (attrpath_value, definition)
             }
         })
@@ -160,7 +160,7 @@ impl NixFile {
         let token_at_offset = self
             .syntax_root
             .syntax()
-            .token_at_offset(TextSize::from(index as u32));
+            .token_at_offset(TextSize::from(u32::try_from(index)?));
 
         // The token_at_offset function takes indices to mean a location _between_ characters,
         // which in this case is some spacing followed by the attribute name:
@@ -252,7 +252,7 @@ impl NixFile {
     // Internal function mainly to make `attrpath_value_at` independently testable.
     fn attrpath_value_call_package_argument_info(
         &self,
-        attrpath_value: ast::AttrpathValue,
+        attrpath_value: &ast::AttrpathValue,
         relative_to: &Path,
     ) -> anyhow::Result<Option<CallPackageArgumentInfo>> {
         let Some(attrpath) = attrpath_value.attrpath() else {
@@ -326,7 +326,7 @@ impl NixFile {
         // Check that <arg2> is a path expression.
         let path = if let Expr::Path(actual_path) = arg2 {
             // Try to statically resolve the path and turn it into a nixpkgs-relative path.
-            if let ResolvedPath::Within(p) = self.static_resolve_path(actual_path, relative_to) {
+            if let ResolvedPath::Within(p) = self.static_resolve_path(&actual_path, relative_to) {
                 Some(p)
             } else {
                 // We can't statically know an existing path inside Nixpkgs used as <arg2>.
@@ -417,7 +417,7 @@ impl NixFile {
     ///
     /// Given the path expression `./bar.nix` in `./foo.nix` and an absolute path of the
     /// current directory, the function returns `ResolvedPath::Within(./bar.nix)`.
-    pub fn static_resolve_path(&self, node: ast::Path, relative_to: &Path) -> ResolvedPath {
+    pub fn static_resolve_path(&self, node: &ast::Path, relative_to: &Path) -> ResolvedPath {
         if node.parts().count() != 1 {
             // If there's more than 1 interpolated part, it's of the form `./foo/${bar}/baz`.
             return ResolvedPath::Interpolated;
@@ -563,12 +563,7 @@ mod tests {
             (24, 11, Left("testL")),
         ];
         let expected = BTreeMap::from_iter(cases.map(|(line, column, result)| {
-            (
-                Position { line, column },
-                result
-                    .map(ToString::to_string)
-                    .map_right(|node| node.to_string()),
-            )
+            (Position { line, column }, result.map(ToString::to_string))
         }));
         let actual = BTreeMap::from_iter(cases.map(|(line, column, _)| {
             (

src/problem/npv_160.rs

@@ -24,11 +24,9 @@ impl fmt::Display for TopLevelPackageMovedOutOfByName {
             file,
         } = self;
         let relative_package_file = structure::relative_file_for_package(package_name);
-        let call_package_arg = if let Some(path) = call_package_path {
-            format!("./{}", path)
-        } else {
-            "...".into()
-        };
+        let call_package_arg = call_package_path
+            .as_ref()
+            .map_or_else(|| "...".into(), |path| format!("./{}", path));
         writedoc!(
             f,
             "

src/problem/npv_161.rs

@@ -24,11 +24,9 @@ impl fmt::Display for TopLevelPackageMovedOutOfByNameWithCustomArguments {
             file,
         } = self;
         let relative_package_file = structure::relative_file_for_package(package_name);
-        let call_package_arg = if let Some(path) = call_package_path {
-            format!("./{}", path)
-        } else {
-            "...".into()
-        };
+        let call_package_arg = call_package_path
+            .as_ref()
+            .map_or_else(|| "...".into(), |path| format!("./{}", path));
         writedoc!(
             f,
             "

src/problem/npv_162.rs

@@ -24,11 +24,9 @@ impl fmt::Display for NewTopLevelPackageShouldBeByName {
             file,
         } = self;
         let relative_package_file = structure::relative_file_for_package(package_name);
-        let call_package_arg = if let Some(path) = call_package_path {
-            format!("./{}", path)
-        } else {
-            "...".into()
-        };
+        let call_package_arg = call_package_path
+            .as_ref()
+            .map_or_else(|| "...".into(), |path| format!("./{}", path));
         writedoc!(
             f,
             "

src/problem/npv_163.rs

@@ -24,11 +24,9 @@ impl fmt::Display for NewTopLevelPackageShouldBeByNameWithCustomArgument {
             file,
         } = self;
         let relative_package_file = structure::relative_file_for_package(package_name);
-        let call_package_arg = if let Some(path) = call_package_path {
-            format!("./{}", path)
-        } else {
-            "...".into()
-        };
+        let call_package_arg = call_package_path
+            .as_ref()
+            .map_or_else(|| "...".into(), |path| format!("./{}", path));
         writedoc!(
             f,
             "

src/ratchet.rs

@@ -13,15 +13,15 @@ use crate::validation::{self, Validation, Validation::Success};
 /// The ratchet value for the entirety of Nixpkgs.
 #[derive(Default)]
 pub struct Nixpkgs {
-    /// Sorted list of packages in package_map
+    /// Sorted list of packages in `package_map`
     pub package_names: Vec<String>,
     /// The ratchet values for all packages
     pub package_map: HashMap<String, Package>,
 }
 
 impl Nixpkgs {
     /// Validates the ratchet checks for Nixpkgs
-    pub fn compare(from: Self, to: Self) -> Validation<()> {
+    pub fn compare(from: &Self, to: Self) -> Validation<()> {
         validation::sequence_(
             // We only loop over the current attributes,
             // we don't need to check ones that were removed
@@ -73,7 +73,7 @@ pub enum RatchetState<Ratchet: ToProblem> {
     /// 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.
+    /// This ratchet can't be applied. State transitions from/to `NonApplicable` are always allowed.
     NonApplicable,
 }
 

src/references.rs

@@ -5,6 +5,7 @@ use anyhow::Context;
 use relative_path::RelativePath;
 use rowan::ast::AstNode;
 
+use crate::nix_file::ResolvedPath;
 use crate::problem::{npv_121, npv_122, npv_123, npv_124, npv_125, npv_126};
 use crate::structure::read_dir_sorted;
 use crate::validation::{self, ResultIteratorExt, Validation::Success};
@@ -132,9 +133,7 @@ fn check_nix_file(
                 return Success(());
             };
 
-            use crate::nix_file::ResolvedPath;
-
-            match nix_file.static_resolve_path(path, absolute_package_dir) {
+            match nix_file.static_resolve_path(&path, absolute_package_dir) {
                 ResolvedPath::Interpolated => npv_121::NixFileContainsPathInterpolation::new(
                     relative_package_dir,
                     subpath,

src/structure.rs

@@ -106,7 +106,7 @@ pub fn check_structure(
                             path,
                             &shard_name,
                             shard_name_valid,
-                            package_entry,
+                            &package_entry,
                         )
                     })
                     .collect_vec()?;
@@ -125,7 +125,7 @@ fn check_package(
     path: &Path,
     shard_name: &str,
     shard_name_valid: bool,
-    package_entry: DirEntry,
+    package_entry: &DirEntry,
 ) -> validation::Result<String> {
     let package_path = package_entry.path();
     let package_name = package_entry.file_name().to_string_lossy().into_owned();

src/validation.rs

@@ -4,7 +4,7 @@ use itertools::{
     Either::{Left, Right},
     Itertools,
 };
-use Validation::*;
+use Validation::{Failure, Success};
 
 /// The validation result of a check.  Instead of exiting at the first failure, this type can
 /// accumulate multiple failures.  This can be achieved using the functions `and`, `sequence` and
@@ -25,7 +25,7 @@ impl<A, P: Into<Problem>> From<P> for Validation<A> {
 
 /// A type alias representing the result of a check, either:
 ///
-/// - Err(anyhow::Error): A fatal failure, typically I/O errors.
+/// - `Err(anyhow::Error)`: A fatal failure, typically I/O errors.
 ///   Such failures are not caused by the files in Nixpkgs.
 ///   This hints at a bug in the code or a problem with the deployment.
 ///