Many, many, many performance improvements in the compiler (I hope). I did not change functionality anywhere, but I did some profiling and significantly reduced the runtime of many methods, and especially focused on some key methods that run many times.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@1739 980ebf18-57e1-0310-9a29-db15c13687c0

24 files changed, 255 insertions, 218 deletions

diff --git a/lib/puppet/client/master.rb b/lib/puppet/client/master.rb
index dda47fae1..8d7721ae5 100644
--- a/lib/puppet/client/master.rb
+++ b/lib/puppet/client/master.rb
@@ -200,7 +200,7 @@ class Puppet::Client::MasterClient < Puppet::Client
                 retry
             rescue => detail
                 raise Puppet::Error.new("Cannot remove %s: %s" %
-                    [Puppet[statefile], detail])
+                    [Puppet[:statefile], detail])
             end
         end
     end
diff --git a/lib/puppet/config.rb b/lib/puppet/config.rb
index 337a50bbc..8d7b6b895 100644
--- a/lib/puppet/config.rb
+++ b/lib/puppet/config.rb
@@ -6,6 +6,7 @@ module Puppet
 # The class for handling configuration files.
 class Config
     include Enumerable
+    include Puppet::Util
 
     @@sync = Sync.new
 
@@ -17,14 +18,20 @@ class Config
 
         # Yay, recursion.
         self.reparse() unless param == :filetimeout
-        if @config.include?(param)
-            if @config[param]
-                val = @config[param].value
-                return val
+
+        # Cache the returned values; this method was taking close to
+        # 10% of the compile time.
+        unless @returned[param]
+            if @config.include?(param)
+                if @config[param]
+                    @returned[param] = @config[param].value
+                end
+            else
+                raise ArgumentError, "Undefined configuration parameter '%s'" % param
             end
-        else
-            raise ArgumentError, "Undefined configuration parameter '%s'" % param
         end
+
+        return @returned[param]
     end
 
     # Set a config value.  This doesn't set the defaults, it sets the value itself.
@@ -39,6 +46,9 @@ class Config
                 @order << param
             end
             @config[param].value = value
+            if @returned.include?(param)
+                @returned.delete(param)
+            end
         end
 
         return value
@@ -109,6 +119,7 @@ class Config
                 obj.clear
             end
         }
+        @returned.clear
 
         # Don't clear the 'used' in this case, since it's a config file reparse,
         # and we want to retain this info.
@@ -119,6 +130,7 @@ class Config
 
     # This is mostly just used for testing.
     def clearused
+        @returned.clear
         @used = []
     end
 
@@ -190,6 +202,7 @@ class Config
         @config = {}
 
         @created = []
+        @returned = {}
     end
 
     # Make a directory with the appropriate user, group, and mode
@@ -483,15 +496,6 @@ class Config
         end
     end
 
-    def symbolize(param)
-        case param
-        when String: return param.intern
-        when Symbol: return param
-        else
-            raise ArgumentError, "Invalid param type %s" % param.class
-        end
-    end
-
     # Convert our list of objects into a component that can be applied.
     def to_component
         transport = self.to_transportable
@@ -695,6 +699,7 @@ Generated on #{Time.now}.
             @value = nil
         end
 
+        # Do variable interpolation on the value.
         def convert(value)
             return value unless value
             return value unless value.is_a? String
diff --git a/lib/puppet/error.rb b/lib/puppet/error.rb
index 920bd086f..64b877fda 100644
--- a/lib/puppet/error.rb
+++ b/lib/puppet/error.rb
@@ -4,7 +4,6 @@ module Puppet # :nodoc:
     # errors, but...
     class Error < RuntimeError
         attr_accessor :line, :file
-        attr_writer :backtrace
 
         def backtrace
             if defined? @backtrace
@@ -39,13 +38,13 @@ module Puppet # :nodoc:
 
     # An error class for when I don't know what happened.  Automatically
     # prints a stack trace when in debug mode.
-    class DevError < Error
+    class DevError < Puppet::Error
         # XXX This is probably the wrong way to do this, but...
         def set_backtrace(trace)
             if Puppet[:trace]
                 puts trace
             end
-            super(trace)
+            super
         end
     end
 end
