Merge pull request #3154 from hlindberg/PUP-3363_trailing-comma-assign

(PUP-3363) Make transformation of unparenthesized calls handle errors

5 files changed, 57 insertions, 9 deletions

diff --git a/lib/puppet/pops/model/factory.rb b/lib/puppet/pops/model/factory.rb
index cf77d9185..a814a56b9 100644
--- a/lib/puppet/pops/model/factory.rb
+++ b/lib/puppet/pops/model/factory.rb
@@ -785,6 +785,14 @@ class Puppet::Pops::Model::Factory
     STATEMENT_CALLS[name]
   end
 
+  class ArgsToNonCallError < RuntimeError
+    attr_reader :args, :name_expr
+    def initialize(args, name_expr)
+      @args = args
+      @name_expr = name_expr
+    end
+  end
+
   # Transforms an array of expressions containing literal name expressions to calls if followed by an
   # expression, or expression list.
   #
@@ -793,7 +801,12 @@ class Puppet::Pops::Model::Factory
       expr = expr.current if expr.is_a?(Puppet::Pops::Model::Factory)
       name = memo[-1]
       if name.is_a?(Model::QualifiedName) && STATEMENT_CALLS[name.value]
-        the_call = Puppet::Pops::Model::Factory.CALL_NAMED(name, false, expr.is_a?(Array) ? expr : [expr])
+        if expr.is_a?(Array)
+          expr = expr.reject {|e| e.is_a?(Puppet::Pops::Parser::LexerSupport::TokenValue) }
+        else
+          expr = [expr]
+        end
+        the_call = Puppet::Pops::Model::Factory.CALL_NAMED(name, false, expr)
         # last positioned is last arg if there are several
         record_position(the_call, name, expr.is_a?(Array) ? expr[-1]  : expr)
         memo[-1] = the_call
@@ -803,6 +816,8 @@ class Puppet::Pops::Model::Factory
           # an argument to the name to call transform above.
           expr.rval_required = true
         end
+      elsif expr.is_a?(Array)
+        raise ArgsToNonCallError.new(expr, name)
       else
         memo << expr
         if expr.is_a?(Model::CallNamedFunctionExpression)
diff --git a/lib/puppet/pops/parser/egrammar.ra b/lib/puppet/pops/parser/egrammar.ra
index 54b183a81..b9cc896ca 100644
--- a/lib/puppet/pops/parser/egrammar.ra
+++ b/lib/puppet/pops/parser/egrammar.ra
@@ -86,7 +86,7 @@ syntactic_statements
 #
 syntactic_statement
   : assignment                             =LOW { result = val[0] }
-  | syntactic_statement COMMA assignment   =LOW { result = aryfy(val[0]).push val[2] }
+  | syntactic_statement COMMA assignment   =LOW { result = aryfy(val[0]).push(val[1]).push(val[2]) }
 
 # Assignment (is right recursive since assignment is right associative)
 assignment
diff --git a/lib/puppet/pops/parser/eparser.rb b/lib/puppet/pops/parser/eparser.rb
index 26ff778fe..ef14487e8 100644
--- a/lib/puppet/pops/parser/eparser.rb
+++ b/lib/puppet/pops/parser/eparser.rb
@@ -1296,7 +1296,7 @@ module_eval(<<'.,.,', 'egrammar.ra', 87)
 
 module_eval(<<'.,.,', 'egrammar.ra', 88)
   def _reduce_9(val, _values, result)
-     result = aryfy(val[0]).push val[2] 
+     result = aryfy(val[0]).push(val[1]).push(val[2]) 
     result
   end
 .,.,
diff --git a/lib/puppet/pops/parser/parser_support.rb b/lib/puppet/pops/parser/parser_support.rb
index cb1c83aa2..ee4f16938 100644
--- a/lib/puppet/pops/parser/parser_support.rb
+++ b/lib/puppet/pops/parser/parser_support.rb
@@ -163,7 +163,25 @@ class Puppet::Pops::Parser::Parser
   # expression, or expression list
   #
   def transform_calls(expressions)
-    Factory.transform_calls(expressions)
+    # Factory transform raises an error if a non qualified name is followed by an argument list
+    # since there is no way that that can be transformed back to sanity. This occurs in situations like this:
+    #
+    #  $a = 10, notice hello
+    #
+    # where the "10, notice" forms an argument list. The parser builds an Array with the expressions and includes
+    # the comma tokens to enable the error to be reported against the first comma.
+    #
+    begin
+      Factory.transform_calls(expressions)
+    rescue Puppet::Pops::Model::Factory::ArgsToNonCallError => e
+      # e.args[1] is the first comma token in the list
+      # e.name_expr is the function name expression
+      if e.name_expr.is_a?(Puppet::Pops::Model::QualifiedName)
+        error(e.args[1], "attempt to pass argument list to the function '#{e.name_expr.value}' which cannot be called without parentheses")
+      else
+        error(e.args[1], "illegal comma separated argument list")
+      end
+    end
   end
 
   # Transforms a LEFT followed by the result of attribute_operations, this may be a call or an invalid sequence
diff --git a/spec/unit/pops/parser/parse_calls_spec.rb b/spec/unit/pops/parser/parse_calls_spec.rb
index ee80544f5..6f75eabc8 100644
--- a/spec/unit/pops/parser/parse_calls_spec.rb
+++ b/spec/unit/pops/parser/parse_calls_spec.rb
@@ -59,11 +59,6 @@ describe "egrammar parsing function calls" do
       dump(parse("$a = foo()")).should == "(= $a (call foo))"
     end
 
-    #    # For regular grammar where a bare word can not be a "statement"
-    #    it "$a = foo bar # illegal, must have parentheses" do
-    #      expect { dump(parse("$a = foo bar"))}.to raise_error(Puppet::ParseError)
-    #    end
-
     # For egrammar where a bare word can be a "statement"
     it "$a = foo bar # illegal, must have parentheses" do
       dump(parse("$a = foo bar")).should == "(block\n  (= $a foo)\n  bar\n)"
@@ -101,4 +96,24 @@ describe "egrammar parsing function calls" do
         ].join("\n")
     end
   end
+
+  context "When parsing an illegal argument list" do
+    it "raises an error if argument list is not for a call" do
+      expect do
+        parse("$a  = 10, 3")
+      end.to raise_error(/illegal comma/)
+    end
+
+    it "raises an error if argument list is for a potential call not allowed without parentheses" do
+      expect do
+        parse("foo 10, 3")
+      end.to raise_error(/attempt to pass argument list to the function 'foo' which cannot be called without parentheses/)
+    end
+
+    it "does no raise an error for an argument list to an allowed call" do
+      expect do
+        parse("notice 10, 3")
+      end.to_not raise_error()
+    end
+  end
 end