[4638] Cleanup of plurals and inheritance relationships in AST

Changed the grammar so that the following "plural" constructs always
parse as an ASTArray:

- funcvalues
- rvalues
- resourceinstances
- anyparams
- params
- caseopts
- casevalues

And the following "singluar" construct never parses as an ASTArray:

- statement

The previous behavior was for these constructs to parse as a scalar
when they represented a single item and an ASTArray when they
contained zero or multiple items.  ("Statement" could sometimes
represent a single item because a single resource declaration could
represent multiple resources).  This complicated other grammar rules
and caused ambiguous handling of nested arrays.

Also made these changes to the AST class hierarchy:

- ResourceInstance no longer derives from ASTArray.  This relationship
  was not meaningful because a ResourceInstance is a (title,
  parameters) pair, not an array, and it produced complications when
  we wanted to represent an array of ResourceInstance objects.

- Resource no longer derives from ResourceReference.  No significant
  functionality was being inherited and the relationship doesn't make
  sense in an AST context.

- ResourceOverride no longer derives from Resource.  No significant
  functionality was being inherited and the relationship doesn't make
  sense in an AST context.

- Resource can now represent a compound resource instance such as
  "notify { foo: ; bar: }".  This saves the parser from having to
  use represent a statement as an array of objects.

- ASTArray's evaluate method never flattens out arrays of arrays.

5 files changed, 73 insertions, 104 deletions

diff --git a/lib/puppet/parser/ast/astarray.rb b/lib/puppet/parser/ast/astarray.rb
index 432300c7a..b62c820ca 100644
--- a/lib/puppet/parser/ast/astarray.rb
+++ b/lib/puppet/parser/ast/astarray.rb
@@ -21,22 +21,8 @@ class Puppet::Parser::AST
 
     # Evaluate our children.
     def evaluate(scope)
-      # Make a new array, so we don't have to deal with the details of
-      # flattening and such
-      items = []
-
-      # First clean out any AST::ASTArrays
-      @children.each { |child|
-        if child.instance_of?(AST::ASTArray)
-          child.each do |ac|
-            items << ac
-          end
-        else
-          items << child
-        end
-      }
-
-      rets = items.flatten.collect { |child|
+      result = []
+      @children.each do |child|
         if child.respond_to? :instantiate
           if is_a_namespace
             # no problem, just don't evaluate it.
@@ -48,10 +34,14 @@ class Puppet::Parser::AST
             raise error
           end
         else
-          child.safeevaluate(scope)
+          item = child.safeevaluate(scope)
+          if !item.nil?
+            # nil values are implicitly removed.
+            result.push(item)
+          end
         end
-      }
-      rets.reject { |o| o.nil? }
+      end
+      result
     end
 
     def push(*ary)
@@ -69,10 +59,4 @@ class Puppet::Parser::AST
       "[" + @children.collect { |c| c.to_s }.join(', ') + "]"
     end
   end
-
-  # A simple container class, containing the parameters for an object.
-  # Used for abstracting the grammar declarations.  Basically unnecessary
-  # except that I kept finding bugs because I had too many arrays that
-  # meant completely different things.
-  class ResourceInstance < ASTArray; end
 end
diff --git a/lib/puppet/parser/ast/caseopt.rb b/lib/puppet/parser/ast/caseopt.rb
index 4e296e82f..db4c2b024 100644
--- a/lib/puppet/parser/ast/caseopt.rb
+++ b/lib/puppet/parser/ast/caseopt.rb
@@ -18,16 +18,12 @@ class Puppet::Parser::AST
       # Cache the @default value.
       return @default if defined?(@default)
 
-      if @value.is_a?(AST::ASTArray)
-        @value.each { |subval|
-          if subval.is_a?(AST::Default)
-            @default = true
-            break
-          end
-        }
-      else
-        @default = true if @value.is_a?(AST::Default)
-      end
+      @value.each { |subval|
+        if subval.is_a?(AST::Default)
+          @default = true
+          break
+        end
+      }
 
       @default ||= false
 
@@ -36,23 +32,15 @@ class Puppet::Parser::AST
 
     # You can specify a list of values; return each in turn.
     def eachvalue(scope)
-      if @value.is_a?(AST::ASTArray)
-        @value.each { |subval|
-          yield subval.safeevaluate(scope)
-        }
-      else
-        yield @value.safeevaluate(scope)
-      end
+      @value.each { |subval|
+        yield subval.safeevaluate(scope)
+      }
     end
 
     def eachopt
-      if @value.is_a?(AST::ASTArray)
-        @value.each { |subval|
-          yield subval
-        }
-      else
-        yield @value
-      end
+      @value.each { |subval|
+        yield subval
+      }
     end
 
     # Evaluate the actual statements; this only gets called if
diff --git a/lib/puppet/parser/ast/resource.rb b/lib/puppet/parser/ast/resource.rb
index 0c58538d5..23207149f 100644
--- a/lib/puppet/parser/ast/resource.rb
+++ b/lib/puppet/parser/ast/resource.rb
@@ -3,26 +3,15 @@ require 'puppet/parser/ast/resource_reference'
 # Any normal puppet resource declaration.  Can point to a definition or a
 # builtin type.
 class Puppet::Parser::AST
