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

New standard proposal #5498

Closed
wants to merge 33 commits into from
Closed

New standard proposal #5498

wants to merge 33 commits into from

Conversation

rbran
Copy link
Contributor

@rbran rbran commented Jun 4, 2024

This is an proposal to standardize the inner-workings of the API, specially with the FFI types.

Mostly it consist of creating standard functions to create/manger FFI wrapper types, such as functions into_raw, as_raw, from_raw, ref_from_raw, the use of NonNull<T> instead of the naked pointer, hide inner representations, etc.

This don't necessarily has the objective of being merged, but act like a guide to new implementations.

rbran added 30 commits May 29, 2024 07:56
@emesare emesare self-requested a review July 10, 2024 00:28
});
}
// SAFETY: FunctionParameter and BNFunctionParameter are transparent,
// so this can be safelly transmuted
Copy link
Member

Choose a reason for hiding this comment

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

safelly -> safely

@rssor
Copy link
Member

rssor commented Jul 10, 2024

so overall, I'm not really a fan of doing away with the Ref<> type -- part of the initial design philosophy was to completely separate the wrapping of an object with the semantics of its ownership (similar to &str vs String). This is something we got right in the C++ API and that we got wrong in the Python API (and this has had a very long tail of memory leaks and making it hard to understand how the API interacts with the core).

The current system where API users can't ever directly own the wrapper objects (meaning they either see them through a reference or through a Ref<> that they own) provides a lot of clarity especially for those of us who work on both sides of the API boundary.

I'm fine on being more consistent about into_raw/from_raw/etc. and establishing clear conventions

I think in general we're better off not transmuting and just wrapping explicitly with newtypes, that way we can rework the internals of any wrappers and it's never going to result in user-facing changes at all.

@rbran
Copy link
Contributor Author

rbran commented Jul 11, 2024

so overall, I'm not really a fan of doing away with the Ref<> type -- part of the initial design philosophy was to completely separate the wrapping of an object with the semantics of its ownership (similar to &str vs String). This is something we got right in the C++ API and that we got wrong in the Python API (and this has had a very long tail of memory leaks and making it hard to understand how the API interacts with the core).

There are many problems with having an Ref:

  1. In rust if you say Ref, people think Reference, what is a borrowed value, the exactly opposite of what a Ref<T> means.

The ownership separation is very different from &str and String, instead it more similar to Rc<T> and Arc<T>.

  1. If a value is always a Ref<T>, then it should be only T.

If a type always and only exists as Rc<T>, then instead of returning Rc<T>, you should encapsulate it and hide the implementation details, like pub struct T(Rc<InnerT>), only exposing the required object.

  1. To much complexity

Exposing the Ref<T> to the user requires him to know about Ref, RefCountable, T, Guard and how they interact with each other, meanwhile we can simplify it to just T, requiring 1/4 of the effort to understand.

The current system where API users can't ever directly own the wrapper objects (meaning they either see them through a reference or through a Ref<> that they own) provides a lot of clarity especially for those of us who work on both sides of the API boundary.

That's exactly my idea, I'm not exactly deleting the Ref object, just hiding it using encapsulation. The point is that if you hide Ref, then you don't need it anymore on the API.

I think in general we're better off not transmuting and just wrapping explicitly with newtypes, that way we can rework the internals of any wrappers and it's never going to result in user-facing changes at all.

My philosophy for FFI is always build safe object, that is as close as possible to zero-cost. Although it's usually more complex at first, you will notice that any early-abstractions have a tendency on create more complexity.

The pilling up of complexity have a tendency to show up on iterators and other kinds of high level abstractions. Eg the Array<T>, ArrayGuard<T>, RefCountable and CoreArrayProvider need to be exposed to create owned Arrays. Also making impossible to implement Index<usize> for the Array<T> type.

@rbran rbran closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants