Fixing #472. Apparently this has been broken since I did the parser redesign. I had to fix the scope trees so that subclass scopes are subscopes of the parent scopes, which used to be the case but was far more complicated.

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

8 files changed, 307 insertions, 185 deletions

diff --git a/lib/puppet/parser/ast/component.rb b/lib/puppet/parser/ast/component.rb
index 1b14d508b..8f82d530a 100644
--- a/lib/puppet/parser/ast/component.rb
+++ b/lib/puppet/parser/ast/component.rb
@@ -39,7 +39,7 @@ class Puppet::Parser::AST
             if exported or origscope.exported?
                 scope.exported = true
             end
-            scope = scope
+
             # Additionally, add a tag for whatever kind of class
             # we are
             if @type != "" and ! @type.nil?
diff --git a/lib/puppet/parser/ast/hostclass.rb b/lib/puppet/parser/ast/hostclass.rb
index 4a4664f0f..65dd27542 100644
--- a/lib/puppet/parser/ast/hostclass.rb
+++ b/lib/puppet/parser/ast/hostclass.rb
@@ -33,19 +33,21 @@ class Puppet::Parser::AST
             if @parentclass
                 if pklass = self.parentclass
                     pklass.safeevaluate :scope => scope
+
+                    scope = parent_scope(scope, pklass)
                 else
                     parsefail "Could not find class %s" % @parentclass
                 end
             end
 
-            # Set the class before we do anything else, so that it's set
-            # during the evaluation and can be inspected.
-            scope.setclass(self)
-
             unless hash[:nosubscope]
                 scope = subscope(scope)
             end
 
+            # Set the class before we do anything else, so that it's set
+            # during the evaluation and can be inspected.
+            scope.setclass(self)
+
             # Now evaluate our code, yo.
             if self.code
                 return self.code.evaluate(:scope => scope)
@@ -58,6 +60,14 @@ class Puppet::Parser::AST
             @parentclass = nil
             super
         end
+
+        def parent_scope(scope, klass)
+            if s = scope.class_scope(klass)
+                return s
+            else
+                raise Puppet::DevError, "Could not find scope for %s" % klass.fqname
+            end
+        end
     end
 end
 
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 12a3c3430..83a8d8d7f 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -108,6 +108,17 @@ class Puppet::Parser::Scope
         return true
     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.
+    def class_scope(klass)
+        if klass.respond_to?(:fqname)
+            @classtable[klass.fqname]
+        else
+            @classtable[klass]
+        end
+    end
+
     # Return the list of collections.
     def collections
         @collecttable
@@ -411,7 +422,7 @@ class Puppet::Parser::Scope
                 raise Puppet::DevError, "Got a %s with no fully qualified name" %
                     obj.class
             end
-            @classtable[obj.fqname] = obj
+            @classtable[obj.fqname] = self
         else
             raise Puppet::DevError, "Invalid class %s" % obj.inspect
         end
diff --git a/test/language/ast.rb b/test/language/ast.rb
index 0e4c6c31c..a71d26c76 100755
--- a/test/language/ast.rb
+++ b/test/language/ast.rb
@@ -159,185 +159,6 @@ class TestAST < Test::Unit::TestCase
         end
     end
 
