fix module property cache

When caching module properties, we must take into account whether
we're requesting one or multiple values. Otherwise we might get
the wrong cached result.

Task-number: QBS-539

Change-Id: I9ca17d3a537922f37dc70be4dbfd73416b6bdfc3
Reviewed-by: Christian Kandeler <christian.kandeler@digia.com>

3 files changed, 58 insertions, 11 deletions

diff --git a/src/lib/corelib/jsextensions/moduleproperties.cpp b/src/lib/corelib/jsextensions/moduleproperties.cpp
index 93aab3975..b8a2d27d8 100644
--- a/src/lib/corelib/jsextensions/moduleproperties.cpp
+++ b/src/lib/corelib/jsextensions/moduleproperties.cpp
@@ -124,7 +124,8 @@ QScriptValue ModuleProperties::moduleProperties(QScriptContext *context, QScript
     const QString moduleName = internalModuleName(context->argument(0).toString());
     const QString propertyName = context->argument(1).toString();
 
-    QVariant value = qbsEngine->retrieveFromPropertyCache(moduleName, propertyName, properties);
+    QVariant value = qbsEngine->retrieveFromPropertyCache(moduleName, propertyName, oneValue,
+                                                          properties);
     if (!value.isValid()) {
         if (oneValue)
             value = PropertyFinder().propertyValue(properties->value(), moduleName, propertyName);
@@ -138,7 +139,7 @@ QScriptValue ModuleProperties::moduleProperties(QScriptContext *context, QScript
 
         // Cache the variant value. We must not cache the QScriptValue here, because it's a
         // reference and the user might change the actual object.
-        qbsEngine->addToPropertyCache(moduleName, propertyName, properties, value);
+        qbsEngine->addToPropertyCache(moduleName, propertyName, oneValue, properties, value);
     }
     return engine->toScriptValue(value);
 }
diff --git a/src/lib/corelib/language/scriptengine.cpp b/src/lib/corelib/language/scriptengine.cpp
index de28c7c30..bbc84d580 100644
--- a/src/lib/corelib/language/scriptengine.cpp
+++ b/src/lib/corelib/language/scriptengine.cpp
@@ -53,6 +53,29 @@ namespace Internal {
 
 const bool debugJSImports = false;
 
+bool operator==(const ScriptEngine::PropertyCacheKey &lhs,
+        const ScriptEngine::PropertyCacheKey &rhs)
+{
+    return lhs.m_oneValue == rhs.m_oneValue
+            && lhs.m_propertyMap == rhs.m_propertyMap
+            && lhs.m_moduleName == rhs.m_moduleName
+            && lhs.m_propertyName == rhs.m_propertyName;
+}
+
+static inline uint combineHash(uint h1, uint h2, uint seed)
+{
+    // stolen from qHash(QPair)
+    return ((h1 << 16) | (h1 >> 16)) ^ h2 ^ seed;
+}
+
+uint qHash(const ScriptEngine::PropertyCacheKey &k, uint seed = 0)
+{
+    return combineHash(qHash(k.m_moduleName),
+                       combineHash(qHash(k.m_propertyName),
+                                   combineHash(qHash(k.m_oneValue), qHash(k.m_propertyMap),
+                                               seed), seed), seed);
+}
+
 ScriptEngine::ScriptEngine(const Logger &logger, QObject *parent)
     : QScriptEngine(parent), m_logger(logger)
 {
@@ -125,17 +148,16 @@ void ScriptEngine::addPropertyRequestedFromArtifact(const Artifact *artifact,
 }
 
 void ScriptEngine::addToPropertyCache(const QString &moduleName, const QString &propertyName,
-        const PropertyMapConstPtr &propertyMap, const QVariant &value)
+        bool oneValue, const PropertyMapConstPtr &propertyMap, const QVariant &value)
 {
-    m_propertyCache.insert(qMakePair(moduleName + QLatin1Char('.') + propertyName, propertyMap),
+    m_propertyCache.insert(PropertyCacheKey(moduleName, propertyName, oneValue, propertyMap),
                            value);
 }
 
 QVariant ScriptEngine::retrieveFromPropertyCache(const QString &moduleName,
-        const QString &propertyName, const PropertyMapConstPtr &propertyMap)
+        const QString &propertyName, bool oneValue, const PropertyMapConstPtr &propertyMap)
 {
-    return m_propertyCache.value(qMakePair(moduleName + QLatin1Char('.') + propertyName,
-                                           propertyMap));
+    return m_propertyCache.value(PropertyCacheKey(moduleName, propertyName, oneValue, propertyMap));
 }
 
 void ScriptEngine::defineProperty(QScriptValue &object, const QString &name,
@@ -508,5 +530,12 @@ void ScriptEngine::uninstallImportFunctions()
     globalObject().setProperty(QLatin1String("loadExtension"), QScriptValue());
 }
 
+ScriptEngine::PropertyCacheKey::PropertyCacheKey(const QString &moduleName,
+        const QString &propertyName, bool oneValue, const PropertyMapConstPtr &propertyMap)
+    : m_moduleName(moduleName), m_propertyName(propertyName), m_oneValue(oneValue),
+      m_propertyMap(propertyMap)
+{
+}
+
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h
index 435e2c748..3d90f7b0c 100644
--- a/src/lib/corelib/language/scriptengine.h
+++ b/src/lib/corelib/language/scriptengine.h
@@ -37,7 +37,6 @@
 #include <tools/filetime.h>
 
 #include <QHash>
-#include <QPair>
 #include <QProcessEnvironment>
 #include <QScriptEngine>
 #include <QStack>
@@ -77,9 +76,9 @@ public:
     }
 
     void addToPropertyCache(const QString &moduleName, const QString &propertyName,
-        const PropertyMapConstPtr &propertyMap, const QVariant &value);
+            bool oneValue, const PropertyMapConstPtr &propertyMap, const QVariant &value);
     QVariant retrieveFromPropertyCache(const QString &moduleName, const QString &propertyName,
-            const PropertyMapConstPtr &propertyMap);
+            bool oneValue, const PropertyMapConstPtr &propertyMap);
 
     void defineProperty(QScriptValue &object, const QString &name, const QScriptValue &descriptor);
     void setObservedProperty(QScriptValue &object, const QString &name, const QScriptValue &value,
@@ -128,9 +127,27 @@ private:
     static QScriptValue js_loadExtension(QScriptContext *context, QScriptEngine *qtengine);
     static QScriptValue js_loadFile(QScriptContext *context, QScriptEngine *qtengine);
 
+    class PropertyCacheKey
+    {
+    public:
+        PropertyCacheKey(const QString &moduleName, const QString &propertyName, bool oneValue,
+                         const PropertyMapConstPtr &propertyMap);
+    private:
+        const QString &m_moduleName;
+        const QString &m_propertyName;
+        const bool m_oneValue;
+        const PropertyMapConstPtr &m_propertyMap;
+
+        friend bool operator==(const PropertyCacheKey &lhs, const PropertyCacheKey &rhs);
+        friend uint qHash(const ScriptEngine::PropertyCacheKey &k, uint seed);
+    };
+
+    friend bool operator==(const PropertyCacheKey &lhs, const PropertyCacheKey &rhs);
+    friend uint qHash(const ScriptEngine::PropertyCacheKey &k, uint seed);
+
     ScriptValueCache m_scriptValueCache;
     QHash<QString, QScriptValue> m_jsImportCache;
-    QHash<QPair<QString, PropertyMapConstPtr>, QVariant> m_propertyCache;
+    QHash<PropertyCacheKey, QVariant> m_propertyCache;
     PropertyList m_propertiesRequestedInScript;
     QHash<QString, PropertyList> m_propertiesRequestedFromArtifact;
     Logger m_logger;