Jira (PUP-9305) Built-in unique function behaves differently with undef

3 views
Skip to first unread message

Erik Hansen (JIRA)

unread,
Nov 7, 2018, 2:35:04 PM11/7/18
to puppe...@googlegroups.com
Erik Hansen created an issue
 
Puppet / Bug PUP-9305
Built-in unique function behaves differently with undef
Issue Type: Bug Bug
Assignee: Unassigned
Created: 2018/11/07 11:34 AM
Priority: Normal Normal
Reporter: Erik Hansen

The unique function introduced in PUP-7620 introduces a possible breaking change, as it behaves differently than the stdlib function when passed an undef value.

Reproduction:

With the following manifest, unique.pp:

 

$unique = unique($this_is_undef)
notice($unique)

With stdlib installed (puppet apply unique.pp):

Warning: Unknown variable: 'this_is_undef'. at /root/unique.pp:1:22
Notice: Scope(Class[main]): 
Notice: Compiled catalog for pe-2016413-master.puppetdebug.vlan in environment production in 0.19 seconds
Notice: Applied catalog in 0.33 seconds

Without stdlib installed:

Warning: Unknown variable: 'this_is_undef'. (file: /root/unique.pp, line: 1, column: 22)
Error: Evaluation Error: Error while evaluating a Function Call, 'unique' expects one of:
 (String string, Callable[String] block?)
 rejected: parameter 'string' expects a String value, got Undef
 (Hash hash, Callable[Any] block?)
 rejected: parameter 'hash' expects a Hash value, got Undef
 (Array array, Callable[Any] block?)
 rejected: parameter 'array' expects an Array value, got Undef
 (Iterable iterable, Callable[Any] block?)
 rejected: parameter 'iterable' expects an Iterable value, got Undef (file: /root/unique.pp, line: 1, column: 15) on node pe-201813-master.puppetdebug.vlan

 

Add Comment Add Comment
 
