qdoc: Fix handling of overloads when adopting functions between parents

When adopting a function node from one parent to another, QDoc did not
touch the linked list of overloads for the adopted function. This
caused situations where traversing the node tree, trying to find
functions for an aggregate (parent), yielded also overloads that are
not supposed to be found as they are not members of that parent.

One visible place was the classes-with-obsolete-members generated list,
where QDoc erraneously found obsolete overloads for a class that had
adopted function(s) from the global namespace.

To fix, remove an adoptee from the overload list of its primary function
(maintained by the original parent), and re-add it to the overload list
maintained by the new parent.

Refactor functions that manage linked lists; using recursion is
unnecessary.

Fixes: QTBUG-89003
Change-Id: Ibd215cd4b1d97003ac79fe02a71452e4a7a154db
Reviewed-by: Topi Reiniö <topi.reinio@qt.io>
(cherry picked from commit 550c3031c2fbafe89daec4046dd45946ee8a788b)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

12 files changed, 139 insertions, 56 deletions

diff --git a/src/qdoc/aggregate.cpp b/src/qdoc/aggregate.cpp
index 698d8773d..16b6d5031 100644
--- a/src/qdoc/aggregate.cpp
+++ b/src/qdoc/aggregate.cpp
@@ -271,8 +271,6 @@ void Aggregate::markUndocumentedChildrenInternal()
 
 /*!
   This is where we set the overload numbers for function nodes.
-  \note Overload numbers for related non-members are handled
-  separately.
  */
 void Aggregate::normalizeOverloads()
 {
@@ -288,12 +286,10 @@ void Aggregate::normalizeOverloads()
                 primary->setNextOverload(fn);
                 it.value() = primary;
                 fn = primary;
-            } else {
-                fn->clearOverloadFlag();
             }
         }
         int count = 0;
-        fn->setOverloadNumber(0);
+        fn->setOverloadNumber(0); // also clears the overload flag
         FunctionNode *internalFn = nullptr;
         while (fn != nullptr) {
             FunctionNode *next = fn->nextOverload();
@@ -317,11 +313,8 @@ void Aggregate::normalizeOverloads()
             internalFn->setOverloadNumber(++count);
             internalFn = internalFn->nextOverload();
         }
-        // process next function in function map.
     }
-    /*
-      Recursive part.
-     */
+
     for (auto *node : qAsConst(m_children)) {
         if (node->isAggregate())
             static_cast<Aggregate *>(node)->normalizeOverloads();
@@ -462,25 +455,26 @@ void Aggregate::addFunction(FunctionNode *fn)
         m_functionMap.insert(fn->name(), fn);
     else
         it.value()->appendOverload(fn);
-    m_functionCount++;
 }
 
 /*!
-  When an Aggregate adopts a function that is a child of
+  When an Aggregate adopts a function \a fn that is a child of
   another Aggregate, the function is inserted into this
-  Aggregate's function map, if the function's name is not
-  already in the function map. If the function's name is
-  already in the function map, do nothing. The overload
-  link is already set correctly.
+  Aggregate's function map.
+
+  The function is also removed from the overload list
+  that's relative to the the original parent \a firstParent,
+  unless it's a primary function.
 
   \note This is a private function.
  */
