[clang] Improve constexpr-unknown diagnostics.

[efriedma-quic]

APValue::ConstexprUnknown() constructs a broken LValue that doesn't have an lvalue path, which confuses later error handling. It turns out we don't actually use the result of createConstexprUnknownAPValues for anything, so just stop using it. Just construct the LValue directly when we need it.

Make findCompleteObject emit errors more aggressively; allowing it to succeed for constexpr-unknown objects leads to weird states where it succeeds, but doesn't return a well-formed object.

Delete the check for constexpr-unknown in dynamic_cast handling: it's not necessary, and breaks with the other changes in this patch.

These changes allow us to produce proper diagnostics when something fails to be evaluated, instead of just printing a generic top-level error without any notes.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Eli Friedman (efriedma-quic)

Changes

APValue::ConstexprUnknown() constructs a broken LValue that doesn't have an lvalue path, which confuses later error handling. It turns out we don't actually use the result of createConstexprUnknownAPValues for anything, so just stop using it. Just construct the LValue directly when we need it.

Make findCompleteObject emit errors more aggressively; allowing it to succeed for constexpr-unknown objects leads to weird states where it succeeds, but doesn't return a well-formed object.

Delete the check for constexpr-unknown in dynamic_cast handling: it's not necessary, and breaks with the other changes in this patch.

These changes allow us to produce proper diagnostics when something fails to be evaluated, instead of just printing a generic top-level error without any notes.

---

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

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticASTKinds.td (+2)
• (modified) clang/lib/AST/ExprConstant.cpp (+22-37)
• (modified) clang/test/SemaCXX/constant-expression-cxx11.cpp (+3-3)
• (modified) clang/test/SemaCXX/constant-expression-p2280r4.cpp (+52-6)

diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index d2cd86d05d55a..2e2a9597ccb5f 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -240,6 +240,8 @@ def note_constexpr_access_static_temporary : Note<
   "outside the expression that created the temporary">;
 def note_constexpr_access_unreadable_object : Note<
   "%sub{access_kind}0 object '%1' whose value is not known">;