diff --git a/lib/puppet/parser/ast.rb b/lib/puppet/parser/ast.rb
index 6f9c9492c..ccd6d216a 100644
--- a/lib/puppet/parser/ast.rb
+++ b/lib/puppet/parser/ast.rb
@@ -43,6 +43,15 @@ class Puppet::Parser::AST
         return @@midline
     end
 
+    # Does this ast object set something?  If so, it gets evaluated first.
+    def self.settor?
+        if defined? @settor
+            @settor
+        else
+            false
+        end
+    end
+
     # Evaluate the current object.  Basically just iterates across all
     # of the contained children and evaluates them in turn, returning a
     # list of all of the collected values, rejecting nil values
@@ -72,8 +81,23 @@ class Puppet::Parser::AST
     # it can enable you to catch the error where it happens, rather than
     # much higher up the stack.
     def safeevaluate(*args)
-        exceptwrap do
-            self.evaluate(*args)
+        # We duplicate code here, rather than using exceptwrap, because this
+        # is called so many times during parsing.
+        #exceptwrap do
+        #    self.evaluate(*args)
+        #end
+        begin
+            return self.evaluate(*args)
+        rescue Puppet::Error => detail
+            raise adderrorcontext(detail)
+        rescue => detail
+            message = options[:message] || "%s failed with error %s: %s" %
+                    [self.class, detail.class, detail.to_s]
+
+            error = options[:type].new(message)
+            # We can't use self.fail here because it always expects strings,
+            # not exceptions.
+            raise adderrorcontext(error, detail)
         end
     end
 
@@ -96,8 +120,6 @@ class Puppet::Parser::AST
     end
     #---------------------------------------------------------------
     # Now autoload everything.
-    # XXX We can't do this, because it causes multiple loads of some
-    # things.
     @autoloader = Puppet::Autoload.new(self,
         "puppet/parser/ast"
     )
diff --git a/lib/puppet/parser/ast/astarray.rb b/lib/puppet/parser/ast/astarray.rb
index fb0a3f671..a0bd5bf89 100644
--- a/lib/puppet/parser/ast/astarray.rb
+++ b/lib/puppet/parser/ast/astarray.rb
@@ -25,10 +25,6 @@ class Puppet::Parser::AST
                 # This is such a stupid hack.  I've no real idea how to make a
                 # "real" declarative language, so I hack it so it looks like
                 # one, yay.
-                setlist = [
-                    AST::VarDef, AST::ResourceDefaults, AST::Function
-                ]
-
                 settors = []
                 others = []
 
@@ -40,34 +36,30 @@ class Puppet::Parser::AST
                 @children.each { |child|
                     if child.instance_of?(AST::ASTArray)
                         child.each do |ac|
-                            items << ac
+                            if ac.class.settor?
+                                settors << ac
+                            else
+                                others << ac
+                            end
                         end
                     else
-                        items << child
-                    end
-                }
-
-                # Now sort them all according to the type of action
-                items.each { |child|
-                    if setlist.include?(child.class)
-                        settors << child
-                    else
-                        others << child
+                        if child.class.settor?
+                            settors << child
+                        else
+                            others << child
+                        end
                     end
                 }
                 rets = [settors, others].flatten.collect { |child|
                     child.safeevaluate(:scope => scope)
                 }
+                return rets.reject { |o| o.nil? }
             else
                 # If we're not declarative, just do everything in order.
-                rets = @children.collect { |item|
+                return @children.collect { |item|
                     item.safeevaluate(:scope => scope)
-                }
+                }.reject { |o| o.nil? }
             end
-
-            rets = rets.reject { |obj| obj.nil? }
-
-            return rets
         end
 
         def push(*ary)
diff --git a/lib/puppet/parser/ast/component.rb b/lib/puppet/parser/ast/component.rb
index 312b11d99..f67d53b4b 100644
--- a/lib/puppet/parser/ast/component.rb
+++ b/lib/puppet/parser/ast/component.rb
@@ -96,17 +96,29 @@ class Puppet::Parser::AST
             @parentclass = nil
             super
 
-            # Deal with metaparams in the argument list.
+            # Convert the arguments to a hash for ease of later use.
             if @arguments
