kwargs and named argument support

ID:2541998
 
Feb 4 2020, 1:30 pm
Applies to:DM Language
Status: Open
Issue hasn't been assigned a status value.
While working on some low level frameworks I've been running into issues where you cant pass along arguments properly to allow named arguments to work when you're relaying proc calls. Like the following:

/proc/EntryPoint(...) // Trying to use named arguments with this will runtime
  for(var/thing in somelist)
    // Even if it didn't runtime above, args doesn't contain var names
    call(thing, somelist[thing])(arglist(args))


The proc called by EntryPoint can't receive named arguments because of a couple issues. Keyword arguments you attempt to use with EntryPoint() will runtime as a bad argument because there is no argument to match the name and even if EntryPoint() had the same argument names the args list doesn't contain var names.

My request then has a couple parts:

Allow named arguments for procs that use "..."

Add a new list similar to args called kwargs that uses key value pairs when it possibly can so you can include the given arg names when you use "..." or the regular var names otherwise.

For bonus points it would be *very* nice if named arguments used with arglist() were ignored if the proc doesn't have an argument named the same. This would allow for some much cleaner code in some places.

The above example after these changes would just be
/proc/EntryPoint(...)
  for(var/thing in somelist)
    call(thing, somelist[thing])(arglist(kwargs))


The workaround I'm forced to use to do this otherwise looks like this

// This is just a define so we can pretend we're making a normal proc call
#define EntryPoint(arguments...) _EntryPoint(list(arguments))
/proc/_EntryPoint(list/arguments)
  for(var/thing in somelist)
    call(thing, somelist[thing])(arglist(arguments))
Feb 4 2020, 3:29 pm
I'm really not sure I understand the use case here, and I definitely don't understand the naming choice of "kwargs".
Feb 4 2020, 8:32 pm
Do you mean you don't know what usecase would need this or do you not know what I'm trying to show in the examples?
Feb 4 2020, 10:10 pm
Mostly the former, slightly the latter, and I don't know why the name would be "kwargs".
Feb 5 2020, 5:54 am
Feb 5 2020, 7:46 am
The examples I gave are very close to my actual usecase just simplified a bit. I'll try to explain it a bit more in depth here but I'll leave out a lot of the optimizations or other changes that aren't important to explain the usecase.

I've been developing/maintaining an event system that allows code to register to "listen" to events. Any datum can send or receive these events and they're handled basically like this:

/datum/var/list/signal_registry = list()

/datum/proc/RegisterSignal(datum/listener, id, procpath)
  if(!signal_registry[id])
    signal_registry[id] = list()
  signal_registry[id][listener] = procpath

/datum/proc/SendSignal(id, ...)
  var/list/arguments = args.Copy(2)
  for(var/listener in signal_registry[id])
    call(listener, signal_registry[id][listener])(arguments)

// As an example here's a signal sent just before things move
/atom/movable/Move(atom/newloc, direction, step_x, step_y)
  // The string id can be anything it just needs to be unique
  SendSignal("move", newloc, direction, step_x, step_y)
  return ..()

// And here's some shoes that cause the wearer pain when they try to move using that signal
/obj/painful_shoes/proc/Equipped()
  wearer.RegisterSignal(src, "move", .proc/wearer_moving)

/obj/painful_shoes/proc/wearer_moving(atom/newloc, direction, step_x, step_y)
  if(newloc == wearer.loc && direction == wearer.dir && step_x = wearer.step_x && step_y == wearer.step_y)
    return // They aren't actually going anywhere
  wearer.cause_pain()


In the example above you can't use named arguments in SendSignal because you would get a runtime. Even if SendSignal had the correctly named arguments, it wouldn't work because the args list does not contain argument names.

This has been a very powerful system for us in ss13 but I'd like to make it easier to use when we're passing arguments around to things that have very large amounts of arguments they could take. For signals in particular this isn't so big a deal but we have datums called components which are basically chunks of behavior that can be attached to any datum in the game to give them that behavior. A kind of composition instead of inheritance basically. For example one component makes things slippery when applied to anything from turfs to mobs without us having to write this behavior into the /atom type. Lots of these take a ton of arguments and we can't use named arguments due to the issues illustrated above.

For further details on the usecase I wrote an introductory guide directed at ss13 devs here https://tgstation13.org/phpBB/viewtopic.php?f=5&t=22674
Feb 5 2020, 9:00 am
I still don't see what the point would be in having a separate var like kwargs, only I wouldn't call it that under any circumstances.

It sounds to me like what you're really asking is for a way for named args not to have to be validated in some procs, which is done at runtime before the proc executes.
Feb 5 2020, 10:38 am
As I've said, I need something like args that has the argument names as the key. The validation thing would be nice but isn't required.

Edit: Yeah it needs to not be validated when the proc is designed to take variable amounts of arguments as is usually indicated by "..." so this is partly a request for that syntax to actually have function.
Feb 7 2020, 6:30 pm
Just curious: what's to stop you from sending a dictionary of values instead of 20 args?
Feb 7 2020, 10:53 pm
In response to Major Falcon
Major Falcon wrote:
Just curious: what's to stop you from sending a dictionary of values instead of 20 args?

It looks less pretty. Compare:
SendSignal("change", list("from" = 1, "to" = 2))
SendSignal("change", from = 1, to = 2)
Feb 9 2020, 2:38 am
Also if you want to access those values from the middleman proc you have to access the list instead of being able to use proper vars. Making a new list every time you call the proc also isn't doing the code any favors. It's all around not great and I'd really rather not have to do the workaround I wrote above.
Feb 9 2020, 12:50 pm
I really dig the system you're detailing.
It's like a simplified ECS. It would be sweet to have that implemented in BYOND itself.

