CppEditor: Prevent duplicate symbols in document-scope locator

If declaration and definition of a symbol are present, offer only the
definition, as in the function and class locators.

Fixes: QTCREATORBUG-13894
Change-Id: I6794607c4c45831df53b022bbdad9db7205a890b
Reviewed-by: David Schulz <david.schulz@qt.io>

src/plugins/clangcodemodel/clangdlocatorfilters.cpp

@@ -14,7 +14,9 @@
 #include <languageclient/locatorfilter.h>
 #include <projectexplorer/session.h>
 #include <utils/link.h>
+#include <utils/algorithm.h>
 
+#include <QHash>
 #include <set>
 #include <tuple>
 
@@ -219,6 +221,11 @@ public:
     }
 
 private:
+    void prepareSearch(const QString &) override
+    {
+        m_content = TextEditor::TextDocument::currentTextDocument()->plainText();
+    }
+
     Core::LocatorFilterEntry generateLocatorEntry(const DocumentSymbol &info,
                                                   const Core::LocatorFilterEntry &parent) override
     {
@@ -227,8 +234,7 @@ private:
         entry.displayName = ClangdClient::displayNameFromDocumentSymbol(
                     static_cast<SymbolKind>(info.kind()), info.name(),
                     info.detail().value_or(QString()));
-        const Position &pos = info.range().start();
+        entry.internalData = QVariant::fromValue(info);
-        entry.internalData = QVariant::fromValue(Utils::LineColumn(pos.line(), pos.character()));
         entry.extraInfo = parent.extraInfo;
         if (!entry.extraInfo.isEmpty())
             entry.extraInfo.append("::");
@@ -239,6 +245,66 @@ private:
 
         return entry;
     }
+
+    // Filter out declarations for which a definition is also present.
+    QList<Core::LocatorFilterEntry> matchesFor(QFutureInterface<Core::LocatorFilterEntry> &future,
+                                               const QString &entry) override
+    {
+        QList<Core::LocatorFilterEntry> allMatches
+            = DocumentLocatorFilter::matchesFor(future, entry);
+        QHash<QString, QList<Core::LocatorFilterEntry>> possibleDuplicates;
+        for (const Core::LocatorFilterEntry &e : std::as_const(allMatches))
+            possibleDuplicates[e.displayName + e.extraInfo] << e;
+        const QTextDocument doc(m_content);
+        for (auto it = possibleDuplicates.cbegin(); it != possibleDuplicates.cend(); ++it) {
+            const QList<Core::LocatorFilterEntry> &duplicates = it.value();
+            if (duplicates.size() == 1)
+                continue;
+            QList<Core::LocatorFilterEntry> declarations;
+            QList<Core::LocatorFilterEntry> definitions;
+            for (const Core::LocatorFilterEntry &candidate : duplicates) {
+                const auto symbol = qvariant_cast<DocumentSymbol>(candidate.internalData);
+                const SymbolKind kind = static_cast<SymbolKind>(symbol.kind());
+                if (kind != SymbolKind::Class && kind != SymbolKind::Function)
+                    break;
+                const Range range = symbol.range();
+                const Range selectionRange = symbol.selectionRange();
+                if (kind == SymbolKind::Class) {
+                    if (range.end() == selectionRange.end())
+                        declarations << candidate;
+                    else
+                        definitions << candidate;
+                    continue;
+                }
+                const int startPos = selectionRange.end().toPositionInDocument(&doc);
+                const int endPos = range.end().toPositionInDocument(&doc);
+                const QString functionBody = m_content.mid(startPos, endPos - startPos);
+
+                // Hacky, but I don't see anything better.
+                if (functionBody.contains('{') && functionBody.contains('}'))
+                    definitions << candidate;
+                else
+                    declarations << candidate;
+            }
+            if (definitions.size() == 1
+                && declarations.size() + definitions.size() == duplicates.size()) {
+                for (const Core::LocatorFilterEntry &decl : std::as_const(declarations))
+                    Utils::erase(allMatches, [&decl](const Core::LocatorFilterEntry &e) {
+                        return e.internalData == decl.internalData;
+                    });
+            }
+        }
+
+        // The base implementation expects the position in the internal data.
+        for (Core::LocatorFilterEntry &e : allMatches) {
+            const Position pos = qvariant_cast<DocumentSymbol>(e.internalData).range().start();
+            e.internalData = QVariant::fromValue(Utils::LineColumn(pos.line(), pos.character()));
+        }
+
+        return allMatches;
+    }
+
+    QString m_content;
 };
 
 class ClangdCurrentDocumentFilter::Private

src/plugins/cppeditor/cppcurrentdocumentfilter.cpp

@@ -9,7 +9,9 @@
 #include <coreplugin/editormanager/editormanager.h>
 #include <coreplugin/editormanager/ieditor.h>
 #include <coreplugin/idocument.h>
+#include <utils/algorithm.h>
 
+#include <QHash>
 #include <QRegularExpression>
 
 using namespace CPlusPlus;
@@ -100,8 +102,37 @@ QList<Core::LocatorFilterEntry> CppCurrentDocumentFilter::matchesFor(
     }
 
     // entries are unsorted by design!
-
     betterEntries += goodEntries;
