instance_variables changes return type between 1.8 and 1.9

The return type of `instance_variables` changes between Ruby 1.8 and 1.9
releases; it used to return an array of strings in the form "@foo", but
now returns an array of symbols in the form :@foo.

Nothing else in the stack cares which form you get - you can pass the
string or symbol to things like `instance_variable_set` and they will work
transparently.

Having the same form in all releases of Puppet is a win, though, so we
pick a unification and enforce than on all releases.  That way developers
who do set math on them (eg: for YAML rendering) don't have to handle the
distinction themselves.

In the sane tradition, we bring older releases into conformance with newer
releases, so we return symbols rather than strings, to be more like the
future versions of Ruby are.

We also carefully support reloading, by only wrapping when we don't
already have the original version of the method aliased away somewhere.

This includes a second monkey-patch, on the `resolv` library that ships with
Ruby 1.8.  This is the only bit of code in the core stack that made
assumptions about the return format of names from `instance_methods`.

Specifically, it assumed they were strings, and could be evaluated using
`instance_eval` to fetch out the value.  The fix is to replace that with a
call to `instance_variable_get` which, like it does in Ruby 1.9, avoids the
cost of eval parsing as well as being more robust overall.

I did audit the rest of the standard library on 1.8.5 and 1.8.7 to verify
this.  We only need worry if we start using the SOAP or Tk support.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>

Author: Daniel Pittman

lib/puppet/parser/yaml_trimmer.rb

@@ -1,5 +1,5 @@
 module Puppet::Parser::YamlTrimmer
-  REMOVE = %w{@scope @source}
+  REMOVE = [:@scope, :@source]
 
   def to_yaml_properties
     r = instance_variables - REMOVE

lib/puppet/resource/status.rb

@@ -12,7 +12,10 @@ class Status
       attr_reader :source_description, :default_log_level, :time, :resource
       attr_reader :change_count, :out_of_sync_count, :resource_type, :title
 
-      YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title @skipped @failed}
+      YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count
+                           @out_of_sync_count @tags @time @events @out_of_sync
+                           @changed @resource_type @title @skipped @failed}.
+        map(&:to_sym)
 
       # Provide a boolean method for each of the states.
       STATES.each do |attr|

lib/puppet/simple_graph.rb

@@ -526,11 +526,8 @@ def instance_variable_get(v)
   end
 
   def to_yaml_properties
-    other_vars = instance_variables.
-      map {|v| v.to_s}.
-      reject { |v| %w{@in_to @out_from @upstream_from @downstream_from}.include?(v) }
-
-    (other_vars + %w{@vertices @edges}).sort.uniq
+    (instance_variables + [:@vertices, :@edges] -
+     [:@in_to, :@out_from, :@upstream_from, :@downstream_from]).sort.uniq
   end
 
   def yaml_initialize(tag, var)

lib/puppet/transaction/event.rb

@@ -8,7 +8,7 @@ class Puppet::Transaction::Event
   include Puppet::Util::Logging
 
   ATTRIBUTES = [:name, :resource, :property, :previous_value, :desired_value, :historical_value, :status, :message, :file, :line, :source_description, :audited]
-  YAML_ATTRIBUTES = %w{@audited @property @previous_value @desired_value @historical_value @message @name @status @time}
+  YAML_ATTRIBUTES = %w{@audited @property @previous_value @desired_value @historical_value @message @name @status @time}.map(&:to_sym)
   attr_accessor *ATTRIBUTES
   attr_writer :tags
   attr_accessor :time
@@ -55,7 +55,7 @@ def to_s
   end
 
   def to_yaml_properties
-    (YAML_ATTRIBUTES.map {|ya| ya.to_s} & instance_variables.map{|iv| iv.to_s}).sort
+    (YAML_ATTRIBUTES & instance_variables).sort
   end
 
   private

lib/puppet/transaction/report.rb

@@ -141,7 +141,7 @@ def exit_status
   end
 
   def to_yaml_properties
