[PUP-5712] TypeParser cannot handle negative numbers Created: 2016/01/14  Updated: 2016/03/17  Resolved: 2016/02/12

Status: Closed
Project: Puppet
Component/s: None
Affects Version/s: None
Fix Version/s: PUP 4.4.0

Type: Bug Priority: Normal
Reporter: Thomas Hallgren Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File negative.tar    
Template:
Story Points: 1
Sprint: Language 2016-01-27, Language 2016-02-10, Language 2016-02-24
Release Notes: Not Needed

 Description   

The Puppet::Pops::Types::TypeParser used when parsing dispatch declarations for a puppet functions written in Ruby is not capable of parsing negative numbers. It is therefore not possible to define a dispatch with a parameter type like Integer[default,-1]. The regular parser does not have this problem however so puppet apply -e 'Integer[default,-1]' executes without errors.



 Comments   
Comment by Peter Huene [ 2016/01/14 ]

For what it's worth, the native compiler parses (at least once an outstanding PR is merged) type specification strings using a "postfix expression" parser, i.e. the same postfix expression rule for the "puppet" parser. It then evaluates the resulting expression in an "empty" evaluation context (no scope, node, or catalog access), ensuring the resulting value is a type. This allows for arbitrary expressions in a type specification without side-effects, e.g. notice assert_type('Integer[[10][0], 100+10-100]', 10) results in 10 being printed. It was done this way simply because I didn't want to have to write a separate parser / evaluator from what is used normally.

I see the Ruby implementation as a subset of the native compiler's functionality, so we don't need to make it accept arbitrary expressions; could just add support for unary negation operations.

Comment by Henrik Lindberg [ 2016/01/14 ]

Peter Huene good points (and sounds like a good implementation). Sounds like the result will be a type with literal values, only that it was constructed from constexprs. That would be ok in any context. I think we need to specify more clearly what is allowed when declaring the types of parameters - I think it may have access to global scope in the Ruby impl. Say someone did this:

function foo(Enum[$::osfamily, banana] $x) { ... }
(code}
 
I don't thing that should be allowed as the static type then depends on the evaluation context of a particular node.
 
It should however be ok to create an arbitrary (free) instance of a type anywhere:
{code:puppet}
$a = Enum[$::osfamily, banana]

Comment by Henrik Lindberg [ 2016/01/14 ]

I should have added - any constexpr would be fine as long as it is never presented to the external world as anything besides literal values (not needed any computation.
Obviously something like:

Integer[1, 1+1]

is trivial to implement, but this is also a constexpr:

{1 => Integer, 2 =>String, 3 => Integer}.filter |$k, $t| { $k % 2 == 0 }.map |$k,$v|{$v}

Which, if we want an implemetation of the type system in some other language would cause a lot of extra work.

Hence, when asking for the signature of a function, the native (in fact, any implementation) must have evaluated type parameters down to literal values.

Comment by Thomas Hallgren [ 2016/01/15 ]

PR-4574 does not cover constant expression evaluation. I suggest moving that discussion to a separate JIRA.

Comment by Henrik Lindberg [ 2016/01/15 ]

PR 4574 is reviewed and it is ok to merge to stable when we get a green light.

Will move ticket to blocked as it is blocked on the 4.3.2 release.

Comment by Henrik Lindberg [ 2016/01/27 ]

merged to stable at: c1ab5a8

Comment by Henrik Lindberg [ 2016/01/27 ]

merged to master and updated the release version since it is not sure if there will be a 4.3.3

Comment by John Duarte [ 2016/02/12 ]

I am unable to evoke different behavior between versions of puppet before and after this change. Using the module attached, using a parameter of Integer[default,-1] $num = 0 results in a failure on both versions. I presume this is the result of the type not allowing the upper bound to be declared before the lower bound. Transposing these bounds Integer[-1,default] $num = 0 allows the this parameter to be used successfully in the function.

Example of interaction of Integer[-1,default]:

# cat manifests/init.pp
class negative (
  Integer[-1,default] $num = 0
) {
 
  $foo = test_negative($num)
 
  notice "Lucky number ${foo}"
 
}
 
# puppet apply -e 'class { "negative":  }'
Notice: Scope(Class[Negative]): Lucky number 0
Notice: Compiled catalog for c8kgioj3oy655f0.delivery.puppetlabs.net in environment production in 0.13 seconds
Notice: Applied catalog in 0.02 seconds
# puppet apply -e 'class { "negative": num => -1 }'
Notice: Scope(Class[Negative]): Lucky number -1
Notice: Compiled catalog for c8kgioj3oy655f0.delivery.puppetlabs.net in environment production in 0.04 seconds
Notice: Applied catalog in 0.01 seconds

Example of interaction with Integer[default,-1]

# cat manifests/init.pp
class negative (
  Integer[default,-1] $num = 0
) {
 
  $foo = test_negative($num)
 
  notice "Lucky number ${foo}"
 
}
 
# puppet apply -e 'class { "negative":  }'
Error: Evaluation Error: Error while evaluating a Resource Statement, Class[Negative]: parameter 'num' expects an Integer[default, -1] value, got Integer  at line 1:1 on node c8kgioj3oy655f0.delivery.puppetlabs.net
# puppet apply -e 'class { "negative": num => -1 }'
Notice: Scope(Class[Negative]): Lucky number -1
Notice: Compiled catalog for c8kgioj3oy655f0.delivery.puppetlabs.net in environment production in 0.04 seconds
Notice: Applied catalog in 0.02 seconds

Obviously, there is a nuance to this issue that I am not understanding.

Comment by Thomas Hallgren [ 2016/02/12 ]

It's not possible to provoke the difference using pp code since the pp parser doesn't use the TypeParser for parsing the actual type arguments. You will need to write a ruby function with a dispatcher. Such a test would just do the exact same thing as the existing unit tests though, so I'm not sure if it's worth pursuing.

Comment by John Duarte [ 2016/02/12 ]

Thanks Thomas Hallgren. The above is using a ruby function test_negative that is included in the module. Not sure what piece I am missing.

Comment by Thomas Hallgren [ 2016/02/12 ]

That ruby function is:
a) Not written to use the 4.x API
b) Does not use a 4.x dispatcher that asserts the types of the parameters.

