Merge pull request #288 from crystal-ameba/Sija/lint-not-nil-rule

Add `Lint/NotNil` rule

spec/ameba/ast/scope_spec.cr

@@ -135,14 +135,16 @@ module Ameba::AST
       scope = Scope.new as_node("foo = 1")
       scope.add_variable Crystal::Var.new "foo"
       scope.assign_variable("foo", Crystal::Var.new "foo")
-      scope.find_variable("foo").not_nil!.assignments.size.should eq 1
+      var = scope.find_variable("foo").should_not be_nil
+      var.assignments.size.should eq 1
     end
 
     it "does not create the assignment if variable is wrong" do
       scope = Scope.new as_node("foo = 1")
       scope.add_variable Crystal::Var.new "foo"
       scope.assign_variable("bar", Crystal::Var.new "bar")
-      scope.find_variable("foo").not_nil!.assignments.size.should eq 0
+      var = scope.find_variable("foo").should_not be_nil
+      var.assignments.size.should eq 0
     end
   end
 

spec/ameba/formatter/todo_formatter_spec.cr

@@ -86,8 +86,8 @@ module Ameba
         s1.add_issue DummyRule.new, {2, 2}, "message1"
         s2.add_issue DummyRule.new, {2, 2}, "message2"
 
-        file = formatter.finished([s1, s2])
+        file = formatter.finished([s1, s2]).should_not be_nil
-        content = File.read(file.not_nil!.path)
+        content = File.read(file.path)
         content.should contain <<-CONTENT
         # Problems found: 3
         # Run `ameba --only Ameba/DummyRule` for details

spec/ameba/rule/lint/not_nil_spec.cr

@@ -0,0 +1,42 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  subject = NotNil.new
+
+  describe NotNil do
+    it "passes for valid cases" do
+      expect_no_issues subject, <<-CRYSTAL
+        (1..3).first?.not_nil!(:foo)
+        not_nil!
+        CRYSTAL
+    end
+
+    it "reports if there is a `not_nil!` call" do
+      expect_issue subject, <<-CRYSTAL
+        (1..3).first?.not_nil!
+                    # ^^^^^^^^ error: Avoid using `not_nil!`
+        CRYSTAL
+    end
+
+    context "macro" do
+      it "doesn't report in macro scope" do
+        expect_no_issues subject, <<-CRYSTAL
+          {{ [1, 2, 3].first.not_nil! }}
+          CRYSTAL
+      end
+    end
+
+    it "reports rule, pos and message" do
+      s = Source.new %(
+        (1..3).first?.not_nil!
+      ), "source.cr"
+      subject.catch(s).should_not be_valid
+      issue = s.issues.first
+
+      issue.rule.should_not be_nil
+      issue.location.to_s.should eq "source.cr:1:15"
+      issue.end_location.to_s.should eq "source.cr:1:22"
+      issue.message.should eq "Avoid using `not_nil!`"
+    end
+  end
+end

src/ameba/rule/lint/not_nil.cr

@@ -0,0 +1,57 @@
+module Ameba::Rule::Lint
+  # This rule is used to identify usages of `not_nil!` calls.
+  #
+  # For example, this is considered a code smell:
+  #
+  # ```
+  # names = %w[Alice Bob]
+  # alice = names.find { |name| name == "Alice" }.not_nil!
+  # ```
+  #
+  # And can be written as this:
+  #
+  # ```
+  # names = %w[Alice Bob]
+  # alice = names.find { |name| name == "Alice" }
+  #
+  # if alice
+  #   # ...
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Lint/NotNil:
+  #   Enabled: true
+  # ```
+  class NotNil < Base
+    include AST::Util
+
+    properties do
+      description "Identifies usage of `not_nil!` calls"
+    end
+
+    NOT_NIL_NAME = "not_nil!"
+    MSG          = "Avoid using `not_nil!`"
+
+    def test(source)
+      AST::NodeVisitor.new self, source, skip: [
+        Crystal::Macro,
+        Crystal::MacroExpression,
+        Crystal::MacroIf,
+        Crystal::MacroFor,
+      ]
+    end
+
+    def test(source, node : Crystal::Call)
+      return unless node.name == NOT_NIL_NAME
+      return unless node.obj && node.args.empty?
+
+      return unless name_location = node.name_location
+      return unless end_location = name_end_location(node)
+
+      issue_for name_location, end_location, MSG
+    end
+  end
+end

src/ameba/rule/lint/percent_array.cr

@@ -41,12 +41,12 @@ module Ameba::Rule::Lint
         when .string_array_start?, .symbol_array_start?
           start_token = token.dup
         when .string?
-          if start_token && issue.nil?
+          if (_start = start_token) && !issue
-            issue = array_entry_invalid?(token.value, start_token.not_nil!.raw)
+            issue = array_entry_invalid?(token.value, _start.raw)
           end
         when .string_array_end?
-          if issue
+          if (_start = start_token) && (_issue = issue)
-            issue_for start_token.not_nil!, issue.not_nil!
+            issue_for _start, _issue
           end
           issue = start_token = nil
         end

src/ameba/rule/lint/redundant_with_object.cr

@@ -37,8 +37,8 @@ module Ameba::Rule::Lint
     def test(source, node : Crystal::Call)
       return if node.name != "each_with_object" ||
                 node.args.size != 1 ||
-                node.block.nil? ||
+                !(block = node.block) ||
-                with_index_arg?(node.block.not_nil!)
+                with_index_arg?(block)
 
       issue_for node.name_location, node.name_end_location, MSG
     end