SCLang: ClassLib: Implement doesNotUnderstandAbout preserving kwargs across doesNotUnderstand calls

[JordanHendersonMusic]

Purpose and Motivation

When calling an unknown method without keyword args the supercollider vm passes all the information back into the language allowing for transparent wrapper classes. This is not the case with keyword arguments whose data is lost.

This PR fixes that issue by implementing a new method in Object.sc doesNotUnderstandAbout {|selector, argumentsArray, keywordArgumentsPairs|..}, creating methods to make using this easier, updating the objectprototyping mechanism, and updating the rest of the class library

A huge thank you to @telephon who has been invaluable in designing the class library methods and the help documentation!

Fixes #6064.
See #6227 for the messy history!

This PR has two commits: first implement the core mechanisms, second, update the class library.

1 SCLang: ClassLib: implement doesNotUnderstandAbout.

The core mechanisms of doesNotUnderstandAbout.

• SCLang: create doesNotUnderstandAbout.
• DoesNotUnderstandError: add an additional argument, allowing the keyword pairs to be passed through.
• CPP: change the existing cpp method doesNotUnderstandAbout to call the sclang method doesNotUnderstandAbout by wrapping the non-keyword arguments and keyword arguments in separate arrays, if there are no keywords, pass nil.

When the user invokes does-not-understand behaviour, e.g., 12.someMethodNotUnderstandByInts(1, 2, foo: 3),
either the old doesNotUnderstand or doesNotUnderstandAbout are called, depending on which is further down in the class hierarchy.

This means the most concrete class has all the control, following the rules of method overriding which users expect.

Here is an example.

OldBase { 
	doesNotUnderstand { |sel ...args|
		^"called base with %".format([sel, args])
	}
}
NewChild : OldBase {
	doesNotUnderstandAbout { |sel, args, kwargs|
		^"called child with %".format([sel, args, kwargs])
	}
}

...

NewChild().plink // called child with [plink, [], nil]

NewChild().plink(1, 2) // called child with [plink, [1, 2], nil]

NewChild().plink(1, 2, boo: 3) // called child with [plink, [1, 2], [boo, 3]]

NewBase { 
	doesNotUnderstandAbout { |sel, args, kwargs|
		^"called base with %".format([sel, args, kwargs])
	}
}
OldChild : NewBase {
	doesNotUnderstand { |sel ...args|
		^"called child with %".format([sel, args])
	}
}

...

OldChild().plink // called child with [plink, []]

OldChild().plink(1, 2) // called child with [plink, [1, 2]]

OldChild().plink(1, 2, boo: 3) 
// WARNING: OldChild does not implement doesNotUnderstandAbout and keyword arguments will be dropped.
// called child with [plink, [1, 2]]

2 ClassLib: Replace doesNotUnderstand with doesNotUnderstandAbout.

Implement performWithKeys, valueWithKeys, and functionPerformWithKeys which allow an easy way to perform/value/functionPerform with both keyword arguments and normal arguments.

• This also centralises the error checking in Method/FunctionDef so that you can't call a method with overlapping arguments and keyword-arguments.

• In future, the methods in Method and FunctionDef could be turned into primitives.

Update almost all calls to doesNotUnderstand with calls to doesNotUnderstandAbout.

Types of changes

• Documentation
• Bug fix
• New feature

To-do list

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

[telephon]

But I am still thinking about how we could just do it with doesNotUnderstand. If you have an idea, drop it in!

[JordanHendersonMusic]

Okay here is the current bug, and its not nice...

T_Base {
	doesNotUnderstand { |selector ...args|
		^\base
	}
}

T_Child : T_Base {
	doesNotUnderstandAbout { |selector, argumentsArray, keywordArgumentPairs|
		^\child
	}
}

T_Child2 : T_Base {
	doesNotUnderstandAbout { |selector, argumentsArray, keywordArgumentPairs|
		this.postln; 
		^\child
	}
}

T_Child().foo(1, 2) // returns \base

