-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use macros to define messages rather than eval #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Always good to move away from eval
😊
I have just two minor nitpicks / questions.
access to non-existent properties does not throw errors:
...
This is necessary to ensure forward-compatibility with remote messages where new fields may be added without breaking old code.
I'm not sure I understand this correctly, but I don't think this is true? We could have message.non_existing_property
throw an error but then use some special setproperty_nothrow!(message, property, value)
in _inflate()
to allow unknown fields in incoming messages. I'm not saying message.non_existing_property
should throw (that's a discussion to be had elsewhere), but I just wanted to make sure we're not dismissing this option for the wrong reasons.
Sure, yes, we could limit the non-throwing behavior to just inflation, and still throw for use in direct use of the property. I am happy with that option. |
@ettersi updated code so that non-existent property access throws errors unless a keyword argument |
I think we can avoid a lot of the scoping complications by not blanket-escaping the entire macro return value. I can have a look at this tomorrow. |
Agree. And I tried doing it several times and failed. The problem is that the macro call for The current version assumes that |
The right place to fix this is base Julia. Couple of things to think through to decide if we want a temporary fix here ... see my comments on #46. |
All ready to merge now, having addressed all of @ettersi's comments. Pending @notthetup approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry 🙈
What does this PR do?
@message
macroMessageClass()
andAbstractMessageClass()
API (BREAKING!!!)Performative.AGREE
, etc, this is non-breaking)msgID
andperf
deprecated properties in favor of the officialmessageID
andperformative
properties (non-breaking provided users avoided the deprecated properties)What was the problem?
The
MessageClass()
based API defines new data types dynamically. These data types are defined througheval
. During package compilation data types in the module are cached, and soeval
into the module later can break it. While we were careful to only dynamically add types to non-cached project modules, this scales poorly and prevents pre-compilation of downstream packages.The message types defined using
MessageClass()
were dynamic, i.e., properties were dynamically added to them. This prevented any type inference on the messages and hence required dynamic dispatch on downstream code using the messages, making that code non-performant.What is the solution?
The solution is to allow concretely typed structs as messages, and to be able to define them statically in a way compatible with pre-compilation.
We now define messages with a macro:
All message structs are mutable. The
@message
macro also automatically adds a few fields to the struct:performative::Symbol
messageID::String
inReplyTo::String
sender::AgentID
recipient::AgentID
sentAt::Int64
Messages can be constructed easily:
and properties can be accessed as per normal:
Access to non-existent properties throw errors unless
ignore_missingfields=true
when setting the property. This is used in inflation to ensure forward-compatibility with remote messages where new fields may be added without breaking old code.We can also define abstract message types that can be used as base types for other messages:
Java allows concrete messages to be extended. To enable the same functionality in Julia, we allow concrete message to be defined with the same name as an abstract base class:
Performatives are guessed automatically based on message classname. By default, the performative is
Performative.INFORM
. If a message classname ends with aReq
, the default performative changes toPerformative.REQUEST
. Performatives may be overridden at declaration or at construction (and are mutable):Finally, it is always possible that we receive a message from a remote container with a message class that we do not have a definition for. These are handled by generating a parametric
GenericMessage
that can contain any properties:If no classname is specified,
GenericMessage
defaults to the JavaGenericMessage
class:Reminder
This isn't new, but easy to forget. Any package that defines strongly typed messages should call
registermessages()
during package initialization:This ensures that the messages are registered with
Fjage.jl
, and can be constructed on demand when the classname is encountered in JSON coming in from a remote container. Without this registration, aGenericMessage
with that classname will be constructed instread of the correct message type.