Move scope parenting & class_scope from Compiler to Scope

This refactor fixes about a quarter of the test failures on master and (I
hope) will simplify some of the integration issues on the testing branch.
It is my best guess at The Right Thing To Do (or at least a step in that
direction) but I could be persuaded otherwise.

The basic idea is to take responsibility for maintaining scope hierarchy and
class_name -> class_scope mapping out of the compiler class and put it in the
scope class where it arguably belongs.  To maintain the semantics, class
scopes are all tracked by the "top level" scope, though this could be relaxed
if the nesting semantics were ever needed.

If this winds up being the right thing to do, related routines (e.g. newscope)
should be sorted out as well.

6 files changed, 97 insertions, 101 deletions

diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
index 6b6cd6815..51df86ec6 100644
--- a/lib/puppet/parser/compiler.rb
+++ b/lib/puppet/parser/compiler.rb
@@ -51,32 +51,11 @@ class Puppet::Parser::Compiler
         parser.nodes?
     end
 
-    # Store the fact that we've evaluated a class, and store a reference to
-    # the scope in which it was evaluated, so that we can look it up later.
-    def class_set(name, scope)
-        if existing = @class_scopes[name]
-            if existing.nodescope? != scope.nodescope?
-                raise Puppet::ParseError, "Cannot have classes, nodes, or definitions with the same name"
-            else
-                raise Puppet::DevError, "Somehow evaluated %s %s twice" % [ existing.nodescope? ? "node" : "class", name]
-            end
-        end
-        @class_scopes[name] = scope
+    # Store the fact that we've evaluated a class
+    def add_class(name)
         @catalog.add_class(name) unless name == ""
     end
 
-    # Return the scope associated with a class.  This is just here so
-    # that subclasses can set their parent scopes to be the scope of
-    # their parent class, and it's also used when looking up qualified
-    # variables.
-    def class_scope(klass)
-        # They might pass in either the class or class name
-        if klass.respond_to?(:name)
-            @class_scopes[klass.name]
-        else
-            @class_scopes[klass]
-        end
-    end
 
     # Return a list of all of the defined classes.
     def classlist
@@ -138,7 +117,7 @@ class Puppet::Parser::Compiler
         classes.each do |name|
             # If we can find the class, then make a resource that will evaluate it.
             if klass = scope.find_hostclass(name)
-                found << name and next if class_scope(klass)
+                found << name and next if scope.class_scope(klass)
 
                 resource = klass.mk_plain_resource(scope)
 
@@ -180,27 +159,16 @@ class Puppet::Parser::Compiler
     end
 
     # Create a new scope, with either a specified parent scope or
-    # using the top scope.  Adds an edge between the scope and
-    # its parent to the graph.
+    # using the top scope.  
     def newscope(parent, options = {})
         parent ||= topscope
         options[:compiler] = self
         options[:parser] ||= self.parser
         scope = Puppet::Parser::Scope.new(options)
-        @scope_graph.add_edge(parent, scope)
+        scope.parent = parent
         scope
     end
 
-    # Find the parent of a given scope.  Assumes scopes only ever have
-    # one in edge, which will always be true.
-    def parent(scope)
-        if ary = @scope_graph.adjacent(scope, :direction => :in) and ary.length > 0
-            ary[0]
-        else
-            nil
-        end
-    end
-
     # Return any overrides for the given resource.
     def resource_overrides(resource)
         @resource_overrides[resource.ref]
@@ -236,7 +204,7 @@ class Puppet::Parser::Compiler
 
         # Now set the node scope appropriately, so that :topscope can
         # behave differently.
-        @node_scope = class_scope(astnode)
+        @node_scope = topscope.class_scope(astnode)
     end
 
     # Evaluate our collections and return true if anything returned an object.
@@ -384,15 +352,10 @@ class Puppet::Parser::Compiler
     def init_main
         # Create our initial scope and a resource that will evaluate main.
         @topscope = Puppet::Parser::Scope.new(:compiler => self, :parser => self.parser)
-        @scope_graph.add_vertex(@topscope)
     end
 
     # Set up all of our internal variables.
     def initvars
-        # The table for storing class singletons.  This will only actually
-        # be used by top scopes and node scopes.
-        @class_scopes = {}
-
         # The list of objects that will available for export.
         @exported_resources = {}
 
@@ -406,9 +369,6 @@ class Puppet::Parser::Compiler
         # but they each refer back to the scope that created them.
         @collections = []
 
