From c7874ec26d6499a1196ce23432beb33764490dec Mon Sep 17 00:00:00 2001 From: Akshay Date: Sun, 20 Feb 2022 10:55:20 +0530 Subject: new lint: useless_has_attr --- bin/tests/data/useless_has_attr.nix | 9 +++ bin/tests/main.rs | 3 +- bin/tests/snapshots/main__useless_has_attr.snap | 34 ++++++++++ lib/src/lints.rs | 1 + lib/src/lints/useless_has_attr.rs | 83 +++++++++++++++++++++++++ lib/src/make.rs | 4 ++ 6 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 bin/tests/data/useless_has_attr.nix create mode 100644 bin/tests/snapshots/main__useless_has_attr.snap create mode 100644 lib/src/lints/useless_has_attr.rs 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 { + 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)) +} -- cgit v1.2.3