Implement doesNotUnderstandWithKeys

[JordanHendersonMusic]

Purpose and Motivation

Information is needlessly lost when calling doesNotUnderstand when there are keywords args.

Now, when a selector isn't understood and there are keysword args in the call, the following method is called, here is Objects implementation.

doesNotUnderstandWithKeys { |selector, withoutKeys, withKeys|
    DoesNotUnderstandWithKeysError(this, selector, [withoutKeys, withKeys]).throw;
}

Fixes #6064

Here is some behaviour as it relates to identity dictionary.

Old:

e = (
   \speak : {|self, words, reason| postf("I said % because %\n", words, reason) }
);

e.speak("meow", reason: "a big dog barked")
// WARNING: keyword arg 'reason' not found in call to IdentityDictionary:doesNotUnderstand
// I said meow because nil

New.

e = (
   \speak : {|self, words, reason| postf("I said % because %\n", words, reason) }
);

e.speak("meow", reason: "a big dog barked")
// -> I said meow because a big dog barked

I admit, nil is probably a better reason to meow.

More examples

e = (
   \speak : {|self, words, reason| postf("I said % because %\n", words, reason) }
);

e.speak("meow", "ahahah", reason: "a big dog barked")
// ERROR: Key reason already added

e = (
   \speak : {|self, words, link, reason| postf("I said % % %\n", words, link, reason) }
);

e.speak("meow", link: "because", reason: "a big dog barked")
// -> I said meow because a big dog barked

e.speak("meow", "because", reason: "a big dog barked")
// -> I said meow because a big dog barked

e.speak("meow", reason: "a big dog barked")
// -> I said meow nil a big dog barked

e.speak("meow", asdf: 234, reason: "a big dog barked"); 
// throws
//  Key 'asdf' not found in function call

e.speak(reason: "a big dog barked");
// -> I said nil because a big dog barked

Also provides some helper methods in Object to make redirecting easier.
These are used to call functions, class methods and methods respectively.

// private impl prDoesNotUnderstandWithKeysHelper { |callingFunc, expectedArgNames, withoutArray, withEvent| ...}
doesNotUnderstandWithKeysFunctionHelper { |func, withoutArray, withEvent| ...}
doesNotUnderstandWithKeysClassMethodHelper { |class, methodName, withoutArray, withEvent| ...}
doesNotUnderstandWithKeysMethodHelper { |class, this_object, methodName, withoutArray, withEvent| ...}
doesNotUnderstandWithKeys { |selector, withoutKeys, withKeys|...}

Types of changes

• Documentation
• Bug fix
• New feature
• Breaking change ... think this unlikely, but if the user depended on an error being thrown it could break something.

To-do list

• Code is tested
• All tests are passing
• Updated documentation
• This PR is ready for review

[JordanHendersonMusic]

I've added implementation for IdentityDictionary, Scale and Tuning. Most classes should be able to support this, Maybe looks a little more complex though. One open question is whether the \forward mechanism of IdentityDictionary makes sense with keyword args? For now, I have opted to not do the forwarding if there are keyword args — I'd appreciate any thoughts on this.

[JordanHendersonMusic]

All documentation is now updated, and I have not been able to find any issues, this is now ready for a review.

[telephon]