-        # A graph for maintaining scope relationships.
-        @scope_graph = Puppet::SimpleGraph.new
-
         # For maintaining the relationship between scopes and their resources.
         @catalog = Puppet::Resource::Catalog.new(@node.name)
         @catalog.version = @parser.version
diff --git a/lib/puppet/parser/resource_type.rb b/lib/puppet/parser/resource_type.rb
index a67965960..3dbcbcb62 100644
--- a/lib/puppet/parser/resource_type.rb
+++ b/lib/puppet/parser/resource_type.rb
@@ -165,6 +165,7 @@ class Puppet::Parser::ResourceType
 
         scope.setvar("title", resource.title) unless set.include? :title
         scope.setvar("name", resource.name) unless set.include? :name
+        scope.class_set(self.name,scope)
     end
 
     # Create a new subscope in which to evaluate our code.
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index d6d663041..eda53a8cb 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -18,7 +18,7 @@ class Puppet::Parser::Scope
     attr_accessor :level, :parser, :source, :resource
     attr_accessor :base, :keyword, :nodescope
     attr_accessor :top, :translated, :compiler
-    attr_writer :parent
+    attr_accessor :parent
 
     # A demeterific shortcut to the catalog.
     def catalog
@@ -139,6 +139,34 @@ class Puppet::Parser::Scope
         @defaults = Hash.new { |dhash,type|
             dhash[type] = {}
         }
+
+        # The table for storing class singletons.  This will only actually
+        # be used by top scopes and node scopes.
+        @class_scopes = {}
+    end
+
+    # Store the fact that we've evaluated a class, and store a reference to
+    # the scope in which it was evaluated, so that we can look it up later.
+    def class_set(name, scope)
+        return parent.class_set(name,scope) if parent
+        if existing = @class_scopes[name]
+            if existing.nodescope? != scope.nodescope?
+                raise Puppet::ParseError, "Cannot have classes, nodes, or definitions with the same name"
+            else
+                raise Puppet::DevError, "Somehow evaluated %s %s twice" % [ existing.nodescope? ? "node" : "class", name]
+            end
+        end
+        @class_scopes[name] = scope
+    end
+
+    # Return the scope associated with a class.  This is just here so
+    # that subclasses can set their parent scopes to be the scope of
+    # their parent class, and it's also used when looking up qualified
+    # variables.
+    def class_scope(klass)
+        # They might pass in either the class or class name
+        k = klass.respond_to?(:name) ? klass.name : klass
+        @class_scopes[k] || (parent && parent.class_scope(k))
     end
 
     # Collect all of the defaults set at any higher scopes.
@@ -182,7 +210,7 @@ class Puppet::Parser::Scope
             warning "Could not look up qualified variable '%s'; class %s could not be found" % [name, klassname]
             return usestring ? "" : :undefined
         end
-        unless kscope = compiler.class_scope(klass)
+        unless kscope = class_scope(klass)
             warning "Could not look up qualified variable '%s'; class %s has not been evaluated" % [name, klassname]
             return usestring ? "" : :undefined
         end
@@ -251,15 +279,6 @@ class Puppet::Parser::Scope
         self.nodescope
     end
 
-    # We probably shouldn't cache this value...  But it's a lot faster
-    # than doing lots of queries.
-    def parent
-        unless defined?(@parent)
-            @parent = compiler.parent(self)
-        end
-        @parent
-    end
-
     # Return the list of scopes up to the top scope, ordered with our own first.
     # This is used for looking up variables and defaults.
     def scope_path
diff --git a/spec/unit/parser/compiler.rb b/spec/unit/parser/compiler.rb
index 8a41242d6..d23d22565 100755
--- a/spec/unit/parser/compiler.rb
+++ b/spec/unit/parser/compiler.rb
@@ -40,26 +40,10 @@ describe Puppet::Parser::Compiler do
         @compiler = Puppet::Parser::Compiler.new(@node, @parser)
     end
 
