Merge pull request #314 from crystal-ameba/Sija/style-query-bool-methods-rule

spec/ameba/rule/style/query_bool_methods_spec.cr

@@ -0,0 +1,72 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Style
+  subject = QueryBoolMethods.new
+
+  describe QueryBoolMethods do
+    it "passes for valid cases" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          class_property? foo = true
+          property? foo = true
+          property foo2 : Bool? = true
+          setter panda = true
+        end
+
+        module Bar
+          class_getter? bar : Bool = true
+          getter? bar : Bool
+          getter bar2 : Bool? = true
+          setter panda : Bool = true
+
+          def initialize(@bar = true)
+          end
+        end
+        CRYSTAL
+    end
+
+    it "reports only valid properties" do
+      expect_issue subject, <<-CRYSTAL
+        class Foo
+          class_property? foo = true
+          class_property bar = true
+                       # ^^^ error: Consider using 'class_property?' for 'bar'
+          class_property baz = true
+                       # ^^^ error: Consider using 'class_property?' for 'baz'
+        end
+        CRYSTAL
+    end
+
+    {% for call in %w[getter class_getter property class_property] %}
+      it "reports `{{ call.id }}` assign with Bool" do
+        expect_issue subject, <<-CRYSTAL, call: {{ call }}
+          class Foo
+            %{call}   foo = true
+            _{call} # ^^^ error: Consider using '%{call}?' for 'foo'
+          end
+          CRYSTAL
+      end
+
+      it "reports `{{ call.id }}` type declaration assign with Bool" do
+        expect_issue subject, <<-CRYSTAL, call: {{ call }}
+          class Foo
+            %{call}   foo : Bool = true
+            _{call} # ^ error: Consider using '%{call}?' for 'foo'
+          end
+          CRYSTAL
+      end
+
+      it "reports `{{ call.id }}` type declaration with Bool" do
+        expect_issue subject, <<-CRYSTAL, call: {{ call }}
+          class Foo
+            %{call}   foo : Bool
+            _{call} # ^ error: Consider using '%{call}?' for 'foo'
+
+            def initialize(@foo = true)
+            end
+          end
+          CRYSTAL
+      end
+    {% end %}
+  end
+end

src/ameba/ast/util.cr

@@ -58,6 +58,13 @@ module Ameba::AST::Util
     is_literal
   end
 
+  # Returns `true` if current `node` is a `Crystal::Path`
+  # matching given *name*, `false` otherwise.
+  def path_named?(node, name) : Bool
+    node.is_a?(Crystal::Path) &&
+      name == node.names.join("::")
+  end
+
   # Returns a source code for the current node.
   # This method uses `node.location` and `node.end_location`
   # to determine and cut a piece of source of the node.

src/ameba/ast/variabling/variable.cr

@@ -122,11 +122,10 @@ module Ameba::AST
     # `false` if not.
     def used_in_macro?(scope = @scope)
       scope.inner_scopes.each do |inner_scope|
-        return true if MacroReferenceFinder.new(inner_scope.node, node.name).references
+        return true if MacroReferenceFinder.new(inner_scope.node, node.name).references?
       end
-      return true if MacroReferenceFinder.new(scope.node, node.name).references
+      return true if MacroReferenceFinder.new(scope.node, node.name).references?
-      return true if (outer_scope = scope.outer_scope) &&
+      return true if (outer_scope = scope.outer_scope) && used_in_macro?(outer_scope)
-                     used_in_macro?(outer_scope)
 
       false
     end
@@ -169,7 +168,7 @@ module Ameba::AST
     end
 
     private class MacroReferenceFinder < Crystal::Visitor
-      property references = false
+      property? references = false
 
       def initialize(node, @reference : String = reference)
         node.accept self

src/ameba/rule/style/is_a_nil.cr

@@ -20,19 +20,19 @@ module Ameba::Rule::Style
   #   Enabled: true
   # ```
   class IsANil < Base
+    include AST::Util
+
     properties do
       description "Disallows calls to `is_a?(Nil)` in favor of `nil?`"
     end
 
-    MSG            = "Use `nil?` instead of `is_a?(Nil)`"
+    MSG = "Use `nil?` instead of `is_a?(Nil)`"
-    PATH_NIL_NAMES = %w(Nil)
 
     def test(source, node : Crystal::IsA)
       return if node.nil_check?
 
       const = node.const
-      return unless const.is_a?(Crystal::Path)
+      return unless path_named?(const, "Nil")
-      return unless const.names == PATH_NIL_NAMES
 
       issue_for const, MSG
     end

src/ameba/rule/style/query_bool_methods.cr

@@ -0,0 +1,72 @@
+module Ameba::Rule::Style
+  # A rule that disallows boolean properties without the `?` suffix - defined
+  # using `Object#(class_)property` or `Object#(class_)getter` macros.
+  #
+  # Favour this:
+  #
+  # ```
+  # class Person
+  #   property? deceased = false
+  #   getter? witty = true
+  # end
+  # ```
+  #
+  # Over this:
+  #
+  # ```
+  # class Person
+  #   property deceased = false
+  #   getter witty = true
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Style/QueryBoolMethods:
+  #   Enabled: true
+  # ```
+  class QueryBoolMethods < Base
+    include AST::Util
+
+    properties do
+      description "Reports boolean properties without the `?` suffix"
+    end
+
+    MSG = "Consider using '%s?' for '%s'"
+
+    CALL_NAMES = %w[getter class_getter property class_property]
+
+    def test(source, node : Crystal::ClassDef | Crystal::ModuleDef)
+      calls =
+        case body = node.body
+        when Crystal::Call
+          [body] if body.name.in?(CALL_NAMES)
+        when Crystal::Expressions
+          body.expressions
+            .select(Crystal::Call)
+            .select!(&.name.in?(CALL_NAMES))
+        end
+
+      return unless calls
+
+      calls.each do |exp|
+        exp.args.each do |arg|
+          name_node, is_bool =
+            case arg
+            when Crystal::Assign
+              {arg.target, arg.value.is_a?(Crystal::BoolLiteral)}
+            when Crystal::TypeDeclaration
+              {arg.var, path_named?(arg.declared_type, "Bool")}
+            else
+              {nil, false}
+            end
+
+          if name_node && is_bool
+            issue_for name_node, MSG % {exp.name, name_node}
+          end
+        end
+      end
+    end
+  end
+end