Timur,

Thanks for reporting this issue, which version of Puppet are you running?

2.7.11 I believe I’ve seen that on 2.7.10 too

The way I needed the behaviour of passing :undef (or nil) into function is stacking classes/defines with hiera like:

class foo ( $x = hiera('value_x', undef)) {

# some code here

  class { 'foo:bar':
    x => $x
  }
}

class foo:bar ( $x = undef ) {

# do some processing with $x

}

Thus if you don’t have value_x defined in hiera you pass the undef to wrapped class.

Do you think converting undef into nil rather than into ‘’ will break much things? In ruby nil is often essentially the same as undef in puppet whereas ‘’ is not.

My understanding is that there may be a fair amount of existing code out there in the world that does things like call “split” on a function parameter, without checking first to verify that the parameter is a string. Assuming that’s the issue of concern expressed in the previous ticket, then it won’t matter whether we pass a symbol or nil; either one will break those functions.

However, from what I can see so far, I think that you are correct that the current behavior eliminates the ability to handle certain use cases like yours.

Will try to dig up a few more historical details and then update this ticket.

I discussed this with Matt Robinson, who had been involved to some degree with the original ticket. His recollection was that there were arguments for both of the two approaches—so it does not sound like there was an overwhelming argument in support of the current behavior.

It seems to me that an important consideration here is: “what is the target audience of the function definition API?” If we’re targeting developers, then I don’t think it’s unreasonable to expect authors to know that they may need to account for nil or :undef values being passed in as arguments. If we’re targeting a broader audience and making the function definition as simple as possible is the primary goal, then I can perhaps see the case for passing undefs as empty strings.

My overall take on this, though, is that there are cases in the world where there is a meaningful semantic distinction between “null” and an empty string (plenty of precedent for this in relational databases, etc.), and that our current implementation explicitly prevents users from handling that distinction. Thus I’d be inclined to mark this ticket as accepted and propose that we change the behavior… probably to passing ‘nil’.

Will leave this ticket open for comments for a while and see if there are strong feelings one way or the other.

I’m in favour of making undef behave like ruby nil in templates and functions, might be a painful transition but ultimately it means any beginners guide to Ruby actually applies to our code where right now almost every sysadmin who comes to Ruby via puppet finds it a rather painful experience.

R.I. Pienaar wrote:

I’m in favour of making undef behave like ruby nil in templates and functions, might be a painful transition but ultimately it means any beginners guide to Ruby actually applies to our code where right now almost every sysadmin who comes to Ruby via puppet finds it a rather painful experience.

This is the right answer: we shouldn’t use :undef anywhere except for our own convenience internally. It shouldn’t be visible to anyone outside of core – not custom functions, custom types, providers, or anything. We should use Ruby nil everywhere that we currently use :undef internally.

We could do a transitionary period that converts undef to empty string and generate a warning. After a period of time change the warning to an error.

I agree about the general strategy of never exposing :undef outside of our walls… so, for this specific ticket, what we are talking about is changing the current behavior where we translate undefs to ‘’ before passing them to functions; we’d pass nil instead. As far as I can tell, everyone seems in relative agreement on that point.

Do we need to introduce a transition period with a deprecation warning? It would certainly be the safest path forward if we are concerned about this change breaking a lot of things, but it would postpone the ability for users such as the author of this ticket to actually handle this type of use case. So, the sooner we are able to introduce the change, the better.

Thoughts? Daniel / R.I. do you gentlemen have any ideas as to exactly how large of an impact this change would have on the community?

I think we’d def need a transition period which would have to incude us auditing the stdlib and shipped functions for bad assumptions about input.

It’s probably a terrible idea but we could catch NilClass style exceptions and recall the function with nils as “” for some transition period? I have no idea if this is a good idea, suspect not.

Added Jeff as a watcher—Jeff, any thoughts about how this would impact stdlib?

Chris Price wrote:

Added Jeff as a watcher—Jeff, any thoughts about how this would impact stdlib?

I’m not sure other than checking the spec tests.

I’ve never seen a function that returns undef but I totally see the value nd the need to be consistent with regard to casting arguments and return values.

R.I. Pienaar wrote:

I’m in favour of making undef behave like ruby nil in templates and functions, might be a painful transition but ultimately it means any beginners guide to Ruby actually applies to our code where right now almost every sysadmin who comes to Ruby via puppet finds it a rather painful experience.

+1

A criticism I often received when training people was that “Puppet is inconsistent”. I believe how we cast things to strings in functions, particularly undef values, is a major contributor to this perception.

This is absolutely not something we should change in a minor release and ideally we’ll have a full major release where we give a deprecation warning with clear steps to take for authors of functions and types.

After we get the deprecation warning in for Telly, we should create another ticket for handling the implementation later.

Also—might want to look for other places where :undef is being special-cased; it seems like the consensus is that we should probably change all of these instances, not just the case relating to function parameters.

Chris Price wrote:

I discussed this with Matt Robinson, who had been involved to some degree with the original ticket. His recollection was that there were arguments for both of the two approaches—so it does not sound like there was an overwhelming argument in support of the current behavior.

It seems to me that an important consideration here is: “what is the target audience of the function definition API?” If we’re targeting developers, then I don’t think it’s unreasonable to expect authors to know that they may need to account for nil or :undef values being passed in as arguments. If we’re targeting a broader audience and making the function definition as simple as possible is the primary goal, then I can perhaps see the case for passing undefs as empty strings.

My overall take on this, though, is that there are cases in the world where there is a meaningful semantic distinction between “null” and an empty string (plenty of precedent for this in relational databases, etc.), and that our current implementation explicitly prevents users from handling that distinction. Thus I’d be inclined to mark this ticket as accepted and propose that we change the behavior… probably to passing ‘nil’.

Will leave this ticket open for comments for a while and see if there are strong feelings one way or the other.

I think that, semantically, nil makes more sense than the empty string; but ideally undef shouldn’t be changed. I am running into an issue with hiera similar Timur. If I pass undef into hiera as a default value, I expect to get undef out on the other side, not the empty string. If I get nil back, that is easier to test for, but I don’t think that is something I should have to test for, it’s why I’m giving a default value.

That is a reasonable concern, thanks for mentioning it.

FWIW in 3.x as it stands, the sample code behaves as Timur expected:

[eric@glitch.local ~/Sandbox/puppet-undef/manifests]% puppet apply --modulepath=/Users/eric/Sandbox 13210-undef-back-from-functions.pp                                                                                                          Error: Duplicate declaration: Notify[x] is already declared in file /Users/eric/Sandbox/puppet-undef/manifests/13210-undef-back-from-functions.pp 
 at line 2; cannot redeclare on node

I know this bug is marked as closed, but I think there is a way to fix this without redefining ‘undef’ in Puppet

See the code in https://github.com/raphink/puppet/compare/dev;function_honor_undef, which essentially does this:

• Define a new :honor_undef parameter when creating functions, which prevents :undef from being munged when passed as an argument to a function. This way, function authors can choose to manage :undef themselves instead of having it default to ‘’;
• Use :honor_undef => true in the hiera() function, so ‘undef’ can be passed in Puppet as a default value.

Would that be an acceptable way to fixing this issue?

Looks ok and thank you for the idea however I can’t say right now if it would have fixed the issue as I haven’t used puppet for nearly a year now.