new lint: useless_has_attr

6 files changed, 133 insertions, 1 deletions

diff --git a/bin/tests/data/useless_has_attr.nix b/bin/tests/data/useless_has_attr.nix
new file mode 100644
index 0000000..f4c9fd5
--- /dev/null
+++ b/bin/tests/data/useless_has_attr.nix
@@ -0,0 +1,9 @@
+[
+  # trivial
+  (if x ? a then x.a else default)
+  (if x.a ? b then x.a.b else default)
+  (if x ? a.b then x.a.b else default)
+
+  # complex body
+  (if x ? a then x.a else if b then c else d)
+]
diff --git a/bin/tests/main.rs b/bin/tests/main.rs
index 3403f65..3425cfa 100644
--- a/bin/tests/main.rs
+++ b/bin/tests/main.rs
@@ -64,5 +64,6 @@ test_lint! {
     faster_groupby => session_info!("2.5"),
     faster_zipattrswith => session_info!("2.6"),
     deprecated_to_path => session_info!("2.4"),
-    bool_simplification
+    bool_simplification,
+    useless_has_attr
 }
diff --git a/bin/tests/snapshots/main__useless_has_attr.snap b/bin/tests/snapshots/main__useless_has_attr.snap
new file mode 100644
index 0000000..df72eaf
--- /dev/null
+++ b/bin/tests/snapshots/main__useless_has_attr.snap
@@ -0,0 +1,34 @@
+---
+source: bin/tests/main.rs
+expression: "&out"
+
+---
+[W19] Warning: This `if` expression can be simplified with `or`
+   ╭─[data/useless_has_attr.nix:3:4]
+   │
+ 3 │   (if x ? a then x.a else default)
+   ·    ───────────────┬──────────────  
+   ·                   ╰──────────────── Consider using x.a or default instead of this if expression
+───╯
+[W19] Warning: This `if` expression can be simplified with `or`
+   ╭─[data/useless_has_attr.nix:4:4]
+   │
+ 4 │   (if x.a ? b then x.a.b else default)
+   ·    ─────────────────┬────────────────  
+   ·                     ╰────────────────── Consider using x.a.b or default instead of this if expression
+───╯
+[W19] Warning: This `if` expression can be simplified with `or`
+   ╭─[data/useless_has_attr.nix:5:4]
+   │
+ 5 │   (if x ? a.b then x.a.b else default)
+   ·    ─────────────────┬────────────────  
+   ·                     ╰────────────────── Consider using x.a.b or default instead of this if expression
+───╯
+[W19] Warning: This `if` expression can be simplified with `or`
+   ╭─[data/useless_has_attr.nix:8:4]
+   │
+ 8 │   (if x ? a then x.a else if b then c else d)
+   ·    ────────────────────┬────────────────────  
+   ·                        ╰────────────────────── Consider using x.a or (if b then c else d) instead of this if expression
+───╯
+
diff --git a/lib/src/lints.rs b/lib/src/lints.rs
index 582cabe..7d73277 100644
--- a/lib/src/lints.rs
+++ b/lib/src/lints.rs
@@ -19,4 +19,5 @@ lints! {
     faster_zipattrswith,
     deprecated_to_path,
     bool_simplification,
+    useless_has_attr,
 }
diff --git a/lib/src/lints/useless_has_attr.rs b/lib/src/lints/useless_has_attr.rs
new file mode 100644
index 0000000..aae560a
--- /dev/null
+++ b/lib/src/lints/useless_has_attr.rs
@@ -0,0 +1,83 @@
+use crate::{make, session::SessionInfo, Metadata, Report, Rule, Suggestion};
+
+use if_chain::if_chain;
+use macros::lint;
+use rnix::{
+    types::{BinOp, BinOpKind, IfElse, Select, TypedNode},
+    NodeOrToken, SyntaxElement, SyntaxKind,
+};
+
+/// ## What it does
+/// Checks for expressions that use the "has attribute" operator: `?`,
+/// where the `or` operator would suffice.
+///
+/// ## Why is this bad?
+/// The `or` operator is more readable.
+///
+/// ## Example
+/// ```nix
+/// if x ? a then x.a else some_default
+/// ```
+///
+/// Use `or` instead:
+///
+/// ```nix
+/// x.a or some_default
+/// ```
+#[lint(
+    name = "useless_has_attr",
+    note = "This `if` expression can be simplified with `or`",
+    code = 19,
+    match_with = SyntaxKind::NODE_IF_ELSE
+)]
+struct UselessHasAttr;
+
+impl Rule for UselessHasAttr {
+    fn validate(&self, node: &SyntaxElement, _sess: &SessionInfo) -> Option<Report> {
+        if_chain! {
+            if let NodeOrToken::Node(node) = node;
+            if let Some(if_else_expr) = IfElse::cast(node.clone());
+            if let Some(condition_expr) = if_else_expr.condition();
+            if let Some(default_expr) = if_else_expr.else_body();
+            if let Some(cond_bin_expr) = BinOp::cast(condition_expr.clone());
+            if let Some(BinOpKind::IsSet) = cond_bin_expr.operator();
+
+            // set ? attr_path
+            // ^^^--------------- lhs
+            //      ^^^^^^^^^^--- rhs
+            if let Some(set) = cond_bin_expr.lhs();
+            if let Some(attr_path) = cond_bin_expr.rhs();
+
+            // check if body of the `if` expression is of the form `set.attr_path`
+            if let Some(body_expr) = if_else_expr.body();
+            if let Some(body_select_expr) = Select::cast(body_expr.clone());
+            let expected_body = make::select(&set, &attr_path);
+
+            // text comparison will do for now
+            if body_select_expr.node().text() == expected_body.node().text();
+            then {
+                let at = node.text_range();
+                // `or` is tightly binding, we need to parenthesize non-literal exprs
+                let default_with_parens = match default_expr.kind() {
+                    SyntaxKind::NODE_LIST
+                    | SyntaxKind::NODE_PAREN
+                    | SyntaxKind::NODE_STRING
+                    | SyntaxKind::NODE_ATTR_SET
+                    | SyntaxKind::NODE_IDENT => default_expr,
+                    _ => make::parenthesize(&default_expr).node().clone(),
+                };
+                let replacement = make::or_default(&set, &attr_path, &default_with_parens).node().clone();
+                let message = format!(
+                    "Consider using `{}` instead of this `if` expression",
+                    replacement
+                );
+                Some(
+                    self.report()
+                        .suggest(at, message, Suggestion::new(at, replacement)),
+                )
+            } else {
+                None
+            }
+        }
+    }
+}
diff --git a/lib/src/make.rs b/lib/src/make.rs
index c36fa7e..2c33ac8 100644
--- a/lib/src/make.rs
+++ b/lib/src/make.rs
@@ -88,3 +88,7 @@ pub fn empty() -> types::Root {
 pub fn binary(lhs: &SyntaxNode, op: &str, rhs: &SyntaxNode) -> types::BinOp {
     ast_from_text(&format!("{} {} {}", lhs, op, rhs))
 }
+
+pub fn or_default(set: &SyntaxNode, index: &SyntaxNode, default: &SyntaxNode) -> types::OrDefault {
+    ast_from_text(&format!("{}.{} or {}", set, index, default))
+}