-                @arguments.each do |arg, defvalue|
-                    next unless Puppet::Type.metaparamclass(arg)
-                    if defvalue
-                        warnonce "%s is a metaparam; this value will inherit to all contained elements" % arg
-                    else
-                        raise Puppet::ParseError,
-                            "%s is a metaparameter; please choose another name" %
-                            name
-                    end
+                unless @arguments.is_a? Array
+                    @arguments = [@arguments]
+                end
+                oldargs = @arguments
+                @arguments = {}
+                oldargs.each do |arg, val|
+                    @arguments[arg] = val
+                end
+            else
+                @arguments = {}
+            end
+
+            # Deal with metaparams in the argument list.
+            @arguments.each do |arg, defvalue|
+                next unless Puppet::Type.metaparamclass(arg)
+                if defvalue
+                    warnonce "%s is a metaparam; this value will inherit to all contained elements" % arg
+                else
+                    raise Puppet::ParseError,
+                        "%s is a metaparameter; please choose another name" %
+                        name
                 end
             end
         end
@@ -120,7 +132,7 @@ class Puppet::Parser::AST
         # Set our parent class, with a little check to avoid some potential
         # weirdness.
         def parentclass=(name)
-            if name == @type
+            if name == self.type
                 parsefail "Parent classes must have dissimilar names"
             end
 
@@ -171,35 +183,23 @@ class Puppet::Parser::AST
         # Check whether a given argument is valid.  Searches up through
         # any parent classes that might exist.
         def validattr?(param)
-            return true if Puppet::Type.metaparam?(param)
-
             param = param.to_s
-            found = false
-            unless @arguments.is_a? Array
-                @arguments = [@arguments]
-            end
 
-            found = @arguments.detect { |arg|
-                if arg.is_a? Array
-                    arg[0] == param
-                else
-                    arg == param
-                end
-            }
-
-            if found
+            if @arguments.include?(param)
                 # It's a valid arg for us
                 return true
-            elsif defined? @parentclass and @parentclass
-                # Else, check any existing parent
-                if parent = @scope.lookuptype(@parentclass) and parent != []
-                    return parent.validarg?(param)
-                elsif builtin = Puppet::Type.type(@parentclass)
-                    return builtin.validattr?(param)
-                else
-                    raise Puppet::Error, "Could not find parent class %s" %
-                        @parentclass
-                end
+#            elsif defined? @parentclass and @parentclass
+#                # Else, check any existing parent
+#                if parent = @scope.lookuptype(@parentclass) and parent != []
+#                    return parent.validarg?(param)
+#                elsif builtin = Puppet::Type.type(@parentclass)
+#                    return builtin.validattr?(param)
+#                else
+#                    raise Puppet::Error, "Could not find parent class %s" %
+#                        @parentclass
+#                end
+            elsif Puppet::Type.metaparam?(param)
+                return true
             else
                 # Or just return false
                 return false
diff --git a/lib/puppet/parser/ast/function.rb b/lib/puppet/parser/ast/function.rb
index f67531ae3..6729431b7 100644
--- a/lib/puppet/parser/ast/function.rb
+++ b/lib/puppet/parser/ast/function.rb
@@ -3,6 +3,8 @@ class Puppet::Parser::AST
     class Function < AST::Branch
         attr_accessor :name, :arguments
 
+        @settor = true
+
         def evaluate(hash)
             # We don't need to evaluate the name, because it's plaintext
 
diff --git a/lib/puppet/parser/ast/resourceparam.rb b/lib/puppet/parser/ast/resourceparam.rb
index 248e91b32..317820a3c 100644
--- a/lib/puppet/parser/ast/resourceparam.rb
+++ b/lib/puppet/parser/ast/resourceparam.rb
@@ -12,17 +12,12 @@ class Puppet::Parser::AST
         # Return the parameter and the value.
         def evaluate(hash)
             scope = hash[:scope]
-            param = @param
-            value = @value.safeevaluate(:scope => scope)
 
-            args = {:name => param, :value => value, :source => scope.source}
-            [:line, :file].each do |p|
-                if v = self.send(p)
-                    args[p] = v
-                end
-            end
-
-            return Puppet::Parser::Resource::Param.new(args)
+            return Puppet::Parser::Resource::Param.new(
+                :name => @param,
+                :value => @value.safeevaluate(:scope => scope),
+                :source => scope.source, :line => self.line, :file => self.file
+            )
         end
 
         def tree(indent = 0)
