From dec8ae4dce52b8f2a09dae3d2744ef08fa715173 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Tue, 27 Feb 2024 16:06:24 -0800 Subject: [PATCH 1/3] Accept UnaryMinusExpression as class parameter type Previously, class parameters of the form `Integer[-1] $param` would fail compilation, because the value `-1` was lexed as a UnaryMinusExpression containing a LiteralInteger. And since the LiteralEvaluator didn't implement the `literal_UnaryMinusExpression` method, the visitor called `literal_XXX` for each ancestor class, until reaching `literal_Object`, which always raises. This adds the `literal_UnaryMinusExpression` method and returns -1 times the expression it wraps. This is similar to how the TypeParser interprets UnaryMinusExpressions[1]. [1] https://github.com/puppetlabs/puppet/blob/8.5.0/lib/puppet/pops/types/type_parser.rb#L161 --- lib/puppet/pops/evaluator/literal_evaluator.rb | 4 ++++ spec/unit/pops/evaluator/literal_evaluator_spec.rb | 12 +++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/lib/puppet/pops/evaluator/literal_evaluator.rb b/lib/puppet/pops/evaluator/literal_evaluator.rb index c56f08fd1cb..74a4f9f63d5 100644 --- a/lib/puppet/pops/evaluator/literal_evaluator.rb +++ b/lib/puppet/pops/evaluator/literal_evaluator.rb @@ -76,6 +76,10 @@ def literal_AccessExpression(o) o.keys.map { |v| literal(v) } end + def literal_UnaryMinusExpression(o) + -literal(o.expr) + 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/spec/unit/pops/evaluator/literal_evaluator_spec.rb b/spec/unit/pops/evaluator/literal_evaluator_spec.rb index c9e8d661c9c..63ee699a786 100644 --- a/spec/unit/pops/evaluator/literal_evaluator_spec.rb +++ b/spec/unit/pops/evaluator/literal_evaluator_spec.rb @@ -17,6 +17,7 @@ 'a' => 'a', 'a::b' => 'a::b', 'Integer[1]' => [1], + 'Integer[-1]' => [-1], 'File' => "file", # special values @@ -37,7 +38,16 @@ expect(leval.literal(parser.parse_string('undef'))).to be_nil end - ['1+1', '[1,2, 1+2]', '{a=>1+1}', '"x$y"', '"x${y}z"', 'Integer[1-3]', 'Optional[[String]]'].each do |source| + [ '', + '1+1', + '[1,2, 1+2]', + '{a=>1+1}', + '"x$y"', + '"x${y}z"', + 'Integer[1-3]', + 'Integer[-1-3]', + 'Optional[[String]]' + ].each do |source| it "throws :not_literal for non literal expression '#{source}'" do expect{leval.literal(parser.parse_string(source))}.to throw_symbol(:not_literal) end From 1737a5a93d63501ba11554d85175cb8e1fb3cd8f Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Tue, 27 Feb 2024 16:41:36 -0800 Subject: [PATCH 2/3] Allow non-literal parameter type issue to be controlled by strict If strict is off, the issue will be ignored. If strict is warning, then a warning will be reported, but compilation will continue: Warning: The parameter '$i' must be a literal type, not a Puppet::Pops::Model::AccessExpression If strict is error, then compilation will fail. --- .../pops/validation/validator_factory_4_0.rb | 2 ++ spec/unit/pops/validator/validator_spec.rb | 21 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/lib/puppet/pops/validation/validator_factory_4_0.rb b/lib/puppet/pops/validation/validator_factory_4_0.rb index d103c94b74d..7cab8348abc 100644 --- a/lib/puppet/pops/validation/validator_factory_4_0.rb +++ b/lib/puppet/pops/validation/validator_factory_4_0.rb @@ -36,6 +36,8 @@ def severity_producer p[Issues::NAME_WITH_HYPHEN] = :error p[Issues::EMPTY_RESOURCE_SPECIALIZATION] = :ignore p[Issues::CLASS_NOT_VIRTUALIZABLE] = :error + + p[Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE] = Puppet[:strict] == :off ? :ignore : Puppet[:strict] p end end diff --git a/spec/unit/pops/validator/validator_spec.rb b/spec/unit/pops/validator/validator_spec.rb index 5e54702a6db..037804ff7a7 100644 --- a/spec/unit/pops/validator/validator_spec.rb +++ b/spec/unit/pops/validator/validator_spec.rb @@ -211,6 +211,13 @@ def with_environment(environment, env_params = {}) expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION) end end + + it 'produces a warning for non-literal class parameters' do + acceptor = validate(parse('class test(Integer[2-1] $port) {}')) + expect(acceptor.warning_count).to eql(1) + expect(acceptor.error_count).to eql(0) + expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE) + end end context 'with --strict set to error' do @@ -262,6 +269,13 @@ def with_environment(environment, env_params = {}) expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION) end end + + it 'produces an error for non-literal class parameters' do + acceptor = validate(parse('class test(Integer[2-1] $port) {}')) + expect(acceptor.warning_count).to eql(0) + expect(acceptor.error_count).to eql(1) + expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE) + end end context 'with --strict set to off' do @@ -292,6 +306,13 @@ def with_environment(environment, env_params = {}) expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION) end end + + it 'does not produce an error or warning for non-literal class parameters' do + acceptor = validate(parse('class test(Integer[2-1] $port) {}')) + expect(acceptor.warning_count).to eql(0) + expect(acceptor.error_count).to eql(0) + expect(acceptor).to_not have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE) + end end context 'irrespective of --strict' do From 17ba937feb82b58191516d74ea71fc71e0c5e0d8 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Wed, 28 Feb 2024 17:17:19 -0800 Subject: [PATCH 3/3] Add additional literal evaluator tests The "undef to nil" test is special because otherwise the value nil when interpolated in the name of the test results in empty string. --- .../pops/evaluator/literal_evaluator_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/unit/pops/evaluator/literal_evaluator_spec.rb b/spec/unit/pops/evaluator/literal_evaluator_spec.rb index 63ee699a786..7bb77922c80 100644 --- a/spec/unit/pops/evaluator/literal_evaluator_spec.rb +++ b/spec/unit/pops/evaluator/literal_evaluator_spec.rb @@ -16,8 +16,26 @@ '"a"' => 'a', 'a' => 'a', 'a::b' => 'a::b', + 'Boolean[true]' => [true], 'Integer[1]' => [1], 'Integer[-1]' => [-1], + 'Integer[-5, -1]' => [-5, -1], + 'Integer[-5, 5]' => [-5, 5], + # we can't actually represent MIN_INTEGER because it's glexed as + # UnaryMinusExpression containing a positive LiteralInteger and the integer + # must be <= MAX_INTEGER + "Integer[#{Puppet::Pops::MIN_INTEGER + 1}]" => [-0x7FFFFFFFFFFFFFFF], + "Integer[0, #{Puppet::Pops::MAX_INTEGER}]" => [0, 0x7FFFFFFFFFFFFFFF], + 'Integer[0, default]' => [0, :default], + 'Integer[Infinity]' => ['infinity'], + 'Float[Infinity]' => ['infinity'], + 'Array[Integer, 1]' => ['integer', 1], + 'Hash[Integer, String, 1, 3]' => ['integer', 'string', 1, 3], + 'Regexp[/-1/]' => [/-1/], + 'Sensitive[-1]' => [-1], + 'Timespan[-5, 5]' => [-5, 5], + 'Timestamp["2012-10-10", 1]' => ['2012-10-10', 1], + 'Undef' => 'undef', 'File' => "file", # special values