Basic unified parse tree working

lib/reek/block_context.rb

@@ -1,13 +1,33 @@
 require 'reek/code_context'
 
 module Reek
+
+  module ParameterSet
+    def names
+      return @names if @names
+      return (@names = []) if empty?
+      arg = slice(1)
+      case slice(0)
+      when :masgn
+        @names = arg[1..-1].map {|lasgn| Name.new(lasgn[1]) }
+      when :lasgn
+        @names = [Name.new(arg)]
+      end
+    end
+
+    def include?(name)
+      names.include?(name)
+    end
+  end
+
   class BlockContext < CodeContext
 
     def initialize(outer, exp)
       super
-      @parameters = []
-      @local_variables = []
       @name = Name.new('block')
+      @parameters = exp[0] if exp
+      @parameters ||= []
+      @parameters.extend(ParameterSet)
     end
 
     def inside_a_block?
@@ -22,16 +42,15 @@ def nested_block?
       @outer.inside_a_block?
     end
 
-    def record_parameter(sym)
-      @parameters << Name.new(sym)
+    def record_local_variable(sym)
     end
 
     def outer_name
       "#{@outer.outer_name}#{@name}/"
     end
 
     def variable_names
-      @parameters + @local_variables
+      @parameters.names
     end
   end
 end

lib/reek/class_context.rb

@@ -34,20 +34,20 @@ def myself
     def find_module(modname)
       sym = modname.to_s
       return nil unless myself
-      myself.const_defined?(sym) ? myself.const_get(sym) : nil
+      @myself.const_defined?(sym) ? @myself.const_get(sym) : nil
     end
 
     def is_overriding_method?(name)
       return false unless myself
-      myself.is_overriding_method?(name.to_s)
+      @myself.is_overriding_method?(name.to_s)
     end
 
     def is_struct?
       @superclass == [:const, :Struct]
     end
 
     def num_methods
-      meths = myself ? myself.non_inherited_methods : @parsed_methods
+      meths = myself ? @myself.non_inherited_methods : @parsed_methods
       meths.length
     end
 

lib/reek/code_parser.rb

@@ -14,8 +14,16 @@ module Reek
 
   class CodeParser < SexpProcessor
 
+    def self.unify(sexp)   # :nodoc:
+      unifier = Unifier.new
+      unifier.processors.each do |proc|
+        proc.unsupported.delete :cfunc # HACK
+      end
+      return unifier.process(sexp[0])
+    end
+
     def self.parse_tree_for(code)   # :nodoc:
-      ParseTree.new.parse_tree_for_string(code)
+      unify(ParseTree.new.parse_tree_for_string(code))
     end
 
     # Creates a new Ruby code checker. Any smells discovered by
@@ -41,7 +49,8 @@ def check_source(code)
     # Any smells found are saved in the +Report+ object that
     # was passed to this object's constructor.
     def check_object(obj)
-      check_parse_tree(ParseTree.new.parse_tree(obj))
+      sexp = CodeParser.unify(ParseTree.new.parse_tree(obj))
+      check_parse_tree(sexp)
     end
 
     def process_default(exp)
@@ -114,8 +123,7 @@ def process_call(exp)
     end
 
     def process_fcall(exp)
-      @element.record_depends_on_self
-      @element.refs.record_reference_to_self
+      @element.record_use_of_self
       process_default(exp)
     end
 
@@ -125,8 +133,7 @@ def process_cfunc(exp)
     end
 
     def process_vcall(exp)
-      @element.record_depends_on_self
-      @element.refs.record_reference_to_self
+      @element.record_use_of_self
       s(exp)
     end
 
@@ -206,7 +213,7 @@ def pop(exp)
     end
 
     def check_parse_tree(sexp)  # :nodoc:
-      sexp.each { |exp| process(exp) }
+      process(sexp)
     end
   end
 end

lib/reek/method_context.rb

@@ -36,22 +36,22 @@ def has_parameter(sym)
     def record_call_to(exp)
       @calls[exp] += 1
       receiver, meth = exp[1..2]
