Fix most clippy pedantic and nursery lint warnings

CONTRIBUTING.md

@@ -17,11 +17,18 @@ The most important tools and commands in this environment are:
   ```bash
   cargo test
   ```
-- Linting and formatting:
+- Formatting:
   ```bash
-  cargo clippy --all-targets
   treefmt
   ```
+- Linting:
+  ```bash
+  cargo clippy --all-targets
+  ```
+  Or optionally:
+  ```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,
@@ -314,14 +315,15 @@ fn by_name(
                 // 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)
-                    }
+                    location.map_or_else(
+                        || Success(Tight),
+                        |_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()
+                        },
+                    )
                 }
                 // The attribute is manually defined, e.g. in `all-packages.nix`.
                 // This means we need to enforce it to look like this:
@@ -352,8 +354,7 @@ fn by_name(
                             )
                             .with_context(|| {
                                 format!(
-                                    "Failed to get the definition info for attribute {}",
-                                    attribute_name
+                                    "Failed to get the definition info for attribute {attribute_name}"
                                 )
                             })?;
 
@@ -454,8 +455,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`.
     //
@@ -527,10 +528,14 @@ fn handle_non_by_name_attribute(
                 nixpkgs_path
             )
             .with_context(|| {
-                format!("Failed to get the definition info for attribute {}", attribute_name)
+                format!("Failed to get the definition info for attribute {attribute_name}")
             })?;
 
         // At this point, we completed two different checks for whether it's a `callPackage`.
+
+        // Allow pedantic lint: https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
+        // fixing it hurts readability of code due to comment restructuring
+        #[allow(clippy::unnested_or_patterns)]
         match (is_semantic_call_package, optional_syntactic_call_package) {
             // Something like `<attr> = { }`
             (false, None)

src/location.rs

@@ -28,7 +28,7 @@ pub struct LineIndex {
 }
 
 impl LineIndex {
-    pub fn new(s: &str) -> LineIndex {
+    pub fn new(s: &str) -> Self {
         let mut newlines = vec![];
         let mut index = 0;
         // Iterates over all newline-split parts of the string, adding the index of the newline to
@@ -37,7 +37,7 @@ impl LineIndex {
             index += split.len();
             newlines.push(index - 1);
         }
-        LineIndex { newlines }
+        Self { newlines }
     }
 
     /// Returns the line number for a string index.

src/main.rs

@@ -1,10 +1,3 @@
-// #![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)]
 mod eval;
 mod location;
 mod nix_file;
@@ -54,7 +47,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 +57,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 +81,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 +240,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,8 +48,8 @@ pub struct NixFile {
 }
 
 impl NixFile {
-    /// Creates a new NixFile, failing for I/O or parse errors.
-    fn new(path: impl AsRef<Path>) -> anyhow::Result<NixFile> {
+    /// Creates a new `NixFile`, failing for I/O or parse errors.
+    fn new(path: impl AsRef<Path>) -> anyhow::Result<Self> {
         let Some(parent_dir) = path.as_ref().parent() else {
             anyhow::bail!("Could not get parent of path {}", path.as_ref().display())
         };
@@ -67,7 +67,7 @@ impl NixFile {
             // rnix's `rnix::Parse::ok` returns `Result<_, _>`, so no error is thrown away like it
             // would be with `std::result::Result::ok`.
             .ok()
-            .map(|syntax_root| NixFile {
+            .map(|syntax_root| Self {
                 parent_dir: parent_dir.to_path_buf(),
                 path: path.as_ref().to_owned(),
                 syntax_root,
@@ -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,
 }
 
@@ -92,12 +92,12 @@ impl<Context: ToProblem> RatchetState<Context> {
     fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
         match (optional_from, to) {
             // Loosening a ratchet is not allowed.
-            (Some(RatchetState::Tight), RatchetState::Loose(loose_context)) => {
+            (Some(Self::Tight), Self::Loose(loose_context)) => {
                 Context::to_problem(name, Some(()), loose_context).into()
             }
 
             // Introducing a loose ratchet is also not allowed.
-            (None, RatchetState::Loose(loose_context)) => {
+            (None, Self::Loose(loose_context)) => {
                 Context::to_problem(name, None, loose_context).into()
             }