add `explain` subcommand and explanations to all lints

12 files changed, 308 insertions, 44 deletions

diff --git a/lib/src/lib.rs b/lib/src/lib.rs
index 196cbf8..5347666 100644
--- a/lib/src/lib.rs
+++ b/lib/src/lib.rs
@@ -226,25 +226,29 @@ pub trait Rule {
 /// Contains information about the lint itself. Do not implement manually,
 /// look at the `lint` attribute macro instead for implementing rules
 pub trait Metadata {
-    fn name() -> &'static str
-    where
-        Self: Sized;
-    fn note() -> &'static str
-    where
-        Self: Sized;
-    fn code() -> u32
-    where
-        Self: Sized;
-    fn report() -> Report
-    where
-        Self: Sized;
+    fn name(&self) -> &'static str;
+    fn note(&self) -> &'static str;
+    fn code(&self) -> u32;
+    fn report(&self) -> Report;
     fn match_with(&self, with: &SyntaxKind) -> bool;
     fn match_kind(&self) -> Vec<SyntaxKind>;
 }
 
+/// Contains offline explanation for each lint
+/// The `lint` macro scans nearby doc comments for
+/// explanations and derives this trait.
+///
+/// FIXME: the lint macro does way too much, maybe
+/// split it into smaller macros.
+pub trait Explain {
+    fn explanation(&self) -> &'static str {
+        "no explanation found"
+    }
+}
+
 /// Combines Rule and Metadata, do not implement manually, this is derived by
 /// the `lint` macro.
-pub trait Lint: Metadata + Rule + Send + Sync {}
+pub trait Lint: Metadata + Explain + Rule + Send + Sync {}
 
 /// Helper utility to take lints from modules and insert them into a map for efficient
 /// access. Mapping is from a SyntaxKind to a list of lints that apply on that Kind.
diff --git a/lib/src/lints/bool_comparison.rs b/lib/src/lints/bool_comparison.rs
index 0b5733b..5c9bee8 100644
--- a/lib/src/lints/bool_comparison.rs
+++ b/lib/src/lints/bool_comparison.rs
@@ -1,4 +1,4 @@
-use crate::{make, Lint, Metadata, Report, Rule, Suggestion};
+use crate::{make, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,6 +7,25 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode,
 };
 
+/// ## What it does
+/// Checks for expressions of the form `x == true`, `x != true` and
+/// suggests using the variable directly.
+///
+/// ## Why is this bad?
+/// Unnecessary code.
+///
+/// ## Example
+/// Instead of checking the value of `x`:
+///
+/// ```
+/// if x == true then 0 else 1
+/// ```
+///
+/// Use `x` directly:
+///
+/// ```
+/// if x then 0 else 1
+/// ```
 #[lint(
     name = "bool_comparison",
     note = "Unnecessary comparison with boolean",
@@ -71,7 +90,7 @@ impl Rule for BoolComparison {
                     non_bool_side,
                     bool_side
                 );
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/collapsible_let_in.rs b/lib/src/lints/collapsible_let_in.rs
index 878d218..21199a8 100644
--- a/lib/src/lints/collapsible_let_in.rs
+++ b/lib/src/lints/collapsible_let_in.rs
@@ -1,13 +1,41 @@
-use crate::{make, Lint, Metadata, Report, Rule, Suggestion};
+use crate::{make, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
-use rowan::Direction;
 use rnix::{
     types::{LetIn, TypedNode},
-    NodeOrToken, SyntaxElement, SyntaxKind, TextRange
+    NodeOrToken, SyntaxElement, SyntaxKind, TextRange,
 };
+use rowan::Direction;
 
+/// ## What it does
+/// Checks for `let-in` expressions whose body is another `let-in`
+/// expression.
+///
+/// ## Why is this bad?
+/// Unnecessary code, the `let-in` expressions can be merged.
+///
+/// ## Example
+///
+/// ```
+/// let
+///   a = 2;
+/// in
+/// let
+///   b = 3;
+/// in
+///   a + b
+/// ```
+///
+/// Merge both `let-in` expressions:
+///
+/// ```
+/// let
+///   a = 2;
+///   b = 3;
+/// in
+///   a + b
+/// ```
 #[lint(
     name = "collapsible let in",
     note = "These let-in expressions are collapsible",
@@ -47,7 +75,7 @@ impl Rule for CollapsibleLetIn {
                 let replacement = make::empty().node().clone();
 
                 Some(
-                    Self::report()
+                    self.report()
                         .diagnostic(first_annotation, first_message)
                         .suggest(second_annotation, second_message, Suggestion::new(replacement_at, replacement))
                 )
@@ -57,4 +85,3 @@ impl Rule for CollapsibleLetIn {
         }
     }
 }
