Fix "Expression depends on non-NOTIFYable properties" regression

CONSTANT properties are by nature non-NOTIFYable.

The issue behind the regression is caused by the fact that we were
capturing a property regardless of whether or not it was const.
There were two states that captureRequired was expressing:

true: We're reading the property of a QObject, and we're not quite sure
where the QObject comes from or what it is. So, when reading that
property at run-time, make sure that we capture where we read that
property so that if it changes we can re-evaluate the entire
expression.

false: We're reading the property of a QObject, and we know that it's
the scope object or context object, which we know very well. Instead of
registering a property capture every time, we can do that ahead of time
and then register all those captures in one shot in
registerQmlDependencies().

There is a third state that is only relevant when captureRequired is
false: We're reading a property from the scope or context object, but
it's a CONSTANT property, so we don't need to register a dependency
at all.

This patch adds replaces captureRequired with the PropertyCapturePolicy
enum, which accounts for the third state and, as a bonus, makes the
code easier to understand.

Task-number: QTBUG-66361
Change-Id: I6cef1deb76538fbdacf1324b4467403dd40dd7de
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

5 files changed, 65 insertions, 17 deletions

diff --git a/src/qml/compiler/qqmlirbuilder.cpp b/src/qml/compiler/qqmlirbuilder.cpp
index 6de0ed6f7d..6c8ca4bbfc 100644
--- a/src/qml/compiler/qqmlirbuilder.cpp
+++ b/src/qml/compiler/qqmlirbuilder.cpp
@@ -2231,20 +2231,28 @@ QV4::Compiler::Codegen::Reference JSCodeGen::fallbackNameLookup(const QString &n
         QQmlPropertyData *data = lookupQmlCompliantProperty(_scopeObject, name);
         if (!data)
             return Reference::fromName(this, name);
+
         Reference base = Reference::fromStackSlot(this, _qmlContextSlot);
-        bool captureRequired = !data->isConstant() && !data->isQmlBinding();
-        return Reference::fromQmlScopeObject(base, data->coreIndex(), data->notifyIndex(),
-                                             captureRequired);
+        Reference::PropertyCapturePolicy capturePolicy;
+        if (!data->isConstant() && !data->isQmlBinding())
+            capturePolicy = Reference::CaptureAtRuntime;
+        else
+            capturePolicy = data->isConstant() ? Reference::DontCapture : Reference::CaptureAheadOfTime;
+        return Reference::fromQmlScopeObject(base, data->coreIndex(), data->notifyIndex(), capturePolicy);
     }
 
     if (_contextObject) {
         QQmlPropertyData *data = lookupQmlCompliantProperty(_contextObject, name);
         if (!data)
             return Reference::fromName(this, name);
+
         Reference base = Reference::fromStackSlot(this, _qmlContextSlot);
-        bool captureRequired = !data->isConstant() && !data->isQmlBinding();
-        return Reference::fromQmlContextObject(base, data->coreIndex(), data->notifyIndex(),
-                                               captureRequired);
+        Reference::PropertyCapturePolicy capturePolicy;
+        if (!data->isConstant() && !data->isQmlBinding())
+            capturePolicy = Reference::CaptureAtRuntime;
+        else
+            capturePolicy = data->isConstant() ? Reference::DontCapture : Reference::CaptureAheadOfTime;
+        return Reference::fromQmlContextObject(base, data->coreIndex(), data->notifyIndex(), capturePolicy);
     }
 
     if (m_globalNames.contains(name)) {
diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 5f0d7395f7..b2808784a0 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -3087,7 +3087,7 @@ Codegen::Reference &Codegen::Reference::operator =(const Reference &other)
         qmlBase = other.qmlBase;
         qmlCoreIndex = other.qmlCoreIndex;
         qmlNotifyIndex = other.qmlNotifyIndex;
-        captureRequired = other.captureRequired;
+        capturePolicy = other.capturePolicy;
         break;
     }
 
@@ -3124,7 +3124,7 @@ bool Codegen::Reference::operator==(const Codegen::Reference &other) const
     case QmlScopeObject:
     case QmlContextObject:
         return qmlCoreIndex == other.qmlCoreIndex && qmlNotifyIndex == other.qmlNotifyIndex
-                && captureRequired == other.captureRequired;
+                && capturePolicy == other.capturePolicy;
     }
     return true;
 }
@@ -3453,18 +3453,18 @@ QT_WARNING_POP
         Instruction::LoadScopeObjectProperty load;
         load.base = qmlBase;
         load.propertyIndex = qmlCoreIndex;
-        load.captureRequired = captureRequired;
+        load.captureRequired = capturePolicy == CaptureAtRuntime;
         codegen->bytecodeGenerator->addInstruction(load);
