[clang] Perform implicit lvalue-to-rvalue cast with new interpreter

[tbaeder]

Summary

Extra bonus patch.

The EvaluateAsRValue() documentation mentions that an implicit
lvalue-to-rvalue cast is being performed if the result is an lvalue.
However, that was not being done if the new constant interpreter was in
use.

Just always do it.

This touches ExprConstant.cpp, but is a NFC patch if the new constant interpreter is not in use.

[shafik]

clang/lib/AST/ExprConstant.cpp

14938

Curious why these two checks don't go w/ the ::Evaluate(Result, Info, E) below.

[tbaeder]

clang/lib/AST/ExprConstant.cpp

14938

No real reason I think, it's just that from looking at them, they seem to make sense to me in the general case.

[tbaeder]

Can I get another opinion on this before the weekend?

[erichkeane]

Would be great if we had a better test here... is there anything we can do to validate this is happening other than checking for that one note?

[aaron.ballman]

In D132136#3751702, @erichkeane wrote:

Would be great if we had a better test here... is there anything we can do to validate this is happening other than checking for that one note?

+1 to this request, but the changes themselves LGTM otherwise

[tbaeder]

In D132136#3751702, @erichkeane wrote:

Would be great if we had a better test here... is there anything we can do to validate this is happening other than checking for that one note?