-    (instance_variables - ["@external_times"]).sort
+    (instance_variables - [:@external_times]).sort
   end
 
   private

lib/puppet/util/log.rb

@@ -233,6 +233,7 @@ def message=(msg)
 
   def level=(level)
     raise ArgumentError, "Puppet::Util::Log requires a log level" unless level
+    raise ArgumentError, "Puppet::Util::Log requires a symbol or string" unless level.respond_to? "to_sym"
     @level = level.to_sym
     raise ArgumentError, "Invalid log level #{@level}" unless self.class.validlevel?(@level)
 

lib/puppet/util/monkey_patches.rb

@@ -153,3 +153,56 @@ def tap
     self
   end unless method_defined?(:tap)
 end
+
+
+########################################################################
+# The return type of `instance_variables` changes between Ruby 1.8 and 1.9
+# releases; it used to return an array of strings in the form "@foo", but
+# now returns an array of symbols in the form :@foo.
+#
+# Nothing else in the stack cares which form you get - you can pass the
+# string or symbol to things like `instance_variable_set` and they will work
+# transparently.
+#
+# Having the same form in all releases of Puppet is a win, though, so we
+# pick a unification and enforce than on all releases.  That way developers
+# who do set math on them (eg: for YAML rendering) don't have to handle the
+# distinction themselves.
+#
+# In the sane tradition, we bring older releases into conformance with newer
+# releases, so we return symbols rather than strings, to be more like the
+# future versions of Ruby are.
+#
+# We also carefully support reloading, by only wrapping when we don't
+# already have the original version of the method aliased away somewhere.
+if RUBY_VERSION[0,3] == '1.8'
+  unless Object.respond_to?(:puppet_original_instance_variables)
+
+    # Add our wrapper to the method.
+    class Object
+      alias :puppet_original_instance_variables :instance_variables
+
+      def instance_variables
+        puppet_original_instance_variables.map(&:to_sym)
+      end
+    end
+
+    # The one place that Ruby 1.8 assumes something about the return format of
+    # the `instance_variables` method is actually kind of odd, because it uses
+    # eval to get at instance variables of another object.
+    #
+    # This takes the original code and applies replaces instance_eval with
+    # instance_variable_get through it.  All other bugs in the original (such
+    # as equality depending on the instance variables having the same order
+    # without any promise from the runtime) are preserved. --daniel 2012-03-11
+    require 'resolv'
+    class Resolv::DNS::Resource
+      def ==(other) # :nodoc:
+        return self.class == other.class &&
+          self.instance_variables == other.instance_variables &&
+          self.instance_variables.collect {|name| self.instance_variable_get name} ==
+          other.instance_variables.collect {|name| other.instance_variable_get name}
+      end
+    end
+  end
+end

lib/puppet/util/zaml.rb

@@ -148,7 +148,7 @@ def to_zaml(z)
         else
           instance_variables.each { |v|
             z.nl
-            v[1..-1].to_zaml(z)       # Remove leading '@'
+            v.to_s[1..-1].to_zaml(z)       # Remove leading '@'
             z.emit(': ')
             yaml_property_munge(instance_variable_get(v)).to_zaml(z)
           }

spec/unit/application/inspect_spec.rb

@@ -260,7 +260,8 @@ def retrieve
         event.property.should == "content"
         event.status.should == "failure"
         event.audited.should == true
-        event.instance_variables.should_not include("@previous_value")
+        event.instance_variables.should_not include "@previous_value"
+        event.instance_variables.should_not include :@previous_value
       end
 
       it "should continue to the next resource" do

spec/unit/resource/status_spec.rb

@@ -148,7 +148,7 @@
       @status.line = 27
       @status.evaluation_time = 2.7
       @status.tags = %w{one two}
-      @status.to_yaml_properties.should == Puppet::Resource::Status::YAML_ATTRIBUTES.sort
+      @status.to_yaml_properties.should =~ Puppet::Resource::Status::YAML_ATTRIBUTES
     end
   end
 end