[Development] QProperty and library coding guide

[Volker Hilsheimer]

> On 20 Jul 2020, at 20:30, Thiago Macieira <thiago.macieira at intel.com> wrote:
> 
> On Monday, 20 July 2020 08:40:06 PDT Oswald Buddenhagen wrote:
>> anyway, this can be trivially bypassed by just using an arbitrary
>> address and subtracting the offset. or using offsetof, indeed. in any
>> case it's 3 minutes of work. 
> 
> Sorry, an arbitrary address won't work either because it's still a 
> dereference.
> 
> Suppose:
> 
>    const auto dummy = static_cast<Object *>(0x4000);
>    const auto member = &dummy->member;
>    qptrdiff offset = quintptr(member) - quintptr(dummy)
>    return static_cast<Object *>(quintptr(this) - offset);
> 
> The problem is that the first line is creating a pointer to a memory location 
> that does not have a valid Object object. So when the second line does 
> 	dummy->member
> this expression is UB. It doesn't matter that the compiler usually implements 
> the full expression &dummy->member as arithmetic on the pointers without 
> dereferencing them; from the language's point of view, a dereference did 
> happen and therefore it's UB. This is no different than:
> 	Object *ptr = nullptr;
> 	ptr->staticFunction();
> See commit 88cf9402e336fddeb673c92f3c14da47a9f8450b[1].
> 
> Also note how both ASan and UBSan are likely to complain. Whatever our 
> implementation is, it must pass both sanitisers.
> 
> [1] https://code.qt.io/cgit/qt/qtbase.git/commit/?
> id=88cf9402e336fddeb673c92f3c14da47a9f8450b
> -- 
> Thiago Macieira - thiago.macieira (AT) intel.com
>  Software Architect - Intel DPG Cloud Engineering


Taking a step back.

I wonder if we can avoid this problem if we require people to write a bit more code. The idea to operate on the property directly looks nice at first sight, ie

auto oldValue = object->enabled;
object->enabled = newValue;

but that seems to be the only reason why we need the maybe-zero-sized members in public classes, and the pointer-arithmetic to get to “this"; I’m not convinced that it's worth it.

Realistically [1] we will have a lot of classes that would be a mix of old and new properties. Especially with that in mind, we do need to make sure that we have API consistency. For example, the property getters for boolean properties have always been called isProperty; never just “property". The new system departs from this. Making it cleverly generate an isProperty getter is just more magic.

Requiring the implementing getter/setter methods explicitly, just today, is fine, I think. We might get something better later, but time is ticking and this shouldn’t stop the show.

To set up bindings, one needs to do something like this with the current desitn (I might miss some variants):

sink->otherProperty.setBinding(source->property);
sink->otherProperty = Qt::makePropertyBinding(source->property);
sink->otherProperty = Qt::makeBinding([&](){ return source->property; });

If instead we need to make a call on the source object, then it would remove the entire “this” pointer problem. To make the call unique for each property, we just have to declare a type for each property, which is easy.

sink->otherProperty = source->makeBinding<Source::propertyType>();


I’ve hacked a quick poc up at

https://codereview.qt-project.org/c/qt/qtbase/+/308308

This is obviously not taking a lot of use cases into account, so I might be missing a lot, but the concept should be fairly extensible, for intance for subscribe, setValue, value methods. The only downside I see is that 

object->property = newValue;


is not possible with this, but having to write

object->setProperty(newValue);

like we do today can’t be a blocker for the entire concept of bindable and lazily evaluted properties.


Volker


[1] There might be many classes that we can’t port entirely over to the new property system, at least not without subtle changes to behavior that we can’t accept. Qt’s properties are not designed to be orthogonal; changing them frequently has side effects on other properties; getters sometimes apply some additional logic for defaults and overrides; we have virtual setters, etc. This has turned out to be quite complex over the last 10 days of trying, and the resulting code has not always been a step into the direction of maintainability.