C++: Offer only signals when completing in a connect() call

... at the second argument.
The logic is as follows: The clang code model checks whether the set of
completions contains any signals. If so, it instructs the built-in code
model to analyze the AST to find out whether the completion location was
at the second argument of a call to QObject::connect(). In that case, we
filter out all non-signals, because they are not valid at that location.

Fixes: QTCREATORBUG-13558
Change-Id: I9c7d0bd16161c723aef822280626cd06ece7df93
Reviewed-by: Christian Stenger <christian.stenger@qt.io>

src/plugins/clangcodemodel/clangcompletionassistprocessor.cpp

@@ -117,9 +117,8 @@ static bool isTheSameFunctionOverload(const CodeCompletion &completion,
             && lastItem->text() == name;
 }
 
-static QList<AssistProposalItemInterface *> toAssistProposalItems(
+QList<AssistProposalItemInterface *> ClangCompletionAssistProcessor::toAssistProposalItems(
-        const CodeCompletions &completions,
+        const CodeCompletions &completions) const
-        const ClangCompletionAssistInterface *interface)
 {
     // TODO: Handle Qt4's SIGNAL/SLOT
     //   Possibly check for m_completionOperator == T_SIGNAL
@@ -127,7 +126,23 @@ static QList<AssistProposalItemInterface *> toAssistProposalItems(
 
     QList<AssistProposalItemInterface *> items;
     items.reserve(completions.size());
+
+    // If there are signals among the candidates, we employ the built-in code model to find out
+    // whether the cursor was on the second argument of a (dis)connect() call.
+    // If so, we offer only signals, as nothing else makes sense in that context.
+    bool considerOnlySignals = false;
+    if (m_position != -1 && Utils::anyOf(completions, [](const CodeCompletion &c) {
+        return c.completionKind == CodeCompletion::SignalCompletionKind;
+    })) {
+        considerOnlySignals = CppTools::CppModelManager::instance()
+                ->positionRequiresSignal(m_interface->fileName(), m_content, m_position);
+    }
+
     for (const CodeCompletion &codeCompletion : completions) {
+        if (considerOnlySignals && codeCompletion.completionKind
+                != CodeCompletion::SignalCompletionKind) {
+            continue;
+        }
         if (codeCompletion.text.isEmpty())
             continue; // It's an OverloadCandidate which has text but no typedText.
 
@@ -146,7 +161,7 @@ static QList<AssistProposalItemInterface *> toAssistProposalItems(
         } else {
             auto *lastItem = static_cast<ClangAssistProposalItem *>(items.last());
             if (isTheSameFunctionOverload(codeCompletion, name, lastItem)) {
-                addFunctionOverloadAssistProposalItem(items, items.back(), interface,
+                addFunctionOverloadAssistProposalItem(items, items.back(), m_interface.data(),
                                                       codeCompletion, name);
             } else {
                 addAssistProposalItem(items, codeCompletion, name);
@@ -235,9 +250,9 @@ void ClangCompletionAssistProcessor::handleAvailableCompletions(const CodeComple
     // Completions are sorted the way that all items with fix-its come after all items without them
     // therefore it's enough to check only the first one.
     if (!completions.isEmpty() && !completions.front().requiredFixIts.isEmpty())
-        m_completions = toAssistProposalItems(applyCompletionFixIt(completions), m_interface.data());
+        m_completions = toAssistProposalItems(applyCompletionFixIt(completions));
     else
-        m_completions = toAssistProposalItems(completions, m_interface.data());
+        m_completions = toAssistProposalItems(completions);
 
     if (m_addSnippets && !m_completions.isEmpty())
         addSnippets();
@@ -681,6 +696,13 @@ bool ClangCompletionAssistProcessor::sendCompletionRequest(int position,
                                         functionNameStart.line,
                                         functionNameStart.column);
         setLastCompletionPosition(filePath, position);
+        if (m_sentRequestType == NormalCompletion) {
+            if (!customFileContent.isEmpty())
+                m_content = customFileContent;
+            else if (const CppTools::CppEditorDocumentHandle * const doc = cppDocument(filePath))
+                m_content = doc->contents();
+            m_position = position;
+        }
         return true;
     }
 

src/plugins/clangcodemodel/clangcompletionassistprocessor.h

@@ -66,6 +66,8 @@ private:
     TextEditor::IAssistProposal *createFunctionHintProposal(
             const CodeCompletions &completions);
 
+    QList<TextEditor::AssistProposalItemInterface *> toAssistProposalItems(
+            const CodeCompletions &completions) const;
     bool completeInclude(const QTextCursor &cursor);
     bool completeInclude(int position);
     void completeIncludePath(const QString &realPath, const QStringList &suffixes);
@@ -96,6 +98,8 @@ private:
     unsigned m_completionOperator;
     enum CompletionRequestType { NormalCompletion, FunctionHintCompletion };
     CompletionRequestType m_sentRequestType = NormalCompletion;
+    int m_position = -1;
+    QByteArray m_content;
     bool m_requestSent = false;
     bool m_addSnippets = false; // For type == Type::NormalCompletion
     bool m_fallbackToNormalCompletion = true;

src/plugins/clangcodemodel/test/clangcodecompletion_test.cpp

@@ -112,15 +112,7 @@ public:
         QTC_ASSERT(resource.isValid(), return);
         const QByteArray contents = QByteArray(reinterpret_cast<const char*>(resource.data()),
                                                resource.size());
-        cursorPosition = findCursorMarkerPosition(contents);
+        finish(fileName, contents, temporaryDir);
-        if (!contents.isEmpty()) {
-            if (!temporaryDir) {
-                m_temporaryDir.reset(new CppTools::Tests::TemporaryDir);
-                temporaryDir = m_temporaryDir.data();
-            }
-
-            filePath = temporaryDir->createFile(fileName, contents);
-        }
     }
 
     static TestDocument fromExistingFile(const QString &filePath)
@@ -132,6 +124,14 @@ public:
         return testDocument;
     }
 
+    static TestDocument fromString(const QByteArray &fileName, const QByteArray &contents,
+                                   CppTools::Tests::TemporaryDir *tempDir = nullptr)
+    {
+        TestDocument testDocument;
+        testDocument.finish(fileName, contents, tempDir);
+        return testDocument;
+    }
+
     static int findCursorMarkerPosition(const QByteArray &contents)
     {
         return contents.indexOf(" /* COMPLETE HERE */");
@@ -147,6 +147,21 @@ public:
 
 private:
     TestDocument() = default;
+
+    void finish(const QByteArray &fileName, const QByteArray &contents,
+                CppTools::Tests::TemporaryDir *temporaryDir = nullptr)
+    {
+        cursorPosition = findCursorMarkerPosition(contents);
+        if (!contents.isEmpty()) {
+            if (!temporaryDir) {
+                m_temporaryDir.reset(new CppTools::Tests::TemporaryDir);
+                temporaryDir = m_temporaryDir.data();
+            }
+
+            filePath = temporaryDir->createFile(fileName, contents);
+        }
+    }
+
     QSharedPointer<CppTools::Tests::TemporaryDir> m_temporaryDir;
 };
 
@@ -313,12 +328,15 @@ class ProjectLessCompletionTest
 public:
     ProjectLessCompletionTest(const QByteArray &testFileName,
                               const QString &textToInsert = QString(),
-                              const QStringList &includePaths = QStringList())
+                              const QStringList &includePaths = QStringList(),
+                              const QByteArray &contents = {})
     {
         CppTools::Tests::TestCase garbageCollectionGlobalSnapshot;
         QVERIFY(garbageCollectionGlobalSnapshot.succeededSoFar());
 
-        const TestDocument testDocument(testFileName, globalTemporaryDir());
+        const auto testDocument = contents.isEmpty()
+                ? TestDocument(testFileName, globalTemporaryDir())
+                : TestDocument::fromString(testFileName, contents, globalTemporaryDir());
         QVERIFY(testDocument.isCreatedAndHasValidCursorPosition());
         OpenEditorAtCursorPosition openEditor(testDocument);
         QVERIFY(openEditor.succeeded());
@@ -712,6 +730,95 @@ void ClangCodeCompletionTest::testCompleteProjectDependingCodeInGeneratedUiFile(
     QVERIFY(hasItem(proposal, "setupUi"));
 }
 
+static QByteArray makeSignalCompletionContents(const QByteArray &customContents)
+{
+    static const QByteArray definitions = R"(
+        class QObject {
+        public:
+            void aSignal() __attribute__((annotate("qt_signal")));
+            void anotherSignal() __attribute__((annotate("qt_signal")));
+            void notASignal();
+            static void connect();
+            static void disconnect();
+        };
+        class DerivedFromQObject : public QObject {
+        public:
+            void myOwnSignal() __attribute__((annotate("qt_signal")));
+            void alsoNotASignal();
+        };
+        class NotAQObject {
+        public:
+            void notASignal();
+            void alsoNotASignal();
+            static void connect();
+        };)";
+
+    return definitions + customContents + " /* COMPLETE HERE */";
+}
+
+void ClangCodeCompletionTest::testSignalCompletion_data()
+{
+    QTest::addColumn<QByteArray>("customContents");
+    QTest::addColumn<QByteArrayList>("hits");
+
+    QTest::addRow("positive: connect() on QObject class")
+            << QByteArray("int main() { QObject::connect(dummy, QObject::")
+            << QByteArrayList{"aSignal", "anotherSignal"};
+    QTest::addRow("positive: connect() on QObject object")
+            << QByteArray("int main() { QObject o; o.connect(dummy, QObject::")
+            << QByteArrayList{"aSignal", "anotherSignal"};
+    QTest::addRow("positive: connect() on QObject pointer")
+            << QByteArray("int main() { QObject *o; o->connect(dummy, QObject::")
+            << QByteArrayList{"aSignal", "anotherSignal"};
+    QTest::addRow("positive: connect() on QObject rvalue")
+            << QByteArray("int main() { QObject().connect(dummy, QObject::")
+            << QByteArrayList{"aSignal", "anotherSignal"};
+    QTest::addRow("positive: connect() on QObject pointer rvalue")
+            << QByteArray("int main() { (new QObject)->connect(dummy, QObject::")
+            << QByteArrayList{"aSignal", "anotherSignal"};
+    QTest::addRow("positive: disconnect() on QObject")
+            << QByteArray("int main() { QObject::disconnect(dummy, QObject::")
+            << QByteArrayList{"aSignal", "anotherSignal"};
+    QTest::addRow("positive: connect() in member function of derived class")
+            << QByteArray("void DerivedFromQObject::alsoNotASignal() { connect(this, DerivedFromQObject::")
+            << QByteArrayList{"aSignal", "anotherSignal", "myOwnSignal"};
+
+    const QByteArrayList allQObjectFunctions{"aSignal", "anotherSignal", "notASignal", "connect",
+                                             "disconnect", "QObject", "~QObject", "operator="};
+    QTest::addRow("negative: different function name")
+            << QByteArray("int main() { QObject::notASignal(dummy, QObject::")
+            << allQObjectFunctions;
+    QTest::addRow("negative: connect function from other class")
+            << QByteArray("int main() { NotAQObject::connect(dummy, QObject::")
+            << allQObjectFunctions;
+    QTest::addRow("negative: first argument")
+            << QByteArray("int main() { QObject::connect(QObject::")
+            << allQObjectFunctions;
+    QTest::addRow("negative: third argument")
+            << QByteArray("int main() { QObject::connect(dummy1, dummy2, QObject::")
+            << allQObjectFunctions;
+
+    QTest::addRow("negative: not a QObject")
+            << QByteArray("int main() { QObject::connect(dummy, NotAQObject::")
+            << QByteArrayList{"notASignal", "alsoNotASignal", "connect", "NotAQObject",
+                              "~NotAQObject", "operator="};
+}
+
+
+void ClangCodeCompletionTest::testSignalCompletion()
+{
+    QFETCH(QByteArray, customContents);
+    QFETCH(QByteArrayList, hits);
+
+    const QByteArray contents = makeSignalCompletionContents(customContents);
+    const ProjectLessCompletionTest t("signalcompletion.cpp", {}, {}, contents);
+
+    QVERIFY(t.proposal);
+    QCOMPARE(t.proposal->size(), hits.size());
+    for (const QByteArray &hit : qAsConst(hits))
+        QVERIFY(hasItem(t.proposal, hit));
+}
+
 } // namespace Tests
 } // namespace Internal
 } // namespace ClangCodeModel

