qmllint: do not construct well known valuetypes from string (part 1)

Add a warning when well-known QML_STRUCTURED_VALUE's type are
constructed from strings in bindings, with a fix-it to
automatically generate the QML_STRUCTURED_VALUE way of constructing
value types.
Do not provide a fix-it in the case that a value type is constructed
from an invalid string, like "50x70" instead of
"50,70" for a QPointF, for example.

Previously, qmlcachegen was compiling some of the string assignments for
structured types like points, and some not, like vector2d. In the latter
case, it printed the generic "Cannot assign literal of type ..." from
the qmlIncompatible category.

This commit only handles point, size and rect. A later commit will take
care of the quick/gui types matrix4x4, vectorNd and quaternion.

Now that the methods are all accessible from one point and consistently
reused, implement qqmljsvaluetypefromstringcheck that checks if a given
type should be constructed from a stringliteral, and generates a
suggestion using the new way of constructing structured value types if
it can. This is implemented into a separate header because it might be
needed for multiple checks.
Currently, it is only needed for the stringliteral "binding" check,
but it will also be needed for stringliteral "assignment" checks,
where you warn about `myPoint ="1,2"`, for example (see QTBUG-118547).

To avoid code-duplication in the quicklintplugin, extract the code also
needed for the qquickliteralbindingcheck into a separate base class
LiteralBindingCheckBase, such that qquickliteralbindingcheck, in a later
commit, may just extend LiteralBindingCheckBase using its
isValueTypeFromStringConstruction override.

Task-number: QTBUG-118323
Fixes: QTBUG-118546
Change-Id: I8572798d94593b1044cdb49116c8468f998dbbd7
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>

11 files changed, 243 insertions, 12 deletions

diff --git a/src/qmlcompiler/CMakeLists.txt b/src/qmlcompiler/CMakeLists.txt
index 1fd1299d5e..22a1030afd 100644
--- a/src/qmlcompiler/CMakeLists.txt
+++ b/src/qmlcompiler/CMakeLists.txt
@@ -20,6 +20,7 @@ qt_internal_add_module(QmlCompiler
         qqmljsimporter.cpp qqmljsimporter_p.h
         qqmljsimportvisitor.cpp qqmljsimportvisitor_p.h
         qqmljsliteralbindingcheck.cpp qqmljsliteralbindingcheck_p.h
+        qqmljsvaluetypefromstringcheck.cpp qqmljsvaluetypefromstringcheck_p.h
         qqmljsloadergenerator.cpp qqmljsloadergenerator_p.h
         qqmljslogger_p.h qqmljslogger.cpp
         qqmljsloggingutils.h qqmljsloggingutils.cpp qqmljsloggingutils_p.h
diff --git a/src/qmlcompiler/qqmljsliteralbindingcheck.cpp b/src/qmlcompiler/qqmljsliteralbindingcheck.cpp
index e3fffc2b36..ab963cf9ce 100644
--- a/src/qmlcompiler/qqmljsliteralbindingcheck.cpp
+++ b/src/qmlcompiler/qqmljsliteralbindingcheck.cpp
@@ -8,6 +8,7 @@
 #include <private/qqmljsmetatypes_p.h>
 #include <private/qqmlsa_p.h>
 #include <private/qqmlsasourcelocation_p.h>
+#include <private/qqmlstringconverters_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -46,11 +47,17 @@ static bool canConvertForLiteralBinding(QQmlJSTypeResolver *resolver,
 }
 
 QQmlJSLiteralBindingCheck::QQmlJSLiteralBindingCheck(QQmlSA::PassManager *passManager)
-    : QQmlSA::PropertyPass(passManager),
+    : LiteralBindingCheckBase(passManager),
       m_resolver(QQmlSA::PassManagerPrivate::resolver(*passManager))
 {
 }
 
