NPV-169: detect too-broad with expressions; ratchet them down

src/files.rs

@@ -1,5 +1,10 @@
+use crate::problem::npv_169;
+use crate::ratchet::RatchetState;
 use relative_path::RelativePath;
 use relative_path::RelativePathBuf;
+
+use rnix::SyntaxKind;
+use rowan::ast::AstNode;
 use std::collections::BTreeMap;
 use std::path::Path;
 
@@ -8,46 +13,77 @@ 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
+/// The maximum number of non-trivia tokens allowed under a single `with` expression.
+const WITH_MAX_TOKENS: usize = 125;
+
+/// The maximum fraction of a file's non-trivia tokens that a single `with` expression may cover.
+const WITH_MAX_FILE_FRACTION: f64 = 0.25;
+
+/// Files with fewer than this many non-trivia tokens are exempt from the `with` check entirely.
+/// Small files don't benefit much from restricting `with` scope.
+const WITH_FILE_MIN_TOKENS: usize = 50;
+
+/// Counts the non-trivia (non-whitespace, non-comment) tokens under a syntax node.
+fn count_non_trivia_tokens(node: &rnix::SyntaxNode) -> usize {
+    node.descendants_with_tokens()
+        .filter(|element| element.as_token().is_some_and(|t| !t.kind().is_trivia()))
+        .count()
+}
+
+/// Finds the first `with` expression in the syntax tree that is overly broad, meaning it either:
+///
+/// - Contains more than [`WITH_MAX_TOKENS`] non-trivia tokens, or
+/// - Covers more than [`WITH_MAX_FILE_FRACTION`] of the file's total non-trivia tokens.
+///
+/// Files with fewer than [`WITH_FILE_MIN_TOKENS`] non-trivia tokens are exempt.
+///
+/// Large `with` scopes shadow variables across a wide region, making static analysis unreliable
+/// and code harder to understand. Small, tightly-scoped uses (e.g. `with lib.maintainers; [...]`)
+/// are fine.
+///
+/// Returns `Some(node)` for the first offending `with` node, or `None` if no such node exists.
+fn find_overly_broad_with(syntax: &rnix::SyntaxNode) -> Option<rnix::SyntaxNode> {
+    let file_tokens = count_non_trivia_tokens(syntax);
+
+    if file_tokens < WITH_FILE_MIN_TOKENS {
+        return None;
+    }
+
+    syntax
+        .descendants()
+        .filter(|node| node.kind() == SyntaxKind::NODE_WITH)
+        .find(|node| {
+            let with_tokens = count_non_trivia_tokens(node);
+            with_tokens > WITH_MAX_TOKENS
+                || with_tokens as f64 > WITH_MAX_FILE_FRACTION * file_tokens as f64
+        })
+}
+
+/// Runs ratchet checks on all Nix files in the Nixpkgs tree, 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| {
-        // 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, |relative_path, nix_file| {
+        Ok(Success(ratchet::File {
+            top_level_with: check_top_level_with(relative_path, nix_file),
+        }))
     })
 }
 
-/// 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()
-    }))
+/// Checks a single Nix file for overly broad `with` expressions. Returns [`RatchetState::Loose`]
+/// with a problem if such a `with` is found, or [`RatchetState::Tight`] if the file is clean.
+fn check_top_level_with(
+    relative_path: &RelativePath,
+    nix_file: &nix_file::NixFile,
+) -> RatchetState<ratchet::DoesNotIntroduceToplevelWiths> {
+    if find_overly_broad_with(nix_file.syntax_root.syntax()).is_some() {
+        RatchetState::Loose(
+            npv_169::OverlyBroadWith::new(relative_path.to_relative_path_buf()).into(),
+        )
+    } else {
+        RatchetState::Tight
+    }
 }
 
 /// Recursively collects all Nix files in the relative `dir` within `base`
@@ -63,7 +99,7 @@ fn collect_nix_files(
 
         let absolute_path = entry.path();
 
-        // We'll get to every file based on directory recursion, no need to follow symlinks.
+        // We reach every file via directory recursion, no need to follow symlinks.
         if absolute_path.is_symlink() {
             continue;
         }
@@ -75,3 +111,30 @@ fn collect_nix_files(
     }
     Ok(())
 }
+
+/// 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(&RelativePath, &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 file_results: Vec<validation::Validation<(RelativePathBuf, ratchet::File)>> = 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 val = f(&path, nix_file)?.map(|file| (path, file));
+            Ok::<_, anyhow::Error>(val)
+        })
+        .collect_vec()?;
+
+    Ok(validation::sequence(file_results).map(|entries| entries.into_iter().collect()))
+}

