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,72 @@ 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
+/// Finds the first `with` expression in the syntax tree whose body contains another scope-defining
+/// construct (`with`, `let...in`, or attribute set). Such `with` expressions are problematic because
+/// they shadow variables in the enclosing scope, making it impossible for static analysis tools to
+/// determine which bindings are in scope.
+///
+/// Returns `Some(node)` for the first offending `with` node, or `None` if no such node exists.
+fn find_invalid_withs(syntax: &rnix::SyntaxNode) -> Option<rnix::SyntaxNode> {
+    syntax
+        .descendants()
+        .filter(|node| node.kind() == rnix::SyntaxKind::NODE_WITH)
+        .filter(|node| {
+            node.descendants()
+                .map(|child| {
+                    // Skip the `with` node itself; we only care about its descendants.
+                    if child == *node {
+                        return None;
+                    }
+                    // Any of these nested constructs means the `with` may shadow bindings
+                    // that the inner scope tries to reference.
+                    match child.kind() {
+                        SyntaxKind::NODE_WITH => Some(node),
+                        SyntaxKind::NODE_LET_IN => Some(node),
+                        SyntaxKind::NODE_ATTR_SET => Some(node),
+                        _ => None,
+                    }
+                })
+                .any(|node| node.is_some())
+        })
+        .take(1)
+        .last()
+}
+
+/// 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_invalid_withs(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 +94,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 +106,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 @@
+Validated successfully

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 ];
+  };
+}