(PUP-11981) Syntactically incorrect types cause nil

This commit adds logic to validate class parameters are literal and provides a
descriptive warning if parameter(s) are not literal, adds QualifiedReference
and AccessExpression as a valid literal values & updates spec tests to check
for more cases of invalid class parameters and any that broke as a result of
the changes.

The logic to validate class parameters is done in
internal_check_parameter_type_literal method where the appropriate literal
method is called on each parameter. There are different literal methods for
each literal object/type [1]. For example if the parameter is String type, then
literal_LiteralString is called. If a parameter is found to be not literal,
then a :not_literal will be thrown & warning message with the invalid class
parameter and its type will be printed.

QualifiedReference is considered literal since it is a literal reference,
however, there is a possibility it could be a faulty dangling reference that
doesn't reference anything. AccessExpresions are anything with the Access
Operator ([ ]) such as Optional[String] or Variant[Boolean, Float] [2]. Since
these are valid class parameters, AccessExpressions are also considered
literal.

The logic to add QualifiedReference & AccessExpression as valid
literal values is done in the literal_QualifiedReference &
literal_AccessExpression methods. The literal_AccessExpression works by
validating each key in the expression is literal. The
literal_QualifiedReference method looks at o.value, similar to other existing
literal methods like literal_LiteralString.

[1] http://puppet-on-the-edge.blogspot.com/2014/02/puppet-internals-polymorphic-dispatch.html
[2] https://github.com/puppetlabs/puppet-specifications/blob/master/language/expressions.md#assignment-operator

For more information on this issue, see #9140

(cherry picked from commit adbb02c)

(PUP-11981) Add test for non-literal class params in ClassInfoService spec

(cherry picked from commit 8992763)

Author: AriaXLi

lib/puppet/pops/evaluator/literal_evaluator.rb

@@ -10,9 +10,8 @@ module Evaluator
 # QualifiedName
 # Default (produced :default)
 # Regular Expression (produces ruby regular expression)
-#
-# Not considered literal
-# QualifiedReference  # i.e. File, FooBar
+# QualifiedReference
+# AccessExpresion
 #
 class LiteralEvaluator
 
@@ -66,6 +65,16 @@ def literal_LiteralRegularExpression(o)
     o.value
   end
 
+  def literal_QualifiedReference(o)
+    o.value
+  end
+
+  def literal_AccessExpression(o)
+    # to prevent parameters with [[]] like Optional[[String]]
+    throw :not_literal if o.keys.size == 1 && o.keys[0].is_a?(Model::LiteralList)
+    o.keys.map { |v| literal(v) }
+  end
+
   def literal_ConcatenatedString(o)
     # use double quoted string value if there is no interpolation
     throw :not_literal unless o.segments.size == 1 && o.segments[0].is_a?(Model::LiteralString)

lib/puppet/pops/issues.rb

@@ -206,6 +206,10 @@ def self.hard_issue(issue_code, *args, &block)
     _("The numeric parameter name '$%{name}' cannot be used (clashes with numeric match result variables)") % { name: name }
   end
 
+  ILLEGAL_NONLITERAL_PARAMETER_TYPE = issue :ILLEGAL_NONLITERAL_PARAMETER_TYPE, :name, :type_class do
+    _("The parameter '$%{name}' must be a literal type, not %{type_class}") % { name: name, type_class: label.a_an(type_class) }
+  end
+
   # In certain versions of Puppet it may be allowed to assign to a not already assigned key
   # in an array or a hash. This is an optional validation that may be turned on to prevent accidental
   # mutation.

lib/puppet/pops/validation/checker4_0.rb

@@ -468,6 +468,7 @@ def check_HostClassDefinition(o)
     internal_check_parameter_name_uniqueness(o)
     internal_check_reserved_params(o)
     internal_check_no_idem_last(o)
+    internal_check_parameter_type_literal(o)
   end
 
   def check_ResourceTypeDefinition(o)
@@ -478,6 +479,18 @@ def check_ResourceTypeDefinition(o)
     internal_check_no_idem_last(o)
   end
 
+  def internal_check_parameter_type_literal(o)
+    o.parameters.each do |p|
+      next unless p.type_expr
+
+      type = nil
+      catch :not_literal do
+        type = literal(p.type_expr)
+      end
+      acceptor.accept(Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE, p, { name: p.name, type_class: p.type_expr.class }) if type.nil?
+    end
+  end
+
   def internal_check_return_type(o)
     r = o.return_type
     internal_check_type_ref(o, r) unless r.nil?

spec/unit/info_service_spec.rb

@@ -313,6 +313,12 @@ class Borked($Herp+$Derp) {}
         CODE
         'json_unsafe.pp' => <<-CODE,
              class json_unsafe($arg1 = /.*/, $arg2 = default, $arg3 = {1 => 1}) {}
+        CODE
+        'non_literal.pp' => <<-CODE,
+            class oops(Integer[1-3] $bad_int) { }
+        CODE
+        'non_literal_2.pp' => <<-CODE,
+           class oops_2(Optional[[String]] $double_brackets) { }
           CODE
        })
     end
@@ -509,6 +515,19 @@ class json_unsafe($arg1 = /.*/, $arg2 = default, $arg3 = {1 => 1}) {}
          })
      end
 
+     it "errors with a descriptive message if non-literal class parameter is given" do
+       files = ['non_literal.pp', 'non_literal_2.pp'].map {|f| File.join(code_dir, f) }
+       result = Puppet::InfoService.classes_per_environment({'production' => files })
+       expect(result).to eq({
+        "production"=>{
+           "#{code_dir}/non_literal.pp" =>
+           {:error=> "The parameter '$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: #{code_dir}/non_literal.pp, line: 1, column: 37)"},
+           "#{code_dir}/non_literal_2.pp" =>
+           {:error=> "The parameter '$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: #{code_dir}/non_literal_2.pp, line: 1, column: 44)"}
+          } # end production env
+        })
+     end
+
     it "produces no type entry if type is not given" do
       files = ['fum.pp'].map {|f| File.join(code_dir, f) }
       result = Puppet::InfoService.classes_per_environment({'production' => files })

spec/unit/pops/evaluator/literal_evaluator_spec.rb

@@ -16,6 +16,8 @@
     '"a"'     => 'a',
     'a'       => 'a',
     'a::b'    => 'a::b',
+    'Integer[1]' => [1],
+    'File' => "file",
 
     # special values
     'default' => :default,
@@ -35,9 +37,9 @@
     expect(leval.literal(parser.parse_string('undef'))).to be_nil
   end
 
-  ['1+1', 'File', '[1,2, 1+2]', '{a=>1+1}', 'Integer[1]', '"x$y"', '"x${y}z"'].each do |source|
+  ['1+1', '[1,2, 1+2]', '{a=>1+1}', '"x$y"', '"x${y}z"', 'Integer[1-3]', 'Optional[[String]]'].each do |source|
     it "throws :not_literal for non literal expression '#{source}'" do
-      expect{leval.literal(parser.parse_string('1+1'))}.to throw_symbol(:not_literal)
+      expect{leval.literal(parser.parse_string(source))}.to throw_symbol(:not_literal)
     end
   end
 end