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,50 +13,62 @@ 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
+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| {
+                    if child == *node {
+                        return None;
+                    }
+                    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()
+}
+
 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_files_top_level_with_lib(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(
+fn check_files_top_level_with_lib(
     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(open_scope_with_lib) = find_invalid_withs(nix_file.syntax_root.syntax()) {
+        RatchetState::Loose(
+            {
+                npv_169::TopLevelWithMayShadowVariablesAndBreakStaticChecks::new(
+                    RelativePathBuf::from_path(
+                        nix_file.path.clone().strip_prefix(nixpkgs_path).unwrap(),
+                    )
+                    .unwrap(),
+                    open_scope_with_lib.to_string(),
+                )
+            }
+            .into(),
+        )
+    } else {
+        RatchetState::Tight
+    }
 }
 
-/// Recursively collects all Nix files in the relative `dir` within `base`
-/// into the `files` `Vec`.
 fn collect_nix_files(
     base: &Path,
     dir: &RelativePath,
@@ -63,7 +80,6 @@ fn collect_nix_files(
 
         let absolute_path = entry.path();
 
-        // We'll get to every file based on directory recursion, no need to follow symlinks.
         if absolute_path.is_symlink() {
             continue;
         }
@@ -75,3 +91,26 @@ fn collect_nix_files(
     }
     Ok(())
 }
+
+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>> {
+    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| {
+            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,18 @@ impl Package {
     }
 }
 
-pub struct File {}
+pub struct File {
+    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(())
+    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 +193,16 @@ impl ToProblem for UsesByName {
         }
     }
 }
+
+// The ratchet value of an attribute for the check that new nixpkgs changes do not
+// introduce top level with or withs that could shadow scope.
+
+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 ];
+  };
+}