Qualify non-dependent types of a class template with its declaration #67566

diseraluca · 2023-09-27T15:00:54Z

When
clang::TypeName::getFullyQualifiedType/clang::TypeName::getFullyQualifiedName encounter a non-dependent type/type alias that is defined under a templated class/struct, it qualifies the type/type alias under a specialization of the templated class, if any was declared.

That is, in:

template<typename T>
class Foo {
    using Bar = T;
}

using Baz = Foo<int>;

Usages of Foo::Bar will be qualified as Foo<int>::Bar.

When the using Baz = Foo<int>; line is removed, as there are would be no specialization encountered in the translation unit, usages of Foo::Bar would instead be qualified as Foo<T>::Bar.

When multiple specializations are present, the one that is chosen is the first one that is encountered; due to the current implementation of the behavior and the fact that iterating the specialization of a ClassTemplateDecl respects the order in which the specializations were stored in.

That is, if we were to add a reference to some other specialization in the above example, so that it would be parsed before the current using Baz = Foo<int>;, say:

template<typename T>
class Foo {
    using Bar = T;
}

using Bat = Foo<double>;
using Baz = Foo<int>;

Then usages of Foo::Bar would be qualified as Foo<double>::Bar instead of Foo<int>::Bar.

Should the same declaration be added, instead, after the using Baz = Foo<int>; line, then the qualification would remain consistent with our previous version.

To provide a more consistent behavior, that avoids changing the output when relatively unrelated declarations are present in the translation unit,
clang::TypeName::getFullyQualifiedType/clang::TypeName::getFullyQualifiedName will now qualify such types using the original templated declaration instead of one of the specializations.

diseraluca · 2023-09-27T15:01:15Z

This might be a breaking change, so I'm not sure if it will be
possible to merge it, but I would like to give some context on where
the change is coming from.

I'm trying (in my position as an employee of The Qt Company), to
migrate our documentation generation tool, QDoc, which depends on both
of Clang's C and C++ APIs, and similarly our CI systems, to support
LLVM 16 and 17 to catch up with the current releases.

