[Clang] Ignore assumptions with side effects at compile time

[Sirraide]

Fixes #85519.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (Sirraide)

Changes

Fixes #85519.

---

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

2 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (+3)
• (modified) clang/test/SemaCXX/cxx23-assume.cpp (+10)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 7137efb7876de2..fa9e8ecf654378 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -5594,6 +5594,9 @@ static EvalStmtResult EvaluateStmt(StmtResult &Result, EvalInfo &Info,
         if (Assumption->isValueDependent())
           return ESR_Failed;
 
+        if (Assumption->HasSideEffects(Info.getCtx()))
+          continue;
+
         bool Value;
         if (!EvaluateAsBooleanCondition(Assumption, Value, Info))
           return ESR_Failed;
diff --git a/clang/test/SemaCXX/cxx23-assume.cpp b/clang/test/SemaCXX/cxx23-assume.cpp
index 2d7c9b174d9019..478da092471aff 100644
--- a/clang/test/SemaCXX/cxx23-assume.cpp
+++ b/clang/test/SemaCXX/cxx23-assume.cpp
@@ -126,3 +126,13 @@ static_assert(f5<D>() == 1); // expected-note 3 {{while checking constraint sati
 static_assert(f5<double>() == 2);
 static_assert(f5<E>() == 1); // expected-note {{while checking constraint satisfaction}} expected-note {{in instantiation of}}
 static_assert(f5<F>() == 2); // expected-note {{while checking constraint satisfaction}} expected-note {{in instantiation of}}
+
+// Do not validate assumptions whose evaluation would have side-effects.
+constexpr int foo() {
+  int a = 0;
+  [[assume(a++)]] [[assume(++a)]]; // expected-warning 2 {{has side effects that will be discarded}} ext-warning 2 {{C++23 extension}}
+  [[assume((a+=1))]]; // expected-warning {{has side effects that will be discarded}} ext-warning {{C++23 extension}}
+  return a;
+}
+
+static_assert(foo() == 0);

[shafik]

This is not really an NFC change so you should have waited for an approval. This LGTM

[Sirraide]

This is not really an NFC change so you should have waited for an approval.

Yeah, that’s something I wasn’t sure about in this particular case: The code that was causing the problem was added and merged in by me earlier this week, so I figured it was fine (and this seemed like an ‘obvious’ fix seeing as this is what we’re also checking for in codegen), but I can wait for approval next time if a situation like this comes up again.

[tbaederr]

Is it expected that a failed assumption with side effects does not result in an error?

constexpr bool f(int &a) {

    a = 10;
    return false;
}

constexpr int c() {
    int a = 0;
    [[assume(f(a))]];

    return a;
}
static_assert(c() == 0);

[Sirraide]

Is it expected that a failed assumption with side effects does not result in an error?

Yes, because it can’t evaluate the assumption without causing side effects, so the assumption isn’t checked at all; I’ve thought about this a bit too, but unless there’s already some way to evaluate an expression at compile time w/o causing any side-effects (neither at runtime nor at compile time), then this is probably the best we can do.

As an aside, GCC also checks assumptions at compile time, and it too ignores assumptions that might have side effects at compile time.

[Sirraide]

The standard also says that ‘The expression is not evaluated’ [dcl.attr.assume], so we can only really get away with evaluating it by the as-if rule iff it has no side-effects, so this is just bringing compile-time in line with what we’ve already decided to do in codegen.

[AaronBallman]

I think this is also https://eel.is/c++draft/expr.const#6.4, the note is a bit more clear than the standard wording.

[AaronBallman]

LGTM as well.

[Sirraide]

I think this is also https://eel.is/c++draft/expr.const#6.4, the note is a bit more clear than the standard wording.

That’s also a good point; I sort of forgot about that one.

Either way, I’ll wait for a review next time seeing as this was perhaps not that ‘obvious’ of a change as I had originally thought.