-    it "should be able to store references to class scopes" do
-        lambda { @compiler.class_set "myname", "myscope" }.should_not raise_error
-    end
-
-    it "should be able to retrieve class scopes by name" do
-        @compiler.class_set "myname", "myscope"
-        @compiler.class_scope("myname").should == "myscope"
-    end
-
-    it "should be able to retrieve class scopes by object" do
-        klass = mock 'ast_class'
-        klass.expects(:name).returns("myname")
-        @compiler.class_set "myname", "myscope"
-        @compiler.class_scope(klass).should == "myscope"
-    end
-
-    it "should be able to return a class list containing all set classes" do
-        @compiler.class_set "", "empty"
-        @compiler.class_set "one", "yep"
-        @compiler.class_set "two", "nope"
+    it "should be able to return a class list containing all added classes" do
+        @compiler.add_class ""
+        @compiler.add_class "one"
+        @compiler.add_class "two"
 
         @compiler.classlist.sort.should == %w{one two}.sort
     end
@@ -116,7 +100,14 @@ describe Puppet::Parser::Compiler do
             scope = mock 'scope'
             newscope = @compiler.newscope(scope)
 
-            @compiler.parent(newscope).should equal(scope)
+            newscope.parent.should equal(scope)
+        end
+
+        it "should set the parent scope of the new scope to its topscope if the parent passed in is nil" do
+            scope = mock 'scope'
+            newscope = @compiler.newscope(nil)
+
+            newscope.parent.should equal(@compiler.topscope)
         end
     end
 
@@ -406,6 +397,7 @@ describe Puppet::Parser::Compiler do
             @compiler.catalog.stubs(:tag)
 
             @class.expects(:mk_plain_resource).with(@scope)
+            @scope.stubs(:class_scope).with(@class)
 
             @compiler.evaluate_classes(%w{myclass}, @scope)
         end
@@ -416,6 +408,7 @@ describe Puppet::Parser::Compiler do
             @resource.expects(:evaluate).never
 
             @class.expects(:mk_plain_resource).returns(@resource)
+            @scope.stubs(:class_scope).with(@class)
 
             @compiler.evaluate_classes(%w{myclass}, @scope)
         end
@@ -425,6 +418,7 @@ describe Puppet::Parser::Compiler do
 
             @resource.expects(:evaluate)
             @class.expects(:mk_plain_resource).returns(@resource)
+            @scope.stubs(:class_scope).with(@class)
 
             @compiler.evaluate_classes(%w{myclass}, @scope, false)
         end
@@ -432,7 +426,7 @@ describe Puppet::Parser::Compiler do
         it "should skip classes that have already been evaluated" do
             @compiler.catalog.stubs(:tag)
 
-            @compiler.expects(:class_scope).with(@class).returns("something")
+            @scope.stubs(:class_scope).with(@class).returns("something")
 
             @compiler.expects(:add_resource).never
 
@@ -445,7 +439,7 @@ describe Puppet::Parser::Compiler do
         it "should skip classes previously evaluated with different capitalization" do
             @compiler.catalog.stubs(:tag)
             @scope.stubs(:find_hostclass).with("MyClass").returns(@class)
-            @compiler.expects(:class_scope).with(@class).returns("something")
+            @scope.stubs(:class_scope).with(@class).returns("something")
             @compiler.expects(:add_resource).never
             @resource.expects(:evaluate).never
             Puppet::Parser::Resource.expects(:new).never
@@ -457,6 +451,7 @@ describe Puppet::Parser::Compiler do
 
             @compiler.stubs(:add_resource)
             @scope.stubs(:find_hostclass).with("notfound").returns(nil)
+            @scope.stubs(:class_scope).with(@class)
 
             Puppet::Parser::Resource.stubs(:new).returns(@resource)
             @class.stubs :mk_plain_resource
@@ -534,7 +529,7 @@ describe Puppet::Parser::Compiler do
 
             # The #evaluate method normally does this.
             scope = stub 'scope', :source => "mysource"
-            @compiler.class_set(node_class.name, scope)
+            @compiler.topscope.expects(:class_scope).with(node_class).returns(scope)
             node_resource.stubs(:evaluate)
 
             @compiler.compile
@@ -582,23 +577,4 @@ describe Puppet::Parser::Compiler do
             lambda { @compiler.compile }.should raise_error(Puppet::ParseError)
         end
     end
-
-    # #620 - Nodes and classes should conflict, else classes don't get evaluated
-    describe "when evaluating nodes and classes with the same name (#620)" do
-
-        before do
-            @node = stub :nodescope? => true
-            @class = stub :nodescope? => false
-        end
-
-        it "should fail if a node already exists with the same name as the class being evaluated" do
-            @compiler.class_set("one", @node)
-            lambda { @compiler.class_set("one", @class) }.should raise_error(Puppet::ParseError)
-        end
-
-        it "should fail if a class already exists with the same name as the node being evaluated" do
-            @compiler.class_set("one", @class)
-            lambda { @compiler.class_set("one", @node) }.should raise_error(Puppet::ParseError)
-        end
-    end
 end
