qmllint: parse/lint argument files only once

Before a37e4270a5e66507aeda483ec4d32262096b44cc, qmllint used to create
up to two qqmljsscopes for each file it got as command line parameters.

a37e4270a5e66507aeda483ec4d32262096b44cc enhanced this situation by
avoiding the duplicate scopes, but still processed each file twice (once
by the LinterVisitor for the actual linting and once via lazy-loading
when the file gets pulled in as dependency for another file to be linted).
This lead to QTBUG-146688 where processing the same file into the same
scope has some buggy side-effects and breaks the assumption that
lazy-loaded scopes are never modified after creation.

This patch addresses this issue by making qmllint process each
QML file from its command line once by merging the linting process
and the lazy-loading mechanism together.

Rework QmlJSLinter to work in two stages:
1. collect all files to be linted with prepareFileForBatchLinting()
2. do the actual linting with lintFileInBatch()

 -- Stage 1:
In prepareFileForBatchLinting, set up the file-to-be-linted with a
linting factory to lazy-load it via LinterVisitor instead of
QQmlJSImportVisitor.

"unprepared" files needed by the file-to-be-linted are still loaded
via QQmlJSImportVisitor.

 -- Stage 2:
In lintFile, populate the lazy file if needed and continue with the
rest of the linting process.
At this stage, the QQmlJSScopes of the QML files used by the
current-QML-file-to-be-linted can be populated via lazy construction,
which means that cycle references can be resolved. A first attempt
where all the linting pipeline ran during lazy-loading in stage 1 was
unsuccessful at resolving cyclic dependencies between files.

Remove m_logger from QQmlJSLinter: there may be multiple active loggers,
for example when lazy-loading a file via LinterVisitor during the
linting process of another file.

To avoid QTBUG-146688, make prepareForBatchLinting() return false when
linting would re-populate an already populated scope.
Re-populating a scope breaks all the references to its children which
leads to QTBUG-146688.

Adapt qmllint/main.cpp to prepare all file passed by commandline before
starting to lint them.

Adapt tst_qmllint to:
* simulate clean snippet scope that are actually reused. Snippets
  are not used by other QML code so their scope and children can
  safely be deleted.
* clear the cache when scope re-populating happens on non-snippets. Note
  that clearing the cache for each linted file would be cleaner but
  increases the test runtime by a factor of 2.5x.

Fixes: QTBUG-146688
Change-Id: Ibc7586501388988d60d8ca0fbfff4949ed3c3c22
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

1 files changed, 117 insertions, 78 deletions

diff --git a/tools/qmllint/main.cpp b/tools/qmllint/main.cpp
index 3f412c806e..47846ce718 100644
--- a/tools/qmllint/main.cpp
+++ b/tools/qmllint/main.cpp
@@ -67,6 +67,71 @@ bool argumentsFromCommandLineAndFile(QStringList& allArguments, const QStringLis
     return true;
 }
 