This message was sent by Atlassian JIRA (v7.7.1#77002-sha1:e75ca93)
Atlassian logo

Erik Hansen (JIRA)

unread,
Nov 7, 2018, 2:39:04 PM11/7/18
to puppe...@googlegroups.com
Erik Hansen updated an issue
Change By: Erik Hansen
The unique function introduced in PUP-7620 introduces a possible breaking change, as it behaves differently than the stdlib function when passed an undef value.
h5. Reproduction:


With the following manifest, unique.pp:

 
{code:java}
$unique = unique($this_is_undef)
notice($unique)
{code}

With stdlib installed (puppet apply unique.pp):
{code:java}
Warning: Unknown variable: 'this_is_undef'. at /root/unique.pp:1:22
Notice: Scope(Class[main]):
Notice: Compiled catalog for pe-2016413-master.puppetdebug.vlan in environment production in 0.19 seconds
Notice: Applied catalog in 0.33 seconds{code}
Without stdlib installed:
{code:java}

Warning: Unknown variable: 'this_is_undef'. (file: /root/unique.pp, line: 1, column: 22)
Error: Evaluation Error: Error while evaluating a Function Call, 'unique' expects one of:
(String string, Callable[String] block?)
rejected: parameter 'string' expects a String value, got Undef
(Hash hash, Callable[Any] block?)
rejected: parameter 'hash' expects a Hash value, got Undef
(Array array, Callable[Any] block?)
rejected: parameter 'array' expects an Array value, got Undef
(Iterable iterable, Callable[Any] block?)
rejected: parameter 'iterable' expects an Iterable value, got Undef (file: /root/unique.pp, line: 1, column: 15) on node pe-201813-master.puppetdebug.vlan{code}
 

Henrik Lindberg (JIRA)

unread,
Nov 7, 2018, 5:17:03 PM11/7/18
to puppe...@googlegroups.com
Henrik Lindberg commented on Bug PUP-9305
 
Re: Built-in unique function behaves differently with undef

We can make the unique function in core return undef when getting an undef value, although it is strange to call unique in the first place on an undef value (since something has probably gone wrong?). It works in the stdlib version since undef is given to a 3.x function as an empty string and unique constructs a new string with unique characters from that string - so the result is an empty string. It would be strange if the core unique function returned an empty string when given an undef value, but returning undef or possibly an empty array would be fine.

PUP-7620 added the core unique function for Puppet 5.0.0 so technically it is allowed to break API (even if this was unintentional).

Erik Hansen Is it ok to fix this by returning undef for undef ?

Henrik Lindberg (JIRA)

unread,
Nov 7, 2018, 5:17:04 PM11/7/18
to puppe...@googlegroups.com

Erik Hansen (JIRA)

unread,
Nov 7, 2018, 5:48:03 PM11/7/18
to puppe...@googlegroups.com
Erik Hansen commented on Bug PUP-9305
 
Re: Built-in unique function behaves differently with undef

Henrik Lindberg - undef for undef is I think what we want, since that's what the stdlib function does – unless you think this is an issue with my code that was masked by the stdlib version? 

Some added context - we originally saw this in some acceptance testing which caused a hard failure with the new function, the stdlib function would pass the test without issues.  

More context here: https://github.com/puppetlabs/puppet_metrics_dashboard/pull/11

 

Henrik Lindberg (JIRA)

unread,
Nov 8, 2018, 8:18:04 AM11/8/18
to puppe...@googlegroups.com

Erik Hansen Well stdlib version of the unique function turns undef into empty string - so would not be exactly the same thing. I looked at that PR and made two comments there.

Sean Millichamp (JIRA)

unread,
Nov 9, 2018, 6:46:02 AM11/9/18
to puppe...@googlegroups.com

Just wanted to add my $0.02: I wish more of the stdlib functions handled types more strictly than they have and I think the behavior of the core unique makes a lot more sense. If I call unique on something I expect that it will be an array. If it is not an array then I probably haven't handled something correctly earlier in the code (either a bug or failed to validate input correctly) and I would want it to raise an error.

Please don't change the core unique behavior as added in Puppet 5. It is the right direction for the language/platform.

Henrik Lindberg (JIRA)

unread,
Nov 9, 2018, 7:35:03 AM11/9/18
to puppe...@googlegroups.com

Sean Millichamp That is what I am leaning towards (not changing) since "give me a unique set of undef" is a strange thing to ask for and most likely masks some other problem if it was to return undef. We had a similar request to make empty() accept undef and return true - there we had lots of breakage of legacy code when it did not accept undef, and it seemed reasonably harmless to allow that even if you can argue that undef is indeed undefined and you cannot even know if it is something "empty or not". OTOH, undef is often referred to as "the empty set".

In this case (for unique) this ticket was filed long after 5.0.0 was released - so cannot be a common problem and is easily avoided by using type checking. You can argue that if undef is indeed "the empty set", then a unique should return that empty set - e.g. an empty array. But, if undef represents the "empty set", then it would be reasonable to return undef - but the problem with returning undef is that it should return something that is iterable (for example a String if given a String, and that works also on an empty string). So to comply with that requirement it would need to return an empty iterable (for example an empty Array).

So, my position is:

  • I don't like the idea of turning undef into an Array since I think this masks a problem
  • I don't like the idea of turning undef into undef since it will move the error to where you try to iterate over the undef result - so this goes against "catch the error early"
  • I don't like making undef iterable as if it was an empty array - I think that would just mask other errors and move the problem making it more difficult to find the cause.

If accepting something like Optional[Array] and wanting to do a unique it is easy to use conditional logic or calling then

# Three different ways to check with conditional logic
$result = if $maybe_an_array =~ Array { $maybe_an_array.unique } # results in undef if not an array
$result = $maybe_an_array ? { Array => $maybe_an_array.unique, default => undef }
$result = $maybe_an_array.then |$x| { $x.unique }

which all gives you undef for undef and the call to unique otherwise.

That is a long way of arriving at: closing this as "won't do".

Austin Boyd (JIRA)

unread,
Dec 12, 2019, 8:49:04 AM12/12/19
to puppe...@googlegroups.com
Austin Boyd updated an issue
 
Change By: Austin Boyd
Zendesk Ticket IDs: 33515
Zendesk Ticket Count: 1
Reply all
Reply to author
Forward
0 new messages