[Development] Proposal: QVariant/std::any QModelIndex::internalData() for Qt 7

[Bogdan Vatra]

Hi,

@Igor, @Volker: Thanks for your feedback!

QPersistentModelIndex is not a proper solution here, for a practical reason: 
are you going to use QPMI *everywhere*? We experience crashes not only in our 
code but inside Qt's own code, QSortFilterProxyModel, QIdentityProxyModel, 
etc. which rely on internalPointer for their own bookkeeping. No amount of 
QPMI in application code fixes that. Using QPMI everywhere is also quite 
hacky; it's like having a QPersistentString that I must use to check whether 
my QString is still valid...

On "QModelIndex should be thought of like an STL iterator": the analogy is 
forced, because with an STL container *I* have full control. The iterator is 
only invalidated when *my* code changes the container. With QAIM, framework 
code invalidates indices behind my back: any Q*ModelView or delegate may call 
QCoreApplication::processEvents(), and Qt's own signals carry QModelIndex by 
value across queued connections, so an index can already be stale at the 
moment it is delivered. A single posted dataChanged (or any structural signal) 
is enough to make internalPointer point to something that no longer exists. 
This is the daily reality of highly dynamic models, the current contract is 
not just hard to follow, Qt itself breaks it on my behalf.

> And the model knows when a model index might have been invalidated, and
> would need to somehow update the payload in constructed QModelIndex
> instances - what should QModelIndex::internalData() return if the index
> has been removed from the model?

It returns the QVariant. :) That's the point: the payload is a *value*, not a 
reference into model state, so there is nothing for the model to update. 
internalData() returns whatever was stored when the index was created, and 
accessing it is always safe, removed from the model or not. What that value 
*means* is the model's business: store a shared_ptr and the data stays alive 
as long as any index references it; store a weak_ptr and lock() failing tells 
you the item is gone, so you bail out gracefully. Compare with 
internalPointer(), where the model equally cannot update anything, but the 
result is a dangling pointer and UB.

> So what do we win?

Defined behavior! :)

> a view or other generic client code cannot generally assume anything
> about what internalData() might return

Of course, the problem is on the model side where we don't know if 
internalPointer is still valid or not. E.g. QSortFilterProxyModel *is* a 
model, and it is precisely where these crashes happen today. Qt's own proxies 
would be the first beneficiaries ;-) .

> This can already be done by anyone right now, just adding a Qt::UserRole
> to the model to return a key-for-index, and implementing a persistent
> mapping from key-to-item/index in their model.

We do exactly this today (an in-house bimap between ids and items), and yes, 
it works, a lookup miss is how we detect a stale index. But look at what it 
costs: a parallel data structure that must be maintained in lockstep with the 
model, CRUD on the map for every item created or destroyed, and a lookup on 
every single access, on the hottest path in model/view. And every author of a 
dynamic model has to hand-roll this same boilerplate; it is reference 
counting, reimplemented badly, once per model. A shared_ptr in the payload is 
the same validity guarantee for an O(1) refcount and zero bookkeeping. And it 
doesn't help Qt's own proxies either way: QSortFilterProxyModel cannot use my 
application-level bimap, so its internalPointer crashes remain no matter how 
good my map is.

On cost: rather than debate this in the abstract, I went ahead and implemented 
it for Qt 7: https://codereview.qt-project.org/c/qt/qtbase/+/749550. 
QModelIndex does lose trivial copyability, but an empty payload costs next to 
nothing at copy time, so only models that opt in actually pay. I'll attach 
benchmarks for the index-heavy paths (views, QSFPM) to the review, so we can 
settle the cost question on numbers rather than assumptions.

Cheers,
BogDan.