Decouple QQmlJSLogger from QQmlImportVisitor

QQmlImportVisitor was accepting ctor paratemeters for the QQmlJSLogger,
creating own logger internally. This seems wrong since in that case we
kind of have separate logger for visitor and type resolver (among other
entities)

On top of this, the import visitor had a silent logging by default
(and the QQmlJSLogger is not silent on the contrary) which in fact hid
some issues that should've been reported by qmllint (but they weren't)
For consistency, the silent logger is still used. And the ultimate fix
would be to use FindWarningsVisitor instead of QQmlJSImportVisitor
as currently we do 2 AST traversals in qmllint

Change-Id: I4c54b76d130e7e8f31c90a148edc1c02f7e86ab8
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

4 files changed, 41 insertions, 30 deletions

diff --git a/tools/qmllint/codegen.cpp b/tools/qmllint/codegen.cpp
index 6e711152df..0480b96f26 100644
--- a/tools/qmllint/codegen.cpp
+++ b/tools/qmllint/codegen.cpp
@@ -56,7 +56,12 @@ void Codegen::setDocument(QmlIR::JSCodeGen *codegen, QmlIR::Document *document)
             // dynamically (QTBUG-95530). Currently this has no other side effects. Re-evaluate once
             // that changes.
             QQmlJSTypeResolver::Indirect, QQmlJSTypeResolver::Static, m_logger);
-    QQmlJSImportVisitor visitor(m_importer,
+    // TODO: using emptyLogger for visitor actually hides potential issues but
+    // using m_logger instead fails some tests, so for now let's leave the old
+    // behavior for consistency. the proper fix is anyway to remove this visitor
+    // and use FindWarningsVisitor instead.
+    QQmlJSLogger emptyLogger(QString(), QString(), /* silent */ true);
+    QQmlJSImportVisitor visitor(m_importer, &emptyLogger,
                                 QQmlJSImportVisitor::implicitImportDirectory(
                                         m_fileName, m_importer->resourceFileMapper()),
                                 m_qmltypesFiles);
diff --git a/tools/qmllint/findwarnings.cpp b/tools/qmllint/findwarnings.cpp
index d2e3fdfa0c..c7bba11450 100644
--- a/tools/qmllint/findwarnings.cpp
+++ b/tools/qmllint/findwarnings.cpp
@@ -131,12 +131,13 @@ void FindWarningVisitor::endVisit(QQmlJS::AST::UiObjectDefinition *uiod)
     childScope->addOwnProperty(property);
 }
 
-FindWarningVisitor::FindWarningVisitor(QQmlJSImporter *importer, QStringList qmltypesFiles,
-                                       QString code, QList<QQmlJS::SourceLocation> comments,
-                                       QString fileName, bool silent)
-    : QQmlJSImportVisitor(importer,
-                          implicitImportDirectory(fileName, importer->resourceFileMapper()),
-                          qmltypesFiles, fileName, code, silent)
+FindWarningVisitor::FindWarningVisitor(QQmlJSImporter *importer, QQmlJSLogger *logger,
+                                       QStringList qmltypesFiles,
+                                       QList<QQmlJS::SourceLocation> comments)
+    : QQmlJSImportVisitor(
+            importer, logger,
+            implicitImportDirectory(logger->fileName(), importer->resourceFileMapper()),
+            qmltypesFiles)
 {
     parseComments(comments);
 }
