Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve strategy for making event changes backward compatible (by adding function overloading) #418

Open
jsiwek opened this issue Jun 17, 2019 · 6 comments

Comments

@jsiwek
Copy link
Member

commented Jun 17, 2019

It's been common for us to change event parameters (adding, removing, changing), but that always has the effect of breaking any user scripts that implement an event handler.

One idea was making handler arguments optional and matching them by name: #262. Another I think was a more explicit event deprecation and overloading (as in function overloading) mechanism.

It would be nice for 3.0 to get a solution that gives a better migration path (user experience) than what we have now (which results in breaking people's code).

First step is to lay out concrete examples of what the migration path looks like with various proposed implementations, then compare/contrast.

@jsiwek jsiwek added this to the 3.0.0 milestone Jun 17, 2019

@jsiwek jsiwek self-assigned this Jun 17, 2019

@jsiwek jsiwek added this to Unassigned / Todo in Release 3.0.0 via automation Jun 17, 2019

@jsiwek jsiwek changed the title Improve strategy for making event changed backward compatible Improve strategy for making event changes backward compatible Jun 17, 2019

@jsiwek jsiwek moved this from Unassigned / Todo to Assigned / In Progress in Release 3.0.0 Jun 17, 2019

@jsiwek

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

Probably not worth it to re-hash the solution based on optional/named-matching of parameters. It has merit, but either the syntax for event handlers is too similar to languages that require full/positional parameter set or the historical basis for the scripting language to only do positional matching made it feel odd to introduce a change at this late of a time. Maybe still something to reconsider if ever starting a new version of the scripting language from scratch.

Here's the alternate deprecation strategy/solution based overloading:

Use-Case: Adding an event parameter

Step 0

event foo(bar: bool)

Step 1

event foo(bar: bool)
event foo(bar: bool, grault: string)

Deprecation process done.

OR

event foo(bar: bool) &deprecated
event foo(bar: bool, grault: string)

User adapts their handler upon seeing deprecation warning and we later take a Step 2 to remove the deprecated version.

Notes

There's not any real reason to introduce a deprecation here. It only creates unnecessary work for users.
Maybe if number of overloads becomes excessive, then we deprecate the under-specified ones.

Use-Case: Removing an event parameter

Step 0

event foo(bar: bool, grault: string)

Step 1

event foo(bar: bool, grault: string) &deprecated
event foo(bar: bool)

User adapts their handler on seeing deprecation warning.

Step 2

event foo(bar: bool)

Deprecation process done.

Use-Case: Changing a parameter's semantics

Step 0

event foo(wrong_semantics: bool)

Step 1

event foo(wrong_semantics: bool) &deprecated
event foo(wrong_semantics: bool, fixed_semantics: bool)

User adapts their handler on seeing deprecation warning.

Step 2

event foo(wrong_semantics: bool, fixed_semantics: bool) &deprecated
event foo(fixed_semantics: bool)

User adapts their event again.

Step 3

event foo(fixed_semantics: bool)

Deprecation process done.

Notes

Potential pitfall if user "skips a version". Maybe we can just have a policy of "don't ever do that without carefully reading release notes, or else whatever subtle problems occur are your own fault".

We could avoid the 2x user-annoyance if this could work at Step 1:

event foo(wrong_semantics: bool) &deprecated
event foo(fixed_semantics: bool)

But that requires inspecting/matching parameter names which seems like something we wanted to avoid.

Use-Case: Combos (remove + add parameters simultaneously)

Step 0

event foo(bar: addr)

Step 1

event foo(bar: addr) &deprecated
event foo(baz: string)

User adapts their handler.

Step 2

event foo(baz: string)

Deprecation process done.

Implementation / Performance

Are we generally adding an overloading mechanism to each function flavor: function, hook, and event ?

I'd say yes. Conceptually, we'd need hooks to support the same deprecation process as events, and overloading of functions is generally something that's familiar/useful to people.

We'll need to name-mangle the various function flavors.

  • At any "declaration site" (in scripts or bifs)

  • At any "call site" scripting expressions (includes various forms of
    generating events/hooks)

  • At any "implementation site" scripting expressions (function definition +
    event/hook handlers)

  • When inserting a function/event/hook ID into a Scope, we'd put both the
    unmangled name and the mangled name into the Scope. Inserting the
    unmangled name is to prevent one from re-using that name for a
    non-function variable by mistake.

  • We have various internal lookup operations that search for functions by name
    that may need to change. e.g. EventRegistry::Lookup, Scope::Lookup,
    lookup_ID. We could keep using this same interface everywhere for the
    moment and just make such lookups fail if they would be ambiguous (we
    shouldn't have any such cases at the moment). We could provide new APIs
    that map an unmangled name to all mangled names for use where it matters.

  • Our current event queuing logic looks like:

    if ( foo )
        QueueEvent(foo, arg1);

    For events that have no overloads, we can keep that the same, but for
    events that do have overloads, we can't generate EventHandlerPtr names
    that are as convenient to use (have to mangle). This may be ok, since overloading
    events isn't a common use-case, so for the few times we need it, we can do:

    vector<EventHandlerPtr> handlers = lookup_event("foo");
    
    for ( auto ehp : handlers )
        {
        if ( ! ehp ) continue;
    
        if ( ehp->FType()->Args()->NumFields() == 1 )
            QueueEvent(foo, arg1);
        else
            QueueEvent(foo, arg1, arg2);
        }

    The actual function signature inspection logic would vary depending on the
    circumstances.

    If we were to instead generally mangle arguments in order to automatically
    generate the right event(s), that possibly introduces run-time performance
    overhead. Unless maybe we found a way macro or template-metaprogram a way
    to mangle event parameter types into the associated event handler at compile
    time. I imagine our types can get a bit complex and make that difficult.

Other considerations

Don't think there's any conflicts with plugins. If someone implements an overload in a plugin, that still goes through the same mangling/resolve process as if it were not in a plugin.

Missing anything?

@0xxon

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Without going into the technical specifics - I like the way that the syntax looks for this way more - and the fact that it does allow deprecation warnings (e.g. when removing event parameters - which actually has happened a few times for me).

@rsmmr

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Me too. I like relying on relying on a combination of overloading and &deprecated.

I have a couple of thoughts on the use cases, but need to mull over that a bit more. And need to think through the implementation more, the mangling worries me a bit, that seems quite a complex change.

@rsmmr

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Couple more thoughts here:

  • Jon and I discussed the implementation more the other day. It might be possible to avoid the mangling by allowing to associate an ID with more than one function, either directly within the scope or by extending functions to become containers of several versions (like we do with bodies already, but now including the function signature). Without anything further, that would incur a performance penalty because each call would now have to find out first which version to execute. However, I believe that could often be done statically at parse time and then cached along with the call.

  • The "Changing a parameter's semantics" use case seems rate enough that I wouldn't worry much. In some cases that sequence might be fine; in others maybe we'd just rename the new event. Either way seems case by case.

@jsiwek

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

  • by extending functions to become containers of several versions (like we do with bodies already, but now including the function signature). Without anything further, that would incur a performance penalty because each call would now have to find out first which version to execute. However, I believe that could often be done statically at parse time and then cached along with the call.

Trying out this approach most appeals to me:

  • IDs continue mapping to a single function object

  • Function objects change to contain a list of overloads

  • Script-layer calls change to statically cache which function overload to use

  • Core-layer calls can go unaltered if we leave current Call() API to just select the first overload (we can't possibly have more than one at the moment for any of those).

  • Provide new core-layer API to actually be able to select an overload (e.g. by matching list of types)

  • Part of the above two points is that the EventHandler / EventHandlerPtr convenience objects we create will need to work similarly: by default they select first overload, but we also need an API call to be able to select which overload to use and have our QueueEvent calls able to use the information at the time when the function call actually occurs

  • Adjust Zeekygen documentation output. Two possibilities: (1) allow referencing overloads directly based on signature. e.g. :zeek:see:`foo(int)` or (2) no change to referencing syntax, always generate the docs such that overloads are grouped with each other, referencing the function name takes you to its full group of overloads. Option (2) is easier, as fully specifying various composite types in a function signature can get crazy real quick.

@rsmmr

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Option (2) on the docs sound good to me, plus skipping any &deprecated versions from being documented.

@jsiwek jsiwek changed the title Improve strategy for making event changes backward compatible Improve strategy for making event changes backward compatible (by adding function overloading) Jul 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.