refactor(Storage)!: typed storage dispatch foundation + capacity_/numpy ownership (#941)

[yingjerkao]

What this delivers (re-scoped)

Rebased onto current master (which now carries the Scalar→std::variant refactor via #1011) and narrowed to the foundation, in two commits:

1. Typed storage foundation (Its time: Typed Storage Dispatch #941) — StorageVariant generated from Type_list (single source of truth, compile-time exhaustiveness), storage_cast<T>, as_storage_variant(), storage_as_type_or_replace<T>, and typed StorageImplementation<T>::data() -> T*. Static-asserts pin the variant to the dtype list. Exercised directly by new Storage_test cases (no dependency on the linalg conversion).
2. capacity_ removed + numpy zero-copy capsule ownership (Its time: Typed Storage Dispatch #941 rulings 2, 4) — append/resize are exact realloc, O(n), numpy-like; Storage.numpy()/Tensor.numpy(share_mem=True) genuinely share via an intrusive_ptr-holding capsule (the leaky release() is gone).

What changed vs the original PR

The original #1015 also carried two commits that are now dropped:

• refactor(Scalar)!: PIMPL → std::variant — fully superseded by refactor(Scalar)!: replace PIMPL virtual dispatch with std::variant (#847, fixes #935) #1011 (the same refactor, in its final reviewed form, already on master).
• fix(linalg)!: convert CPU Add/Sub/Mul/Div/Cpr/Mod to typed dispatch + true-division — deferred to a follow-up PR. It collides head-on with master's fix(Tensor & UniTensor): support rank-zero tensors and zero extents #1026 (rank-zero refactor of all six arithmetic files + the init_broadcast_binary_output helper), fix(linalg): handle zero-extent tensors #1043 (zero-extent), and fix(linalg): replace lower-enum-index dtype selection with Type.type_promote #984 (type_promote). Correctly landing it means re-implementing the storage_cast dispatch and the breaking true-division (int/int → Double, Its time: Typed Storage Dispatch #941 §True division) on top of master's now-refactored arithmetic — a numerical change that warrants its own focused, reviewable PR against current master. The original work is preserved at commit ff30d0090 for that follow-up.

Conflict-resolution note

During the rebase, master's StorageTest.InitByPtrRejectsZeroLength (a safety test added after this PR branched) required _Init_byptr(ptr, 0) to always throw; the capacity_-removal commit had weakened that guard to #ifdef UNI_DEBUG (a release-build free() hazard). Kept the always-on guard.

Testing

Full CPU test_main 1784 tests, 1773 passed / 11 skipped / 0 failed on the openblas-cpu (non-ASan) preset; storage/numpy-ownership/append pytests pass; clang-format v14 clean.

Follow-ups (tracked separately)

CPU linalg typed-dispatch conversion + true-division (deferred here); GPU conversion (#1013).

🤖 Generated with Claude Code

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[ianmccul]

Wow!

[ianmccul]

why not value_type *start_ ?

[ianmccul]

Better implemented via a variant visitor

[ianmccul]

Better implemented as a variant visitor

[ianmccul]

Better implemented as a variant visitor

[ianmccul]

Better if this returns value_type. Higher level operation on Storage_base layer can turn it into Scalar as needed.

Scalar Storage::get_item(idx) {
  return visit_storage_value(..., [](auto v) { return Scalar(v); });
}

Why not name it operator[] ?

[ianmccul]

I think its possible to remove this function? It seems to be dead code? Even before this PR, grep -rn _Init_byptr_safe the only hits are these lines.

[ianmccul]

This has to go. If for no other reason, the extern __SII is an order-of-initialization bug #673 and resolving that is a key goal of #941

Replace with something like

boost::intrusive_ptr<Storage_base> make_storage(unsigned int dtype) {
  constexpr std::size_t n_non_void = std::variant_size_v<Type_list> - 1;
  return internal::make_storage_impl(dtype, std::make_index_sequence<n_non_void>{});
}

which could be implemented using the same std::index_sequence pattern already used for as_storage_variant():

template <std::size_t... Is>
boost::intrusive_ptr<Storage_base>
make_storage_impl(unsigned int dtype, std::index_sequence<Is...>) {
  boost::intrusive_ptr<Storage_base> out;
  bool matched = (... || [&] {
    using T = std::variant_alternative_t<Is + 1, Type_list>; // skip void
    if (dtype == Type_class::cy_typeid_v<T>) {
      out = new StorageImplementation<T>();
      return true;
    }
    return false;
  }());
  cytnx_error_msg(!matched, "[Storage] invalid dtype %u%s", dtype, "\n");
  return out;
}

[ianmccul]

Split into two helpers:

boost::intrusive_ptr<Storage_base>
make_storage(unsigned int dtype);

boost::intrusive_ptr<Storage_base>
make_storage(unsigned int dtype, cytnx_uint64 size, int device = Device.cpu,
             bool init_zero = true);

Then for typed C++ sources:

template <CytnxType T>
boost::intrusive_ptr<StorageImplementation<T>>
make_storage(cytnx_uint64 size, int device = Device.cpu, bool init_zero = true) {
  auto out = boost::intrusive_ptr<StorageImplementation<T>>(new StorageImplementation<T>());
  out->Init(size, device, init_zero);
  return out;
}

For from_vector, the cleanest version does not need dtype dispatch at all because T is already known:

template <CytnxType T>
void _from_vector(const std::vector<T>& vin, int device = Device.cpu) {
  auto impl = make_storage<T>(vin.size(), device, false);

  if constexpr (std::is_same_v<T, cytnx_bool>) {
    impl->_cpy_bool(impl->data(), vin);
  } else {
    std::memcpy(impl->data(), vin.data(), sizeof(T) * vin.size());
  }

  this->_impl = impl;
}

[ianmccul]

even better to remove ->Init() and pass size, device and init_zero into the constructor

[ianmccul]

also needs to go, same pattern as for Storage_init_interface

[ianmccul]

Change to visitor pattern

[ianmccul]

Once this has been through another iteration to resolve the above issues, I'll work on a PR for #1003

[pcchen]

Code Review

Verdict: Request changes. The architecture is right, the deletion count (−16.4k) is glorious, and this unblocks #1003/#941 — but there are two verified HIGH runtime defects, plus a needed rebase. Both defects are line-verified against the branch head d2893495.

HIGH-1: every GPU integer÷integer Div now throws at runtime

• src/linalg/Div.cpp allocates the output as make_floating_point_dtype(type_promote(...)) → Double for int/int — correct, matches this PR's CPU true-division semantics.
• But the new cuDiv_dispatch_typed<TL,TR> (cuDiv_dispatch.cu) computes TO = Type_class::type_promote_gpu_t<TL,TR> — the plain promotion (Type.hpp:471; Int64 for int/int, no floating-point flooring) — and then hard-errors on mismatch:
  cytnx_error_msg(out->dtype() != cy_typeid_gpu_v<TO>, "[cuDiv_dispatch] output dtype mismatch...").
• Net effect: (integer tensor on GPU) / (integer tensor or scalar) throws "output dtype mismatch" on every call.
• One-line fix: using TO = Type_class::make_floating_point_t<Type_class::type_promote_gpu_t<TL, TR>>; — mirroring the CPU visitor in Div.cpp.
• ⚠️ CI cannot catch this: there is no GPU CI (#538). It will only surface on a user's machine.

HIGH-2: numpy zero-copy view → use-after-free on resize/append

• The capsule design (ruling 4) correctly keeps the Storage_base object alive as long as the numpy array.
• But Storage::resize (Storage.hpp:793) calls _impl->resize(), which does free(this->start_); this->start_ = htmp; on the same impl (StorageImplementation.cpp). The numpy array captured the old data pointer at view creation → dangling after any size change. Ruling 2 (capacity_ removed; every size change reallocates) makes the window maximal — even a single append triggers it.
• pytests/numpy_ownership_test.py's docstring claims safety on the premise that "every size-changing Storage operation replaces the impl with a fresh buffer" — that premise is false: the impl is mutated in place; only the buffer inside it is replaced.
• Repro:

  s = cytnx.Storage(5, cytnx.Type.Double)
  arr = s.numpy()   # zero-copy view
  s.resize(10)      # frees the old buffer in place
  arr[0]            # use-after-free; arr[0] = 1 → heap corruption

• Fix options, in order of preference: (a) make size-changing ops actually swap in a fresh impl, which makes the docstring's safety argument true and matches the evident ruling-4 intent; (b) mark the storage borrowed after numpy() and error on size changes; (c) copy instead of view. Whichever is chosen, add a regression test that resizes under a live view.

Interaction with @ianmccul's comments

His 11 inline comments (visitor-pattern conversions, make_storage() factory replacing the __SII extern globals — which is the #673 order-of-initialization bug that #941 exists to fix — value_type *start_, dead _Init_byptr_safe, get_item → value_type/operator[], constructor-instead-of-Init()) are all orthogonal to the two defects above and all worth doing; nothing here conflicts with them. Since #1003 is queued behind this PR, resolving both lists in one iteration keeps that path short.

Housekeeping

• The PR is CONFLICTING — master has moved since 07-07 (#997, #1010, #1022, #1024, #1031). Rebase needed before the next iteration.
• Commit ba822de1 (Scalar → std::variant) overlaps #1011's territory — a sentence on how this sequences with #1011 would prevent double-work.

Posted by Claude Code on behalf of @pcchen

[ianmccul]

My recommendation is rebase, and have claude address the two Code Review comments, and also the comments I made - they are all straightforward and might as well get it done in one, rather than churn through another PR.

I wouldn't worry too much about HIGH-1: it will get rewritten with #1003 anyway; an emergency fix would be to revert to the old behavior, but if doing that please make a comment on #1003 that the existing behavior is incorrect.

[pcchen]

Plan for the next iteration + coordination with #1011

Agreed with @ianmccul's recommendation above — one combined iteration rather than churning through another round. Claude Code will execute it on this branch. Concretely, the iteration will cover:

1. Rebase onto current master (refactor(pybind)!: guard UniTensor +/- on matching metadata; drop * / (#934) #997, refactor(Symmetry): make Symmetry a plain value type over std::variant #1010, refactor(UniTensor): add versioned file header #1022, fix(UniTensor): read file names without requiring null terminator #1024, fix(UniTensor): make clone_meta() exception-safe to stop leaking on validation throws #1031 have landed since 07-07).
2. HIGH-1 (GPU integer Div): fixed properly rather than reverted — it's one line in cuDiv_dispatch_typed: using TO = Type_class::make_floating_point_t<Type_class::type_promote_gpu_t<TL, TR>>;, mirroring the CPU visitor in Div.cpp. That's less work than the revert-plus-Merge CUDA unary and binary elementwise operations through one typed kernel framework #1003-note and keeps GPU semantics consistent with this PR's CPU true-division. (Merge CUDA unary and binary elementwise operations through one typed kernel framework #1003 rewrites it later either way.)
3. HIGH-2 (numpy view UAF): make size-changing Storage operations swap in a fresh impl instead of mutating the buffer in place — which makes numpy_ownership_test.py's safety argument actually true — plus a regression test that resizes under a live view.
4. @ianmccul's 11 inline comments: visitor-pattern conversions, make_storage() factory replacing the __SII extern globals (Segmentation fault when initializing Storage in C++ #673), value_type *start_, dead _Init_byptr_safe removal, get_item → value_type/operator[], constructor-instead-of-Init().

Sequencing with #1011: this PR's first commit (ba822de1) is a snapshot of #1011 — same refactor, same title — with the Storage work stacked on top, and the snapshot is already one iteration stale (#1011 was revised 07-08 with the visit-variant astype dispatch and in-place-promotion fixes). To avoid restacking on a moving target, the order is: #1011 merges first, then the combined iteration here — the rebase then drops the embedded Scalar commit entirely and this PR shrinks to its true Storage-only scope. Until then, please route Scalar review comments to #1011 and Storage/dispatch/linalg comments here; the Scalar.hpp/Scalar.cpp hunks in this diff are the stale copy.

Posted by Claude Code on behalf of @pcchen

[codecov]

Codecov Report

❌ Patch coverage is 97.22222% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.49%. Comparing base (6600059) to head (9afe152).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/backend/Storage.cpp	92.85%	1 Missing ⚠️
src/backend/StorageImplementation.cpp	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   60.38%   60.49%   +0.11%     
==========================================
  Files         229      229              
  Lines       31866    31873       +7     
  Branches       71       71              
==========================================
+ Hits        19241    19282      +41     
+ Misses      12604    12570      -34     
  Partials       21       21              

Flag	Coverage Δ
cpp	60.45% <97.22%> (+0.11%)	⬆️
python	64.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
C++ backend	58.04% <96.15%> (+<0.01%)	⬆️
Python bindings	75.01% <100.00%> (+0.75%)	⬆️
Python package	64.13% <ø> (ø)

Files with missing lines	Coverage Δ
include/Type.hpp	97.77% <ø> (ø)
include/backend/Storage.hpp	98.90% <100.00%> (+2.25%)	⬆️
pybind/storage_py.cpp	81.85% <100.00%> (+13.98%)	⬆️
pybind/tensor_py.cpp	68.70% <100.00%> (+0.20%)	⬆️
src/backend/utils_internal_cpu/Movemem_cpu.cpp	85.00% <100.00%> (ø)
src/backend/Storage.cpp	69.08% <92.85%> (+1.72%)	⬆️
src/backend/StorageImplementation.cpp	64.90% <93.75%> (-2.15%)	⬇️

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6600059...9afe152. Read the comment docs.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.