+def note_constexpr_access_unknown_variable : Note<
+  "%sub{access_kind}0 variable %1 whose value is not known">;
 def note_constexpr_access_deleted_object : Note<
   "%sub{access_kind}0 heap allocated object that has been deleted">;
 def note_constexpr_modify_global : Note<
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index bf9208763b1ab..f9453658580c9 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -202,7 +202,9 @@ namespace {
     assert(!isBaseAnAllocSizeCall(Base) &&
            "Unsized arrays shouldn't appear here");
     unsigned MostDerivedLength = 0;
-    Type = getType(Base);
+    // The type of Base is a reference type if the base is a constexpr-unknown
+    // variable. In that case, look through the reference type.
+    Type = getType(Base).getNonReferenceType();
 
     for (unsigned I = 0, N = Path.size(); I != N; ++I) {
       if (Type->isArrayType()) {
@@ -571,7 +573,6 @@ namespace {
     typedef std::map<MapKeyTy, APValue> MapTy;
     /// Temporaries - Temporary lvalues materialized within this stack frame.
     MapTy Temporaries;
-    MapTy ConstexprUnknownAPValues;
 
     /// CallRange - The source range of the call expression for this call.
     SourceRange CallRange;
@@ -646,9 +647,6 @@ namespace {
     APValue &createTemporary(const KeyT *Key, QualType T,
                              ScopeKind Scope, LValue &LV);
 
-    APValue &createConstexprUnknownAPValues(const VarDecl *Key,
-                                            APValue::LValueBase Base);
-
     /// Allocate storage for a parameter of a function call made in this frame.
     APValue &createParam(CallRef Args, const ParmVarDecl *PVD, LValue &LV);
 
@@ -1955,15 +1953,6 @@ APValue &CallStackFrame::createTemporary(const KeyT *Key, QualType T,
   return createLocal(Base, Key, T, Scope);
 }
 
-APValue &
-CallStackFrame::createConstexprUnknownAPValues(const VarDecl *Key,
-                                               APValue::LValueBase Base) {
-  APValue &Result = ConstexprUnknownAPValues[MapKeyTy(Key, Base.getVersion())];
-  Result = APValue(Base, CharUnits::Zero(), APValue::ConstexprUnknown{});
-
-  return Result;
-}
-
 /// Allocate storage for a parameter of a function call made in this frame.
 APValue &CallStackFrame::createParam(CallRef Args, const ParmVarDecl *PVD,
                                      LValue &LV) {
@@ -3493,7 +3482,7 @@ static bool evaluateVarDeclInit(EvalInfo &Info, const Expr *E,
   APValue::LValueBase Base(VD, Frame ? Frame->Index : 0, Version);
 
   auto CheckUninitReference = [&](bool IsLocalVariable) {
-    if (!Result->hasValue() && VD->getType()->isReferenceType()) {
+    if (!Result || (!Result->hasValue() && VD->getType()->isReferenceType())) {
       // C++23 [expr.const]p8
       // ... For such an object that is not usable in constant expressions, the
       // dynamic type of the object is constexpr-unknown. For such a reference
@@ -3509,7 +3498,7 @@ static bool evaluateVarDeclInit(EvalInfo &Info, const Expr *E,
           Info.FFDiag(E, diag::note_constexpr_use_uninit_reference);
         return false;
       }
-      Result = &Info.CurrentCall->createConstexprUnknownAPValues(VD, Base);
+      Result = nullptr;
     }
     return true;
   };
@@ -3552,7 +3541,7 @@ static bool evaluateVarDeclInit(EvalInfo &Info, const Expr *E,
   // ... its lifetime began within the evaluation of E;
   if (isa<ParmVarDecl>(VD)) {
     if (AllowConstexprUnknown) {
-      Result = &Info.CurrentCall->createConstexprUnknownAPValues(VD, Base);
+      Result = nullptr;
       return true;
     }
 
@@ -3659,12 +3648,8 @@ static bool evaluateVarDeclInit(EvalInfo &Info, const Expr *E,
 
   Result = VD->getEvaluatedValue();
 
-  if (!Result) {
-    if (AllowConstexprUnknown)
-      Result = &Info.CurrentCall->createConstexprUnknownAPValues(VD, Base);
-    else
-      return false;
-  }
+  if (!Result && !AllowConstexprUnknown)
+    return false;
 
   return CheckUninitReference(/*IsLocalVariable=*/false);
 }
@@ -3947,11 +3932,6 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
   const FieldDecl *LastField = nullptr;
   const FieldDecl *VolatileField = nullptr;
 
-  // C++23 [expr.const]p8 If we have an unknown reference or pointers and it
-  // does not have a value then bail out.
-  if (O->allowConstexprUnknown() && !O->hasValue())
-    return false;
-
   // Walk the designator's path to find the subobject.
   for (unsigned I = 0, N = Sub.Entries.size(); /**/; ++I) {
     // Reading an indeterminate value is undefined, but assigning over one is OK.
@@ -4491,6 +4471,14 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E,
 
     if (!evaluateVarDeclInit(Info, E, VD, Frame, LVal.getLValueVersion(), BaseVal))
       return CompleteObject();
+    // If evaluateVarDeclInit sees a constexpr-unknown variable, it returns
+    // a null BaseVal. Any constexpr-unknown variable seen here is an error:
+    // we can't access a constexpr-unknown object.
+    if (!BaseVal) {
+      Info.FFDiag(E, diag::note_constexpr_access_unknown_variable, 1) << AK << VD;
+      Info.Note(VD->getLocation(), diag::note_declared_at);
+      return CompleteObject();
+    }
   } else if (DynamicAllocLValue DA = LVal.Base.dyn_cast<DynamicAllocLValue>()) {
     std::optional<DynAlloc *> Alloc = Info.lookupDynamicAlloc(DA);
     if (!Alloc) {
@@ -6057,15 +6045,6 @@ struct CheckDynamicTypeHandler {
 /// dynamic type.
 static bool checkDynamicType(EvalInfo &Info, const Expr *E, const LValue &This,
                              AccessKinds AK, bool Polymorphic) {
-  // We are not allowed to invoke a virtual function whose dynamic type
-  // is constexpr-unknown, so stop early and let this fail later on if we
-  // attempt to do so.
-  // C++23 [expr.const]p5.6
-  // an invocation of a virtual function ([class.virtual]) for an object whose
-  // dynamic type is constexpr-unknown;
-  if (This.allowConstexprUnknown())
-    return true;
-
   if (This.Designator.Invalid)
     return false;
 
@@ -9063,6 +9042,12 @@ bool LValueExprEvaluator::VisitVarDecl(const Expr *E, const VarDecl *VD) {
   if (!evaluateVarDeclInit(Info, E, VD, Frame, Version, V))
     return false;
 
+  if (!V) {
+    Result.set(VD);
+    Result.AllowConstexprUnknown = true;
+    return true;
+  }
+
   return Success(*V, E);
 }
 
diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index ab4e50072f654..c390fee1c38d9 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -1466,7 +1466,7 @@ namespace InstantiateCaseStmt {
 
 namespace ConvertedConstantExpr {
   extern int &m;
-  extern int &n; // pre-cxx23-note 2{{declared here}}
+  extern int &n; // expected-note 2{{declared here}}
 
   constexpr int k = 4;
   int &m = const_cast<int&>(k);
@@ -1475,9 +1475,9 @@ namespace ConvertedConstantExpr {
   // useless note and instead just point to the non-constant subexpression.
   enum class E {
     em = m,
-    en = n, // expected-error {{enumerator value is not a constant expression}} cxx11_20-note {{initializer of 'n' is unknown}}
+    en = n, // expected-error {{enumerator value is not a constant expression}} cxx11_20-note {{initializer of 'n' is unknown}} cxx23-note {{read of non-constexpr variable 'n'}}
     eo = (m + // expected-error {{not a constant expression}}
-          n // cxx11_20-note {{initializer of 'n' is unknown}}
+          n // cxx11_20-note {{initializer of 'n' is unknown}} cxx23-note {{read of non-constexpr variable 'n'}}
           ),
     eq = reinterpret_cast<long>((int*)0) // expected-error {{not a constant expression}} expected-note {{reinterpret_cast}}
   };
diff --git a/clang/test/SemaCXX/constant-expression-p2280r4.cpp b/clang/test/SemaCXX/constant-expression-p2280r4.cpp
index 50637917ba210..ec545c2973f15 100644
--- a/clang/test/SemaCXX/constant-expression-p2280r4.cpp
+++ b/clang/test/SemaCXX/constant-expression-p2280r4.cpp
@@ -35,7 +35,7 @@ constexpr int how_many(Swim& swam) {
   return (p + 1 - 1)->phelps();
 }
 
-void splash(Swim& swam) {
+void splash(Swim& swam) {                 // nointerpreter-note {{declared here}}
   static_assert(swam.phelps() == 28);     // ok
   static_assert((&swam)->phelps() == 28); // ok
   Swim* pswam = &swam;                    // expected-note {{declared here}}
@@ -43,8 +43,10 @@ void splash(Swim& swam) {
                                           // expected-note {{read of non-constexpr variable 'pswam' is not allowed in a constant expression}}
   static_assert(how_many(swam) == 28);    // ok
   static_assert(Swim().lochte() == 12);   // ok
-  static_assert(swam.lochte() == 12);     // expected-error {{static assertion expression is not an integral constant expression}}
-  static_assert(swam.coughlin == 12);     // expected-error {{static assertion expression is not an integral constant expression}}
+  static_assert(swam.lochte() == 12);     // expected-error {{static assertion expression is not an integral constant expression}} \
+                                          // nointerpreter-note {{virtual function called on object 'swam' whose dynamic type is not constant}}
+  static_assert(swam.coughlin == 12);     // expected-error {{static assertion expression is not an integral constant expression}} \
+                                          // nointerpreter-note {{read of variable 'swam' whose value is not known}}
 }
 
 extern Swim dc;
@@ -253,12 +255,56 @@ namespace uninit_reference_used {
 
 namespace param_reference {
   constexpr int arbitrary = -12345;
-  constexpr void f(const int &x = arbitrary) { // expected-note {{declared here}}
+  constexpr void f(const int &x = arbitrary) { // nointerpreter-note 3 {{declared here}} interpreter-note {{declared here}}
     constexpr const int &v1 = x; // expected-error {{must be initialized by a constant expression}} \
     // expected-note {{reference to 'x' is not a constant expression}}
     constexpr const int &v2 = (x, arbitrary); // expected-warning {{left operand of comma operator has no effect}}
-    constexpr int v3 = x; // expected-error {{must be initialized by a constant expression}}
-    static_assert(x==arbitrary); // expected-error {{static assertion expression is not an integral constant expression}}
+    constexpr int v3 = x; // expected-error {{must be initialized by a constant expression}} \
+                          // nointerpreter-note {{read of variable 'x' whose value is not known}}
+    static_assert(x==arbitrary); // expected-error {{static assertion expression is not an integral constant expression}} \
+                                 // nointerpreter-note {{read of variable 'x' whose value is not known}}
     static_assert(&x - &x == 0);
   }
 }
+
+namespace dropped_note {
+  extern int &x; // expected-note {{declared here}}
+  constexpr int f() { return x; } // nointerpreter-note {{read of non-constexpr variable 'x'}} \
+                                  // interpreter-note {{initializer of 'x' is unknown}}
+  constexpr int y = f(); // expected-error {{constexpr variable 'y' must be initialized by a constant expression}} expected-note {{in call to 'f()'}}
+}
+
+namespace dynamic {
+  struct A {virtual ~A();};
+  struct B : A {};
+  void f(A& a) {
+    constexpr B* b = dynamic_cast<B*>(&a); // expected-error {{must be initialized by a constant expression}} \
+                                           // nointerpreter-note {{dynamic_cast applied to object 'a' whose dynamic type is not constant}}
+    constexpr void* b2 = dynamic_cast<void*>(&a); // expected-error {{must be initialized by a constant expression}} \
+                                                  // nointerpreter-note {{dynamic_cast applied to object 'a' whose dynamic type is not constant}}
+  }
+}
+
+namespace pointer_comparisons {
+  extern int &extern_n; // interpreter-note 2 {{declared here}}
+  extern int &extern_n2;
+  constexpr int f1(bool b, int& n) {
+    if (b) {
+      return &extern_n == &n;
+    }
+    return f1(true, n);
+  }
+  // FIXME: interpreter incorrectly rejects; both sides are the same constexpr-unknown value.
+  static_assert(f1(false, extern_n)); // interpreter-error {{static assertion expression is not an integral constant expression}} \
+                                      // interpreter-note {{initializer of 'extern_n' is unknown}}
+  // FIXME: We should diagnose this: we don't know if the references bind
+  // to the same object.
+  static_assert(&extern_n != &extern_n2); // interpreter-error {{static assertion expression is not an integral constant expression}} \
+                                          // interpreter-note {{initializer of 'extern_n' is unknown}}
+  void f2(const int &n) {
+    // FIXME: We should not diagnose this: the two objects provably have
+    // different addresses because the lifetime of "n" extends across
+    // the initialization.
+    constexpr int x = &x == &n; // nointerpreter-error {{must be initialized by a constant expression}}
+  }
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[shafik]

LGTM, this is much cleaner way to do things. I wish folks had caught some of this during the original implementation but this is all great. Thanks for the fixes. I will try and do a second pass on the tests later on.