diff --git a/spec/unit/parser/resource_type.rb b/spec/unit/parser/resource_type.rb
index ddf8ce951..3afab69a5 100755
--- a/spec/unit/parser/resource_type.rb
+++ b/spec/unit/parser/resource_type.rb
@@ -214,6 +214,7 @@ describe Puppet::Parser::ResourceType do
 
             @scope.expects(:setvar).with("foo", "bar")
             @scope.expects(:setvar).with("boo", "baz")
+            @scope.stubs(:class_set).with("foo",@scope)
 
             @type.set_resource_parameters(@resource, @scope)
         end
@@ -222,6 +223,7 @@ describe Puppet::Parser::ResourceType do
             @type.set_arguments :foo => nil
             @resource.expects(:to_hash).returns(:foo => "bar")
             @scope.expects(:setvar).with("foo", "bar")
+            @scope.stubs(:class_set).with("foo",@scope)
 
             @type.set_resource_parameters(@resource, @scope)
         end
@@ -238,6 +240,7 @@ describe Puppet::Parser::ResourceType do
             @resource.expects(:to_hash).returns({})
 
             @scope.expects(:setvar).with("foo", "something")
+            @scope.stubs(:class_set).with("foo",@scope)
 
             @type.set_resource_parameters(@resource, @scope)
         end
@@ -254,6 +257,7 @@ describe Puppet::Parser::ResourceType do
 
             @resource.expects(:title).returns 'teetle'
             @scope.expects(:setvar).with("title", "teetle")
+            @scope.stubs(:class_set).with("foo",@scope)
 
             @type.set_resource_parameters(@resource, @scope)
         end
@@ -263,6 +267,7 @@ describe Puppet::Parser::ResourceType do
 
             @resource.expects(:name).returns 'nombre'
             @scope.expects(:setvar).with("name", "nombre")
+            @scope.stubs(:class_set).with("foo",@scope)
 
             @type.set_resource_parameters(@resource, @scope)
         end
diff --git a/spec/unit/parser/scope.rb b/spec/unit/parser/scope.rb
index d7800e4b3..04ae3047c 100755
--- a/spec/unit/parser/scope.rb
+++ b/spec/unit/parser/scope.rb
@@ -10,6 +10,41 @@ describe Puppet::Parser::Scope do
         @scope = Puppet::Parser::Scope.new()
         @scope.parent = @topscope
     end
+ 
+    it "should be able to store references to class scopes" do
+        lambda { @scope.class_set "myname", "myscope" }.should_not raise_error
+    end
+
+    it "should be able to retrieve class scopes by name" do
+        @scope.class_set "myname", "myscope"
+        @scope.class_scope("myname").should == "myscope"
+    end
+
+    it "should be able to retrieve class scopes by object" do
+        klass = mock 'ast_class'
+        klass.expects(:name).returns("myname")
+        @scope.class_set "myname", "myscope"
+        @scope.class_scope(klass).should == "myscope"
+    end
+
+    # #620 - Nodes and classes should conflict, else classes don't get evaluated
+    describe "when evaluating nodes and classes with the same name (#620)" do
+
+        before do
+            @node = stub :nodescope? => true
+            @class = stub :nodescope? => false
+        end
+
+        it "should fail if a node already exists with the same name as the class being evaluated" do
+            @scope.class_set("one", @node)
+            lambda { @scope.class_set("one", @class) }.should raise_error(Puppet::ParseError)
+        end
+
+        it "should fail if a class already exists with the same name as the node being evaluated" do
+            @scope.class_set("one", @class)
+            lambda { @scope.class_set("one", @node) }.should raise_error(Puppet::ParseError)
+        end
+    end
 
     describe "when looking up a variable" do
         it "should default to an empty string" do
@@ -52,7 +87,7 @@ describe Puppet::Parser::Scope do
                 klass = @parser.newclass(name)
                 Puppet::Parser::Resource.new(:type => "class", :title => name, :scope => @scope, :source => mock('source')).evaluate
 
-                return @compiler.class_scope(klass)
+                return @scope.class_scope(klass)
             end
 
             it "should be able to look up explicitly fully qualified variables from main" do