naomimyselfandi commented 6 months ago

FSO's event system is very powerful in many respects, but in others, it shows its age. More modern languages typically support keyword parameters as a first-class construct, as in Kotlin and C#; using a wrapper object, as in Java, Javascript, and Lua; or using a wrapper object with syntactic sugar, as in Python. While FSO supports optional parameters, they are all positional; there is simply no concept of a keyword parameter. This lack has three main consequences. The first is a fairly mild quality of life concern: for any K > 1, if an operator accepts K optional parameters, for any N ∈ (1, K], it is impossible to specify the Nth optional parameter without also specifying the N-1th. The second is more significant: many operators can accept an arbitrarily large number of arguments, and these operators cannot, in general, be extended with new functionality. Lastly, it is often non-obvious what an optional parameter means; ( is-event-true-delay "some event" 0 ( true ) ) comes to mind.

The second limitation can be seen in operators as mundane as is-destroyed-delay. Its cousin is-event-true-delay has long ago been enhanced with an optional parameter which controls whether it influences directives. This was possible since is-event-true-delay originally had a fixed arity of 2. No such relief is possible for is-destroyed-delay, nor is it possible to control whether is-destroyed-delay provides a directive value. Keyword parameters would provide a solution.

Proposed implementation

Keyword arguments would be represented using the pseudo-operator kwargs. When kwargs immediately follows an operator with keyword parameter, it is not a traditional argument, but instead a map of keyword arguments; it would not be evaluated as an expression, but instead interpreted specially. To preserve compatibility, kwargs is never required; that is, for some operator foo which supports kwargs, ( foo ... ) is strictly equivaent to ( foo ( kwargs ) ... ), where ... is any list of zero or more expressions. Internally, this would be implemented by a new function in sexp.cpp which detects the presence of kwargs and skips it if it is present.

Suppose is-destroyed-delay were enhanced with a keyword parameter, directive-controller, which indicates whether it influences the display of a directive. If a mission wishes to present a directive "Enforce node blockade" which becomes true when certain ships are destroyed and is present from mission start, it could use the following notation to do so:

( is-destroyed-delay
   ( kwargs
      "directive-controller" ( false )
   )
   3
   "Antagonist"
   "Villain"
   "Enemy"
   "Bad Guy"
   "Humphry"
)

The existing form, ( is-destroyed-delay 3 "Antagonist" "Villain" "Enemy" "Bad Guy" "Humphry" ), would be treated as though ( kwargs ) were present directly after is-destroyed-delay.

sexp.cpp would need to be enhanced with functions for interpreting keyword parameters. This functionality is entirely opt-in: an operator only supports kwargs if it needs to, and other operators do not need to change their implementation. That is, there is no general expectation that kwargs be handled; if it is present where it is not expected, it is simply reported as an error.

LuaSEXP operator definitions would be able to declare keyword parameters. Such an operator's action function would be expected to receive a table mapping keyword parameter names to values at the head of its arguments list, e.g. mn.LuaSEXPs['my-cool-operator'].Action = function(kwargs, foo, bar) ... end.

Risks

The proposed implementation has been designed to minimize risk as much as possible: no new parsing forms are required, and kwargs can be processed using extant mechanisms.

In theory, kwargs may be the name of a LuaSEXP operator. This is always a risk when introducing new operators, and the name has been chosen to minimize the chance of a collision.

Upgraded operators must be tested extensively in both their original and kwargs forms.

Alternatives

New operators could be introduced, e.g. is-destroyed-delay-silent.

Instead of the kwargs pseudo-operator, a new parse form, such as keyword-name = ( expression ), could be introduced.

Many of the use cases for keyword parameters involve boolean values, and dynamic values will likely be rare. A simple list of flags could be used instead; this would greatly simplify the implementation at the expense of some expressive power:

( is-destroyed-delay
   ( flags "dont-influence-directive" )
   3
   "Antagonist"
   "Villain"
   "Enemy"
   "Bad Guy"
   "Humphry"
)

Non-goals

Converting existing optional parameters into keyword parameters is inherently unsafe. While is-event-true-delay's optional parameter motivated this example, it must remain an optional positional parameter; while it could, in theory, be deprecated in favor of an equivalent keyword parameter, doing so would become extremely awkward should the need arise to add further optional parameters. send-message's priority parameter is even more of a non-starter.

It is also not a goal to provide sweeping upgrades to existing operators. Instead, one or two high-value upgrades will be chosen and carefully validated. Subsequent upgrades will be delivered as follow-up work.

Lastly, mandatory keyword parameters are not a goal, since for any existing operator, they would break compatibility. They may be implemented if and when a new operator will benefit from them.

Questions

Should variables and <argument> be supported as the name of a keyword argument? (Leaning towards no, unless it simplifies the implementation.)

Should kwargs be permitted as a LuaSEXP operator name? (Strongly leaning towards no.)

