NPV-169: detect top-level with expressions that shadow scope

Add a ratchet check that prevents introducing new top-level `with`
expressions whose body contains attr sets, let-in bindings, or nested
with expressions. Existing instances are grandfathered via the ratchet.

Co-authored-by: Lucas Eduardo Wendt <lucas59356@gmail.com>

Author: philiptaron

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,75 @@ 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;
+
+/// 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.
+///
+/// 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);
+
+    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, |nix_file| {
+        Ok(Success(ratchet::File {
+            top_level_with: check_top_level_with(nixpkgs_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(
+/// Checks a single Nix file for top-level `with` expressions that contain nested scope-defining
+/// constructs. 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(
     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()
-    }))
+    nix_file: &nix_file::NixFile,
+) -> RatchetState<ratchet::DoesNotIntroduceToplevelWiths> {
+    if let Some(offending_with) = find_overly_broad_with(nix_file.syntax_root.syntax()) {
+        let relative_path =
+            RelativePathBuf::from_path(nix_file.path.clone().strip_prefix(nixpkgs_path).unwrap())
+                .unwrap();
+        RatchetState::Loose(
+            npv_169::TopLevelWithMayShadowVariablesAndBreakStaticChecks::new(
+                relative_path,
+                offending_with.to_string(),
+            )
+            .into(),
+        )
+    } else {
+        RatchetState::Tight
+    }
 }
 
 /// Recursively collects all Nix files in the relative `dir` within `base`
@@ -63,7 +97,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 +109,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<F: Fn(&nix_file::NixFile) -> validation::Result<ratchet::File>>(
+    nixpkgs_path: &Path,
+    nix_file_store: &mut NixFileStore,
+    f: F,
+) -> 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(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,11 @@ pub enum Problem {
     NewTopLevelPackageShouldBeByNameWithCustomArgument(
         npv_163::NewTopLevelPackageShouldBeByNameWithCustomArgument,
     ),
+
+    /// NPV-169: top-level with may shadow variables and break static checks
+    TopLevelWithMayShadowVariablesAndBreakStaticChecks(
+        npv_169::TopLevelWithMayShadowVariablesAndBreakStaticChecks,
+    ),
 }
 
 fn indent_definition(column: usize, definition: &str) -> String {

src/problem/npv_169.rs

@@ -0,0 +1,24 @@
+use relative_path::RelativePathBuf;
+use std::fmt;
+
+#[derive(Clone)]
+pub struct TopLevelWithMayShadowVariablesAndBreakStaticChecks {
+    file: RelativePathBuf,
+    node: String,
+}
+
+impl TopLevelWithMayShadowVariablesAndBreakStaticChecks {
+    pub fn new(file: RelativePathBuf, node: String) -> Self {
+        Self { file, node }
+    }
+}
+
+impl fmt::Display for TopLevelWithMayShadowVariablesAndBreakStaticChecks {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        let Self { file, node: _node } = self;
+        write!(
+            f,
+            "- {file}: Top level with is discouraged as it may shadow variables and break static checks."
+        )
+    }
+}

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-basic/expected

@@ -0,0 +1,2 @@
+- test.nix: Top level with is discouraged as it may shadow variables and break static checks.
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

tests/nowith-basic/main/default.nix

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

tests/nowith-basic/main/test.nix

@@ -0,0 +1,3 @@
+with lib;
+
+2

tests/nowith-meta-toplevel-lib/expected

@@ -0,0 +1,2 @@
+- test.nix: Top level with is discouraged as it may shadow variables and break static checks.
+This PR introduces additional instances of discouraged patterns as listed above. Merging is discouraged but would not break the base branch.

tests/nowith-meta-toplevel-lib/main/default.nix

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

tests/nowith-meta-toplevel-lib/main/test.nix

@@ -0,0 +1,15 @@
+{
+  stdenv,
+  lib,
+}:
+
+stdenv.mkDerivation {
+  pname = "test";
+  version = "1.0";
+
+  src = ./.;
+
+  meta = with lib; {
+    maintainers = [ maintainers.johndoe ];
+  };
+}