@@ -147,10 +148,11 @@ void FindWarningVisitor::parseComments(const QList<QQmlJS::SourceLocation> &comm
     QHash<int, QSet<QQmlJSLoggerCategory>> enablesPerLine;
     QHash<int, QSet<QQmlJSLoggerCategory>> oneLineDisablesPerLine;
 
-    const QStringList lines = m_code.split(u'\n');
+    const QString code = m_logger->code();
+    const QStringList lines = code.split(u'\n');
 
     for (const auto &loc : comments) {
-        const QString comment = m_code.mid(loc.offset, loc.length);
+        const QString comment = code.mid(loc.offset, loc.length);
         if (!comment.startsWith(u" qmllint ") && !comment.startsWith(u"qmllint "))
             continue;
 
@@ -163,17 +165,17 @@ void FindWarningVisitor::parseComments(const QList<QQmlJS::SourceLocation> &comm
         QSet<QQmlJSLoggerCategory> categories;
         for (qsizetype i = 2; i < words.size(); i++) {
             const QString category = words.at(i);
-            const auto option = m_logger.options().constFind(category);
-            if (option != m_logger.options().constEnd())
+            const auto option = m_logger->options().constFind(category);
+            if (option != m_logger->options().constEnd())
                 categories << option->m_category;
             else
-                m_logger.logWarning(
+                m_logger->logWarning(
                         u"qmllint directive on unknown category \"%1\""_qs.arg(category),
                         Log_Syntax, loc);
         }
 
         if (categories.isEmpty()) {
-            for (const auto &option : m_logger.options())
+            for (const auto &option : m_logger->options())
                 categories << option.m_category;
         }
 
@@ -196,8 +198,8 @@ void FindWarningVisitor::parseComments(const QList<QQmlJS::SourceLocation> &comm
         } else if (command == u"enable"_qs) {
             enablesPerLine[loc.startLine + 1] |= categories;
         } else {
-            m_logger.logWarning(u"Invalid qmllint directive \"%1\" provided"_qs.arg(command),
-                                Log_Syntax, loc);
+            m_logger->logWarning(u"Invalid qmllint directive \"%1\" provided"_qs.arg(command),
+                                 Log_Syntax, loc);
         }
     }
 
@@ -211,7 +213,7 @@ void FindWarningVisitor::parseComments(const QList<QQmlJS::SourceLocation> &comm
         currentlyDisabled.unite(oneLineDisablesPerLine[i]);
 
         if (!currentlyDisabled.isEmpty())
-            m_logger.ignoreWarnings(i, currentlyDisabled);
+            m_logger->ignoreWarnings(i, currentlyDisabled);
 
         currentlyDisabled.subtract(oneLineDisablesPerLine[i]);
     }
@@ -235,5 +237,5 @@ bool FindWarningVisitor::check()
         outstandingConnection.uiod->initializer->accept(this);
     }
 
-    return !m_logger.hasWarnings() && !m_logger.hasErrors();
+    return !m_logger->hasWarnings() && !m_logger->hasErrors();
 }
diff --git a/tools/qmllint/findwarnings.h b/tools/qmllint/findwarnings.h
index eab390284c..8d9475f467 100644
--- a/tools/qmllint/findwarnings.h
+++ b/tools/qmllint/findwarnings.h
@@ -55,9 +55,8 @@ class FindWarningVisitor : public QQmlJSImportVisitor
 {
     Q_DISABLE_COPY_MOVE(FindWarningVisitor)
 public:
-    explicit FindWarningVisitor(QQmlJSImporter *importer, QStringList qmltypesFiles, QString code,
-                                QList<QQmlJS::SourceLocation> comments, QString fileName,
-                                bool silent);
+    explicit FindWarningVisitor(QQmlJSImporter *importer, QQmlJSLogger *logger,
+                                QStringList qmltypesFiles, QList<QQmlJS::SourceLocation> comments);
     ~FindWarningVisitor() override = default;
     bool check();
 
diff --git a/tools/qmllint/main.cpp b/tools/qmllint/main.cpp
index ee16409f4e..3c9d19eb58 100644
--- a/tools/qmllint/main.cpp
+++ b/tools/qmllint/main.cpp
@@ -171,18 +171,23 @@ static bool lint_file(const QString &filename, const bool silent, QJsonArray *js
 
             importer.setResourceFileMapper(mapper);
 
-            FindWarningVisitor v { &importer,         qmltypesFiles, code,
-                                   engine.comments(), filename,      silent || json };
+            QQmlJSLogger logger(filename, code, silent || json);
+            FindWarningVisitor v {
+                &importer,
+                &logger,
+                qmltypesFiles,
+                engine.comments(),
+            };
 
             for (auto it = options.cbegin(); it != options.cend(); ++it) {
-                v.logger().setCategoryError(it.value().m_category, it.value().m_error);
-                v.logger().setCategoryLevel(it.value().m_category, it.value().m_level);
+                logger.setCategoryError(it.value().m_category, it.value().m_error);
+                logger.setCategoryLevel(it.value().m_category, it.value().m_level);
             }
 
             parser.rootNode()->accept(&v);
             success = v.check();
 
-            Codegen codegen { &importer, filename, qmltypesFiles, &v.logger(), code };
+            Codegen codegen { &importer, filename, qmltypesFiles, &logger, code };
             QQmlJSSaveFunction saveFunction = [](const QV4::CompiledData::SaveableUnitPointer &,
                                                  const QQmlJSAotFunctionMap &,
                                                  QString *) { return true; };
@@ -191,17 +196,17 @@ static bool lint_file(const QString &filename, const bool silent, QJsonArray *js
 
             QLoggingCategory::setFilterRules(u"qt.qml.compiler=false"_qs);
 
-            CodegenWarningInterface interface(&v.logger());
+            CodegenWarningInterface interface(&logger);
             qCompileQmlFile(filename, saveFunction, &codegen, &error, true, &interface);
 
-            success &= !v.logger().hasWarnings() && !v.logger().hasErrors();
+            success &= !logger.hasWarnings() && !logger.hasErrors();
 
             if (json) {
-                for (const auto &error : v.logger().errors())
+                for (const auto &error : logger.errors())
                     addJsonWarning(error);
-                for (const auto &warning : v.logger().warnings())
+                for (const auto &warning : logger.warnings())
                     addJsonWarning(warning);
-                for (const auto &info : v.logger().infos())
+                for (const auto &info : logger.infos())
                     addJsonWarning(info);
             }
         };