Merge pull request #460 from crystal-ameba/fix-issue-459

Make sure we only return files from `GlobUtils#expand` method

.github/workflows/ci.yml

@@ -19,7 +19,7 @@ jobs:
 
     steps:
       - name: Set timezone to UTC
-        uses: szenius/set-timezone@v1.2
+        uses: szenius/set-timezone@v2.0
 
       - name: Install Crystal
         uses: crystal-lang/install-crystal@v1

Makefile

@@ -1,34 +1,92 @@
+.POSIX:
+all:
+
+# Recipes
+
+## Build ameba
+##   $ make
+## Run tests
+##   $ make test
+## Install ameba
+##   $ sudo make install
+
+-include Makefile.local # for optional local options
+
+BUILD_TARGET ::= bin/ameba
+
+DESTDIR ?=          ## Install destination dir
+PREFIX ?= /usr/local## Install path prefix
+BINDIR ?= $(DESTDIR)$(PREFIX)/bin
+
+# The crystal command to use
 CRYSTAL_BIN ?= crystal
+# The shards command to use
 SHARDS_BIN ?= shards
-PREFIX ?= /usr/local
+# The install command to use
+INSTALL_BIN ?= /usr/bin/install
+
 SHARD_BIN ?= ../../bin
 CRFLAGS ?= -Dpreview_mt
 
+SRC_SOURCES ::= $(shell find src -name '*.cr' 2>/dev/null)
+DOC_SOURCE   ::= src/**
+
+.PHONY: all
+all: build
+
 .PHONY: build
-build:
+build: ## Build the application binary
+build: $(BUILD_TARGET)
+
+$(BUILD_TARGET): $(SRC_SOURCES)
 	$(SHARDS_BIN) build $(CRFLAGS)
 
+docs: ## Generate API docs
+docs: $(SRC_SOURCES)
+	$(CRYSTAL_BIN) docs -o docs $(DOC_SOURCE)
+
 .PHONY: lint
-lint: build
-	./bin/ameba
+lint: ## Run ameba on ameba's code base
+lint: $(BUILD_TARGET)
+	$(BUILD_TARGET)
 
 .PHONY: spec
+spec: ## Run the spec suite
 spec:
 	$(CRYSTAL_BIN) spec
 
 .PHONY: clean
+clean: ## Remove application binary
 clean:
-	rm -f ./bin/ameba ./bin/ameba.dwarf
+	@rm -f "$(BUILD_TARGET)" "$(BUILD_TARGET).dwarf"
 
 .PHONY: install
-install: build
-	mkdir -p $(PREFIX)/bin
-	cp ./bin/ameba $(PREFIX)/bin
+install: ## Install application binary into $DESTDIR
+install: $(BUILD_TARGET)
+	$(INSTALL_BIN) -m 0755 "$(BUILD_TARGET)" "$(BINDIR)/ameba"
 
 .PHONY: bin
 bin: build
 	mkdir -p $(SHARD_BIN)
-	cp ./bin/ameba $(SHARD_BIN)
+	cp $(BUILD_TARGET) $(SHARD_BIN)
 
 .PHONY: test
+test: ## Run the spec suite and linter
 test: spec lint
+
+.PHONY: help
+help: ## Show this help
+	@echo
+	@printf '\033[34mtargets:\033[0m\n'
+	@grep -hE '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) |\
+		sort |\
+		awk 'BEGIN {FS = ":.*?## "}; {printf "  \033[36m%-15s\033[0m %s\n", $$1, $$2}'
+	@echo
+	@printf '\033[34moptional variables:\033[0m\n'
+	@grep -hE '^[a-zA-Z_-]+ \?=.*?## .*$$' $(MAKEFILE_LIST) |\
+		sort |\
+		awk 'BEGIN {FS = " \\?=.*?## "}; {printf "  \033[36m%-15s\033[0m %s\n", $$1, $$2}'
+	@echo
+	@printf '\033[34mrecipes:\033[0m\n'
+	@grep -hE '^##.*$$' $(MAKEFILE_LIST) |\
+		awk 'BEGIN {FS = "## "}; /^## [a-zA-Z_-]/ {printf "  \033[36m%s\033[0m\n", $$2}; /^##  / {printf "  %s\n", $$2}'

spec/ameba/ast/scope_spec.cr

@@ -180,6 +180,28 @@ module Ameba::AST
     end
   end
 
+  describe "#def?" do
+    context "when check_outer_scopes: true" do
+      it "returns true if outer scope is Crystal::Def" do
+        nodes = as_nodes("def foo; 3.times {}; end")
+        outer_scope = Scope.new nodes.def_nodes.first
+        scope = Scope.new nodes.block_nodes.first, outer_scope
+        scope.def?(check_outer_scopes: true).should be_true
+      end
+    end
+
+    it "returns true if Crystal::Def" do
+      nodes = as_nodes("def foo; end")
+      scope = Scope.new nodes.def_nodes.first
+      scope.def?.should be_true
+    end
+
+    it "returns false otherwise" do
+      scope = Scope.new as_node("a = 1")
+      scope.def?.should be_false
+    end
+  end
+
   describe "#in_macro?" do
     it "returns true if Crystal::Macro" do
       nodes = as_nodes <<-CRYSTAL

spec/ameba/glob_utils_spec.cr

@@ -1,11 +1,7 @@
 require "../spec_helper"
 
 module Ameba
-  struct GlobUtilsClass
-    include GlobUtils
-  end
-
-  subject = GlobUtilsClass.new
+  subject = GlobUtils
   current_file_basename = File.basename(__FILE__)
   current_file_path = "spec/ameba/#{current_file_basename}"
 
@@ -45,6 +41,12 @@ module Ameba
         subject.expand(["**/#{current_file_basename}", "**/#{current_file_basename}"])
           .should eq [current_file_path]
       end