T_Child().foo(1, 2, bar: 3) // segfaults
T_Child2().foo(1, 2, bar: 3) // returns \child

I've been staring at this for a few hours so will have to take a break.

[capital-G]

pythoThanks for this amazing feature and providing a very nice summary of all the changes! This will allow to build more complex structures on the fly much more easy! <3

However... if a base class has implement doesNotUnderstand, it will called and the keywords discarded.
Additionally, if a child of that base implements doesNotUnderstandAbout, then only when there are no keywords will the base class's doesNotUnderstand method be called. This is confusing, but necessary for backwards compatibility.

...

There are only a few places where doesNotUnderstand was used in the class library, this replaces the majority of them.

Could you elaborate on what would break if we "keep it simple" here?

I totally understand the motivation to keep friction low for users, but I think such a legacy-based approach will (and has?) create more headaches for future code and coders.
I think we should not go for the infamous total dictum of we do not break userspace, as a fix for outside code would be easy in that case, but the complexities of doesNotUnderstand vs doesNotUnderstandAbout would remain (and most likely indefinitely?).

Personally, I would prefer to keep future maintenance and complexity lower by doing a "clean slate" design, rather than creating detours to preserve legacy that makes the code base more and more complex over the years - especially when it operates in such a global space as Object. The class library should have enough tests to make any standard lib breakage transparent.

---

On another note, I hope this is not de-railing any efforts, has been thought of to extend the sclang syntax instead and add the Python equivalent of **kwargs? *args has already an equivalent of ..args within sclang, which catches all positional arguments which have not been matched in a list.
**kwargs does the equivalent by collecting any unmatched keywords in a dict and this seems to be the motivation of the PR IIUC. The interpreter is already aware of detecting unmatched keywords, so it should not be too hard to catch them into a dict and pass them to a syntactically declared argument (famous last words...)?

This would not just allow to tweak doesNotUnderstand without breaking any existing code (we pass all unmatched kwargs to **kwargs and if **kwargs is not declared, the excessive kwargs are simply dropped) but also to use a kwargs construction on any function or method within sclang. This is especially handy on inheritance and constructors.

IMO this would be a great addition to the sclang syntax which has not seen any updates in a long time?

[JordanHendersonMusic]

@capital-G Thanks!
Unfortunately the top description is out of date, I've updated it now!

Could you elaborate on what would break if we "keep it simple" here?

I think the current solution is the simplest, but in the past there have been two other concepts of 'simple'.

one

If you had two methods, then all class would need both doesNotUnderstand and doesNotUnderstandAbout. If these differ in any way, you'd end up with different behaviour depending on whether you had keywords or not, that would be confusing and lead to odd bugs.

two

If you could add an extra argument into doesNotUnderstand to accept the keywords, however, this is impossible with the current syntax,, variable arguments are rather problematic for future compatibility.

three — the current approach

I think the current approach, which I appreciate has changed quite a bit, is actually quite simple.

It does normal method overriding rules by walking the class hierarchy, but matches either doesNotUnderstand or doesNotUnderstandAbout, which ever comes first (when does-not-understand behaviour is triggered).

This means the class closest to the object's class has all the control.

I think this is the simplest form this PR can take while not breaking user code.

One case where this might not do what the user expects is if they override doesNotUnderstand on a class high up in the class hierarchy that is in the standard library, which later in the hierarchy, implements doesNotUnderstandAbout.... this, I hope, would be rare!?

Basically, by doing this, both doesNotUnderstand and doesNotUnderstandAbout are considered the same method during does-not-understand behaviour. This means the user should never implement both (but if they did, doesNotUnderstandAbout would be called).

The class library should have enough tests to make any standard lib breakage transparent.

It doesn't. Practically, this would need to be tested in develop.

so it should not be too hard to catch them into a dict and pass them to a syntactically declared argument (famous last words...)?

I don't think this will be possible due to how variable args are implemented as they greedily grab everything on the stack. Generally, they are very hard to reason about.