You need to place a function under 'lib/functions' instead of under 'lib/parser/functions' and it must contain a dispatcher. So something like this:

Puppet::Functions.create_function(:test_negative) do
  dispatch :test_negative do
    param 'Integer[-1, default]', :negative
  end
 
  def test_negative(negative)
     negative
  end
end

Comment by John Duarte [ 2016/02/12 ]

Thanks Thomas Hallgren!!!

Given the proper function definition with typed parameters the previously error regarding type specification is no longer presented.

With puppet at 793c74993f24a306997f75813d819e366bb1f821 no error occurs:

root@y9bwuo9pfdnh2al:/etc/puppetlabs/code/environments/production/modules# puppet apply -e 'class { "negative": num => -1 }'
Notice: Scope(Class[Negative]): Lucky number -1
Notice: Compiled catalog for y9bwuo9pfdnh2al.delivery.puppetlabs.net in environment production in 0.05 seconds
Notice: Applied catalog in 0.23 seconds
root@y9bwuo9pfdnh2al:/etc/puppetlabs/code/environments/production/modules# puppet apply -e 'class { "negative":  }'
Notice: Scope(Class[Negative]): Lucky number 0
Notice: Compiled catalog for y9bwuo9pfdnh2al.delivery.puppetlabs.net in environment production in 0.16 seconds
Notice: Applied catalog in 0.02 seconds

Previously released version shows error:

root@c8kgioj3oy655f0:/etc/puppetlabs/code/environments/production/modules# puppet apply -e 'class { "negative": num => -1 }'
Error: Evaluation Error: Error while evaluating a Function Call, The expression <Integer[-1, default]> is not a valid type specification. at /etc/puppetlabs/code/environments/production/modules/negative/manifests/init.pp:5:10 on node c8kgioj3oy655f0.delivery.puppetlabs.net
root@c8kgioj3oy655f0:/etc/puppetlabs/code/environments/production/modules# puppet apply -e 'class { "negative":  }'
Error: Evaluation Error: Error while evaluating a Function Call, The expression <Integer[-1, default]> is not a valid type specification. at /etc/puppetlabs/code/environments/production/modules/negative/manifests/init.pp:5:10 on node c8kgioj3oy655f0.delivery.puppetlabs.net

Comment by John Duarte [ 2016/02/12 ]

Thomas Hallgren does this need any release notes?

Comment by Thomas Hallgren [ 2016/02/12 ]

No, I don't think it does.

Generated at Thu Jan 23 00:38:18 PST 2020 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.