diff --git a/lib/puppet/parser/ast/vardef.rb b/lib/puppet/parser/ast/vardef.rb
index 30eef204a..145700e27 100644
--- a/lib/puppet/parser/ast/vardef.rb
+++ b/lib/puppet/parser/ast/vardef.rb
@@ -3,6 +3,8 @@ class Puppet::Parser::AST
     class VarDef < AST::Branch
         attr_accessor :name, :value
 
+        @settor = true
+
         # Look up our name and value, and store them appropriately.  The
         # lexer strips off the syntax stuff like '$'.
         def evaluate(hash)
diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/resource.rb
index 5d09eaa9b..af5d10501 100644
--- a/lib/puppet/parser/resource.rb
+++ b/lib/puppet/parser/resource.rb
@@ -85,25 +85,42 @@ class Puppet::Parser::Resource
 
     # Retrieve the associated definition and evaluate it.
     def evaluate
-        if builtin?
+        if klass = @ref.definedtype
+            finish()
+            scope.deleteresource(self)
+            return klass.evaluate(:scope => scope,
+                                  :type => self.type,
+                                  :name => self.title,
+                                  :arguments => self.to_hash,
+                                  :scope => self.scope,
+                                  :exported => self.exported
+            )
+        elsif builtin?
             devfail "Cannot evaluate a builtin type"
-        end
-
-        unless klass = scope.finddefine(self.type)
+        else
             self.fail "Cannot find definition %s" % self.type
         end
 
-        finish()
-
-        scope.deleteresource(self)
 
-        return klass.evaluate(:scope => scope,
-                              :type => self.type,
-                              :name => self.title,
-                              :arguments => self.to_hash,
-                              :scope => self.scope,
-                              :exported => self.exported
-        )
+#        if builtin?
+#            devfail "Cannot evaluate a builtin type"
+#        end
+#
+#        unless klass = scope.finddefine(self.type)
+#            self.fail "Cannot find definition %s" % self.type
+#        end
+#
+#        finish()
+#
+#        scope.deleteresource(self)
+#
+#        return klass.evaluate(:scope => scope,
+#                              :type => self.type,
+#                              :name => self.title,
+#                              :arguments => self.to_hash,
+#                              :scope => self.scope,
+#                              :exported => self.exported
+#        )
     ensure
         @evaluated = true
     end
@@ -138,8 +155,7 @@ class Puppet::Parser::Resource
 
         # Collect the options necessary to make the reference.
         refopts = [:type, :title].inject({}) do |hash, param|
-            hash[param] = options[param] ||
-                devfail("%s must be passed to Resources" % param)
+            hash[param] = options[param]
             options.delete(param)
             hash
         end
@@ -175,22 +191,25 @@ class Puppet::Parser::Resource
         end
     end
 
+    # This *significantly* reduces the number of calls to Puppet.[].
+    def paramcheck?
+        unless defined? @@paramcheck
+            @@paramcheck = Puppet[:paramcheck]
+        end
+        @@paramcheck
+    end
+
     # Verify that all passed parameters are valid.  This throws an error if there's
     # a problem, so we don't have to worry about the return value.
     def paramcheck(param)
-        # This defaults to true
-        unless Puppet[:paramcheck]
-            return
-        end
-
-        return if param == "name" or param == "title" # always allow these
-
-        # FIXME We might need to do more here eventually.  Metaparams
-        # behave strangely on containers.
-        return if Puppet::Type.metaparam?(param)
-
-        # Now make sure it's a valid argument to our class.
-        unless @ref.typeclass.validattr?(param)
+        # Now make sure it's a valid argument to our class.  These checks
+        # are organized in order of commonhood -- most types, it's a valid argument
+        # and paramcheck is enabled.
+        if @ref.typeclass.validattr?(param)
+            true
+        elsif (param == "name" or param == "title") # always allow these
+            true
+        elsif paramcheck?
             self.fail Puppet::ParseError, "Invalid parameter '%s' for type '%s'" %
                     [param.inspect, @ref.type]
         end
