Warn against possibly improper use of JS extensions in qbs project files

For instance, we inform users when they are doing potentially slow File
I/O operations during project resolving and advise them to use a Probe
instead.

Task-number: QBS-1033
Change-Id: I3e9d0eb36c6ddebc5f4a112c329f96d25856ac0a
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

7 files changed, 99 insertions, 1 deletions

diff --git a/src/lib/corelib/jsextensions/file.cpp b/src/lib/corelib/jsextensions/file.cpp
index 071c2a9da..1a6d259e4 100644
--- a/src/lib/corelib/jsextensions/file.cpp
+++ b/src/lib/corelib/jsextensions/file.cpp
@@ -131,6 +131,13 @@ QScriptValue File::js_copy(QScriptContext *context, QScriptEngine *engine)
                                    Tr::tr("copy expects 2 arguments"));
     }
 
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({
+            DubiousContext(EvalContext::PropertyEvaluation),
+            DubiousContext(EvalContext::RuleExecution, DubiousContext::SuggestMoving)
+    });
+    se->checkContext(QLatin1String("File.copy()"), dubiousContexts);
+
     const QString sourceFile = context->argument(0).toString();
     const QString targetFile = context->argument(1).toString();
     QString errorMessage;
@@ -160,11 +167,17 @@ QScriptValue File::js_directoryEntries(QScriptContext *context, QScriptEngine *e
         return context->throwError(QScriptContext::SyntaxError,
                                    Tr::tr("directoryEntries expects 2 arguments"));
     }
+
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({
+            DubiousContext(EvalContext::PropertyEvaluation, DubiousContext::SuggestMoving)
+    });
+    se->checkContext(QLatin1String("File.directoryEntries()"), dubiousContexts);
+
     const QString path = context->argument(0).toString();
     const QDir::Filters filters = static_cast<QDir::Filters>(context->argument(1).toUInt32());
     QDir dir(path);
     const QStringList entries = dir.entryList(filters, QDir::Name);
-    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
     se->addDirectoryEntriesResult(path, filters, entries);
     return qScriptValueFromSequence(engine, entries);
 }
@@ -176,6 +189,11 @@ QScriptValue File::js_remove(QScriptContext *context, QScriptEngine *engine)
         return context->throwError(QScriptContext::SyntaxError,
                                    Tr::tr("remove expects 1 argument"));
     }
+
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({ DubiousContext(EvalContext::PropertyEvaluation) });
+    se->checkContext(QLatin1String("File.remove()"), dubiousContexts);
+
     QString fileName = context->argument(0).toString();
 
     QString errorMessage;
@@ -205,6 +223,11 @@ QScriptValue File::js_makePath(QScriptContext *context, QScriptEngine *engine)
         return context->throwError(QScriptContext::SyntaxError,
                                    Tr::tr("makePath expects 1 argument"));
     }
+
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({ DubiousContext(EvalContext::PropertyEvaluation) });
+    se->checkContext(QLatin1String("File.makePath()"), dubiousContexts);
+
     return QDir::root().mkpath(context->argument(0).toString());
 }
 
@@ -216,6 +239,10 @@ QScriptValue File::js_move(QScriptContext *context, QScriptEngine *engine)
                                    Tr::tr("move expects 2 arguments"));
     }
 
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({ DubiousContext(EvalContext::PropertyEvaluation) });
+    se->checkContext(QLatin1String("File.move()"), dubiousContexts);
+
     const QString sourceFile = context->argument(0).toString();
     const QString targetFile = context->argument(1).toString();
     const bool overwrite = context->argumentCount() > 2 ? context->argument(2).toBool() : true;
diff --git a/src/lib/corelib/jsextensions/process.cpp b/src/lib/corelib/jsextensions/process.cpp
index ad67999b6..ab4df57b8 100644
--- a/src/lib/corelib/jsextensions/process.cpp
+++ b/src/lib/corelib/jsextensions/process.cpp
@@ -73,6 +73,12 @@ QScriptValue Process::ctor(QScriptContext *context, QScriptEngine *engine)
         return context->throwError(QLatin1String("Process()"));
     }
 
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts ({
+            DubiousContext(EvalContext::PropertyEvaluation, DubiousContext::SuggestMoving)
+    });
+    se->checkContext(QLatin1String("qbs.Process"), dubiousContexts);
+
     QScriptValue obj = engine->newQObject(t, QScriptEngine::ScriptOwnership);
 
     // Get environment
diff --git a/src/lib/corelib/jsextensions/propertylist.mm b/src/lib/corelib/jsextensions/propertylist.mm
index 4e431e499..2bf2acacc 100644
--- a/src/lib/corelib/jsextensions/propertylist.mm
+++ b/src/lib/corelib/jsextensions/propertylist.mm
@@ -40,6 +40,7 @@
 
 #include "propertylist.h"
 
+#include <language/scriptengine.h>
 #include <tools/hostosinfo.h>
 
 #include <QFile>
