CppEditor: Merge AddIncludeForForwardDeclaration into AddIncludeForUndefinedIdentifier

* Fixes multiple addition of same include.
* Takes over the check of AddIncludeForForwardDeclaration for forward
  headers.

Task-number: QTCREATORBUG-9704
Change-Id: I84629d35ae433385942a9157e5d32ef04159d07f
Reviewed-by: Christian Stenger <christian.stenger@digia.com>

doc/src/editors/creator-editors.qdoc

@@ -1827,10 +1827,6 @@
 
             \li  String literal
 
-        \row
-            \li  #include Header File
-            \li  Adds the matching #include statement for a forward-declared class or struct
-            \li  Forward-declared class or struct
         \row
             \li  Add Definition in ...
             \li  Inserts a definition stub for a function declaration either in the header file
@@ -1957,9 +1953,9 @@
                 icon appears:
                 \inlineimage qml-toolbar-indicator.png
         \row
-            \li Add #include for undeclared identifier
+            \li Add #include for undeclared or forward declared identifier
             \li Adds an #include directive to the current file to make the
-                declaration of a symbol available.
+                definition of a symbol available.
             \li Undeclared identifier
         \row
             \li Reformat Pointers or References

src/plugins/cppeditor/cppeditorplugin.h

@@ -156,6 +156,9 @@ private slots:
     void test_quickfix_AddIncludeForUndefinedIdentifier_onBaseOfQualifiedName();
     void test_quickfix_AddIncludeForUndefinedIdentifier_onTemplateName();
     void test_quickfix_AddIncludeForUndefinedIdentifier_onTemplateNameInsideArguments();
+    void test_quickfix_AddIncludeForUndefinedIdentifier_withForwardDeclaration();
+    void test_quickfix_AddIncludeForUndefinedIdentifier_withForwardDeclaration2();
+    void test_quickfix_AddIncludeForUndefinedIdentifier_withForwardHeader();
     void test_quickfix_AddIncludeForUndefinedIdentifier_inserting_ignoremoc();
     void test_quickfix_AddIncludeForUndefinedIdentifier_inserting_sortingTop();
     void test_quickfix_AddIncludeForUndefinedIdentifier_inserting_sortingMiddle();

src/plugins/cppeditor/cppquickfix_test.cpp

@@ -219,11 +219,11 @@ QuickFixTestCase::~QuickFixTestCase()
 }
 
 void QuickFixTestCase::run(const QList<QuickFixTestDocument::Ptr> &theTestFiles,
-                           CppQuickFixFactory *factory, const QString &incPath)
+                           CppQuickFixFactory *factory, const QString &incPath, int resultIndex)
 {
     ProjectPart::HeaderPaths hps;
     hps += ProjectPart::HeaderPath(incPath, ProjectPart::HeaderPath::IncludePath);
-    QuickFixTestCase(theTestFiles, factory, hps);
+    QuickFixTestCase(theTestFiles, factory, hps, resultIndex);
 }
 
 /// Delegates directly to AddIncludeForUndefinedIdentifierOp for easier testing.
@@ -2404,6 +2404,137 @@ void CppEditorPlugin::test_quickfix_AddIncludeForUndefinedIdentifier_onTemplateN
     QuickFixTestCase::run(testFiles, &factory, TestIncludePaths::globalIncludePath());
 }
 
