gemini-code-assist[bot] commented on code in PR #587: URL: https://github.com/apache/tvm-ffi/pull/587#discussion_r3282271847
########## include/tvm/ffi/extra/visit_error_context.h: ########## @@ -0,0 +1,255 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/*! + * \file tvm/ffi/extra/visit_error_context.h + * \brief VisitErrorContext: typed payload for Error::extra_context that records the + * chain of ObjectRefs visited during a recursive visit when an error is thrown. + */ +#ifndef TVM_FFI_EXTRA_VISIT_ERROR_CONTEXT_H_ +#define TVM_FFI_EXTRA_VISIT_ERROR_CONTEXT_H_ + +#include <tvm/ffi/container/array.h> +#include <tvm/ffi/container/list.h> +#include <tvm/ffi/error.h> +#include <tvm/ffi/extra/base.h> +#include <tvm/ffi/memory.h> +#include <tvm/ffi/object.h> +#include <tvm/ffi/optional.h> +#include <tvm/ffi/reflection/access_path.h> + +namespace tvm { +namespace ffi { + +/*! + * \brief Object class for VisitErrorContext. + * + * \sa VisitErrorContext + */ +class VisitErrorContextObj : public Object { + public: + /*! + * \brief Visit records that get populated, which include the object visit + * path pattern in innermost-first order. Best-effort — not exhaustive. + */ + List<ObjectRef> reverse_visit_pattern; + + /*! + * \brief Pre-existing Error::extra_context payload before we placed the + * VisitErrorContext. + */ + Optional<ObjectRef> prev_error_context; + + /// \cond Doxygen_Suppress + static constexpr bool _type_mutable = true; + static constexpr TVMFFISEqHashKind _type_s_eq_hash_kind = kTVMFFISEqHashKindUnsupported; + TVM_FFI_DECLARE_OBJECT_INFO_FINAL("ffi.VisitErrorContext", VisitErrorContextObj, Object); + /// \endcond +}; + +/*! + * \brief Typed payload attached to Error::extra_context to support + * visit-context-aware error reporting. + * + * The VisitErrorContext captures the reverse_visit_pattern — + * the chain of nodes visited before an error was thrown — so callers + * can translate it via FindAccessPaths into a structured access path + * for richer error messages. + * + * Typical usage: + * + * 1. A recursive visit is instrumented with + * TVM_FFI_VISIT_BEGIN / _END(node). The deepest detection + * point throws via TVM_FFI_VISIT_THROW(kind, node), which + * seeds the context with the throw site; enclosing BEGIN/END pairs + * append parent nodes on rethrow. + * + * 2. The root catch handler retrieves the context via + * TryGetFromError(err), then resolves the chain into one or more + * reflection::AccessPath instances via FindAccessPaths(root, ctx). + * + * 3. The caller uses the AccessPath to enrich the error message + * with structured position info (e.g., ".body[2].cond.lhs"). + */ +class VisitErrorContext : public ObjectRef { + public: + /*! \brief Get the VisitErrorContext attached to err's extra_context. + * \param err The error to inspect. + * \return The attached VisitErrorContext, or NullOpt if absent. + */ + TVM_FFI_COLD_CODE + static Optional<VisitErrorContext> TryGetFromError(const Error& err) { + std::optional<ObjectRef> extra_context = err.extra_context(); + if (extra_context) { + return extra_context->as<VisitErrorContext>(); + } + return std::nullopt; + } + + /*! \brief Find all access paths that match the pattern specified in the + * VisitErrorContext. + * \param root The root ObjectRef to search from. + * \param visit_context The VisitErrorContext to match against. + * \param allow_prefix_match If true, also report paths where only a + * prefix of the pattern was matched (i.e., + * the algorithm descended through some + * matched records but could not find further + * matches before reaching a leaf). Default + * false — only full pattern matches are + * reported. + * \return Array of matched access paths. + */ + TVM_FFI_COLD_CODE + TVM_FFI_EXTRA_CXX_API static Array<reflection::AccessPath> FindAccessPaths( + const ObjectRef& root, const VisitErrorContext& visit_context, + bool allow_prefix_match = false); + + /// \cond Doxygen_Suppress + explicit VisitErrorContext(ObjectPtr<VisitErrorContextObj> n) : ObjectRef(std::move(n)) {} + TVM_FFI_DEFINE_OBJECT_REF_METHODS_NOTNULLABLE(VisitErrorContext, ObjectRef, VisitErrorContextObj); + /// \endcond +}; + +/*! + * \brief Begin a visit try block. + * + * Must be paired with TVM_FFI_VISIT_END(node) at the end of the + * visit body. Expands to an open `try {` — a mismatched _END macro is a + * compile error (unclosed try block). + * + * \code{.cpp} + * void MyVisitor::VisitNode(const ObjectRef& node) { + * TVM_FFI_VISIT_BEGIN(); + * DispatchVisit(node); + * TVM_FFI_VISIT_END(node); + * } + * \endcode + */ +#define TVM_FFI_VISIT_BEGIN() try { +/*! + * \brief End a visit try block and catch+re-throw any Error, + * appending node to the VisitErrorContext on the way up. + * + * Must be paired with TVM_FFI_VISIT_BEGIN() above the visit body. + * + * \param node The ObjectRef at the current visit level (appended to the + * error context's reverse_visit_pattern on exception). + */ +#define TVM_FFI_VISIT_END(node) \ + } \ + catch (::tvm::ffi::Error & _tvm_ffi_visit_err_) { \ + ::tvm::ffi::details::UpdateVisitErrorContext(_tvm_ffi_visit_err_, (node)); \ + throw; \ + } + +/*! + * \brief Throw an error from inside a visit, with `node` recorded + * as the innermost frame of the resulting VisitErrorContext. + * + * Use this when the bad spot is somewhere the BEGIN/END pair does not + * already record — typically a child field of the currently-visited node, + * or a helper called from a visit that has no BEGIN/END of its own. The + * throw site is seeded as the innermost frame; enclosing + * TVM_FFI_VISIT_BEGIN/END pairs continue to append their nodes + * on rethrow as the stack unwinds. The macro mirrors TVM_FFI_THROW — + * it returns an ostream you stream a message into. + * + * If `node` here is the same as the enclosing END's node (a redundant + * throw at the same level), FindAccessPaths normalizes the consecutive + * duplicate during matching, so user code does not need to guard against + * it — but in that case the throw would have been recorded by END + * anyway and a plain TVM_FFI_THROW would suffice. + * + * \code{.cpp} + * // Visiting a TPair node; pin the bad subfield (.lhs) as the throw + * // site so the resulting AccessPath ends at .lhs, not at the TPair + * // node itself. The surrounding END appends `node` as the next frame. + * void Visitor::Visit(const ObjectRef& node) { + * TVM_FFI_VISIT_BEGIN(); + * if (auto pair = node.as<TPair>()) { + * if (!IsValid(pair.value()->lhs)) { + * TVM_FFI_VISIT_THROW(ValueError, pair.value()->lhs) + * << "invalid lhs"; + * } + * } + * TVM_FFI_VISIT_END(node); + * } + * \endcode + * + * \param ErrorKind The kind of error to throw (e.g. TypeError, ValueError). + * \param node The ObjectRef at the throw site (innermost frame). + */ +#define TVM_FFI_VISIT_THROW(ErrorKind, node) \ + ::tvm::ffi::details::ErrorBuilder( \ + #ErrorKind, TVMFFIBacktrace(__FILE__, __LINE__, TVM_FFI_FUNC_SIG, 0), \ + TVM_FFI_ALWAYS_LOG_BEFORE_THROW, ::std::nullopt, \ + ::std::optional<::tvm::ffi::ObjectRef>(::tvm::ffi::details::MakeVisitErrorContext(node))) \ + .stream() + +namespace details { +/*! + * \brief Build a fresh VisitErrorContext seeded with `node` as the + * innermost (and currently only) frame of reverse_visit_pattern. + * + * Used by TVM_FFI_VISIT_THROW to attach the throw-site + * node to the Error's extra_context at construction time. + */ +TVM_FFI_COLD_CODE +inline VisitErrorContext MakeVisitErrorContext(const ObjectRef& node) { + ObjectPtr<VisitErrorContextObj> obj = make_object<VisitErrorContextObj>(); + obj->reverse_visit_pattern = List<ObjectRef>{node}; + return VisitErrorContext(std::move(obj)); +} + +/*! + * \brief Implementation helper for TVM_FFI_VISIT_END(node). + * Calling convention may change; do not call directly from user code. + */ +TVM_FFI_COLD_CODE +inline void UpdateVisitErrorContext(Error& err, const ObjectRef& node) { // NOLINT(*) + // NOTE: This function mutates the ErrorObj in place via ObjectUnsafe. + // Expected to run only inside the exception throw chain, where the Error + // is single-owned by this thread. The tradeoff avoids reallocating a + // fresh Error per catch frame; the immutability invariant returns once + // the unwind window closes. + std::optional<ObjectRef> extra_context = err.extra_context(); + if (extra_context) { + Optional<VisitErrorContext> visit_context = extra_context->as<VisitErrorContext>(); + if (visit_context) { + visit_context.value()->reverse_visit_pattern.push_back(node); + return; + } + } + // Build a fresh VisitErrorContext, preserving any pre-existing payload. + ObjectPtr<VisitErrorContextObj> new_context = make_object<VisitErrorContextObj>(); + new_context->reverse_visit_pattern = List<ObjectRef>{node}; + if (extra_context) new_context->prev_error_context = *extra_context; + + ErrorObj* error_obj = + static_cast<ErrorObj*>(details::ObjectUnsafe::RawObjectPtrFromObjectRef(err)); + if (error_obj->extra_context != nullptr) { + details::ObjectUnsafe::DecRefObjectHandle(error_obj->extra_context); + } + error_obj->extra_context = + details::ObjectUnsafe::MoveObjectPtrToTVMFFIObjectPtr(std::move(new_context)); +} Review Comment:  The `UpdateVisitErrorContext` function mutates the `ErrorObj` and its `VisitErrorContext` payload in-place. While this is an efficient optimization for the exception unwinding path (avoiding repeated allocations), it technically violates the immutability contract typically expected of `ObjectRef` types in this framework. If an `Error` object were captured and shared (e.g., by a custom catch block that doesn't immediately rethrow), this mutation could lead to unexpected side effects in other holders of the reference. Given the specific usage in the `TVM_FFI_VISIT_END` macro, this is likely safe, but it is a subtle point that might affect future extensions or custom error handling logic. ########## src/ffi/extra/visit_error_context.cc: ########## @@ -0,0 +1,331 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/* + * \file src/ffi/extra/visit_error_context.cc + * \brief VisitErrorContext implementation — breadcrumb-trail collection and access-path + * extraction for recursive Object visits. + */ +#include <tvm/ffi/container/array.h> +#include <tvm/ffi/container/dict.h> +#include <tvm/ffi/container/list.h> +#include <tvm/ffi/container/map.h> +#include <tvm/ffi/extra/visit_error_context.h> +#include <tvm/ffi/memory.h> +#include <tvm/ffi/reflection/accessor.h> +#include <tvm/ffi/string.h> + +#include <utility> +#include <vector> + +namespace tvm { +namespace ffi { + +/** + * \brief Internal handler for finding all access paths in a root ObjectRef + * that match the breadcrumb pattern stored in a VisitErrorContext. + */ +class VisitErrorAccessPathFinder { + public: + TVM_FFI_COLD_CODE + explicit VisitErrorAccessPathFinder(VisitErrorContext context, bool allow_prefix_match) + : context_(std::move(context)), allow_prefix_match_(allow_prefix_match) { + // Normalize the breadcrumb pattern before matching. Two kinds of noise can + // accumulate in reverse_visit_pattern at recording time: + // + // 1. Consecutive duplicates of the same ObjectRef. This is expected when + // TVM_FFI_VISIT_THROW(kind, node) and an enclosing + // TVM_FFI_VISIT_END(node) at the same level both record + // the same `node` — the throw site seeds the context with `node`, and + // the catch handler immediately above appends `node` again. Treat any + // run of identical adjacent entries as a single frame. + // + // 2. Null / undefined ObjectRefs. These can leak in when a visited node + // was mutated or torn down between recording and matching, leaving a + // stale slot. We can never match a null pointer against a live tree, + // so drop them silently rather than letting them break the chain. + // + // The matcher operates on `records_` from here on; the original + // `context_->reverse_visit_pattern` is left untouched so callers that + // inspect the context for debugging still see the raw history. + const List<ObjectRef>& raw = context_->reverse_visit_pattern; + records_.reserve(raw.size()); + for (const ObjectRef& entry : raw) { + if (!entry.defined()) continue; + if (!records_.empty() && entry.same_as(records_.back())) continue; + records_.push_back(entry); + } + } + + TVM_FFI_COLD_CODE + Array<reflection::AccessPath> Find(const ObjectRef& root) { + this->VisitObject(root); + return Array<reflection::AccessPath>(results_.begin(), results_.end()); + } + + private: + /** + * \brief Stack-allocated mirror of AccessStepObj used during the descent hot path. + * For kAttr: stores const TVMFFIFieldInfo* encoded as void* in key (via + * kTVMFFIOpaquePtr). String allocation is deferred to ToAccessStep(). + * Not an Object — pure struct, no Object header / refcount. + */ + struct TempAccessStep { + reflection::AccessKind kind; + Any key{}; // For kAttr: FieldInfo pointer encoded as void* (kTVMFFIOpaquePtr). + // For kArrayItem: int64 index. + // For kMapItem: the user's key. + + static TempAccessStep Attr(const TVMFFIFieldInfo* fi) { + TempAccessStep s; + s.kind = reflection::AccessKind::kAttr; + // We store `fi` as an OPAQUE POINTER inside `Any` (kTVMFFIOpaquePtr). + // `Any` is used here purely as a value-carrying slot for the bits of + // the pointer — it does NOT take ownership, does NOT dereference + // through it, and does NOT mutate the pointee. The FieldInfo struct + // is read-only metadata owned by the type-info registry and lives + // for the lifetime of the type info, so the underlying object is + // unaffected by the const_cast below. + // + // The const_cast is needed only because `Any` provides TypeTraits for + // `void*` but not for `const void*`. On retrieval (ToAccessStep() + // below) we cast back to `const TVMFFIFieldInfo*` before any use. + s.key = Any(const_cast<void*>(static_cast<const void*>(fi))); Review Comment:  The use of `const_cast` to store a `const TVMFFIFieldInfo*` as a `void*` in `Any` is a workaround for the current `Any` implementation's lack of `const void*` support. While safe here because the pointer is restored to its original `const` qualifier before any use (line 133), it bypasses type safety. If the `Any` class can be updated to support `const void*` or if a more type-safe way to store metadata pointers exists in the framework, that would be a better long-term solution. ########## src/ffi/extra/visit_error_context.cc: ########## @@ -0,0 +1,331 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/* + * \file src/ffi/extra/visit_error_context.cc + * \brief VisitErrorContext implementation — breadcrumb-trail collection and access-path + * extraction for recursive Object visits. + */ +#include <tvm/ffi/container/array.h> +#include <tvm/ffi/container/dict.h> +#include <tvm/ffi/container/list.h> +#include <tvm/ffi/container/map.h> +#include <tvm/ffi/extra/visit_error_context.h> +#include <tvm/ffi/memory.h> +#include <tvm/ffi/reflection/accessor.h> +#include <tvm/ffi/string.h> + +#include <utility> +#include <vector> + +namespace tvm { +namespace ffi { + +/** + * \brief Internal handler for finding all access paths in a root ObjectRef + * that match the breadcrumb pattern stored in a VisitErrorContext. + */ +class VisitErrorAccessPathFinder { + public: + TVM_FFI_COLD_CODE + explicit VisitErrorAccessPathFinder(VisitErrorContext context, bool allow_prefix_match) + : context_(std::move(context)), allow_prefix_match_(allow_prefix_match) { + // Normalize the breadcrumb pattern before matching. Two kinds of noise can + // accumulate in reverse_visit_pattern at recording time: + // + // 1. Consecutive duplicates of the same ObjectRef. This is expected when + // TVM_FFI_VISIT_THROW(kind, node) and an enclosing + // TVM_FFI_VISIT_END(node) at the same level both record + // the same `node` — the throw site seeds the context with `node`, and + // the catch handler immediately above appends `node` again. Treat any + // run of identical adjacent entries as a single frame. + // + // 2. Null / undefined ObjectRefs. These can leak in when a visited node + // was mutated or torn down between recording and matching, leaving a + // stale slot. We can never match a null pointer against a live tree, + // so drop them silently rather than letting them break the chain. + // + // The matcher operates on `records_` from here on; the original + // `context_->reverse_visit_pattern` is left untouched so callers that + // inspect the context for debugging still see the raw history. + const List<ObjectRef>& raw = context_->reverse_visit_pattern; + records_.reserve(raw.size()); + for (const ObjectRef& entry : raw) { + if (!entry.defined()) continue; + if (!records_.empty() && entry.same_as(records_.back())) continue; + records_.push_back(entry); + } + } + + TVM_FFI_COLD_CODE + Array<reflection::AccessPath> Find(const ObjectRef& root) { + this->VisitObject(root); + return Array<reflection::AccessPath>(results_.begin(), results_.end()); + } + + private: + /** + * \brief Stack-allocated mirror of AccessStepObj used during the descent hot path. + * For kAttr: stores const TVMFFIFieldInfo* encoded as void* in key (via + * kTVMFFIOpaquePtr). String allocation is deferred to ToAccessStep(). + * Not an Object — pure struct, no Object header / refcount. + */ + struct TempAccessStep { + reflection::AccessKind kind; + Any key{}; // For kAttr: FieldInfo pointer encoded as void* (kTVMFFIOpaquePtr). + // For kArrayItem: int64 index. + // For kMapItem: the user's key. + + static TempAccessStep Attr(const TVMFFIFieldInfo* fi) { + TempAccessStep s; + s.kind = reflection::AccessKind::kAttr; + // We store `fi` as an OPAQUE POINTER inside `Any` (kTVMFFIOpaquePtr). + // `Any` is used here purely as a value-carrying slot for the bits of + // the pointer — it does NOT take ownership, does NOT dereference + // through it, and does NOT mutate the pointee. The FieldInfo struct + // is read-only metadata owned by the type-info registry and lives + // for the lifetime of the type info, so the underlying object is + // unaffected by the const_cast below. + // + // The const_cast is needed only because `Any` provides TypeTraits for + // `void*` but not for `const void*`. On retrieval (ToAccessStep() + // below) we cast back to `const TVMFFIFieldInfo*` before any use. + s.key = Any(const_cast<void*>(static_cast<const void*>(fi))); + return s; + } + + static TempAccessStep ArrayItem(int64_t index) { + TempAccessStep s; + s.kind = reflection::AccessKind::kArrayItem; + s.key = Any(index); + return s; + } + + static TempAccessStep MapItem(Any k) { + TempAccessStep s; + s.kind = reflection::AccessKind::kMapItem; + s.key = std::move(k); + return s; + } + + /*! \brief Materialize the heap-allocated AccessStep. Called only at match time. + * The String allocation for fi->name happens HERE, once. */ + reflection::AccessStep ToAccessStep() const { + if (kind == reflection::AccessKind::kAttr) { + // Recover the original `const TVMFFIFieldInfo*` from the opaque + // pointer stored in `key`. The bits round-trip unchanged; the + // const-qualifier is restored here, immediately before any use. + const TVMFFIFieldInfo* fi = static_cast<const TVMFFIFieldInfo*>(key.cast<void*>()); + return reflection::AccessStep::Attr(String(fi->name.data, fi->name.size)); + } else if (kind == reflection::AccessKind::kArrayItem) { + return reflection::AccessStep::ArrayItem(key.cast<int64_t>()); + } else { + // kMapItem + return reflection::AccessStep::MapItem(key); + } + } + }; + + void VisitAny(Any value) { + // Skip null Any silently — error-path defensive. + if (value == nullptr) return; + const int32_t type_index = value.type_index(); + if (type_index < TypeIndex::kTVMFFIStaticObjectBegin) { + // Primitive — cannot hold an ObjectRef chain entry. + return; + } + switch (type_index) { + case TypeIndex::kTVMFFIArray: + this->VisitSequence( + details::AnyUnsafe::MoveFromAnyAfterCheck<Array<Any>>(std::move(value))); + break; + case TypeIndex::kTVMFFIList: + this->VisitSequence(details::AnyUnsafe::MoveFromAnyAfterCheck<List<Any>>(std::move(value))); + break; + case TypeIndex::kTVMFFIMap: + this->VisitMap(details::AnyUnsafe::MoveFromAnyAfterCheck<Map<Any, Any>>(std::move(value))); + break; + case TypeIndex::kTVMFFIDict: + this->VisitMap(details::AnyUnsafe::MoveFromAnyAfterCheck<Dict<Any, Any>>(std::move(value))); + break; + default: + if (type_index >= TypeIndex::kTVMFFIStaticObjectBegin) { + ObjectRef obj = details::AnyUnsafe::MoveFromAnyAfterCheck<ObjectRef>(std::move(value)); + this->VisitObject(obj); + } + break; + } + } + + void VisitObject(const ObjectRef& node) { + // Defensive: error path; never throw. + if (!node.defined()) return; + const TVMFFITypeInfo* type_info = TVMFFIGetTypeInfo(node->type_index()); + if (type_info == nullptr || type_info->metadata == nullptr) return; + + bool matched_step = num_pattern_step_matched_ < records_.size() && + node.same_as(records_[records_.size() - 1 - num_pattern_step_matched_]); + if (matched_step) { + ++num_pattern_step_matched_; + if (num_pattern_step_matched_ == records_.size()) { + // Full match — materialize the AccessPath and record. + results_.push_back(this->MaterializeAccessPath()); + --num_pattern_step_matched_; + return; + } Review Comment:  In `VisitObject`, the search returns immediately after finding a full pattern match. This prevents the algorithm from finding nested occurrences of the same pattern (e.g., if the same sequence of object pointers appears again deeper in the tree). While this is generally safe for tree structures and avoids redundant matches, it limits the search to the outermost matches in each branch. If exhaustive matching of all possible occurrences (including nested ones) is required, the early return should be removed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