diff --git a/lib/puppet/parser/resource/param.rb b/lib/puppet/parser/resource/param.rb
index 259e935d1..600e44781 100644
--- a/lib/puppet/parser/resource/param.rb
+++ b/lib/puppet/parser/resource/param.rb
@@ -1,13 +1,14 @@
 # The parameters we stick in Resources.
 class Puppet::Parser::Resource::Param
     attr_accessor :name, :value, :source, :line, :file
+    include Puppet::Util
     include Puppet::Util::Errors
     include Puppet::Util::MethodHelper
 
     def initialize(hash)
         set_options(hash)
         requiredopts(:name, :value, :source)
-        @name = @name.intern if @name.is_a?(String)
+        @name = symbolize(@name)
     end
 
     def inspect
diff --git a/lib/puppet/parser/resource/reference.rb b/lib/puppet/parser/resource/reference.rb
index 2210b71c2..f71b3719f 100644
--- a/lib/puppet/parser/resource/reference.rb
+++ b/lib/puppet/parser/resource/reference.rb
@@ -49,7 +49,10 @@ class Puppet::Parser::Resource::Reference
     end
 
     def to_s
-        "%s[%s]" % [type, title]
+        unless defined? @namestring
+            @namestring = "%s[%s]" % [type, title]
+        end
+        @namestring
     end
 
     def typeclass
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 9b59ebd64..02fd72b0d 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -24,7 +24,7 @@ class Puppet::Parser::Scope
     include Puppet::Util::Errors
     attr_accessor :parent, :level, :interp, :source, :host
     attr_accessor :name, :type, :topscope, :base, :keyword, :namespace
-    attr_accessor :top, :context, :translated, :exported
+    attr_accessor :top, :translated, :exported
 
     # Whether we behave declaratively.  Note that it's a class variable,
     # so all scopes behave the same.
@@ -83,13 +83,11 @@ class Puppet::Parser::Scope
 
     # Verify that the given object isn't defined elsewhere.
     def chkobjectclosure(obj)
-        if @definedtable.include?(obj.ref)
+        if exobj = @definedtable[obj.ref]
             typeklass = Puppet::Type.type(obj.type)
             if typeklass and ! typeklass.isomorphic?
                 Puppet.info "Allowing duplicate %s" % type
             else
-                exobj = @definedtable[obj.ref]
-
                 # Either it's a defined type, which are never
                 # isomorphic, or it's a non-isomorphic type.
                 msg = "Duplicate definition: %s is already defined" % obj.ref
@@ -238,8 +236,7 @@ class Puppet::Parser::Scope
             @interp = @parent.interp
             @source = hash[:source] || @parent.source
             @topscope = @parent.topscope
-            @context = @parent.context
-            @inside = @parent.inside
+            #@inside = @parent.inside # Used for definition inheritance
             @host = @parent.host
             @type ||= @parent.type
         end
@@ -247,20 +244,14 @@ class Puppet::Parser::Scope
         # Our child scopes and objects
         @children = []
 
-        # The symbol table for this scope
-        @symtable = Hash.new(nil)
+        # The symbol table for this scope.  This is where we store variables.
+        @symtable = {}
 
         # All of the defaults set for types.  It's a hash of hashes,
         # with the first key being the type, then the second key being
         # the parameter.
         @defaultstable = Hash.new { |dhash,type|
-            dhash[type] = Hash.new(nil)
-        }
-
-        # Map the names to the tables.
-        @map = {
-            "variable" => @symtable,
-            "defaults" => @defaultstable
+            dhash[type] = {}
         }
 
         unless @interp
@@ -288,7 +279,7 @@ class Puppet::Parser::Scope
 
         # The table for storing class singletons.  This will only actually
         # be used by top scopes and node scopes.
-        @classtable = Hash.new(nil)
+        @classtable = {}
 
         self.class.declarative = declarative
 
@@ -315,7 +306,6 @@ class Puppet::Parser::Scope
         # but they each refer back to the scope that created them.
         @collecttable = []
 
-        @context = nil
         @topscope = self
         @type = "puppet"
         @name = "top"
@@ -369,15 +359,15 @@ class Puppet::Parser::Scope
     # Look up a variable.  The simplest value search we do.  Default to returning
     # an empty string for missing values, but support returning a constant.
     def lookupvar(name, usestring = true)
