gemini-code-assist[bot] commented on code in PR #587: URL: https://github.com/apache/tvm-ffi/pull/587#discussion_r3235207179
########## src/ffi/extra/structural_error_context.cc: ########## @@ -0,0 +1,301 @@ +/* + * 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) {} + + 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; + + const List<ObjectRef>& records = context_->reverse_visit_pattern; + bool matched_step = + num_pattern_step_matched_ < records.size() && + node.same_as(records[static_cast<int64_t>(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; + } + } + + this->VisitChildrenFields(node, type_info); + + if (matched_step) --num_pattern_step_matched_; + } + + void VisitChildrenFields(ObjectRef node, const TVMFFITypeInfo* type_info) { + // Snapshot results_.size() before descent to detect any inner full or + // prefix match recorded by a deeper call. If results_ grew, some inner + // node was recorded — we do not record again here. + size_t results_before = results_.size(); + reflection::ForEachFieldInfo(type_info, [&](const TVMFFIFieldInfo* field_info) { + reflection::FieldGetter getter(field_info); + Any child_val = getter(node); + this->PushStep(TempAccessStep::Attr(field_info)); + this->VisitAny(std::move(child_val)); + this->PopStep(); + }); + + // Leaf-with-prefix-match: this node contributed a match step, but no + // inner result was recorded from its subtree. Record the current node's + // path as the best-effort prefix. path_steps_ reflects the path TO this + // node (the step leading here was pushed by our caller), so + // MaterializeAccessPath() yields the correct prefix path. + // Skip when path_steps_ is empty — AccessPath::Root() gives no useful info. + if (allow_prefix_match_ && num_pattern_step_matched_ > 0 && + num_pattern_step_matched_ < context_->reverse_visit_pattern.size() && + !path_steps_.empty() && results_.size() == results_before) { + results_.push_back(this->MaterializeAccessPath()); + } + } + + template <typename SeqType> + void VisitSequence(const SeqType& seq) { + for (size_t i = 0; i < seq.size(); ++i) { + Any item = seq[static_cast<int64_t>(i)]; + if (item == nullptr) continue; + this->PushStep(TempAccessStep::ArrayItem(static_cast<int64_t>(i))); + this->VisitAny(std::move(item)); + this->PopStep(); + } + } + + template <typename MapType> + void VisitMap(const MapType& m) { + for (const std::pair<Any, Any>& kv : m) { + if (kv.first == nullptr || kv.second == nullptr) continue; + this->PushStep(TempAccessStep::MapItem(kv.first)); + this->VisitAny(kv.second); + this->PopStep(); + } + } + + /*! \brief Append a TempAccessStep to the descent stack; cache unchanged. */ + void PushStep(TempAccessStep step) { path_steps_.push_back(std::move(step)); } + + /*! \brief Pop the top TempAccessStep; truncate materialized_paths_ to match. */ + void PopStep() { + path_steps_.pop_back(); + if (materialized_paths_.size() > path_steps_.size()) { + materialized_paths_.erase( + materialized_paths_.begin() + static_cast<std::ptrdiff_t>(path_steps_.size()), + materialized_paths_.end()); + } + } + + // Cache invariant maintained jointly with PushStep / PopStep: + // materialized_paths_[k] (when present) is the AccessPath built + // from path_steps_[0..k+1] as they currently exist, and + // materialized_paths_.size() <= path_steps_.size(). + // + // - PushStep: appends to path_steps_; cache unchanged. + // - PopStep: pops path_steps_; truncates cache to match. + // - MaterializeAccessPath: extends cache up to path_steps_.size(), + // chaining new AccessPath nodes via Extend. + // + // Lazy materialization avoids per-descent AccessPath allocation; + // cache amortizes across consecutive matches with shared prefix + // (LCA sharing). + /*! \brief Materialize the AccessPath at the current descent depth. */ + reflection::AccessPath MaterializeAccessPath() { + for (size_t idx = materialized_paths_.size(); idx < path_steps_.size(); ++idx) { + reflection::AccessPath parent = + (idx == 0) ? reflection::AccessPath::Root() : materialized_paths_[idx - 1]; + materialized_paths_.push_back(parent->Extend(path_steps_[idx].ToAccessStep())); + } + return materialized_paths_.back(); + } Review Comment:  The `MaterializeAccessPath` function will crash if `path_steps_` is empty. This occurs when the root node itself is a full match for the pattern (e.g., if the error occurred immediately at the start of the visit). In this case, `materialized_paths_` remains empty, and calling `.back()` results in undefined behavior or a crash. ```suggestion reflection::AccessPath MaterializeAccessPath() { if (path_steps_.empty()) return reflection::AccessPath::Root(); for (size_t idx = materialized_paths_.size(); idx < path_steps_.size(); ++idx) { reflection::AccessPath parent = (idx == 0) ? reflection::AccessPath::Root() : materialized_paths_[idx - 1]; materialized_paths_.push_back(parent->Extend(path_steps_[idx].ToAccessStep())); } return materialized_paths_.back(); } ``` ########## src/ffi/extra/structural_error_context.cc: ########## @@ -0,0 +1,301 @@ +/* + * 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) {} + + 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; + + const List<ObjectRef>& records = context_->reverse_visit_pattern; + bool matched_step = + num_pattern_step_matched_ < records.size() && + node.same_as(records[static_cast<int64_t>(records.size() - 1 - num_pattern_step_matched_)]); Review Comment:  Accessing `records` by index inside `VisitObject` can be inefficient if `List` is implemented as a linked list (which is common for the `List` container in this project). Since `VisitObject` is called for every node in the tree, this results in $O(N \cdot M)$ complexity where $N$ is the tree size and $M$ is the pattern length. Consider converting the `reverse_visit_pattern` to a `std::vector<ObjectRef>` once in the `StructuralErrorAccessPathFinder` constructor to ensure $O(1)$ access during traversal. ########## include/tvm/ffi/extra/structural_error_context.h: ########## @@ -0,0 +1,200 @@ +/* + * 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/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 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. + */ + List<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> extra_context = err.extra_context(); + if (extra_context) { + return extra_context->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, const ObjectRef& node) { // NOLINT(*) + std::optional<ObjectRef> extra_context = err.extra_context(); + if (extra_context) { + Optional<StructuralErrorContext> structural_context = + extra_context->as<StructuralErrorContext>(); + if (structural_context) { + structural_context.value()->reverse_visit_pattern.push_back(node); + return; + } + } + // Build a fresh StructuralErrorContext, preserving any pre-existing payload. + ObjectPtr<StructuralErrorContextObj> new_context = make_object<StructuralErrorContextObj>(); + new_context->reverse_visit_pattern = List<ObjectRef>{node}; + if (extra_context) new_context->previous_error_context = *extra_context; + + // Write directly to the ErrorObj cell using ObjectUnsafe patterns. + // This is safe here because we are the sole owner during exception unwind. + 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:  This code performs an in-place mutation of the `ErrorObj` by directly modifying its `extra_context` field. Since `ErrorObj` is logically immutable and `Error` is an `ObjectRef` (which can be shared), this mutation might lead to unexpected side effects if other parts of the code hold a reference to the same error object. While this is likely done for performance to avoid re-allocating the error during exception propagation, it bypasses the standard immutability guarantees of the object system. -- 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]
