feat: add ffi::Expected<T> for exception-free error handling #399

guan404ming · 2026-01-11T15:38:35Z

Why

Enable exception-free C++ API similar to Rust's Result or C++23's std::expected, as requested in #234.

How

Add Expected<T> class holding either success value T or Error
Add Function::CallExpected<T>() for exception-free function calls
Add is_expected type trait and Expected handling in unpack_call
Support registering functions that return Expected<T>"

gemini-code-assist · 2026-01-11T15:38:52Z

Summary of Changes

Hello @guan404ming, I'm Gemini Code Assist¹! 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

Introduced ffi::Expected: A new type for exception-free error handling, similar to Rust's Result or C++23's std::expected, allowing functions to return either a success value T or an Error.
Added Function::CallExpected(): A new method for FFI function calls that returns an Expected, converting any caught exceptions into Error objects within the Expected type.
Enhanced FFI Type System: Integrated Expected into the FFI type traits (is_expected and RetSupported) and the unpack_call mechanism, enabling seamless handling of functions returning Expected.
Support for Registering Expected-Returning Functions: Functions that return Expected can now be registered with the FFI system, allowing them to be called from other languages or contexts while preserving the explicit error handling.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

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.

include/tvm/ffi/expected.h

tqchen · 2026-01-12T14:15:11Z

Thanks a lot for the contribution, this would benefit from careful reviews, @junrushao @DarkSharpness @Ubospica can you help

Kathryn-cat · 2026-01-14T16:24:22Z

@guan404ming Thanks for the contribution! I will take a closer look sometime this week to provide a round of review.

Kathryn-cat · 2026-01-20T07:58:09Z

include/tvm/ffi/expected.h

