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.

3 files changed, 60 insertions, 44 deletions

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