-void Aggregate::adoptFunction(FunctionNode *fn)
+void Aggregate::adoptFunction(FunctionNode *fn, Aggregate *firstParent)
 {
-    auto it = m_functionMap.find(fn->name());
-    if (it == m_functionMap.end())
-        m_functionMap.insert(fn->name(), fn);
-    ++m_functionCount;
+    auto *primary = firstParent->m_functionMap.value(fn->name());
+    if (primary && primary != fn)
+        primary->removeOverload(fn);
+    fn->setNextOverload(nullptr);
+    addFunction(fn);
 }
 
 /*!
@@ -536,17 +530,15 @@ void Aggregate::addChild(Node *child)
   list and in its searchable data structures. But the child is
   also added to the child list and searchable data structures
   of this Aggregate.
-
-  The one caveat is that if the child being adopted is a function
-  node, it's next overload pointer is not altered.
  */
 void Aggregate::adoptChild(Node *child)
 {
     if (child->parent() != this) {
         m_children.append(child);
+        auto firstParent = child->parent();
         child->setParent(this);
         if (child->isFunction()) {
-            adoptFunction(static_cast<FunctionNode *>(child));
+            adoptFunction(static_cast<FunctionNode *>(child), firstParent);
         } else if (!child->name().isEmpty()) {
             m_nonfunctionMap.insert(child->name(), child);
             if (child->isEnumType())
diff --git a/src/qdoc/aggregate.h b/src/qdoc/aggregate.h
index 6a720a368..a8a8bb46c 100644
--- a/src/qdoc/aggregate.h
+++ b/src/qdoc/aggregate.h
@@ -91,22 +91,19 @@ public:
 
 protected:
     Aggregate(NodeType type, Aggregate *parent, const QString &name)
-        : PageNode(type, parent, name), m_functionCount(0)
-    {
-    }
+        : PageNode(type, parent, name) {}
     ~Aggregate() override;
     void removeFunctionNode(FunctionNode *fn);
 
 private:
     friend class Node;
     void addFunction(FunctionNode *fn);
-    void adoptFunction(FunctionNode *fn);
+    void adoptFunction(FunctionNode *fn, Aggregate *firstParent);
     static bool isSameSignature(const FunctionNode *f1, const FunctionNode *f2);
 
 protected:
     NodeList m_children {};
     NodeList m_relatedByProxy {};
-    int m_functionCount {};
     FunctionMap m_functionMap {};
 
 private:
diff --git a/src/qdoc/functionnode.cpp b/src/qdoc/functionnode.cpp
index dc499efad..8f00e508f 100644
--- a/src/qdoc/functionnode.cpp
+++ b/src/qdoc/functionnode.cpp
@@ -322,7 +322,7 @@ bool FunctionNode::changeMetaness(Metaness from, Metaness to)
     return false;
 }
 
-/*! \fn void FunctionNode::setOverloadNumber(unsigned char number)
+/*!
   Sets the function node's overload number to \a number. If \a number
   is 0, the function node's overload flag is set to false. If
   \a number is greater than 0, the overload flag is set to true.
@@ -334,47 +334,60 @@ void FunctionNode::setOverloadNumber(signed short number)
 }
 
 /*!
-  If this function's next overload pointer is null, set it to
-  \a functionNode. Otherwise continue down the overload list by calling
-  this function recursively for the next overload.
+  Appends \a functionNode to the linked list of overloads for this function.
 
-  Although this function appends an overload function to the list of
+  \note Although this function appends an overload function to the list of
   overloads for this function's name, it does not set the function's
   overload number or it's overload flag. If the function has the
-  \c{\\overload} in its qdoc comment, that will set the overload
+  \c{\\overload} in its QDoc comment, that will set the overload
   flag. But qdoc treats the \c{\\overload} command as a hint that the
   function should be documented as an overload. The hint is almost
-  always correct, but qdoc reserves the right to decide which function
+  always correct, but QDoc reserves the right to decide which function
   should be the primary function and which functions are the overloads.
   These decisions are made in Aggregate::normalizeOverloads().
  */
 void FunctionNode::appendOverload(FunctionNode *functionNode)
 {
-    if (m_nextOverload == nullptr)
-        m_nextOverload = functionNode;
-    else
-        m_nextOverload->appendOverload(functionNode);
+    auto current = this;
+    while (current->m_nextOverload)
+        current = current->m_nextOverload;
+    current->m_nextOverload = functionNode;
+    functionNode->m_nextOverload = nullptr;
 }
 
 /*!
-  This function assumes that this FunctionNode is marked as an
-  overload function. It asks if the next overload is marked as
-  an overload. If not, then remove that FunctionNode from the
-  overload list and return it. Otherwise call this function
-  recursively for the next overload.
+  Removes \a functionNode from the linked list of function overloads.
+*/
+void FunctionNode::removeOverload(FunctionNode *functionNode)
+{
+    auto head = this;
+    auto **indirect = &head;
+    while ((*indirect) != functionNode) {
+        if (!(*indirect)->m_nextOverload)
+            return;
+        indirect = &(*indirect)->m_nextOverload;
+    }
+    *indirect = functionNode->m_nextOverload;
+}
+
+/*!
+  Returns the primary function - the first function
+  from the linked list of overloads that is \e not
+  marked as an overload. If found, the primary function
+  is removed from the list and returned. Otherwise
+  returns \c nullptr.
  */
 FunctionNode *FunctionNode::findPrimaryFunction()
 {
-    if (m_nextOverload != nullptr) {
-        if (!m_nextOverload->isOverload()) {
-            FunctionNode *t = m_nextOverload;
-            m_nextOverload = t->nextOverload();
-            t->setNextOverload(nullptr);
-            return t;
-        }
-        return m_nextOverload->findPrimaryFunction();
-    }
-    return nullptr;
+    auto current = this;
+    while (current->m_nextOverload && current->m_nextOverload->isOverload())
+        current = current->m_nextOverload;
+
+    auto primary = current->m_nextOverload;
+    if (primary)
+        current->m_nextOverload = primary->m_nextOverload;
+
+    return primary;
 }
 
 /*!
diff --git a/src/qdoc/functionnode.h b/src/qdoc/functionnode.h
index aea086a77..bb8e4b28c 100644
--- a/src/qdoc/functionnode.h
+++ b/src/qdoc/functionnode.h
@@ -177,6 +177,7 @@ public:
     void setOverloadFlag() { m_overloadFlag = true; }
     void setOverloadNumber(signed short number);
     void appendOverload(FunctionNode *functionNode);
+    void removeOverload(FunctionNode *functionNode);
     signed short overloadNumber() const { return m_overloadNumber; }
     FunctionNode *nextOverload() { return m_nextOverload; }
     void setNextOverload(FunctionNode *functionNode) { m_nextOverload = functionNode; }
diff --git a/tests/auto/qdoc/generatedoutput/expected_output/dontdocument/dontdocument.qhp b/tests/auto/qdoc/generatedoutput/expected_output/dontdocument/dontdocument.qhp
index 3cc86e329..8484915eb 100644
--- a/tests/auto/qdoc/generatedoutput/expected_output/dontdocument/dontdocument.qhp
+++ b/tests/auto/qdoc/generatedoutput/expected_output/dontdocument/dontdocument.qhp
@@ -21,6 +21,7 @@
         </toc>
         <keywords>
             <keyword name="Classes" id="Classes" ref="classes.html"/>
+            <keyword name="Obsolete Classes" id="Obsolete Classes" ref="obsolete-classes.html"/>
             <keyword name="QDOCTEST_MACRO" id="QDOCTEST_MACRO" ref="testqdoc.html#QDOCTEST_MACRO"/>
             <keyword name="QDOCTEST_MACRO2" id="QDOCTEST_MACRO2" ref="testqdoc-test.html#QDOCTEST_MACRO2"/>
             <keyword name="QDoc Test C++ Classes" id="QDoc Test C++ Classes" ref="testcpp-module.html"/>
@@ -47,6 +48,7 @@
         </keywords>
         <files>
             <file>classes.html</file>
+            <file>obsolete-classes.html</file>
             <file>seenclass.html</file>
             <file>testcpp-module.html</file>
             <file>testqdoc-test-members.html</file>
diff --git a/tests/auto/qdoc/generatedoutput/expected_output/obsolete-classes.html b/tests/auto/qdoc/generatedoutput/expected_output/obsolete-classes.html
new file mode 100644
index 000000000..93a35078b
--- /dev/null
+++ b/tests/auto/qdoc/generatedoutput/expected_output/obsolete-classes.html
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+  <meta charset="utf-8">
+<!-- classlists.qdoc -->
+  <title>Obsolete Classes | TestCPP</title>
+</head>
+<body>
+<div class="sidebar">
+<div class="toc">
+<h3><a name="toc">Contents</a></h3>
+<ul>
+<li class="level1"><a href="#classes-with-obsolete-members">Classes with obsolete members</a></li>
+</ul>
+</div>
+<div class="sidebar-content" id="sidebar-content"></div></div>
+<h1 class="title">Obsolete Classes</h1>
+<span class="subtitle"></span>
+<!-- $$$obsolete-classes.html-description -->
+<div class="descr"> <a name="details"></a>
+<a name="classes-with-obsolete-members"></a>
+<h2 id="classes-with-obsolete-members">Classes with obsolete members</h2>
+<div class="flowListDiv">
+<dl class="flowList odd"><dt class="alphaChar"><b>T</b></dt>
+<dd><a href="..//testqdoc-test-obsolete.html">Test</a> (<a href="testqdoc.html">TestQDoc</a>)</dd>
+<dd><a href="..//testqdoc-testderived-obsolete.html">TestDerived</a> (<a href="testqdoc.html">TestQDoc</a>)</dd>
+</dl>
+</div>
+</div>
+<!-- @@@obsolete-classes.html -->
+</body>
+</html>
diff --git a/tests/auto/qdoc/generatedoutput/expected_output/properties/testcpp.index b/tests/auto/qdoc/generatedoutput/expected_output/properties/testcpp.index
index 2566c1fc9..feb482503 100644
--- a/tests/auto/qdoc/generatedoutput/expected_output/properties/testcpp.index
+++ b/tests/auto/qdoc/generatedoutput/expected_output/properties/testcpp.index
@@ -6,6 +6,9 @@
         <function name="QDOCTEST_MACRO2" href="testqdoc-test.html#QDOCTEST_MACRO2" status="active" access="public" documented="true" related="1" since="Test 1.1" meta="macrowithparams" virtual="non" const="false" static="false" final="false" override="false" type="" brief="A macro with argument x" signature="QDOCTEST_MACRO2(int &amp;x)" groups="testgroup">
             <parameter type="int &amp;" name="x" default=""/>
         </function>
+        <page name="obsolete-classes.html" href="obsolete-classes.html" status="active" location="classlists.qdoc" documented="true" subtype="page" title="Obsolete Classes" fulltitle="Obsolete Classes" subtitle="" module="TestCPP">
+            <contents name="classes-with-obsolete-members" title="Classes with obsolete members" level="1"/>
+        </page>
         <page name="https://wiki.qt.io/QProperty" href="https://wiki.qt.io/QProperty" status="active" location="properties.qdoc" documented="true" subtype="externalpage" title="QProperty" fulltitle="QProperty" subtitle=""/>
         <namespace name="TestQDoc" href="testqdoc.html" status="active" access="public" location="testcpp.h" documented="true" module="TestCPP" brief="A namespace">
             <contents name="usage" title="Usage" level="1"/>
diff --git a/tests/auto/qdoc/generatedoutput/expected_output/test.qhp b/tests/auto/qdoc/generatedoutput/expected_output/test.qhp
index 0faff8362..315c4fb09 100644
--- a/tests/auto/qdoc/generatedoutput/expected_output/test.qhp
+++ b/tests/auto/qdoc/generatedoutput/expected_output/test.qhp
@@ -12,6 +12,7 @@
                     <section ref="test-demos-demo-example.html" title="Demo"/>
                     <section ref="test-demos-hidden-example.html" title="Hidden Demo"/>
                     <section ref="test-empty-qmlmodule.html" title="No QML Types Here"/>
+                    <section ref="obsolete-classes.html" title="Obsolete Classes"/>
                     <section ref="testcpp-module.html" title="QDoc Test C++ Classes"/>
                     <section ref="test-componentset-example.html" title="QML Documentation Example"/>
                     <section ref="qmlmodules.html" title="QML Modules"/>
@@ -65,6 +66,7 @@
             <keyword name="DocTest" id="QML.DocTest" ref="qml-qdoc-test-doctest.html"/>
             <keyword name="DocTest" id="QML.QDoc.Test1.DocTest" ref="qml-qdoc-test-doctest.html"/>
             <keyword name="No QML Types Here" id="No QML Types Here" ref="test-empty-qmlmodule.html"/>
+            <keyword name="Obsolete Classes" id="Obsolete Classes" ref="obsolete-classes.html"/>
             <keyword name="ProgressBar" id="QML.ProgressBar" ref="qml-uicomponents-progressbar.html"/>
             <keyword name="ProgressBar" id="QML.UIComponents1.ProgressBar" ref="qml-uicomponents-progressbar.html"/>
             <keyword name="QDOCTEST_MACRO" id="QDOCTEST_MACRO" ref="testqdoc.html#QDOCTEST_MACRO"/>
@@ -135,6 +137,7 @@
         </keywords>
         <files>
             <file>images/leonardo-da-vinci.png</file>
+            <file>obsolete-classes.html</file>
             <file>qdoc-test-qmlmodule.html</file>
             <file>qml-int.html</file>
             <file>qml-qdoc-test-abstractparent-members.html</file>
diff --git a/tests/auto/qdoc/generatedoutput/expected_output/testcpp.index b/tests/auto/qdoc/generatedoutput/expected_output/testcpp.index
index 3d2752435..38fb04a73 100644
--- a/tests/auto/qdoc/generatedoutput/expected_output/testcpp.index
+++ b/tests/auto/qdoc/generatedoutput/expected_output/testcpp.index
@@ -6,6 +6,9 @@
         <function name="QDOCTEST_MACRO2" href="testqdoc-test.html#QDOCTEST_MACRO2" status="active" access="public" documented="true" related="1" since="Test 1.1" meta="macrowithparams" virtual="non" const="false" static="false" final="false" override="false" type="" brief="A macro with argument x" signature="QDOCTEST_MACRO2(int &amp;x)" groups="testgroup">
             <parameter type="int &amp;" name="x" default=""/>
         </function>
+        <page name="obsolete-classes.html" href="obsolete-classes.html" status="active" location="classlists.qdoc" documented="true" subtype="page" title="Obsolete Classes" fulltitle="Obsolete Classes" subtitle="" module="TestCPP">
+            <contents name="classes-with-obsolete-members" title="Classes with obsolete members" level="1"/>
+        </page>
         <namespace name="TestQDoc" href="testqdoc.html" status="active" access="public" location="testcpp.h" documented="true" module="TestCPP" brief="A namespace">
             <contents name="usage" title="Usage" level="1"/>
             <function name="QDOCTEST_MACRO" href="testqdoc.html#QDOCTEST_MACRO" status="active" access="public" documented="true" related="0" meta="macrowithoutparams" virtual="non" const="false" static="false" final="false" override="false" type="" signature="QDOCTEST_MACRO"/>
diff --git a/tests/auto/qdoc/generatedoutput/testdata/configs/testcpp.qdocconf b/tests/auto/qdoc/generatedoutput/testdata/configs/testcpp.qdocconf
index d69f11aff..b1d26f27e 100644
--- a/tests/auto/qdoc/generatedoutput/testdata/configs/testcpp.qdocconf
+++ b/tests/auto/qdoc/generatedoutput/testdata/configs/testcpp.qdocconf
@@ -3,7 +3,8 @@ project = TestCPP
 includepaths += -I../testcpp
 
 headers = ../testcpp/testcpp.h
-sources = ../testcpp/testcpp.cpp
+sources = ../testcpp/testcpp.cpp \
+          ../testcpp/classlists.qdoc
 exampledirs = ../testcpp/snippets
 
 macro.CMDFN = \\\\fn
diff --git a/tests/auto/qdoc/generatedoutput/testdata/testcpp/classlists.qdoc b/tests/auto/qdoc/generatedoutput/testdata/testcpp/classlists.qdoc
new file mode 100644
index 000000000..7e57b57ee
--- /dev/null
+++ b/tests/auto/qdoc/generatedoutput/testdata/testcpp/classlists.qdoc
@@ -0,0 +1,35 @@
+/****************************************************************************
+**
+** 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$
+**
+****************************************************************************/
+
+/*!
+    \page obsolete-classes.html
+    \title Obsolete Classes
+
+    \section1 Classes with obsolete members
+    \generatelist obsoletecppmembers
+*/
diff --git a/tests/auto/qdoc/generatedoutput/tst_generatedoutput.cpp b/tests/auto/qdoc/generatedoutput/tst_generatedoutput.cpp
index 4ac2ce290..05bfeaa3a 100644
--- a/tests/auto/qdoc/generatedoutput/tst_generatedoutput.cpp
+++ b/tests/auto/qdoc/generatedoutput/tst_generatedoutput.cpp
@@ -238,6 +238,7 @@ void tst_generatedOutput::htmlFromCpp()
                    "testqdoc-testderived.html "
                    "testqdoc-testderived-members.html "
                    "testqdoc-testderived-obsolete.html "
+                   "obsolete-classes.html "
                    "testqdoc.html");
 }