[Development] RFC: QVariant changes in Qt6

[Lars Knoll]

Hi Olivier,

Thanks for looking through this and coming up with a proposal. I like the direction.

> On 22 Nov 2019, at 14:32, Olivier Goffart <olivier at woboq.com> wrote:
> 
> Hi,
> 
> This is a follow-up on what was discussed in the (second part of the) QtCore session in the QtCS.
> Lars and others have been mentioning that they dislike implicit conversions within QVariant. Creating a new class (QAny) has been suggested, that would be like QVariant but without the conversions.
> I am personally not in favor of this change because we are using QVariant all over the place in our API and so we cannot really deprecate QVariant. It will cause much confusion to user to have two similar classes. And the difference is not big enough to force a new class.
> 
> So here is what I suggest we do in Qt6. None of this is implemented yet, it is only proposed on this list for feedback.
> 
> 
> 1. operator==
> 
> In Qt6, QVariant::operator==() will no longer do any conversions.
> If two QVariant does not have exactly the same, they will no longer be considered equal.
> Also, in Qt6, QMetatype will gain ability to register the operator==, and therefore it will work for any type (and not only for builtin type as currently).
> 
> So right now,
>   QVariant(QByteArray("Hello")) == QVariant(QString("Hello"))
> is true, but in Qt6 it will be false.
> 
> This is a behavior change, but I believe this is something we can afford to do.
> I do not have data on how much code will break with this change, but i feel most use of operator== are there for optimisations: i.e:   setFoo(const QVariant &foo) { if (m_foo == foo) return; ... }
> Maybe we'll have more data once we actually implement the change and see if too many things breaks.

This should be relatively uncontroversial. The current behaviour is at the very least unexpected by users.
> 
> 
> 2. operator< and other comparison operator
> 
> Deprecate in Qt 5.15, remove in Qt 6
> 
> It is not possible to implement it correctly with a total order.
> 
> I could not find direct use of the operator in the code indexed on https://code.woboq.org/qt5 (only in QmlDesigner::operator< which is itself not used)
> Sorting on variant does not really make sense. Even code that does, like QAbstractItemModelPrivate::isVariantLessThan does not use operator<.
> 
> Where this is used may be the use of QVariant as a key in a QMap. This is problematic because the operator< does not implement a total order, so you can have funny results.
> I could not find instances of that in Qt or QtCreator, but Github search for "QMap<QVariant," shows many result :-(
> I'd still want to deprecate it. User could wrap QVariant in their own MySortedVariant with their own operators that does what they need for their use case.

+1. Total ordering will only work in special cases, as we do not control the types of data stored in a QVariant. Using a QMap<QVariant, …> sounds like a design mistake in any case. Let’s deprecate and remove in Qt 6.
> 
> 
> 3. conversions in QVariant::value
> 
> We would like to avoid having automatic conversion in QVariant::value.
> So Qt6 would be
>   std::optional<T> QVariant::value() const;
> And we could deprecate the current one in Qt5.15 in favor of qvariant_cast which is explicit.
> 
> This one is a bit more controversial maybe. Because there are thousands of call to QVariant::value all over the place. But "value()" is the ideal name for the non-converting variant.
> A clazy script to replace QVariant::value with qvariant_cast will be in order.

I like this idea. value() is the perfect API to return the value of the variant without implicit conversions. The advantage of the above approach is that it offers a way to already migrate in Qt 5 and will give compile errors in Qt6 for code that hasn’t been migrated to use qvariant_cast.

One more idea here: qvariant_cast was done as a free standing function because of limitations in VC++ 6. We could also add a template<typename T> QVariant::cast<T>() (or maybe to() or convertedTo()). IMO that would make the code look a bit nicer than using the freestanding qvariant_cast method.
> 
> 
> 4. All the implicit constructors for builtin types.
> 
> QVariant has many implicit constructors for all the builtin types.
> I suggest to replace them all with a template<typename T> QVariant(T&&) constructor. (same as std::any.) So builtin types are no longer special.

+1. This should be source compatible.
> 
> 
> 5. All the method toXxx (where Xxx is a builtin type)
> 
> Leave them as-is?
> However some of them are for types that may go outside of QtCore, these should be deprecated in Qt 5.15 and removed in Qt6

They do basically the same as qvariant_cast, so we could simply deprecate them all and replace by the cast() method mentioned above.
> 
> 
> 6. QVariant::Type and QMetaType::Type enums
> 
> QVariant::Type is already marked as obsolete in the documentation, but not yet marked as deprecated.
> So we can remove it in Qt6, and we should try to mark it as deprecated in Qt 5.15. But that's hard because it is used all over the place.
> 
> QMetaType::Type will be marked as deprecated in Qt6, but i'm afraid we cannot simply remove it.
> In  general, all the integer id API for QMetaType will be deprecated in Qt6, one should use QMetaType by value. The integer id will stay in Qt6. This means that there will still be a central registry of types but it would only be there for the types for which we ask the id (and for the builtin types)

Sounds ok to me.

Cheers,
Lars