-        value = lookup("variable", name)
-        if value == :undefined
-            if usestring
-                return ""
-            else
-                return :undefined
-            end
+        # We can't use "if @symtable[name]" here because the value might be false
+        if @symtable.include?(name)
+            return @symtable[name]
+        elsif self.parent 
+            return @parent.lookupvar(name, usestring)
+        elsif usestring
+            return ""
         else
-            return value
+            return :undefined
         end
     end
 
@@ -395,7 +385,8 @@ class Puppet::Parser::Scope
 
     # Return the list of remaining overrides.
     def overrides
-        @overridetable.collect { |name, overs| overs }.flatten
+        #@overridetable.collect { |name, overs| overs }.flatten
+        @overridetable.values.flatten
     end
 
     def resources
@@ -492,29 +483,36 @@ class Puppet::Parser::Scope
     def setvar(name,value)
         #Puppet.debug "Setting %s to '%s' at level %s" %
         #    [name.inspect,value,self.level]
-        if @@declarative and @symtable.include?(name)
-            raise Puppet::ParseError, "Cannot reassign variable %s" % name
-        else
-            if @symtable.include?(name)
+        if @symtable.include?(name)
+            if @@declarative
+                raise Puppet::ParseError, "Cannot reassign variable %s" % name
+            else
                 Puppet.warning "Reassigning %s to %s" % [name,value]
             end
-            @symtable[name] = value
         end
+        @symtable[name] = value
     end
 
     # Return an interpolated string.
     def strinterp(string)
-        newstring = string.gsub(/\\\$|\$\{(\w+)\}|\$(\w+)/) do |value|
-            # If it matches the backslash, then just retun the dollar sign.
-            if value == '\\$'
-                '$'
-            else # look the variable up
-                var = $1 || $2
-                lookupvar($1 || $2)
+        # Most strings won't have variables in them.
+        if string =~ /\$/
+            string = string.gsub(/\\\$|\$\{(\w+)\}|\$(\w+)/) do |value|
+                # If it matches the backslash, then just retun the dollar sign.
+                if value == '\\$'
+                    '$'
+                else # look the variable up
+                    lookupvar($1 || $2)
+                end
             end
         end
 
-        return newstring.gsub(/\\t/, "\t").gsub(/\\n/, "\n").gsub(/\\s/, "\s")
+        # And most won't have whitespace replacements.
+        if string =~ /\\/
+            return string.gsub(/\\t/, "\t").gsub(/\\n/, "\n").gsub(/\\s/, "\s")
+        else
+            return string
+        end
     end
 
     # Add a tag to our current list.  These tags will be added to all
@@ -573,20 +571,19 @@ class Puppet::Parser::Scope
     def translate
         ret = @children.collect do |child|
             case child
-            when self.class
-                child.translate
             when Puppet::Parser::Resource
                 child.to_trans
+            when self.class
+                child.translate
             else
                 devfail "Got %s for translation" % child.class
             end
         end.reject { |o| o.nil? }
         bucket = Puppet::TransBucket.new ret
-        unless self.type
-            devfail "A Scope with no type"
-        end
-        if @type == ""
-            bucket.type = "main"
+
+        case self.type
+        when "": bucket.type = "main"
+        when nil: devfail "A Scope with no type"
         else
             bucket.type = @type
         end
@@ -615,34 +612,6 @@ class Puppet::Parser::Scope
             return ary
         end
     end
-
-    protected
-
-    # This method abstracts recursive searching.  It accepts the type
-    # of search being done and then either a literal key to search for or
-    # a Proc instance to do the searching.
-    def lookup(type,sub, usecontext = false)
-        table = @map[type]
-        if table.nil?
-            self.fail "Could not retrieve %s table at level %s" %
-                    [type,self.level]
-        end
-
-        if sub.is_a?(Proc) and obj = sub.call(table)
-            return obj
-        elsif table.include?(sub)
-            return table[sub]
-        elsif ! @parent.nil?
-            # Context is used for retricting overrides.
-            if usecontext and self.context != @parent.context
-                return :undefined
-            else
-                return @parent.lookup(type,sub, usecontext)
-            end
-        else
-            return :undefined
-        end
-    end
 end
 
 # $Id$
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 55421eb4c..9d3465e44 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -135,12 +135,13 @@ class Type < Puppet::Element
 
     # Do an on-demand plugin load
     def self.loadplugin(name)
