[clang][Interp] Reject invalid declarations and expressions

[tbaeder]

Summary

I'm not really sure if this is the right thing to do. These expressions and declarations have been marked as containing errors before, so they can't work properly during constant evaluation anyway. But I don't see them being rejected in the current constant interpreter.

[aaron.ballman]

I'd like to see some test coverage for what these changes catch that wasn't caught before. On its face, this seems reasonable in that we don't want to bother running the interpreter (further) once we know we've hit an expression, declaration, or type that's not valid.

[tbaeder]

Tried to find an example for an invalid declaration but couldn't even manage to find one :|

[aaron.ballman]

clang/test/AST/Interp/functions.cpp

88–97

There should definitely be diagnostics here, right? There's a use of an undeclared identifier in each function, but also, invalid() doesn't have a return statement despite returning an int.

[tbaeder]

clang/test/AST/Interp/functions.cpp

88–97

Right, I forgot to update the tests, sorry!

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1166–1167

Because we can't find a case where we hit this, I think we should try asserting instead.

[tbaeder]

Ping

[aaron.ballman]

LGTM!

clang/lib/AST/Interp/ByteCodeExprGen.cpp

bool ByteCodeExprGen<Emitter>::VisitCompoundAssignOperator(
 
   // And store the result in LHS.
   if (DiscardResult)
     return this->emitStorePop(*ResultT, E);
   return this->emitStore(*ResultT, E);
 }
 
 template <class Emitter> bool ByteCodeExprGen<Emitter>::discard(const Expr *E) {
+  if (E->containsErrors())
+    return false;
+
   OptionScope<Emitter> Scope(this, /*NewDiscardResult=*/true);
   return this->Visit(E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visit(const Expr *E) {
+  if (E->containsErrors())
+    return false;
+
   OptionScope<Emitter> Scope(this, /*NewDiscardResult=*/false);
   return this->Visit(E);
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitBool(const Expr *E) {
   if (std::optional<PrimType> T = classify(E->getType())) {
     return visit(E);
else
     return this->emitRetValue(Exp);
 }
 
 /// Toplevel visitDecl().
 /// We get here from evaluateAsInitializer().
 /// We need to evaluate the initializer and return its value.
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitDecl(const VarDecl *VD) {
+  assert(!VD->isInvalidDecl() && "Trying to constant evaluate an invalid decl");
   std::optional<PrimType> VarT = classify(VD->getType());
 
   // Create and initialize the variable.
   if (!this->visitVarDecl(VD))
     return false;
 
   // Get a pointer to the variable
   if (shouldBeGloballyIndexed(VD)) {
     auto GlobalIndex = P.getGlobal(VD);
     assert(GlobalIndex); // visitVarDecl() didn't return false.

clang/test/AST/Interp/functions.cpp

static_assert(f() == 5, ""); // expected-error {{not an integral constant expression}} \
                              // ref-note {{undefined function 'f'}}
 constexpr int a() {
   return f();
 }
 constexpr int f() {
   return 5;
 }
 static_assert(a() == 5, "");
+
+constexpr int invalid() {
+  // Invalid expression in visit().
+  while(huh) {} // expected-error {{use of undeclared identifier}} \
+                // ref-error {{use of undeclared identifier}}
+
+  return 0;
+}
+
+constexpr void invalid2() {
+  int i = 0;
+  // Invalid expression in discard().
+  huh(); // expected-error {{use of undeclared identifier}} \
+         // ref-error {{use of undeclared identifier}}
+}