-
Notifications
You must be signed in to change notification settings - Fork 63
Add network framework #786
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
base: main
Are you sure you want to change the base?
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.
Thanks for doing this! I had a quick look, I'll try to give a more detailed review later, but I'm a bit busy with v0.3.2 and such atm. (yeah, sorry, this won't go in that release).
I actually did a bit of work on this in the past, have pushed https://github.com/madsmtm/objc2/tree/wip-network if you want to take a look. IIRC I got stuck on the naming of things. I think I'd like the prefix NW
, e.g. nw_path_t
-> NWPath
like Swift does it, but that's already used by NetworkExtension, so we might have to do something different here.
Use of self in wrapper types
This is handled in global_analysis.rs
, I suspect it'll fix itself if you figure out the name translation stuff (it works in my branch above).
Arrays generated as ArrayUnknownABI
I'd actually rather lean towards skipping nw_ethernet_channel_send
and nw_ethernet_channel_set_receive_handler
altogether for now, arrays do have a weird ABI in function arguments, I can't recall the details but it's something like that they actually need to be passed by reference (and I'm not sure rustc
handles this correctly?).
Type-aliases for generated types
No, I'd strongly prefer that, if we expose them, that type aliases like these aren't wrapped in NWRetained
(even though that's technically incorrect, the type alias is a pointer).
nw_object!( | ||
#[doc(alias = "nw_protocol_definition_t")] | ||
pub struct ProtocolDefinition; | ||
); |
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.
I'm leaning towards modifying header-translator
to output these instead of manually emitting them. I didn't do so for the Dispatch types, but there's ~30 types using NW_OBJECT_DECL
, so here it might make more sense?
Besides, I am actually regretting it a bit for Dispatch, because we're missing the documentation comments for its types.
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.
So that means adding it as something like Stmt::NetworkDecl
(with a lot of fields copied from Stmt::OpaqueDecl
).
pub fn new_with_interface_type(interface_type: InterfaceType) -> NWRetained<Self> { | ||
nw_path_monitor_create_with_type(interface_type) | ||
} |
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.
There should be a way for header-translator
to automatically emit these as associated functions, you might need to modify the find_fn_implementor
logic in name_translation.rs
(it's kind of a mess tbh).
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.
Worth to note that in your branch, you translate the following signature:
nw_txt_record_find_key_t nw_txt_record_find_key(nw_txt_record_t txt_record, const char * key);
into:
impl nw_txt_record_find_key_t {
pub unsafe fn new(txt_record: &NWTxtRecord, key: NonNull<c_char>) -> nw_txt_record_find_key_t;
}
By looking at Swift, I think it's meant to be attached to NWTxtRecord
instead. Some methods are also outliers, i.e in this case the function and the type very much share the name.
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.
I am not entirely familiar with the header-translation pipeline. Currently it seems that many things are handled in different places but maybe that's because I am not familiar with how it works.
For example:
nw_txt_record_t nw_txt_record_copy(nw_txt_record_t txt_record);
is generated into
impl NWTxtRecord {
pub fn copy(txt_record: Option<&NWTxtRecord>) -> Option<NWRetained<NWTxtRecord>> { .. }
}
Given that we change implementer to NWTxtRecord
, I'd have expect the first argument be replaced with &self
but I am not sure where this transform should happen.
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.
OK so this can be done in global_analysis.rs
// Replace the first argument with self when translating standalone functions into instance methods
//
// nw_txt_record_t nw_txt_record_copy(nw_txt_record_t txt_record);
//
// To:
//
// impl NWTxtRecord {
// fn copy(&self) -> NWRetained<NWTxtRecord>;
// }
//
if id.library_name() == "Network"
&& arguments
.first()
.and_then(|(_, first_arg_ty)| first_arg_ty.implementable())
.is_some_and(|v| v == cf_item)
{
*first_arg_is_self = true;
}
From what I can see in the docs, when searching for Considering this, I think using the |
I think that class prefix is only relevant for Swift/Objc due semantics of how modules work in those languages. Rust does not have this problem, hence a few deprecated types from NetworkExtension that carry the same name shouldn't matter. On the other side I don't know why the prefix is even used in Rust bindings, it looks ugly. Personally I don't care all that much for prefix. Whichever way we go, my primary objective is to be able to use network framework from Rust :) |
Alright so I kinda forced header translator to produce something decent. We might need to separate Latest bindings: |
This is a boilerplate PR for adding bindings for network framework.
objc-network
nw_object
macros to create network (NW) objectsNWRetained
type. Same asRetained
but calls intonw_retain
andnw_release
.Path
andPathMonitor
types raising some questions on how we should handle pointers, bindings.Changes to header-translator
nw_
types in order to useNWRetained
instead ofRetained
NW_RETURNS_RETAINED
,NW_NOESCAPE
,NW_SWIFT_DISABLE_ASYNC
,NW_ENUM
Questions
For what it's worth, it compiles. However I have raised some questions in the code, maybe you could help me to fix the missing bits. Of course in general I'd like to know if the direction I took is the one that you envisioned for network framework and other frameworks too.
Use of self in wrapper types
A lot of
nw_*
methods are generated with network objects passed asNonNull<T>
. This is handled this way inheader-translator
and I had copied this bit verbatim assuming there is no difference withdispatch2
. However It feels weird that i need to useNonNull::from(self)
, i.e:What woudl be the right approach?
Arrays generated as ArrayUnknownABI
A 6-char C-array was generated as
ArrayUnknownABI
. What should I specify and where to generate a normal C array? Currently skipping the symbol and providing it manually vialib.rs
:Type-aliases for generated types
I have to define type-aliases in
lib.rs
since I subtitutenw_*_t
types with my own structs, for example:Should I type-alias it wrapped in
NWRetained
, i.eNWRetained<AdvertiseDescriptor>
?