Due to a bug fix that happened between 15 and 16, our product behaves
differently on the two versions, and we are using
clang::TypeName::getFullyQualifiedType/clang::TypeName::getFullyQualifiedName
to stabilize the software behavior on pre-16 and post-16 LLVM
releases, which is the suggested migration path
( #61810 ,
#58040 ).

Using those functions has been generally working for us, but the
behavior that I'm changing in this patch has been a blocker.

As a documentation generation tool we generally require a certain
consistency between generations of different version of the same
codebase, unless something in the code changed so that it justifies
the changes in the documentation itself.

Due to Qt's codebase being quite convoluted, we don't really have too
much control about the order that certain specializations will be
encountered in, albeit we could do some trick to try and control that
from QDoc.

Nonetheless, we are generally required to have the output be stable
when the documented code is not meaningfully changed between release
sets of our documentation.

Having the produced output change because of the first encountered
specialization changing in the codebase is not feasible, as it break
this desiderable property of a documentation set and may break our
regression-testing suite that is enforced in CI.

Furthermore, the chosen specialization may produce incorrect
information, which we cannot show to the end user of the documentation
set.

For example, we currently have QFuture define a const_iterator
alias that is later used as the argument or return type of some of our
documented callables.

Due to the way the current codebase is set up, the chosen
specialization is QFuture<void> so that, for example,
QFuture::constBegin() would be shown to the user as returning a
QFuture<void>::const_iterator.
Nonetheless, QFuture<void> is the only specialization that cannot
have a const_iterator and, as a consequence, doesn't have a
constBegin method in the first place.

It is obviously impossible to have this change in 16/17.
My current plan is to maintain our own implementation of
clang::TypeName::getFullyQualifiedType/clang::TypeName::getFullyQualifiedName,
but would prefer to be able to converge back when we can consider 18
the minimum version needed for QDoc, quite far in the future.

I further generally think that a more stable output is a preferred
property for this type of computations considering the possible
usages, such that the lack of the current behavior is either a better
default or a non-meaningful change in many expected usages.

llvmbot · 2023-09-27T15:02:05Z

@llvm/pr-subscribers-clang

Changes

…ation

When
clang::TypeName::getFullyQualifiedType/clang::TypeName::getFullyQualifiedName encounter a non-dependent type/type alias that is defined under a templated class/struct, it qualifies the type/type alias under a specialization of the templated class, if any was declared.

That is, in:

template&lt;typename T&gt;
class Foo {
    using Bar = T;
}

using Baz = Foo&lt;int&gt;;

Usages of Foo::Bar will be qualified as Foo<int>::Bar.

When the using Baz = Foo<int>; line is removed, as there are would be no specialization encountered in the translation unit, usages of Foo::Bar would instead be qualified as Foo<T>::Bar.

When multiple specializations are present, the one that is chosen is the first one that is encountered; due to the current implementation of the behavior and the fact that iterating the specialization of a ClassTemplateDecl respects the order in which the specializations were stored in.

That is, if we were to add a reference to some other specialization in the above example, so that it would be parsed before the current using Baz = Foo<int>;, say:

template&lt;typename T&gt;
class Foo {
    using Bar = T;
}

using Bat = Foo&lt;double&gt;;
using Baz = Foo&lt;int&gt;;

Then usages of Foo::Bar would be qualified as Foo<double>::Bar instead of Foo<int>::Bar.

Should the same declaration be added, instead, after the using Baz = Foo<int>; line, then the qualification would remain consistent with our previous version.

To provide a more consistent behavior, that avoids changing the output when relatively unrelated declarations are present in the translation unit,
clang::TypeName::getFullyQualifiedType/clang::TypeName::getFullyQualifiedName will now qualify such types using the original templated declaration instead of one of the specializations.

Full diff: https://github.com/llvm/llvm-project/pull/67566.diff

2 Files Affected:

(modified) clang/lib/AST/QualTypeNames.cpp (-21)
(modified) clang/unittests/Tooling/QualTypeNamesTest.cpp (+40-43)

diff --git a/clang/lib/AST/QualTypeNames.cpp b/clang/lib/AST/QualTypeNames.cpp
index 7557336f0aafa88..32fba2ab887d27c 100644
--- a/clang/lib/AST/QualTypeNames.cpp
+++ b/clang/lib/AST/QualTypeNames.cpp
@@ -272,27 +272,6 @@ static NestedNameSpecifier *createNestedNameSpecifierForScopeOf(
   const auto *Outer = dyn_cast_or_null<NamedDecl>(DC);
   const auto *OuterNS = dyn_cast_or_null<NamespaceDecl>(DC);
   if (Outer && !(OuterNS && OuterNS->isAnonymousNamespace())) {
-    if (const auto *CxxDecl = dyn_cast<CXXRecordDecl>(DC)) {
-      if (ClassTemplateDecl *ClassTempl =
-              CxxDecl->getDescribedClassTemplate()) {
-        // We are in the case of a type(def) that was declared in a
-        // class template but is *not* type dependent.  In clang, it
-        // gets attached to the class template declaration rather than
-        // any specific class template instantiation.  This result in
-        // 'odd' fully qualified typename:
-        //
-        //    vector<_Tp,_Alloc>::size_type
-        //
-        // Make the situation is 'useable' but looking a bit odd by
-        // picking a random instance as the declaring context.
-        if (ClassTempl->spec_begin() != ClassTempl->spec_end()) {
-          Decl = *(ClassTempl->spec_begin());
-          Outer = dyn_cast<NamedDecl>(Decl);
-          OuterNS = dyn_cast<NamespaceDecl>(Decl);
-        }
-      }
-    }
-
     if (OuterNS) {
       return createNestedNameSpecifier(Ctx, OuterNS, WithGlobalNsPrefix);
     } else if (const auto *TD = dyn_cast<TagDecl>(Outer)) {
diff --git a/clang/unittests/Tooling/QualTypeNamesTest.cpp b/clang/unittests/Tooling/QualTypeNamesTest.cpp
index 686d189cf69eb2f..18286f9e74921dd 100644
--- a/clang/unittests/Tooling/QualTypeNamesTest.cpp
+++ b/clang/unittests/Tooling/QualTypeNamesTest.cpp
@@ -85,7 +85,7 @@ TEST(QualTypeNameTest, Simple) {
   // Namespace alias
   Visitor.ExpectedQualTypeNames["CheckL"] = "A::B::C::MyInt";
   Visitor.ExpectedQualTypeNames["non_dependent_type_var"] =
-      "Foo<X>::non_dependent_type";
+      "Foo<T>::non_dependent_type";
   Visitor.ExpectedQualTypeNames["AnEnumVar"] = "EnumScopeClass::AnEnum";
   Visitor.ExpectedQualTypeNames["AliasTypeVal"] = "A::B::C::InnerAlias<int>";
   Visitor.ExpectedQualTypeNames["AliasInnerTypeVal"] =
@@ -175,20 +175,19 @@ TEST(QualTypeNameTest, Simple) {
 TEST(QualTypeNameTest, Complex) {
   TypeNameVisitor Complex;
   Complex.ExpectedQualTypeNames["CheckTX"] = "B::TX";
-  Complex.runOver(
-      "namespace A {"
-      "  struct X {};"
-      "}"
-      "using A::X;"
-      "namespace fake_std {"
-      "  template<class... Types > class tuple {};"
-      "}"
-      "namespace B {"
-      "  using fake_std::tuple;"
-      "  typedef tuple<X> TX;"
-      "  TX CheckTX;"
-      "  struct A { typedef int X; };"
-      "}");
+  Complex.runOver("namespace A {"
+                  "  struct X {};"
+                  "}"
+                  "using A::X;"
+                  "namespace fake_std {"
+                  "  template<class... Types > class tuple {};"
+                  "}"
+                  "namespace B {"
+                  "  using fake_std::tuple;"
+                  "  typedef tuple<X> TX;"
+                  "  TX CheckTX;"
+                  "  struct A { typedef int X; };"
+                  "}");
 }
 
 TEST(QualTypeNameTest, DoubleUsing) {
@@ -223,33 +222,31 @@ TEST(QualTypeNameTest, GlobalNsPrefix) {
   GlobalNsPrefix.ExpectedQualTypeNames["GlobalZVal"] = "::Z";
   GlobalNsPrefix.ExpectedQualTypeNames["CheckK"] = "D::aStruct";
   GlobalNsPrefix.ExpectedQualTypeNames["YZMPtr"] = "::A::B::X ::A::B::Y::Z::*";
-  GlobalNsPrefix.runOver(
-      "namespace A {\n"
-      "  namespace B {\n"
-      "    int IntVal;\n"
-      "    bool BoolVal;\n"
-      "    struct X {};\n"
-      "    X XVal;\n"
-      "    template <typename T> class CCC { };\n"
-      "    template <typename T>\n"
-      "    using Alias = CCC<T>;\n"
-      "    Alias<int> IntAliasVal;\n"
-      "    struct Y { struct Z { X YZIPtr; }; };\n"
-      "    Y::Z ZVal;\n"
-      "    X Y::Z::*YZMPtr;\n"
-      "  }\n"
-      "}\n"
-      "struct Z {};\n"
-      "Z GlobalZVal;\n"
-      "namespace {\n"
-      "  namespace D {\n"
-      "    namespace {\n"
-      "      class aStruct {};\n"
-      "      aStruct CheckK;\n"
-      "    }\n"
-      "  }\n"
-      "}\n"
-  );
+  GlobalNsPrefix.runOver("namespace A {\n"
+                         "  namespace B {\n"
+                         "    int IntVal;\n"
+                         "    bool BoolVal;\n"
+                         "    struct X {};\n"
+                         "    X XVal;\n"
+                         "    template <typename T> class CCC { };\n"
+                         "    template <typename T>\n"
+                         "    using Alias = CCC<T>;\n"
+                         "    Alias<int> IntAliasVal;\n"
+                         "    struct Y { struct Z { X YZIPtr; }; };\n"
+                         "    Y::Z ZVal;\n"
+                         "    X Y::Z::*YZMPtr;\n"
+                         "  }\n"
+                         "}\n"
+                         "struct Z {};\n"
+                         "Z GlobalZVal;\n"
+                         "namespace {\n"
+                         "  namespace D {\n"
+                         "    namespace {\n"
+                         "      class aStruct {};\n"
+                         "      aStruct CheckK;\n"
+                         "    }\n"
+                         "  }\n"
+                         "}\n");
 }
 
 TEST(QualTypeNameTest, InlineNamespace) {
@@ -297,4 +294,4 @@ TEST(QualTypeNameTest, ConstUsing) {
                         using ::A::S;
                         void foo(const S& param1, const S param2);)");
 }
-}  // end anonymous namespace
+} // end anonymous namespace

github-actions · 2023-09-27T15:10:44Z

✅ With the latest revision this PR passed the C/C++ code formatter.

mizvekov · 2023-09-27T15:28:28Z

CC @vgvassilev @pcanal @Sterling-Augustine
As the removed workaround dates back to the initial implementation imported in 0dd191a from Cling.

When `clang::TypeName::getFullyQualifiedType`/`clang::TypeName::getFullyQualifiedName` encounter a non-dependent type/type alias that is defined under a templated class/struct, it qualifies the type/type alias under a specialization of the templated class, if any was declared. That is, in: ``` template<typename T> class Foo { using Bar = T; } using Baz = Foo<int>; ``` Usages of `Foo::Bar` will be qualified as `Foo<int>::Bar`. When the `using Baz = Foo<int>;` line is removed, as there are would be no specialization encountered in the translation unit, usages of `Foo::Bar` would instead be qualified as `Foo<T>::Bar`. When multiple specializations are present, the one that is chosen is the first one that is encountered; due to the current implementation of the behavior and the fact that iterating the specialization of a `ClassTemplateDecl` respects the order in which the specializations were stored in. That is, if we were to add a reference to some other specialization in the above example, so that it would be parsed before the current `using Baz = Foo<int>;`, say: ``` template<typename T> class Foo { using Bar = T; } using Bat = Foo<double>; using Baz = Foo<int>; ``` Then usages of `Foo::Bar` would be qualified as `Foo<double>::Bar` instead of `Foo<int>::Bar`. Should the same declaration be added, instead, after the `using Baz = Foo<int>;` line, then the qualification would remain consistent with our previous version. To provide a more consistent behavior, that avoids changing the output when relatively unrelated declarations are present in the translation unit, `clang::TypeName::getFullyQualifiedType`/`clang::TypeName::getFullyQualifiedName` will now qualify such types using the original templated declaration instead of one of the specializations.

diseraluca · 2023-09-27T17:19:13Z

Run clang-format on the patch.

I made a bit of a mess as I haven't used the PR model in a very long time. Hopefully this is correct.

mizvekov · 2023-09-27T17:25:35Z

It's recommended to avoid introducing pure clang-format changes mixed with other non-style changes.

The recommended way to avoid that is to run clang-format on the modified files on a separate commit, and then just merge that, no review required.

vgvassilev · 2023-09-29T20:10:56Z

@diseraluca, thanks for the thorough description. The point of these routines is to produce code that compiles. I am not sure if we change Foo<int>::Bar with Foo<T>::Bar it will compile.

Due to the way the current codebase is set up, the chosen specialization is QFuture<void> so that, for example, QFuture::constBegin() would be shown to the user as returning a QFuture<void>::const_iterator. Nonetheless, QFuture<void> is the only specialization that cannot have a const_iterator and, as a consequence, doesn't have a constBegin method in the first place.

I suspect that QFuture<void> is an explicit specialization. The intent of the code was to pick up an implicit specialization which follows much more closely the template pattern. Would selecting the first non-explicit instantiation fix your usecase?

diseraluca · 2023-09-29T21:09:31Z

@diseraluca, thanks for the thorough description. The point of these routines is to produce code that compiles. I am not sure if we change Foo<int>::Bar with Foo<T>::Bar it will compile.

Due to the way the current codebase is set up, the chosen specialization is QFuture<void> so that, for example, QFuture::constBegin() would be shown to the user as returning a QFuture<void>::const_iterator. Nonetheless, QFuture<void> is the only specialization that cannot have a const_iterator and, as a consequence, doesn't have a constBegin method in the first place.

I suspect that QFuture<void> is an explicit specialization. The intent of the code was to pick up an implicit specialization which follows much more closely the template pattern. Would selecting the first non-explicit instantiation fix your usecase?

Thank you for your answer @vgvassilev.

I gave it a quick try, and we would still end up with the same result in our codebase. But, generally, this would not probably be feasible for us as a solution.

Since that is the case, and we have somewhat conflicting requirements, I think the best path would be for me to indefinitely maintain a custom implementation of getFullyQualified*, leaving upstream as is.

I would thus suggest to close this without merging.

vgvassilev · 2023-09-29T21:15:29Z

I gave it a quick try, and we would still end up with the same result in our codebase. But, generally, this would not probably be feasible for us as a solution.

do you have an idea why? Can you show the diff of the changes you made? Is the void specialization not explicit?

diseraluca · 2023-09-29T22:06:52Z

I gave it a quick try, and we would still end up with the same result in our codebase. But, generally, this would not probably be feasible for us as a solution.

do you have an idea why? Can you show the diff of the changes you made? Is the void specialization not explicit?

I did a quick test with this:

diff --git a/clang/lib/AST/QualTypeNames.cpp b/clang/lib/AST/QualTypeNames.cpp
index 26aaa96a1dc6..8b882eda83bb 100644
--- a/clang/lib/AST/QualTypeNames.cpp
+++ b/clang/lib/AST/QualTypeNames.cpp
@@ -287,8 +287,13 @@ static NestedNameSpecifier *createNestedNameSpecifierForScopeOf(
         //
         // Make the situation is 'useable' but looking a bit odd by
         // picking a random instance as the declaring context.
-        if (ClassTempl->spec_begin() != ClassTempl->spec_end()) {
-          Decl = *(ClassTempl->spec_begin());
+        auto specialization_iterator = std::find_if(
+            ClassTempl->spec_begin(), ClassTempl->spec_end(), [](auto a) {
+              return !a->isExplicitInstantiationOrSpecialization();
+            });
+
+        if (specialization_iterator != ClassTempl->spec_end()) {
+          Decl = *specialization_iterator;
           Outer = dyn_cast<NamedDecl>(Decl);
           OuterNS = dyn_cast<NamespaceDecl>(Decl);
         }

Do please let me know if this is incorrect or if I misunderstood your proposal.

We do have explicit references to the void specialization in the codebase so it would explain the previous choice. But I'm not aware of why it would be chosen as a non-explicit one too.

I can try to debug that on Monday if that can be useful. Albeit it might take some time.

vgvassilev · 2023-09-30T06:18:35Z

I gave it a quick try, and we would still end up with the same result in our codebase. But, generally, this would not probably be feasible for us as a solution.

do you have an idea why? Can you show the diff of the changes you made? Is the void specialization not explicit?

I did a quick test with this:
diff --git a/clang/lib/AST/QualTypeNames.cpp b/clang/lib/AST/QualTypeNames.cpp
index 26aaa96a1dc6..8b882eda83bb 100644
--- a/clang/lib/AST/QualTypeNames.cpp
+++ b/clang/lib/AST/QualTypeNames.cpp
@@ -287,8 +287,13 @@ static NestedNameSpecifier *createNestedNameSpecifierForScopeOf(
         //
         // Make the situation is 'useable' but looking a bit odd by
         // picking a random instance as the declaring context.
-        if (ClassTempl->spec_begin() != ClassTempl->spec_end()) {
-          Decl = *(ClassTempl->spec_begin());
+        auto specialization_iterator = std::find_if(
+            ClassTempl->spec_begin(), ClassTempl->spec_end(), [](auto a) {
+              return !a->isExplicitInstantiationOrSpecialization();
+            });
+
+        if (specialization_iterator != ClassTempl->spec_end()) {
+          Decl = *specialization_iterator;
           Outer = dyn_cast<NamedDecl>(Decl);
           OuterNS = dyn_cast<NamespaceDecl>(Decl);
         }
Do please let me know if this is incorrect or if I misunderstood your proposal.

We do have explicit references to the void specialization in the codebase so it would explain the previous choice. But I'm not aware of why it would be chosen as a non-explicit one too.

I can try to debug that on Monday if that can be useful. Albeit it might take some time.

You probably need isExplicitSpecialization()

diseraluca · 2023-09-30T08:06:18Z

I gave it a quick try, and we would still end up with the same result in our codebase. But, generally, this would not probably be feasible for us as a solution.

do you have an idea why? Can you show the diff of the changes you made? Is the void specialization not explicit?

I did a quick test with this:
diff --git a/clang/lib/AST/QualTypeNames.cpp b/clang/lib/AST/QualTypeNames.cpp
index 26aaa96a1dc6..8b882eda83bb 100644
--- a/clang/lib/AST/QualTypeNames.cpp
+++ b/clang/lib/AST/QualTypeNames.cpp
@@ -287,8 +287,13 @@ static NestedNameSpecifier *createNestedNameSpecifierForScopeOf(
         //
         // Make the situation is 'useable' but looking a bit odd by
         // picking a random instance as the declaring context.
-        if (ClassTempl->spec_begin() != ClassTempl->spec_end()) {
-          Decl = *(ClassTempl->spec_begin());
+        auto specialization_iterator = std::find_if(
+            ClassTempl->spec_begin(), ClassTempl->spec_end(), [](auto a) {
+              return !a->isExplicitInstantiationOrSpecialization();
+            });
+
+        if (specialization_iterator != ClassTempl->spec_end()) {
+          Decl = *specialization_iterator;
           Outer = dyn_cast<NamedDecl>(Decl);
           OuterNS = dyn_cast<NamespaceDecl>(Decl);
         }
Do please let me know if this is incorrect or if I misunderstood your proposal.
We do have explicit references to the void specialization in the codebase so it would explain the previous choice. But I'm not aware of why it would be chosen as a non-explicit one too.
I can try to debug that on Monday if that can be useful. Albeit it might take some time.
You probably need isExplicitSpecialization()

Wouldn't isExplicitSpecializationOrInstantiation be a stronger guarantee than isExplicitSpecialization, such that it would exclude a superset of what is excluded by isExplicitSpecialization? If I did not misunderstand their source code.

Nonetheless, I've made a test run with isExplicitSpecialization, but in Qt's codebase we still get the same result.

I generally don't think we can depend on this kind of behavior, especially as it is far too difficult to control for the kind of consistency we want.

I initially missed the fact that the intention of this was to produce compilable code. While similar to our use-case it has slightly different requirements and I feel that may not be compatible with ours.

I think that is fine, and I think we don't need to force it to work for all use cases, especially because I really don't want to break other people code. My assumption was that this might be an improvement for other use-cases too, but I failed to see the "give me code that compiles" use case.

The only thing that comes to mind would be to condition the behavior to be there or not. Similar to how we have WighGlobalNsPrefix we might have another boolean or similar, so that the default behavior remains the same but it can be conditioned to avoid this case.

But I don't think that is a particularly good solution, so I'm pretty happy with maintaining our own version too.

vgvassilev · 2023-09-30T08:18:43Z

Wouldn't isExplicitSpecializationOrInstantiation be a stronger guarantee than isExplicitSpecialization, such that it would exclude a superset of what is excluded by isExplicitSpecialization? If I did not misunderstand their source code.

I wanted to filter out instantiations.

I generally don't think we can depend on this kind of behavior, especially as it is far too difficult to control for the kind of consistency we want.

I think we could make the template specializations consistent so that you at least cure the symptom of your problem.

My assumption was that this might be an improvement for other use-cases too, but I failed to see the "give me code that compiles" use case.

Understood. That's a pretty strong requirement for this case and we cannot really make a compromise. The idea is to offer an API which gives the user how would they reach certain AST node from within the global scope if they had to type it.

The only thing that comes to mind would be to condition the behavior to be there or not. Similar to how we have WighGlobalNsPrefix we might have another boolean or similar, so that the default behavior remains the same but it can be conditioned to avoid this case.

I would avoid doing that. In fact, I still do not see why the proposed solution would not work. If it solves the minimal case that you provided in the description of this PR, I am afraid that there is some bit from the Qt setup that we do not understand.

diseraluca · 2023-10-01T16:31:31Z

I still do not see why the proposed solution would not work. If it solves the minimal case that you provided in the description of this PR, I am afraid that there is some bit from the Qt setup that we do not understand.

I cannot for certain say if there is an issue or not with how the Qt codebase is setup; but I think it is out of the scope of the task we are trying to solve to change that based on the behavior of QDoc interoperations with Clang.

For what it is worth, I've been playing with a very small example to check a few possibilities, and even excluding explicit specializations we still have certain cases that are not stable.

Say in

#pragma once

template<typename T>
class QList {
public:
	using Foo = const T&;

	void begin(Foo);
};

Adding something as using Bar = QList<int>;, and this being the only code in the translation unit, we still have the addition of such a line modifying our output, qualifying the type of the first parameter of begin as QList<int>::Foo, even with your proposal.

Say we can stabilize that, so that it always qualify in the same way, say QList<int>.

If we are documenting, maybe, some method QList::Foo QList::bar() and
we show to the user the return type as QList<int>::Foo, this is going to be misleading,
albeit not as erroneous as the QFuture<void> case, for our user.

Indeed, the return type of bar really depends on the instantiation of QList that we are looking at and it would be extremely confusing for a user skimming our documentation to think that it always returns a QList<int>::Foo.
As a documentation generator we really want to look at the generic case for our output.

Now, say we have a very stable output, albeit not adequate for our case, it might simplify doing some postprocessing to qualify in the way we want, but at that point we still need to maintain something relatively similar to getFullyQualified*, thus why it isn't that much of a difference for our use case to only have a more stable output.

There are some other cases that might be iffy for our use-case too, with this behavior.

Thus why I think, albeit unfortunate, our use-cases just happen to be somewhat in conflict with regards to the output of getFullyQualified*.

@vgvassilev Both me an my team are deeply thankful for your help. It was really kind to try and find a solution for our case.

I will close this PR as I don't think it can be merged, as it would break other people use cases.
I will leave it open 1 or 2 days more in case someone else wants to chime in with something.

vgvassilev · 2023-10-01T16:40:11Z

@diseraluca, since this code touches a case where we do some best effort recovery, would it be possible to change that interface to take a lambda/callback function and specialize it on your end to cover the docs case?

diseraluca · 2023-10-01T20:16:57Z

@vgvassilev If that is an acceptable interface for the LLVM interface then, yes, it would be perfect from our side, and I'm more than happy to update the PR in the next few days.

Just to be sure that I understood your proposal.

getFullyQualified* calls will accept a new parameter, a callable, that will be passed down the call chain up to createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...) and will be called when the teplate case is encountered?
Or are you thinking more of a callable that replaces the call to createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...)?

vgvassilev · 2023-10-02T20:43:12Z

@vgvassilev If that is an acceptable interface for the LLVM interface then, yes, it would be perfect from our side, and I'm more than happy to update the PR in the next few days.

Just to be sure that I understood your proposal.

getFullyQualified* calls will accept a new parameter, a callable, that will be passed down the call chain up to createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...) and will be called when the teplate case is encountered? Or are you thinking more of a callable that replaces the call to createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...)?

I hesitate. Maybe we can pass a custom "policy" option and incorporate your code in there... We reiterate if the solution does not look good?

diseraluca · 2023-10-04T13:04:36Z

@vgvassilev If that is an acceptable interface for the LLVM interface then, yes, it would be perfect from our side, and I'm more than happy to update the PR in the next few days.
Just to be sure that I understood your proposal.
getFullyQualified* calls will accept a new parameter, a callable, that will be passed down the call chain up to createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...) and will be called when the teplate case is encountered? Or are you thinking more of a callable that replaces the call to createNestedNameSpecifierForScopeOf(const ASTContext &, const Decl *, ...)?

I hesitate. Maybe we can pass a custom "policy" option and incorporate your code in there... We reiterate if the solution does not look good?

From our point of view any solution is acceptable. But do note that from our side it is not so much about incorporating custom code as much as removing that specific behavior at this point in time.

Could you expand about the policy? Are you talking about a "printing policy" or a custom policy for getFullyQualified?

diseraluca · 2023-10-17T10:03:25Z

Closing this one, as we intend to maintain our own local version.

QDoc depends on the LLVM project, specifically Clang and its exported APIs, to perform its operations. The Clang APIs are used by QDoc to parse the source code of a project, both to find the user-provided documentation and to perform certain sanity checks about it. For many elements, the documentation that is at the end shown to the user, uses text that is extracted from the source code by the usage of Clang. For example, when a user documents a method, the stringified user-presentable version of the types of the arguments of that method will be obtained by a usage of Clang's APIs by QDoc. In-between LLVM 15 and LLVM 16, the upstream project fixed a multi-year long bug that modified the way in which the common operations to obtain certain stringified representations of the code actually format that code. For example, the stringified representation of a type's name will be qualified differently on LLVM 15 and LLVM 16+. Due to the way QDoc acquires the representation for certain source code components and the changes between LLVM 15 and LLVM 16, the output that QDoc produces will be different between the two versions. QDoc provides a regression testing suite that is based on the final output documentation that QDoc builds and that is enforced in CI. The changes in the final documentation output caused by the changes in the LLVM project made it difficult to update the provisioned version in CI as the regression test suite would break and it is not currently possible to provide a version that would work on both LLVM 15 and LLVM 16 with the current codebase. Furthermore, the changes in the output documentation produce generally less information dense sets, which is a step back from the previous format that the usage of Clang would produce. The suggested path of conversion between the two version is the usage of `clang::TypeName::getFullyQualifiedName` to handle the retrieval of type representations (llvm/llvm-project#61810). While that is mostly adept at the purposes that QDoc intends to use Clang for, a very specific behavior of `clang::TypeName::getFullyQualifiedName` is just not suitable for the production of documentation sets. Changes to this behavior were attempted to push upstream, but due to an unfortunate collision of interest with other projects depending on the behavior it was not possible to find a satisfying compromise (llvm/llvm-project#67566). Furthermore, even if a compromise was possible, the changes would not be available until at least LLVM 18, which is the next release, so that any transition to LLVM 16 would incur into the same issue. Due to this unsuitable behavior, QDoc does not have a path of migration that would be consistent on both LLVM 15 and LLVM 16, as `clang::TypeName::getFullyQualifiedName` would not be usable. This would leave QDoc with producing a custom solution. This custom solution would generally look very similar to `clang::TypeName::getFullyQualifiedName`, as the purposes of QDoc and that of that function are mostly equivalent. Hence, instead of producing a completely custom solution, `QualTypeNames`, the file that provides `clang::TypeName::getFullyQualifiedName` is imported from the upstream LLVM project and redistributed as part of QDoc, so as to allow us to perform the very small modifications we need for it to be suitable for our project. A new directory structure "clang/AST" was added under the source directory for QDoc, to replicate the the actual include path that is exported from the LLVM project. Under "clang/AST", "QualTypeNames.h" was added, replicating the upstream code that defines `clang::TypeName::getFullyQualifiedName`. A "qt_attribution.json" file was added under "clang/AST" to satisfy the license requirement from the LLVM project. Similarly, an "LLVM_LICENSE.txt" was put under the same directory to redistribute the license file of the LLVM project, as part of license compliance. Task-number: QTBUG-111580 Change-Id: I3f51222ea1cfe2f3e526cc2b43df211622b85fb9 Reviewed-by: Qt CI Bot <[email protected]> Reviewed-by: Topi Reiniö <[email protected]>

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 27, 2023

diseraluca requested review from mizvekov and AaronBallman September 27, 2023 15:02

diseraluca force-pushed the fully_qualified_type_class_inner_type_template branch from d306cc7 to d19d716 Compare September 27, 2023 17:17

diseraluca changed the title ~~Qualify non-dependent types of a class template with the class declar…~~ Qualify non-dependent types of a class template with its declaration Sep 27, 2023

diseraluca closed this Oct 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qualify non-dependent types of a class template with its declaration #67566

Qualify non-dependent types of a class template with its declaration #67566

diseraluca commented Sep 27, 2023 •

edited

Loading

diseraluca commented Sep 27, 2023

llvmbot commented Sep 27, 2023

github-actions bot commented Sep 27, 2023 •

edited

Loading

mizvekov commented Sep 27, 2023

diseraluca commented Sep 27, 2023

mizvekov commented Sep 27, 2023

vgvassilev commented Sep 29, 2023

diseraluca commented Sep 29, 2023

vgvassilev commented Sep 29, 2023

diseraluca commented Sep 29, 2023

vgvassilev commented Sep 30, 2023

diseraluca commented Sep 30, 2023

vgvassilev commented Sep 30, 2023

diseraluca commented Oct 1, 2023

vgvassilev commented Oct 1, 2023 •

edited

Loading

diseraluca commented Oct 1, 2023 •

edited

Loading

vgvassilev commented Oct 2, 2023

diseraluca commented Oct 4, 2023

diseraluca commented Oct 17, 2023

Qualify non-dependent types of a class template with its declaration #67566

Qualify non-dependent types of a class template with its declaration #67566

Conversation

diseraluca commented Sep 27, 2023 • edited Loading

diseraluca commented Sep 27, 2023

llvmbot commented Sep 27, 2023

github-actions bot commented Sep 27, 2023 • edited Loading

mizvekov commented Sep 27, 2023

diseraluca commented Sep 27, 2023

mizvekov commented Sep 27, 2023

vgvassilev commented Sep 29, 2023

diseraluca commented Sep 29, 2023

vgvassilev commented Sep 29, 2023

diseraluca commented Sep 29, 2023

vgvassilev commented Sep 30, 2023

diseraluca commented Sep 30, 2023

vgvassilev commented Sep 30, 2023

diseraluca commented Oct 1, 2023

vgvassilev commented Oct 1, 2023 • edited Loading

diseraluca commented Oct 1, 2023 • edited Loading

vgvassilev commented Oct 2, 2023

diseraluca commented Oct 4, 2023

diseraluca commented Oct 17, 2023

diseraluca commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

vgvassilev commented Oct 1, 2023 •

edited

Loading

diseraluca commented Oct 1, 2023 •

edited

Loading