[clang] ASTContext: flesh out implementation of getCommonNNS

[mizvekov]

This properly implements getCommonNNS, for getting the common NestedNameSpecifier, for which the previous implementation was a bare minimum placeholder.

[mizvekov]

This was originally split-off from #130537

[llvmbot]

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

This properly implements getCommonNNS, for getting the common NestedNameSpecifier, for which the previous implementation was a bare minimum placeholder.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3-1)
• (modified) clang/lib/AST/ASTContext.cpp (+115-11)
• (modified) clang/test/SemaCXX/sugar-common-types.cpp (+2-2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0adbc19f40096..e256a382dffd6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -264,7 +264,9 @@ Improvements to Clang's diagnostics
   as function arguments or return value respectively. Note that
   :doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
   feature will be default-enabled with ``-Wthread-safety`` in a future release.
-- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~) and minus(-) as signed integers 
+- Clang will now do a better job producing common nested names, when producing
+  common types for ternary operator, template argument deduction and multiple return auto deduction.
+- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~) and minus(-) as signed integers
   except for the case where the operand is an unsigned integer
   and throws warning if they are compared with unsigned integers (##18878).
 - The ``-Wunnecessary-virtual-specifier`` warning has been added to warn about
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 7ed5b033d9bd8..8ae938244601b 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -13462,13 +13462,114 @@ static ElaboratedTypeKeyword getCommonTypeKeyword(const T *X, const T *Y) {
                                             : ElaboratedTypeKeyword::None;
 }
 
+static NestedNameSpecifier *getCommonNNS(ASTContext &Ctx,
+                                         NestedNameSpecifier *X,
+                                         NestedNameSpecifier *Y, bool IsSame) {
+  if (X == Y)
+    return X;
+
+  NestedNameSpecifier *Canon = Ctx.getCanonicalNestedNameSpecifier(X);
+  if (Canon != Ctx.getCanonicalNestedNameSpecifier(Y)) {
+    assert(!IsSame && "Should be the same NestedNameSpecifier");
+    return nullptr;
+  }
+
+  NestedNameSpecifier *R = nullptr;
+  switch (auto KX = X->getKind(), KY = Y->getKind(); KX) {
+  case NestedNameSpecifier::SpecifierKind::Identifier: {
+    assert(KY == NestedNameSpecifier::SpecifierKind::Identifier);
+    IdentifierInfo *II = X->getAsIdentifier();
+    assert(II == Y->getAsIdentifier());
+    NestedNameSpecifier *P =
+        ::getCommonNNS(Ctx, X->getPrefix(), Y->getPrefix(), /*IsSame=*/true);
+    R = NestedNameSpecifier::Create(Ctx, P, II);
+    break;
+  }
+  case NestedNameSpecifier::SpecifierKind::Namespace:
+  case NestedNameSpecifier::SpecifierKind::NamespaceAlias: {
+    assert(KY == NestedNameSpecifier::SpecifierKind::Namespace ||
+           KY == NestedNameSpecifier::SpecifierKind::NamespaceAlias);
+    NestedNameSpecifier *P = ::getCommonNNS(Ctx, X->getPrefix(), Y->getPrefix(),
+                                            /*IsSame=*/false);
+    NamespaceAliasDecl *AX = X->getAsNamespaceAlias(),
+                       *AY = Y->getAsNamespaceAlias();
+    if (declaresSameEntity(AX, AY)) {
+      R = NestedNameSpecifier::Create(Ctx, P, ::getCommonDeclChecked(AX, AY));
+      break;
+    }
+    NamespaceDecl *NX = AX ? AX->getNamespace() : X->getAsNamespace(),
+                  *NY = AY ? AY->getNamespace() : Y->getAsNamespace();
+    R = NestedNameSpecifier::Create(Ctx, P, ::getCommonDeclChecked(NX, NY));
+    break;
+  }
+  case NestedNameSpecifier::SpecifierKind::TypeSpec:
+  case NestedNameSpecifier::SpecifierKind::TypeSpecWithTemplate: {
+    assert(KY == NestedNameSpecifier::SpecifierKind::TypeSpec ||
+           KY == NestedNameSpecifier::SpecifierKind::TypeSpecWithTemplate);
+    bool Template =
+        KX == NestedNameSpecifier::SpecifierKind::TypeSpecWithTemplate &&
+        KY == NestedNameSpecifier::SpecifierKind::TypeSpecWithTemplate;
+
+    const Type *TX = X->getAsType(), *TY = Y->getAsType();
+    if (TX == TY) {
+      NestedNameSpecifier *P =
+          ::getCommonNNS(Ctx, X->getPrefix(), Y->getPrefix(),
+                         /*IsSame=*/false);
+      R = NestedNameSpecifier::Create(Ctx, P, Template, TX);
+      break;
+    }
+    // TODO: Try to salvage the original prefix.
+    // If getCommonSugaredType removed any top level sugar, the original prefix
+    // is not applicable anymore.
+    NestedNameSpecifier *P = nullptr;
+    const Type *T = Ctx.getCommonSugaredType(QualType(X->getAsType(), 0),
+                                             QualType(Y->getAsType(), 0),
+                                             /*Unqualified=*/true)
+                        .getTypePtr();
+    switch (T->getTypeClass()) {
+    case Type::Elaborated: {
+      auto *ET = cast<ElaboratedType>(T);
+      R = NestedNameSpecifier::Create(Ctx, ET->getQualifier(), Template,
+                                      ET->getNamedType().getTypePtr());
+      break;
+    }
+    case Type::DependentName: {
+      auto *DN = cast<DependentNameType>(T);
+      R = NestedNameSpecifier::Create(Ctx, DN->getQualifier(),
+                                      DN->getIdentifier());
+      break;
+    }
+    case Type::DependentTemplateSpecialization: {
+      auto *DTST = cast<DependentTemplateSpecializationType>(T);
+      T = Ctx.getDependentTemplateSpecializationType(
+                 DTST->getKeyword(), /*NNS=*/nullptr, DTST->getIdentifier(),
+                 DTST->template_arguments())
+              .getTypePtr();
+      P = DTST->getQualifier();
+      R = NestedNameSpecifier::Create(Ctx, DTST->getQualifier(), Template, T);
+      break;
+    }
+    default:
+      R = NestedNameSpecifier::Create(Ctx, P, Template, T);
+      break;
+    }
+    break;
+  }
+  case NestedNameSpecifier::SpecifierKind::Global:
+    assert(KY == NestedNameSpecifier::SpecifierKind::Global);
+    return X;
+  case NestedNameSpecifier::SpecifierKind::Super:
+    assert(KY == NestedNameSpecifier::SpecifierKind::Super);
+    return X;
+  }
+  assert(Ctx.getCanonicalNestedNameSpecifier(R) == Canon);
+  return R;
+}
+
 template <class T>
-static NestedNameSpecifier *getCommonNNS(ASTContext &Ctx, const T *X,
-                                         const T *Y) {
-  // FIXME: Try to keep the common NNS sugar.
-  return X->getQualifier() == Y->getQualifier()
-             ? X->getQualifier()
-             : Ctx.getCanonicalNestedNameSpecifier(X->getQualifier());
+static NestedNameSpecifier *getCommonQualifier(ASTContext &Ctx, const T *X,
+                                               const T *Y, bool IsSame) {
+  return ::getCommonNNS(Ctx, X->getQualifier(), Y->getQualifier(), IsSame);
 }
 
 template <class T>
@@ -13879,8 +13980,9 @@ static QualType getCommonNonSugarTypeNode(ASTContext &Ctx, const Type *X,
                *NY = cast<DependentNameType>(Y);
     assert(NX->getIdentifier() == NY->getIdentifier());
     return Ctx.getDependentNameType(
-        getCommonTypeKeyword(NX, NY), getCommonNNS(Ctx, NX, NY),
-        NX->getIdentifier(), NX->getCanonicalTypeInternal());
+        getCommonTypeKeyword(NX, NY),
+        getCommonQualifier(Ctx, NX, NY, /*IsSame=*/true), NX->getIdentifier(),
+        NX->getCanonicalTypeInternal());
   }
   case Type::DependentTemplateSpecialization: {
     const auto *TX = cast<DependentTemplateSpecializationType>(X),
@@ -13889,8 +13991,9 @@ static QualType getCommonNonSugarTypeNode(ASTContext &Ctx, const Type *X,
     auto As = getCommonTemplateArguments(Ctx, TX->template_arguments(),
                                          TY->template_arguments());
     return Ctx.getDependentTemplateSpecializationType(
-        getCommonTypeKeyword(TX, TY), getCommonNNS(Ctx, TX, TY),
-        TX->getIdentifier(), As);
+        getCommonTypeKeyword(TX, TY),
+        getCommonQualifier(Ctx, TX, TY, /*IsSame=*/true), TX->getIdentifier(),
+        As);
   }
   case Type::UnaryTransform: {
     const auto *TX = cast<UnaryTransformType>(X),
@@ -14047,7 +14150,8 @@ static QualType getCommonSugarTypeNode(ASTContext &Ctx, const Type *X,
   case Type::Elaborated: {
     const auto *EX = cast<ElaboratedType>(X), *EY = cast<ElaboratedType>(Y);
     return Ctx.getElaboratedType(
-        ::getCommonTypeKeyword(EX, EY), ::getCommonNNS(Ctx, EX, EY),
+        ::getCommonTypeKeyword(EX, EY),
+        ::getCommonQualifier(Ctx, EX, EY, /*IsSame=*/false),
         Ctx.getQualifiedType(Underlying),
         ::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl()));
   }
diff --git a/clang/test/SemaCXX/sugar-common-types.cpp b/clang/test/SemaCXX/sugar-common-types.cpp
index 39a762127811f..3c4de0e2abd5e 100644
--- a/clang/test/SemaCXX/sugar-common-types.cpp
+++ b/clang/test/SemaCXX/sugar-common-types.cpp
@@ -44,7 +44,7 @@ template <class T> struct S1 {
 };
 
 N t10 = 0 ? S1<X1>() : S1<Y1>(); // expected-error {{from 'S1<B1>' (aka 'S1<int>')}}
-N t11 = 0 ? S1<X1>::S2<X2>() : S1<Y1>::S2<Y2>(); // expected-error {{from 'S1<int>::S2<B2>' (aka 'S2<void>')}}
+N t11 = 0 ? S1<X1>::S2<X2>() : S1<Y1>::S2<Y2>(); // expected-error {{from 'S1<B1>::S2<B2>' (aka 'S2<void>')}}
 
 template <class T> using Al = S1<T>;
 
@@ -88,7 +88,7 @@ N t19 = 0 ? (__underlying_type(EnumsX::X)){} : (__underlying_type(EnumsY::Y)){};
 // expected-error@-1 {{rvalue of type 'B1' (aka 'int')}}
 
 N t20 = 0 ? (__underlying_type(EnumsX::X)){} : (__underlying_type(EnumsY::X)){};
-// expected-error@-1 {{rvalue of type '__underlying_type(Enums::X)' (aka 'int')}}
+// expected-error@-1 {{rvalue of type '__underlying_type(EnumsB::X)' (aka 'int')}}
 
 using QX = const SB1 *;
 using QY = const ::SB1 *;

[erichkeane]

A few small comments, but comments on the function would be great. Code itself looks fine as best I can tell.