gemini-code-assist[bot] commented on code in PR #586: URL: https://github.com/apache/tvm-ffi/pull/586#discussion_r3230767425
########## src/ffi/extra/structural_error_context.cc: ########## @@ -0,0 +1,289 @@ +/* + * 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/structural_error_context.cc + * \brief StructuralErrorContext 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/structural_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 StructuralErrorContext. + */ +class StructuralErrorAccessPathFinder { + public: + explicit StructuralErrorAccessPathFinder(StructuralErrorContext context, bool allow_prefix_match) + : context_(std::move(context)), + allow_prefix_match_(allow_prefix_match), + num_pattern_step_matched_(0) {} + + 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; + // Any has TypeTraits<void*> (kTVMFFIOpaquePtr) but not const void*. + // Cast away const for storage; restore on retrieval. + s.key = Any(const_cast<void*>(static_cast<const void*>(fi))); Review Comment:  Casting away `const` from `TVMFFIFieldInfo*` to store it in `Any` is slightly risky if `Any`'s destructor or other operations assume ownership or mutability. Since `TVMFFIFieldInfo` is typically static metadata, this is likely safe here, but it's a pattern to use with caution. Ensure that `Any` is used only for opaque pointer storage in this context. ########## include/tvm/ffi/extra/structural_error_context.h: ########## @@ -0,0 +1,197 @@ +/* + * 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/structural_error_context.h + * \brief StructuralErrorContext: typed payload for Error::extra_context that records the + * chain of ObjectRefs visited during a recursive structural visit when an error is thrown. + */ +#ifndef TVM_FFI_EXTRA_STRUCTURAL_ERROR_CONTEXT_H_ +#define TVM_FFI_EXTRA_STRUCTURAL_ERROR_CONTEXT_H_ + +#include <tvm/ffi/container/array.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 StructuralErrorContext. + * + * \sa StructuralErrorContext + */ +class StructuralErrorContextObj : public Object { + public: + /*! + * \brief Visit records that get populated, which include the object visit + * path pattern in innermost-first order. Best-effort — not exhaustive. + */ + Array<ObjectRef> reverse_visit_pattern; + + /*! + * \brief Pre-existing Error::extra_context payload before we placed the + * StructuralErrorContext. + */ + Optional<ObjectRef> previous_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.StructuralErrorContext", StructuralErrorContextObj, + Object); + /// \endcond +}; + +/*! + * \brief Typed payload attached to Error::extra_context to support + * structural-context-aware error reporting. + * + * The StructuralErrorContext 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 structural visit is instrumented with + * TVM_FFI_STRUCTURAL_VISIT_BEGIN / _END(node). On throw, the + * macros record the visited nodes into a StructuralErrorContext + * and attach it to the Error's extra_context. + * + * 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 StructuralErrorContext : public ObjectRef { + public: + /*! \brief Get the StructuralErrorContext attached to err's extra_context. + * \param err The error to inspect. + * \return The attached StructuralErrorContext, or NullOpt if absent. + */ + static Optional<StructuralErrorContext> TryGetFromError(const Error& err) { + std::optional<ObjectRef> ec = err.extra_context(); + if (ec) { + return ec->as<StructuralErrorContext>(); + } + return std::nullopt; + } + + /*! \brief Find all access paths that match the pattern specified in the + * StructuralErrorContext. + * \param root The root ObjectRef to search from. + * \param structural_context The StructuralErrorContext 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_EXTRA_CXX_API static Array<reflection::AccessPath> FindAccessPaths( + const ObjectRef& root, const StructuralErrorContext& structural_context, + bool allow_prefix_match = false); + + /// \cond Doxygen_Suppress + explicit StructuralErrorContext(ObjectPtr<StructuralErrorContextObj> n) + : ObjectRef(std::move(n)) {} + TVM_FFI_DEFINE_OBJECT_REF_METHODS_NOTNULLABLE(StructuralErrorContext, ObjectRef, + StructuralErrorContextObj); + /// \endcond +}; + +/*! + * \brief Begin a structural-visit try block. + * + * Must be paired with TVM_FFI_STRUCTURAL_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_STRUCTURAL_VISIT_BEGIN(); + * DispatchVisit(node); + * TVM_FFI_STRUCTURAL_VISIT_END(node); + * } + * \endcode + */ +#define TVM_FFI_STRUCTURAL_VISIT_BEGIN() try { +/*! + * \brief End a structural-visit try block and catch+re-throw any Error, + * appending node to the StructuralErrorContext on the way up. + * + * Must be paired with TVM_FFI_STRUCTURAL_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_STRUCTURAL_VISIT_END(node) \ + } \ + catch (::tvm::ffi::Error & _tvm_ffi_visit_err_) { \ + ::tvm::ffi::details::UpdateStructuralErrorContext(_tvm_ffi_visit_err_, (node)); \ + throw; \ + } + +namespace details { +/*! + * \brief Implementation helper for TVM_FFI_STRUCTURAL_VISIT_END(node). + * Calling convention may change; do not call directly from user code. + * + * \note Safe to call only while the Error is owned by the current thread during + * exception unwind, before any external observer (Python wrapper, logging + * hook) takes a reference. After the catch handler returns to wider scope, + * treat the error as immutable. Writes directly to ErrorObj::extra_context + * using ObjectUnsafe refcount-aware patterns (no separate fn-ptr needed). + */ +inline void UpdateStructuralErrorContext(Error err, ObjectRef node) { + std::optional<ObjectRef> ec = err.extra_context(); + if (ec) { + Optional<StructuralErrorContext> sec = ec->as<StructuralErrorContext>(); + if (sec) { + sec.value()->reverse_visit_pattern.push_back(node); Review Comment:  Mutating an `Array` member directly via `push_back` can be inefficient if the underlying `ArrayObj` is shared, as it will trigger a copy-on-write (COW) operation at every level of the recursive visit. While acceptable for error reporting where performance is less critical, consider using a `std::vector` internally during the accumulation phase if tree depths are expected to be very large. ########## src/ffi/extra/structural_error_context.cc: ########## @@ -0,0 +1,289 @@ +/* + * 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/structural_error_context.cc + * \brief StructuralErrorContext 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/structural_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 StructuralErrorContext. + */ +class StructuralErrorAccessPathFinder { + public: + explicit StructuralErrorAccessPathFinder(StructuralErrorContext context, bool allow_prefix_match) + : context_(std::move(context)), + allow_prefix_match_(allow_prefix_match), + num_pattern_step_matched_(0) {} + + 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; + // Any has TypeTraits<void*> (kTVMFFIOpaquePtr) but not const void*. + // Cast away const for storage; restore on retrieval. + 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) { + 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))); Review Comment:  Using `MoveFromAnyAfterCheck<Array<Any>>` on an `Any` that might contain a specialized `Array<T>` (like `Array<ObjectRef>`) assumes that the FFI `Array` implementation is type-erased or that `Array<Any>` is compatible with all `Array<T>` at the ABI level. If `Array<T>` is strict about its template parameter, this check might fail for specialized arrays. -- 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]