Specifically, it isn't possible to put variable args into an array and pass them to the function as, if my understanding is correct, the stack is 'reinterpreted' as an array, allowing you to read right up to the end. This means you can't place the kwargs at the end of the function signature, they'd have to go at the beginning (a breaking change), therefore, you'd have to change how variable args work, and given their complexity, would almost definitely mean breaking user code some where in very subtle complicated ways.

I do agree that this would be a better solution as it would engender the second simple solution above, but what I've done here could quite easily be adapted to support this — actually, I've already done the grouping of args and kwargs and put them on the stack, it would be trivial to adopt this in future.

[telephon]

Since I can build now, I was able to properly test stuff, runs well here and looks good. I'll make a review for some cosmetics.

[JordanHendersonMusic]

@telephon Thanks for the latest review, this has been a lot of back and forth but I think it has really improved and looks well integrated, so thanks for looking at this throughout!

I think the way the two methods are called internally is now correct and will hopefully be the least surprising? There is a clear description of this in the opening PR description. It follows on from your initial comment on this PR.

---

There are 2 things that need looking at.

1 CI passes on mac but fails on Linux.

TestNodeProxy_Server:test_synthDef_isReleased_afterFree (2.17s)
   ! Removing the  NodeProxy source function should remove SynthDef from server

I'm not that familiar with node proxy, but working on one and not the other is suspicious, any ideas?

2 What to do with functionPerfomList and functionPerformWithKeys?

Right now, only functionPerformWithKeys is called in the classlibrary and the two methods are independent, meaning if the user has overriddenfunctionPerformList, it will now do nothing.

This is a breaking change.

There are three possible solutions I can think of (in order of my preference).

a. Leave it and deprecate functionPerformList, I doubt many classes actually use this.

b. The most complete option.

Object-functionPerformWithKeys { |selector, argumentsArray, keywordArgumentPairs|
	    if(this.class.findRespondingMethodFor(\functionPerformList) != Object.findRespondingMethodFor(\functionPerformList)){
	        if(keywordArgumentPairs.isNil.not){
	            "Dropping keyword arguments in functionPerformWithKeys".warn
	        };
	        ^this.functionPerformList(selector, argumentsArray)
	    };
	    ^this
     }

c. The quick fix that implicitly drops keyword arguments.

Object-functionPerformWithKeys { |selector, argumentsArray, keywordArgumentPairs|
	    ^this.functionPerformList(selector, argumentsArray)
}

FIXED: This segfault

Details

T_Base {
	doesNotUnderstand { |selector ...args| ^\base }
}

T_Child : T_Base {
	doesNotUnderstandAbout { |selector, argumentsArray, keywordArgumentPairs|
		^\child
	}
}

T_Child2 : T_Base {
	doesNotUnderstandAbout { |selector, argumentsArray, keywordArgumentPairs|
		this.postln; 
		^\child
	}
}

T_Child().foo(1, 2) // returns \base
T_Child().foo(1, 2, bar: 3) // segfaults
T_Child2().foo(1, 2, bar: 3) // returns \child

I still haven't made any headway on this.

---

There is the open questions regarding the multitude of different ways things can be 'evaluated' or 'performed': value, valueArray, valueWithKeys, valueKeyValuePairs, valueWithEnvir valueEnvir, valueArrayEnvir, perform, performWithKeys performList ...

However, this can be discussed later, and if I get some time, I will try to make an RFC to simplify these.

[telephon]

1. This test runs fine for me here on macOS, both in isolation and in the Test series it belongs to.
2. Since functionPerformList takes the arguments as a single array without ellipsis, we could add an argument here. This is what we would have done also with doesNotUnderstand if it had been possible. It is also possible to add an argument to performList. All this would best be handled in a primitive (the conversion of the keywordArgumentPairs etc.). But I am not sure if this is too much work or too intrusive. But it would be the most compatible variant, probably.
3. I have tested this, for me the first two give a segfault, not the third. I hope this helps!

