Object prototyping is NOT broken for higher oder functions – but WHY?

julian · March 24, 2024, 9:32am

Ok, so since we all move this way, let the thread cover this as well (btw. @jamshark70 thanks for chiming in! I just tried to keep things easier to overview).

If we support KeywordArgs as a way to pass keyword arguments to objects in messages, then this would have to be true all over the system. Would be great to have, let’s see how clearly it can be done.

Here is also a Pull Request that solves the problem “from the other side”. It is ready for discussion, too, was a lot of work, so be kind and careful :):

github.com/supercollider/supercollider

Implement `doesNotUnderstandWithKeys`

supercollider:develop ← JordanHendersonMusic:UnknownKeyWords

opened 05:13PM - 02 Mar 24 UTC

JordanHendersonMusic

+697 -68

## Purpose and Motivation Information is needlessly lost when calling doesNot…Understand 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. ```supercollider doesNotUnderstandWithKeys { |selector, withoutKeys, withKeys| DoesNotUnderstandWithKeysError(this, selector, [withoutKeys, withKeys]).throw; } ``` Fixes #6064 Here is some behaviour as it relates to identity dictionary. Old: ```supercollider 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. ```supercollider 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 ```supercollider 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 ``` ```supercollider 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. ```supercollider // 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 - [x] Code is tested - [x] All tests are passing - [x] Updated documentation - [x] This PR is ready for review

smoge · March 24, 2024, 10:54am

Undoubtedly, it is an improvement, and the documentation and comments are excellent.

The lesson is that adding redundancy increases complexity and failures, especially if they are undocumented.

jordan · March 24, 2024, 2:28pm

Just following up with a description what is changed in the interpreter for that PR…

When you call

\obj.asdf(1, 2, a: 3)

The following byte code is generated.

0   40          PushLiteral Symbol 'obj'
1   64          PushSpecialValue 1
2   65          PushSpecialValue 2
3   41          PushLiteral Symbol 'a'
4   2C 03       PushInt 3
6   B0          TailCallReturnFromFunction
7   0A 05 01 00 SendMsgX 'asdf'
11  F2          BlockReturn

In Line 7 0A 05 01 00:

0A means SendMsgX, which is send message with keyword arguments
05 is how many things are on the stack ('obj', 1, 2, 'a' & 3).
the next value 01 tells us how may keyword arguments there are. \obj.asdf(1, 2, a: 3, b: 4) produces 0A 07 02 00, for the 2 keyword args.
I don’t know what the final 00 is.

That PR just turns the stack from this:

obj, 1, 2, a, 3

into this

obj, [1, 2], [a, 3]

Then calls doesNotUnderstandWithKeys.

That would mean the interpreter would have to loop over all arguments to every sendMsg to see if the slot is a kind of KeywordArg, then switch behaviour, unpacking it, rather than switching in the parser. This could incur a performance cost as it would have to happen everywhere.

What happens if someone wrote this?

Synth(\x, KeywordArgs([\a: 1]), KeywordArgs([\b: 2]));

or

Synth(\x, KeywordArgs([\a: 1]), 2, KeywordArgs([\b: 2]));

Regarding expanding KeywordArgs automagically onto the stack…
I think switching on class when passing arguments will be confusing… but you could generalise this even more in a smalltalk way…

KeywordArgs {
   appendToArgumentStream{|stream| ... }
}

… which is kinda fun!

I don’t think the interpret should change. The code is old, fragile and hard to understand.

Instead, I think, sc should remove all existing ways of calling a function (all value*, perform* and the like…) and have only…

value, which would stay the same
valueWith{|args, kwargsPairs, lookupInCurrentEnvir| }
performWith{|selector, args, kwargsPairs, lookupInCurrentEnvir| }

and maybe functionPerformWith{|selector, args, kwards, lookupInCurrentEnvir| ... } for prototyping.

These would all be primitives.

This would incur a small performance cost over existing methods, but reduce the amount of C++ that needs to be maintained and, I think, it very explicit.

jamshark70 · March 24, 2024, 2:53pm

I think we could eliminate the “SendMsgX”.

There’s no way to process arguments without looping over them – when encountering a KeywordArgs, just dispatch to that part of the code. No need to overcomplicate.

Illegal, per language syntax definition. Keywords args must be in the last position, and there’s no reason to support multiple instances. (If the user does the above, 1. Runtime error for multiple KeywordArgs instances, which would be disallowed. 2. Warning because Synth *new has no a argument.)

hjh