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>

18 files changed, 269 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;
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/copy-command.qbs b/tests/auto/blackbox/testdata/suspicious-calls/copy-command.qbs
new file mode 100644
index 000000000..e01435fc0
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/copy-command.qbs
@@ -0,0 +1,23 @@
+import qbs
+import qbs.File
+
+Product {
+    type: ["out"]
+    Group {
+        files: ["test.txt"]
+        fileTags: ["in"]
+    }
+    Rule {
+        inputs: ["in"]
+        Artifact {
+            filePath: "dummy.txt"
+            fileTags: ["out"]
+        }
+        prepare: {
+            var cmd = new JavaScriptCommand();
+            cmd.silent = true;
+            cmd.sourceCode = function() { File.copy(input.filePath, output.filePath); };
+            return [cmd];
+        }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/copy-eval.qbs b/tests/auto/blackbox/testdata/suspicious-calls/copy-eval.qbs
new file mode 100644
index 000000000..93bfca81d
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/copy-eval.qbs
@@ -0,0 +1,10 @@
+import qbs
+import qbs.File
+
+Product {
+    name: {
+        File.copy(sourceDirectory + "/copy-eval.qbs",
+                  sourceDirectory + "/copy-eval2.qbs");
+        return "blubb"
+    }
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/copy-prepare.qbs b/tests/auto/blackbox/testdata/suspicious-calls/copy-prepare.qbs
new file mode 100644
index 000000000..b6098e8de
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/copy-prepare.qbs
@@ -0,0 +1,24 @@
+import qbs
+import qbs.File
+
+Product {
+    type: ["out"]
+    Group {
+        files: ["test.txt"]
+        fileTags: ["in"]
+    }
+    Rule {
+        inputs: ["in"]
+        Artifact {
+            filePath: "dummy.txt"
+            fileTags: ["out"]
+        }
+        prepare: {
+            File.copy(input.filePath, output.filePath);
+            var cmd = new JavaScriptCommand();
+            cmd.description = "no-op";
+            cmd.sourceCode = function() { };
+            return [cmd];
+        }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/copy-probe.qbs b/tests/auto/blackbox/testdata/suspicious-calls/copy-probe.qbs
new file mode 100644
index 000000000..dce27b196
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/copy-probe.qbs
@@ -0,0 +1,13 @@
+import qbs
+import qbs.File
+
+Product {
+    Probe {
+        property string baseDir: project.sourceDirectory
+
+        configure: {
+            File.copy(baseDir + "/copy-probe.qbs",
+                      baseDir + "/copy-probe2.qbs");
+        }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/direntries-command.qbs b/tests/auto/blackbox/testdata/suspicious-calls/direntries-command.qbs
new file mode 100644
index 000000000..d15351c25
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/direntries-command.qbs
@@ -0,0 +1,25 @@
+import qbs
+import qbs.File
+
+Product {
+    type: ["out"]
+    Group {
+        files: ["test.txt"]
+        fileTags: ["in"]
+    }
+    Rule {
+        inputs: ["in"]
+        Artifact {
+            filePath: "dummy.txt"
+            fileTags: ["out"]
+        }
+        prepare: {
+            var cmd = new JavaScriptCommand();
+            cmd.silent = true;
+            cmd.sourceCode = function() {
+                var dummy = File.directoryEntries(product.sourceDirectory, File.Files);
+            };
+            return [cmd];
+        }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/direntries-eval.qbs b/tests/auto/blackbox/testdata/suspicious-calls/direntries-eval.qbs
new file mode 100644
index 000000000..4cd0d634e
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/direntries-eval.qbs
@@ -0,0 +1,6 @@
+import qbs
+import qbs.File
+
+Product {
+    name: File.directoryEntries(sourceDirectory, File.Files)[0]
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/direntries-prepare.qbs b/tests/auto/blackbox/testdata/suspicious-calls/direntries-prepare.qbs
new file mode 100644
index 000000000..6f7320110
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/direntries-prepare.qbs
@@ -0,0 +1,24 @@
+import qbs
+import qbs.File
+
+Product {
+    type: ["out"]
+    Group {
+        files: ["test.txt"]
+        fileTags: ["in"]
+    }
+    Rule {
+        inputs: ["in"]
+        Artifact {
+            filePath: "dummy.txt"
+            fileTags: ["out"]
+        }
+        prepare: {
+            var dummy = File.directoryEntries(product.sourceDirectory, File.Files);
+            var cmd = new JavaScriptCommand();
+            cmd.silent = true;
+            cmd.sourceCode = function() { };
+            return [cmd];
+        }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/direntries-probe.qbs b/tests/auto/blackbox/testdata/suspicious-calls/direntries-probe.qbs
new file mode 100644
index 000000000..dc91adc8d
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/direntries-probe.qbs
@@ -0,0 +1,14 @@
+import qbs
+import qbs.File
+
+Product {
+    Probe {
+        property string baseDir: project.sourceDirectory
+        property stringList subDirs
+
+        configure: {
+            subDirs = File.directoryEntries(baseDir, File.AllDirs);
+            found = true;
+        }
+    }
+}
diff --git a/tests/auto/blackbox/testdata/suspicious-calls/test.txt b/tests/auto/blackbox/testdata/suspicious-calls/test.txt
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/tests/auto/blackbox/testdata/suspicious-calls/test.txt
diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp
index fee806d39..0c2f5c91b 100644
--- a/tests/auto/blackbox/tst_blackbox.cpp
+++ b/tests/auto/blackbox/tst_blackbox.cpp
@@ -165,6 +165,35 @@ void TestBlackbox::sevenZip()
     QVERIFY2(output.contains("archivable.qbs"), output.constData());
 }
 
+void TestBlackbox::suspiciousCalls()
+{
+    const QString projectDir = testDataDir + "/suspicious-calls";
+    QDir::setCurrent(projectDir);
+    rmDirR(relativeBuildDir());
+    QFETCH(QString, projectFile);
+    QbsRunParameters params(QStringList() << "-f" << projectFile);
+    QCOMPARE(runQbs(params), 0);
+    QFETCH(QByteArray, expectedWarning);
+    QVERIFY2(m_qbsStderr.contains(expectedWarning), m_qbsStderr.constData());
+}
+
+void TestBlackbox::suspiciousCalls_data()
+{
+    QTest::addColumn<QString>("projectFile");
+    QTest::addColumn<QByteArray>("expectedWarning");
+    QTest::newRow("File.copy() in Probe") << "copy-probe.qbs" << QByteArray();
+    QTest::newRow("File.copy() during evaluation") << "copy-eval.qbs" << QByteArray("File.copy()");
+    QTest::newRow("File.copy() in prepare script")
+            << "copy-prepare.qbs" << QByteArray("File.copy()");
+    QTest::newRow("File.copy() in command") << "copy-command.qbs" << QByteArray();
+    QTest::newRow("File.directoryEntries() in Probe") << "direntries-probe.qbs" << QByteArray();
+    QTest::newRow("File.directoryEntries() during evaluation")
+            << "direntries-eval.qbs" << QByteArray("File.directoryEntries()");
+    QTest::newRow("File.directoryEntries() in prepare script")
+            << "direntries-prepare.qbs" << QByteArray();
+    QTest::newRow("File.directoryEntries() in command") << "direntries-command.qbs" << QByteArray();
+}
+
 void TestBlackbox::tar()
 {
     if (HostOsInfo::hostOs() == HostOsInfo::HostOsWindows)
diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h
index d628cfff0..06df7f53c 100644
--- a/tests/auto/blackbox/tst_blackbox.h
+++ b/tests/auto/blackbox/tst_blackbox.h
@@ -168,6 +168,8 @@ private slots:
     void renameDependency();
     void separateDebugInfo();
     void sevenZip();
+    void suspiciousCalls();
+    void suspiciousCalls_data();
     void systemRunPaths();
     void systemRunPaths_data();
     void tar();