From a79b0c6558ef8b9ca1a0f5ae553c9cf73acccc82 Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Tue, 25 Aug 2020 14:32:56 +0200 Subject: [PATCH] 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 --- .../clangcompletionassistprocessor.cpp | 34 ++++- .../clangcompletionassistprocessor.h | 4 + .../test/clangcodecompletion_test.cpp | 129 ++++++++++++++++-- .../test/clangcodecompletion_test.h | 3 + src/plugins/cpptools/cppmodelmanager.cpp | 110 +++++++++++++++ src/plugins/cpptools/cppmodelmanager.h | 3 + 6 files changed, 266 insertions(+), 17 deletions(-) diff --git a/src/plugins/clangcodemodel/clangcompletionassistprocessor.cpp b/src/plugins/clangcodemodel/clangcompletionassistprocessor.cpp index ebbc2e58e7b..19c669542db 100644 --- a/src/plugins/clangcodemodel/clangcompletionassistprocessor.cpp +++ b/src/plugins/clangcodemodel/clangcompletionassistprocessor.cpp @@ -117,9 +117,8 @@ static bool isTheSameFunctionOverload(const CodeCompletion &completion, && lastItem->text() == name; } -static QList toAssistProposalItems( - const CodeCompletions &completions, - const ClangCompletionAssistInterface *interface) +QList ClangCompletionAssistProcessor::toAssistProposalItems( + const CodeCompletions &completions) const { // TODO: Handle Qt4's SIGNAL/SLOT // Possibly check for m_completionOperator == T_SIGNAL @@ -127,7 +126,23 @@ static QList toAssistProposalItems( QList 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 toAssistProposalItems( } else { auto *lastItem = static_cast(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; } diff --git a/src/plugins/clangcodemodel/clangcompletionassistprocessor.h b/src/plugins/clangcodemodel/clangcompletionassistprocessor.h index e3ed93a2cff..4240ae0c136 100644 --- a/src/plugins/clangcodemodel/clangcompletionassistprocessor.h +++ b/src/plugins/clangcodemodel/clangcompletionassistprocessor.h @@ -66,6 +66,8 @@ private: TextEditor::IAssistProposal *createFunctionHintProposal( const CodeCompletions &completions); + QList 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; diff --git a/src/plugins/clangcodemodel/test/clangcodecompletion_test.cpp b/src/plugins/clangcodemodel/test/clangcodecompletion_test.cpp index 86948ccaafd..521dc4a6638 100644 --- a/src/plugins/clangcodemodel/test/clangcodecompletion_test.cpp +++ b/src/plugins/clangcodemodel/test/clangcodecompletion_test.cpp @@ -112,15 +112,7 @@ public: QTC_ASSERT(resource.isValid(), return); const QByteArray contents = QByteArray(reinterpret_cast(resource.data()), resource.size()); - cursorPosition = findCursorMarkerPosition(contents); - if (!contents.isEmpty()) { - if (!temporaryDir) { - m_temporaryDir.reset(new CppTools::Tests::TemporaryDir); - temporaryDir = m_temporaryDir.data(); - } - - filePath = temporaryDir->createFile(fileName, contents); - } + finish(fileName, contents, temporaryDir); } 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 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("customContents"); + QTest::addColumn("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 diff --git a/src/plugins/clangcodemodel/test/clangcodecompletion_test.h b/src/plugins/clangcodemodel/test/clangcodecompletion_test.h index d04a5828faa..150ef9651ba 100644 --- a/src/plugins/clangcodemodel/test/clangcodecompletion_test.h +++ b/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 diff --git a/src/plugins/cpptools/cppmodelmanager.cpp b/src/plugins/cpptools/cppmodelmanager.cpp index ae4083f350e..ca8fe286868 100644 --- a/src/plugins/cpptools/cppmodelmanager.cpp +++ b/src/plugins/cpptools/cppmodelmanager.cpp @@ -56,6 +56,8 @@ #include #include #include +#include +#include #include #include #include @@ -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 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 typeMatches = exprType(ast->base_expression, document, scope); + if (typeMatches.isEmpty()) + return false; + const std::function 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 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) { diff --git a/src/plugins/cpptools/cppmodelmanager.h b/src/plugins/cpptools/cppmodelmanager.h index 292f333b8df..4d44e58a0be 100644 --- a/src/plugins/cpptools/cppmodelmanager.h +++ b/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);