[JordanHendersonMusic]

Since functionPerformList takes the arguments as a single array without ellipsis, we could add an argument here.

The two main places where this is used in the class library, Dictionary and EnvironmentRedirct both call this method like this...

this.functionPerformList(\sel, arg1, arg2, arg3)

...rather than...

this.functionPerformList(\sel, [arg1, arg2, arg3])

Do you think it would be okay to classify the former as a bug rather than a feature then? If so, functionPerformWithKeys can be deleted.

Same thing is true for performList where there are even examples in the help files that call it like... s.performList(\sendMsg, \b_gen, buf.bufnum, \sine1, 7, a)

[JordanHendersonMusic]

@telephon I've fixed the segfault, just a typo.

It is just the second point with functionPerformList vs functionPerformWithKeys and how to deal with backward compatibility that needs addressing.

[telephon]

Same thing is true for performList where there are even examples in the help files that call it like... s.performList(\sendMsg, \b_gen, buf.bufnum, \sine1, 7, a)

Oh no! It is such a haunt. Well, then I think we really have to introduce new methods.

So the methods have to go like this, with a simplification:

makePerformableArray { |selector, argumentsArray, keywordArgumentPairs|
	var method = this.findRespondingMethodFor(seleector);
	if(method.isNil) { MethodError(selector, receiver).throw };
	^method.makePerformableArray(argumentsArray, keywordArgumentPairs)
}

performWithKeys { |selector, argumentsArray, keywordArgumentPairs|
	var argList = if(keywordArgumentPairs.isNil) { argumentsArray } { 
		this.makePerformableArray(selector, argumentsArray, keywordArgumentPairs) 
	};
	^this.performList(selector, argList)
}

functionPerformWithKeys { |argumentsArray, keywordArgumentPairs|
	var argList = if(keywordArgumentPairs.isNil) { argumentsArray } {  
		this.def.makePerformableArray(selector, argumentsArray, keywordArgumentPairs) 
	};
	^this.functionPerformList(selector, argList)
}

[JordanHendersonMusic]

So the methods have to go like this

@telephon: some thoughts on the implementations you've suggested there.

performWithKeys is essentially what you have there and I agree because it is ultimately a primitive and the user should not be implementing their version of perform. It is having the final keyword from c++ attached. This makes is safe to assume that perform is going to call the selector as a method, and therefore safe to create arguments by find the method attached to 'this' with the same name.

... but, is functionPerformList the same? Could the user add their own code in here that doesn't look up the selector on 'this'? If so, you can't create the arguments in functionPerformWithKeys and would have to duplicate the logic.

There might be something like this happening in EnvironmentRedirect with doFunctionPerformList. I'm not too sure what the code is doing there.

This is almost the same issue as figuring out which doesNotUnderstand version to call.

If functionPerformList is always guaranteed to call the selector on 'this', then we can go one step further and hide functionPerformWithKeys from the user — by either marking it as 'internal' (perhaps implFunctionPerformWithKeys?) or by simply leaving it out of the documentation. This way, functionPerformWithKeys would have one implementation in Object and no where else.

[telephon]

The motivation behind functionPerformList is to have a way to allow objects and functions to behave differently: objects just return themselves, functions are called with arguments.

• Any element of a prototype object that wants to behave like a function - in this situation - can implement it. In principle, you are right, such an object could have the purpose to collect the arguments and pass it on to some other object. Such a delegator would have to correctly implement findRespondingMethodFor, which is reasonable in a way, but might also be overly strict?

• Since we are, again, in the same situation in performWithKeys vs. performList, where the receiver could also be a delegator, it might be ok to demand that a delegator just implement performWithKeys explicitly, while all other objects implicitly pass it on to performList?

The doFunctionPerform in EnvironmentRedirect is factored out to allow its subclass ProxySpace behave differently on doesNotUnderstand. Now this one is different in so far as it finally really has args, and could be extended with an extra argument, as it should have been with all the methods we discussed.