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

This fixes problems when a user enters commas where they are not
supposed to be. As a result, an expression will be parsed as being an
argument list for an unparenthesized function call. The transformation
logic for such calls did not take one case into account; a non call
followed by an argument list. e.g:

$a = 1,10

Which resulted in a strange AST model (a literal list with an assignment
and a 10).

This commit adds error checking and raising of an exception in the
transformation which is caught by parser_support and formatted into an
error - either about an illegal comma (when the LHS cannot possibly be
a call at all (as in the above exampel), or a more elaborate
message about that what could be a function call requires parentheses.

In order to enable positioning of the error message on the first comma
in the argumet list, the comma tokens were required in the expression
list fed to the transformer. Subsequently these tokens must be filtered
out by the transformation, and passed on in the raised exception (since
the receiver would otherwise not know which token that caused the
problem (it is nested inside the stucture it passes on to be
transformed).

Unparenthesized function calls are a very bad idea...

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