+
+      it "does not list folders" do
+        subject.expand(["**/*"]).each do |path|
+          fail "#{path.inspect} should be a file" unless File.file?(path)
+        end
+      end
     end
   end
 end

spec/ameba/rule/lint/spec_filename_spec.cr

@@ -4,6 +4,16 @@ module Ameba::Rule::Lint
   subject = SpecFilename.new
 
   describe SpecFilename do
+    it "passes if relative file path does not start with `spec/`" do
+      expect_no_issues subject, code: "", path: "src/spec/foo.cr"
+      expect_no_issues subject, code: "", path: "src/spec/foo/bar.cr"
+    end
+
+    it "passes if file extension is not `.cr`" do
+      expect_no_issues subject, code: "", path: "spec/foo.json"
+      expect_no_issues subject, code: "", path: "spec/foo/bar.json"
+    end
+
     it "passes if filename is correct" do
       expect_no_issues subject, code: "", path: "spec/foo_spec.cr"
       expect_no_issues subject, code: "", path: "spec/foo/bar_spec.cr"

spec/ameba/rule/lint/useless_assign_spec.cr

@@ -388,13 +388,30 @@ module Ameba::Rule::Lint
           CRYSTAL
       end
 
-      it "doesn't report if this is a record declaration" do
+      it "doesn't report record declaration" do
         expect_no_issues subject, <<-CRYSTAL
+          record Foo, foo : String
           record Foo, foo = "foo"
           CRYSTAL
       end
 
-      it "doesn't report if this is an accessor declaration" do
+      it "doesn't report record declarations (generics)" do
+        expect_no_issues subject, <<-CRYSTAL
+          record Foo(T), foo : T
+          record Foo(T), foo = T.new
+          CRYSTAL
+      end
+
+      pending "doesn't report type declaration as a call argument" do
+        expect_no_issues subject, <<-CRYSTAL
+          foo Foo(T), foo : T
+          foo Foo, foo : Nil
+          foo foo : String
+          foo foo : String, bar : Int32?, baz : Bool
+          CRYSTAL
+      end
+
+      it "doesn't report accessor declarations" do
         accessor_macros = %w[setter class_setter]
         %w[getter class_getter property class_property].each do |name|
           accessor_macros << name
@@ -445,7 +462,7 @@ module Ameba::Rule::Lint
         expect_issue subject, <<-CRYSTAL
           class A
             foo : String? = "foo"
-          # ^^^^^^^^^^^^^^^^^^^^^ error: Useless assignment to variable `foo`
+          # ^^^ error: Useless assignment to variable `foo`
 
             def method
               foo = "bar"
@@ -976,7 +993,7 @@ module Ameba::Rule::Lint
       it "reports if it's not referenced at a top level" do
         expect_issue subject, <<-CRYSTAL
           a : String?
-          # ^^^^^^^^^ error: Useless assignment to variable `a`
+          # ^{} error: Useless assignment to variable `a`
           CRYSTAL
       end
 
@@ -984,7 +1001,7 @@ module Ameba::Rule::Lint
         expect_issue subject, <<-CRYSTAL
           def foo
             a : String?
-          # ^^^^^^^^^^^ error: Useless assignment to variable `a`
+          # ^ error: Useless assignment to variable `a`
           end
           CRYSTAL
       end
@@ -993,7 +1010,17 @@ module Ameba::Rule::Lint
         expect_issue subject, <<-CRYSTAL
           class Foo
             a : String?
-          # ^^^^^^^^^^^ error: Useless assignment to variable `a`
+          # ^ error: Useless assignment to variable `a`
+          end
+          CRYSTAL
+      end
+
+      it "doesn't report if it's referenced in a lib" do
+        expect_no_issues subject, <<-CRYSTAL
+          lib LibFoo
+            struct Foo
+              a : Int32
+            end
           end
           CRYSTAL
       end

