Stop evaluating trailing requires clause after overload resolution

[erichkeane]

Summary

Reported as it showed up as a constriants failure after the deferred
instantiation patch, we were checking constraints TWICE after overload
resolution. The first is during overload resolution, the second is when
diagnosing a use.

This patch modifies DiagnoseUseOfDecl to skip the trailing requires
clause check in some cases. First, of course, after choosing a candidate
after overload resolution.

The second is when evaluating a shadow using constructor, which had its
constraints checked when picking a constructor (as this is ALWAYS an
overload situation!).

[shafik]

clang/lib/Sema/SemaExpr.cpp

6780

This code is commented out, did you mean to remove it? Since this is the only code using ConstraintFailLoc then that should go to if you delete this code.

clang/test/SemaTemplate/concepts.cpp

712

[erichkeane]

Thanks for the review @shafik

[ChuanqiXu]

LGTM basically.

clang/lib/Sema/SemaExpr.cpp

6748

It looks like ConstraintFailLoc is not used?

[erichkeane]

clang/lib/Sema/SemaExpr.cpp

6748

Woops! Thanks for the catch! I'll remove it. It was a part of a previous implementation.

6780

Woops! Yes, thank you!

[ChuanqiXu]

LGTM then.

clang/include/clang/Sema/Sema.h

public:
   void DiagnoseUnguardedAvailabilityViolations(Decl *FD);
 
   void handleDelayedAvailabilityCheck(sema::DelayedDiagnostic &DD, Decl *Ctx);
 
   //===--------------------------------------------------------------------===//
   // Expression Parsing Callbacks: SemaExpr.cpp.
 
   bool CanUseDecl(NamedDecl *D, bool TreatUnavailableAsInvalid);
+  // A version of DiagnoseUseOfDecl that should be used if overload resolution
+  // has been used to find this declaration, which means we don't have to bother
+  // checking the trailing requires clause.
+  bool DiagnoseUseOfOverloadedDecl(NamedDecl *D, SourceLocation Loc) {
+    return DiagnoseUseOfDecl(
+        D, Loc, /*UnknownObjCClass=*/nullptr, /*ObjCPropertyAccess=*/false,
+        /*AvoidPartialAvailabilityChecks=*/false, /*ClassReceiver=*/nullptr,
+        /*SkipTrailingRequiresClause=*/true);
+  }
+
   bool DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
                          const ObjCInterfaceDecl *UnknownObjCClass = nullptr,
                          bool ObjCPropertyAccess = false,
                          bool AvoidPartialAvailabilityChecks = false,
-                         ObjCInterfaceDecl *ClassReciever = nullptr);
+                         ObjCInterfaceDecl *ClassReciever = nullptr,
+                         bool SkipTrailingRequiresClause = false);
   void NoteDeletedFunction(FunctionDecl *FD);
   void NoteDeletedInheritingConstructor(CXXConstructorDecl *CD);
   bool DiagnosePropertyAccessorMismatch(ObjCPropertyDecl *PD,
                                         ObjCMethodDecl *Getter,
                                         SourceLocation Loc);
   void DiagnoseSentinelCalls(NamedDecl *D, SourceLocation Loc,
                              ArrayRef<Expr *> Args);
 

clang/lib/Sema/SemaDeclCXX.cpp

Sema::BuildCXXConstructExpr(SourceLocation ConstructLoc, QualType DeclInitType,
                             bool HadMultipleCandidates,
                             bool IsListInitialization,
                             bool IsStdInitListInitialization,
                             bool RequiresZeroInit,
                             unsigned ConstructKind,
                             SourceRange ParenRange) {
   if (auto *Shadow = dyn_cast<ConstructorUsingShadowDecl>(FoundDecl)) {
     Constructor = findInheritingConstructor(ConstructLoc, Constructor, Shadow);
-    if (DiagnoseUseOfDecl(Constructor, ConstructLoc))
+    // The only way to get here is if we did overlaod resolution to find the
+    // shadow decl, so we don't need to worry about re-checking the trailing
+    // requires clause.
+    if (DiagnoseUseOfOverloadedDecl(Constructor, ConstructLoc))
       return ExprError();
   }
 
   return BuildCXXConstructExpr(
       ConstructLoc, DeclInitType, Constructor, Elidable, ExprArgs,
       HadMultipleCandidates, IsListInitialization, IsStdInitListInitialization,
       RequiresZeroInit, ConstructKind, ParenRange);
 }

