QDoc: Hack-fix to correctly mark some nodes as copy-constructors

QDoc represents documentable elements extracted from source code as
elements of the `Node` class and its derivatives.

In particular, it represents C++ function, methods and so on as
inhabitants of `FunctionNode`.

`FunctionNode` stores a simplified view of the properties of such an
element that are later used by QDoc to perform some sanity checks and to
actually generate the documentation for the element.

One such property, the "metaness", indicates what kind of element the
inhabitant of a `FunctionNode` might actually represent.

The "metaness" is generally set to identify some special cases that
later branch the execution of code and command the way in which the
documentation for the element is generated.

For example, due to the desire to sort the methods of a C++ class when
QDoc is listing its element, a special metaness is assigned to, for
example, constructors, move-constructors and copy-constructors.

The "metaness" of a `FunctionNode`, together with some of its other
properties, is generally set by QDoc while parsing the various source
files through `ClangCodeParser` and, specifically, in
`ClangCodeParser::processFunction`, where an instance of a
`FunctionNode` is populated by its equivalent code-level libclang
`CxCursor`.

The condition that would identify whether a `FunctionNode` should have a
metaness representing a copy-constructor was bugged since its
introduction.

In particular, the current implementation considers the spelling of the
type of the parameters of a method, if it is a constructor, and marks it
as a copy-constructor when a parameter that is a reference is spelled
exactly the same as the parent class of the method.

That is, when a constructor "X" of class "X" has a parameter "X&".

This specific implementation that used the spelling, failed to identify
the very general usage of const-references in a copy constructor.

When the type of a parameter is qualified, such as with "const",
libclang will consider that qualifier in its spelling unless removed,
for which there is no direct method in pre-16 libclang.

The implementation would remove the reference part, "&" or "&&", through
`clang_getPointeeType`, which is further incorrect as it would work for
pointer types too, but would not address any of the possible qualifiers
in the spelling.

Then, it would directly check the spelling to the name of the
constructor, that is, the name of the containing class.

Not addressing the qualifiers would thus fail for reference types that
were qualified. For example, a constructor "X" with an argument of type
"const X&" would be spelled by libclang as "const X" but would be
compared by the implementation to "X", thus always failing.

This was particularly visible for copy-constructors, as the most common
way to implement a copy-constructor is with a unary-constructor whose
argument is of type "const X&".

Indeed, in the years since this code was implemented, QDoc never
actually identified the "metaness" of copy-constructor correctly,
albeit, fortunately, with little impact on the produced documentation.

To avoid the issue, qualifiers are now handled in
"ClangCodeParser::processFunction" when identifying the metaness of a
constructor.

While this solves the issue at hand, "ClangCodeParser::processFunction"
is still bugged with regards to its intention in multiple ways, such as
considering all arguments for the "metaness" checks or not correctly
handling the non-common cases of copy/move-assignment-operators.

An annotation was added to the code to handle the issue when LLVM16,
where a series of patches that we pushed upstream will be available,
will be out, as it will allow us to greatly simplify the code and
correct the bugs as the same time.

The remaining bugs are not expected to be corrected at the current time
due to the unnecessary complexity of the required handling due to the
current limitation in libclang's AST.

The bug are considered safe to preserves for a limited amount of time as
they do not generally impact the way in which the Qt Project writes the
relevant code-elements, and are only dangerous when some specific cases
would otherwise be encountered.

The output documentation will not currently be affected by this change,
albeit the produced "index" files, an intermediate representation of the
code that QDoc produces that is generally consumed both by QDoc and
externel consumers, will incur modifications due to the corrected
identifying of the copy-constructors.

Change-Id: I59ec2c897106a34300b91a175664c0ba07d28f5c
Reviewed-by: Topi Reiniö <topi.reinio@qt.io>

1 files changed, 57 insertions, 5 deletions

diff --git a/src/qdoc/clangcodeparser.cpp b/src/qdoc/clangcodeparser.cpp
index 60e6bf239..1ae195047 100644
--- a/src/qdoc/clangcodeparser.cpp
+++ b/src/qdoc/clangcodeparser.cpp
@@ -897,15 +897,67 @@ void ClangVisitor::processFunction(FunctionNode *fn, CXCursor cursor)
     Parameters &parameters = fn->parameters();
     parameters.clear();
     parameters.reserve(numArg);
+
+    // TODO: [clang16-required][unsound-code][bug-ridden]
+    // The following code that sets the metaness of the currently
+    // processed function node incorrect in multiple ways with regards
+    // to the specification of move/copy constructors/assignment
+    // operators.
+    //
+    // For example, it doesn't correctly allow for qualifiers nor does
+    // it correctly respect the argument disposition.
+    //
+    // It was previously bugged and still present a series of bugs
+    // such as being able to misidentify constructors types.
+    //
+    // A by-specification implementation in our currently used
+    // libclang is not particularly feasible due to the required
+    // complexity and is preferred to be avoided.
+    //
+    // To solve the issue, we pushed a series of patches to upstream
+    // llvm to expose some of the C++ API methods that would
+    // trivialize the following code.
+    //
+    // Those methods will be available on LLVM 16, which is currently
+    // expected for January 2023.
+    // When LLVM 16 comes out and we upgrade to it, the following code
+    // should be fixed and the minimum required version of libclang
+    // for QDoc should be updated.
     for (int i = 0; i < numArg; ++i) {
         CXType argType = clang_getArgType(funcType, i);
         if (fn->isCtor()) {
-            if (fromCXString(clang_getTypeSpelling(clang_getPointeeType(argType))) == fn->name()) {
-                if (argType.kind == CXType_RValueReference)
-                    fn->setMetaness(FunctionNode::MCtor);
-                else if (argType.kind == CXType_LValueReference)
-                    fn->setMetaness(FunctionNode::CCtor);
+            // TODO:KLUDGE: [temporary-hack]
+            // The following is used to fix a very specific bug in the
+            // way QDoc assigns the metaness of constructors.
+            // It is nonetheless quite imperfect in its handling and
+            // is only used as a temporary fix while LLVM 16 is not
+            // available.
+            //
+            // This is expected to be removed as soon as LLVM 16 comes out.
+            static auto remove_qualifiers = [](QString type_spelling) {
+                // REMARK: While, generally, a type with
+                // both qualifiers would be written "const volatile
+                // T", clang accepts "volatile const T" too.
+                //
+                // Nonetheless, we do not handle that case as
+                // libclang, when providing the spelling, currently
+                // always writes the const-qualifier before the
+                // volatile-qualifier.
+                if (type_spelling.startsWith("const "))
+                    type_spelling.remove(0, QString("const ").size());
+
+                if (type_spelling.startsWith("volatile "))
+                    type_spelling.remove(0, QString("volatile ").size());
+
+                return type_spelling;
+            };
+
+            const QString type_spelling{remove_qualifiers(fromCXString(clang_getTypeSpelling(clang_getPointeeType(argType))))};
+            if (type_spelling == fn->name()) {
+                if (argType.kind == CXType_RValueReference) fn->setMetaness(FunctionNode::MCtor);
+                else if (argType.kind == CXType_LValueReference) fn->setMetaness(FunctionNode::CCtor);
             }
+
         } else if ((kind == CXCursor_CXXMethod) && (fn->name() == QLatin1String("operator="))) {
             if (argType.kind == CXType_RValueReference)
                 fn->setMetaness(FunctionNode::MAssign);