-    def test_hostclass
-        interp, scope, source = mkclassframing
-
-        # Create the class we're testing, first with no parent
-        klass = interp.newclass "first",
-            :code => AST::ASTArray.new(
-                :children => [resourcedef("file", "/tmp",
-                        "owner" => "nobody", "mode" => "755")]
-            )
-
-        assert_nothing_raised do
-            klass.evaluate(:scope => scope)
-        end
-
-        # Then try it again
-        assert_nothing_raised do
-            klass.evaluate(:scope => scope)
-        end
-
-        assert(scope.setclass?(klass), "Class was not considered evaluated")
-
-        tmp = scope.findresource("File[/tmp]")
-        assert(tmp, "Could not find file /tmp")
-        assert_equal("nobody", tmp[:owner])
-        assert_equal("755", tmp[:mode])
-
-        # Now create a couple more classes.
-        newbase = interp.newclass "newbase",
-            :code => AST::ASTArray.new(
-                :children => [resourcedef("file", "/tmp/other",
-                        "owner" => "nobody", "mode" => "644")]
-            )
-
-        newsub = interp.newclass "newsub",
-            :parent => "newbase",
-            :code => AST::ASTArray.new(
-                :children => [resourcedef("file", "/tmp/yay",
-                        "owner" => "nobody", "mode" => "755"),
-                    resourceoverride("file", "/tmp/other",
-                            "owner" => "daemon")
-                ]
-            )
-
-        # Override a different variable in the top scope.
-        moresub = interp.newclass "moresub",
-            :parent => "newbase",
-            :code => AST::ASTArray.new(
-                :children => [resourceoverride("file", "/tmp/other",
-                            "mode" => "755")]
-            )
-
-        assert_nothing_raised do
-            newsub.evaluate(:scope => scope)
-        end
-
-        assert_nothing_raised do
-            moresub.evaluate(:scope => scope)
-        end
-
-        assert(scope.setclass?(newbase), "Did not eval newbase")
-        assert(scope.setclass?(newsub), "Did not eval newsub")
-
-        yay = scope.findresource("File[/tmp/yay]")
-        assert(yay, "Did not find file /tmp/yay")
-        assert_equal("nobody", yay[:owner])
-        assert_equal("755", yay[:mode])
-
-        other = scope.findresource("File[/tmp/other]")
-        assert(other, "Did not find file /tmp/other")
-        assert_equal("daemon", other[:owner])
-        assert_equal("755", other[:mode])
-    end
-
-    def test_component
-        interp, scope, source = mkclassframing
-
-        # Create a new definition
-        klass = interp.newdefine "yayness",
-            :arguments => [["owner", stringobj("nobody")], %w{mode}],
-            :code => AST::ASTArray.new(
-                :children => [resourcedef("file", "/tmp/$name",
-                        "owner" => varref("owner"), "mode" => varref("mode"))]
-            )
-
-        # Test validattr? a couple different ways
-        [:owner, "owner", :schedule, "schedule"].each do |var|
-            assert(klass.validattr?(var), "%s was not considered valid" % var.inspect)
-        end
-
-        [:random, "random"].each do |var|
-            assert(! klass.validattr?(var), "%s was considered valid" % var.inspect)
-        end
-        # Now call it a couple of times
-        # First try it without a required param
-        assert_raise(Puppet::ParseError) do
-            klass.evaluate(:scope => scope,
-                :name => "bad",
-                :arguments => {"owner" => "nobody"}
-            )
-        end
-
-        # And make sure it didn't create the file
-        assert_nil(scope.findresource("File[/tmp/bad]"),
-            "Made file with invalid params")
-
-        assert_nothing_raised do
-            klass.evaluate(:scope => scope,
-                :name => "first",
-                :arguments => {"mode" => "755"}
-            )
-        end
-
-        firstobj = scope.findresource("File[/tmp/first]")
-        assert(firstobj, "Did not create /tmp/first obj")
-
-        assert_equal("file", firstobj.type)
-        assert_equal("/tmp/first", firstobj.title)
-        assert_equal("nobody", firstobj[:owner])
-        assert_equal("755", firstobj[:mode])
-
-        # Make sure we can't evaluate it with the same args
-        assert_raise(Puppet::ParseError) do
-            klass.evaluate(:scope => scope,
-                :name => "first",
-                :arguments => {"mode" => "755"}
-            )
-        end
-
-        # Now create another with different args
-        assert_nothing_raised do
-            klass.evaluate(:scope => scope,
-                :name => "second",
-                :arguments => {"mode" => "755", "owner" => "daemon"}
-            )
-        end
-
-        secondobj = scope.findresource("File[/tmp/second]")
-        assert(secondobj, "Did not create /tmp/second obj")
-
-        assert_equal("file", secondobj.type)
-        assert_equal("/tmp/second", secondobj.title)
-        assert_equal("daemon", secondobj[:owner])
-        assert_equal("755", secondobj[:mode])
-    end
-
-    # Make sure that classes set their namespaces to themselves.  This
-    # way they start looking for definitions in their own namespace.
-    def test_hostclass_namespace
-        interp, scope, source = mkclassframing
-
-        # Create a new class
-        klass = nil
-        assert_nothing_raised do
-            klass = interp.newclass "funtest"
-        end
-
-        # Now define a definition in that namespace
-
-        define = nil
-        assert_nothing_raised do
-            define = interp.newdefine "funtest::mydefine"
-        end
-
-        assert_equal("funtest", klass.namespace,
-            "component namespace was not set in the class")
-
-        assert_equal("funtest", define.namespace,
-            "component namespace was not set in the definition")
-
-        newscope = klass.subscope(scope)
-
-        assert_equal("funtest", newscope.namespace,
-            "Scope did not inherit namespace")
-
-        # Now make sure we can find the define
-        assert(newscope.finddefine("mydefine"),
-            "Could not find definition in my enclosing class")
-    end
-
     def test_node
         interp = mkinterp
         scope = mkscope(:interp => interp)