-class Resource < AST::ResourceReference
+class Resource < AST::Branch
 
   associates_doc
 
-  attr_accessor :title, :type, :exported, :virtual
-  attr_reader :parameters
+  attr_accessor :type, :instances, :exported, :virtual
 
   # Does not actually return an object; instead sets an object
   # in the current scope.
   def evaluate(scope)
-    # Evaluate all of the specified params.
-    paramobjects = parameters.collect { |param|
-      param.safeevaluate(scope)
-    }
-
-    resource_titles = @title.safeevaluate(scope)
-
-    # it's easier to always use an array, even for only one name
-    resource_titles = [resource_titles] unless resource_titles.is_a?(Array)
-
     # We want virtual to be true if exported is true.  We can't
     # just set :virtual => self.virtual in the initialization,
     # because sometimes the :virtual attribute is set *after*
@@ -30,49 +19,49 @@ class Resource < AST::ResourceReference
     # is true.  Argh, this was a very tough one to track down.
     virt = self.virtual || self.exported
 
-    # This is where our implicit iteration takes place; if someone
-    # passed an array as the name, then we act just like the called us
-    # many times.
-    fully_qualified_type, resource_titles = scope.resolve_type_and_titles(type, resource_titles)
+    # First level of implicit iteration: build a resource for each
+    # instance.  This handles things like:
+    # file { '/foo': owner => blah; '/bar': owner => blah }
+    @instances.collect { |instance|
 
-    resource_titles.flatten.collect { |resource_title|
-      exceptwrap :type => Puppet::ParseError do
-        resource = Puppet::Parser::Resource.new(
-          fully_qualified_type, resource_title,
-          :parameters => paramobjects,
-          :file => self.file,
-          :line => self.line,
-          :exported => self.exported,
-          :virtual => virt,
-          :source => scope.source,
-          :scope => scope,
-        
-          :strict => true
-        )
+      # Evaluate all of the specified params.
+      paramobjects = instance.parameters.collect { |param|
+        param.safeevaluate(scope)
+      }
 
-        # And then store the resource in the compiler.
-        # At some point, we need to switch all of this to return
-        # resources instead of storing them like this.
-        scope.compiler.add_resource(scope, resource)
-        resource
-      end
-    }.reject { |resource| resource.nil? }
-  end
+      resource_titles = instance.title.safeevaluate(scope)
+
+      # it's easier to always use an array, even for only one name
+      resource_titles = [resource_titles] unless resource_titles.is_a?(Array)
+
+      fully_qualified_type, resource_titles = scope.resolve_type_and_titles(type, resource_titles)
+
+      # Second level of implicit iteration; build a resource for each
+      # title.  This handles things like:
+      # file { ['/foo', '/bar']: owner => blah }
+      resource_titles.flatten.collect { |resource_title|
+        exceptwrap :type => Puppet::ParseError do
+          resource = Puppet::Parser::Resource.new(
+            fully_qualified_type, resource_title,
+            :parameters => paramobjects,
+            :file => self.file,
+            :line => self.line,
+            :exported => self.exported,
+            :virtual => virt,
+            :source => scope.source,
+            :scope => scope,
 
-  # Set the parameters for our object.
-  def parameters=(params)
-    if params.is_a?(AST::ASTArray)
-      @parameters = params
-    else
+            :strict => true
+          )
 
-            @parameters = AST::ASTArray.new(
-                
-        :line => params.line,
-        :file => params.file,
-        
-        :children => [params]
-      )
-    end
+          # And then store the resource in the compiler.
+          # At some point, we need to switch all of this to return
+          # resources instead of storing them like this.
+          scope.compiler.add_resource(scope, resource)
+          resource
+        end
+      }
+    }.flatten.reject { |resource| resource.nil? }
   end
 end
 end
diff --git a/lib/puppet/parser/ast/resource_instance.rb b/lib/puppet/parser/ast/resource_instance.rb
new file mode 100644
index 000000000..ebfb17bf1
--- /dev/null
+++ b/lib/puppet/parser/ast/resource_instance.rb
@@ -0,0 +1,9 @@
+require 'puppet/parser/ast/branch'
+
+class Puppet::Parser::AST
+  class ResourceInstance < Branch
+    # A simple container for a parameter for an object.  Consists of a
+    # title and a set of parameters.
+    attr_accessor :title, :parameters
+  end
+end
diff --git a/lib/puppet/parser/ast/resource_override.rb b/lib/puppet/parser/ast/resource_override.rb
index e0be889ff..d638202ab 100644
--- a/lib/puppet/parser/ast/resource_override.rb
+++ b/lib/puppet/parser/ast/resource_override.rb
@@ -3,12 +3,11 @@ require 'puppet/parser/ast/resource'
 class Puppet::Parser::AST
   # Set a parameter on a resource specification created somewhere else in the
   # configuration.  The object is responsible for verifying that this is allowed.
-  class ResourceOverride < Resource
+  class ResourceOverride < AST::Branch
 
     associates_doc
 
-    attr_accessor :object
-    attr_reader :parameters
+    attr_accessor :object, :parameters
 
     # Iterate across all of our children.
     def each