Merge pull request #374 from crystal-ameba/lint-documentation-rule

Add `Lint/Documentation` rule

.github/workflows/ci.yml

@@ -36,4 +36,4 @@ jobs:
         run: shards build -Dpreview_mt
 
       - name: Run ameba linter
-        run: bin/ameba --all
+        run: bin/ameba

Makefile

@@ -10,7 +10,7 @@ build:
 
 .PHONY: lint
 lint: build
-	./bin/ameba --all
+	./bin/ameba
 
 .PHONY: spec
 spec:

README.md

@@ -99,15 +99,6 @@ $ ameba --explain crystal/command/format.cr:26:83 # same thing
 
 ### Run in parallel
 
-Starting from 0.31.0 Crystal [supports parallelism](https://crystal-lang.org/2019/09/06/parallelism-in-crystal.html).
-It allows to run linting in parallel too.
-In order to take advantage of this feature you need to build ameba with preview_mt support:
-
-```sh
-$ crystal build src/cli.cr -Dpreview_mt -o bin/ameba
-$ make install
-```
-
 Some quick benchmark results measured while running Ameba on Crystal repo:
 
 ```sh

spec/ameba/ast/visitors/scope_visitor_spec.cr

@@ -2,6 +2,17 @@ require "../../../spec_helper"
 
 module Ameba::AST
   describe ScopeVisitor do
+    {% for type in %w[class module enum].map(&.id) %}
+      it "creates a scope for the {{ type }} def" do
+        rule = ScopeRule.new
+        ScopeVisitor.new rule, Source.new <<-CRYSTAL
+          {{ type }} Foo
+          end
+          CRYSTAL
+        rule.scopes.size.should eq 1
+      end
+    {% end %}
+
     it "creates a scope for the def" do
       rule = ScopeRule.new
       ScopeVisitor.new rule, Source.new <<-CRYSTAL
@@ -54,5 +65,33 @@ module Ameba::AST
         outer_block.outer_scope.should be_nil
       end
     end
+
+    context "#visibility" do
+      it "is being properly set" do
+        rule = ScopeRule.new
+        ScopeVisitor.new rule, Source.new <<-CRYSTAL
+          private class Foo
+          end
+          CRYSTAL
+        rule.scopes.size.should eq 1
+        rule.scopes.first.visibility.should eq Crystal::Visibility::Private
+      end
+
+      it "is being inherited from the outer scope(s)" do
+        rule = ScopeRule.new
+        ScopeVisitor.new rule, Source.new <<-CRYSTAL
+          private class Foo
+            class Bar
+              def baz
+              end
+            end
+          end
+          CRYSTAL
+        rule.scopes.size.should eq 3
+        rule.scopes.each &.visibility.should eq Crystal::Visibility::Private
+        rule.scopes.last.node.visibility.should eq Crystal::Visibility::Private
+        rule.scopes[0...-1].each &.node.visibility.should eq Crystal::Visibility::Public
+      end
+    end
   end
 end

spec/ameba/rule/lint/documentation_spec.cr

@@ -0,0 +1,151 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Lint
+  subject = Documentation.new
+    .tap(&.ignore_classes = false)
+    .tap(&.ignore_modules = false)
+    .tap(&.ignore_enums = false)
+    .tap(&.ignore_defs = false)
+    .tap(&.ignore_macros = false)
+
+  describe Documentation do
+    it "passes for undocumented private types" do
+      expect_no_issues subject, <<-CRYSTAL
+        private class Foo
+          def foo
+          end
+        end
+
+        private module Bar
+          def bar
+          end
+        end
+
+        private enum Baz
+        end
+
+        private def bat
+        end
+
+        private macro bag
+        end
+      CRYSTAL
+    end
+
+    it "passes for documented public types" do
+      expect_no_issues subject, <<-CRYSTAL
+        # Foo
+        class Foo
+          # foo
+          def foo
+          end
+        end
+
+        # Bar
+        module Bar
+          # bar
+          def bar
+          end
+        end
+
+        # Baz
+        enum Baz
+        end
+
+        # bat
+        def bat
+        end
+
+        # bag
+        macro bag
+        end
+      CRYSTAL
+    end
+
+    it "fails if there is an undocumented public type" do
+      expect_issue subject, <<-CRYSTAL
+        class Foo
+      # ^^^^^^^^^ error: Missing documentation
+        end
+
+        module Bar
+      # ^^^^^^^^^^ error: Missing documentation
+        end
+
+        enum Baz
+      # ^^^^^^^^ error: Missing documentation
+        end
+
+        def bat
+      # ^^^^^^^ error: Missing documentation
+        end
+
+        macro bag
+      # ^^^^^^^^^ error: Missing documentation
+        end
+      CRYSTAL
+    end
+
+    context "properties" do
+      describe "#ignore_classes" do
+        it "lets the rule to ignore method definitions if true" do
+          rule = Documentation.new
+          rule.ignore_classes = true
+
+          expect_no_issues rule, <<-CRYSTAL
+            class Foo
+            end
+            CRYSTAL
+        end
+      end
+
+      describe "#ignore_modules" do
+        it "lets the rule to ignore method definitions if true" do
+          rule = Documentation.new
+          rule.ignore_modules = true
+
+          expect_no_issues rule, <<-CRYSTAL
+            module Bar
+            end
+            CRYSTAL
+        end
+      end
+
+      describe "#ignore_enums" do
+        it "lets the rule to ignore method definitions if true" do
+          rule = Documentation.new
+          rule.ignore_enums = true
+
+          expect_no_issues rule, <<-CRYSTAL
+            enum Baz
+            end
+            CRYSTAL
+        end
+      end
+
+      describe "#ignore_defs" do
+        it "lets the rule to ignore method definitions if true" do
+          rule = Documentation.new
+          rule.ignore_defs = true
+
+          expect_no_issues rule, <<-CRYSTAL
+            def bat
+            end
+            CRYSTAL
+        end
+      end
+
+      describe "#ignore_macros" do
+        it "lets the rule to ignore macros if true" do
+          rule = Documentation.new
+          rule.ignore_macros = true
+
+          expect_no_issues rule, <<-CRYSTAL
+            macro bag
+            end
+            CRYSTAL
+        end
+      end
+    end
+  end
+end

spec/spec_helper.cr

@@ -43,6 +43,9 @@ module Ameba
       description "Internal rule to test scopes"
     end
 
+    def test(source, node : Crystal::VisibilityModifier, scope : AST::Scope)
+    end
+
     def test(source, node : Crystal::ASTNode, scope : AST::Scope)
       @scopes << scope
     end

src/ameba/ast/scope.cr

@@ -7,6 +7,9 @@ module Ameba::AST
     # Whether the scope yields.
     setter yields = false
 
+    # Scope visibility level
+    setter visibility : Crystal::Visibility?
+
     # Link to local variables
     getter variables = [] of Variable
 
@@ -121,10 +124,7 @@ module Ameba::AST
     # end
     # ```
     def spawn_block?
-      return false unless node.is_a?(Crystal::Block)
-
-      call = node.as(Crystal::Block).call
-      call.try(&.name) == "spawn"
+      node.as?(Crystal::Block).try(&.call).try(&.name) == "spawn"
     end
 
     # Returns `true` if current scope sits inside a macro.
@@ -167,9 +167,12 @@ module Ameba::AST
     # Returns `true` if current scope (or any of inner scopes) yields,
     # `false` otherwise.
     def yields?(check_inner_scopes = true)
-      return true if @yields
-      return inner_scopes.any?(&.yields?) if check_inner_scopes
-      false
+      @yields || (check_inner_scopes && inner_scopes.any?(&.yields?))
+    end
+
+    # Returns visibility of the current scope (could be inherited from the outer scope).
+    def visibility
+      @visibility || outer_scope.try(&.visibility)
     end
 
     # Returns `true` if current scope is a def, `false` otherwise.

src/ameba/ast/visitors/node_visitor.cr

@@ -1,34 +1,6 @@
 require "./base_visitor"
 
 module Ameba::AST
-  # List of nodes to be visited by Ameba's rules.
-  NODES = {
-    Alias,
-    IsA,
-    Assign,
-    Call,
-    Block,
-    Case,
-    ClassDef,
-    ClassVar,
-    Def,
-    EnumDef,
-    ExceptionHandler,
-    Expressions,
-    HashLiteral,
-    If,
-    InstanceVar,
-    LibDef,
-    ModuleDef,
-    NilLiteral,
-    StringInterpolation,
-    Unless,
-    Var,
-    When,
-    While,
-    Until,
-  }
-
   # An AST Visitor that traverses the source and allows all nodes
   # to be inspected by rules.
   #
@@ -36,6 +8,34 @@ module Ameba::AST
   # visitor = Ameba::AST::NodeVisitor.new(rule, source)
   # ```
   class NodeVisitor < BaseVisitor
+    # List of nodes to be visited by Ameba's rules.
+    NODES = {
+      Alias,
+      IsA,
+      Assign,
+      Call,
+      Block,
+      Case,
+      ClassDef,
+      ClassVar,
+      Def,
+      EnumDef,
+      ExceptionHandler,
+      Expressions,
+      HashLiteral,
+      If,
+      InstanceVar,
+      LibDef,
+      ModuleDef,
+      NilLiteral,
+      StringInterpolation,
+      Unless,
+      Var,
+      When,
+      While,
+      Until,
+    }
+
     @skip : Array(Crystal::ASTNode.class)?
 
     def initialize(@rule, @source, skip = nil)
@@ -43,6 +43,11 @@ module Ameba::AST
       super @rule, @source
     end
 
+    def visit(node : Crystal::VisibilityModifier)
+      node.exp.visibility = node.modifier
+      true
+    end
+
     {% for name in NODES %}
       # A visit callback for `Crystal::{{ name }}` node.
       #

src/ameba/ast/visitors/scope_visitor.cr

@@ -25,6 +25,7 @@ module Ameba::AST
     @scope_queue = [] of Scope
     @current_scope : Scope
     @current_assign : Crystal::ASTNode?
+    @visibility_modifier : Crystal::Visibility?
     @skip : Array(Crystal::ASTNode.class)?
 
     def initialize(@rule, @source, skip = nil)
@@ -36,12 +37,18 @@ module Ameba::AST
 
     private def on_scope_enter(node)
       return if skip?(node)
-      @current_scope = Scope.new(node, @current_scope)
+
+      scope = Scope.new(node, @current_scope)
+      scope.visibility = @visibility_modifier
+
+      @current_scope = scope
     end
 
     private def on_scope_end(node)
       @scope_queue << @current_scope
 
+      @visibility_modifier = nil
+
       # go up if this is not a top level scope
       return unless outer_scope = @current_scope.outer_scope
       @current_scope = outer_scope
@@ -64,6 +71,12 @@ module Ameba::AST
       end
     {% end %}
 
+    # :nodoc:
+    def visit(node : Crystal::VisibilityModifier)
+      @visibility_modifier = node.exp.visibility = node.modifier
+      true
+    end
+
     # :nodoc:
     def visit(node : Crystal::Yield)
       @current_scope.yields = true
@@ -109,6 +122,7 @@ module Ameba::AST
       @current_assign = node.value unless node.value.nil?
     end
 
+    # :nodoc:
     def end_visit(node : Crystal::TypeDeclaration)
       return unless (var = node.var).is_a?(Crystal::Var)
 

src/ameba/config.cr

@@ -240,6 +240,7 @@ class Ameba::Config
 
   # :nodoc:
   module RuleConfig
+    # Define rule properties
     macro properties(&block)
       {% definitions = [] of NamedTuple %}
       {% if block.body.is_a? Assign %}

src/ameba/rule/base.cr

@@ -112,6 +112,7 @@ module Ameba::Rule
       name.hash
     end
 
+    # Adds an issue to the *source*
     macro issue_for(*args, **kwargs, &block)
       source.add_issue(self, {{ *args }}, {{ **kwargs }}) {{ block }}
     end

src/ameba/rule/lint/documentation.cr

@@ -0,0 +1,66 @@
+module Ameba::Rule::Lint
+  # A rule that enforces documentation for public types:
+  # modules, classes, enums, methods and macros.
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Lint/Documentation:
+  #   Enabled: true
+  # ```
+  class Documentation < Base
+    properties do
+      enabled false
+      description "Enforces public types to be documented"
+
+      ignore_classes false
+      ignore_modules true
+      ignore_enums false
+      ignore_defs true
+      ignore_macros false
+    end
+
+    MSG = "Missing documentation"
+
+    MACRO_HOOK_NAMES = %w[
+      inherited
+      included extended
+      method_missing method_added
+      finished
+    ]
+
+    def test(source)
+      AST::ScopeVisitor.new self, source
+    end
+
+    def test(source, node : Crystal::ClassDef, scope : AST::Scope)
+      ignore_classes? || check_missing_doc(source, node, scope)
+    end
+
+    def test(source, node : Crystal::ModuleDef, scope : AST::Scope)
+      ignore_modules? || check_missing_doc(source, node, scope)
+    end
+
+    def test(source, node : Crystal::EnumDef, scope : AST::Scope)
+      ignore_enums? || check_missing_doc(source, node, scope)
+    end
+
+    def test(source, node : Crystal::Def, scope : AST::Scope)
+      ignore_defs? || check_missing_doc(source, node, scope)
+    end
+
+    def test(source, node : Crystal::Macro, scope : AST::Scope)
+      node.name.in?(MACRO_HOOK_NAMES) ||
+        ignore_macros? || check_missing_doc(source, node, scope)
+    end
+
+    private def check_missing_doc(source, node, scope)
+      visibility = scope.visibility
+
+      return if visibility && !visibility.public?
+      return if node.doc.presence
+
+      issue_for(node, MSG)
+    end
+  end
+end