From e8c955da4cbb042e6f9b89307d143f5bfa6779fa Mon Sep 17 00:00:00 2001 From: Akshay Date: Sun, 31 Oct 2021 14:35:26 +0530 Subject: add `explain` subcommand and explanations to all lints --- lib/src/lib.rs | 30 ++++++++++++++------------ lib/src/lints/bool_comparison.rs | 23 ++++++++++++++++++-- lib/src/lints/collapsible_let_in.rs | 37 ++++++++++++++++++++++++++++----- lib/src/lints/empty_let_in.rs | 25 ++++++++++++++++++---- lib/src/lints/empty_pattern.rs | 31 +++++++++++++++++++++++++-- lib/src/lints/eta_reduction.rs | 30 ++++++++++++++++++++++++-- lib/src/lints/legacy_let_syntax.rs | 32 ++++++++++++++++++++++++++-- lib/src/lints/manual_inherit.rs | 28 +++++++++++++++++++++++-- lib/src/lints/manual_inherit_from.rs | 28 +++++++++++++++++++++++-- lib/src/lints/redundant_pattern_bind.rs | 25 +++++++++++++++++++--- lib/src/lints/unquoted_splice.rs | 28 +++++++++++++++++++++++-- lib/src/lints/useless_parens.rs | 35 ++++++++++++++++++++++++++----- 12 files changed, 308 insertions(+), 44 deletions(-) (limited to 'lib') 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; } +/// 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 { 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 { None } }, - _ => None + _ => None, } } -- cgit v1.2.3