QmlCompiler: Phrase the shadow check as separate pass

... and add a test to qmllint to check that it actually does something.

Task-number: QTBUG-98305
Change-Id: Ib14bc6822cc15200018646c3a0395d0786ec28a8
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Fabian Kosmale <fabian.kosmale@qt.io>

src/qmlcompiler/CMakeLists.txt

@@ -19,6 +19,7 @@ qt_internal_add_module(QmlCompilerPrivate
         qqmljsregistercontent.cpp qqmljsregistercontent_p.h
         qqmljsresourcefilemapper.cpp qqmljsresourcefilemapper_p.h
         qqmljsscope.cpp qqmljsscope_p.h
+        qqmljsshadowcheck.cpp qqmljsshadowcheck_p.h
         qqmljsstreamwriter.cpp qqmljsstreamwriter_p.h
         qqmljstypedescriptionreader.cpp qqmljstypedescriptionreader_p.h
         qqmljstypepropagator.cpp qqmljstypepropagator_p.h

src/qmlcompiler/qqmljsshadowcheck.cpp

@@ -0,0 +1,115 @@
+/****************************************************************************
+**
+** Copyright (C) 2021 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the tools applications of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:GPL-EXCEPT$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include "qqmljsshadowcheck_p.h"
+
+QT_BEGIN_NAMESPACE
+
+void QQmlJSShadowCheck::run(
+        const InstructionAnnotations *annotations, const Function *function,
+        QQmlJS::DiagnosticMessage *error)
+{
+    m_annotations = annotations;
+    m_function = function;
+    m_error = error;
+    m_state = initialState(function, m_typeResolver);
+    decode(m_function->code.constData(), static_cast<uint>(m_function->code.length()));
+}
+
+void QQmlJSShadowCheck::generate_LoadProperty(int nameIndex)
+{
+    checkShadowing(m_state.accumulatorIn, m_jsUnitGenerator->stringForIndex(nameIndex));
+}
+
+void QQmlJSShadowCheck::generate_GetLookup(int index)
+{
+    checkShadowing(m_state.accumulatorIn, m_jsUnitGenerator->lookupName(index));
+}
+
+void QQmlJSShadowCheck::generate_StoreProperty(int nameIndex, int base)
+{
+    checkShadowing(m_state.registers[base], m_jsUnitGenerator->stringForIndex(nameIndex));
+}
+
+void QQmlJSShadowCheck::generate_SetLookup(int index, int base)
+{
+    checkShadowing(m_state.registers[base], m_jsUnitGenerator->lookupName(index));
+}
+
+QV4::Moth::ByteCodeHandler::Verdict QQmlJSShadowCheck::startInstruction(QV4::Moth::Instr::Type)
+{
+    m_state = nextStateFromAnnotations(m_state, *m_annotations);
+    return ProcessInstruction;
+}
+
+void QQmlJSShadowCheck::endInstruction(QV4::Moth::Instr::Type)
+{
+}
+
+void QQmlJSShadowCheck::checkShadowing(
+        const QQmlJSRegisterContent &baseType, const QString &memberName)
+{
+    if (baseType.storedType()->accessSemantics() != QQmlJSScope::AccessSemantics::Reference)
+        return;
+
+    switch (baseType.variant()) {
+    case QQmlJSRegisterContent::ObjectProperty:
+    case QQmlJSRegisterContent::ExtensionObjectProperty:
+    case QQmlJSRegisterContent::ScopeProperty:
+    case QQmlJSRegisterContent::ExtensionScopeProperty: {
+        const QQmlJSRegisterContent member = m_typeResolver->memberType(baseType, memberName);
+
+        // You can have something like parent.QtQuick.Screen.pixelDensity
+        // In that case "QtQuick" cannot be resolved as member type and we would later have to look
+        // for "QtQuick.Screen" instead. However, you can only do that with attached properties and
+        // those are not shadowable.
+        if (!member.isValid()) {
+            Q_ASSERT(m_typeResolver->isPrefix(memberName));
+            return;
+        }
+
+        if (member.isProperty()) {
+            if (member.property().isFinal())
+                return; // final properties can't be shadowed
+        } else if (!member.isMethod()) {
+            return; // Only properties and methods can be shadowed
+        }
+
+        setError(u"Member %1 of %2 can be shadowed"_qs
+                         .arg(memberName, m_state.accumulatorIn.descriptiveName()));
+        return;
+    }
+    default:
+        // In particular ObjectById is fine as that cannot change into something else
+        // Singleton should also be fine, unless the factory function creates an object
+        // with different property types than the declared class.
+        return;
+    }
+}
+
+QT_END_NAMESPACE

src/qmlcompiler/qqmljsshadowcheck_p.h

@@ -0,0 +1,76 @@
+/****************************************************************************
+**
+** Copyright (C) 2021 The Qt Company Ltd.
+** Contact: https://www.qt.io/licensing/
+**
+** This file is part of the tools applications of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:GPL-EXCEPT$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and The Qt Company. For licensing terms
+** and conditions see https://www.qt.io/terms-conditions. For further
+** information use the contact form at https://www.qt.io/contact-us.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3 as published by the Free Software
+** Foundation with exceptions as appearing in the file LICENSE.GPL3-EXCEPT
+** included in the packaging of this file. Please review the following
+** information to ensure the GNU General Public License requirements will
+** be met: https://www.gnu.org/licenses/gpl-3.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#ifndef QQMLJSSHADOWCHECK_P_H
+#define QQMLJSSHADOWCHECK_P_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 <private/qqmljscompilepass_p.h>
+
+QT_BEGIN_NAMESPACE
+
+class QQmlJSShadowCheck : public QQmlJSCompilePass
+{
+public:
+    QQmlJSShadowCheck(QV4::Compiler::JSUnitGenerator *jsUnitGenerator,
+                      QQmlJSTypeResolver *typeResolver, QQmlJSLogger *logger)
+        : QQmlJSCompilePass(jsUnitGenerator, typeResolver, logger)
+    {}
+
+    ~QQmlJSShadowCheck() = default;
+
+    void run(const InstructionAnnotations *annotations, const Function *function,
+             QQmlJS::DiagnosticMessage *error);
+
+private:
+    void generate_LoadProperty(int nameIndex) override;
+    void generate_GetLookup(int index) override;
+    void generate_StoreProperty(int nameIndex, int base) override;
+    void generate_SetLookup(int index, int base) override;
+
+    QV4::Moth::ByteCodeHandler::Verdict startInstruction(QV4::Moth::Instr::Type) override;
+    void endInstruction(QV4::Moth::Instr::Type) override;
+
+    void checkShadowing(const QQmlJSRegisterContent &baseType, const QString &propertyName);
+
+    const InstructionAnnotations *m_annotations = nullptr;
+    State m_state;
+};
+
+QT_END_NAMESPACE
+
+#endif // QQMLJSSHADOWCHECK_P_H

src/qmlcompiler/qqmljstypepropagator.cpp

@@ -639,8 +639,6 @@ void QQmlJSTypePropagator::propagatePropertyLookup(const QString &propertyName)
                     QString::fromLatin1("Type of property \"%2\" not found").arg(propertyName),
                     Log_Type, getCurrentSourceLocation());
         }
-
-        rejectShadowableMember(m_state.accumulatorIn, m_state.accumulatorOut);
     }
 }
 
@@ -694,8 +692,6 @@ void QQmlJSTypePropagator::generate_StoreProperty(int nameIndex, int base)
                          .arg(m_state.accumulatorIn.descriptiveName(), property.descriptiveName()));
         return;
     }
-
-    rejectShadowableMember(callBase, property);
 }
 
 void QQmlJSTypePropagator::generate_SetLookup(int index, int base)
@@ -1692,40 +1688,4 @@ bool QQmlJSTypePropagator::canConvertFromTo(const QQmlJSRegisterContent &from,
     return m_typeResolver->canConvertFromTo(from, to);
 }
 
-void QQmlJSTypePropagator::rejectShadowableMember(const QQmlJSRegisterContent &base,
-                                                  const QQmlJSRegisterContent &member)
-{
-    if (m_typeResolver->semantics() == QQmlJSTypeResolver::Dynamic
-        && member.scopeType()->accessSemantics() == QQmlJSScope::AccessSemantics::Reference) {
-        switch (base.variant()) {
-        case QQmlJSRegisterContent::ObjectProperty:
-        case QQmlJSRegisterContent::ExtensionObjectProperty:
-        case QQmlJSRegisterContent::ScopeProperty:
-        case QQmlJSRegisterContent::ExtensionScopeProperty: {
-            QString name;
-
-            if (member.isProperty()) {
-                const QQmlJSMetaProperty property = member.property();
-                if (property.isFinal()) // final properties can't be shadowed
-                    break;
-                name = member.property().propertyName();
-            } else if (member.isMethod()) {
-                name = member.method().front().methodName();
-            } else {
-                Q_UNREACHABLE();
-            }
-
-            setError(u"Member %1 of %2 can be shadowed"_qs
-                             .arg(name, m_state.accumulatorIn.descriptiveName()));
-            return;
-        }
-        default:
-            // In particular ObjectById is fine as that cannot change into something else
-            // Singleton should also be fine, unless the factory function creates an object
-            // with different property types than the declared class.
-            break;
-        }
-    }
-}
-
 QT_END_NAMESPACE

src/qmlcompiler/qqmljstypepropagator_p.h

@@ -220,8 +220,6 @@ private:
     void propagateScopeLookupCall(const QString &functionName, int argc, int argv);
     void saveRegisterStateForJump(int offset);
     bool canConvertFromTo(const QQmlJSRegisterContent &from, const QQmlJSRegisterContent &to);
-    void rejectShadowableMember(const QQmlJSRegisterContent &base,
-                                const QQmlJSRegisterContent &property);
 
     QString registerName(int registerIndex) const;
 

tests/auto/qml/qmllint/data/shadowable.qml

@@ -0,0 +1,6 @@
+import QtQml
+
+QtObject {
+    property NotSoSimple a: NotSoSimple {}
+    property int b: a.complicated
+}

tests/auto/qml/qmllint/tst_qmllint.cpp

@@ -82,6 +82,7 @@ private Q_SLOTS:
 
     void attachedPropertyReuse();
 
+    void shadowable();
     void tooFewParameters();
     void qQmlV4Function();
     void missingBuiltinsNoCrash();
@@ -1055,6 +1056,12 @@ void TestQmllint::attachedPropertyReuse()
                             "scope. Reference it by id instead")));
 }
 
+void TestQmllint::shadowable()
+{
+    QVERIFY(runQmllint("shadowable.qml", false, {"--compiler=warning"}, false).contains(
+            QStringLiteral("with type NotSoSimple can be shadowed")));
+}
+
 void TestQmllint::tooFewParameters()
 {
     QVERIFY(runQmllint("tooFewParams.qml", false, {"--compiler=warning"}, false).contains(

tools/qmllint/codegen.cpp

@@ -28,8 +28,9 @@
 
 #include "codegen.h"
 
-#include <QtQmlCompiler/private/qqmljstypepropagator_p.h>
 #include <QtQmlCompiler/private/qqmljsimportvisitor_p.h>
+#include <QtQmlCompiler/private/qqmljsshadowcheck_p.h>
+#include <QtQmlCompiler/private/qqmljstypepropagator_p.h>
 
 #include <QFileInfo>
 
@@ -326,7 +327,11 @@ bool Codegen::generateFunction(
     function->code = context->code;
     function->sourceLocations = context->sourceLocationTable.get();
 
-    propagator.run(function, error);
+    QQmlJSCompilePass::InstructionAnnotations annotations = propagator.run(function, error);
+    if (!error->isValid()) {
+        QQmlJSShadowCheck shadowCheck(m_unitGenerator, m_typeResolver.get(), m_logger);
+        shadowCheck.run(&annotations, function, error);
+    }
     if (error->isValid()) {
         error->type = context->returnsClosure ? QtDebugMsg : QtWarningMsg;
         return false;