-
diff --git a/lib/src/lints/empty_let_in.rs b/lib/src/lints/empty_let_in.rs
index aae1377..b255c23 100644
--- a/lib/src/lints/empty_let_in.rs
+++ b/lib/src/lints/empty_let_in.rs
@@ -1,12 +1,30 @@
-use crate::{Lint, Metadata, Report, Rule, Suggestion};
+use crate::{Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
 use rnix::{
-    types::{LetIn, TypedNode, EntryHolder},
+    types::{EntryHolder, LetIn, TypedNode},
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for `let-in` expressions which create no new bindings.
+///
+/// ## Why is this bad?
+/// `let-in` expressions that create no new bindings are useless.
+/// These are probably remnants from debugging or editing expressions.
+///
+/// ## Example
+///
+/// ```
+/// let in pkgs.statix
+/// ```
+///
+/// Preserve only the body of the `let-in` expression:
+///
+/// ```
+/// pkgs.statix
+/// ```
 #[lint(
     name = "empty let-in",
     note = "Useless let-in expression",
@@ -31,11 +49,10 @@ impl Rule for EmptyLetIn {
                 let at = node.text_range();
                 let replacement = body;
                 let message = "This let-in expression has no entries";
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
         }
     }
 }
-
diff --git a/lib/src/lints/empty_pattern.rs b/lib/src/lints/empty_pattern.rs
index 6fb7558..5312548 100644
--- a/lib/src/lints/empty_pattern.rs
+++ b/lib/src/lints/empty_pattern.rs
@@ -1,4 +1,4 @@
-use crate::{make, Lint, Metadata, Report, Rule, Suggestion};
+use crate::{make, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,6 +7,33 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for an empty variadic pattern: `{...}`, in a function
+/// argument.
+///
+/// ## Why is this bad?
+/// The intention with empty patterns is not instantly obvious. Prefer
+/// an underscore identifier instead, to indicate that the argument
+/// is being ignored.
+///
+/// ## Example
+///
+/// ```
+/// client = { ... }: {
+///   imports = [ self.nixosModules.irmaseal-pkg ];
+///   services.irmaseal-pkg.enable = true;
+/// };
+/// ```
+///
+/// Replace the empty variadic pattern with `_` to indicate that you
+/// intend to ignore the argument:
+///
+/// ```
+/// client = _: {
+///   imports = [ self.nixosModules.irmaseal-pkg ];
+///   services.irmaseal-pkg.enable = true;
+/// };
+/// ```
 #[lint(
     name = "empty pattern",
     note = "Found empty pattern in function argument",
@@ -28,7 +55,7 @@ impl Rule for EmptyPattern {
                 let at = node.text_range();
                 let message = "This pattern is empty, use `_` instead";
                 let replacement = make::ident("_").node().clone();
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/eta_reduction.rs b/lib/src/lints/eta_reduction.rs
index 79a5101..3a483d0 100644
--- a/lib/src/lints/eta_reduction.rs
+++ b/lib/src/lints/eta_reduction.rs
@@ -1,4 +1,4 @@
-use crate::{Lint, Metadata, Report, Rule, Suggestion};
+use crate::{Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,6 +7,32 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode,
 };
 
+/// ## What it does
+/// Checks for eta-reducible functions, i.e.: converts lambda
+/// expressions into free standing functions where applicable.
+///
+/// ## Why is this bad?
+/// Oftentimes, eta-reduction results in code that is more natural
+/// to read.
+///
+/// ## Example
+///
+/// ```
+/// let
+///   double = i: 2 * i;
+/// in
+/// map (x: double x) [ 1 2 3 ]
+/// ```
+///
+/// The lambda passed to the `map` function is eta-reducible, and the
+/// result reads more naturally:
+///
+/// ```
+/// let
+///   double = i: 2 * i;
+/// in
+/// map double [ 1 2 3 ]
+/// ```
 #[lint(
     name = "eta reduction",
     note = "This function expression is eta reducible",
@@ -43,7 +69,7 @@ impl Rule for EtaReduction {
                         "Found eta-reduction: `{}`",
                         replacement.text().to_string()
                     );
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/legacy_let_syntax.rs b/lib/src/lints/legacy_let_syntax.rs
index 2087e27..139f633 100644
--- a/lib/src/lints/legacy_let_syntax.rs
+++ b/lib/src/lints/legacy_let_syntax.rs
@@ -1,4 +1,4 @@
-use crate::{make, Lint, Metadata, Report, Rule, Suggestion};
+use crate::{make, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,6 +7,34 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for legacy-let syntax that was never formalized.
+///
+/// ## Why is this bad?
+/// This syntax construct is undocumented, refrain from using it.
+///
+/// ## Example
+///
+/// Legacy let syntax makes use of an attribute set annotated with
+/// `let` and expects a `body` attribute.
+/// ```
+/// let {
+///   body = x + y;
+///   x = 2;
+///   y = 3;
+/// }
+/// ```
+///
+/// This is trivially representible via `rec`, which is documented
+/// and more widely known:
+///
+/// ```
+/// rec {
+///   body = x + y;
+///   x = 2;
+///   y = 3;
+/// }.body
+/// ```
 #[lint(
     name = "legacy let syntax",
     note = "Using undocumented `let` syntax",
@@ -36,7 +64,7 @@ impl Rule for ManualInherit {
                 let message = "Prefer `rec` over undocumented `let` syntax";
                 let replacement = selected.node().clone();
 
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/manual_inherit.rs b/lib/src/lints/manual_inherit.rs
index 0a6933c..2d119c3 100644
--- a/lib/src/lints/manual_inherit.rs
+++ b/lib/src/lints/manual_inherit.rs
@@ -1,4 +1,4 @@
-use crate::{make, Lint, Metadata, Report, Rule, Suggestion};
+use crate::{make, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,6 +7,30 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for bindings of the form `a = a`.
+///
+/// ## Why is this bad?
+/// If the aim is to bring attributes from a larger scope into
+/// the current scope, prefer an inherit statement.
+///
+/// ## Example
+///
+/// ```
+/// let
+///   a = 2;
+/// in
+///   { a = a; b = 3; }
+/// ```
+///
+/// Try `inherit` instead:
+///
+/// ```
+/// let
+///   a = 2;
+/// in
+///   { inherit a; b = 3; }
+/// ```
 #[lint(
     name = "manual inherit",
     note = "Assignment instead of inherit",
@@ -35,7 +59,7 @@ impl Rule for ManualInherit {
                 let at = node.text_range();
                 let replacement = make::inherit_stmt(&[key]).node().clone();
                 let message = "This assignment is better written with `inherit`";
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/manual_inherit_from.rs b/lib/src/lints/manual_inherit_from.rs
index 794aaf9..8d0f539 100644
--- a/lib/src/lints/manual_inherit_from.rs
+++ b/lib/src/lints/manual_inherit_from.rs
@@ -1,4 +1,4 @@
-use crate::{make, Lint, Metadata, Report, Rule, Suggestion};
+use crate::{make, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,6 +7,30 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for bindings of the form `a = someAttr.a`.
+///
+/// ## Why is this bad?
+/// If the aim is to extract or bring attributes of an attrset into
+/// scope, prefer an inherit statement.
+///
+/// ## Example
+///
+/// ```
+/// let
+///   mtl = pkgs.haskellPackages.mtl;
+/// in
+///   null
+/// ```
+///
+/// Try `inherit` instead:
+///
+/// ```
+/// let
+///   inherit (pkgs.haskellPackages) mtl;
+/// in
+///   null
+/// ```
 #[lint(
     name = "manual inherit from",
     note = "Assignment instead of inherit from",
@@ -40,7 +64,7 @@ impl Rule for ManualInherit {
                     make::inherit_from_stmt(set, &[key]).node().clone()
                 };
                 let message = "This assignment is better written with `inherit`";
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/redundant_pattern_bind.rs b/lib/src/lints/redundant_pattern_bind.rs
index aebc549..5b0711f 100644
--- a/lib/src/lints/redundant_pattern_bind.rs
+++ b/lib/src/lints/redundant_pattern_bind.rs
@@ -1,4 +1,4 @@
-use crate::{Lint, Metadata, Report, Rule, Suggestion};
+use crate::{Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,10 +7,29 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for binds of the form `inputs @ { ... }` in function
+/// arguments.
+///
+/// ## Why is this bad?
+/// The variadic pattern here is redundant, as it does not capture
+/// anything.
+///
+/// ## Example
+///
+/// ```
+/// inputs @ { ... }: inputs.nixpkgs
+/// ```
+///
+/// Remove the pattern altogether:
+///
+/// ```
+/// inputs: inputs.nixpkgs
+/// ```
 #[lint(
     name = "redundant pattern bind",
     note = "Found redundant pattern bind in function argument",
-    code = 10,
+    code = 11,
     match_with = SyntaxKind::NODE_PATTERN
 )]
 struct RedundantPatternBind;
@@ -32,7 +51,7 @@ impl Rule for RedundantPatternBind {
                 let at = node.text_range();
                 let message = format!("This pattern bind is redundant, use `{}` instead", ident.as_str());
                 let replacement = ident.node().clone();
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/unquoted_splice.rs b/lib/src/lints/unquoted_splice.rs
index 4d1ed69..c2fd6e4 100644
--- a/lib/src/lints/unquoted_splice.rs
+++ b/lib/src/lints/unquoted_splice.rs
@@ -1,4 +1,4 @@
-use crate::{make, Lint, Metadata, Report, Rule, Suggestion};
+use crate::{make, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
@@ -7,6 +7,30 @@ use rnix::{
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for antiquote/splice expressions that are not quoted.
+///
+/// ## Why is this bad?
+/// An *anti*quoted expression should always occur within a *quoted*
+/// expression.
+///
+/// ## Example
+///
+/// ```
+/// let
+///   pkgs = nixpkgs.legacyPackages.${system};
+/// in
+///   pkgs
+/// ```
+///
+/// Quote the splice expression:
+///
+/// ```
+/// let
+///   pkgs = nixpkgs.legacyPackages."${system}";
+/// in
+///   pkgs
+/// ```
 #[lint(
     name = "unquoted splice",
     note = "Found unquoted splice expression",
@@ -24,7 +48,7 @@ impl Rule for UnquotedSplice {
                 let at = node.text_range();
                 let replacement = make::quote(&node).node().clone();
                 let message = "Consider quoting this splice expression";
-                Some(Self::report().suggest(at, message, Suggestion::new(at, replacement)))
+                Some(self.report().suggest(at, message, Suggestion::new(at, replacement)))
             } else {
                 None
             }
diff --git a/lib/src/lints/useless_parens.rs b/lib/src/lints/useless_parens.rs
index 2d6ba8f..36ad1b7 100644
--- a/lib/src/lints/useless_parens.rs
+++ b/lib/src/lints/useless_parens.rs
@@ -1,12 +1,37 @@
-use crate::{Lint, Metadata, Report, Rule, Suggestion, Diagnostic};
+use crate::{Diagnostic, Metadata, Report, Rule, Suggestion};
 
 use if_chain::if_chain;
 use macros::lint;
 use rnix::{
-    types::{ParsedType, KeyValue, Paren, TypedNode, Wrapper},
+    types::{KeyValue, Paren, ParsedType, TypedNode, Wrapper},
     NodeOrToken, SyntaxElement, SyntaxKind,
 };
 
+/// ## What it does
+/// Checks for unnecessary parentheses.
+///
+/// ## Why is this bad?
+/// Unnecessarily parenthesized code is hard to read.
+///
+/// ## Example
+///
+/// ```
+/// let
+///   double = (x: 2 * x);
+///   ls = map (double) [ 1 2 3 ];
+/// in
+///   (2 + 3)
+/// ```
+///
+/// Remove unnecessary parentheses:
+///
+/// ```
+/// let
+///   double = x: 2 * x;
+///   ls = map double [ 1 2 3 ];
+/// in
+///   2 + 3
+/// ```
 #[lint(
     name = "useless parens",
     note = "These parentheses can be omitted",
@@ -27,7 +52,7 @@ impl Rule for UselessParens {
 
             if let Some(diagnostic) = do_thing(parsed_type_node);
             then {
-                let mut report = Self::report();
+                let mut report = self.report();
                 report.diagnostics.push(diagnostic);
                 Some(report)
             } else {
@@ -79,7 +104,7 @@ fn do_thing(parsed_type_node: ParsedType) -> Option<Diagnostic> {
             if let Some(parsed_inner) = ParsedType::cast(inner_node);
             if matches!(
                 parsed_inner,
-                ParsedType::List(_) 
+                ParsedType::List(_)
                 | ParsedType::Paren(_)
                 | ParsedType::Str(_)
                 | ParsedType::AttrSet(_)
@@ -95,6 +120,6 @@ fn do_thing(parsed_type_node: ParsedType) -> Option<Diagnostic> {
                 None
             }
         },
-        _ => None
+        _ => None,
     }
 }