From 78a4b52f008545d46e2b57f8bd86dfe8cb0cba8a Mon Sep 17 00:00:00 2001 From: Aria Li Date: Tue, 19 Dec 2023 10:26:17 -0800 Subject: [PATCH] (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 https://github.com/puppetlabs/puppet/issues/9140 --- lib/puppet/pops/evaluator/literal_evaluator.rb | 15 ++++++++++++--- lib/puppet/pops/issues.rb | 4 ++++ lib/puppet/pops/validation/checker4_0.rb | 13 +++++++++++++ .../unit/pops/evaluator/literal_evaluator_spec.rb | 6 ++++-- 4 files changed, 33 insertions(+), 5 deletions(-) diff --git a/lib/puppet/pops/evaluator/literal_evaluator.rb b/lib/puppet/pops/evaluator/literal_evaluator.rb index c3da4c0c226..295ba7db5f5 100644 --- a/lib/puppet/pops/evaluator/literal_evaluator.rb +++ b/lib/puppet/pops/evaluator/literal_evaluator.rb @@ -11,9 +11,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 @@ -67,6 +66,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) diff --git a/lib/puppet/pops/issues.rb b/lib/puppet/pops/issues.rb index b6196365681..9ac8abd936e 100644 --- a/lib/puppet/pops/issues.rb +++ b/lib/puppet/pops/issues.rb @@ -207,6 +207,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. diff --git a/lib/puppet/pops/validation/checker4_0.rb b/lib/puppet/pops/validation/checker4_0.rb index eb7ac5cc778..5824fd00eca 100644 --- a/lib/puppet/pops/validation/checker4_0.rb +++ b/lib/puppet/pops/validation/checker4_0.rb @@ -471,6 +471,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) @@ -481,6 +482,18 @@ def check_ResourceTypeDefinition(o) internal_check_no_idem_last(o) end + def internal_check_parameter_type_literal(o) + type = nil + o.parameters.each do |p| + next unless p.type_expr + + 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? diff --git a/spec/unit/pops/evaluator/literal_evaluator_spec.rb b/spec/unit/pops/evaluator/literal_evaluator_spec.rb index 425fdf1d8ad..c9e8d661c9c 100644 --- a/spec/unit/pops/evaluator/literal_evaluator_spec.rb +++ b/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