-        unless Puppet[:pluginpath].split(":").include?(Puppet[:plugindest])
+        paths = Puppet[:pluginpath].split(":")
+        unless paths.include?(Puppet[:plugindest])
             Puppet.notice "Adding plugin destination %s to plugin search path" %
                 Puppet[:plugindest]
             Puppet[:pluginpath] += ":" + Puppet[:plugindest]
         end
-        Puppet[:pluginpath].split(":").each do |dir|
+        paths.each do |dir|
             file = ::File.join(dir, name.to_s + ".rb")
             if FileTest.exists?(file)
                 begin
@@ -213,11 +214,11 @@ class Type < Puppet::Element
     def self.type(name)
         @types ||= {}
 
-        if name.is_a?(String)
-            name = name.intern
-        end
+        name = symbolize(name)
 
-        unless @types.include? name
+        if t = @types[name]
+            return t
+        else
             if typeloader.load(name)
                 unless @types.include? name
                     Puppet.warning "Loaded puppet/type/#{name} but no class was created"
@@ -226,9 +227,9 @@ class Type < Puppet::Element
                 # If we can't load it from there, try loading it as a plugin.
                 loadplugin(name)
             end
-        end
 
-        @types[name]
+            return @types[name]
+        end
     end
 
     def self.typeloader
@@ -857,11 +858,17 @@ class Type < Puppet::Element
 
     def self.validattr?(name)
         name = symbolize(name)
-        if self.validstate?(name) or self.validparameter?(name) or self.metaparam?(name)
-            return true
-        else
-            return false
+        @validattrs ||= {}
+
+        unless @validattrs.include?(name)
+            if self.validstate?(name) or self.validparameter?(name) or self.metaparam?(name)
+                @validattrs[name] = true
+            else
+                @validattrs[name] = false
+            end
         end
+
+        @validattrs[name]
     end
 
     # abstract accessing parameters and states, and normalize
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 755c254d2..74a01de4f 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -414,7 +414,7 @@ module Util
 
     def symbolize(value)
         if value.respond_to? :intern
-            return value.intern
+            value.intern
         else
             value
         end
diff --git a/lib/puppet/util/errors.rb b/lib/puppet/util/errors.rb
index ad129ee34..d077abdee 100644
--- a/lib/puppet/util/errors.rb
+++ b/lib/puppet/util/errors.rb
@@ -22,7 +22,7 @@ module Puppet::Util::Errors
     def exceptwrap(options = {})
         options[:type] ||= Puppet::DevError
         begin
-            retval = yield
+            return yield
         rescue Puppet::Error => detail
             raise adderrorcontext(detail)
         rescue => detail
diff --git a/lib/puppet/util/methodhelper.rb b/lib/puppet/util/methodhelper.rb
index c229e8efb..7de723c3b 100644
--- a/lib/puppet/util/methodhelper.rb
+++ b/lib/puppet/util/methodhelper.rb
@@ -10,14 +10,14 @@ module Puppet::Util::MethodHelper
 
     # Iterate over a hash, treating each member as an attribute.
     def set_options(options)
-        options.dup.each do |param,value|
+        options.each do |param,value|
             method = param.to_s + "="
-            unless self.respond_to?(method)
+            begin
+                self.send(method, value)
+            rescue NoMethodError
                 self.fail "Invalid parameter %s to object class %s" %
                         [param,self.class.to_s]
             end
-
-            self.send(method,value)
         end
     end
 
diff --git a/test/language/functions.rb b/test/language/functions.rb
index 062971bf1..accb027cf 100755
--- a/test/language/functions.rb
+++ b/test/language/functions.rb
@@ -263,6 +263,7 @@ class TestLangFunctions < Test::Unit::TestCase
             end
 
             scope.setvar("yayness", string)
+            assert_equal(string, scope.lookupvar("yayness", false))
 
             assert_nothing_raised("An empty string was not a valid variable value") do
                 ast.evaluate(:scope => scope)
