feat: add ffi::Expected<T> for exception-free error handling #399
Conversation
Summary of ChangesHello @guan404ming, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the error handling capabilities within the FFI layer by introducing a new Expected type. This type provides a robust, exception-free mechanism for FFI functions to communicate either a successful result or a detailed error, aligning with modern C++ practices and similar patterns in other languages. The changes ensure that FFI calls can explicitly manage potential failures without relying on exceptions for control flow, leading to more predictable and maintainable code. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
guan404ming
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ffi::Expected<T> for exception-free error handling, a valuable addition for creating more robust FFI functions. The implementation is comprehensive, covering the Expected<T> class, integration with tvm::ffi::Function via CallExpected, and support for packed functions returning Expected<T>. The tests are thorough and cover many important use cases.
The overall approach of using tvm::ffi::Any for storage within Expected<T> is a clever way to leverage the existing FFI infrastructure. I have a few suggestions to simplify the TypeTraits specialization and to add move semantics for value/error accessors, which would improve code clarity and performance. Overall, this is a well-executed feature.
| TVM_FFI_INLINE T value() const { | ||
| if (is_err()) { | ||
| TVM_FFI_THROW(RuntimeError) << "Bad expected access: contains error"; | ||
| } | ||
| return data_.cast<T>(); | ||
| } |
There was a problem hiding this comment.
The value() method is const-qualified, which means it always returns a copy of the contained value. This can be inefficient when the Expected object is an rvalue (e.g., a temporary returned from a function). Consider adding an rvalue-qualified overload to allow moving the value out. This would improve performance in scenarios like std::move(expected).value().
You can change the existing method to be const&-qualified and add a &&-qualified overload:
TVM_FFI_INLINE T value() const& {
if (is_err()) {
TVM_FFI_THROW(RuntimeError) << "Bad expected access: contains error";
}
return data_.cast<T>();
}
TVM_FFI_INLINE T value() && {
if (is_err()) {
TVM_FFI_THROW(RuntimeError) << "Bad expected access: contains error";
}
return std::move(data_).cast<T>();
}| TVM_FFI_INLINE Error error() const { | ||
| TVM_FFI_ICHECK(is_err()) << "Expected does not contain an error"; | ||
| return data_.cast<Error>(); | ||
| } |
There was a problem hiding this comment.
Similar to value(), the error() method is const-qualified and always returns a copy. You can add an rvalue-qualified overload to allow moving the Error object out, which is more efficient.
TVM_FFI_INLINE Error error() const& {
TVM_FFI_ICHECK(is_err()) << "Expected does not contain an error";
return data_.cast<Error>();
}
TVM_FFI_INLINE Error error() && {
TVM_FFI_ICHECK(is_err()) << "Expected does not contain an error";
return std::move(data_).cast<Error>();
}| TVM_FFI_INLINE static void CopyToAnyView(const Expected<T>& src, TVMFFIAny* result) { | ||
| // Extract value from src.data_ and copy it properly | ||
| const TVMFFIAny* src_any = reinterpret_cast<const TVMFFIAny*>(&src.data_); | ||
|
|
||
| if (TypeTraits<T>::CheckAnyStrict(src_any)) { | ||
| // It contains T, copy it out and move to result | ||
| T value = TypeTraits<T>::CopyFromAnyViewAfterCheck(src_any); | ||
| TypeTraits<T>::MoveToAny(std::move(value), result); | ||
| } else { | ||
| // It contains Error, copy it out and move to result | ||
| Error err = TypeTraits<Error>::CopyFromAnyViewAfterCheck(src_any); | ||
| TypeTraits<Error>::MoveToAny(std::move(err), result); | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of CopyToAnyView is correct but can be simplified. Instead of checking the type and manually performing a copy-then-move for each case, you can leverage the Any type's copy constructor to handle the logic. This makes the code more concise and less error-prone, as it relies on the already-tested Any copy semantics.
TVM_FFI_INLINE static void CopyToAnyView(const Expected<T>& src, TVMFFIAny* result) {
// An Expected<T> is represented by its underlying value (T or Error)
// at the FFI boundary. We can simply copy the contained Any.
Any copied_any = src.data_;
*result = details::AnyUnsafe::MoveAnyToTVMFFIAny(std::move(copied_any));
}| TVM_FFI_INLINE static void MoveToAny(Expected<T> src, TVMFFIAny* result) { | ||
| // Extract value from src.data_ and move it properly | ||
| TVMFFIAny* src_any = reinterpret_cast<TVMFFIAny*>(&src.data_); | ||
|
|
||
| if (TypeTraits<T>::CheckAnyStrict(src_any)) { | ||
| // It contains T, move it out and move to result | ||
| T value = TypeTraits<T>::MoveFromAnyAfterCheck(src_any); | ||
| TypeTraits<T>::MoveToAny(std::move(value), result); | ||
| } else { | ||
| // It contains Error, move it out and move to result | ||
| Error err = TypeTraits<Error>::MoveFromAnyAfterCheck(src_any); | ||
| TypeTraits<Error>::MoveToAny(std::move(err), result); | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to CopyToAnyView, the MoveToAny implementation can be greatly simplified. Since src is passed by value (effectively a move), you can move the underlying Any object directly. This is more readable and directly expresses the intent of moving the contained value.
TVM_FFI_INLINE static void MoveToAny(Expected<T> src, TVMFFIAny* result) {
// An Expected<T> is represented by its underlying value (T or Error)
// at the FFI boundary. We can simply move the contained Any.
*result = details::AnyUnsafe::MoveAnyToTVMFFIAny(std::move(src.data_));
}
guan404ming
force-pushed
the
feat/expected-type
branch
from
2f70b38 to
f1a4dc9
Compare
|
Thanks a lot for the contribution, this would benefit from careful reviews, @junrushao @DarkSharpness @Ubospica can you help |
|
@guan404ming Thanks for the contribution! I will take a closer look sometime this week to provide a round of review. |
3 participants
Why
Enable exception-free C++ API similar to Rust's Result or C++23's std::expected, as requested in #234.
How
Expected<T>class holding either success value T or ErrorFunction::CallExpected<T>()for exception-free function callsis_expectedtype trait and Expected handling inunpack_callExpected<T>"