Should an unexpected keyword be a warning, error, or neither? (Leaning towards a warning.)

Which operators should be upgraded?

wookieejedi commented 6 months ago

I agree that Keyword parameters would really help update the style and ability to use events with a more easy to use and read set of parameters, there have been a lot of times where being able to update and tune keywords without listing and tracking every single i index variable would have been incredibly useful.

MageKing17 commented 6 months ago

Retrofitting existing operators to use keyword parameters is inherently unsafe.

As long as kwargs itself is optional (and doesn't raise any errors by not being present), I'd think existing SEXPs could be retrofitted without issue.

naomimyselfandi commented 6 months ago

As long as kwargs itself is optional (and doesn't raise any errors by not being present), I'd think existing SEXPs could be retrofitted without issue.

On reflection, my framing of that example was deficient. I've amended it for clarity; the new text is reproduced below.

Converting existing optional parameters into keyword parameters is inherently unsafe. While is-event-true-delay's optional parameter motivated this example, it must remain an optional positional parameter; while it could, in theory, be deprecated in favor of an equivalent keyword parameter, doing so would become extremely awkward should the need arise to add further optional parameters. send-message's priority parameter is even more of a non-starter.

I've also amended the beginning of the Implementation section to clarify that kwargs is optional.

MjnMixael commented 6 months ago

I like the idea of this and the overall design seems pretty good based on my work in sexp.cpp. One thing that's not addressed that may or may not prove challenging is the FRED side of things. All of it's sexp menus are contextual and, at least for FRED2 (as opposed to qtFRED which I cannot speak to yet), the code is fairly rigid. This is generally handled by large switch statements in sexp.cpp so perhaps it's just a matter of offsetting the op parameter by 1 if kwargs is present. Hard to say without a deeper look and testing.

EatThePath commented 4 months ago

a few thoughts on re-reading this now

Retrofitting

In principal I think kwargs and existing optional positionals can coexist so long as having them overlap is an error. So if you have an operator that has params (op required0 required1 optional2 optional3) ( or for brevity (r0 r1 o2 o3) then these are valid:

(op 10 20)
(op (kw  
  "r0" 10 
  "r1" 20 ) )
(op (kw  
    "o2" 30
    "o3" 40)
  10
  20)
(op (kw  
    "o3" 40)
  10
  20)

But the following would be an error:

(op (kw  
    "o2" 30)
  10
  20
  40)
(op (kw  
    "o2" 30)
  10
  20
  0
  40)

So long as the positional list ends before the position of the first named argument there isn't any ambiguity, best I can tell.

Order and side-effects

Especially if retrofitting, it may be preferable to put the kwargs structure at the bottom of the argument list rather than the top.

I believe operators are evaluated sequentially, top to bottom, depth before breadth. So if we have a tree like this

(A
  B
  (C D)
  (E))

Then each operator will execute in their alphabetical order. But if we retrofit it with keyword optionals, and then someone alters the above event to this:

(A
  (kw "e" (E))
  B
  (C D))

suddenly the E operator is getting executed before it would have been before. Things being that extremely order sensitive is probably rare but I think worth worrying about. With lua operators any manner of side effect could be going in scriptland that we can't guess at.

I believe the intent in the proposal is to not evaluate the kwarg list until each arg's proper place in the positional arguments. but I'm not in favor of that. If order does matter to an event then the user shouldn't need to know the parser that well or do any mental gymnastics to figure out what order things will happen in. Similarly I wouldn't want the kwarg list sorted in any way before execution, letting people order their kwarg list however they like and respecting that order might end up useful in niche cases. So, rather:

(A
  B
  (C D)
  (kw "e" (E)))

I think a tail position would also work well with allowing retrofitting per the above thoughts, in terms of intuitively communicating the restriction.

Misc

Having soaked my brain in too much fennel I itch to push form a different form as in the "alternatives" section that could have no collision with lua operators and FRED would know to display differently, a list-of-pairs form if you will. To keep the last example rolling, something like:

(A
  B
  (C D)
  {e (E)})

However the reasons listed in the proposal for wanting to do it as a "pseudo-operator" have merit. Just my two cents, I guess.

Answers

Should variables and be supported as the name of a keyword argument? (Leaning towards no, unless it simplifies the implementation.)

That answer seems reasonable. My gut says "allow it if it's trivial to do so, don't try to make it work if it's going to be hard", but it's a weakly held position.

Should kwargs be permitted as a LuaSEXP operator name? (Strongly leaning towards no.)

I'm not sure how the parser currently handles name collisions like this. But if it's consistent with handling of other operators, then it's reasonable to take it off the list. Or in other words: Allowing lua kwargs feels roughly as dangerous as allowing lua when, so if we guard against that by all means, if we don't then I wouldn't add new safeguard checking just for this project alone.