From 470c938210bf43997e4ac6f58be1b8cecbe8d403 Mon Sep 17 00:00:00 2001
From: John Mair <jrmair@gmail.com>
Date: Sun, 6 Jan 2013 00:16:44 +0100
Subject: [PATCH] Pry::CodeObject, Pry::WrappedModule: introduce
 safe_to_evaluate? method

Unsure where to put safe_to_evaluate? method, so it's currently duplicated in
both the mentioned modules. If we can simply switch to a defined?(str) != "method" test
then we could put the method on Pry::Method.looks_like_a_method?(str, binding) and call it from both locations, avoiding duplication.

https://www.evernote.com/shard/s130/sh/4e839419-387e-43ab-81d5-06f87aa2cbac/2c640432e3ecccb2035c69c347f63af1
---
 lib/pry/code_object.rb    | 32 +++++++++++++++++++++++++-------
 lib/pry/wrapped_module.rb | 37 ++++++++++++-------------------------
 2 files changed, 37 insertions(+), 32 deletions(-)

diff --git a/lib/pry/code_object.rb b/lib/pry/code_object.rb
index 9d3baf17..b90758a1 100644
--- a/lib/pry/code_object.rb
+++ b/lib/pry/code_object.rb
@@ -67,7 +67,9 @@ class Pry
 
     # lookup variables and constants and `self` that are not modules
     def default_lookup
-      if variable_or_constant_or_self?(str)
+
+      # we skip instance methods as we want those to fall through to method_or_class_lookup()
+      if safe_to_evaluate?(str) && !looks_like_an_instance_method?(str)
         obj = target.eval(str)
 
         # restrict to only objects we KNOW for sure support the full API
@@ -90,7 +92,7 @@ class Pry
       # Pry::Method.from_binding when str is nil.
       # Once we refactor Pry::Method.from_str() so it doesnt lookup
       # from bindings, we can get rid of this check
-      return nil if !str || str.empty?
+      return nil if str.to_s.empty?
 
       obj = if str =~ /::(?:\S+)\Z/
         Pry::WrappedModule.from_str(str,target) || Pry::Method.from_str(str, target)
@@ -107,12 +109,28 @@ class Pry
       [::Proc, ::Method, ::UnboundMethod].any? { |o| obj.is_a?(o) }
     end
 
-    # Whether `str` represents `self` or a variable (or constant) when looked up
-    # in the context of the `target` binding. This is used to
-    # distinguish it from methods or expressions.
+
+    # Returns true if `str` looks like a method, i.e Klass#method
+    # We need to consider this case because method lookups should fall
+    # through to the `method_or_class_lookup()` method but a
+    # defined?() on a "Klass#method` string will see the `#` as a
+    # comment and only evaluate the `Klass` part.
+    # @param [String] str
+    # @return [Boolean] Whether the string looks like an instance method.
+    def looks_like_an_instance_method?(str)
+      str =~ /\S#\S/
+    end
+
+    # We use this method to decide whether code is safe to eval. Method's are
+    # generally not, but everything else is.
+    # TODO: is just checking != "method" enough??
+    # TODO: see duplication of this method in Pry::WrappedModule
     # @param [String] str The string to lookup
-    def variable_or_constant_or_self?(str)
-      str.strip == "self" || str !~ /\S#\S/ && target.eval("defined? #{str} ") =~ /variable|constant/
+    # @return [Boolean]
+    def safe_to_evaluate?(str)
+      return true if str.strip == "self"
+      kind = target.eval("defined?(#{str})")
+      kind =~ /variable|constant/
     end
 
     def target_self
diff --git a/lib/pry/wrapped_module.rb b/lib/pry/wrapped_module.rb
index d695e6f8..4fa3a239 100644
--- a/lib/pry/wrapped_module.rb
+++ b/lib/pry/wrapped_module.rb
@@ -27,7 +27,7 @@ class Pry
     # @example
     #   Pry::WrappedModule.from_str("Pry::Code")
     def self.from_str(mod_name, target=TOPLEVEL_BINDING)
-      if variable_or_constant_or_self_from_binding_is_a_module?(target, mod_name)
+      if safe_to_evaluate?(mod_name, target)
         Pry::WrappedModule.new(target.eval(mod_name))
       else
         nil
@@ -39,30 +39,17 @@ class Pry
     class << self
       private
 
-      # Check whether the variable `mod_name` in binding `target` is a variable
-      # or a constant. If we dont limit to variables/constants then `from_str` could end up
-      # executing methods which is not good, i.e `show-source pry`
-      # @param [Binding] target
-      # @param [String] mod_name The string to lookup in the binding.
-      # @return [Boolean] Whether the string represents a variable or constant.
-      def variable_or_constant_or_self?(target, mod_name)
-        return true if mod_name.strip == "self"
-
-        kind = target.eval("defined?(#{mod_name})")
-        kind == "constant" || kind =~ /variable/
-      end
-
-      # Verify that the looked up string represents 1. a variable or
-      # constant and 2. Is a module.
-      # @param [Binding] target
-      # @param [String] mod_name The string to look up in the binding.
-      # @return [Boolean] Whether the string represents a module.
-      def variable_or_constant_or_self_from_binding_is_a_module?(target, mod_name)
-        if variable_or_constant_or_self?(target, mod_name)
-          target.eval(mod_name).is_a?(Module)
-        else
-          nil
-        end
+      # We use this method to decide whether code is safe to eval. Method's are
+      # generally not, but everything else is.
+      # TODO: is just checking != "method" enough??
+      # TODO: see duplication of this method in Pry::CodeObject
+      # @param [String] str The string to lookup.
+      # @param [Binding] target Where the lookup takes place.
+      # @return [Boolean]
+      def safe_to_evaluate?(str, target)
+        return true if str.strip == "self"
+        kind = target.eval("defined?(#{str})")
+        kind =~ /variable|constant/
       end
     end