[c++20] Fix incorrect assumptions in checks for comparison category types.

In the presence of modules, we can have multiple lookup results for the
same entity, and we need to re-check for completeness each time we
consider a type.

Author: zygoloid

clang/lib/AST/ComparisonCategories.cpp

@@ -59,7 +59,7 @@ ComparisonCategoryInfo::ValueInfo *ComparisonCategoryInfo::lookupValueInfo(
   // a new entry representing it.
   DeclContextLookupResult Lookup = Record->getCanonicalDecl()->lookup(
       &Ctx.Idents.get(ComparisonCategories::getResultString(ValueKind)));
-  if (Lookup.size() != 1 || !isa<VarDecl>(Lookup.front()))
+  if (Lookup.empty() || !isa<VarDecl>(Lookup.front()))
     return nullptr;
   Objects.emplace_back(ValueKind, cast<VarDecl>(Lookup.front()));
   return &Objects.back();
@@ -70,7 +70,7 @@ static const NamespaceDecl *lookupStdNamespace(const ASTContext &Ctx,
   if (!StdNS) {
     DeclContextLookupResult Lookup =
         Ctx.getTranslationUnitDecl()->lookup(&Ctx.Idents.get("std"));
-    if (Lookup.size() == 1)
+    if (!Lookup.empty())
       StdNS = dyn_cast<NamespaceDecl>(Lookup.front());
   }
   return StdNS;
@@ -81,7 +81,7 @@ static CXXRecordDecl *lookupCXXRecordDecl(const ASTContext &Ctx,
                                           ComparisonCategoryType Kind) {
   StringRef Name = ComparisonCategories::getCategoryString(Kind);
   DeclContextLookupResult Lookup = StdNS->lookup(&Ctx.Idents.get(Name));
-  if (Lookup.size() == 1)
+  if (!Lookup.empty())
     if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(Lookup.front()))
       return RD;
   return nullptr;

clang/lib/Sema/SemaDeclCXX.cpp

@@ -7501,8 +7501,7 @@ class DefaultedComparisonSynthesizer
       VarDecl *EqualVD = S.Context.CompCategories.getInfoForType(StrongOrdering)
                              .getValueInfo(ComparisonCategoryResult::Equal)
                              ->VD;
-      RetVal = S.BuildDeclarationNameExpr(
-          CXXScopeSpec(), DeclarationNameInfo(), EqualVD);
+      RetVal = getDecl(EqualVD);
       if (RetVal.isInvalid())
         return StmtError();
       RetVal = buildStaticCastToR(RetVal.get());
@@ -7527,10 +7526,14 @@ class DefaultedComparisonSynthesizer
   }
 
 private:
+  ExprResult getDecl(ValueDecl *VD) {
+    return S.BuildDeclarationNameExpr(
+        CXXScopeSpec(), DeclarationNameInfo(VD->getDeclName(), Loc), VD);
+  }
+
   ExprResult getParam(unsigned I) {
     ParmVarDecl *PD = FD->getParamDecl(I);
-    return S.BuildDeclarationNameExpr(
-        CXXScopeSpec(), DeclarationNameInfo(PD->getDeclName(), Loc), PD);
+    return getDecl(PD);
   }
 
   ExprPair getCompleteObject() {
@@ -7622,8 +7625,7 @@ class DefaultedComparisonSynthesizer
       Stmt *InitStmt = new (S.Context) DeclStmt(DeclGroupRef(VD), Loc, Loc);
 
       // cmp != 0
-      ExprResult VDRef = S.BuildDeclarationNameExpr(
-          CXXScopeSpec(), DeclarationNameInfo(Name, Loc), VD);
+      ExprResult VDRef = getDecl(VD);
       if (VDRef.isInvalid())
         return StmtError();
       llvm::APInt ZeroVal(S.Context.getIntWidth(S.Context.IntTy), 0);
@@ -7639,8 +7641,7 @@ class DefaultedComparisonSynthesizer
         return StmtError();
 
       // return cmp;
-      VDRef = S.BuildDeclarationNameExpr(
-          CXXScopeSpec(), DeclarationNameInfo(Name, Loc), VD);
+      VDRef = getDecl(VD);
       if (VDRef.isInvalid())
         return StmtError();
       StmtResult ReturnStmt = S.BuildReturnStmt(Loc, VDRef.get());
@@ -10235,11 +10236,25 @@ QualType Sema::CheckComparisonCategoryType(ComparisonCategoryType Kind,
   assert(getLangOpts().CPlusPlus &&
          "Looking for comparison category type outside of C++.");
 
+  // Use an elaborated type for diagnostics which has a name containing the
+  // prepended 'std' namespace but not any inline namespace names.
+  auto TyForDiags = [&](ComparisonCategoryInfo *Info) {
+    auto *NNS =
+        NestedNameSpecifier::Create(Context, nullptr, getStdNamespace());
+    return Context.getElaboratedType(ETK_None, NNS, Info->getType());
+  };
+
   // Check if we've already successfully checked the comparison category type
   // before. If so, skip checking it again.
   ComparisonCategoryInfo *Info = Context.CompCategories.lookupInfo(Kind);
-  if (Info && FullyCheckedComparisonCategories[static_cast<unsigned>(Kind)])
+  if (Info && FullyCheckedComparisonCategories[static_cast<unsigned>(Kind)]) {
+    // The only thing we need to check is that the type has a reachable
+    // definition in the current context.
+    if (RequireCompleteType(Loc, TyForDiags(Info), diag::err_incomplete_type))
+      return QualType();
+
     return Info->getType();
+  }
 
   // If lookup failed
   if (!Info) {
@@ -10258,18 +10273,10 @@ QualType Sema::CheckComparisonCategoryType(ComparisonCategoryType Kind,
   if (Info->Record->hasDefinition())
     Info->Record = Info->Record->getDefinition();
 
-  // Use an elaborated type for diagnostics which has a name containing the
-  // prepended 'std' namespace but not any inline namespace names.
-  QualType TyForDiags = [&]() {
-    auto *NNS =
-        NestedNameSpecifier::Create(Context, nullptr, getStdNamespace());
-    return Context.getElaboratedType(ETK_None, NNS, Info->getType());
-  }();
-
-  if (RequireCompleteType(Loc, TyForDiags, diag::err_incomplete_type))
+  if (RequireCompleteType(Loc, TyForDiags(Info), diag::err_incomplete_type))
     return QualType();
 
-  InvalidSTLDiagnoser UnsupportedSTLError{*this, Loc, TyForDiags};
+  InvalidSTLDiagnoser UnsupportedSTLError{*this, Loc, TyForDiags(Info)};
 
   if (!Info->Record->isTriviallyCopyable())
     return UnsupportedSTLError(USS_NonTrivial);

clang/test/SemaCXX/compare-cxx2a.cpp

@@ -3,6 +3,9 @@
 
 // RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -fsyntax-only -pedantic -verify -Wsign-compare -std=c++2a %s
 
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -std=c++2a -x c++ %S/Inputs/std-compare.h -emit-pch -o %t.pch
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -fsyntax-only -pedantic -verify -Wsign-compare -std=c++2a %s -include-pch %t.pch
+
 #include "Inputs/std-compare.h"
 
 #define ASSERT_TYPE(...) static_assert(__is_same(__VA_ARGS__))

clang/test/SemaCXX/compare-modules-cxx2a.cpp

@@ -0,0 +1,54 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -fcxx-exceptions -verify -std=c++2a -fmodules -I%S/Inputs %s -fno-modules-error-recovery
+
+#pragma clang module build compare
+module compare {
+  explicit module cmp {}
+  explicit module other {}
+}
+#pragma clang module contents
+#pragma clang module begin compare.cmp
+#include "std-compare.h"
+#pragma clang module end
+#pragma clang module endbuild
+
+struct CC { CC(...); };
+
+void a() { void(0 <=> 0); } // expected-error {{include <compare>}}
+
+struct A {
+  CC operator<=>(const A&) const = default; // expected-error {{include <compare>}}
+};
+auto va = A() <=> A(); // expected-note {{required here}}
+
+#pragma clang module import compare.other
+
+// [email protected]:* 2+{{previous definition}}
+
+void b() { void(0 <=> 0); } // expected-error 1+{{definition of 'strong_ordering' must be imported}}
+
+struct B {
+  CC operator<=>(const B&) const = default; // expected-error 1+{{definition of 'strong_ordering' must be imported}}
+};
+auto vb = B() <=> B(); // expected-note {{required here}}
+
+#pragma clang module import compare.cmp
+
+void c() { void(0 <=> 0); }
+
+struct C {
+  CC operator<=>(const C&) const = default;
+};
+auto vc = C() <=> C();
+
+
+#pragma clang module build compare2
+module compare2 {}
+#pragma clang module contents
+#pragma clang module begin compare2
+#include "std-compare.h"
+#pragma clang module end
+#pragma clang module endbuild
+
+#pragma clang module import compare2
+
+void g() { void(0.0 <=> 0.0); }