src/plugins/clangcodemodel/test/clangcodecompletion_test.h

@@ -57,6 +57,9 @@ private slots:
     void testCompleteProjectDependingCode();
     void testCompleteProjectDependingCodeAfterChangingProject();
     void testCompleteProjectDependingCodeInGeneratedUiFile();
+
+    void testSignalCompletion_data();
+    void testSignalCompletion();
 };
 
 } // namespace Tests

src/plugins/cpptools/cppmodelmanager.cpp

@@ -56,6 +56,8 @@
 #include <coreplugin/icore.h>
 #include <coreplugin/progressmanager/progressmanager.h>
 #include <coreplugin/vcsmanager.h>
+#include <cplusplus/ASTPath.h>
+#include <cplusplus/TypeOfExpression.h>
 #include <extensionsystem/pluginmanager.h>
 #include <projectexplorer/project.h>
 #include <projectexplorer/projectexplorer.h>
@@ -343,6 +345,114 @@ void CppModelManager::globalFollowSymbol(
                                symbolFinder, inNextSplit);
 }
 
+bool CppModelManager::positionRequiresSignal(const QString &filePath, const QByteArray &content,
+                                             int position) const
+{
+    if (content.isEmpty())
+        return false;
+
+    // Insert a dummy prefix if we don't have a real one. Otherwise the AST path will not contain
+    // anything after the CallAST.
+    QByteArray fixedContent = content;
+    if (position > 2 && content.mid(position - 2, 2) == "::")
+        fixedContent.insert(position, 'x');
+
+    const Snapshot snapshot = this->snapshot();
+    const Document::Ptr document = snapshot.preprocessedDocument(fixedContent, filePath);
+    document->check();
+    QTextDocument textDocument(QString::fromUtf8(fixedContent));
+    QTextCursor cursor(&textDocument);
+    cursor.setPosition(position);
+
+    // Are we at the second argument of a function call?
+    const QList<AST *> path = ASTPath(document)(cursor);
+    if (path.isEmpty() || !path.last()->asSimpleName())
+        return false;
+    const CallAST *callAst = nullptr;
+    for (auto it = path.crbegin(); it != path.crend(); ++it) {
+        if ((callAst = (*it)->asCall()))
+            break;
+    }
+    if (!callAst)
+        return false;
+    if (!callAst->expression_list || !callAst->expression_list->next)
+        return false;
+    const ExpressionAST * const secondArg = callAst->expression_list->next->value;
+    if (secondArg->firstToken() > path.last()->firstToken()
+            || secondArg->lastToken() < path.last()->lastToken()) {
+        return false;
+    }
+
+    // Is the function called "connect" or "disconnect"?
+    if (!callAst->base_expression)
+        return false;
+    Scope *scope = document->globalNamespace();
+    for (auto it = path.crbegin(); it != path.crend(); ++it) {
+        if (const CompoundStatementAST * const stmtAst = (*it)->asCompoundStatement()) {
+            scope = stmtAst->symbol;
+            break;
+        }
+    }
+    const NameAST *nameAst = nullptr;
+    const LookupContext context(document, snapshot);
+    if (const IdExpressionAST * const idAst = callAst->base_expression->asIdExpression()) {
+        nameAst = idAst->name;
+    } else if (const MemberAccessAST * const ast = callAst->base_expression->asMemberAccess()) {
+        nameAst = ast->member_name;
+        TypeOfExpression exprType;
+        exprType.setExpandTemplates(true);
+        exprType.init(document, snapshot);
+        const QList<LookupItem> typeMatches = exprType(ast->base_expression, document, scope);
+        if (typeMatches.isEmpty())
+            return false;
+        const std::function<const NamedType *(const FullySpecifiedType &)> getNamedType
+                = [&getNamedType](const FullySpecifiedType &type ) -> const NamedType * {
+            Type * const t = type.type();
+            if (const auto namedType = t->asNamedType())
+                return namedType;
+            if (const auto pointerType = t->asPointerType())
+                return getNamedType(pointerType->elementType());
+            if (const auto refType = t->asReferenceType())
+                return getNamedType(refType->elementType());
+            return nullptr;
+        };
+        const NamedType *namedType = getNamedType(typeMatches.first().type());
+        if (!namedType && typeMatches.first().declaration())
+            namedType = getNamedType(typeMatches.first().declaration()->type());
+        if (!namedType)
+            return false;
+        const ClassOrNamespace * const result = context.lookupType(namedType->name(), scope);
+        if (!result)
+            return false;
+        scope = result->rootClass();
+        if (!scope)
+            return false;
+    }
+    if (!nameAst || !nameAst->name)
+        return false;
+    const Identifier * const id = nameAst->name->identifier();
+    if (!id)
+        return false;
+    const QString funcName = QString::fromUtf8(id->chars(), id->size());
+    if (funcName != "connect" && funcName != "disconnect")
+        return false;
+
+    // Is the function a member function of QObject?
+    const QList<LookupItem> matches = context.lookup(nameAst->name, scope);
+    for (const LookupItem &match : matches) {
+        if (!match.scope())
+            continue;
+        const Class *klass = match.scope()->asClass();
+        if (!klass || !klass->name())
+            continue;
+        const Identifier * const classId = klass->name()->identifier();
+        if (classId && QString::fromUtf8(classId->chars(), classId->size()) == "QObject")
+            return true;
+    }
+
+    return false;
+}
+
 void CppModelManager::addRefactoringEngine(RefactoringEngineType type,
                                            RefactoringEngineInterface *refactoringEngine)
 {

src/plugins/cpptools/cppmodelmanager.h

@@ -169,6 +169,9 @@ public:
                             SymbolFinder *symbolFinder,
                             bool inNextSplit) const final;
 
+    bool positionRequiresSignal(const QString &filePath, const QByteArray &content,
+                                int position) const;
+
     void renameUsages(CPlusPlus::Symbol *symbol, const CPlusPlus::LookupContext &context,
                       const QString &replacement = QString());
     void findUsages(CPlusPlus::Symbol *symbol, const CPlusPlus::LookupContext &context);