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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1546
 
 def warn_consteval_if_always_true : Warning<
+  "consteval if is always %select{true|false}0 in this context">,
----------------
To be consistent with other tautological warnings


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8821
   "operand to 'typeid'">, InGroup<PotentiallyEvaluatedExpression>;
+def warn_is_constant_evaluated_tauto_constexpr : Warning<
+  "'%select{std::is_constant_evaluated|__builtin_is_constant_evaluated}0' will always evaluate to %select{false|true}1 in this context">,
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:1277
+    /// as PotentiallyEvaluated.
+    RuntimeEvaluated
   };
----------------
hazohelet wrote:
> 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.
Right, thanks for clarifying, i missed that


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2440
+            S.ExprEvalContexts.back().Context;
+        if ((D.getDeclSpec().getTypeQualifiers() == DeclSpec::TQ_const ||
+             isNonlocalVariable(ThisDecl)) &&
----------------
Why are we looking at whether static vars are const qualified?


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1513-1516
+    EnterExpressionEvaluationContext Consteval(
+        Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+        /*LambdaContextDecl=*/nullptr,
+        Sema::ExpressionEvaluationContextRecord::EK_Other, IsConstexpr);
----------------
hazohelet wrote:
> 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
I missed that, makes sense!


================
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;
----------------
hazohelet wrote:
> 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.
> 
Thanks for the detailed explanation.
Separating InitializerScopeRAII from evaluation scope is something i support.
Did you try removing the test in `DiagIfReachable`, see how much breaks?
In any case, there are 2 fixme in that function already, that should cover us.
It might be useful to better describe these changes in the release notes / commit message


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp:96
 int main(int, char**) {
-  if (!std::is_constant_evaluated()) {
-    test_containers<std::deque<int>, std::deque<int>>();
----------------
this is a funny one, what's the history of that?


================
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
----------------
Mordante wrote:
> hazohelet wrote:
> > 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!
> I really would like a regex. To me the current message misses an important piece of information; the `true` part. I care less about the rest of the message, but stripping the `true` means a warning like `std::is_constant_evaluated' will always evaluate to FALSE` would be valid too.
Agreed with Mordante


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

https://reviews.llvm.org/D155064