Fix given_or_derived behavior in object initialization

[seanmil]

Currently, a given_or_derived attribute kind is treated as required in both the hash and positional argument initialization. As a required argument a value must always be given, thus preventing the "derived" half ever being used. This patch changes the behavior so that given_or_derived attributes are marked as optional in the initialization hash/arguments.

[seanmil]

I spent quite a bit of time reading through the code and Henrik's series of blog posts before I came to the decision this PR was the right direction. I'd love for @hlindberg to comment as to his thoughts, if he's so inclined.

[hlindberg]

Oh dear, it was ages ago since I had all that current in my head.
Could you help me with an example of usage as it at the moment is a bit too abstract to grok - especially what any possible downsides of the change could be.

[hlindberg]

Byw, do you have access to the (very long) document about PCore?
If not I will go find it for you.

[hlindberg]

Here is the PCore design document, which was the presentation and motivation for the type system and PCore. Much of this was later turned into specs and implementation. I think the document is still a good read for getting the background and ideas behind the type system. The doc is open to anyone with the link: https://docs.google.com/document/d/1Oq4VFISrhU44nnKWsYNB_Q_OLnPnodJz67pB8d53jys/edit?tab=t.0

[seanmil]

@hlindberg sure, thanks for responding! Thanks for the link to the PCore doc - I had it, somewhere, but forgot I did. I put together a simple (but dumb) example here: https://github.com/seanmil/example_given_or_derived

Without this patch the given_or_derived argument is treated as required during initialization, which means that a conforming implementation would never be able to use the "derived" part. Given the PCore document (which is the clearest reference I've found) I believe this is the correct behavior.

Here is the output of the example I linked without the patch:

Error: Evaluation Error: Error while evaluating a Method call, The function 'new_Example' was called with arguments it does not accept. It expects one of:
  (Struct[{'int' => Integer, 'maybe_half' => Numeric}] hash)
    rejected: parameter 'hash' expects a value for key 'maybe_half'
  (Integer int, Numeric maybe_half)
    rejected: expects 2 arguments, got 1 (file: modpath/example_given_or_derived/manifests/init.pp, line: 2, column: 21)

And here it is with the patch:

Notice: Scope(Class[Example_given_or_derived]): Ex1.int: 44
Notice: Scope(Class[Example_given_or_derived]): Ex1.maybe_half: 22
Notice: Scope(Class[Example_given_or_derived]): ex2.int: 44
Notice: Scope(Class[Example_given_or_derived]): ex2.maybe_half: 10.0
Notice: Scope(Class[Example_given_or_derived]): ex3.int: 12
Notice: Scope(Class[Example_given_or_derived]): ex3.maybe_half: 6
Notice: Scope(Class[Example_given_or_derived]): ex4.int: 12
Notice: Scope(Class[Example_given_or_derived]): ex4.maybe_half: 24.3
Notice: Compiled catalog for xxxx in environment production in 0.02 seconds
Notice: Applied catalog in 0.00 seconds

Note that I just pushed an update that simplifies the fix and ensures it works with both hash and positional argument initialization (I had only considered hash initialization in the first version).

I also tinkered with a patch that would have PCore enforce that a given value was returned in the event the Object attribute's Ruby method didn't, but while it was a good further learning experience with the code it wasn't quite as elegant as I would have liked and, more importantly, I'm not sure it is necessary. I couldn't find anywhere in the spec where the PCore implementation is required to make sure the given value is returned so I think it is okay to trust the method author to return the value given (if given).

[hlindberg]

Thanks for the example! Agree that this is clearly a bug and that treating it as optional is the right thing to do.
The checking of the datatype of returned values from the derived value method would be a nice guard rail, but could get quite expensive if the datatype is complex (i.e calling the type system to check) - otoh, would not be more expensive than if the value was given.

[hlindberg]

Thought a bit more, I am not sure that "given or derived" is stated to guarantee that the given value is returned - imagine an attribute where you want it to be derived from the given value. No idea if that is very useful or if it needs to guarantee the return of a given.

[seanmil]

Thought a bit more, I am not sure that "given or derived" is stated to guarantee that the given value is returned - imagine an attribute where you want it to be derived from the given value. No idea if that is very useful or if it needs to guarantee the return of a given.

That's an excellent point - okay, I'll shelve the other branch but it sounds like this fix is good to go. Thanks so much for your review & feedback! Cheers!

[hlindberg]

Actually- after more thought, the behavior of changing a given value is like having a "setter" method for an attribute (e.g. making a string all upper case, trimming white space, etc). The "given or derived" is more of a "getter" style method.
I therefore think it is a good idea to check that a given value is indeed returned and not some other getter/setter style computed value as that would be confusing.