clang/lib/Sema/SemaExpr.cpp

 ///
 /// \returns true if there was an error (this declaration cannot be
 /// referenced), false otherwise.
 ///
 bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
                              const ObjCInterfaceDecl *UnknownObjCClass,
                              bool ObjCPropertyAccess,
                              bool AvoidPartialAvailabilityChecks,
-                             ObjCInterfaceDecl *ClassReceiver) {
+                             ObjCInterfaceDecl *ClassReceiver,
+                             bool SkipTrailingRequiresClause) {
   SourceLocation Loc = Locs.front();
   if (getLangOpts().CPlusPlus && isa<FunctionDecl>(D)) {
     // If there were any diagnostics suppressed by template argument deduction,
     // emit them now.
     auto Pos = SuppressedDiagnostics.find(D->getCanonicalDecl());
     if (Pos != SuppressedDiagnostics.end()) {
       for (const PartialDiagnosticAt &Suppressed : Pos->second)
         Diag(Suppressed.first, Suppressed.second);
if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
     // [expr.prim.id]p4
     //   A program that refers explicitly or implicitly to a function with a
     //   trailing requires-clause whose constraint-expression is not satisfied,
     //   other than to declare it, is ill-formed. [...]
     //
     // See if this is a function with constraints that need to be satisfied.
     // Check this before deducing the return type, as it might instantiate the
     // definition.
-    if (FD->getTrailingRequiresClause()) {
+    if (!SkipTrailingRequiresClause && FD->getTrailingRequiresClause()) {
       ConstraintSatisfaction Satisfaction;
       if (CheckFunctionConstraints(FD, Satisfaction, Loc,
                                    /*ForOverloadResolution*/ true))
         // A diagnostic will have already been generated (non-constant
         // constraint expression, for example)
         return true;
       if (!Satisfaction.IsSatisfied) {
         Diag(Loc,
ExprResult Sema::BuildCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
   NamedDecl *NDecl = nullptr;
   if (UnaryOperator *UnOp = dyn_cast<UnaryOperator>(NakedFn)) {
     if (UnOp->getOpcode() == UO_AddrOf) {
       CallingNDeclIndirectly = true;
       NakedFn = UnOp->getSubExpr()->IgnoreParens();
     }
   }
 
   if (auto *DRE = dyn_cast<DeclRefExpr>(NakedFn)) {
     NDecl = DRE->getDecl();
 
     FunctionDecl *FDecl = dyn_cast<FunctionDecl>(NDecl);
     if (FDecl && FDecl->getBuiltinID()) {
       // Rewrite the function decl for this builtin by replacing parameters
       // with no explicit address space with the address space of the arguments
       // in ArgExprs.
       if ((FDecl =
                rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs))) {
         NDecl = FDecl;
         Fn = DeclRefExpr::Create(
             Context, FDecl->getQualifierLoc(), SourceLocation(), FDecl, false,
             SourceLocation(), FDecl->getType(), Fn->getValueKind(), FDecl,
             nullptr, DRE->isNonOdrUse());
       }
     }
-  } else if (isa<MemberExpr>(NakedFn))
-    NDecl = cast<MemberExpr>(NakedFn)->getMemberDecl();
+  } else if (auto *ME = dyn_cast<MemberExpr>(NakedFn))
+    NDecl = ME->getMemberDecl();
 
   if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(NDecl)) {
     if (CallingNDeclIndirectly && !checkAddressOfFunctionIsAvailable(
                                       FD, /*Complain=*/true, Fn->getBeginLoc()))
       return ExprError();
 
     checkDirectCallValidity(*this, Fn, FD, ArgExprs);
 
     // If this expression is a call to a builtin function in HIP device
     // compilation, allow a pointer-type argument to default address space to be
     // passed as a pointer-type parameter to a non-default address space.
     // If Arg is declared in the default address space and Param is declared
     // in a non-default address space, perform an implicit address space cast to
     // the parameter type.
     if (getLangOpts().HIP && getLangOpts().CUDAIsDevice && FD &&
         FD->getBuiltinID()) {
       for (unsigned Idx = 0; Idx < FD->param_size(); ++Idx) {
         ParmVarDecl *Param = FD->getParamDecl(Idx);
         if (!ArgExprs[Idx] || !Param || !Param->getType()->isPointerType() ||
             !ArgExprs[Idx]->getType()->isPointerType())
           continue;
 

clang/lib/Sema/SemaOverload.cpp

if (isa<MemberExpr>(NakedMemExpr)) {
     OverloadCandidateSet::iterator Best;
     bool Succeeded = false;
     switch (CandidateSet.BestViableFunction(*this, UnresExpr->getBeginLoc(),
                                             Best)) {
     case OR_Success:
       Method = cast<CXXMethodDecl>(Best->Function);
       FoundDecl = Best->FoundDecl;
       CheckUnresolvedMemberAccess(UnresExpr, Best->FoundDecl);
-      if (DiagnoseUseOfDecl(Best->FoundDecl, UnresExpr->getNameLoc()))
+      if (DiagnoseUseOfOverloadedDecl(Best->FoundDecl, UnresExpr->getNameLoc()))
         break;
       // If FoundDecl is different from Method (such as if one is a template
       // and the other a specialization), make sure DiagnoseUseOfDecl is
       // called on both.
       // FIXME: This would be more comprehensively addressed by modifying
       // DiagnoseUseOfDecl to accept both the FoundDecl and the decl
       // being used.
       if (Method != FoundDecl.getDecl() &&
-                      DiagnoseUseOfDecl(Method, UnresExpr->getNameLoc()))
+          DiagnoseUseOfOverloadedDecl(Method, UnresExpr->getNameLoc()))
         break;
       Succeeded = true;
       break;
 
     case OR_No_Viable_Function:
       CandidateSet.NoteCandidates(
           PartialDiagnosticAt(
               UnresExpr->getMemberLoc(),

clang/test/SemaTemplate/concepts.cpp

template <long Size> struct Container {
   using var_templ = int;
 };
 Container<4>::var_templ<int> inst;
 Container<5>::var_templ<int> inst_fail;
 // expected-error@-1{{constraints not satisfied for alias template 'var_templ'}}
 // expected-note@#CMVT_REQ{{because 'sizeof(int) == arity' (4 == 5) evaluated to false}}
 } // namespace ConstrainedMemberVarTemplate 
 
+// These should not diagnose, where we were unintentionally doing so before by
+// checking trailing requires clause twice, yet not having the ability to the
+// 2nd time, since it was no longer a dependent variant.
+namespace InheritedFromPartialSpec {
+template<class C>
+constexpr bool Check = true;
+
+template<typename T>
+struct Foo {
+  template<typename U>
+    Foo(U&&) requires (Check<U>){}
+  template<typename U>
+    void MemFunc(U&&) requires (Check<U>){}
+  template<typename U>
+    static void StaticMemFunc(U&&) requires (Check<U>){}
+  ~Foo() requires (Check<T>){}
+};
+
+template<>
+  struct Foo<void> : Foo<int> {
+    using Foo<int>::Foo;
+    using Foo<int>::MemFunc;
+    using Foo<int>::StaticMemFunc;
+  };
+
+void use() {
+  Foo<void> F {1.1};
+  F.MemFunc(1.1);
+  Foo<void>::StaticMemFunc(1.1);
+}
+
+template<typename T>
+struct counted_iterator {
+  constexpr auto operator->() const noexcept requires false {
+    return T::Invalid;
+  };
+};
+
+template<class _Ip>
+concept __has_member_pointer = requires { typename _Ip::pointer; };
+
+template<class>
+struct __iterator_traits_member_pointer_or_arrow_or_void { using type = void; };
+template<__has_member_pointer _Ip>
+struct __iterator_traits_member_pointer_or_arrow_or_void<_Ip> { using type = typename _Ip::pointer; };
+
+template<class _Ip>
+  requires requires(_Ip& __i) { __i.operator->(); } && (!__has_member_pointer<_Ip>)
+struct __iterator_traits_member_pointer_or_arrow_or_void<_Ip> {
+  using type = decltype(declval<_Ip&>().operator->());
+};
+
+
+void use2() {
+  __iterator_traits_member_pointer_or_arrow_or_void<counted_iterator<int>> f;
+}
+}// namespace InheritedFromPartialSpec