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

Prefix newly added globals in Python with underscores #2109

Merged
merged 3 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/manual/src/futures.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub trait SayAfterTrait: Send + Sync {
Traits with callback interface support that export async methods can be combined with async Rust code.
See the [async-api-client example](https://github.com/mozilla/uniffi-rs/tree/main/examples/async-api-client) for an example of this.

### Python: uniffi_set_event_loop()
### Python: `uniffi_set_event_loop()`

Python bindings export a function named `uniffi_set_event_loop()` which handles a corner case when
integrating async Rust and Python code. `uniffi_set_event_loop()` is needed when Python async
Expand All @@ -93,3 +93,4 @@ In this case, we need an event loop to run the Python async function, but there'
Use `uniffi_set_event_loop()` to handle this case.
It should be called before the Rust code makes the async call and passed an eventloop to use.

Note that `uniffi_set_event_loop` cannot be glob-imported because it's not part of the library's `__all__`.
2 changes: 1 addition & 1 deletion fixtures/futures/tests/bindings/test_futures.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ async def test():

asyncio.run(test())
# check that all foreign future handles were released
self.assertEqual(len(futures.UNIFFI_FOREIGN_FUTURE_HANDLE_MAP), 0)
self.assertEqual(len(futures._UNIFFI_FOREIGN_FUTURE_HANDLE_MAP), 0)

def test_async_object_param(self):
async def test():
Expand Down
4 changes: 2 additions & 2 deletions uniffi_bindgen/src/bindings/python/gen_python/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,14 +366,14 @@ impl PythonCodeOracle {

/// Get the idiomatic Python rendering of an FFI callback function name
fn ffi_callback_name(&self, nm: &str) -> String {
format!("UNIFFI_{}", nm.to_shouty_snake_case())
format!("_UNIFFI_{}", nm.to_shouty_snake_case())
}

/// Get the idiomatic Python rendering of an FFI struct name
fn ffi_struct_name(&self, nm: &str) -> String {
// The ctypes docs use both SHOUTY_SNAKE_CASE AND UpperCamelCase for structs. Let's use
// UpperCamelCase and reserve shouting for global variables
format!("Uniffi{}", nm.to_upper_camel_case())
format!("_Uniffi{}", nm.to_upper_camel_case())
}

fn ffi_type_label(&self, ffi_type: &FfiType) -> String {
Expand Down
36 changes: 18 additions & 18 deletions uniffi_bindgen/src/bindings/python/templates/Async.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# Stores futures for _uniffi_continuation_callback
_UniffiContinuationHandleMap = _UniffiHandleMap()

UNIFFI_GLOBAL_EVENT_LOOP = None
_UNIFFI_GLOBAL_EVENT_LOOP = None

"""
Set the event loop to use for async functions
Expand All @@ -20,18 +20,18 @@
for the thread. Use `uniffi_set_event_loop` to force an eventloop to be used in this case.
"""
def uniffi_set_event_loop(eventloop: asyncio.BaseEventLoop):
global UNIFFI_GLOBAL_EVENT_LOOP
UNIFFI_GLOBAL_EVENT_LOOP = eventloop
global _UNIFFI_GLOBAL_EVENT_LOOP
_UNIFFI_GLOBAL_EVENT_LOOP = eventloop

def _uniffi_get_event_loop():
if UNIFFI_GLOBAL_EVENT_LOOP is not None:
return UNIFFI_GLOBAL_EVENT_LOOP
if _UNIFFI_GLOBAL_EVENT_LOOP is not None:
return _UNIFFI_GLOBAL_EVENT_LOOP
else:
return asyncio.get_running_loop()

# Continuation callback for async functions
# lift the return value or error and resolve the future, causing the async function to resume.
@UNIFFI_RUST_FUTURE_CONTINUATION_CALLBACK
@_UNIFFI_RUST_FUTURE_CONTINUATION_CALLBACK
def _uniffi_continuation_callback(future_ptr, poll_code):
(eventloop, future) = _UniffiContinuationHandleMap.remove(future_ptr)
eventloop.call_soon_threadsafe(_uniffi_set_future_result, future, poll_code)
Expand Down Expand Up @@ -63,7 +63,7 @@ async def _uniffi_rust_call_async(rust_future, ffi_poll, ffi_complete, ffi_free,
ffi_free(rust_future)

{%- if ci.has_async_callback_interface_definition() %}
def uniffi_trait_interface_call_async(make_call, handle_success, handle_error):
def _uniffi_trait_interface_call_async(make_call, handle_success, handle_error):
async def make_call_and_call_callback():
try:
handle_success(await make_call())
Expand All @@ -76,10 +76,10 @@ async def make_call_and_call_callback():
)
eventloop = _uniffi_get_event_loop()
task = asyncio.run_coroutine_threadsafe(make_call_and_call_callback(), eventloop)
handle = UNIFFI_FOREIGN_FUTURE_HANDLE_MAP.insert((eventloop, task))
return UniffiForeignFuture(handle, uniffi_foreign_future_free)
handle = _UNIFFI_FOREIGN_FUTURE_HANDLE_MAP.insert((eventloop, task))
return _UniffiForeignFuture(handle, _uniffi_foreign_future_free)

def uniffi_trait_interface_call_async_with_error(make_call, handle_success, handle_error, error_type, lower_error):
def _uniffi_trait_interface_call_async_with_error(make_call, handle_success, handle_error, error_type, lower_error):
async def make_call_and_call_callback():
try:
try:
Expand All @@ -98,17 +98,17 @@ async def make_call_and_call_callback():
)
eventloop = _uniffi_get_event_loop()
task = asyncio.run_coroutine_threadsafe(make_call_and_call_callback(), eventloop)
handle = UNIFFI_FOREIGN_FUTURE_HANDLE_MAP.insert((eventloop, task))
return UniffiForeignFuture(handle, uniffi_foreign_future_free)
handle = _UNIFFI_FOREIGN_FUTURE_HANDLE_MAP.insert((eventloop, task))
return _UniffiForeignFuture(handle, _uniffi_foreign_future_free)

UNIFFI_FOREIGN_FUTURE_HANDLE_MAP = _UniffiHandleMap()
_UNIFFI_FOREIGN_FUTURE_HANDLE_MAP = _UniffiHandleMap()

@UNIFFI_FOREIGN_FUTURE_FREE
def uniffi_foreign_future_free(handle):
(eventloop, task) = UNIFFI_FOREIGN_FUTURE_HANDLE_MAP.remove(handle)
eventloop.call_soon(uniffi_foreign_future_do_free, task)
@_UNIFFI_FOREIGN_FUTURE_FREE
def _uniffi_foreign_future_free(handle):
(eventloop, task) = _UNIFFI_FOREIGN_FUTURE_HANDLE_MAP.remove(handle)
eventloop.call_soon(_uniffi_foreign_future_do_free, task)

def uniffi_foreign_future_do_free(task):
def _uniffi_foreign_future_do_free(task):
if not task.done():
task.cancel()
{%- endif %}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{% if self.include_once_check("CallbackInterfaceRuntime.py") %}{% include "CallbackInterfaceRuntime.py" %}{% endif %}
{%- let trait_impl=format!("UniffiTraitImpl{}", name) %}
{%- let trait_impl=format!("_UniffiTraitImpl{}", name) %}

# Put all the bits inside a class to keep the top-level namespace clean
class {{ trait_impl }}:
Expand Down Expand Up @@ -75,24 +75,24 @@ def handle_error(status_code, rust_buffer):

{%- match meth.throws_type() %}
{%- when None %}
uniffi_out_return[0] = uniffi_trait_interface_call_async(make_call, handle_success, handle_error)
uniffi_out_return[0] = _uniffi_trait_interface_call_async(make_call, handle_success, handle_error)
{%- when Some(error) %}
uniffi_out_return[0] = uniffi_trait_interface_call_async_with_error(make_call, handle_success, handle_error, {{ error|type_name }}, {{ error|lower_fn }})
uniffi_out_return[0] = _uniffi_trait_interface_call_async_with_error(make_call, handle_success, handle_error, {{ error|type_name }}, {{ error|lower_fn }})
{%- endmatch %}
{%- endif %}
{%- endfor %}

@{{ "CallbackInterfaceFree"|ffi_callback_name }}
def uniffi_free(uniffi_handle):
def _uniffi_free(uniffi_handle):
{{ ffi_converter_name }}._handle_map.remove(uniffi_handle)

# Generate the FFI VTable. This has a field for each callback interface method.
uniffi_vtable = {{ vtable|ffi_type_name }}(
_uniffi_vtable = {{ vtable|ffi_type_name }}(
{%- for (_, meth) in vtable_methods.iter() %}
{{ meth.name()|fn_name }},
{%- endfor %}
uniffi_free
_uniffi_free
)
# Send Rust a pointer to the VTable. Note: this means we need to keep the struct alive forever,
# or else bad things will happen when Rust tries to access it.
_UniffiLib.{{ ffi_init_callback.name() }}(ctypes.byref(uniffi_vtable))
_UniffiLib.{{ ffi_init_callback.name() }}(ctypes.byref(_uniffi_vtable))
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
# Magic number for the Rust proxy to call using the same mechanism as every other method,
# to free the callback once it's dropped by Rust.
IDX_CALLBACK_FREE = 0
_UNIFFI_IDX_CALLBACK_FREE = 0
# Return codes for callback calls
_UNIFFI_CALLBACK_SUCCESS = 0
_UNIFFI_CALLBACK_ERROR = 1
_UNIFFI_CALLBACK_UNEXPECTED_ERROR = 2

class UniffiCallbackInterfaceFfiConverter:
class _UniffiCallbackInterfaceFfiConverter:
_handle_map = _UniffiHandleMap()

@classmethod
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@
{% include "CallbackInterfaceImpl.py" %}

# The _UniffiConverter which transforms the Callbacks in to Handles to pass to Rust.
{{ ffi_converter_name }} = UniffiCallbackInterfaceFfiConverter()
{{ ffi_converter_name }} = _UniffiCallbackInterfaceFfiConverter()
4 changes: 2 additions & 2 deletions uniffi_bindgen/src/bindings/python/templates/HandleMap.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ def get(self, handle):
with self._lock:
return self._map[handle]
except KeyError:
raise InternalError("UniffiHandleMap.get: Invalid handle")
raise InternalError("_UniffiHandleMap.get: Invalid handle")

def remove(self, handle):
try:
with self._lock:
return self._map.pop(handle)
except KeyError:
raise InternalError("UniffiHandleMap.remove: Invalid handle")
raise InternalError("_UniffiHandleMap.remove: Invalid handle")

def __len__(self):
return len(self._map)
3 changes: 0 additions & 3 deletions uniffi_bindgen/src/bindings/python/templates/wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@
{%- for c in ci.callback_interface_definitions() %}
"{{ c.name()|class_name }}",
{%- endfor %}
{%- if ci.has_async_fns() %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this change - this function is documented as being exported by the Python bindings.

Copy link
Contributor Author

@heinrich5991 heinrich5991 May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still exported. __all__ just determines what you get when you do from library import *. You can still get it using import library; library.uniffi_set_event_loop or from library import uniffi_set_event_loop.

EDIT: Docs: https://docs.python.org/3/tutorial/modules.html#importing-from-a-package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that - but I'm not sure this change will meet the expectations of people reading our docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(to be clear, I'm not saying this change is wrong, I'm just not sure it's correct. @badboy @bendk do either of you have thoughts)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry for the stream of thoughts: this function is only used in such edge-cases that I think I'd also be fine with a note in the docs flagging that the symbol must be used by importing it and "from mod import *" doesn't work)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added such a note.

"uniffi_set_event_loop",
{%- endif %}
]

{% import "macros.py" as py %}