EvaluateAsRValue is called from Expr::EvaluateAsRValue(), so I think it would be possible to write a unittest for this. But I think that would be a lot of effort just to test this. There is even unittests/AST/EvaluateAsRValueTest.cpp already, but it tests the wrong thing :(

[aaron.ballman]

In D132136#3753290, @tbaeder wrote:

In D132136#3751702, @erichkeane wrote:

Would be great if we had a better test here... is there anything we can do to validate this is happening other than checking for that one note?

EvaluateAsRValue is called from Expr::EvaluateAsRValue(), so I think it would be possible to write a unittest for this. But I think that would be a lot of effort just to test this. There is even unittests/AST/EvaluateAsRValueTest.cpp already, but it tests the wrong thing :(

The existing test coverage being wrong seems like all the more reason to add correct test coverage. LValue to RValue conversions are important to get right (lol here's a wonderful demonstration of where we didn't bother to see if we got it right that I accidentally stumbled into when trying to give you a constexpr test case: https://godbolt.org/z/bdxbers3M), especially because they're going to impact which overload gets called when picking between an && and & overload.

[tbaeder]

In D132136#3755724, @aaron.ballman wrote:

In D132136#3753290, @tbaeder wrote:

In D132136#3751702, @erichkeane wrote:

Would be great if we had a better test here... is there anything we can do to validate this is happening other than checking for that one note?

EvaluateAsRValue is called from Expr::EvaluateAsRValue(), so I think it would be possible to write a unittest for this. But I think that would be a lot of effort just to test this. There is even unittests/AST/EvaluateAsRValueTest.cpp already, but it tests the wrong thing :(

The existing test coverage being wrong seems like all the more reason to add correct test coverage. LValue to RValue conversions are important to get right (lol here's a wonderful demonstration of where we didn't bother to see if we got it right that I accidentally stumbled into when trying to give you a constexpr test case: https://godbolt.org/z/bdxbers3M), especially because they're going to impact which overload gets called when picking between an && and & overload.

To be clear, can I land this patch without the unittest? I tried adding this to EvaluateAsRValueTest.cpp but I just run into other problems in the new interpreter :) So more unittests would definitely be good.

[tbaeder]

@aaron.ballman Can you comment on my last question? I'd like to land this patch since the others depend on it.

[aaron.ballman]

In D132136#3766140, @tbaeder wrote:

@aaron.ballman Can you comment on my last question? I'd like to land this patch since the others depend on it.

Sorry for the delay!

In D132136#3760738, @tbaeder wrote:

In D132136#3755724, @aaron.ballman wrote:

The existing test coverage being wrong seems like all the more reason to add correct test coverage. LValue to RValue conversions are important to get right (lol here's a wonderful demonstration of where we didn't bother to see if we got it right that I accidentally stumbled into when trying to give you a constexpr test case: https://godbolt.org/z/bdxbers3M), especially because they're going to impact which overload gets called when picking between an && and & overload.

To be clear, can I land this patch without the unittest? I tried adding this to EvaluateAsRValueTest.cpp but I just run into other problems in the new interpreter :) So more unittests would definitely be good.

On the one hand, I'm not comfortable landing this without adequate testing, especially because you want to build more stuff on top of it. I think we need to make sure the foundational bits of evaluation are rock solid before building things on top of them. However, on the other hand, this is foundational enough that it's hard to test this bit without other parts of the interpreter being ready. So my preference is to try to add test coverage even if it's failing currently because other parts of the interpreter aren't ready yet. e.g., (imaginary test case, feel free to use better ones)

template <typename Ty, typename Uy>
struct is_same { static constexpr bool value = false; };

template <typename Ty>
struct is_same<Ty, Ty> { static constexpr bool value = true; };

const volatile int i = 0;
// Test that lvalue to rvalue conversion ends up stripping top-level qualifiers.
// FIXME: this isn't correct yet because we don't have support for qualifier conversions yet.
static_assert(is_same<decltype(+i), int>::value, "oh no!"); // expected-error {{static assertion failed: oh no!}}

This way, we can start getting extensive test cases written while thinking about the situations specific to lvalue to rvalue conversion (or whatever else is being implemented at the moment) instead of trying to come back and remember to write them later.

Is that reasonable, or is the interpreter in such a state where even this kind of testing is basically impossible to come up with?

[tbaeder]

In D132136#3766958, @aaron.ballman wrote:

On the one hand, I'm not comfortable landing this without adequate testing, especially because you want to build more stuff on top of it. I think we need to make sure the foundational bits of evaluation are rock solid before building things on top of them. However, on the other hand, this is foundational enough that it's hard to test this bit without other parts of the interpreter being ready. So my preference is to try to add test coverage even if it's failing currently because other parts of the interpreter aren't ready yet. e.g., (imaginary test case, feel free to use better ones)

template <typename Ty, typename Uy>
struct is_same { static constexpr bool value = false; };

template <typename Ty>
struct is_same<Ty, Ty> { static constexpr bool value = true; };

const volatile int i = 0;
// Test that lvalue to rvalue conversion ends up stripping top-level qualifiers.
// FIXME: this isn't correct yet because we don't have support for qualifier conversions yet.
static_assert(is_same<decltype(+i), int>::value, "oh no!"); // expected-error {{static assertion failed: oh no!}}

This way, we can start getting extensive test cases written while thinking about the situations specific to lvalue to rvalue conversion (or whatever else is being implemented at the moment) instead of trying to come back and remember to write them later.

Is that reasonable, or is the interpreter in such a state where even this kind of testing is basically impossible to come up with?

Soo... I was gonna tell you that the test case is obviously not going to work right now, since record types aren't implemented at all. But the test case actually works just fine. However, it also works without the lvalue-to-rvalue conversion in EvaluteAsRValue. Both with the old and the new interpreter.

I'm going to add an actual unit test that fails without the conversion. But it only checks a very simple case, i.e. that a DeclRefExpr is returned as an integer and not the lvalue.

[tbaeder]

Add a unit test.

[aaron.ballman]

LGTM, this wasn't quite what I had in mind, but I can see how my suggestion was going to be far too tricky to try to implement as a lit test. Thanks for adding the unit test coverage!

clang/lib/AST/ExprConstant.cpp

static bool EvaluateInPlace(APValue &Result, EvalInfo &Info, const LValue &This,
   // For any other type, in-place evaluation is unimportant.
   return Evaluate(Result, Info, E);
 }
 
 /// EvaluateAsRValue - Try to evaluate this expression, performing an implicit
 /// lvalue-to-rvalue cast if it is an lvalue.
 static bool EvaluateAsRValue(EvalInfo &Info, const Expr *E, APValue &Result) {
   assert(!E->isValueDependent());
-  if (Info.EnableNewConstInterp) {
-    if (!Info.Ctx.getInterpContext().evaluateAsRValue(Info, E, Result))
-      return false;
-  } else {
-    if (E->getType().isNull())
-      return false;
+
 
-    if (!CheckLiteralType(Info, E))
-      return false;
 
+  if (Info.EnableNewConstInterp) {
+    if (!Info.Ctx.getInterpContext().evaluateAsRValue(Info, E, Result))
+      return false;
+  } else {
     if (!::Evaluate(Result, Info, E))
       return false;
+  }
 
-    if (E->isGLValue()) {
-      LValue LV;
-      LV.setFrom(Info.Ctx, Result);
-      if (!handleLValueToRValueConversion(Info, E, E->getType(), LV, Result))
-        return false;
-    }
-  }
+  // Implicit lvalue-to-rvalue cast.
 
   // Check this core constant expression is a constant expression.
   return CheckConstantExpression(Info, E->getExprLoc(), E->getType(), Result,
                                  ConstantExprKind::Normal) &&
          CheckMemoryLeaks(Info);
 }
 
 static bool FastEvaluateAsRValue(const Expr *Exp, Expr::EvalResult &Result,

clang/test/AST/Interp/literals.cpp

 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -std=c++11 -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify=ref %s
 
 static_assert(true, "");
 static_assert(false, ""); // expected-error{{failed}} ref-error{{failed}}
 static_assert(nullptr == nullptr, "");
 static_assert(1 == 1, "");
 static_assert(1 == 3, ""); // expected-error{{failed}} ref-error{{failed}}
 
 constexpr int number = 10;
 static_assert(number == 10, "");
 static_assert(number != 10, ""); // expected-error{{failed}} \
                                  // ref-error{{failed}} \
+                                 // expected-note{{evaluates to}} \
                                  // ref-note{{evaluates to}}
 
 constexpr bool getTrue() { return true; }
 constexpr bool getFalse() { return false; }
 constexpr void* getNull() { return nullptr; }
 
 constexpr int neg(int m) { return -m; }
 constexpr bool inv(bool b) { return !b; }

clang/unittests/AST/EvaluateAsRValueTest.cpp

ASSERT_TRUE(runToolOnCodeWithArgs(
         "};"
         "void doSomething() {"
         "  int Constant = 2 + 2;"
         "  (void) Constant;"
         "}",
         Args));
   }
 }
+
+class CheckLValueToRValueConversionVisitor
+    : public clang::RecursiveASTVisitor<CheckLValueToRValueConversionVisitor> {
+public:
+  bool VisitDeclRefExpr(const clang::DeclRefExpr *E) {
+    clang::Expr::EvalResult Result;
+    E->EvaluateAsRValue(Result, E->getDecl()->getASTContext(), true);
+
+    EXPECT_TRUE(Result.Val.hasValue());
+    // Since EvaluateAsRValue does an implicit lvalue-to-rvalue conversion,
+    // the result cannot be a LValue.
+    EXPECT_FALSE(Result.Val.isLValue());
+
+    return true;
+  }
+};
+
+class CheckConversionAction : public clang::ASTFrontendAction {
+public:
+  std::unique_ptr<clang::ASTConsumer>
+  CreateASTConsumer(clang::CompilerInstance &Compiler,
+                    llvm::StringRef FilePath) override {
+    return std::make_unique<Consumer>();
+  }
+
+private:
+  class Consumer : public clang::ASTConsumer {
+  public:
+    ~Consumer() override {}
+
+    void HandleTranslationUnit(clang::ASTContext &Ctx) override {
+      CheckLValueToRValueConversionVisitor Evaluator;
+      Evaluator.TraverseDecl(Ctx.getTranslationUnitDecl());
+    }
+  };
+};
+
+TEST(EvaluateAsRValue, LValueToRValueConversionWorks) {
+  std::string ModesToTest[] = {"", "-fexperimental-new-constant-interpreter"};
+  for (std::string const &Mode : ModesToTest) {
+    std::vector<std::string> Args(1, Mode);
+    ASSERT_TRUE(runToolOnCodeWithArgs(std::make_unique<CheckConversionAction>(),
+                                      "constexpr int a = 20;\n"
+                                      "static_assert(a == 20, \"\");\n",
+                                      Args));
+  }
+}