From 7192635bbd2866d52208f019ba975bba5908139b Mon Sep 17 00:00:00 2001
From: Sijawusz Pur Rahnama <sija@sija.pl>
Date: Fri, 24 Jan 2025 23:17:31 +0100
Subject: [PATCH 1/2] Add `Style/RedundantSelf` rule

---
 ...e_calls_with_self_receiver_visitor_spec.cr | 113 +++++++
 spec/ameba/rule/style/redundant_self_spec.cr  | 295 ++++++++++++++++++
 spec/spec_helper.cr                           |  14 +
 .../scope_calls_with_self_receiver_visitor.cr | 129 ++++++++
 src/ameba/rule/style/redundant_self.cr        | 110 +++++++
 5 files changed, 661 insertions(+)
 create mode 100644 spec/ameba/ast/visitors/scope_calls_with_self_receiver_visitor_spec.cr
 create mode 100644 spec/ameba/rule/style/redundant_self_spec.cr
 create mode 100644 src/ameba/ast/visitors/scope_calls_with_self_receiver_visitor.cr
 create mode 100644 src/ameba/rule/style/redundant_self.cr

diff --git a/spec/ameba/ast/visitors/scope_calls_with_self_receiver_visitor_spec.cr b/spec/ameba/ast/visitors/scope_calls_with_self_receiver_visitor_spec.cr
new file mode 100644
index 000000000..b721cae72
--- /dev/null
+++ b/spec/ameba/ast/visitors/scope_calls_with_self_receiver_visitor_spec.cr
@@ -0,0 +1,113 @@
+require "../../../spec_helper"
+
+module Ameba::AST
+  describe ScopeCallsWithSelfReceiverVisitor do
+    {% for type in %w[class module].map(&.id) %}
+      it "creates a scope for the {{ type }} def" do
+        rule = SelfCallsRule.new
+        visitor = ScopeCallsWithSelfReceiverVisitor.new rule, Source.new <<-CRYSTAL
+          {{ type }} Foo
+            self.foo
+          end
+          CRYSTAL
+
+        call_queue = visitor.scope_call_queue
+        call_queue.size.should eq 1
+        call_queue.should eq rule.call_queue
+
+        scope, calls = call_queue.first
+
+        node = scope.node.should be_a(Crystal::{{ type.capitalize }}Def)
+        node.name.should be_a(Crystal::Path)
+        node.name.to_s.should eq "Foo"
+
+        calls.size.should eq 1
+        calls.first.name.should eq "foo"
+      end
+    {% end %}
+
+    it "creates a scope for the def" do
+      rule = SelfCallsRule.new
+      visitor = ScopeCallsWithSelfReceiverVisitor.new rule, Source.new <<-CRYSTAL
+        class Foo
+          def method
+            self.foo :a, :b, :c
+            self.bar
+            baz :x, :y, :z
+            it_is_a.bat "country"
+            self
+          end
+        end
+        CRYSTAL
+
+      call_queue = visitor.scope_call_queue
+      call_queue.size.should eq 1
+      call_queue.should eq rule.call_queue
+
+      scope, calls = call_queue.first
+
+      node = scope.node.should be_a(Crystal::Def)
+      node.name.should eq "method"
+
+      calls.size.should eq 2
+      calls.first.name.should eq "foo"
+      calls.last.name.should eq "bar"
+    end
+
+    it "creates a scope for the proc" do
+      rule = SelfCallsRule.new
+      visitor = ScopeCallsWithSelfReceiverVisitor.new rule, Source.new <<-CRYSTAL
+        -> { self.foo }
+        CRYSTAL
+
+      call_queue = visitor.scope_call_queue
+      call_queue.size.should eq 1
+      call_queue.should eq rule.call_queue
+    end
+
+    it "creates a scope for the block" do
+      rule = SelfCallsRule.new
+      visitor = ScopeCallsWithSelfReceiverVisitor.new rule, Source.new <<-CRYSTAL
+        3.times { self.foo }
+        CRYSTAL
+
+      call_queue = visitor.scope_call_queue
+      call_queue.size.should eq 1
+      call_queue.should eq rule.call_queue
+    end
+
+    context "inner scopes" do
+      it "creates scope for block inside def" do
+        rule = SelfCallsRule.new
+        visitor = ScopeCallsWithSelfReceiverVisitor.new rule, Source.new <<-CRYSTAL
+          def method
+            self.foo
+            3.times { self.bar }
+          end
+          CRYSTAL
+
+        call_queue = visitor.scope_call_queue
+        call_queue.size.should eq 2
+        call_queue.should eq rule.call_queue
+        call_queue.last_key.outer_scope.should eq call_queue.first_key
+      end
+
+      it "creates scope for block inside block" do
+        rule = SelfCallsRule.new
+        visitor = ScopeCallsWithSelfReceiverVisitor.new rule, Source.new <<-CRYSTAL
+          def method
+            3.times do
+              self.bar
+              2.times { self.baz }
+            end
+          end
+          CRYSTAL
+
+        call_queue = visitor.scope_call_queue
+        call_queue.size.should eq 2
+        call_queue.should eq rule.call_queue
+        call_queue.last_key.outer_scope.should eq call_queue.first_key
+      end
+    end
+  end
+end
diff --git a/spec/ameba/rule/style/redundant_self_spec.cr b/spec/ameba/rule/style/redundant_self_spec.cr
new file mode 100644
index 000000000..3b749de32
--- /dev/null
+++ b/spec/ameba/rule/style/redundant_self_spec.cr
@@ -0,0 +1,295 @@
+require "../../../spec_helper"
+
+module Ameba::Rule::Style
+  describe RedundantSelf do
+    subject = RedundantSelf.new
+
+    it "does not report solitary `self` reference" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def bar
+            self
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report calls without a receiver" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            foo || 42
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in a method call with a reserved keyword" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo
+            if self.responds_to?(:with)
+              self.with { 42 }
+            end
+            self.is_a?(Foo) ? self.class : self.as?(Foo)
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in the presence of a method argument with the same name" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def bar(foo)
+            foo || self.foo
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in the presence of a block argument with the same name" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo
+            42
+          end
+
+          def bar
+            [1, 11, 111].map { |foo| self.foo + foo }
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in the presence of a method argument with the same name inherited from the parent scope" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo
+            42
+          end
+
+          def bar(foo)
+            [1, 11, 111].map { |n| self.foo + n }
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in the presence of a proc argument with the same name" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo
+            42
+          end
+
+          def bar
+            ->(foo : Int32) { self.foo + foo }
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used within the definition of a variable with the same name" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            foo = self.foo
+            foo
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in the presence of a variable with the same name" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            foo = 42
+            bar = self.foo
+            bar
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in the presence of a type declaration variable with the same name" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            foo : Int32 = 42
+            bar = self.foo
+            bar
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used in the presence of a type declaration variable with the same name (2)" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            foo : Int32?
+            bar = self.foo
+            bar
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used with a setter" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def foo=(value); end
+
+          def foo!
+            self.foo = 42
+          end
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used with an operator" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def +(value); end
+          def %(value); end
+          def <<(value); end
+
+          def foo
+            self + "foo"
+            self % "foo"
+            self << "foo"
+          end
+
+          self | String
+        end
+        CRYSTAL
+    end
+
+    it "does not report if `self` is used with a square bracket operator" do
+      expect_no_issues subject, <<-CRYSTAL
+        class Foo
+          def []?(i); end
+          def [](i); end
+          def []=(i, value); end
+
+          def foo?
+            self[0]?
+          end
+
+          def foo
+            self[0]
+          end
+
+          def foo!
+            self[0] = 42
+          end
+        end
+        CRYSTAL
+    end
+
+    it "reports if there is redundant `self` used in a method body" do
+      source = expect_issue subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            self.foo || 42
+          # ^^^^ error: Redundant `self` detected
+          end
+        end
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            foo || 42
+          end
+        end
+        CRYSTAL
+    end
+
+    it "reports if there is redundant `self` used in a method arguments' default values" do
+      source = expect_issue subject, <<-CRYSTAL
+        class Foo
+          def foo
+            42
+          end
+
+          def foo!(bar = self.foo, baz = false)
+                       # ^^^^ error: Redundant `self` detected
+          end
+        end
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL
+        class Foo
+          def foo
+            42
+          end
+
+          def foo!(bar = foo, baz = false)
+          end
+        end
+        CRYSTAL
+    end
+
+    it "reports if there is redundant `self` used in a string interpolation" do
+      source = expect_issue subject, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            "\#{self.foo || 42}"
+             # ^^^^ error: Redundant `self` detected
+          end
+        end
+        CRYSTAL
+
+      expect_correction source, <<-CRYSTAL
+        class Foo
+          def foo; end
+
+          def foo!
+            "\#{foo || 42}"
+          end
+        end
+        CRYSTAL
+    end
+
+    {% for keyword in %w[class module].map(&.id) %}
+      it "reports if there is redundant `self` within {{ keyword }} definition" do
+        source = expect_issue subject, <<-CRYSTAL
+          {{ keyword }} Foo
+            def self.foo; end
+
+            self.foo
+          # ^^^^ error: Redundant `self` detected
+          end
+          CRYSTAL
+
+        expect_correction source, <<-CRYSTAL
+          {{ keyword }} Foo
+            def self.foo; end
+
+            foo
+          end
+          CRYSTAL
+      end
+    {% end %}
+  end
+end
diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr
index e8de14918..5573db455 100644
--- a/spec/spec_helper.cr
+++ b/spec/spec_helper.cr
@@ -62,6 +62,20 @@ module Ameba
     end
   end
 