diff --git a/test/language/ast/component.rb b/test/language/ast/component.rb
new file mode 100755
index 000000000..0492d692d
--- /dev/null
+++ b/test/language/ast/component.rb
@@ -0,0 +1,90 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke A. Kanies on 2006-02-20.
+#  Copyright (c) 2006. All rights reserved.
+
+$:.unshift("../../lib") if __FILE__ =~ /\.rb$/
+
+require 'puppettest'
+require 'puppettest/parsertesting'
+require 'puppettest/resourcetesting'
+
+class TestASTComponent < Test::Unit::TestCase
+	include PuppetTest
+	include PuppetTest::ParserTesting
+    include PuppetTest::ResourceTesting
+	AST = Puppet::Parser::AST
+
+    def test_component
+        interp, scope, source = mkclassframing
+
+        # Create a new definition
+        klass = interp.newdefine "yayness",
+            :arguments => [["owner", stringobj("nobody")], %w{mode}],
+            :code => AST::ASTArray.new(
+                :children => [resourcedef("file", "/tmp/$name",
+                        "owner" => varref("owner"), "mode" => varref("mode"))]
+            )
+
+        # Test validattr? a couple different ways
+        [:owner, "owner", :schedule, "schedule"].each do |var|
+            assert(klass.validattr?(var), "%s was not considered valid" % var.inspect)
+        end
+
+        [:random, "random"].each do |var|
+            assert(! klass.validattr?(var), "%s was considered valid" % var.inspect)
+        end
+        # Now call it a couple of times
+        # First try it without a required param
+        assert_raise(Puppet::ParseError) do
+            klass.evaluate(:scope => scope,
+                :name => "bad",
+                :arguments => {"owner" => "nobody"}
+            )
+        end
+
+        # And make sure it didn't create the file
+        assert_nil(scope.findresource("File[/tmp/bad]"),
+            "Made file with invalid params")
+
+        assert_nothing_raised do
+            klass.evaluate(:scope => scope,
+                :name => "first",
+                :arguments => {"mode" => "755"}
+            )
+        end
+
+        firstobj = scope.findresource("File[/tmp/first]")
+        assert(firstobj, "Did not create /tmp/first obj")
+
+        assert_equal("file", firstobj.type)
+        assert_equal("/tmp/first", firstobj.title)
+        assert_equal("nobody", firstobj[:owner])
+        assert_equal("755", firstobj[:mode])
+
+        # Make sure we can't evaluate it with the same args
+        assert_raise(Puppet::ParseError) do
+            klass.evaluate(:scope => scope,
+                :name => "first",
+                :arguments => {"mode" => "755"}
+            )
+        end
+
+        # Now create another with different args
+        assert_nothing_raised do
+            klass.evaluate(:scope => scope,
+                :name => "second",
+                :arguments => {"mode" => "755", "owner" => "daemon"}
+            )
+        end
+
+        secondobj = scope.findresource("File[/tmp/second]")
+        assert(secondobj, "Did not create /tmp/second obj")
+
+        assert_equal("file", secondobj.type)
+        assert_equal("/tmp/second", secondobj.title)
+        assert_equal("daemon", secondobj[:owner])
+        assert_equal("755", secondobj[:mode])
+    end
+end
+# $Id$
diff --git a/test/language/ast/hostclass.rb b/test/language/ast/hostclass.rb
new file mode 100755
index 000000000..64ecc4026
--- /dev/null
+++ b/test/language/ast/hostclass.rb
@@ -0,0 +1,147 @@
+#!/usr/bin/env ruby
+#
+#  Created by Luke A. Kanies on 2006-02-20.
+#  Copyright (c) 2006. All rights reserved.
+
+$:.unshift("../../lib") if __FILE__ =~ /\.rb$/
+
+require 'puppettest'
+require 'puppettest/parsertesting'
+require 'puppettest/resourcetesting'
+
+class TestASTHostClass < Test::Unit::TestCase
+	include PuppetTest
+	include PuppetTest::ParserTesting
+    include PuppetTest::ResourceTesting
+	AST = Puppet::Parser::AST
+
+    def test_hostclass
+        interp, scope, source = mkclassframing
+
+        # Create the class we're testing, first with no parent
+        klass = interp.newclass "first",
+            :code => AST::ASTArray.new(
+                :children => [resourcedef("file", "/tmp",
+                        "owner" => "nobody", "mode" => "755")]
+            )
+
+        assert_nothing_raised do
+            klass.evaluate(:scope => scope)
+        end
+
+        # Then try it again
+        assert_nothing_raised do
+            klass.evaluate(:scope => scope)
+        end
+
+        assert(scope.setclass?(klass), "Class was not considered evaluated")
+
+        tmp = scope.findresource("File[/tmp]")
+        assert(tmp, "Could not find file /tmp")
+        assert_equal("nobody", tmp[:owner])
+        assert_equal("755", tmp[:mode])
+
+        # Now create a couple more classes.
+        newbase = interp.newclass "newbase",
+            :code => AST::ASTArray.new(
+                :children => [resourcedef("file", "/tmp/other",
+                        "owner" => "nobody", "mode" => "644")]
+            )
+
+        newsub = interp.newclass "newsub",
+            :parent => "newbase",
+            :code => AST::ASTArray.new(
+                :children => [resourcedef("file", "/tmp/yay",
+                        "owner" => "nobody", "mode" => "755"),
+                    resourceoverride("file", "/tmp/other",
+                            "owner" => "daemon")
+                ]
+            )
+
+        # Override a different variable in the top scope.
+        moresub = interp.newclass "moresub",
+            :parent => "newbase",
+            :code => AST::ASTArray.new(
+                :children => [resourceoverride("file", "/tmp/other",
+                            "mode" => "755")]
+            )
+
+        assert_nothing_raised do
+            newsub.evaluate(:scope => scope)
+        end
+
+        assert_nothing_raised do
+            moresub.evaluate(:scope => scope)
+        end
+
+        assert(scope.setclass?(newbase), "Did not eval newbase")
+        assert(scope.setclass?(newsub), "Did not eval newsub")
+
+        yay = scope.findresource("File[/tmp/yay]")
+        assert(yay, "Did not find file /tmp/yay")
+        assert_equal("nobody", yay[:owner])
+        assert_equal("755", yay[:mode])
+
+        other = scope.findresource("File[/tmp/other]")
+        assert(other, "Did not find file /tmp/other")
+        assert_equal("daemon", other[:owner])
+        assert_equal("755", other[:mode])
+    end
+
+    # Make sure that classes set their namespaces to themselves.  This
+    # way they start looking for definitions in their own namespace.
+    def test_hostclass_namespace
+        interp, scope, source = mkclassframing
+
+        # Create a new class
+        klass = nil
+        assert_nothing_raised do
+            klass = interp.newclass "funtest"
+        end
+
+        # Now define a definition in that namespace
+
+        define = nil
+        assert_nothing_raised do
+            define = interp.newdefine "funtest::mydefine"
+        end
+
+        assert_equal("funtest", klass.namespace,
+            "component namespace was not set in the class")
+
+        assert_equal("funtest", define.namespace,
+            "component namespace was not set in the definition")
+
+        newscope = klass.subscope(scope)
+
+        assert_equal("funtest", newscope.namespace,
+            "Scope did not inherit namespace")
+
+        # Now make sure we can find the define
+        assert(newscope.finddefine("mydefine"),
+            "Could not find definition in my enclosing class")
+    end
+
+    # Make sure that our scope is a subscope of the parentclass's scope.
+    def test_parent_scope_from_parentclass
+        interp = mkinterp
+
+        interp.newclass("base")
+        interp.newclass("sub", :parent => "base")
+        scope = mkscope :interp => interp
+
+        ret = nil
+        assert_nothing_raised do
+            ret = scope.evalclasses("sub")
+        end
+
+        subscope = scope.class_scope(scope.findclass("sub"))
+        assert(subscope, "could not find sub scope")
+        pscope = scope.class_scope(scope.findclass("base"))
+        assert(pscope, "could not find parent scope")
+
+        assert(pscope == subscope.parent, "parent scope was not set correctly")
+    end
+end
+
+# $Id$
diff --git a/test/language/resource.rb b/test/language/resource.rb
index 1e6b23136..92a4aa840 100755
--- a/test/language/resource.rb
+++ b/test/language/resource.rb
@@ -449,6 +449,44 @@ class TestResource < Test::Unit::TestCase
         compare_resources(host, res, :params => %w{owner source mode check})
     end
     end