+QQmlJSStructuredTypeError QQmlJSLiteralBindingCheck::check(const QString &typeName,
+                                                           const QString &value) const
+{
+    return QQmlJSValueTypeFromStringCheck::hasError(typeName, value);
+}
+
 static QString literalPrettyTypeName(QQmlSA::BindingType type)
 {
     switch (type) {
@@ -70,14 +77,12 @@ static QString literalPrettyTypeName(QQmlSA::BindingType type)
     Q_UNREACHABLE_RETURN(QString());
 }
 
-void QQmlJSLiteralBindingCheck::onBinding(const QQmlSA::Element &element, const QString &propertyName,
-                           const QQmlSA::Binding &binding, const QQmlSA::Element &bindingScope,
-                           const QQmlSA::Element &value)
+QQmlSA::Property LiteralBindingCheckBase::getProperty(const QString &propertyName,
+                                                      const QQmlSA::Binding &binding,
+                                                      const QQmlSA::Element &bindingScope) const
 {
-    Q_UNUSED(value);
-
     if (!QQmlSA::Binding::isLiteralBinding(binding.bindingType()))
-        return;
+        return {};
 
     const QString unqualifiedPropertyName = [&propertyName]() {
         if (auto idx = propertyName.lastIndexOf(u'.'); idx != -1 && idx != propertyName.size() - 1)
@@ -85,6 +90,18 @@ void QQmlJSLiteralBindingCheck::onBinding(const QQmlSA::Element &element, const
         return propertyName;
     }();
     const auto property = bindingScope.property(unqualifiedPropertyName);
+    return property;
+}
+
+
+void LiteralBindingCheckBase::onBinding(const QQmlSA::Element &element, const QString &propertyName,
+                                        const QQmlSA::Binding &binding,
+                                        const QQmlSA::Element &bindingScope,
+                                        const QQmlSA::Element &value)
+{
+    Q_UNUSED(value);
+
+    const auto property = getProperty(propertyName, binding, bindingScope);
     if (!property.isValid())
         return;
 
@@ -95,6 +112,39 @@ void QQmlJSLiteralBindingCheck::onBinding(const QQmlSA::Element &element, const
                     qmlReadOnlyProperty, binding.sourceLocation());
         return;
     }
+    if (auto propertyType = property.type(); propertyType) {
+        auto construction = check(propertyType.internalId(), binding.stringValue());
+        if (construction.isValid()) {
+            const QString warningMessage =
+                    u"Binding is not supported: Type %1 should be constructed using"
+                    u" QML_STRUCTURED_VALUE's construction mechanism, instead of a "
+                    u"string."_s.arg(propertyType.internalId());
+
+            if (!construction.code.isNull()) {
+                QQmlSA::FixSuggestion suggestion(
+                        u"Replace string by structured value construction"_s,
+                        binding.sourceLocation(), construction.code);
+                emitWarning(warningMessage, qmlIncompatibleType, binding.sourceLocation(), suggestion);
+                return;
+            }
+            emitWarning(warningMessage, qmlIncompatibleType, binding.sourceLocation());
+            return;
+        }
+    }
+
+}
+
+void QQmlJSLiteralBindingCheck::onBinding(const QQmlSA::Element &element,
+                                          const QString &propertyName,
+                                          const QQmlSA::Binding &binding,
+                                          const QQmlSA::Element &bindingScope,
+                                          const QQmlSA::Element &value)
+{
+    LiteralBindingCheckBase::onBinding(element, propertyName, binding, bindingScope, value);
+
+    const auto property = getProperty(propertyName, binding, bindingScope);
+    if (!property.isValid())
+        return;
 
     if (!canConvertForLiteralBinding(m_resolver, resolveLiteralType(binding), property.type())) {
         emitWarning(u"Cannot assign literal of type %1 to %2"_s.arg(
diff --git a/src/qmlcompiler/qqmljsliteralbindingcheck_p.h b/src/qmlcompiler/qqmljsliteralbindingcheck_p.h
index 3fe3915b60..ca5c65e9b4 100644
--- a/src/qmlcompiler/qqmljsliteralbindingcheck_p.h
+++ b/src/qmlcompiler/qqmljsliteralbindingcheck_p.h
@@ -15,15 +15,33 @@
 // We mean it.
 
 #include <QtCore/qglobal.h>
-#include <qtqmlcompilerexports.h>
 #include <QtQmlCompiler/qqmlsa.h>
 
+#include <qtqmlcompilerexports.h>
+#include "qqmljsvaluetypefromstringcheck_p.h"
+
 QT_BEGIN_NAMESPACE
 
 class QQmlJSImportVisitor;
 class QQmlJSTypeResolver;
 
-class Q_QMLCOMPILER_EXPORT QQmlJSLiteralBindingCheck: public QQmlSA::PropertyPass
+class Q_QMLCOMPILER_EXPORT LiteralBindingCheckBase : public QQmlSA::PropertyPass
+{
+public:
+    using QQmlSA::PropertyPass::PropertyPass;
+
+    void onBinding(const QQmlSA::Element &element, const QString &propertyName,
+                   const QQmlSA::Binding &binding, const QQmlSA::Element &bindingScope,
+                   const QQmlSA::Element &value) override;
+
+protected:
+    virtual QQmlJSStructuredTypeError check(const QString &typeName, const QString &value) const = 0;
+
+    QQmlSA::Property getProperty(const QString &propertyName, const QQmlSA::Binding &binding,
+                                 const QQmlSA::Element &bindingScope) const;
+};
+
+class Q_QMLCOMPILER_EXPORT QQmlJSLiteralBindingCheck: public LiteralBindingCheckBase
 {
 public:
     QQmlJSLiteralBindingCheck(QQmlSA::PassManager *manager);
@@ -34,6 +52,9 @@ public:
 
 private:
     QQmlJSTypeResolver *m_resolver;
+
+protected:
+    QQmlJSStructuredTypeError check(const QString &typeName, const QString &value) const override;
 };
 
 QT_END_NAMESPACE
diff --git a/src/qmlcompiler/qqmljsvaluetypefromstringcheck.cpp b/src/qmlcompiler/qqmljsvaluetypefromstringcheck.cpp
new file mode 100644
index 0000000000..40ca97e579
--- /dev/null
+++ b/src/qmlcompiler/qqmljsvaluetypefromstringcheck.cpp
@@ -0,0 +1,63 @@
+// Copyright (C) 2023 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#include "qqmljsvaluetypefromstringcheck_p.h"
+#include <private/qqmlstringconverters_p.h>
+
+QT_BEGIN_NAMESPACE
+
+using namespace Qt::StringLiterals;
+
+/*!
+\internal
+\class ValueTypeFromStringError
+
+Helps to differentiate between three states:
+\list
+\li a value type was constructed by a string in a deprecated way, e.g. "50x70" for a QSizeF
+\li a value type was constructed by an invalid string in a deprecated way, e.g. "50,70" for a QSizeF
+\li a value type was constructed by a string in a non-deprecated way.
+\endlist
+*/
+
+/*!
+\internal
+Checks if known value types are being constructed in a deprecated way, and constructs the code for
+the fix-it.
+*/
+QQmlJSStructuredTypeError QQmlJSValueTypeFromStringCheck::hasError(const QString &typeName,
+                                                                  const QString &value)
+{
+    if (typeName == u"QPointF" || typeName == u"QPoint") {
+        std::array<double, 2> numbers;
+        if (!QQmlStringConverters::isValidNumberString<2, u','>(value, &numbers))
+            return QQmlJSStructuredTypeError::withInvalidString();
+
+        const auto result = QQmlJSStructuredTypeError::fromSuggestedString(
+                u"({ x: %1, y: %2 })"_s.arg(numbers[0]).arg(numbers[1]));
+        return result;
+    } else if (typeName == u"QSizeF" || typeName == u"QSize") {
+        std::array<double, 2> numbers;
+        if (!QQmlStringConverters::isValidNumberString<2, u'x'>(value, &numbers))
+            return QQmlJSStructuredTypeError::withInvalidString();
+
+        const auto result = QQmlJSStructuredTypeError::fromSuggestedString(
+                u"({ width: %1, height: %2 })"_s.arg(numbers[0]).arg(numbers[1]));
+        return result;
+    } else if (typeName == u"QRectF" || typeName == u"QRect") {
+        std::array<double, 4> numbers;
+        if (!QQmlStringConverters::isValidNumberString<4, u',', u',', u'x'>(value, &numbers))
+            return QQmlJSStructuredTypeError::withInvalidString();
+
+        const auto result = QQmlJSStructuredTypeError::fromSuggestedString(
+                u"({ x: %1, y: %2, width: %3, height: %4 })"_s.arg(numbers[0])
+                        .arg(numbers[1])
+                        .arg(numbers[2])
+                        .arg(numbers[3]));
+        return result;
+    }
+
+    return QQmlJSStructuredTypeError::withValidString();
+}
+
+QT_END_NAMESPACE
diff --git a/src/qmlcompiler/qqmljsvaluetypefromstringcheck_p.h b/src/qmlcompiler/qqmljsvaluetypefromstringcheck_p.h
new file mode 100644
index 0000000000..227a75003f
--- /dev/null
+++ b/src/qmlcompiler/qqmljsvaluetypefromstringcheck_p.h
@@ -0,0 +1,48 @@
+// Copyright (C) 2023 The Qt Company Ltd.
+// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0
+
+#ifndef QQMLJSVALUETYPEFROMSTRINGCHECK_H
+#define QQMLJSVALUETYPEFROMSTRINGCHECK_H
+
+//
+//  W A R N I N G
+//  -------------
+//
+// This file is not part of the Qt API.  It exists purely as an
+// implementation detail.  This header file may change from version to
+// version without notice, or even be removed.
+//
+// We mean it.
+//
+
+#include <QtCore/qglobal.h>
+#include <QtCore/qstring.h>
+
+#include <qtqmlcompilerexports.h>
+
+QT_BEGIN_NAMESPACE
+
+struct Q_QMLCOMPILER_EXPORT QQmlJSStructuredTypeError
+{
+    QString code;
+    bool constructedFromInvalidString = false;
+
+    bool isValid() const { return !code.isEmpty() || constructedFromInvalidString; }
+    static QQmlJSStructuredTypeError withInvalidString() { return { QString(), true }; }
+    static QQmlJSStructuredTypeError withValidString() { return { QString(), false }; }
+    static QQmlJSStructuredTypeError fromSuggestedString(const QString &enhancedString)
+    {
+        return { enhancedString, false };
+    }
+};
+
+
+class Q_QMLCOMPILER_EXPORT QQmlJSValueTypeFromStringCheck
+{
+public:
+    static QQmlJSStructuredTypeError hasError(const QString &typeName, const QString &value);
+};
+
+QT_END_NAMESPACE
+
+#endif // QQMLJSVALUETYPEFROMSTRINGCHECK_H
diff --git a/tests/auto/qml/qmllint/data/validLiterals.qml b/tests/auto/qml/qmllint/data/validLiterals.qml
index 4f8c575cd3..55792eaae2 100644
--- a/tests/auto/qml/qmllint/data/validLiterals.qml
+++ b/tests/auto/qml/qmllint/data/validLiterals.qml
@@ -29,9 +29,9 @@ QtObject {
 
     property date date1: "2021-08-13T14:16:21.435Z"
 
-    property point point1: "1,2"
+    property point point1: ({ x: 1, y: 2 })
 
-    property size size1: "50x50"
+    property size size1: ({ width: 50, height: 50 })
 
-    property rect rect1: "10,20,30x30"
+    property rect rect1: ({ x: 10, y: 20, width: 30, height: 30 })
 }
diff --git a/tests/auto/qml/qmllint/data/valueTypesFromString.qml b/tests/auto/qml/qmllint/data/valueTypesFromString.qml
new file mode 100644
index 0000000000..fc013f858f
--- /dev/null
+++ b/tests/auto/qml/qmllint/data/valueTypesFromString.qml
@@ -0,0 +1,7 @@
+import QtQuick
+
+Item {
+    property point p: "30,50"
+    property rect p3: "10, 20, 30x50"
+    property size p4: "30x50"
+}
diff --git a/tests/auto/qml/qmllint/tst_qmllint.cpp b/tests/auto/qml/qmllint/tst_qmllint.cpp
index a33e73ddb6..60cd313b48 100644
--- a/tests/auto/qml/qmllint/tst_qmllint.cpp
+++ b/tests/auto/qml/qmllint/tst_qmllint.cpp
@@ -97,6 +97,7 @@ private Q_SLOTS:
     void lintModule();
 
     void testLineEndings();
+    void valueTypesFromString();
 
 #if QT_CONFIG(library)
     void testPlugin();
@@ -1950,6 +1951,25 @@ void TestQmllint::testLineEndings()
     }
 }
 
+void TestQmllint::valueTypesFromString()
+{
+    runTest("valueTypesFromString.qml",
+            Result{ {
+                            Message{
+                                    u"Binding is not supported: Type QSizeF should be constructed using QML_STRUCTURED_VALUE's construction mechanism, instead of a string."_s },
+                            Message{
+                                    u"Binding is not supported: Type QRectF should be constructed using QML_STRUCTURED_VALUE's construction mechanism, instead of a string."_s },
+                            Message{
+                                    u"Binding is not supported: Type QPointF should be constructed using QML_STRUCTURED_VALUE's construction mechanism, instead of a string."_s },
+                    },
+                    { /*bad messages */ },
+                    {
+                            Message{ u"({ width: 30, height: 50 })"_s },
+                            Message{ u"({ x: 10, y: 20, width: 30, height: 50 })"_s },
+                            Message{ u"({ x: 30, y: 50 })"_s },
+                    } });
+}
+
 #if QT_CONFIG(library)
 void TestQmllint::testPlugin()
 {
diff --git a/tests/auto/qml/qmltc_qprocess/CMakeLists.txt b/tests/auto/qml/qmltc_qprocess/CMakeLists.txt
index 8d25214324..55266c80d9 100644
--- a/tests/auto/qml/qmltc_qprocess/CMakeLists.txt
+++ b/tests/auto/qml/qmltc_qprocess/CMakeLists.txt
@@ -42,6 +42,7 @@ qt6_add_qml_module(tst_qmltc_qprocess
         data/invalidSignalHandlers.qml
         data/QmlBaseFromAnotherModule.qml
         data/invalidTypeAnnotation.qml
+        data/constructFromString.qml
 )
 
 set(common_libraries
diff --git a/tests/auto/qml/qmltc_qprocess/data/constructFromString.qml b/tests/auto/qml/qmltc_qprocess/data/constructFromString.qml
new file mode 100644
index 0000000000..76ec189f3d
--- /dev/null
+++ b/tests/auto/qml/qmltc_qprocess/data/constructFromString.qml
@@ -0,0 +1,7 @@
+import QtQuick 2.15
+
+Item {
+    property point p: "30,50"
+    property rect p3: "10, 20, 30x50"
+    property size p4: "30x50"
+}
diff --git a/tests/auto/qml/qmltc_qprocess/tst_qmltc_qprocess.cpp b/tests/auto/qml/qmltc_qprocess/tst_qmltc_qprocess.cpp
index 404ae7dc63..fdb64f4a29 100644
--- a/tests/auto/qml/qmltc_qprocess/tst_qmltc_qprocess.cpp
+++ b/tests/auto/qml/qmltc_qprocess/tst_qmltc_qprocess.cpp
@@ -54,6 +54,7 @@ private slots:
     void exports();
     void qmlBaseFromAnotherModule();
     void invalidTypeAnnotation();
+    void constructFromString();
 };
 
 #ifndef TST_QMLTC_QPROCESS_RESOURCES
@@ -327,5 +328,17 @@ void tst_qmltc_qprocess::exports()
     QVERIFY(!implementation.contains(u"exportheader.h"_s));
 }
 
+void tst_qmltc_qprocess::constructFromString()
+{
+    const auto errors = runQmltc(u"constructFromString.qml"_s, false);
+    const QString warningMessage =
+            u"constructFromString.qml:%1:%2: Binding is not supported: Type %3 should be"
+            u" constructed using QML_STRUCTURED_VALUE's construction mechanism, instead of a"
+            u" string."_s;
+    QVERIFY(errors.contains(warningMessage.arg(4).arg(23).arg(u"QPointF")));
+    QVERIFY(errors.contains(warningMessage.arg(5).arg(23).arg(u"QRectF")));
+    QVERIFY(errors.contains(warningMessage.arg(6).arg(23).arg(u"QSizeF")));
+}
+
 QTEST_MAIN(tst_qmltc_qprocess)
 #include "tst_qmltc_qprocess.moc"