src/problem/mod.rs

@@ -36,6 +36,8 @@ pub mod npv_161;
 pub mod npv_162;
 pub mod npv_163;
 
+pub mod npv_169;
+
 #[derive(Clone, Display, EnumFrom)]
 pub enum Problem {
     /// NPV-100: attribute is not defined but it should be defined automatically
@@ -131,6 +133,9 @@ pub enum Problem {
     NewTopLevelPackageShouldBeByNameWithCustomArgument(
         npv_163::NewTopLevelPackageShouldBeByNameWithCustomArgument,
     ),
+
+    /// NPV-169: `with` expression covers too much of the file
+    OverlyBroadWith(npv_169::OverlyBroadWith),
 }
 
 fn indent_definition(column: usize, definition: &str) -> String {

src/problem/npv_169.rs

@@ -0,0 +1,32 @@
+use std::fmt;
+
+use indoc::writedoc;
+use relative_path::RelativePathBuf;
+
+#[derive(Clone)]
+pub struct OverlyBroadWith {
+    file: RelativePathBuf,
+}
+
+impl OverlyBroadWith {
+    pub fn new(file: RelativePathBuf) -> Self {
+        Self { file }
+    }
+}
+
+impl fmt::Display for OverlyBroadWith {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let Self { file } = self;
+        writedoc!(
+            f,
+            "
+            - {file}: `with` expression covers too much of the file.
+              Large-scoped `with` expressions shadow bindings and make static analysis unreliable.
+              Prefer one of these alternatives:
+              - Use fully qualified names, e.g. `lib.mkOption` instead of `with lib; mkOption`
+              - Use `inherit (lib) mkOption mkIf;` in a `let` block
+              - Limit `with` to small scopes, e.g. `maintainers = with lib.maintainers; [ ... ]`
+            ",
+        )
+    }
+}

src/ratchet.rs

@@ -62,16 +62,21 @@ impl Package {
     }
 }
 
-pub struct File {}
+/// The ratchet value for a single Nix file in the Nixpkgs tree.
+pub struct File {
+    /// The ratchet value for the check that files do not introduce top-level `with` expressions
+    /// that shadow scope.
+    pub top_level_with: RatchetState<DoesNotIntroduceToplevelWiths>,
+}
 
 impl File {
-    /// Validates the ratchet checks for a top-level package
-    pub fn compare(
-        _name: &RelativePath,
-        _optional_from: Option<&Self>,
-        _to: &Self,
-    ) -> Validation<()> {
-        Success(())
+    /// Validates the ratchet checks for a Nix file.
+    pub fn compare(name: &RelativePath, optional_from: Option<&Self>, to: &Self) -> Validation<()> {
+        validation::sequence_([RatchetState::compare(
+            name.as_str(),
+            optional_from.map(|x| &x.top_level_with),
+            &to.top_level_with,
+        )])
     }
 }
 
@@ -191,3 +196,20 @@ impl ToProblem for UsesByName {
         }
     }
 }
+
+/// The ratchet value for the check that Nix files do not introduce top-level `with` expressions
+/// whose body contains nested scope-defining constructs (other `with`s, `let...in`, or attribute
+/// sets).
+///
+/// Such `with` expressions shadow variables in the enclosing scope, making static analysis
+/// unreliable. This ratchet is tight when a file has no such `with` expressions, and loose when
+/// one is found.
+pub enum DoesNotIntroduceToplevelWiths {}
+
+impl ToProblem for DoesNotIntroduceToplevelWiths {
+    type ToContext = Problem;
+
+    fn to_problem(_name: &str, _optional_from: Option<()>, to: &Self::ToContext) -> Problem {
+        to.clone()
+    }
+}

tests/nowith-absolute-cap/expected

@@ -0,0 +1,8 @@
+- test.nix: `with` expression covers too much of the file.
+  Large-scoped `with` expressions shadow bindings and make static analysis unreliable.
+  Prefer one of these alternatives:
+  - Use fully qualified names, e.g. `lib.mkOption` instead of `with lib; mkOption`
+  - Use `inherit (lib) mkOption mkIf;` in a `let` block
+  - Limit `with` to small scopes, e.g. `maintainers = with lib.maintainers; [ ... ]`
+
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

tests/nowith-absolute-cap/main/default.nix

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