+void CppEditorPlugin::test_quickfix_AddIncludeForUndefinedIdentifier_withForwardDeclaration()
+{
+    QList<QuickFixTestDocument::Ptr> testFiles;
+
+    QByteArray original;
+    QByteArray expected;
+
+    // Header File
+    original = "class Foo {};\n";
+    expected = original;
+    testFiles << QuickFixTestDocument::create(TestIncludePaths::directoryOfTestFile().toUtf8()
+                                              + "/afile.h", original, expected);
+
+    // Source File
+    original =
+        "#include \"header.h\"\n"
+        "\n"
+        "class Foo;\n"
+        "\n"
+        "void f()\n"
+        "{\n"
+        "    @Foo foo;\n"
+        "}\n"
+        ;
+    expected =
+        "#include \"afile.h\"\n"
+        "#include \"header.h\"\n"
+        "\n"
+        "class Foo;\n"
+        "\n"
+        "void f()\n"
+        "{\n"
+        "    Foo foo;\n"
+        "}\n"
+        ;
+    testFiles << QuickFixTestDocument::create(TestIncludePaths::directoryOfTestFile().toUtf8()
+                                              + "/afile.cpp", original, expected);
+
+    // Do not use the test factory, at least once we want to go through the "full stack".
+    AddIncludeForUndefinedIdentifier factory;
+    QuickFixTestCase::run(testFiles, &factory, TestIncludePaths::globalIncludePath());
+}
+
+void CppEditorPlugin::test_quickfix_AddIncludeForUndefinedIdentifier_withForwardDeclaration2()
+{
+    QList<QuickFixTestDocument::Ptr> testFiles;
+
+    QByteArray original;
+    QByteArray expected;
+
+    // Header File
+    original = "template<class T> class Foo {};\n";
+    expected = original;
+    testFiles << QuickFixTestDocument::create(TestIncludePaths::directoryOfTestFile().toUtf8()
+                                              + "/afile.h", original, expected);
+
+    // Source File
+    original =
+        "#include \"header.h\"\n"
+        "\n"
+        "template<class T> class Foo;\n"
+        "\n"
+        "void f()\n"
+        "{\n"
+        "    @Foo foo;\n"
+        "}\n"
+        ;
+    expected =
+        "#include \"afile.h\"\n"
+        "#include \"header.h\"\n"
+        "\n"
+        "template<class T> class Foo;\n"
+        "\n"
+        "void f()\n"
+        "{\n"
+        "    Foo foo;\n"
+        "}\n"
+        ;
+    testFiles << QuickFixTestDocument::create(TestIncludePaths::directoryOfTestFile().toUtf8()
+                                              + "/afile.cpp", original, expected);
+
+    // Do not use the test factory, at least once we want to go through the "full stack".
+    AddIncludeForUndefinedIdentifier factory;
+    QuickFixTestCase::run(testFiles, &factory, TestIncludePaths::globalIncludePath());
+}
+
+void CppEditorPlugin::test_quickfix_AddIncludeForUndefinedIdentifier_withForwardHeader()
+{
+    QList<QuickFixTestDocument::Ptr> testFiles;
+
+    QByteArray original;
+    QByteArray expected;
+
+    // Header File
+    original = "template<class T> class QMyClass {};\n";
+    expected = original;
+    testFiles << QuickFixTestDocument::create(TestIncludePaths::directoryOfTestFile().toUtf8()
+                                              + "/qmyclass.h", original, expected);
+
+    // Forward Header File
+    original = "#include \"qmyclass.h\"\n";
+    expected = original;
+    testFiles << QuickFixTestDocument::create(TestIncludePaths::directoryOfTestFile().toUtf8()
+                                              + "/QMyClass", original, expected);
+
+    // Source File
+    original =
+        "#include \"header.h\"\n"
+        "\n"
+        "void f()\n"
+        "{\n"
+        "    @QMyClass c;\n"
+        "}\n"
+        ;
+    expected =
+        "#include \"QMyClass\"\n"
+        "#include \"header.h\"\n"
+        "\n"
+        "void f()\n"
+        "{\n"
+        "    QMyClass c;\n"
+        "}\n"
+        ;
+    testFiles << QuickFixTestDocument::create(TestIncludePaths::directoryOfTestFile().toUtf8()
+                                              + "/afile.cpp", original, expected);
+
+    // Do not use the test factory, at least once we want to go through the "full stack".
+    AddIncludeForUndefinedIdentifier factory;
+    QuickFixTestCase::run(testFiles, &factory, TestIncludePaths::globalIncludePath(), 1);
+}
+
 /// Check: Ignore *.moc includes
 void CppEditorPlugin::test_quickfix_AddIncludeForUndefinedIdentifier_inserting_ignoremoc()
 {

src/plugins/cppeditor/cppquickfix_test.h

@@ -82,7 +82,7 @@ public:
     ~QuickFixTestCase();
 
     static void run(const QList<QuickFixTestDocument::Ptr> &theTestFiles,
-                    CppQuickFixFactory *factory, const QString &incPath);
+                    CppQuickFixFactory *factory, const QString &incPath, int resultIndex = 0);
 private:
     QSharedPointer<TextEditor::QuickFixOperation> getFix(CppQuickFixFactory *factory,
                                                          CppEditorWidget *editorWidget,

src/plugins/cppeditor/cppquickfixes.cpp

@@ -81,7 +81,6 @@ namespace Internal {
 void registerQuickFixes(ExtensionSystem::IPlugin *plugIn)
 {
     plugIn->addAutoReleasedObject(new AddIncludeForUndefinedIdentifier);
-    plugIn->addAutoReleasedObject(new AddIncludeForForwardDeclaration);
 
     plugIn->addAutoReleasedObject(new FlipLogicalOperands);
     plugIn->addAutoReleasedObject(new InverseLogicalComparison);
@@ -1529,123 +1528,6 @@ void ConvertNumericLiteral::match(const CppQuickFixInterface &interface, QuickFi
 
 namespace {
 
-class AddIncludeForForwardDeclarationOp: public CppQuickFixOperation
-{
-public:
-    AddIncludeForForwardDeclarationOp(const CppQuickFixInterface &interface, int priority,
-                                      Symbol *fwdClass)
-        : CppQuickFixOperation(interface, priority)
-        , fwdClass(fwdClass)
-    {
-        setDescription(QApplication::translate("CppTools::QuickFix",
-                                               "#include Header File"));
-    }
-
-    void perform()
-    {
-        QTC_ASSERT(fwdClass != 0, return);
-        CppRefactoringChanges refactoring(snapshot());
-        CppRefactoringFilePtr currentFile = refactoring.file(fileName());
-
-        SymbolFinder symbolFinder;
-        if (Class *k = symbolFinder.findMatchingClassDeclaration(fwdClass, snapshot())) {
-            const QString headerFile = QString::fromUtf8(k->fileName(), k->fileNameLength());
-
-            // collect the fwd headers
-            Snapshot fwdHeaders;
-            fwdHeaders.insert(snapshot().document(headerFile));
-            foreach (Document::Ptr doc, snapshot()) {
-                QFileInfo headerFileInfo(doc->fileName());
-                if (doc->globalSymbolCount() == 0 && doc->resolvedIncludes().size() == 1)
-                    fwdHeaders.insert(doc);
-                else if (headerFileInfo.suffix().isEmpty())
-                    fwdHeaders.insert(doc);
-            }
-
-            QStringList candidates = fwdHeaders.filesDependingOn(headerFile);
-
-            const QString className = QString::fromUtf8(k->identifier()->chars());
-
-            QString best;
-            foreach (const QString &c, candidates) {
-                QFileInfo headerFileInfo(c);
-                if (headerFileInfo.fileName() == className) {
-                    best = c;
-                    break;
-                } else if (headerFileInfo.fileName().at(0).isUpper()) {
-                    best = c;
-                    // and continue
-                } else if (!best.isEmpty()) {
-                    if (c.count(QLatin1Char('/')) < best.count(QLatin1Char('/')))
-                        best = c;
-                }
-            }
-
-            if (best.isEmpty())
-                best = headerFile;
-
-            const QString include = QString::fromLatin1("<%1>").arg(QFileInfo(best).fileName());
-            insertNewIncludeDirective(include, currentFile, semanticInfo().doc);
-        }
-    }
-
-    static Symbol *checkName(const CppQuickFixInterface &interface, NameAST *ast)
-    {
-        if (ast && interface.isCursorOn(ast)) {
-            if (const Name *name = ast->name) {
-                unsigned line, column;
-                interface.semanticInfo().doc->translationUnit()->getTokenStartPosition(ast->firstToken(), &line, &column);
-
-                Symbol *fwdClass = 0;
-
-                foreach (const LookupItem &r,
-                         interface.context().lookup(name, interface.semanticInfo().doc->scopeAt(line, column))) {
-                    if (!r.declaration())
-                        continue;
-                    else if (ForwardClassDeclaration *fwd = r.declaration()->asForwardClassDeclaration())
-                        fwdClass = fwd;
-                    else if (r.declaration()->isClass())
-                        return 0; // nothing to do.
-                }
-
-                return fwdClass;
-            }
-        }
-
-        return 0;
-    }
-
-private:
-    Symbol *fwdClass;
-};
-
-} // anonymous namespace
-
-void AddIncludeForForwardDeclaration::match(const CppQuickFixInterface &interface,
-                                            QuickFixOperations &result)
-{
-    const QList<AST *> &path = interface.path();
-
-    for (int index = path.size() - 1; index != -1; --index) {
-        AST *ast = path.at(index);
-        if (NamedTypeSpecifierAST *namedTy = ast->asNamedTypeSpecifier()) {
-            if (Symbol *fwdClass = AddIncludeForForwardDeclarationOp::checkName(interface,
-                                                                                namedTy->name)) {
-                result.append(new AddIncludeForForwardDeclarationOp(interface, index, fwdClass));
-                return;
-            }
-        } else if (ElaboratedTypeSpecifierAST *eTy = ast->asElaboratedTypeSpecifier()) {
-            if (Symbol *fwdClass = AddIncludeForForwardDeclarationOp::checkName(interface,
-                                                                                eTy->name)) {
-                result.append(new AddIncludeForForwardDeclarationOp(interface, index, fwdClass));
-                return;
-            }
-        }
-    }
-}
-
-namespace {
-
 class AddLocalDeclarationOp: public CppQuickFixOperation
 {
 public:
@@ -1858,28 +1740,21 @@ QString findShortestInclude(const QString currentDocumentFilePath,
     return result;
 }
 
-QString findIncludeForQtClass(const QString &className,
+QString findQtIncludeWithSameName(const QString &className,
-                              const ProjectPart::HeaderPaths &headerPaths,
+                                  const ProjectPart::HeaderPaths &headerPaths)
-                              bool classFoundInLocator)
 {
     QString result;
 
-    // for QSomething, propose a <QSomething> include -- if such a class was in the locator
+    // Check for a header file with the same name in the Qt include paths
-    if (classFoundInLocator) {
+    foreach (const ProjectPart::HeaderPath &headerPath, headerPaths) {
-        result = QLatin1Char('<') + className + QLatin1Char('>');
+        if (!headerPath.path.contains(QLatin1String("/Qt"))) // "QtCore", "QtGui" etc...
+            continue;
 
-    // otherwise, check for a header file with the same name in the Qt include paths
+        const QString headerPathCandidate = headerPath.path + QLatin1Char('/') + className;
-    } else {
+        const QFileInfo fileInfo(headerPathCandidate);
-        foreach (const ProjectPart::HeaderPath &headerPath, headerPaths) {
+        if (fileInfo.exists() && fileInfo.isFile()) {
-            if (!headerPath.path.contains(QLatin1String("/Qt"))) // "QtCore", "QtGui" etc...
+            result = QLatin1Char('<') + className + QLatin1Char('>');
-                continue;
+            break;
-
-            const QString headerPathCandidate = headerPath.path + QLatin1Char('/') + className;
-            const QFileInfo fileInfo(headerPathCandidate);
-            if (fileInfo.exists() && fileInfo.isFile()) {
-                result = QLatin1Char('<') + className + QLatin1Char('>');
-                break;
-            }
         }
     }
 
@@ -1925,7 +1800,7 @@ NameAST *nameUnderCursor(const QList<AST *> &path)
     return nameAst;
 }
 
-bool canLookup(const CppQuickFixInterface &interface, const NameAST *nameAst)
+bool canLookupDefinition(const CppQuickFixInterface &interface, const NameAST *nameAst)
 {
     QTC_ASSERT(nameAst, return false);
 
@@ -1937,10 +1812,22 @@ bool canLookup(const CppQuickFixInterface &interface, const NameAST *nameAst)
     if (!scope)
         return false;
 
-    // Check if the name resolves to something
+    // Try to find the class/template definition
     const Name *name = nameAst->name;
-    const QList<LookupItem> existingResults = interface.context().lookup(name, scope);
+    const QList<LookupItem> results = interface.context().lookup(name, scope);
-    return !existingResults.isEmpty();
+    foreach (const LookupItem &item, results) {
+        if (Symbol *declaration = item.declaration()) {
+            if (declaration->isClass())
+                return true;
+            if (Template *templ = declaration->asTemplate()) {
+                Symbol *declaration = templ->declaration();
+                if (declaration && declaration->isClass())
+                    return true;
+            }
+        }
+    }
+
+    return false;
 }
 
 QString templateNameAsString(const TemplateNameId *templateName)
@@ -1967,6 +1854,18 @@ QString unqualifiedNameForLocator(const Name *name)
     }
 }
 
+Snapshot forwardingHeaders(const CppQuickFixInterface &interface)
+{
+    Snapshot result;
+
+    foreach (Document::Ptr doc, interface.snapshot()) {
+        if (doc->globalSymbolCount() == 0 && doc->resolvedIncludes().size() == 1)
+            result.insert(doc);
+    }
+
+    return result;
+}
+
 } // anonymous namespace
 
 void AddIncludeForUndefinedIdentifier::match(const CppQuickFixInterface &interface,
@@ -1976,8 +1875,8 @@ void AddIncludeForUndefinedIdentifier::match(const CppQuickFixInterface &interfa
     if (!nameAst)
         return;
 
-    if (canLookup(interface, nameAst))
+    if (canLookupDefinition(interface, nameAst))
-        return; // There are results, so include isn't needed
+        return;
 
     const QString className = unqualifiedNameForLocator(nameAst->name);
     if (className.isEmpty())
@@ -1987,29 +1886,48 @@ void AddIncludeForUndefinedIdentifier::match(const CppQuickFixInterface &interfa
     const ProjectPart::HeaderPaths headerPaths = relevantHeaderPaths(currentDocumentFilePath);
 
     // Find an include file through the locator
-    bool classFoundInLocator = false;
-    QFutureInterface<Core::LocatorFilterEntry> dummyInterface;
     if (CppClassesFilter *classesFilter
             = ExtensionSystem::PluginManager::getObject<CppClassesFilter>()) {
-        const QList<Core::LocatorFilterEntry> matches
+        QFutureInterface<Core::LocatorFilterEntry> dummy;
-                = classesFilter->matchesFor(dummyInterface, className);
+        const QList<Core::LocatorFilterEntry> matches = classesFilter->matchesFor(dummy, className);
+
+        const Snapshot forwardHeaders = forwardingHeaders(interface);
         foreach (const Core::LocatorFilterEntry &entry, matches) {
             IndexItem::Ptr info = entry.internalData.value<IndexItem::Ptr>();
             if (info->symbolName() != className)
                 continue;
-            classFoundInLocator = true;
 
-            // Find the shortest way to include fileName given the includePaths
+            Snapshot localForwardHeaders = forwardHeaders;
-            const QString include = findShortestInclude(currentDocumentFilePath, info->fileName(),
+            localForwardHeaders.insert(interface.snapshot().document(info->fileName()));
-                                                        headerPaths);
+            QStringList headerAndItsForwardingHeaders;
-            if (!include.isEmpty())
+            headerAndItsForwardingHeaders << info->fileName();
-                result.append(new AddIncludeForUndefinedIdentifierOp(interface, 0, include));
+            headerAndItsForwardingHeaders += localForwardHeaders.filesDependingOn(info->fileName());
+
+            foreach (const QString &header, headerAndItsForwardingHeaders) {
+                const QString include = findShortestInclude(currentDocumentFilePath, header,
+                                                            headerPaths);
+                if (!include.isEmpty()) {
+                    const QString headerFileName = QFileInfo(info->fileName()).fileName();
+                    QTC_ASSERT(!headerFileName.isEmpty(), break);
+
+                    int priority = 0;
+                    if (headerFileName == className)
+                        priority = 2;
+                    else if (headerFileName.at(1).isUpper())
+                        priority = 1;
+
+                    result.append(new AddIncludeForUndefinedIdentifierOp(interface, priority,
+                                                                         include));
+                }
+            }
         }
     }
 
-    // If e.g. QString was found in "<qstring.h>" propose an extra prioritized entry "<QString>".
+    // The header file we are looking for might not be (yet) included in any file we have parsed.
+    // As such, it will not be findable via locator. At least for Qt classes, check also for
+    // headers with the same name.
     if (className.size() > 2 && className.at(0) == QLatin1Char('Q') && className.at(1).isUpper()) {
-        const QString include = findIncludeForQtClass(className, headerPaths, classFoundInLocator);
+        const QString include = findQtIncludeWithSameName(className, headerPaths);
         if (!include.isEmpty())
             result.append(new AddIncludeForUndefinedIdentifierOp(interface, 1, include));
     }

src/plugins/cppeditor/cppquickfixes.h

@@ -62,7 +62,7 @@ namespace Internal {
 void registerQuickFixes(ExtensionSystem::IPlugin *plugIn);
 
 /*!
-  Adds an include for an undefined identifier.
+  Adds an include for an undefined identifier or only forward declared identifier.
 
   Activates on: the undefined identifier
 */
@@ -84,17 +84,6 @@ private:
     QString m_include;
 };
 
-/*!
-  Can be triggered on a class forward declaration to add the matching #include.
-
-  Activates on: the name of a forward-declared class or struct
-*/
-class AddIncludeForForwardDeclaration: public CppQuickFixFactory
-{
-public:
-    void match(const CppQuickFixInterface &interface, QuickFixOperations &result);
-};
-
 /*!
   Rewrite
     a op b