+
+    QHash<QString, QList<Core::LocatorFilterEntry>> possibleDuplicates;
+    for (const Core::LocatorFilterEntry &e : std::as_const(betterEntries)) {
+        const IndexItem::Ptr info = qvariant_cast<IndexItem::Ptr>(e.internalData);
+        possibleDuplicates[info->scopedSymbolName() + info->symbolType()] << e;
+    }
+    for (auto it = possibleDuplicates.cbegin(); it != possibleDuplicates.cend(); ++it) {
+        const QList<Core::LocatorFilterEntry> &duplicates = it.value();
+        if (duplicates.size() == 1)
+            continue;
+        QList<Core::LocatorFilterEntry> declarations;
+        QList<Core::LocatorFilterEntry> definitions;
+        for (const Core::LocatorFilterEntry &candidate : duplicates) {
+            const IndexItem::Ptr info = qvariant_cast<IndexItem::Ptr>(candidate.internalData);
+            if (info->type() != IndexItem::Function)
+                break;
+            if (info->isFunctionDefinition())
+                definitions << candidate;
+            else
+                declarations << candidate;
+        }
+        if (definitions.size() == 1
+            && declarations.size() + definitions.size() == duplicates.size()) {
+            for (const Core::LocatorFilterEntry &decl : std::as_const(declarations))
+                Utils::erase(betterEntries, [&decl](const Core::LocatorFilterEntry &e) {
+                    return e.internalData == decl.internalData;
+                });
+        }
+    }
+
     return betterEntries;
 }
 

src/plugins/cppeditor/cpplocatorfilter_test.cpp

@@ -319,7 +319,6 @@ void LocatorFilterTest::testCurrentDocumentFilter()
         ResultData("functionDeclaredOnly()", "MyClass"),
         ResultData("functionDefinedInClass(bool, int)", "MyClass"),
         ResultData("functionDefinedOutSideClass(char)", "MyClass"),
-        ResultData("functionDefinedOutSideClass(char)", "MyClass"),
         ResultData("int myVariable", "MyNamespace"),
         ResultData("myFunction(bool, int)", "MyNamespace"),
         ResultData("MyEnum", "MyNamespace"),
@@ -330,9 +329,6 @@ void LocatorFilterTest::testCurrentDocumentFilter()
         ResultData("functionDeclaredOnly()", "MyNamespace::MyClass"),
         ResultData("functionDefinedInClass(bool, int)", "MyNamespace::MyClass"),
         ResultData("functionDefinedOutSideClass(char)", "MyNamespace::MyClass"),
-        ResultData("functionDefinedOutSideClassAndNamespace(float)",
-                   "MyNamespace::MyClass"),
-        ResultData("functionDefinedOutSideClass(char)", "MyNamespace::MyClass"),
         ResultData("functionDefinedOutSideClassAndNamespace(float)",
                    "MyNamespace::MyClass"),
         ResultData("int myVariable", "<anonymous namespace>"),
@@ -345,7 +341,6 @@ void LocatorFilterTest::testCurrentDocumentFilter()
         ResultData("functionDeclaredOnly()", "<anonymous namespace>::MyClass"),
         ResultData("functionDefinedInClass(bool, int)", "<anonymous namespace>::MyClass"),
         ResultData("functionDefinedOutSideClass(char)", "<anonymous namespace>::MyClass"),
-        ResultData("functionDefinedOutSideClass(char)", "<anonymous namespace>::MyClass"),
         ResultData("main()", ""),
     };
 

src/plugins/cppeditor/indexitem.cpp

@@ -9,7 +9,8 @@ namespace CppEditor {
 
 IndexItem::Ptr IndexItem::create(const QString &symbolName, const QString &symbolType,
                                  const QString &symbolScope, IndexItem::ItemType type,
-                                 const QString &fileName, int line, int column, const QIcon &icon)
+                                 const QString &fileName, int line, int column, const QIcon &icon,
+                                 bool isFunctionDefinition)
 {
     Ptr ptr(new IndexItem);
 
@@ -21,6 +22,7 @@ IndexItem::Ptr IndexItem::create(const QString &symbolName, const QString &symbo
     ptr->m_line = line;
     ptr->m_column = column;
     ptr->m_icon = icon;
+    ptr->m_isFuncDef = isFunctionDefinition;
 
     return ptr;
 }

src/plugins/cppeditor/indexitem.h

@@ -40,7 +40,8 @@ public:
                       const QString &fileName,
                       int line,
                       int column,
-                      const QIcon &icon);
+                      const QIcon &icon,
+                      bool isFunctionDefinition);
     static Ptr create(const QString &fileName, int sizeHint);
 
     QString scopedSymbolName() const
@@ -64,6 +65,7 @@ public:
     ItemType type() const { return m_type; }
     int line() const { return m_line; }
     int column() const { return m_column; }
+    bool isFunctionDefinition() const { return m_isFuncDef; }
 
     void addChild(IndexItem::Ptr childItem) { m_children.append(childItem); }
     void squeeze();
@@ -106,6 +108,7 @@ private:
     ItemType m_type = All;
     int m_line = 0;
     int m_column = 0;
+    bool m_isFuncDef = false;
     QVector<IndexItem::Ptr> m_children;
 };
 

src/plugins/cppeditor/searchsymbols.cpp

@@ -285,7 +285,8 @@ IndexItem::Ptr SearchSymbols::addChildItem(const QString &symbolName, const QStr
                                                StringTable::insert(path),
                                                symbol->line(),
                                                symbol->column() - 1, // 1-based vs 0-based column
-                                               icon);
+                                               icon,
+                                               symbol->asFunction());
     _parent->addChild(newItem);
     return newItem;
 }