+static bool applyFixes(const QQmlJSLinter::Result &linterResult, bool silent, bool dryRun)
+{
+    if (linterResult.status != QQmlJSLinter::LintSuccess
+        && linterResult.status != QQmlJSLinter::HasWarnings) {
+        return true;
+    }
+
+    QString fixedCode;
+    const QQmlJSLinter::FixResult result =
+            QQmlJSLinter::applyFixes(linterResult.logger.get(), &fixedCode, silent);
+    const QString filename = linterResult.logger->filePath();
+
+    if (result != QQmlJSLinter::NothingToFix && result != QQmlJSLinter::FixSuccess) {
+        return false;
+    }
+
+    if (dryRun) {
+        QTextStream(stdout) << fixedCode;
+        return true;
+    }
+    if (result == QQmlJSLinter::NothingToFix) {
+        if (!silent)
+            qWarning().nospace() << "Nothing to fix in " << filename;
+        return true;
+    }
+
+    const QString backupFile = filename + u".bak"_s;
+    if (QFile::exists(backupFile) && !QFile::remove(backupFile)) {
+        if (!silent) {
+            qWarning().nospace() << "Failed to remove old backup file " << backupFile
+                                 << ", aborting";
+        }
+        return false;
+    }
+    if (!QFile::copy(filename, backupFile)) {
+        if (!silent) {
+            qWarning().nospace() << "Failed to create backup file " << backupFile << ", aborting";
+        }
+        return false;
+    }
+
+    QFile file(filename);
+    if (!file.open(QIODevice::WriteOnly)) {
+        if (!silent) {
+            qWarning().nospace() << "Failed to open " << filename
+                                 << " for writing:" << file.errorString();
+        }
+        return false;
+    }
+
+    const QByteArray data = fixedCode.toUtf8();
+    if (file.write(data) != data.size()) {
+        if (!silent) {
+            qWarning().nospace() << "Failed to write new contents to " << filename << ": "
+                                 << file.errorString();
+        }
+        return false;
+    }
+    if (!silent) {
+        qDebug().nospace() << "Applied fixes to " << filename << ". Backup created at "
+                           << backupFile;
+    }
+    return true;
+}
+
 int main(int argc, char *argv[])
 {
     QHashSeed::setDeterministicGlobalSeed();
@@ -365,6 +430,7 @@ All warnings can be set to four levels of severity:
     if (parser.isSet(dryRun))
         defaultSettings.reportConfigForFiles(positionalArguments);
 
+    const bool isFixing = parser.isSet(fixFile);
     QJsonArray jsonFiles;
 
     for (const QString &filename : positionalArguments) {
@@ -460,92 +526,65 @@ All warnings can be set to four levels of severity:
         for (auto &plugin : plugins)
             plugin.setEnabled(!disabledPlugins.contains(plugin.name().toLower()));
 
-        const bool isFixing = parser.isSet(fixFile);
-
-        QQmlJSLinter::LintResult lintResult;
+        QQmlJSLinter::Result lintResult;
 
         if (parser.isSet(moduleOption)) {
-            lintResult = linter.lintModule(filename, silent, useJson ? &jsonFiles : nullptr,
-                                           qmlImportPaths, resourceFiles);
-        } else {
-            lintResult = linter.lintFile(filename, nullptr, silent || isFixing,
-                                         useJson ? &jsonFiles : nullptr, qmlImportPaths,
-                                         qmldirFiles, resourceFiles, categories);
-        }
-        success &= (lintResult == QQmlJSLinter::LintSuccess || lintResult == QQmlJSLinter::HasWarnings);
-        if (success) {
-            const qsizetype value = parser.isSet(maxWarnings)
-                    ? parser.value(maxWarnings).toInt()
-                    : (settings.isSet(maxWarningsSetting)
-                               ? settings.value(maxWarningsSetting).toInt()
-                               : defaultSettings.value(maxWarningsSetting).toInt());
-            if (value != -1 && value < linter.logger()->numWarnings())
-                success = false;
-        }
-
-        if (isFixing) {
-            if (lintResult != QQmlJSLinter::LintSuccess && lintResult != QQmlJSLinter::HasWarnings)
-                continue;
-
-            QString fixedCode;
-            const QQmlJSLinter::FixResult result = linter.applyFixes(&fixedCode, silent);
-
-            if (result != QQmlJSLinter::NothingToFix && result != QQmlJSLinter::FixSuccess) {
-                success = false;
-                continue;
-            }
-
-            if (parser.isSet(dryRun)) {
-                QTextStream(stdout) << fixedCode;
-            } else {
-                if (result == QQmlJSLinter::NothingToFix) {
-                    if (!silent)
-                        qWarning().nospace() << "Nothing to fix in " << filename;
-                    continue;
-                }
-
-                const QString backupFile = filename + u".bak"_s;
-                if (QFile::exists(backupFile) && !QFile::remove(backupFile)) {
-                    if (!silent) {
-                        qWarning().nospace() << "Failed to remove old backup file " << backupFile
-                                             << ", aborting";
-                    }
-                    success = false;
-                    continue;
-                }
-                if (!QFile::copy(filename, backupFile)) {
-                    if (!silent) {
-                        qWarning().nospace()
-                                << "Failed to create backup file " << backupFile << ", aborting";
-                    }
+            QQmlJSLinter::LintOptions options;
+            options.setFlag(QQmlJSLinter::Silent, silent);
+            options.setFlag(QQmlJSLinter::GenerateJson, useJson);
+            lintResult = linter.lintModule(filename, options, qmlImportPaths, resourceFiles);
+            jsonFiles.append(lintResult.json);
+            success &= (lintResult.status == QQmlJSLinter::LintSuccess
+                        || lintResult.status == QQmlJSLinter::HasWarnings);
+            if (success) {
+                const qsizetype value = parser.isSet(maxWarnings)
+                        ? parser.value(maxWarnings).toInt()
+                        : (settings.isSet(maxWarningsSetting)
+                                   ? settings.value(maxWarningsSetting).toInt()
+                                   : defaultSettings.value(maxWarningsSetting).toInt());
+                if (value != -1 && value < lintResult.logger->numWarnings())
                     success = false;
-                    continue;
-                }
+            }
 
-                QFile file(filename);
-                if (!file.open(QIODevice::WriteOnly)) {
-                    if (!silent) {
-                        qWarning().nospace() << "Failed to open " << filename
-                                             << " for writing:" << file.errorString();
-                    }
-                    success = false;
-                    continue;
+            if (isFixing)
+                success &= applyFixes(lintResult, silent, parser.isSet(dryRun));
+        } else {
+            // collect all filenames and parameters before actually linting the files
+            QQmlJSLinter::LintOptions options;
+            options.setFlag(QQmlJSLinter::Silent, silent || isFixing);
+            options.setFlag(QQmlJSLinter::GenerateJson, useJson);
+            linter.prepareFileForBatchLinting(filename, nullptr, options, qmlImportPaths,
+                                              qmldirFiles, resourceFiles, categories);
+        }
+    }
+    if (!parser.isSet(moduleOption)) {
+        for (const QString &filename : positionalArguments) {
+            QQmlJSLinter::Result lintResult = linter.lintFileInBatch(filename);
+            jsonFiles.append(lintResult.json);
+            success &= (lintResult.status == QQmlJSLinter::LintSuccess
+                        || lintResult.status == QQmlJSLinter::HasWarnings);
+
+            if (success) {
+                QQmlToolingSettings settings(
+                        QLatin1String("qmllint"),
+                        { QLatin1String("General"), QLatin1String("Warnings") });
+                if (!parser.isSet(ignoreSettings)) {
+                    QQmlToolingSettings::SearchOptions options;
+                    options.isQmllintSilent = silent;
+                    settings.search(filename, options);
                 }
 
-                const QByteArray data = fixedCode.toUtf8();
-                if (file.write(data) != data.size()) {
-                    if (!silent) {
-                        qWarning().nospace() << "Failed to write new contents to " << filename
-                                             << ": " << file.errorString();
-                    }
+                const qsizetype value = parser.isSet(maxWarnings)
+                        ? parser.value(maxWarnings).toInt()
+                        : (settings.isSet(maxWarningsSetting)
+                                   ? settings.value(maxWarningsSetting).toInt()
+                                   : defaultSettings.value(maxWarningsSetting).toInt());
+                if (value != -1 && value < lintResult.logger->numWarnings())
                     success = false;
-                    continue;
-                }
-                if (!silent) {
-                    qDebug().nospace() << "Applied fixes to " << filename << ". Backup created at "
-                                       << backupFile;
-                }
             }
+
+            if (isFixing)
+                success &= applyFixes(lintResult, silent, parser.isSet(dryRun));
         }
     }