Don't track consteval references for dependent members

[wchilders]

Summary

Previously dependent references to consteval (ReferenceToConsteval) were tracked, and dependent expressions were evaluated as possible immediate invocations.

This resulted in the evaluation of value dependent expressions.

This patch also suppresses duplicated diagnostics in debug builds while working with templates caused by the side effects of FixOverloadedFunctionReference.

[Tyker]

thanks for this,

here is a few comments aside from that this seems fine.

clang/lib/Sema/SemaOverload.cpp

1761

I don't think RebuildingImmediateInvocation should be used here since we are not rebuilding immediate invocations

setSuppressAllDiagnostics or TentativeAnalysisScope seems more adapted.

clang/test/SemaCXX/cxx2a-consteval.cpp

131

could you check that this diagnostic and those like it doesn't appear when the struct A isn't instantiated.

[wchilders]

clang/lib/Sema/SemaOverload.cpp

1761

These don't actually work for this use case due to the queuing nature of the diagnostic. Given the hacky nature of this assertion as it stands, I didn't want to further pollute the code base.

I'm open to suggestions. If you're proposing making MarkDeclRefReferenced not queue in response to diagnostic suppression, that doesn't seem totally unreasonable; we'll just have to add the additional condition to check the diagnostic state in MarkDeclRefReferenced.

clang/test/SemaCXX/cxx2a-consteval.cpp

131

I duplicated this namespace without any of the instantiation, and it does seem to trigger the diagnostic "destructor cannot be declared consteval" -- all other diagnostics are silent. Is that particularly undesirable?

Also, WRT testing. would that the best option here (having a duplicated namespace, "dependent_addressing_uninstantiated" -- without the line triggering instantiation)?

clang/lib/Sema/SemaExpr.cpp

if (BO->getOpcode() == BO_Assign) {
       auto &LHSs = ExprEvalContexts.back().VolatileAssignmentLHSs;
       LHSs.erase(std::remove(LHSs.begin(), LHSs.end(), BO->getLHS()),
                  LHSs.end());
     }
   }
 }
 
 ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) {
+  // Skip checking dependent expressions - If we allow dependent expressions
+  // we end up evaluating dependent constant expressions.
   if (!E.isUsable() || !Decl || !Decl->isConsteval() || isConstantEvaluated() ||
-      RebuildingImmediateInvocation)
+      E.get()->getDependence() || RebuildingImmediateInvocation)
     return E;
 
-  /// Opportunistically remove the callee from ReferencesToConsteval if we can.
+  /// Opportunistically remove the callee from ReferenceToConsteval if we can.
   /// It's OK if this fails; we'll also remove this in
   /// HandleImmediateInvocations, but catching it here allows us to avoid
   /// walking the AST looking for it in simple cases.
   if (auto *Call = dyn_cast<CallExpr>(E.get()->IgnoreImplicit()))
     if (auto *DeclRef =
             dyn_cast<DeclRefExpr>(Call->getCallee()->IgnoreImplicit()))
       ExprEvalContexts.back().ReferenceToConsteval.erase(DeclRef);
 
void Sema::MarkDeclRefReferenced(DeclRefExpr *E, const Expr *Base) {
   // C++11 defect. The address of a pure member should not be an ODR use, even
   // if it's a qualified reference.
   bool OdrUse = true;
   if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(E->getDecl()))
     if (Method->isVirtual() &&
         !Method->getDevirtualizedMethod(Base, getLangOpts().AppleKext))
       OdrUse = false;
 
-  if (auto *FD = dyn_cast<FunctionDecl>(E->getDecl()))
-    if (!isConstantEvaluated() && FD->isConsteval() &&
-        !RebuildingImmediateInvocation)
+  if (auto *FD = dyn_cast<FunctionDecl>(E->getDecl())) {
+    // Skip checking dependent contexts - If we attempt to check
+    // dependent contexts here, we end up increasing the guarantee on
+    // CheckForImmediateInvocation's opportunistic removal from
+    // ReferenceToConsteval to guaranteed removal -- to prevent
+    // incorrect diagnostics. This would require an additional AST
+    // walk to ensure the reference was removed.
+    if (FD->isConsteval() && !isConstantEvaluated() &&
+        !CurContext->isDependentContext() && !RebuildingImmediateInvocation)
       ExprEvalContexts.back().ReferenceToConsteval.insert(E);
+  }
+
   MarkExprReferenced(*this, E->getLocation(), E->getDecl(), E, OdrUse);
 }
 
 /// Perform reference-marking and odr-use handling for a MemberExpr.
 void Sema::MarkMemberReferenced(MemberExpr *E) {
   // C++11 [basic.def.odr]p2:
   //   A non-overloaded function whose name appears as a potentially-evaluated
   //   expression or a member of a set of candidate functions, if selected by

clang/lib/Sema/SemaOverload.cpp

 #include "clang/Sema/SemaInternal.h"
 #include "clang/Sema/Template.h"
 #include "clang/Sema/TemplateDeduction.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/SaveAndRestore.h"
 #include <algorithm>
 #include <cstdlib>
 
 using namespace clang;
 using namespace sema;
 
 using AllowedExplicit = Sema::AllowedExplicit;
 
if (FunctionDecl *Fn
       } else if (isa<UnaryOperator>(From->IgnoreParens())) {
         assert(cast<UnaryOperator>(From->IgnoreParens())->getOpcode() ==
                UO_AddrOf &&
                "Non-address-of operator for overloaded function expression");
         FromType = S.Context.getPointerType(FromType);
       }
 
       // Check that we've computed the proper type after overload resolution.
+#ifndef NDEBUG
       // FIXME: FixOverloadedFunctionReference has side-effects; we shouldn't
       // be calling it from within an NDEBUG block.
+
+      // This is a hack to prevent duplicated diagnostics triggered by
+      // the above side effects in some cases involving immediate invocations.
+      llvm::SaveAndRestore<bool> DisableIITracking(
+          S.RebuildingImmediateInvocation, true);
+
       assert(S.Context.hasSameType(
         FromType,
         S.FixOverloadedFunctionReference(From, AccessPair, Fn)->getType()));
+#endif
     } else {
       return false;
     }
   }
   // Lvalue-to-rvalue conversion (C++11 4.1):
   //   A glvalue (3.10) of a non-function, non-array type T can
   //   be converted to a prvalue.
   bool argIsLValue = From->isGLValue();

