docs(xatu): note deprecated toPath resolution follow-up

src/xatu/BUILD

@@ -8,6 +8,7 @@ rust_library(
         "file_store.rs",
         "parser.rs",
         "rules.rs",
+        "rules/deprecated_to_path.rs",
         "rules/empty_inherit.rs",
         "xatu.rs",
     ],
@@ -29,12 +30,14 @@ rust_test(
     srcs = ["xatu_test.rs"],
     data = [
         "testdata/clean.nix",
-        "testdata/empty_inherit.nix",
         "testdata/invalid.nix",
+        "testdata/lint_findings.nix",
+        "testdata/lint_variants.nix",
     ],
     deps = [
         ":xatu_lib",
         "//src/status:status",
+        "//third_party/crates:googletest",
     ],
 )
 

src/xatu/ast.rs

@@ -1,13 +1,16 @@
 use crate::FileId;
 
-#[derive(Debug, Clone, PartialEq, Eq)]
-pub struct Inherit {
-    pub attr_count: usize,
+#[derive(Debug)]
+pub struct ParsedFile {
+    pub file_id: FileId,
+    pub nodes: Vec<Node>,
+    pub errors: Vec<ParseError>,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub enum NodeData {
-    Inherit(Inherit),
+pub struct Node {
+    pub range: SourceRange,
+    pub data: NodeData,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -23,9 +26,36 @@ impl std::fmt::Display for SourceRange {
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub struct Node {
-    pub range: SourceRange,
-    pub data: NodeData,
+pub enum NodeData {
+    Apply(Apply),
+    Inherit(Inherit),
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Apply {
+    pub function: Expr,
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum Expr {
+    Ident(Ident),
+    Select(Box<Select>),
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Ident {
+    pub name: String,
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Select {
+    pub receiver: Expr,
+    pub attrs: Vec<Ident>,
+}
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Inherit {
+    pub attr_count: usize,
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -33,10 +63,3 @@ pub struct ParseError {
     pub range: SourceRange,
     pub message: String,
 }
-
-#[derive(Debug)]
-pub struct ParsedFile {
-    pub file_id: FileId,
-    pub nodes: Vec<Node>,
-    pub errors: Vec<ParseError>,
-}

src/xatu/file_store.rs

@@ -1,14 +1,5 @@
 use std::path::PathBuf;
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
-pub struct FileId(usize);
-
-#[derive(Debug)]
-pub struct SourceFile {
-    pub path: PathBuf,
-    pub source: String,
-}
-
 #[derive(Debug, Default)]
 pub struct FileStore {
     files: Vec<SourceFile>,
@@ -31,3 +22,12 @@ impl From<Vec<SourceFile>> for FileStore {
         Self { files }
     }
 }
+
+#[derive(Debug)]
+pub struct SourceFile {
+    pub path: PathBuf,
+    pub source: String,
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub struct FileId(usize);

src/xatu/parser.rs

@@ -21,6 +21,91 @@ pub fn parse_file(file_id: FileId, source: &str) -> ast::ParsedFile {
     }
 }
 
+fn convert_node(node: rnix::SyntaxNode) -> Option<ast::Node> {
+    match node.kind() {
+        rnix::SyntaxKind::NODE_APPLY => convert_apply(node),
+        rnix::SyntaxKind::NODE_INHERIT => convert_inherit(node),
+        _ => None,
+    }
+}
+
+fn convert_apply(node: rnix::SyntaxNode) -> Option<ast::Node> {
+    let apply = rnix::ast::Apply::cast(node)?;
+    let range = apply.syntax().text_range().into();
+    let data = apply.try_into().ok()?;
+
+    Some(ast::Node {
+        range,
+        data: ast::NodeData::Apply(data),
+    })
+}
+
+impl TryFrom<rnix::ast::Apply> for ast::Apply {
+    type Error = ();
+
+    fn try_from(apply: rnix::ast::Apply) -> Result<Self, Self::Error> {
+        Ok(Self {
+            function: apply.lambda().ok_or(())?.try_into()?,
+        })
+    }
+}
+
+impl TryFrom<rnix::ast::Expr> for ast::Expr {
+    type Error = ();
+
+    fn try_from(expr: rnix::ast::Expr) -> Result<Self, Self::Error> {
+        match expr {
+            rnix::ast::Expr::Ident(ident) => Ok(Self::Ident(ident.into())),
+            rnix::ast::Expr::Select(select) => Ok(Self::Select(Box::new(select.try_into()?))),
+            _ => Err(()),
+        }
+    }
+}
+
+impl From<rnix::ast::Ident> for ast::Ident {
+    fn from(ident: rnix::ast::Ident) -> Self {
+        Self {
+            name: ident.to_string(),
+        }
+    }
+}
+
+impl TryFrom<rnix::ast::Select> for ast::Select {
+    type Error = ();
+
+    fn try_from(select: rnix::ast::Select) -> Result<Self, Self::Error> {
+        Ok(Self {
+            receiver: select.expr().ok_or(())?.try_into()?,
+            attrs: select
+                .attrpath()
+                .ok_or(())?
+                .attrs()
+                .map(|attr| match attr {
+                    rnix::ast::Attr::Ident(ident) => Ok(ident.into()),
+                    _ => Err(()),
+                })
+                .collect::<Result<Vec<_>, _>>()?,
+        })
+    }
+}
+
+fn convert_inherit(node: rnix::SyntaxNode) -> Option<ast::Node> {
+    let inherit = rnix::ast::Inherit::cast(node)?;
+
+    Some(ast::Node {
+        range: inherit.syntax().text_range().into(),
+        data: ast::NodeData::Inherit(inherit.into()),
+    })
+}
+
+impl From<rnix::ast::Inherit> for ast::Inherit {
+    fn from(inherit: rnix::ast::Inherit) -> Self {
+        Self {
+            attr_count: inherit.attrs().count(),
+        }
+    }
+}
+
 fn convert_parse_error(error: &rnix::ParseError, source_len: usize) -> ast::ParseError {
     ast::ParseError {
         range: parse_error_range(error, source_len),
@@ -50,22 +135,6 @@ fn parse_error_range(error: &rnix::ParseError, source_len: usize) -> ast::Source
     }
 }
 
-fn convert_node(node: rnix::SyntaxNode) -> Option<ast::Node> {
-    match node.kind() {
-        rnix::SyntaxKind::NODE_INHERIT => convert_inherit(node),
-        _ => None,
-    }
-}
-
-fn convert_inherit(node: rnix::SyntaxNode) -> Option<ast::Node> {
-    let inherit = rnix::ast::Inherit::cast(node)?;
-
-    Some(ast::Node {
-        range: inherit.syntax().text_range().into(),
-        data: ast::NodeData::Inherit(inherit.into()),
-    })
-}
-
 impl From<rnix::TextRange> for ast::SourceRange {
     fn from(range: rnix::TextRange) -> Self {
         Self {
@@ -74,11 +143,3 @@ impl From<rnix::TextRange> for ast::SourceRange {
         }
     }
 }
-
-impl From<rnix::ast::Inherit> for ast::Inherit {
-    fn from(inherit: rnix::ast::Inherit) -> Self {
-        Self {
-            attr_count: inherit.attrs().count(),
-        }
-    }
-}

src/xatu/rules.rs

@@ -1,9 +1,17 @@
 use crate::FileId;
 use crate::ast;
+use crate::rules::deprecated_to_path::DeprecatedToPath;
 use crate::rules::empty_inherit::EmptyInherit;
 
+pub mod deprecated_to_path;
 pub mod empty_inherit;
 
+#[derive(Debug)]
+pub struct Diagnostic {
+    pub file_id: FileId,
+    pub finding: Finding,
+}
+
 #[derive(Debug)]
 pub struct Finding {
     pub rule_id: &'static str,
@@ -11,12 +19,6 @@ pub struct Finding {
     pub message: &'static str,
 }
 
-#[derive(Debug)]
-pub struct Diagnostic {
-    pub file_id: FileId,
-    pub finding: Finding,
-}
-
 impl std::fmt::Display for Diagnostic {
     fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         write!(
@@ -54,13 +56,16 @@ pub fn run_rules(file: &ast::ParsedFile) -> Vec<Diagnostic> {
     file.nodes
         .iter()
         .flat_map(|node| {
-            [run_node_rule::<EmptyInherit>(node)]
-                .into_iter()
-                .flatten()
-                .map(|finding| Diagnostic {
-                    file_id: file.file_id,
-                    finding,
-                })
+            [
+                run_node_rule::<DeprecatedToPath>(node),
+                run_node_rule::<EmptyInherit>(node),
+            ]
+            .into_iter()
+            .flatten()
+            .map(|finding| Diagnostic {
+                file_id: file.file_id,
+                finding,
+            })
         })
         .collect()
 }

src/xatu/rules/deprecated_to_path.rs

@@ -0,0 +1,41 @@
+use crate::{ast, rules::NodeRule};
+
+pub struct DeprecatedToPath;
+
+impl NodeRule for DeprecatedToPath {
+    type Node = ast::Apply;
+
+    const ID: &'static str = "deprecated_to_path";
+
+    fn select(data: &ast::NodeData) -> Option<&Self::Node> {
+        match data {
+            ast::NodeData::Apply(apply) => Some(apply),
+            _ => None,
+        }
+    }
+
+    fn check(node: &Self::Node) -> Option<&'static str> {
+        if calls_to_path(&node.function) {
+            Some("builtins.toPath is deprecated")
+        } else {
+            None
+        }
+    }
+}
+
+fn calls_to_path(function: &ast::Expr) -> bool {
+    // TODO: Once Xatu supports name resolution, report unqualified `toPath` only
+    // when it resolves to `builtins.toPath`.
+    match function {
+        ast::Expr::Ident(ident) => ident.name == "toPath",
+        ast::Expr::Select(select) => is_builtins_to_path(select),
+    }
+}
+
+fn is_builtins_to_path(select: &ast::Select) -> bool {
+    matches!(
+        &select.receiver,
+        ast::Expr::Ident(ident) if ident.name == "builtins"
+    ) && select.attrs.len() == 1
+        && select.attrs[0].name == "toPath"
+}

src/xatu/rules/empty_inherit.rs

@@ -10,6 +10,7 @@ impl NodeRule for EmptyInherit {
     fn select(data: &ast::NodeData) -> Option<&Self::Node> {
         match data {
             ast::NodeData::Inherit(inherit) => Some(inherit),
+            _ => None,
         }
     }
 

src/xatu/testdata/clean.nix

@@ -1 +1,6 @@
-{ inherit foo; }
+{ pkgs }:
+
+{
+  path = ./. + "/foo";
+  inherit pkgs;
+}

src/xatu/testdata/empty_inherit.nix

@@ -1 +0,0 @@
-{ inherit; }

src/xatu/testdata/invalid.nix

@@ -1 +1,4 @@
+{ pkgs }:
+
 {
+  inherit pkgs;

src/xatu/testdata/lint_findings.nix

@@ -0,0 +1,6 @@
+{ src }:
+
+{
+  inherit;
+  path = builtins.toPath src;
+}

src/xatu/testdata/lint_variants.nix

@@ -0,0 +1,5 @@
+{ src }:
+
+{
+  path = toPath src;
+}

src/xatu/xatu_test.rs

@@ -1,53 +1,114 @@
 use std::path::{Path, PathBuf};
 
+use googletest::matchers::{contains, eq, is_true};
+use googletest::{OrFail, expect_that, gtest};
 use status::Code;
 use xatu::SourceFile;
 
-fn source_file(path: &str) -> SourceFile {
-    SourceFile {
+fn fixture_source(path: &str) -> googletest::Result<SourceFile> {
+    Ok(SourceFile {
         path: PathBuf::from(path),
-        source: std::fs::read_to_string(testdata_path(path)).expect("fixture should be readable"),
-    }
+        source: std::fs::read_to_string(fixture_path(path)?).or_fail()?,
+    })
 }
 
-fn testdata_path(path: &str) -> PathBuf {
+fn fixture_path(path: &str) -> googletest::Result<PathBuf> {
     if Path::new(path).exists() {
-        return PathBuf::from(path);
+        return Ok(PathBuf::from(path));
     }
 
-    let runfiles = PathBuf::from(std::env::var("TEST_SRCDIR").expect("TEST_SRCDIR should be set"));
-    let workspace = std::env::var("TEST_WORKSPACE").expect("TEST_WORKSPACE should be set");
+    let runfiles = PathBuf::from(std::env::var("TEST_SRCDIR").or_fail()?);
+    let workspace = std::env::var("TEST_WORKSPACE").or_fail()?;
 
-    runfiles.join(workspace).join("src/xatu").join(path)
+    Ok(runfiles.join(workspace).join("src/xatu").join(path))
 }
 
-#[test]
-fn future_sight_reports_clean_sources_as_clean() {
-    let report = xatu::future_sight(vec![source_file("testdata/clean.nix")])
-    .expect("fixture should parse");
+#[gtest]
+fn xatu_accepts_clean_source() -> googletest::Result<()> {
+    let report = xatu::future_sight(vec![fixture_source("testdata/clean.nix")?]).or_fail()?;
 
-    assert!(report.is_clean());
+    expect_that!(report.is_clean(), is_true());
+    Ok(())
 }
 
-#[test]
-fn future_sight_rejects_parse_errors() {
-    let error = xatu::future_sight(vec![source_file("testdata/invalid.nix")])
-    .expect_err("fixture should fail to parse");
+#[gtest]
+fn xatu_rejects_invalid_source() -> googletest::Result<()> {
+    let error = xatu::future_sight(vec![fixture_source("testdata/invalid.nix")?])
+        .err()
+        .or_fail()?;
 
-    assert_eq!(error.code(), Code::FailedPrecondition);
+    expect_that!(error.code(), eq(Code::FailedPrecondition));
+    Ok(())
 }
 
-#[test]
-fn future_sight_reports_empty_inherit_findings() {
-    let report = xatu::future_sight(vec![source_file("testdata/empty_inherit.nix")])
-    .expect("fixture should parse");
+#[gtest]
+fn xatu_reports_deprecated_builtin_to_path() -> googletest::Result<()> {
+    let report = xatu::future_sight(vec![fixture_source("testdata/lint_findings.nix")?]).or_fail()?;
 
-    assert_eq!(report.diagnostics.len(), 1);
-
-    let diagnostic = &report.diagnostics[0];
-    assert_eq!(
-        report.files.get(diagnostic.file_id).path,
-        PathBuf::from("testdata/empty_inherit.nix"),
+    let findings = report
+        .diagnostics
+        .iter()
+        .map(|diagnostic| {
+            (
+                report.files.get(diagnostic.file_id).path.clone(),
+                diagnostic.finding.rule_id,
+            )
+        })
+        .collect::<Vec<_>>();
+    expect_that!(
+        findings,
+        contains(eq(&(
+            PathBuf::from("testdata/lint_findings.nix"),
+            "deprecated_to_path"
+        )))
     );
-    assert_eq!(diagnostic.finding.rule_id, "empty_inherit");
+    Ok(())
+}
+
+#[gtest]
+fn xatu_reports_deprecated_unqualified_to_path() -> googletest::Result<()> {
+    let report = xatu::future_sight(vec![fixture_source("testdata/lint_variants.nix")?]).or_fail()?;
+
+    let findings = report
+        .diagnostics
+        .iter()
+        .map(|diagnostic| {
+            (
+                report.files.get(diagnostic.file_id).path.clone(),
+                diagnostic.finding.rule_id,
+            )
+        })
+        .collect::<Vec<_>>();
+    expect_that!(
+        findings,
+        contains(eq(&(
+            PathBuf::from("testdata/lint_variants.nix"),
+            "deprecated_to_path"
+        )))
+    );
+    Ok(())
+}
+
+#[gtest]
+fn xatu_reports_empty_inherit() -> googletest::Result<()> {
+    let report = xatu::future_sight(vec![fixture_source("testdata/lint_findings.nix")?]).or_fail()?;
+
+    let findings = report
+        .diagnostics
+        .iter()
+        .map(|diagnostic| {
+            (
+                report.files.get(diagnostic.file_id).path.clone(),
+                diagnostic.finding.rule_id,
+            )
+        })
+        .collect::<Vec<_>>();
+    expect_that!(
+        findings,
+        contains(eq(&(
+            PathBuf::from("testdata/lint_findings.nix"),
+            "empty_inherit"
+        )))
+    );
+    Ok(())
 }