-      if receiver.nil?
-        record_depends_on_self
-      else
-        case receiver[0]
-        when :lvar
-          @refs.record_ref(receiver) unless meth == :new
-        when :self
-          record_depends_on_self
-          @refs.record_reference_to_self
-        end
+      receiver ||= [:self]
+      case receiver[0]
+      when :lvar
+        @refs.record_ref(receiver) unless meth == :new
+      when :self
+        record_use_of_self
       end
     end
 
-    def record_instance_variable(sym)
+    def record_use_of_self
       record_depends_on_self
       @refs.record_reference_to_self
+    end
+
+    def record_instance_variable(sym)
+      record_use_of_self
       @outer.record_instance_variable(sym)
     end
 
@@ -64,7 +64,7 @@ def record_local_variable(sym)
     end
 
     def self.is_block_arg?(param)
-      Array === param and param[0] == :block
+      (Array === param and param[0] == :block) or (param.to_s =~ /^\&/)
     end
 
     def record_parameter(param)

lib/reek/module_context.rb

@@ -20,7 +20,7 @@ def myself
     def find_module(modname)
       return nil unless myself
       sym = modname.to_s
-      myself.const_defined?(sym) ? myself.const_get(sym) : nil
+      @myself.const_defined?(sym) ? @myself.const_get(sym) : nil
     end
 
     def outer_name

lib/reek/report.rb

@@ -35,6 +35,12 @@ def [](index)  # :nodoc:
       @report.to_a[index]
     end
 
+    # Creates a formatted report of all the +Smells::SmellWarning+ objects recorded in
+    # this report, with a heading.
+    def full_report(desc)
+      "\"#{desc}\" -- #{length} warnings:\n#{to_s}\n"
+    end
+
     # Creates a formatted report of all the +Smells::SmellWarning+ objects recorded in
     # this report.
     def to_s
@@ -64,11 +70,12 @@ def length
       @sources.inject(0) {|sum, src| sum + src.report.length }
     end
 
+    def smelly_sources
+      @sources.select {|src| src.smelly? }
+    end
+
     def to_s
-      @sources.select {|src| src.smelly? }.map do |src|
-        warnings = src.report
-        "\"#{src}\" -- #{warnings.length} warnings:\n#{warnings.to_s}\n"
-      end.join("\n")
+      smelly_sources.map { |src| src.full_report }.join("\n")
     end
   end
 end

lib/reek/sexp_formatter.rb

@@ -1,58 +1,10 @@
+require 'ruby2ruby'
+
 module Reek
   class SexpFormatter
     def self.format(sexp)
-      return sexp.to_s unless Array === sexp
-      first = sexp[1]
-      second = sexp[2]
-      third = sexp[3]
-      case sexp[0]
-      when :array
-        format_all(sexp, ', ')
-      when :attrasgn
-        result = format(first)
-        if second == :[]=
-          result += "[#{format(third[1])}] = #{format(third[2])}"
-        end
-        result
-      when :call
-        result = format(first)
-        if second.to_s == '[]'
-          result += (third.nil? ? '[]' : "[#{format(third)}]")
-        else
-          result += ".#{second}" + (third ? "(#{format(third)})" : '')
-        end
-        result
-      when :colon2
-        format_all(sexp, '::')
-      when :const, :cvar, :dvar
-        format(first)
-      when :dot2
-        format_all(sexp, '..')
-      when :dstr
-        '"' + format_all(sexp, '') + '"'
-      when :evstr
-        "\#\{#{format(first)}\}"
-      when :fcall, :vcall
-        result = first.to_s
-        result += "(#{format(second)})" if second
-        result
-      when :iter
-        'block'
-      when :lasgn
-        format_all(sexp, '=')
-      when :nth_ref
-        "$#{first}"
-      when :str
-        first
-      when :xstr
-        "`#{first}`"
-      else
-        sexp[-1].to_s
-      end
-    end
-
-    def self.format_all(sexp, glue)
-      sexp[1..-1].map {|arg| format(arg)}.join(glue)
+      sexp = YAML::load(YAML::dump(sexp))
+      Ruby2Ruby.new.process(sexp)
     end
   end
 end

lib/reek/smells/long_parameter_list.rb