+
+    # #472.  Really, this still isn't the best behaviour, but at least
+    # it's consistent with what we have elsewhere.
+    def test_defaults_from_parent_classes
+        # Make a parent class with some defaults in it
+        @interp.newclass("base",
+            :code => defaultobj("file", :owner => "root", :group => "root")
+        )
+
+        # Now a mid-level class with some different values
+        @interp.newclass("middle", :parent => "base",
+            :code => defaultobj("file", :owner => "bin", :mode => "755")
+        )
+
+        # Now a lower class with its own defaults plus a resource
+        @interp.newclass("bottom", :parent => "middle",
+            :code => AST::ASTArray.new(:children => [
+                defaultobj("file", :owner => "adm", :recurse => "true"),
+                resourcedef("file", "/tmp/yayness", {})
+            ])
+        )
+
+        # Now evaluate the class.
+        assert_nothing_raised("Failed to evaluate class tree") do
+            @scope.evalclasses("bottom")
+        end
+
+        # Make sure our resource got created.
+        res = @scope.findresource("File[/tmp/yayness]")
+        assert_nothing_raised("Could not add defaults") do
+            res.adddefaults
+        end
+        assert(res, "could not find resource")
+        {:owner => "adm", :recurse => "true", :group => "root", :mode => "755"}.each do |param, value|
+            assert_equal(value, res[param], "%s => %s did not inherit correctly" %
+                [param, value])
+        end
+    end
 end
 
 # $Id$
diff --git a/test/language/scope.rb b/test/language/scope.rb
index 80f838c59..9b7f1baea 100755
--- a/test/language/scope.rb
+++ b/test/language/scope.rb
@@ -242,7 +242,12 @@ class TestScope < Test::Unit::TestCase
         assert(scope.setclass?(base), "Class incorrectly unset")
         assert(scope.classlist.include?("base"), "Class not in classlist")
 
+        # Make sure we can retrieve the scope.
+        assert_equal(scope, scope.class_scope(base),
+            "class scope was not set correctly")
+
         # Now try it with a normal string
+        Puppet[:trace] = false
         assert_raise(Puppet::DevError) do
             scope.setclass "string"
         end