[PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet marked 9 inline comments as done.
hazohelet added a comment.

Thank you for the review!



================
Comment at: clang/include/clang/Sema/Sema.h:1277
+    /// as PotentiallyEvaluated.
+    RuntimeEvaluated
   };
----------------
cor3ntin wrote:
> I think I'd prefer a boolean for that, `ExpressionEvaluationContext` somewhat matches standard definitions.
> I think we might mostly be able to reuse PotentiallyEvaluatedIfUsed | PotentiallyEvaluated.
> 
> RuntimeEvaluated is anything that does not have a parent context that is constant evaluated/immediate/unevaluated. Maybe you could try to make a function for that?
I made it a boolean instead of a new expression evaluation context.

> RuntimeEvaluated is anything that does not have a parent context that is constant evaluated/immediate/unevaluated. Maybe you could try to make a function for that?
I dont't think it is correct. The definitions body of constexpr-qualified function and non-qualified function both push `PotentiallyEvaluated`, so we need some way to distinguish them, and this time it is a boolean.


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3214-3221
   EnterExpressionEvaluationContext Context(
       Actions,
       isa_and_present<FieldDecl>(D)
           ? Sema::ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed
           : Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
       D);
+  Actions.ExprEvalContexts.back().InConstantEvaluated =
----------------
cor3ntin wrote:
> Maybe we should instead say that a constexpr FieldDecl is ConstantEvaluated ?
> `InConstantEvaluated` in general seems redundant
I removed `InConstantEvaluated`


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1513-1516
+    EnterExpressionEvaluationContext Consteval(
+        Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+        /*LambdaContextDecl=*/nullptr,
+        Sema::ExpressionEvaluationContextRecord::EK_Other, IsConstexpr);
----------------
cor3ntin wrote:
> This seems wrong, the condition of a if statement is not constant evaluated.
In this case `/*ShouldEnter=*/IsConstexpr`, so the constant evaluation context is pushed only when it is a condition of constexpr if. The condition of constexpr if shall be a constant expression as per https://eel.is/c++draft/stmt.if#2


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17962-18006
-/// Determine whether the given declaration is a global variable or
-/// static data member.
-static bool isNonlocalVariable(const Decl *D) {
-  if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(D))
-    return Var->hasGlobalStorage();
-
-  return false;
----------------
cor3ntin wrote:
> Can you explain the changes to this file? they seems to affect test cases unrelated to the goal of this PR
When we push/pop evaluation context here, it behaves synchronously with `InitializerScopeRAII`, in ParseDecl.cpp, which makes the evaluation of `Actions.AddInitializerToDecl` happen outside of the initializer expression evaluation context (e.g. initializer scope popped in 2581 and AddInitializerToDecl evaluated in 2592 in ParseDecl.cpp). This is causing a bug BEFORE this patch (https://godbolt.org/z/b18jxKT54 and the removed FIXME in test/SemaCXX/cxx2a-consteval.cpp) because  we were pushing evaluation context if the variable `isNonlocalVariable`. This PR separates the handling of expression evaluation context of initializers from `InitializerScopeRAII` to resolve these bugs.

The arguable changes in diagnostics caused by this are the removals of
`warn_uninit_self_reference_in_reference_init` against `constexpr int &n = n;` (You mentioned) and
`warn_impcast_integer_precision_constant` against `constexpr signed char a = 2*100;`
Both are diagnosed by calling `Sema::DiagRuntimeBehavior`, and `Sema::DiagIfReachable` called by this intentionally avoids emitting diagnostics against initializer of constexpr variables. (https://github.com/llvm/llvm-project/blob/ab73bd3897b58ecde22ec5eea14b6252f2d69b6e/clang/lib/Sema/SemaExpr.cpp#L20712-L20719)
They were diagnosed BEFORE this patch because the expression evaluation context was outside of the initializer evaluation context, and it emitted false positives against cases like `constexpr signed char a = true ? 3 : 200;` (https://godbolt.org/z/9z1rer7E3)
I think if the evaluated result of constexpr variable initializer does not fit the variable type it should be diagnosed, but the logic should be added to where we handle constexpr variables.



================
Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:1964
   void f() {
-    constexpr int &n = n; // expected-error {{constant expression}} expected-note {{use of reference outside its lifetime}} expected-warning {{not yet bound to a value}}
     constexpr int m = m; // expected-error {{constant expression}} expected-note {{read of object outside its lifetime}}
----------------
cor3ntin wrote:
> This change does not seem correct
This is because it is diagnosed by `Sema::DiagRuntimeBehavior`
In this case constexpr evaluator is emitting an error, so I don't think the loss of this warning is problematic.
Please see my comment about the changes in ParseDeclCXX.cpp for details


================
Comment at: libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp:27
   static_assert(!std::is_constant_evaluated(), "");
-  // expected-warning at -1 0-1 {{'std::is_constant_evaluated' will always evaluate to 'true' in a manifestly constant-evaluated expression}}
+  // expected-warning at -1 0-1 {{'std::is_constant_evaluated' will always evaluate to true in this context}}
 #endif
----------------
philnik wrote:
> Mordante wrote:
> > Since libc++ support the latest ToT Clang and the last two official releases this wont work. The `expected-warning` needs to be a `expected-warning-re` that works for both the new and old diagnostic
> You can also just shorten it to `'std::is_constant_evaluated' will always evaluate to`. Seems good enough to me.
Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155064/new/

https://reviews.llvm.org/D155064