+   */
+  TVM_FFI_INLINE T value() const {
+    if (is_err()) {
+      TVM_FFI_THROW(RuntimeError) << "Bad expected access: contains error";


If value() failed, it appears that it throws an error here but doesn't preserve the original error. Is it possible to retain the original error information?

Done. Changed value() to throw the contained error directly instead of a generic RuntimeError:

if (is_err()) { throw data_.cast<Error>(); }

Kathryn-cat · 2026-01-20T08:20:17Z

include/tvm/ffi/expected.h

+    if (is_err()) {
+      TVM_FFI_THROW(RuntimeError) << "Bad expected access: contains error";
+    }
+    return data_.cast<T>();


For large data, we can use move instead of copy, so I agree with Gemini that we can add an overload function here:

TVM_FFI_INLINE T value() && { if (is_err()) { throw error(); } return std::move(data_).cast<T>(); }

Agree as well. I've added both const& and && qualified overloads for value():

Actually we might not need both here, @tqchen would like to hear your opinion if we wanna keep one of them

I feel it's fine to have both, since their usage would be different: std::move(expected).value() (for the move overload) vs expected.value() (for the const& overload), and this also follows C++ standard.

Kathryn-cat · 2026-01-20T08:29:25Z

include/tvm/ffi/expected.h

+   * \brief Check if the Expected contains an error.
+   * \return True if contains error, false if contains success value.
+   */
+  TVM_FFI_INLINE bool is_err() const { return data_.as<Error>().has_value(); }


It is likely that ObjectRef as a base class of Error can have both is_ok()=True and is_err()=True, since CheckAnyStrict relies on inheritance checking, not type matching. I would suggest returning !is_ok() here.

Also it would be helpful to enhance the test cases on this.

Changed is_err() to return !is_ok() and also fixed is_ok() to check for Error first.

Kathryn-cat · 2026-01-20T08:41:38Z

include/tvm/ffi/expected.h

+  /*!
+   * \brief Access the error value.
+   * \return The error value.
+   * \note Behavior is undefined if the Expected contains a success value.


Let's instead of saying "behavior is undefined", say it throws a RuntimeError on bad access.

Perhaps change the function body into

if (!is_err()) { TVM_FFI_THROW(RuntimeError) << "Bad expected access: contains value, not error"; }

to mimic value() impl.

Sure, I've updated the docstring and changed from TVM_FFI_ICHECK to TVM_FFI_THROW(RuntimeError)

Kathryn-cat · 2026-01-23T08:09:21Z

include/tvm/ffi/expected.h

+/*!
+ * \brief Helper function to create Expected::Err.
+ * \param error The error value.
+ * \return Expected<Any> containing the error.


Let's fix the comment here, should return Expected<T> same as the ExpectedOk case.

Kathryn-cat · 2026-01-23T08:12:38Z

include/tvm/ffi/expected.h

+ * \return Expected<Any> containing the error.
+ * \note Returns Expected<Any> to allow usage in contexts where T is inferred.
+ */
+template <typename T = Any>


Perhaps template <typename T> is cleaner? Do we have to default to Any here

Yes, that would be better. Thanks.

Kathryn-cat · 2026-01-23T08:19:00Z

include/tvm/ffi/expected.h

+   * \return The success value if present, otherwise the default value.
+   */
+  template <typename U = std::remove_cv_t<T>>
+  TVM_FFI_INLINE T value_or(U&& default_value) const {


I think this function might be rarely used, because the philosophy of Expected<T> is handling error explicitly. Let's leave it for discussion here.

I think maybe we should keep this since that is kind of standard practice std::optional and C++23 std::expected both have value_or.

ref std::expected (C++23) and std::optional (C++17)

Kathryn-cat · 2026-01-23T08:43:32Z

include/tvm/ffi/expected.h

+  TVM_FFI_INLINE static void CopyToAnyView(const Expected<T>& src, TVMFFIAny* result) {
+    const TVMFFIAny* src_any = reinterpret_cast<const TVMFFIAny*>(&src.data_);
+    if (TypeTraits<T>::CheckAnyStrict(src_any)) {
+      TypeTraits<T>::MoveToAny(TypeTraits<T>::CopyFromAnyViewAfterCheck(src_any), result);


We possibly don't need to create a copy of T then move it.

Possible simplification:

if (src.is_err()) { TypeTraits<Error>::CopyToAnyView(src.error(), result); } else { TypeTraits<T>::CopyToAnyView(src.value(), result); }

Thanks, this is more clearer.

Kathryn-cat · 2026-01-23T08:46:11Z

include/tvm/ffi/expected.h

+  TVM_FFI_INLINE static void MoveToAny(Expected<T> src, TVMFFIAny* result) {
+    TVMFFIAny* src_any = reinterpret_cast<TVMFFIAny*>(&src.data_);
+    if (TypeTraits<T>::CheckAnyStrict(src_any)) {
+      TypeTraits<T>::MoveToAny(TypeTraits<T>::MoveFromAnyAfterCheck(src_any), result);


ditto,

if (src.is_err()) { TypeTraits<Error>::MoveToAny(std::move(src).error(), result); } else { TypeTraits<T>::MoveToAny(std::move(src).value(), result); }

Kathryn-cat · 2026-01-23T09:03:46Z

include/tvm/ffi/expected.h

+  }
+
+  TVM_FFI_INLINE static std::string TypeSchema() {
+    return R"({"type":"Expected","args":[)" + details::TypeSchema<T>::v() + "]}";


Maybe also mention {"type":"ffi.Error"} in type schema?

Kathryn-cat · 2026-01-23T09:06:29Z

include/tvm/ffi/expected.h

+    return std::nullopt;
+  }
+
+  TVM_FFI_INLINE static std::string GetMismatchTypeInfo(const TVMFFIAny* src) {


Let's remove this

Kathryn-cat · 2026-01-23T23:13:31Z

include/tvm/ffi/function.h

+
+    if (ret_code == 0) {
+      // Success - cast result to T and return Ok
+      return Expected<T>::Ok(std::move(result).cast<T>());


Here's a tricky part. If the result does not match the type T, cast<T>() will throw, and this error will escape CallExpected.

Some proposed change: (feel free to brainstorm better ones)

if (ret_code == 0) { if (auto val = std::move(result).template as<T>()) { return Expected<T>::Ok(std::move(*val)); } else { return Expected<T>::Err(Error("TypeError", "CallExpected: result type mismatch, expected " + TypeTraits<T>::TypeStr(), "")); } }

Nice catch. The implementation looks good. I think there is a potential edge case that we probably don't want ot try casting if T = Any. Here is updated implementation

if (ret_code == 0) { if constexpr (std::is_same_v<T, Any>) { return Expected<T>::Ok(std::move(result)); } else { if (auto val = std::move(result).template as<T>()) { return Expected<T>::Ok(std::move(*val)); } else { return Expected<T>::Err(Error( "TypeError", "CallExpected: result type mismatch, expected " + TypeTraits<T>::TypeStr(), "")); } } }

Kathryn-cat

Nice work @guan404ming, thanks for addressing the comments! The PR implements the proposal cleanly and the functionalities are in good state. My last advice are below, cc' @tqchen for a final round of discussion.

Since it introduces new function calling API to the users, @guan404ming could you document the usage? I think your test case RegisterExpectedReturning shows a good example on registering and calling Expected functions, and when to use the throw exception vs Expected flows. (cc' @junrushao if you could provide a pointer to where to add the docs)

Kathryn-cat · 2026-01-24T00:18:35Z

include/tvm/ffi/function_details.h

-static constexpr bool RetSupported =
-    (std::is_same_v<T, Any> || std::is_void_v<T> || TypeTraits<T>::convert_enabled);
+static constexpr bool RetSupported = (std::is_same_v<T, Any> || std::is_void_v<T> ||
+                                      TypeTraits<T>::convert_enabled || is_expected_v<T>);


I think convert_enabled is already True here, is_expected_v might be redundant

You're right, let me remove it.

Kathryn-cat · 2026-01-24T00:21:16Z

include/tvm/ffi/function_details.h

+    if (expected_result.is_ok()) {
+      *rv = expected_result.value();
+    } else {
+      throw expected_result.error();


Perhaps use move?

if (expected_result.is_ok()) { *rv = std::move(expected_result).value(); } else { throw std::move(expected_result).error(); }

Sure, updated!

guan404ming · 2026-01-24T04:46:23Z

Since it introduces new function calling API to the users, @guan404ming could you document the usage?

Sure, I think I could do it as a follow up after this one!

Kathryn-cat · 2026-01-24T05:19:26Z

@tqchen the CI fails because tvm-ffi's DLPack version is 1.0.2, while dlpack exchange api is tested against PyTorch's native impl (DLPack 1.0.3), perhaps we need to upgrade tvm-ffi's DLPack version

Kathryn-cat · 2026-01-24T05:36:56Z

Since it introduces new function calling API to the users, @guan404ming could you document the usage?

Sure, I think I could do it as a follow up after this one!

Thanks @guan404ming , I think the PR is in good state, let's wait for CI to be fixed and get it in!

guan404ming · 2026-01-24T06:52:40Z

Here is the upgrade pr #420. I've checked there is no any compat issue need to be updated in our src code.

guan404ming · 2026-01-25T13:58:12Z

The ci passed after the upgrade!

tqchen · 2026-01-26T18:57:38Z

include/tvm/ffi/function_details.h

  // use index sequence to do recursive-less unpacking
  if constexpr (std::is_same_v<R, void>) {
    f(ArgValueWithContext<std::tuple_element_t<Is, PackedArgs>>{args, Is, optional_name, f_sig}...);
+  } else if constexpr (is_expected_v<R>) {


This behavior can be a bit confusing. If the ffi.Function is explicitly returning Expected Value (instead of throw using the error handling mechanism, then the function should successully return instead of implicitly throw when error is found?

Mayb need a regression testcase for this

i think the current design unwraps Expected<T> at the FFI boundary. CallExpected use safe_call to catch the error and return Expected<T>; on the contrary, the normal call path would throw an error. This might seem a bit convoluted. Do you have better suggested ideas?

There are two ways that Expected get returned

W0: Expected get "raised" internally by setting the TLS value via TVMFFISetRaised

W1: Expected get returned as a normal return value

And there are two ways to call a function now

C0: Normal call

C1: CallExpected

Would be good to discuss the overall relation in the mix of four cases

C0+ W0: we need to throw the error to caller

C0 + W1: no error should be thrown, the Expected should be returned to the caller

C1 + W0: we should return Expected with the error "raised" set to the returned Expected value

C1 + W1: we need to return the Expected, note that in this case it is harder to distinguish error being returned versus error being raised if the desirable logic is to return an error, but this is more of a rare case.

In any case, it would be good first to make sure behavior of C0 is correct, and the particular context seems to suggest C0+W1, in such case, we should return the value to the caller.

Thanks for the detailed breakdown of the four scenarios (C0/C1 + W0/W1) which really helped clarify the design considerations. Based on my understanding, the key question is about the C0 + W1 scenario (normal Call() on a function returning Expected). Here's how I see the two possible directions:

Option A: Keep current behavior (auto-unwrap)

// Current implementation if (expected_result.is_ok()) { *rv = std::move(expected_result).value(); // unwrap value } else { throw std::move(expected_result).error(); // throw error }

Pros: Caller uses Call() and gets the value directly

Cons: Cannot distinguish "returned error" from "raised error"

Option B: No unwrap, return Expected directly

// Proposed change *rv = f(...); // return Expected as-is

Pros: Clear distinction between returned vs raised errors

Cons: Caller must use Call<Expected>() and handle it explicitly

Personally, I'd slightly lean toward Option B after @tqchen's breakdown. If a function explicitly chooses to return Expected, I think we should respect that and let the caller receive it directly.

I'd like to confirm your preference before updating the implementation. Happy to add regression tests or update implementation for whichever approach we go with.

agree option B is better

Updated with option B. Please take another look, thanks!

tqchen · 2026-01-26T18:58:41Z

include/tvm/ffi/expected.h

+ * \return Expected<T> containing the success value.
+ */
+template <typename T>
+TVM_FFI_INLINE Expected<T> ExpectedOk(T value) {


I think it is good to do round of API review, can you list all the APIs that are not conforming to std::expected, list their names, and discuss choice(i know some comes from rust API style, but good to be explicit). Would be good to list the APIs in the comment

tqchen · 2026-01-26T19:00:43Z

Thanks @guan404ming , i think we are close, i have one behavior comment. Besides that, given this is new public API, i think it would be helpful to do a round of quick API review, can you provide a list of APIs(including methods), would be good to categorize them as:

C0: strictly follows c++ standard
C1: others and list rationale(e.g. we follows rust API)

Would also be good to discuss move and value copy behavior.

guan404ming · 2026-01-27T16:43:01Z

Here is the API review

| API                | Category | Notes                                                              |                                                                         
|--------------------|----------|--------------------------------------------------------------------|                                                                         
| is_ok()            | C0       | Follows std::expected::has_value() semantics                       |                                                                         
| is_err()           | C1       | Rust-style naming; equivalent to !has_value()                      |                                                                         
| has_value()        | C0       | Alias for is_ok(), follows C++ standard                            |                                                                         
| value()            | C0       | Follows std::expected::value()                                     |                                                                         
| value() &&         | C1       | Move-qualified overload for efficiency (not in C++23 standard)     |                                                                         
| error()            | C0       | Follows std::expected::error()                                     |                                                                         
| error() &&         | C1       | Move-qualified overload for efficiency                             |                                                                         
| value_or(default)  | C0       | Follows std::expected::value_or()                                  |                                                                         
| ExpectedOk(value)  | C1       | Rust-style helper; C++ standard has no equivalent factory function |                                                                         
| ExpectedErr(error) | C1       | Rust-style helper; C++ standard has no equivalent factory function |

Rationale for C1 APIs:

Move-qualified overloads (&&): Added for performance when the Expected is a temporary, avoiding unnecessary copies of large objects.
ExpectedOk() / ExpectedErr(): Rust-style factory functions for cleaner syntax with type deduction. C++23 std::expected relies on constructors instead.
is_err(): Rust naming convention; provides symmetry with is_ok().

APIs not implemented (compared to C++23 std::expected):

operator bool(): can use has_value() or is_ok() instead
operator*() / operator->(): unchecked access, omitted for safety
Monadic operations (and_then, transform, or_else, transform_error): may add in future if needed

tqchen · 2026-01-27T18:22:53Z

thanks @guan404ming , cc @Ubospica @Seven-Streams @junrushao @DarkSharpness for quick check on APIs. the ask

please comment on the API naming/choice, especially C1 APIs.
help cross check needs in possible downstream (@Ubospica would be good to confirm xgrammar needs)

DarkSharpness · 2026-01-27T19:12:03Z

Here is the API review

| API                | Category | Notes                                                              |                                                                         
|--------------------|----------|--------------------------------------------------------------------|                                                                         
| is_ok()            | C0       | Follows std::expected::has_value() semantics                       |                                                                         
| is_err()           | C1       | Rust-style naming; equivalent to !has_value()                      |                                                                         
| has_value()        | C0       | Alias for is_ok(), follows C++ standard                            |                                                                         
| value()            | C0       | Follows std::expected::value()                                     |                                                                         
| value() &&         | C1       | Move-qualified overload for efficiency (not in C++23 standard)     |                                                                         
| error()            | C0       | Follows std::expected::error()                                     |                                                                         
| error() &&         | C1       | Move-qualified overload for efficiency                             |                                                                         
| value_or(default)  | C0       | Follows std::expected::value_or()                                  |                                                                         
| ExpectedOk(value)  | C1       | Rust-style helper; C++ standard has no equivalent factory function |                                                                         
| ExpectedErr(error) | C1       | Rust-style helper; C++ standard has no equivalent factory function |

Rationale for C1 APIs:

Move-qualified overloads (&&): Added for performance when the Expected is a temporary, avoiding unnecessary copies of large objects.
ExpectedOk() / ExpectedErr(): Rust-style factory functions for cleaner syntax with type deduction. C++23 std::expected relies on constructors instead.
is_err(): Rust naming convention; provides symmetry with is_ok().

APIs not implemented (compared to C++23 std::expected):

operator bool(): can use has_value() or is_ok() instead
operator*() / operator->(): unchecked access, omitted for safety
Monadic operations (and_then, transform, or_else, transform_error): may add in future if needed

Thanks for the detailed API review.

One small difference worth calling out for ExpectedOk / ExpectedErr is a C++-specific type deduction behavior compared to Rust. In Rust, generic parameters can be inferred from the return type, while in C++ template argument deduction only considers function arguments, not the return type. This means that with factory-style helpers like ExpectedOk, the deduced type may sometimes differ from the desired return type. For example:

template <typename T>
Expect <T> ExpectOk(T value);

Expect<long> function(int x) {
  return ExpectOk(x); // <- in many cases, we want [T = long], which deduced from return type.
                      // but in this case T will deduced as int, which may lead to compile error.
}

In such cases, C++ cannot deduce T = long from the return type, which may lead to a compilation error.

I'm not sure whether this will cause some inconvenience in real usage, but the current design is reasonable for simplicity.

In XGrammar we implement something like PartialExpected, which can be implicitly converted to any Expected<T> and solves the return type inference problem. However, this actually introduces much complexity https://github.com/mlc-ai/xgrammar/blob/aa44ded202fdaa2cb55f48deb31d17c9822a8a1f/cpp/support/utils.h#L130-L212

guan404ming · 2026-01-28T05:09:14Z

Thanks for bringing up the discussion! I agree with keeping the design simple. While return type deduction is nice, explicit type specification is a fair trade-off to keep the API clean.

I think type deduction works for most cases. For exceptions, users can simply use like Expected<long>::Ok(x). For a foundational library like tvm-ffi, the complexity of PartialExpected isn't worth the marginal benefit in my opinion. This strikes the right balance between simplicity and usability.

guan404ming · 2026-01-31T02:59:17Z

One minor thing on consistency with c++, i think it might makes sense to have Unexpected(Error()) so we are being explicit that this is an Unexpected value

Thanks for the nice catch, just updated with Unexpected

Ubospica

The new changes look great to me. Thank you so much!

cc @tqchen @junrushao

Ubospica · 2026-02-01T03:15:16Z

include/tvm/ffi/expected.h

+    if (is_err()) {
+      TVM_FFI_THROW(RuntimeError) << "Bad expected access: contains error";
+    }
+    return data_.cast<T>();


I feel it's fine to have both, since their usage would be different: std::move(expected).value() (for the move overload) vs expected.value() (for the const& overload), and this also follows C++ standard.

guan404ming · 2026-02-01T05:37:11Z

The new changes look great to me. Thank you so much!

Thanks for your review!

tqchen · 2026-02-02T13:28:06Z

include/tvm/ffi/expected.h

+  // NOLINTNEXTLINE(google-explicit-constructor,runtime/explicit)
+  Unexpected(Error error) : error_(std::move(error)) {}
+  /*! \brief The wrapped error. */
+  Error error_;


consider align with std::unexpected<E> API, make it a class and keep it an private member. The error can be accessed via error() member function https://en.cppreference.com/w/cpp/utility/expected/unexpected.html

Would be good to see if we can use Unexpected for E as subclass of Error if such auto deduction is possible.

It will be more consistent in this way, thanks. I just updated with your suggestion.

tqchen · 2026-02-02T13:32:16Z

Some final comments, @Kathryn-cat @DarkSharpness @Seven-Streams please take another look

tqchen · 2026-02-02T16:51:42Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces ffi::Expected<T> for exception-free error handling in the C++ FFI, which is a great addition for API robustness. The implementation of Expected<T>, Unexpected<E>, and the associated TypeTraits is well-designed and consistent with the TVM FFI architecture. The new Function::CallExpected method provides a convenient way to interact with functions that might throw exceptions. The accompanying tests are comprehensive and cover many important edge cases.

I have one suggestion to improve the implementation of CallExpected to make it more consistent with the existing casting behavior in TVM's FFI, allowing for type conversions and providing a more informative error message on type mismatch.

include/tvm/ffi/function.h

tqchen

some final comments

tqchen · 2026-02-02T18:21:22Z

include/tvm/ffi/function.h

+        if (auto val = result.template try_cast<T>()) {
+          return std::move(*val);
+        }
+        return Error("TypeError",


I know it is a bit more verbose, but i think it is helpful to always use Unexpected(Errror()), and ban implicit conversion from Error

All error returns now use explicit Unexpected() wrapper instead of implicit Error conversion, which I think would be more helpful!

tqchen · 2026-02-02T18:21:42Z

include/tvm/ffi/function.h

+        }
+        // Try to extract as T
+        if (auto val = result.template try_cast<T>()) {
+          return std::move(*val);


move optional bvalue should be *std::move(val), would be good to confirm with genai

Verified and changed to *std::move(val)

include/tvm/ffi/function.h

include/tvm/ffi/function_details.h

tqchen · 2026-02-02T18:28:53Z

include/tvm/ffi/function_details.h

+};
+
+template <typename T>
+inline constexpr bool is_expected_v = is_expected<T>::value;


not sure if this is needed anymore given the new convention

I think the trait is still used in test assertions (test_expected.cc:213) to verify Expected type detection, so I keeping it for now.

i see, it is worthwhile minimizing the code mainly because is_expected_v may be a future std type, the test uscase seems to be a duplication, so still good to remove here

Make sense to me. Just removed!

include/tvm/ffi/function.h

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

tqchen · 2026-02-03T18:16:56Z

include/tvm/ffi/function_details.h

+};
+
+template <typename T>
+inline constexpr bool is_expected_v = is_expected<T>::value;


i see, it is worthwhile minimizing the code mainly because is_expected_v may be a future std type, the test uscase seems to be a duplication, so still good to remove here

tqchen · 2026-02-03T18:17:13Z

tests/cpp/test_expected.cc

+  using FuncInfo = tvm::ffi::details::FunctionInfo<decltype(safe_divide)>;
+  static_assert(std::is_same_v<FuncInfo::RetType, Expected<int>>,
+                "Return type should be Expected<int>");
+  static_assert(tvm::ffi::details::is_expected_v<FuncInfo::RetType>,


this duplicates with the is_same_v assertion on top

tqchen · 2026-02-03T18:17:43Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces ffi::Expected<T>, a valuable addition for exception-free error handling in the C++ API, similar to std::expected. The implementation is solid, including the new Function::CallExpected method and comprehensive test coverage. I have one main piece of feedback regarding potential object slicing with custom error types, which I've detailed in a specific comment. Overall, this is a great feature that will improve the robustness of FFI-based code.

gemini-code-assist · 2026-02-03T18:22:12Z

include/tvm/ffi/expected.h

+  /*! \brief Implicit constructor from an Unexpected wrapper. */
+  template <typename E, typename = std::enable_if_t<std::is_base_of_v<Error, std::remove_cv_t<E>>>>
+  // NOLINTNEXTLINE(google-explicit-constructor,runtime/explicit)
+  Expected(Unexpected<E> unexpected) : data_(Any(Error(std::move(unexpected).error()))) {}


The constructor for Expected from Unexpected<E> currently wraps the error in an Error() constructor call. This can lead to object slicing if E is a custom error type that derives from tvm::ffi::Error. When sliced, the specific type information of the custom error is lost, and it becomes a base Error object. This prevents callers from downcasting the error to its original derived type to handle specific error conditions.

By removing the Error() wrapper, the Any container will correctly store the derived error type, preserving its full information and allowing for proper polymorphic error handling.

Expected(Unexpected<E> unexpected) : data_(Any(std::move(unexpected).error())) {}

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

guan404ming changed the title ~~feat: Add ffi::Expected<T> for exception-free error handling~~ feat: add ffi::Expected<T> for exception-free error handling Jan 11, 2026

gemini-code-assist bot reviewed Jan 11, 2026

View reviewed changes

include/tvm/ffi/expected.h Outdated Show resolved Hide resolved

include/tvm/ffi/expected.h Outdated Show resolved Hide resolved

include/tvm/ffi/expected.h Show resolved Hide resolved

include/tvm/ffi/expected.h Show resolved Hide resolved

guan404ming force-pushed the feat/expected-type branch from 2f70b38 to f1a4dc9 Compare January 11, 2026 15:46

guan404ming marked this pull request as ready for review January 12, 2026 02:56

tqchen assigned Kathryn-cat Jan 14, 2026

Kathryn-cat suggested changes Jan 20, 2026

View reviewed changes

guan404ming force-pushed the feat/expected-type branch from f1a4dc9 to 486f68e Compare January 20, 2026 11:20

guan404ming requested a review from Kathryn-cat January 20, 2026 11:21

guan404ming force-pushed the feat/expected-type branch from 486f68e to ad48e1b Compare January 20, 2026 11:27

Kathryn-cat reviewed Jan 23, 2026

View reviewed changes

guan404ming requested a review from Kathryn-cat January 23, 2026 16:35

Kathryn-cat reviewed Jan 23, 2026

View reviewed changes

Kathryn-cat suggested changes Jan 24, 2026

View reviewed changes

guan404ming force-pushed the feat/expected-type branch from 806fe56 to e40b425 Compare January 24, 2026 04:47

Kathryn-cat approved these changes Jan 24, 2026

View reviewed changes

guan404ming force-pushed the feat/expected-type branch from e40b425 to ad1c187 Compare January 25, 2026 13:38

tqchen requested changes Jan 26, 2026

View reviewed changes

Ubospica approved these changes Feb 1, 2026

View reviewed changes

guan404ming force-pushed the feat/expected-type branch 2 times, most recently from d08c3d6 to 30466e2 Compare February 1, 2026 05:34

guan404ming requested a review from tqchen February 1, 2026 05:37

tqchen requested changes Feb 2, 2026

View reviewed changes

guan404ming force-pushed the feat/expected-type branch from f25d0a9 to cd632b2 Compare February 2, 2026 15:57

gemini-code-assist bot reviewed Feb 2, 2026

View reviewed changes

include/tvm/ffi/function.h Outdated Show resolved Hide resolved

tqchen requested changes Feb 2, 2026

View reviewed changes

guan404ming added 11 commits February 4, 2026 00:22

feat: Add ffi::Expected<T> for exception-free error handling

04011a4

refactor: apply review comments

3b0133a

refactor: apply review comments

8c8860e

Streamline error handling in FFI function calls

646676e

refactor: enabling implicit construction from values

90ff089

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

refactor: update implicit constructor annotations for clarity

17211ba

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

Add Unexpected

4ccf559

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

Add doc

0377865

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

Add Unexpected class

a7e8a88

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

Update to use try_cast

e68126f

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

Refactor based on apache#425

83d5812

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

guan404ming force-pushed the feat/expected-type branch from 96c2b5b to 83d5812 Compare February 3, 2026 17:03

tqchen requested changes Feb 3, 2026

View reviewed changes

gemini-code-assist bot reviewed Feb 3, 2026

View reviewed changes

guan404ming added 2 commits February 4, 2026 10:29

Remove is_expected_v

2adcd24

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

Fix implicit constructor in Expected

4148356

Signed-off-by: Guan-Ming Chiu <guanmingchiu@gmail.com>

guan404ming requested a review from tqchen February 4, 2026 03:13

feat: add ffi::Expected<T> for exception-free error handling #399

Are you sure you want to change the base?

feat: add ffi::Expected<T> for exception-free error handling #399

Conversation

guan404ming commented Jan 11, 2026

Why

How

Uh oh!

gemini-code-assist bot commented Jan 11, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tqchen commented Jan 12, 2026

Uh oh!

Kathryn-cat commented Jan 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guan404ming Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kathryn-cat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guan404ming Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

guan404ming Jan 24, 2026 •

edited

Loading

Kathryn-cat left a comment •

edited

Loading

guan404ming Jan 24, 2026 •

edited

Loading

Kathryn-cat Jan 26, 2026 •

edited

Loading