-        if (!captureRequired)
+        if (capturePolicy == CaptureAheadOfTime)
             codegen->_context->scopeObjectPropertyDependencies.insert(qmlCoreIndex, qmlNotifyIndex);
     } return;
     case QmlContextObject: {
         Instruction::LoadContextObjectProperty load;
         load.base = qmlBase;
         load.propertyIndex = qmlCoreIndex;
-        load.captureRequired = captureRequired;
+        load.captureRequired = capturePolicy == CaptureAtRuntime;
         codegen->bytecodeGenerator->addInstruction(load);
-        if (!captureRequired)
+        if (capturePolicy == CaptureAheadOfTime)
             codegen->_context->contextObjectPropertyDependencies.insert(qmlCoreIndex, qmlNotifyIndex);
     } return;
     case Invalid:
diff --git a/src/qml/compiler/qv4codegen_p.h b/src/qml/compiler/qv4codegen_p.h
index 5c4ce14870..e52f63e1f4 100644
--- a/src/qml/compiler/qv4codegen_p.h
+++ b/src/qml/compiler/qv4codegen_p.h
@@ -219,6 +219,28 @@ public:
             return isStackSlot();
         }
 
+        enum PropertyCapturePolicy {
+            /*
+                We're reading a property from the scope or context object, but it's a CONSTANT property,
+                so we don't need to register a dependency at all.
+            */
+            DontCapture,
+            /*
+                We're reading the property of a QObject, and we know that it's the
+                scope object or context object, which we know very well. Instead of registering a
+                property capture every time, we can do that ahead of time and then register all those
+                captures in one shot in registerQmlDependencies().
+            */
+            CaptureAheadOfTime,
+            /*
+                We're reading the property of a QObject, and we're not quite sure where
+                the QObject comes from or what it is. So, when reading that property at run-time,
+                make sure that we capture where we read that property so that if it changes we can
+                re-evaluate the entire expression.
+            */
+            CaptureAtRuntime
+        };
+
         static Reference fromAccumulator(Codegen *cg) {
             return Reference(cg, Accumulator);
         }
@@ -267,20 +289,20 @@ public:
             r.isReadonly = true;
             return r;
         }
-        static Reference fromQmlScopeObject(const Reference &base, qint16 coreIndex, qint16 notifyIndex, bool captureRequired) {
+        static Reference fromQmlScopeObject(const Reference &base, qint16 coreIndex, qint16 notifyIndex, PropertyCapturePolicy capturePolicy) {
             Reference r(base.codegen, QmlScopeObject);
             r.qmlBase = base.storeOnStack().stackSlot();
             r.qmlCoreIndex = coreIndex;
             r.qmlNotifyIndex = notifyIndex;
-            r.captureRequired = captureRequired;
+            r.capturePolicy = capturePolicy;
             return r;
         }
-        static Reference fromQmlContextObject(const Reference &base, qint16 coreIndex, qint16 notifyIndex, bool captureRequired) {
+        static Reference fromQmlContextObject(const Reference &base, qint16 coreIndex, qint16 notifyIndex, PropertyCapturePolicy capturePolicy) {
             Reference r(base.codegen, QmlContextObject);
             r.qmlBase = base.storeOnStack().stackSlot();
             r.qmlCoreIndex = coreIndex;
             r.qmlNotifyIndex = notifyIndex;
-            r.captureRequired = captureRequired;
+            r.capturePolicy = capturePolicy;
             return r;
         }
         static Reference fromThis(Codegen *cg) {
@@ -336,7 +358,7 @@ public:
                 Moth::StackSlot qmlBase;
                 qint16 qmlCoreIndex;
                 qint16 qmlNotifyIndex;
-                bool captureRequired;
+                PropertyCapturePolicy capturePolicy;
             };
         };
         QString name;
diff --git a/tests/auto/qml/qqmlecmascript/data/nonNotifyableConstant.qml b/tests/auto/qml/qqmlecmascript/data/nonNotifyableConstant.qml
new file mode 100644
index 0000000000..424b3e8b07
--- /dev/null
+++ b/tests/auto/qml/qqmlecmascript/data/nonNotifyableConstant.qml
@@ -0,0 +1,5 @@
+import Qt.test 1.0
+
+MyQmlObject {
+    objectName: trueProperty ? "foo" : "bar"
+}
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index b7d5db9914..5e752d124b 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -261,6 +261,7 @@ private slots:
     void doubleEvaluate();
     void forInLoop();
     void nonNotifyable();
+    void nonNotifyableConstant();
     void deleteWhileBindingRunning();
     void callQtInvokables();
     void resolveClashingProperties();
@@ -6889,6 +6890,18 @@ void tst_qqmlecmascript::nonNotifyable()
     delete object;
 }
 
+void tst_qqmlecmascript::nonNotifyableConstant()
+{
+    QQmlComponent component(&engine, testFileUrl("nonNotifyableConstant.qml"));
+    QQmlTestMessageHandler messageHandler;
+
+    // Shouldn't produce an error message about non-NOTIFYable properties,
+    // as the property has the CONSTANT attribute.
+    QScopedPointer<QObject> object(component.create());
+    QVERIFY(object);
+    QCOMPARE(messageHandler.messages().size(), 0);
+}
+
 void tst_qqmlecmascript::forInLoop()
 {
     QQmlComponent component(&engine, testFileUrl("forInLoop.qml"));