clang/test/SemaCXX/cxx2a-consteval.cpp

 
 auto f1() {
   return &f;
 // expected-error@-1 {{take address}}
 }
 
 }
 
+namespace dependent_addressing {
+
+template<int y>
+consteval int t_f_eval(int i) {
+// expected-note@-1+ {{declared here}}
+  return i;
+}
+
+using func_type = int(int);
+
+template<int y>
+auto t_f_eval_proxy() {
+  return &t_f_eval<y>;
+// expected-error@-1 {{take address}}
+}
+
+template<int y>
+struct A {
+  consteval int f1(int i) const {
+// expected-note@-1 {{declared here}}
+    return i + y;
+  }
+  consteval A(int i);
+  consteval A() = default;
+  consteval ~A() = default; // expected-error {{destructor cannot be declared consteval}}
+};
+
+template<int y>
+constexpr auto l_eval = [](int i) consteval {
+// expected-note@-1+ {{declared here}}
+
+  return i + y;
+};
+
+template<int y>
+int test_templ() {
+  func_type* p1 = (&t_f_eval<y>);
+  // expected-error@-1+ {{cannot take address of consteval function 't_f_eval<1>' outside of an immediate invocation}}
+
+  auto ptr = &t_f_eval<y>;
+  // expected-error@-1 {{take address}}
+
+  // FIXME: AddressOfFunctionResolver breaks
+  // func_type* p7 = __builtin_addressof(t_f_eval<y>);
+
+  auto p = t_f_eval<y>;
+  // expected-error@-1 {{take address}}
+
+  auto m1 = &A<y>::f1;
+  // expected-error@-1 {{take address}}
+  auto l1 = &decltype(l_eval<y>)::operator();
+  // expected-error@-1 {{take address}}
+
+  auto pr = t_f_eval_proxy<y>();
+  // expected-note@-1 {{in instantiation of function template specialization}}
+
+  return 0;
+}
+
+auto tr = test_templ<1>();
+// expected-note@-1+ {{in instantiation of function template specialization}}
+
+} // namespace dependent_addressing
+
+namespace dependent_call {
+
+consteval int cf1(int y) {
+  return y;
+}
+
+template<int y>
+auto f1() {
+  constexpr int x = cf1(y);
+  return x;
+}
+
+auto f1r = f1<1>();
+
+} // namespace dependent_call
+
+
 namespace invalid_function {
 
 struct A {
   consteval void *operator new(size_t count);
   // expected-error@-1 {{'operator new' cannot be declared consteval}}
   consteval void *operator new[](size_t count);
   // expected-error@-1 {{'operator new[]' cannot be declared consteval}}
   consteval void operator delete(void* ptr);
void test() {
   { Copy* c; c = new Copy(Copy(&f_eval)); }// expected-error {{cannot take address of consteval}}
   { Copy* c; c = new Copy(std::move(C)); }
   { Copy* c; c = new Copy(std::move(Copy(&f_eval))); }// expected-error {{is not a constant expression}} expected-note {{to a consteval}}
   { Copy* c; c = new Copy(to_lvalue_ref(Copy(&f_eval))); }// expected-error {{is not a constant expression}} expected-note {{to a consteval}}
   { Copy* c; c = new Copy(to_lvalue_ref(std::move(C))); }
   { Copy* c; c = new Copy(to_lvalue_ref(std::move(Copy(&f_eval)))); }// expected-error {{is not a constant expression}} expected-note {{to a consteval}}
 }
 
-} // namespace special_ctor
+} // namespace copy_ctor
+