+  class SelfCallsRule < Rule::Base
+    @[YAML::Field(ignore: true)]
+    getter call_queue = {} of AST::Scope => Array(Crystal::Call)
+
+    properties do
+      description "Internal rule to self calls in test scopes"
+    end
+
+    def test(source, node : Crystal::Call, scope : AST::Scope)
+      @call_queue[scope] ||= [] of Crystal::Call
+      @call_queue[scope] << node
+    end
+  end
+
   class FlowExpressionRule < Rule::Base
     @[YAML::Field(ignore: true)]
     getter expressions = [] of AST::FlowExpression
diff --git a/src/ameba/ast/visitors/scope_calls_with_self_receiver_visitor.cr b/src/ameba/ast/visitors/scope_calls_with_self_receiver_visitor.cr
new file mode 100644
index 000000000..5a52e5401
--- /dev/null
+++ b/src/ameba/ast/visitors/scope_calls_with_self_receiver_visitor.cr
@@ -0,0 +1,129 @@
+module Ameba::AST
+  class ScopeCallsWithSelfReceiverVisitor < Crystal::Visitor
+    @current_scope : AST::Scope
+    @current_assign : Crystal::ASTNode?
+
+    getter scope_call_queue = {} of AST::Scope => Array(Crystal::Call)
+
+    def initialize(rule, source)
+      @current_scope = AST::Scope.new(source.ast) # top level scope
+
+      source.ast.accept self
+
+      scope_call_queue.each do |scope, calls|
+        calls.each do |call|
+          rule.test source, call, scope
+        end
+      end
+    end
+
+    private def on_scope_enter(node)
+      scope = AST::Scope.new(node, @current_scope)
+
+      @current_scope = scope
+    end
+
+    private def on_scope_end(node)
+      # go up if this is not a top level scope
+      if outer_scope = @current_scope.outer_scope
+        @current_scope = outer_scope
+      end
+    end
+
+    private def on_assign_end(target, node)
+      target.is_a?(Crystal::Var) &&
+        @current_scope.assign_variable(target.name, node)
+    end
+
+    # A main visit method that accepts `Crystal::ASTNode`.
+    # Returns `true`, meaning all child nodes will be traversed.
+    def visit(node : Crystal::ASTNode)
+      true
+    end
+
+    def end_visit(node : Crystal::ASTNode)
+      on_scope_end(node) if @current_scope.eql?(node)
+    end
+
+    def visit(node : Crystal::Def)
+      node.name == "->" || on_scope_enter(node)
+    end
+
+    def visit(node : Crystal::Block | Crystal::ProcLiteral)
+      on_scope_enter(node)
+    end
+
+    def visit(node : Crystal::ClassDef | Crystal::ModuleDef)
+      on_scope_enter(node)
+    end
+
+    def visit(node : Crystal::Assign | Crystal::OpAssign | Crystal::MultiAssign | Crystal::UninitializedVar)
+      @current_assign = node
+    end
+
+    def end_visit(node : Crystal::Assign | Crystal::OpAssign)
+      on_assign_end(node.target, node)
+      @current_assign = nil
+    end
+
+    def end_visit(node : Crystal::MultiAssign)
+      node.targets.each { |target| on_assign_end(target, node) }
+      @current_assign = nil
+    end
+
+    def end_visit(node : Crystal::UninitializedVar)
+      on_assign_end(node.var, node)
+      @current_assign = nil
+    end
+
+    def visit(node : Crystal::TypeDeclaration)
+      return unless (var = node.var).is_a?(Crystal::Var)
+
+      @current_scope.add_variable(var)
+      @current_scope.add_type_dec_variable(node)
+
+      @current_assign = node.value if node.value
+    end
+
+    def end_visit(node : Crystal::TypeDeclaration)
+      return unless (var = node.var).is_a?(Crystal::Var)
+
+      on_assign_end(var, node)
+      @current_assign = nil
+    end
+
+    def visit(node : Crystal::Arg)
+      @current_scope.add_argument(node)
+    end
+
+    def visit(node : Crystal::InstanceVar)
+      @current_scope.add_ivariable(node)
+    end
+
+    def visit(node : Crystal::Var)
+      scope = @current_scope
+      variable = scope.find_variable(node.name)
+
+      case
+      when scope.arg?(node) # node is an argument
+        scope.add_argument(node)
+      when variable.nil? && @current_assign # node is a variable
+        scope.add_variable(node)
+      when variable # node is a reference
+        reference = variable.reference(node, scope)
+        if @current_assign.is_a?(Crystal::OpAssign) || !reference.target_of?(@current_assign)
+          variable.reference_assignments!
+        end
+      end
+    end
+
+    def visit(node : Crystal::Call)
+      if (obj = node.obj).is_a?(Crystal::Var) && obj.name == "self"
+        calls = @scope_call_queue[@current_scope] ||= [] of Crystal::Call
+        calls << node
+      end
+
+      true
+    end
+  end
+end
diff --git a/src/ameba/rule/style/redundant_self.cr b/src/ameba/rule/style/redundant_self.cr
new file mode 100644
index 000000000..6a493d069
--- /dev/null
+++ b/src/ameba/rule/style/redundant_self.cr
@@ -0,0 +1,110 @@
+require "compiler/crystal/syntax/token"
+
+module Ameba::Rule::Style
+  # A rule that disallows redundant uses of `self`.
+  #
+  # This is considered bad:
+  #
+  # ```
+  # class Greeter
+  #   getter name : String
+  #
+  #   def self.init
+  #     self.new("Crystal").greet
+  #   end
+  #
+  #   def initialize(@name)
+  #   end
+  #
+  #   def greet
+  #     puts "Hello, my name is #{self.name}"
+  #   end
+  #
+  #   self.init
+  # end
+  # ```
+  #
+  # And needs to be written as:
+  #
+  # ```
+  # class Greeter
+  #   getter name : String
+  #
+  #   def self.init
+  #     new("Crystal").greet
+  #   end
+  #
+  #   def initialize(@name)
+  #   end
+  #
+  #   def greet
+  #     puts "Hello, my name is #{name}"
+  #   end
+  #
+  #   init
+  # end
+  # ```
+  #
+  # YAML configuration example:
+  #
+  # ```
+  # Style/RedundantSelf:
+  #   Enabled: true
+  #   AllowedMethodNames:
+  #     - in?
+  #     - inspect
+  #     - not_nil!
+  # ```
+  class RedundantSelf < Base
+    include AST::Util
+
+    properties do
+      since_version "1.7.0"
+      description "Disallows redundant uses of `self`"
+      allowed_method_names %w[in? inspect not_nil!]
+    end
+
+    MSG = "Redundant `self` detected"
+
+    CRYSTAL_KEYWORDS = Crystal::Keyword.values.map(&.to_s)
+
+    def test(source)
+      AST::ScopeCallsWithSelfReceiverVisitor.new self, source
+    end
+
+    def test(source, node : Crystal::Call, scope : AST::Scope)
+      return unless (obj = node.obj).is_a?(Crystal::Var)
+
+      name = node.name
+
+      return if name.in?(CRYSTAL_KEYWORDS)
+      return if name.in?(allowed_method_names)
+      return if name.ends_with?('=')
+      return if name.chars.none?(&.alphanumeric?)
+
+      vars = Set(String).new
+
+      while scope
+        scope.arguments.each do |arg|
+          vars << arg.name
+        end
+        scope.variables.each do |var|
+          var.assignments.each do |assign|
+            vars << assign.variable.name
+          end
+        end
+        scope = scope.outer_scope
+      end
+
+      return if name.in?(vars)
+
+      if location = obj.location
+        issue_for obj, MSG do |corrector|
+          corrector.remove(location, location.adjust(column_number: {{ "self".size }}))
+        end
+      else
+        issue_for obj, MSG
+      end
+    end
+  end
+end