diff --git a/test/language/resource.rb b/test/language/resource.rb
index 12b1e7d2a..d2ddbb7a4 100755
--- a/test/language/resource.rb
+++ b/test/language/resource.rb
@@ -23,7 +23,8 @@ class TestResource < Test::Unit::TestCase
             :source => @source, :scope => @scope}
         # Check our arg requirements
         args.each do |name, value|
-            try = args.dup.delete(name)
+            try = args.dup
+            try.delete(name)
             assert_raise(Puppet::DevError) do
                 Parser::Resource.new(try)
             end
@@ -291,10 +292,14 @@ class TestResource < Test::Unit::TestCase
     end
 
     def test_addmetaparams
+        mkevaltest @interp
         res = Parser::Resource.new :type => "evaltest", :title => "yay",
             :source => @source, :scope => @scope
 
         assert_nil(res[:schedule], "Got schedule already")
+        assert_nothing_raised do
+            res.addmetaparams
+        end
         @scope.setvar("schedule", "daily")
 
         assert_nothing_raised do
diff --git a/test/lib/puppettest.rb b/test/lib/puppettest.rb
index c6ce54b93..e5192ccc7 100644
--- a/test/lib/puppettest.rb
+++ b/test/lib/puppettest.rb
@@ -120,7 +120,6 @@ module PuppetTest
         #end
 
         Puppet[:ignoreschedules] = true
-        Puppet[:trace] = true
     end
 
     def tempfile
diff --git a/test/lib/puppettest/resourcetesting.rb b/test/lib/puppettest/resourcetesting.rb
index a7d183ca7..8cb59b83d 100644
--- a/test/lib/puppettest/resourcetesting.rb
+++ b/test/lib/puppettest/resourcetesting.rb
@@ -17,6 +17,15 @@ module PuppetTest::ResourceTesting
         return interp, scope, source
     end
 
+    def mkevaltest(interp = nil)
+        interp ||= mkinterp
+        @interp.newdefine("evaltest",
+            :arguments => [%w{one}, ["two", stringobj("755")]],
+            :code => resourcedef("file", "/tmp",
+                "owner" => varref("one"), "mode" => varref("two"))
+        )
+    end
+
     def mkresource(args = {})
 
         if args[:scope] and ! args[:source]
diff --git a/test/other/report.rb b/test/other/report.rb
index b98813927..e31e97505 100755
--- a/test/other/report.rb
+++ b/test/other/report.rb
@@ -87,9 +87,16 @@ class TestReports < Test::Unit::TestCase
             server = Puppet::Server::Report.new()
         }
 
+        report = trans.report
         assert_nothing_raised {
-            server.report_rrdgraph(trans.report)
+            server.report_rrdgraph(report)
         }
+
+        hostdir = File.join(Puppet[:rrddir], report.host)
+
+        assert(FileTest.directory?(hostdir), "Host rrd dir did not get created")
+        index = File.join(hostdir, "index.html")
+        assert(FileTest.exists?(index), "index file was not created")
     end
     else
     $stderr.puts "Install RRD for metric reporting tests"
diff --git a/test/other/transactions.rb b/test/other/transactions.rb
index 381a434c7..82b2bc768 100644
--- a/test/other/transactions.rb
+++ b/test/other/transactions.rb
@@ -40,7 +40,7 @@ class TestTransactions < Test::Unit::TestCase
         assert(metrics, "Did not get any metrics")
         assert(metrics.length > 0, "Did not get any metrics")
 
-        assert(metrics.has_key?("objects"), "Did not get object metrics")
+        assert(metrics.has_key?("resources"), "Did not get object metrics")
         assert(metrics.has_key?("changes"), "Did not get change metrics")
 
         metrics.each do |name, metric|
diff --git a/test/tagging/tagging.rb b/test/tagging/tagging.rb
index b30160bcb..3044992d5 100644
--- a/test/tagging/tagging.rb
+++ b/test/tagging/tagging.rb
@@ -58,7 +58,7 @@ class TestTagging < Test::Unit::TestCase
         }
 
         assert_nothing_raised {
-            assert_equal(%w{solaris}, resource.tags,
+            assert_equal(%w{solaris file}, resource.tags,
                 "Incorrect tags")
         }
     end