Jira (PUP-11066) optional_param does not set Integer to optional for custom ruby functions

18 views
Skip to first unread message

Lucy Wyman (Jira)

unread,
May 14, 2021, 2:44:02 PM5/14/21
to puppe...@googlegroups.com
Lucy Wyman created an issue
 
Puppet / Task PUP-11066
optional_param does not set Integer to optional for custom ruby functions
Issue Type: Task Task
Assignee: Unassigned
Created: 2021/05/14 11:43 AM
Priority: Normal Normal
Reporter: Lucy Wyman

With a custom ruby function with the following definition, passing `nil` to Timeout errors:
code
Puppet::Functions.create_function(:wait, Puppet::Functions::InternalFunction) do
dispatch :wait do
param 'Variant[Future, Array[Future]]', :futures
optional_param 'Integer[1]', :timeout
optional_param 'Hash[String[1], Any]', :options
return_type 'Array[Boltlib::PlanResult]'
end

def wait(futures, timeout = nil, options = {})
code
Inspecting the types that Puppet expects for the function indicates that the timeout is `PIntegerType`, not optional.

With the following definition, the function has the correct signature:
code
Puppet::Functions.create_function(:wait, Puppet::Functions::InternalFunction) do
dispatch :wait do
param 'Variant[Future, Array[Future]]', :futures
optional_param 'Optional[Integer[1]]', :timeout
optional_param 'Hash[String[1], Any]', :options
return_type 'Array[Boltlib::PlanResult]'
end

def wait(futures, timeout = nil, options = {})
code
Add Comment Add Comment
 
This message was sent by Atlassian Jira (v8.13.2#813002-sha1:c495a97)
Atlassian logo

Mihai Buzgau (Jira)

unread,
May 18, 2021, 10:48:02 AM5/18/21
to puppe...@googlegroups.com
Mihai Buzgau updated an issue
Change By: Mihai Buzgau

Mihai Buzgau (Jira)

unread,
May 18, 2021, 10:54:02 AM5/18/21
to puppe...@googlegroups.com
Mihai Buzgau updated an issue
Change By: Mihai Buzgau
Sprint: Triaged tickets

Mihai Buzgau (Jira)

unread,
May 18, 2021, 10:55:02 AM5/18/21
to puppe...@googlegroups.com
Mihai Buzgau updated an issue
Change By: Mihai Buzgau
Team: Night's Watch

Lucy Wyman (Jira)

unread,
May 18, 2021, 12:36:02 PM5/18/21
to puppe...@googlegroups.com
Lucy Wyman commented on Task PUP-11066
 
Re: optional_param does not set Integer to optional for custom ruby functions

Definitely not a blocker since we can just use `optional_param 'Optional[Integer]'`, works just as expected. No rush on our end, just didn't want to forget to make a ticket for it.

Ciprian Badescu (Jira)

unread,
Sep 7, 2021, 4:27:02 AM9/7/21
to puppe...@googlegroups.com
Ciprian Badescu updated an issue
 
Change By: Ciprian Badescu
Sprint: Triaged tickets

Josh Cooper (Jira)

unread,
Sep 28, 2021, 12:57:02 AM9/28/21
to puppe...@googlegroups.com
Josh Cooper commented on Task PUP-11066
 
Re: optional_param does not set Integer to optional for custom ruby functions

I think this is a case where 'optional' means different things in optional_param and Optional[T]. The former marks the parameter as an optional positional argument, while the latter is the type specification for the parameter, and Optional[T] will match either T or surprisingly Undef. See https://puppet.com/docs/puppet/7/lang_data_undef.html and https://tickets.puppetlabs.com/browse/PUP-8101?focusedCommentId=502043&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-502043

So if I have a wait function:

Puppet::Functions.create_function(:wait, Puppet::Functions::InternalFunction) do
 
  dispatch :wait do
 
    optional_param 'Integer[1]', :timeout
 
  end
 
 
 
  def wait(timeout = nil)
 
    puts "timeout #{timeout.inspect}"
 
  end
 
end

Then I can omit the timeout parameter due to optional_param:

$ bx puppet apply -e 'wait()'     
timeout nil

But passing nil as an explicit timeout parameter won't work, because it's not an instance of Integer:

$ bx puppet apply -e 'wait($nada)'  
Warning: Unknown variable: 'nada'. (line: 1, column: 6)
 
Error: Evaluation Error: Error while evaluating a Function Call, 'wait' parameter 'timeout' expects an Integer value, got Undef (line: 1, column: 1) on node localhost

That's a little surprising given we defaulted the function signature to "timeout = nil".

As mentioned earlier, changing the function dispatch to Optional[Integer[1]] works because Optional[T] matches Undef:

$ bx puppet apply -e 'wait($nada)'
 
Warning: Unknown variable: 'nada'. (line: 1, column: 6)
 
timeout nil

As a module author I can imagine the current behavior is surprising. In theory, the optional_param method could automatically wrap the type specification:

    def optional_param(type, name)
 
      internal_param("Optional[#{type}]", name)
 
      @max += 1
 
    end

That way passing undef behaves the same as omitting the parameter. I assume double wrapping as Optional is not an issue? There might be other reasons why that'd be a bad idea...

Lucy Voigt (Jira)

unread,
Oct 11, 2021, 6:36:02 PM10/11/21
to puppe...@googlegroups.com
Lucy Voigt commented on Task PUP-11066

That makes total sense Josh. Should I just close this issue?

john (Jira)

unread,
May 10, 2023, 8:44:03 AM5/10/23
to puppe...@googlegroups.com
john commented on Task PUP-11066

Ciprian Badescu can yuo provide more information why this bug was closed as wont fix. it seems like the last update from Josh Cooper proposed a solution that could work i.e. 

def optional_param(type, name)
 
      internal_param(Puppet::Pops::Types::TypeFactory.optional(type), name)
 
      @max += 1
 
    end

however i dont see any reason why that solution and this bug was rejected
This message was sent by Atlassian Jira (v8.20.11#820011-sha1:0629dd8)
Atlassian logo

Henrik Lindberg (Jira)

unread,
May 15, 2023, 4:39:02 AM5/15/23
to puppe...@googlegroups.com

The proposal to make all optional_parameter wrap the type in Optional is bad because it would break the contract of the function API and possibly deliver undef values to functions that are not implemented to handle that.

 

As Josh pointed out, it is easy to get confused because "optionality" differs depending on context. I regret that we named the "also accepts undef" data type Optional, we were debating several names like Nullable[T], Undefable[T], and Maybe[T]. We also discussed if Undef[T] made sense or not. Unfortunately, it is way too late to make such a change.
Reply all
Reply to author
Forward
0 new messages