[clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

[• Quuxplusone]

Summary

PR52905 was originally papered over in a different way, but I believe this is the actually proper fix, or at least closer to it. We need to detect placeholder types as close to the front-end as possible, and cause them to fail constraints, rather than letting them persist into later stages.

Fixes https://llvm.org/PR52905
Fixes https://llvm.org/PR52909
Fixes https://llvm.org/PR53075

[• Quuxplusone]

clang/lib/AST/ASTContext.cpp

3375

Btw, I strongly suspect that the presence of placeholder type UnknownAny all the way down in here is a bug (@rjmccall, thoughts?). But I found that the test suite didn't pass without this exception, and I'm not terribly interested in tracking down why; I don't have any idea what UnknownAny is, myself.

[rjmccall]

It's a type that the debugger integration uses when it doesn't know the type of a symbol. If it's a data symbol, you make a VarDecl with UnknownAny type; if it's a function symbol, you make a FunctionDecl with UnknownAny. In either case, if the user fails to cast it, it gets diagnosed as an error.

I can't think of any reason why we'd want to build a reference to UnknownAny type; generally it's bad if the type "escapes" into subordinate positions. Probably we should be diagnosing something before we try to propagate the UnknownAny type.

[• Quuxplusone]

clang/lib/AST/ASTContext.cpp

3375

In D118552#3282538, @rjmccall wrote:

It's a type that the debugger integration uses when it doesn't know the type of a symbol. If it's a data symbol, you make a VarDecl with UnknownAny type; if it's a function symbol, you make a FunctionDecl with UnknownAny. In either case, if the user fails to cast it, it gets diagnosed as an error.

I can't think of any reason why we'd want to build a reference to UnknownAny type; generally it's bad if the type "escapes" into subordinate positions. Probably we should be diagnosing something before we try to propagate the UnknownAny type.

I've found https://github.com/llvm/llvm-project/issues/32247 on the same topic, and updated it with a comment and my own test case.

[• Quuxplusone]

Poke CI (clang-format failed).

[• Quuxplusone]

Gentle ping! (@ChuanqiXu @urnathan perhaps?)
I'm hoping to land D118552 followed by a re-land of D117603, this week and ideally tomorrow. :)

[urnathan]

clang/lib/AST/ASTContext.cpp

3373

sorry to be picky, but
(a) why the {...} on the single body stmt? Isn't that against style?
(b) are we sure the if's condition is sufficiently const so that it goes away when the assert is inactive? if's containing a single assert make me nervous.

[• Quuxplusone]

clang/lib/AST/ASTContext.cpp

3373

No problem, I'll move the whole thing into the assert, which will fix both points. Ditto line 3414.

(I did it this way originally only for historical reasons, because I had been putting extra code (printf/abort) into the if to find out what placeholder types I needed to grandfather in.)

[• Quuxplusone]

Update assert style.

[• Quuxplusone]

Update another single-line if too.

[urnathan]

thanks!

clang/lib/AST/ASTContext.cpp

QualType ASTContext::getBlockPointerType(QualType T) const {
   BlockPointerTypes.InsertNode(New, InsertPos);
   return QualType(New, 0);
 }
 
 /// getLValueReferenceType - Return the uniqued reference to the type for an
 /// lvalue reference to the specified type.
 QualType
 ASTContext::getLValueReferenceType(QualType T, bool SpelledAsLValue) const {
-  assert(getCanonicalType(T) != OverloadTy &&
-         "Unresolved overloaded function type");
+  assert((!T->isPlaceholderType() ||
+          T->isSpecificPlaceholderType(BuiltinType::UnknownAny)) &&
+         "Unresolved placeholder type");
 
   // Unique pointers, to guarantee there is only one pointer of a particular
   // structure.
   llvm::FoldingSetNodeID ID;
   ReferenceType::Profile(ID, T, SpelledAsLValue);
 
   void *InsertPos = nullptr;
   if (LValueReferenceType *RT =
ASTContext::getLValueReferenceType(QualType T, bool SpelledAsLValue) const {
   LValueReferenceTypes.InsertNode(New, InsertPos);
 
   return QualType(New, 0);
 }
 
 /// getRValueReferenceType - Return the uniqued reference to the type for an
 /// rvalue reference to the specified type.
 QualType ASTContext::getRValueReferenceType(QualType T) const {
+  assert((!T->isPlaceholderType() ||
+          T->isSpecificPlaceholderType(BuiltinType::UnknownAny)) &&
+         "Unresolved placeholder type");
+
   // Unique pointers, to guarantee there is only one pointer of a particular
   // structure.
   llvm::FoldingSetNodeID ID;
   ReferenceType::Profile(ID, T, false);
 
   void *InsertPos = nullptr;
   if (RValueReferenceType *RT =
         RValueReferenceTypes.FindNodeOrInsertPos(ID, InsertPos))

clang/lib/Sema/SemaOverload.cpp

ExprResult Sema::BuildCallToMemberFunction(Scope *S, Expr *MemExprE,
   DeclAccessPair FoundDecl = DeclAccessPair::make(nullptr, AS_public);
   NestedNameSpecifier *Qualifier = nullptr;
   if (isa<MemberExpr>(NakedMemExpr)) {
     MemExpr = cast<MemberExpr>(NakedMemExpr);
     Method = cast<CXXMethodDecl>(MemExpr->getMemberDecl());
     FoundDecl = MemExpr->getFoundDecl();
     Qualifier = MemExpr->getQualifier();
     UnbridgedCasts.restore();
-  } else if (auto *UnresExpr = dyn_cast<UnresolvedMemberExpr>(NakedMemExpr)) {
+  } else {
+    UnresolvedMemberExpr *UnresExpr = cast<UnresolvedMemberExpr>(NakedMemExpr);
     Qualifier = UnresExpr->getQualifier();
 
     QualType ObjectType = UnresExpr->getBaseType();
     Expr::Classification ObjectClassification
       = UnresExpr->isArrow()? Expr::Classification::makeSimpleLValue()
                             : UnresExpr->getBase()->Classify(Context);
 
     // Add overload candidates
if (isa<MemberExpr>(NakedMemExpr)) {
     // If overload resolution picked a static member, build a
     // non-member call based on that function.
     if (Method->isStatic()) {
       return BuildResolvedCallExpr(MemExprE, Method, LParenLoc, Args, RParenLoc,
                                    ExecConfig, IsExecConfig);
     }
 
     MemExpr = cast<MemberExpr>(MemExprE->IgnoreParens());
-  } else
-    // Unimaged NakedMemExpr type.
-    return ExprError();
+  }
 
   QualType ResultType = Method->getReturnType();
   ExprValueKind VK = Expr::getValueKindForType(ResultType);
   ResultType = ResultType.getNonLValueExprType(Context);
 
   assert(Method && "Member call to something that isn't a method?");
   const auto *Proto = Method->getType()->castAs<FunctionProtoType>();
   CXXMemberCallExpr *TheCall = CXXMemberCallExpr::Create(

clang/lib/Sema/SemaTemplateInstantiate.cpp

TemplateInstantiator::TransformExprRequirement(concepts::ExprRequirement *Req) {
   else {
     Expr *E = Req->getExpr();
     TemplateDeductionInfo Info(E->getBeginLoc());
     Sema::InstantiatingTemplate ExprInst(SemaRef, E->getBeginLoc(), Req, Info,
                                          E->getSourceRange());
     if (ExprInst.isInvalid())
       return nullptr;
     ExprResult TransExprRes = TransformExpr(E);
+    if (!TransExprRes.isInvalid() && !Trap.hasErrorOccurred() &&
+        TransExprRes.get()->hasPlaceholderType())
+      TransExprRes = SemaRef.CheckPlaceholderExpr(TransExprRes.get());
     if (TransExprRes.isInvalid() || Trap.hasErrorOccurred())
       TransExpr = createSubstDiag(SemaRef, Info, [&](llvm::raw_ostream &OS) {
         E->printPretty(OS, nullptr, SemaRef.getPrintingPolicy());
       });
     else
       TransExpr = TransExprRes.get();
   }
 

clang/lib/Sema/TreeTransform.h

 template<typename Derived>
 concepts::ExprRequirement *
 TreeTransform<Derived>::TransformExprRequirement(concepts::ExprRequirement *Req) {
   llvm::PointerUnion<Expr *, concepts::Requirement::SubstitutionDiagnostic *> TransExpr;
   if (Req->isExprSubstitutionFailure())
     TransExpr = Req->getExprSubstitutionDiagnostic();
   else {
     ExprResult TransExprRes = getDerived().TransformExpr(Req->getExpr());
+    if (TransExprRes.isUsable() && TransExprRes.get()->hasPlaceholderType())
+      TransExprRes = SemaRef.CheckPlaceholderExpr(TransExprRes.get());
     if (TransExprRes.isInvalid())
       return nullptr;
     TransExpr = TransExprRes.get();
   }
 
   llvm::Optional<concepts::ExprRequirement::ReturnTypeRequirement> TransRetReq;
   const auto &RetReq = Req->getReturnTypeRequirement();
   if (RetReq.isEmpty())

clang/test/SemaTemplate/constraints.cpp

namespace PR45589 {
   static_assert(test<true, False> == 2);
   static_assert(test<true, True> == 2);
   static_assert(test<true, char> == 2); // satisfaction of second term of || not considered
   static_assert(test<false, False> == 1);
   static_assert(test<false, True> == 2); // constraints are partially ordered
   // FIXME: These diagnostics are excessive.
   static_assert(test<false, char> == 1); // expected-note 2{{while}} expected-note 2{{during}}
 }
-
-namespace PR52905 {
-// A mock for std::convertible_to. Not complete support.
-template <typename _From, typename _To>
-concept convertible_to = __is_convertible_to(_From, _To); // expected-note {{evaluated to false}}
-
-template <typename T>
-class A {
-public:
-  using iterator = void **;
-
-  iterator begin();
-  const iterator begin() const;
-};
-
-template <class T>
-concept Beginable1 = requires(T t) {
-  { t.begin }
-  ->convertible_to<typename T::iterator>; // expected-note {{not satisfied}}
-};
-
-static_assert(Beginable1<A<int>>); // expected-error {{static_assert failed}}
-                                   // expected-note@-1 {{does not satisfy 'Beginable1'}}
-
-template <class T>
-concept Beginable2 = requires(T t) {
-  { t.begin() }
-  ->convertible_to<typename T::iterator>;
-};
-
-static_assert(Beginable2<A<int>>);
-} // namespace PR52905

clang/test/SemaTemplate/pr52909.cpp

+// RUN: %clang_cc1 -std=c++20 -verify %s
+// RUN: %clang_cc1 -std=c++2b -verify %s
+
+namespace PR52905 {
+template <class> concept C = true;
+
+struct A {
+  int begin();
+  int begin() const;
+};
+
+template <class T>
+concept Beginable = requires (T t) {
+  { t.begin } -> C;
+  // expected-note@-1 {{because 't.begin' would be invalid: reference to non-static member function must be called}}
+};
+
+static_assert(Beginable<A>); // expected-error {{static_assert failed}}
+                             // expected-note@-1 {{does not satisfy 'Beginable'}}
+} // namespace PR52905
+
+namespace PR52909a {
+
+template<class> constexpr bool B = true;
+template<class T> concept True = B<T>;
+
+template <class T>
+int foo(T t) requires requires { // expected-note {{candidate template ignored: constraints not satisfied}}
+    {t.begin} -> True; // expected-note {{because 't.begin' would be invalid: reference to non-static member function must be called}}
+}
+{}
+
+struct A { int begin(); };
+auto x = foo(A()); // expected-error {{no matching function for call to 'foo'}}
+
+} // namespace PR52909a
+
+namespace PR52909b {
+
+template<class> concept True = true;
+
+template<class T> concept C = requires {
+    { T::begin } -> True; // expected-note {{because 'T::begin' would be invalid: reference to overloaded function could not be resolved}}
+};
+
+struct A {
+    static void begin(int);
+    static void begin(double);
+};
+
+static_assert(C<A>); // expected-error {{static_assert failed}}
+  // expected-note@-1 {{because 'PR52909b::A' does not satisfy 'C'}}
+
+} // namespace PR52909b
+
+namespace PR53075 {
+template<class> concept True = true;
+
+template<class T> concept C = requires {
+    { &T::f } -> True; // expected-note {{because '&T::f' would be invalid: reference to overloaded function could not be resolved}}
+};
+
+struct S {
+    int *f();
+    int *f() const;
+};
+
+static_assert(C<S>); // expected-error {{static_assert failed}}
+  // expected-note@-1 {{because 'PR53075::S' does not satisfy 'C'}}
+
+} // namespace PR53075