From a425fd9e3fba4690d4abf38bd91fc41fab68ed57 Mon Sep 17 00:00:00 2001
From: Sijawusz Pur Rahnama <sija@sija.pl>
Date: Fri, 24 Jan 2025 23:53:16 +0100
Subject: [PATCH 2/2] Fix newly found offenses

---
 src/ameba/rule/documentation/documentation_admonition.cr | 2 +-
 src/ameba/rule/lint/unneeded_disable_directive.cr        | 2 +-
 src/contrib/read_type_doc.cr                             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/ameba/rule/documentation/documentation_admonition.cr b/src/ameba/rule/documentation/documentation_admonition.cr
index a9fd7e8e9..1cd4d570c 100644
--- a/src/ameba/rule/documentation/documentation_admonition.cr
+++ b/src/ameba/rule/documentation/documentation_admonition.cr
@@ -46,7 +46,7 @@ module Ameba::Rule::Documentation
 
     @[YAML::Field(ignore: true)]
     private getter location : Time::Location do
-      Time::Location.load(self.timezone)
+      Time::Location.load(timezone)
     end
 
     def test(source)
diff --git a/src/ameba/rule/lint/unneeded_disable_directive.cr b/src/ameba/rule/lint/unneeded_disable_directive.cr
index 5c447cbf1..98572b92f 100644
--- a/src/ameba/rule/lint/unneeded_disable_directive.cr
+++ b/src/ameba/rule/lint/unneeded_disable_directive.cr
@@ -47,7 +47,7 @@ module Ameba::Rule::Lint
       return unless directive[:action] == "disable"
 
       directive[:rules].reject do |rule_name|
-        next if rule_name == self.name
+        next if rule_name == name
         source.issues.any? do |issue|
           issue.rule.name == rule_name &&
             issue.disabled? &&
diff --git a/src/contrib/read_type_doc.cr b/src/contrib/read_type_doc.cr
index 53ec0e2df..035a67344 100644
--- a/src/contrib/read_type_doc.cr
+++ b/src/contrib/read_type_doc.cr
@@ -5,7 +5,7 @@ private class DocFinder < Crystal::Visitor
   getter doc : String?
 
   def initialize(nodes, @type_name)
-    self.accept(nodes)
+    accept(nodes)
   end
 
   def visit(node : Crystal::ASTNode)