@@ -27,7 +27,7 @@ def initialize(config)
         @max_params = config['max_params']
         @action = 'has'
       end
-      
+
       #
       # Checks the number of parameters in the given scope.
       # Any smells found are added to the +report+.

lib/reek/source.rb

@@ -101,6 +101,12 @@ def has_smell?(smell_class, patterns)
       report.any? { |smell| smell.matches?(smell_class, patterns) }
     end
 
+    # Creates a formatted report of all the +Smells::SmellWarning+ objects recorded in
+    # this report, with a heading.
+    def full_report
+      report.full_report(@desc)
+    end
+
     def to_s
       @desc
     end

lib/reek/yield_call_context.rb

@@ -6,9 +6,7 @@ class YieldCallContext < CodeContext
 
     def initialize(outer, exp)
       super
-      @parameters = []
-      args = exp[1]
-      @parameters = args[0...-1] if args
+      @parameters = exp[1..-1]
     end
   end
 end

spec/reek/block_context_spec.rb

@@ -0,0 +1,34 @@
+require File.dirname(__FILE__) + '/../spec_helper.rb'
+
+require 'reek/block_context'
+require 'reek/method_context'
+
+include Reek
+
+describe BlockContext do
+
+  it "should record single parameter" do
+    element = StopContext.new
+    element = BlockContext.new(element, s(s(:lasgn, :x), nil))
+    element.variable_names.should == [Name.new(:x)]
+  end
+
+  it "should record single parameter within a method" do
+    element = StopContext.new
+    element = MethodContext.new(element, s(:defn, :help))
+    element = BlockContext.new(element, s(s(:lasgn, :x), nil))
+    element.variable_names.should == [Name.new(:x)]
+  end
+
+  it "records multiple parameters" do
+    element = StopContext.new
+    element = BlockContext.new(element, s(s(:masgn, s(:array, s(:lasgn, :x), s(:lasgn, :y))), nil))
+    element.variable_names.should == [Name.new(:x), Name.new(:y)]
+  end
+
+  it "should not pass parameters upward" do
+    mc = MethodContext.new(StopContext.new, s(:defn, :help))
+    element = BlockContext.new(mc, s(s(:lasgn, :x)))
+    mc.variable_names.should be_empty
+  end
+end

spec/reek/method_context_spec.rb

@@ -56,4 +56,11 @@
     mctx.record_call_to([:call, [:self], :thing])
     mctx.envious_receivers.should be_empty
   end
+
+  it 'should recognise a call on self' do
+    mc = MethodContext.new(StopContext.new, s(:defn, :deep))
+    mc.record_call_to(s(:call, s(:lvar, :text), :each, s(:arglist)))
+    mc.record_call_to(s(:call, nil, :shelve, s(:arglist)))
+    mc.envious_receivers.should be_empty
+  end
 end

spec/reek/sexp_formatter_spec.rb

@@ -7,7 +7,7 @@
 
 def should_print(example)
   it "should format #{example} correctly" do
-    sexp = CodeParser.parse_tree_for(example)[0]
+    sexp = CodeParser.parse_tree_for(example)
     SexpFormatter.format(sexp).should == example
   end
 end
@@ -23,10 +23,10 @@ def should_print(example)
   should_print 'obj.method(arg1, arg2)'
   should_print 'obj.method'
   should_print '$1'
-  should_print 'o=q.downcase'
+  should_print 'o = q.downcase'
   should_print 'true'
   should_print '"-#{q}xxx#{z.size}"'
-  should_print '0..5'
-  should_print '0..temp'
+  should_print '(0..5)'
+  should_print '(0..temp)'
   should_print 'result[opt] = false'
 end

spec/reek/singleton_method_context_spec.rb

@@ -11,7 +11,7 @@
   it "should report full context" do
     element = StopContext.new
     element = ModuleContext.new(element, [0, :mod])
-    element = SingletonMethodContext.new(element,  [:defs, [:vcall, :a], :b, nil])
+    element = SingletonMethodContext.new(element,  [:defs, [:call, nil, :a, [:arglist]], :b, nil])
     element.outer_name.should match(/mod::a\.b/)
   end
 end