src/ameba/ast/scope.cr

@@ -180,20 +180,15 @@ module Ameba::AST
       @visibility || outer_scope.try(&.visibility)
     end
 
-    # Returns `true` if current scope is a def, `false` otherwise.
-    def def?
-      node.is_a?(Crystal::Def)
-    end
-
-    # Returns `true` if current scope is a class, `false` otherwise.
-    def class_def?
-      node.is_a?(Crystal::ClassDef)
-    end
-
-    # Returns `true` if current scope is a module, `false` otherwise.
-    def module_def?
-      node.is_a?(Crystal::ModuleDef)
-    end
+    {% for type in %w[Def ClassDef ModuleDef EnumDef LibDef FunDef].map(&.id) %}
+      {% method_name = type.underscore %}
+      # Returns `true` if current scope is a {{ method_name[0..-5] }} def, `false` otherwise.
+      def {{ method_name }}?(*, check_outer_scopes = false)
+        node.is_a?(Crystal::{{ type }}) ||
+          !!(check_outer_scopes &&
+            outer_scope.try(&.{{ method_name }}?(check_outer_scopes: true)))
+      end
+    {% end %}
 
     # Returns `true` if this scope is a top level scope, `false` otherwise.
     def top_level?

src/ameba/ast/visitors/scope_visitor.cr

@@ -173,9 +173,10 @@ module Ameba::AST
       scope = @current_scope
 
       case
-      when scope.top_level? && record_macro?(node)         then return false
-      when scope.type_definition? && record_macro?(node)   then return false
-      when scope.type_definition? && accessor_macro?(node) then return false
+      when (scope.top_level? || scope.type_definition?) && record_macro?(node)
+        return false
+      when scope.type_definition? && accessor_macro?(node)
+        return false
       when scope.def? && special_node?(node)
         scope.arguments.each do |arg|
           ref = arg.variable.reference(scope)
@@ -194,7 +195,14 @@ module Ameba::AST
     end
 
     private def record_macro?(node)
-      node.name == "record" && node.args.first?.is_a?(Crystal::Path)
+      return false unless node.name == "record"
+
+      case node.args.first?
+      when Crystal::Path, Crystal::Generic
+        true
+      else
+        false
+      end
     end
 
     private def skip?(node)

src/ameba/glob_utils.cr

@@ -1,6 +1,8 @@
 module Ameba
   # Helper module that is utilizes helpers for working with globs.
   module GlobUtils
+    extend self
+
     # Returns all files that match specified globs.
     # Globs can have wildcards or be rejected:
     #
@@ -20,10 +22,13 @@ module Ameba
     # expand(["spec/*.cr", "src"]) # => all files in src folder + first level specs
     # ```
     def expand(globs)
-      globs.flat_map do |glob|
-        glob += "/**/*.cr" if File.directory?(glob)
-        Dir[glob]
-      end.uniq!
+      globs
+        .flat_map do |glob|
+          glob += "/**/*.cr" if File.directory?(glob)
+          Dir[glob]
+        end
+        .uniq!
+        .select! { |path| File.file?(path) }
     end
 
     private def rejected_globs(globs)

src/ameba/rule/lint/spec_filename.cr

@@ -8,6 +8,8 @@ module Ameba::Rule::Lint
   # ```
   # Lint/SpecFilename:
   #   Enabled: true
+  #   IgnoredDirs: [spec/support spec/fixtures spec/data]
+  #   IgnoredFilenames: [spec_helper]
   # ```
   class SpecFilename < Base
     properties do
@@ -26,8 +28,10 @@ module Ameba::Rule::Lint
       name = path_.stem
       path = path_.to_s
 
-      # check files only within spec/ directory
+      # check only files within spec/ directory
       return unless path.starts_with?("spec/")
+      # check only files with `.cr` extension
+      return unless path.ends_with?(".cr")
       # ignore files having `_spec` suffix
       return if name.ends_with?("_spec")
 

src/ameba/rule/lint/useless_assign.cr

@@ -39,15 +39,27 @@ module Ameba::Rule::Lint
     end
 
     def test(source, node, scope : AST::Scope)
+      return if scope.lib_def?(check_outer_scopes: true)
+
       scope.variables.each do |var|
         next if var.ignored? || var.used_in_macro? || var.captured_by_block?
         next if exclude_type_declarations? && scope.assigns_type_dec?(var.name)
 
         var.assignments.each do |assign|
-          next if assign.referenced?
-          issue_for assign.target_node, MSG % var.name
+          check_assignment(source, assign, var)
         end
       end
     end
+
+    private def check_assignment(source, assign, var)
+      return if assign.referenced?
+
+      case target_node = assign.target_node
+      when Crystal::TypeDeclaration
+        issue_for target_node.var, MSG % var.name
+      else
+        issue_for target_node, MSG % var.name
+      end
+    end
   end
 end