@@ -80,6 +81,12 @@ void initializeJsExtensionPropertyList(QScriptValue extensionObject)
 
 QScriptValue PropertyList::ctor(QScriptContext *context, QScriptEngine *engine)
 {
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({
+            DubiousContext(EvalContext::PropertyEvaluation, DubiousContext::SuggestMoving)
+    });
+    se->checkContext(QLatin1String("qbs.PropertyList"), dubiousContexts);
+
     PropertyList *p = new PropertyList(context);
     QScriptValue obj = engine->newQObject(p, QScriptEngine::ScriptOwnership);
     return obj;
diff --git a/src/lib/corelib/jsextensions/temporarydir.cpp b/src/lib/corelib/jsextensions/temporarydir.cpp
index 1fb4c34ec..e7b811f31 100644
--- a/src/lib/corelib/jsextensions/temporarydir.cpp
+++ b/src/lib/corelib/jsextensions/temporarydir.cpp
@@ -40,6 +40,8 @@
 
 #include "temporarydir.h"
 
+#include <language/scriptengine.h>
+
 #include <QScriptEngine>
 #include <QScriptValue>
 #include <QTemporaryDir>
@@ -57,6 +59,12 @@ void initializeJsExtensionTemporaryDir(QScriptValue extensionObject)
 
 QScriptValue TemporaryDir::ctor(QScriptContext *context, QScriptEngine *engine)
 {
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({
+            DubiousContext(EvalContext::PropertyEvaluation, DubiousContext::SuggestMoving)
+    });
+    se->checkContext(QLatin1String("qbs.TemporaryDir"), dubiousContexts);
+
     TemporaryDir *t = new TemporaryDir(context);
     QScriptValue obj = engine->newQObject(t, QScriptEngine::ScriptOwnership);
     return obj;
diff --git a/src/lib/corelib/jsextensions/textfile.cpp b/src/lib/corelib/jsextensions/textfile.cpp
index a631366b1..02166c6a2 100644
--- a/src/lib/corelib/jsextensions/textfile.cpp
+++ b/src/lib/corelib/jsextensions/textfile.cpp
@@ -39,6 +39,7 @@
 
 #include "textfile.h"
 
+#include <language/scriptengine.h>
 #include <logging/translator.h>
 #include <tools/hostosinfo.h>
 
@@ -84,6 +85,12 @@ QScriptValue TextFile::ctor(QScriptContext *context, QScriptEngine *engine)
         return context->throwError(Tr::tr("TextFile constructor takes at most three parameters."));
     }
 
+    ScriptEngine * const se = static_cast<ScriptEngine *>(engine);
+    const DubiousContextList dubiousContexts({
+            DubiousContext(EvalContext::PropertyEvaluation, DubiousContext::SuggestMoving)
+    });
+    se->checkContext(QLatin1String("qbs.TextFile"), dubiousContexts);
+
     return engine->newQObject(t, QScriptEngine::ScriptOwnership);
 }
 
diff --git a/src/lib/corelib/language/scriptengine.cpp b/src/lib/corelib/language/scriptengine.cpp
index b383504b8..3e83fe6cb 100644
--- a/src/lib/corelib/language/scriptengine.cpp
+++ b/src/lib/corelib/language/scriptengine.cpp
@@ -159,6 +159,37 @@ void ScriptEngine::clearImportsCache()
     m_jsImportCache.clear();
 }
 
+void ScriptEngine::checkContext(const QString &operation,
+                                const DubiousContextList &dubiousContexts)
+{
+    for (auto it = dubiousContexts.cbegin(); it != dubiousContexts.cend(); ++it) {
+        const DubiousContext &info = *it;
+        if (info.context != evalContext())
+            continue;
+        QString warning;
+        switch (info.context) {
+        case EvalContext::PropertyEvaluation:
+            warning = Tr::tr("Suspicious use of %1 during property evaluation.").arg(operation);
+            if (info.suggestion == DubiousContext::SuggestMoving)
+                warning += QLatin1Char(' ') + Tr::tr("Should this call be in a Probe instead?");
+            break;
+        case EvalContext::RuleExecution:
+            warning = Tr::tr("Suspicious use of %1 during rule execution.").arg(operation);
+            if (info.suggestion == DubiousContext::SuggestMoving) {
+                warning += QLatin1Char(' ')
+                        + Tr::tr("Should this call be in a JavaScriptCommand instead?");
+            }
+            break;
+        case EvalContext::ProbeExecution:
+        case EvalContext::JsCommand:
+            QBS_ASSERT(false, continue);
+            break;
+        }
+        m_logger.printWarning(ErrorInfo(warning, currentContext()->backtrace()));
+        return;
+    }
+}
+
 void ScriptEngine::addPropertyRequestedFromArtifact(const Artifact *artifact,
                                                     const Property &property)
 {
diff --git a/src/lib/corelib/language/scriptengine.h b/src/lib/corelib/language/scriptengine.h
index 3bbce2c93..837ba4cd3 100644
--- a/src/lib/corelib/language/scriptengine.h
+++ b/src/lib/corelib/language/scriptengine.h
@@ -53,6 +53,8 @@
 #include <QStack>
 #include <QString>
 
+#include <vector>
+
 namespace qbs {
 namespace Internal {
 class Artifact;
@@ -61,6 +63,15 @@ class ScriptImporter;
 class ScriptPropertyObserver;
 
 enum class EvalContext { PropertyEvaluation, ProbeExecution, RuleExecution, JsCommand };
+class DubiousContext
+{
+public:
+    enum Suggestion { NoSuggestion, SuggestMoving };
+    DubiousContext(EvalContext c, Suggestion s = NoSuggestion) : context(c), suggestion(s) { }
+    EvalContext context;
+    Suggestion suggestion;
+};
+using DubiousContextList = std::vector<DubiousContext>;
 
 class ScriptEngine : public QScriptEngine
 {
@@ -76,6 +87,7 @@ public:
 
     void setEvalContext(EvalContext c) { m_evalContext = c; }
     EvalContext evalContext() const { return m_evalContext; }
+    void checkContext(const QString &operation, const DubiousContextList &dubiousContexts);
 
     void addPropertyRequestedInScript(const Property &property) {
         m_propertiesRequestedInScript += property;