This is a very very useful addition, thanks. Currently, for me, the build is broken (due to a #6224), so I can't test it. But I am in favour of adding this feature.

[telephon]

In terms of naming and documentaton, this could be improved a little.

doesNotUnderstandWithKeysClassMethodHelper

• What does this method actually do?
• Maybe there is a simpler name?

withoutArray argument: in the method call of doesNotUnderstandWithKeys this is called withOutKeys, isn't it?

And withEvent is called withKeys, right? This makes things harder to understand than necessary.

Also, I would write without and not withOut, because this is just one word.

This method will be very useful for object prototyping and calling pseudo-object code (envirs) with keyword arguments.

[JordanHendersonMusic]

Once you have the two types of arguments (without and with keywords) it is quite hard to do anything with them as there isn't any obvious way of combining them and calling a function. Perhaps keyword arguments were added later leading to an odd interface?

In this recent commit I've added some more methods to FunctionDef to centralise the error checking, ensuring all keywords are accepted, and none of the arguments collide with each other. Having each class the uses doesNotUnderstandWithKeys deploy their own error checking would lead to inconsistent behaviour.

There are then new versions of perform... and value... accepting the mixture of args and kw-args,
and a version of functionPerformList that accepts both types of arguments, which is needed for object prototyping.

I admit that having so many ways to call a function is verbose, but this approach seems the most consistent with supercollider's code base.

Here are some examples of how this all works together taken from the commit, which I think, provides a decent interface for anyone wanting to use this method.

// From Scale, calls a class method
*doesNotUnderstandWithKeys {|selector, argsArray, keywordArgsAsPairs|
   ^Scale.class
   .findRespondingMethodFor(\newFromKey)
   .evaluateWithArgsAndKwArgs([selector] ++ argsArray, keywordArgsAsPairs.asEvent, Scale) 
   // last argument is 'this'
}

// From IdentityDictionary
doesNotUnderstandWithKeys {|selector, argsArray, keywordArgsAsPairs|
   if(know.not){
      ^this.superPerformList(\doesNotUnderstandWithKeys, selector, argsArray, keywordArgsAsPairs)
   };

   this[selector] !? {|f|
      ^f.functionPerformWithArgsAndKwArgs(\value, [this] ++ argsArray, keywordArgsAsPairs.asEvent)
   };

   this[\forward] !? {|f|
      ^f.functionPerformWithArgsAndKwArgs(\value, [this, selector] ++ argsArray, keywordArgsAsPairs.asEvent)
   };
   ^nil
}

There is also series of tests that pass.

[telephon]

great! Needs a few tweaks, but looks good already.

[telephon]

Maybe it helps to take a look at performWithEnvir, this seems similar.

[JordanHendersonMusic]

Thanks for your feedback @telephon, I've addressed the smaller issues in this last commit (and resolved the comments), the larger ones I will try and get to tomorrow.

[telephon]

Perhaps keyword arguments were added later leading to an odd interface?
Yes, they were, but I think the interface is optimised for speed. Keyword arguments are slower than normal ones.

Once you have the two types of arguments (without and with keywords) it is quite hard to do anything with them as there isn't any obvious way of combining them and calling a function.

It would be easy to do one, this is how I would go about it. Ellipsis arguments would need to be taken care of still.

(
f = { |func, args, keywordArgDict|
	var callArgs = func.defaultArgs.copy.extend(func.argNames.size, nil);
	callArgs.overWrite(args);
	func.argNames.do { |name, i|
		var val = keywordArgDict[name];
		if(val.notNil) { callArgs[i] = val }
	};
	func.valueArray(callArgs)
}
)


f.({ |x, y = 0, freq, amp = 0.2| [x, y, freq, amp] }, [9], (freq:500, amp: 0.2))
	

[JordanHendersonMusic]

I think I've address the points you have raised now.
Now IdentityDictionary looks like this...

if(know.not){
   ^this.superPerformList(\doesNotUnderstandWithKeys, selector, argsArray, keywordArgsAsPairs )
};

this[selector] !? {|f|
   ^f.functionPerformList(
      \value,
      f.def.makePerformableArray([this] ++ argsArray, keywordArgsAsPairs.asEvent)
   )
};

// Unlike in doesNotUnderstand, we don't convert to a setter as this isn't
// possible with keyword args.

// If the keyword arg names don't match this will fail.
// Note how the selector is passed in here.
this[\forward] !? {|f|
   ^f.functionPerformList(
      \value,
      f.def.makePerformableArray([this, selector] ++ argsArray, keywordArgsAsPairs.asEvent)
   )
};

^nil

[telephon]

Here a few more suggestions. It is fun, I hope it is ok that there are so many requests. It is a deeper change, so we need to make it really as good as possible.

[JordanHendersonMusic]

I've just pushed a commit that addressed your comments on the code, I think it look much better with your suggestions @telephon. I haven't finished looking at the docs yet, as I'll wait to see what happens with the other conversation before making progress there.

[telephon]

Many improvments thank you so much, I am sorry to have a few "backward" suggestions …

[JordanHendersonMusic]

I just found another serious bug related to the way variable arguments are expanded while making perform/value - With, this time in valueArray.

For some testing function...

f = { |a, b, c, d ...e| 
   var i = 0;
   var j = 1;
	
   a.debug(\a);
   b.debug(\b);
   c.debug(\c);
   d.debug(\d);
   e.debug(\e);
};

This works as expected.

f.valueArray(\a, \b, \c, \d, [\e, \f, \g])

a: a
b: b
c: c
d: d
e: [e, f, g]

This does not.

a = [\a, \b, \c, \d, [\e, \f, \g]]
f.valueArray(a)

a: a
b: b
c: c
d: d
e: [[e, f, g]] <<<< double wrapped.

valueArray is supposed to work by taking the last item on the stack and expanding it, which it does!
However, when called with an actually array, the total number of items on the stack is not the array's SIZE, but its CAPACITY.
The solution is to alway call valueArray like this...

a = [\a, \b, \c, \d, [\e, \f, \g]]
f.valueArray(*a)  <<< teeny little star fixes problem

a: a
b: b
c: c
d: d
e: [e, f, g] <<<< correct

... making it quite brittle.

This is quite a big change as many (but not all) use cases get this wrong, a quick grep shows about 80 calls to this method.

This will also be the case for valueArrayEnvir.

I will make a separate issue on this. [#6233]

[telephon]

I don't think it is an issue, but see my comment there.

[telephon]

don't think that is possible because it isn't called via an array (nor list) in the code, everywhere calls it like this, although the arguments suggest otherwise?!

func.functionPerformList(\value, this, args);

Hm: https://scsynth.org/t/object-prototyping-is-not-broken-for-higher-oder-functions-but-why/9171

[telephon]

I was kinda thinking something different.

How about we expand performWith and valueWith to take a third argument, an array of varArgs, and a fourth a bool, indicating whether arguments should be looked up in the currentEnvironment?

Then we could have:

* `value` — for when the user calls a function directly (this would remain untouched)

* `valueWith` — for when the 'code' calls a function

* `performWith` — for when the 'code' calls a method

* `funtionPerformWith` — for when object prototyping calls a function

All four could be implemented as primitives.

This, I think, covers all cases, meaning we can remove all other value* or perform* methods.

I know we got rid of the varArgs, but to make it more easily compatible with valueArray this might be a compromise — although I wonder if valueArray call syntax is really needed?

Compare this to what we have now; there are many possible combinations of postfixes to value and perform, valueArrayEnvir, valueArray, valueEnvir, and not all the possible combinations have been made yet — performArrayEnvir?

This would be slightly slower when calling the *With methods, as you'd need to put the arguments in an array, but vastly reduce the number of ways something can be called.

@JordanHendersonMusic should we discuss this now or shift it to a future "reform"? We can always add arguments to the methods somethingWith, so going forward with the current PR won't do harm as far as I can see.

Are there any open points we should discuss before we go through it once more for cosmetics?

[JordanHendersonMusic]

Is rather discuss that in the future as it's a larger change that isn't necessarily connected to this. I'd also need some time to play around with the idea a bit.

I think everything is settled, there hadn't really been a review of the C++ but it is okay I think.

[telephon]

Yes, I agree. So I'll go through everything and make copy editing level suggestions.

[telephon]

So this is a last copy edit, I just found one structural thing (we can drop the conversions with asDict).

The keywordArgs argument names should still be unified across the code. Currently we have: keywordArgumentEnvir, argsWithKeys, and keywordArgsAsPairs. Since no conversion needs to be done, these can now all be the same. E.g. keywordArgs.

Maybe you can add a brief comment in the help files that anything that responds correctly to keysValuesDo will do.

[JordanHendersonMusic]

Keeping the docs all synchronised is harder than writing the code...

I've renamed all the references to keywordArgumentEnvir to keywordArgumentPairs.
Also (hopefully) fixed all the docs and formatting.

[telephon]

just final cleanup.

[JordanHendersonMusic]

Would it be useful if I squashed the commits? Or is that something that gets done automatically?

[telephon]

Ideally, there should be a third person looking over it and would then squash to commit.

[lennart]

I read through the whole PR (not yet into the depths of the codebase) so I only noted potential stylistic corrections. Apart from that, as far as I can see, this PR does what it claims to do.

[adcxyz]

This will be brilliantly useful, for error messages in keyword calls,
and especially for object modeling with dictionaries (which I use a lot).
ATM, I cannot test because compiling on macOS has hurdles,
but will do ASAP.
Thanks!

[JordanHendersonMusic]

So because this is actually quite a large change. I'm going to make a new PR with just a couple of small commits in to keep history clean and to make this easier to review.

@adcxyz thanks, its something I've ran into in many places because it's needed to make proper wrapper classes!

[JordanHendersonMusic]

@telephon @adcxyz @nuss @lennart

I'm closing this PR because its a mess.
#6297 is the neat version, with only 4 commits.