I am not suggesting that your needs shouldn't be met, just merely trying to understand your issue and perhaps help introduce workarounds because, intuitively (AKA I have no idea), what you're asking for seems like an incredibly fundamental change to how I imagine DM works, and you may want a solution in the interim.

Also, IIRC, arglist isn't the fastest proc, but I am not sure how it compares to the performance of lists in general.

You could implement a new dictionary datatype with a builder that recycles or grows the pool as necessitated. Or you could specialize signal datums with the appropriate information (of course, that defeats the purpose of composition to a degree but you could only have so many variants in true ECS fashion, right?)

I do find it funny that ... exists without true utilitarian purpose.
Feb 9 2020, 7:29 pm
As I've described, the system already exists and we have a workaround. This feature request is to make it not be so hacky and hopefully also get a bit of a performance improvement over making the list ourselves.

The system works nicely and is used extensively in some codebases of ss13 already. If you're curious about using it, it can easily be taken out and used elsewhere
Feb 9 2020, 10:52 pm
I used the same kind of workaround with the now deprecated/obsolete client.winsetlist() (the built-in winset() can now directly accept an associative list). As explained in the comments below, using the workaround is still awkward, since you have to include backslashes when writing the arguments on multiple lines.

Procs such as this, which always take an associative list as an argument, would benefit from the feature being discussed here. The idea is that it would sometimes be nice to imply in the proc definition that the args be a non-validated associative list, so that we don't have to actually write list() or a series of backslashes with every proc call. It would make the code much cleaner and easier to understand. This could take the form of an easy-to-use operator like "...", a new proc setting (using the "set" keyword), or a combination of both.
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//
//  winsetlist (client)
//
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
    /*
    Format:
        client.winsetlist("control_id1.param1" = "value1", "control_id2.param2" = "value2",...)

    Args:
        Arbitrary number of control parameters, including the ids, and the associated values to be set.

    Sets parameters for the src client's skin. The parameters can be written directly in associative list() format,
    or passed as an actual associative list.

////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

    This can also be written on separate lines as:

    client.winsetlist(
    "control_id1.param1" = "value1",\
    "control_id2.param2" = "value2"\
    )

    Doing it this way requires a backslash after each argument line,
    due to limitation of preprocessor macros that confines them to parsing only a single line.
    Placing a backslash before a new line escapes that line, and hides it from the compiler, so that we can get away with it.
    This is an example of how a text macro can be used outside of a text string.

////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

    Alternatively, you can simply pass a list:

    var/list/newparams = list(
    "control_id1.param1" = "value1",
    "control_id2.param2" = "value2"
    )

    client.winsetlist(newparams)

    Since we are using the list() proc directly, backslashes are not required.
    Unlike preprocessor macros, procs will recognize new lines as whitespace.
    */
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

client/proc/__winsetlist()
    if(length(args) && istype(args[1], /list))
        if(length(args[1]) && istype(args[1][1], /list))
            winset(src, null, list2params(args[1][1]))
        else
            winset(src, null, list2params(args[1]))

#define winsetlist(args...) __winsetlist(list(##args))
// This allows us to format our arguments in the same way that an associative list is formatted in list().
// To put it simply, it allows us to avoid having to actually write list() as the argument to the procedure, because it's already built in.
Feb 11 2020, 3:39 pm
Thinking about this today. I thought you could achieve this effect if you simply named all of the args you could be expecting in SendSignal(). Then another issue arose:

In order for SendSignal() to provide the same args to wearer_moving(), you would need another proc to replace args.Copy() or some other way to refer wearer_moving() to the rest of the args submitted through SendSignal().

I am thinking you might be better off with, instead of the lists or dictionaries, if you had a datum with all the kinds of vars you could be expecting for any given signal. BYOND doesn't store extra data unless it has changed from the default value so memory is not a concern there.
Feb 12 2020, 6:37 pm
SendSignal can't know what arguments will be going through it because it's used for all signals with a variety of different arguments. Only the sender and the receiver know which signal and thus which arguments will be going through.

Creating a datum is far too expensive for the usecase and is also extremely unwieldy. I've not put the optimized version of the code here since the optimized version is not as readable, a lot of profiling has gone into the system though. Lists are not used for signals at the moment either because they would be too expensive as well, so currently they just don't support named arguments at all. Nor can they reasonably unless this feature request is implemented and it's performant enough to use.

The key thing to understand about this system, and the SendSignal proc in particular, is just how generic it is and how often it gets called. Every single time anything at all moves 3-5 signals get sent off. This is manageable due to just how extreme the optimizations are. All sorts of other actions send off signals so depending on the amount of players you can easily get thousands of signals a second. *Every* one of these signals go through this one proc and every different signal source can have different arguments.

As bad as that sounds for performance, there have been a lot of optimizations that have gone into this to make signals as free as possible. Creating a datum, a list, anything, every time you send a signal would be a massive step back on that. If anything this system has overall improved performance due to it being much much easier to implement code that reacts to changes rather than constantly checking if changes have happened.
Feb 13 2020, 9:17 am
Copy. Do you think you can elaborate a little more about these optimizations just to satiate my curiosity? Is it some kind of datum pool? Did you say the code was available somewhere for reading? My interest is piqued.
Feb 13 2020, 11:03 am
I've sent you a message in the pager, this isn't the best place to continue this conversation