[Clang] Add a compatibiliy warning for non-literals in constexpr.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2724

You should re-flow the whole diagnostic to the usual 80-col limit.

clang/lib/Sema/SemaDeclCXX.cpp

1903–1912

Formatting looks off here (I was expecting the else if on the same line as the curly brace, but maybe clang-format is being weird?)

1907

clang/test/SemaCXX/constant-expression-cxx2b.cpp

99

Shouldn't this be a cxx2b-warning instead?

[cor3ntin]

• fix formatting

• use cxx2b in the test as per aaron request. This doesn't really matter as this part

of the test file is only executed in c++2b mode though.

[hubert.reinterpretcast]

clang/lib/Sema/SemaDeclCXX.cpp

1905

This seems to trigger even when the type is dependent:

<stdin>:1:36: warning: definition of a variable of non-literal type in a constexpr function is incompatible with C++ standards before C++2b [-Wpre-c++2b-compat]
auto qq = [](auto x) { decltype(x) n; };
                                   ^
1 warning generated.

This also seems to emit even when Kind is not Sema::CheckConstexprKind::Diagnose (unlike the static/thread_local case above). Is the CheckLiteralType logic not reusable for this?

clang/test/SemaCXX/constant-expression-cxx2b.cpp

219

Not sure how much we want the message in this case. The lambda is not marked constexpr (although it is implicitly constexpr in C++2b).

Note that we don't get a message for this:

auto qq = [] { return 0; static int x = 42; };
constexpr int qx = qq();

I am not sure how difficult it would be to come at this from the used-in-constant-evaluation side, but there is probably a larger class of messages in the same situation (so it would probably be a separate endeavour).

[cor3ntin]

• Check That the type is dependant
• Only emit the warning when Kind == Sema::CheckConstexprKind::Diagnose

[cor3ntin]

Formatting

[cor3ntin]

clang/lib/Sema/SemaDeclCXX.cpp

1905

You are right, thanks for noticing that, it was rather bogus.
The reason I'm not using CheckLiteralType is to avoid duplicating a diagnostics message, as CheckLiteralType doesn't allow us to pass parameter to the diagnostic message.

It leaves us with an uncovered scenario though: We do not emit the warning on template instantiation, and I don't think there is an easy way to do that.

[aaron.ballman]

I'm seeing precommit CI failures:

Failed Tests (1):

Clang :: SemaCXX/constant-expression-cxx2b.cpp

clang/lib/Sema/SemaDeclCXX.cpp

1905

The reason I'm not using CheckLiteralType is to avoid duplicating a diagnostics message, as CheckLiteralType doesn't allow us to pass parameter to the diagnostic message.

Huh?

static bool CheckLiteralType(Sema &SemaRef, Sema::CheckConstexprKind Kind,
                             SourceLocation Loc, QualType T, unsigned DiagID,
                             Ts &&...DiagArgs) {
  ...
}

I would hope DiagArgs should do exactly that? :-)

[cor3ntin]

• Fix test
• Add a comment explaining why we do not use CheckLiteralType

@aaron,ballman I completely missed that...!

However, CheckLiteralType does a bunch of tests that we do not need
to diagnose *why* a type is not literal, which we do not care about here.

I tried to play around with the code and trying to use CheckLiteralType, or to modify
it for this scenario does not appear to me an improvenent, so I rather keep the change as is.

I also fixed the fail test, i think i accidentally removed a line from the test file...

[aaron.ballman]

In D122249#3402331, @cor3ntin wrote:

• Fix test
• Add a comment explaining why we do not use CheckLiteralType

@aaron,ballman I completely missed that...!

No worries!

However, CheckLiteralType does a bunch of tests that we do not need
to diagnose *why* a type is not literal, which we do not care about here.

I think those kinds of diagnostics could be useful to a developer who has enabled this compat warning. It may not be immediately clear why the type is not a literal, and those notes can help some folks get to that answer much more quickly. Do you have some examples of whether the diagnostics clearly are distracting or useless?

clang/lib/Sema/SemaDeclCXX.cpp

1905

It leaves us with an uncovered scenario though: We do not emit the warning on template instantiation, and I don't think there is an easy way to do that.

I believe the code paths that lead us here all come from Sema::CheckConstexprFunctionDefinition() which is called from Sema::ActOnFinishFunctionBody() which seems to be called when instantiating templates in Sema::InstantiateFunctionDefinition(), so perhaps some more investigation is needed as to why we're not reaching this for template instantiations.

[cor3ntin]

clang/lib/Sema/SemaDeclCXX.cpp

1905

We could add something in addition of Sema::CheckConstexprKind::CheckValid and Sema::CheckConstexprKind::Diagnose, but

• not for implicit lambdas, because we should not warn on lambdas that won't be constexpr
• for explicit constexpr lambdas / functions, it would force us to call CheckConstexprFunctionDefinition on instanciation, which we currently don't do, and is not free for that one warning - and we would have to not-reemit the other warnings. It seems like quite a fair amount of work for a diagnostic not enabled by default.

[cor3ntin]

After talks with Aaron, I'm using CheckLiteralType
in all cases, which adds notes about why a type
is not literal.

We still need to branch on CPlusPlus2b because the
diagnostics take a variable number of arguments,
and I'd rather not have more diagnostics.

[hubert.reinterpretcast]

clang/lib/Sema/SemaDeclCXX.cpp

1905–1907

Apply Aaron's suggestion (https://reviews.llvm.org/D122249?id=417358#inline-1168823) to the updated code.

[cor3ntin]

• Add a commebt to clarify which diagnostic message is being printed,

as per Aaron suggestion.

[hubert.reinterpretcast]

This LGTM (with minor comment). Please wait for Aaron to respond re: the handling of template instantiations.

clang/lib/Sema/SemaDeclCXX.cpp

1905

so perhaps some more investigation is needed as to why we're not reaching this for template instantiations.

@aaron.ballman, do you have any position on whether we want this investigation before moving forward with this patch?

1912–1913

Minor nit: The coding standards were updated (some time ago now) to recommend keeping if-else chains consistently braced or not-braced.

[cor3ntin]

Add braces

[aaron.ballman]

LGTM

clang/lib/Sema/SemaDeclCXX.cpp

1905

so perhaps some more investigation is needed as to why we're not reaching this for template instantiations.

@aaron.ballman, do you have any position on whether we want this investigation before moving forward with this patch?

@hubert.reinterpretcast -- I think @cor3ntin did that investigation and found that we don't make it to this code path because of https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L14961.

I think this is incremental progress, and we can handle the template instantiation cases in a follow-up.

[cor3ntin]

I thing to support the template case, we would have to modify TemplateDeclInstantiator::VisitVarDecl - which seems the best way to have a diagnostic without running through the whole function AST twice.
However, there is currently no check of any sort there so it would be a bit novel and odd!

Thank you both for the review

Edit: Longer comment

As per http://eel.is/c++draft/dcl.constexpr#7, the standard doesn't require to check the whole function on instantiation.
And there would be issue in doing that: all the warnings, emitted in other cases, would have to not be re-emitted

consider

constexpr void f(auto) {
if(false)    goto a;
a:;
}

void g() {
    f(0);
}

We do not want to warn twice here.
So we can't reuse the existing infrastructure.
The functions checking for constant-evaluations are not great either because they may ignore branches etc.
So I think the two options are

• During instantiation warn fir non-literal var decls if they are in the context of a constexpr function
• Make a new function like CheckConstexprFunctionBody for only that warning and go through the whole tree twice
• Modify CheckConstexprFunctionBody with a new Sema::CheckConstexprKind thing for instantiation, which also goes through the whole tree twice.

None of these options seem great.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_cxx11_compat_constexpr_local_var : Warning<
   "variable declaration in a constexpr %select{function|constructor}0 "
   "is incompatible with C++ standards before C++14">,
   InGroup<CXXPre14Compat>, DefaultIgnore;
 def ext_constexpr_static_var : ExtWarn<
   "definition of a %select{static|thread_local}1 variable "
   "in a constexpr %select{function|constructor}0 "
   "is a C++2b extension">, InGroup<CXX2b>;
-def warn_cxx20_compat_constexpr_static_var : Warning<
-  "definition of a %select{static|thread_local}1 variable "
-  "in a constexpr %select{function|constructor}0 "
+def warn_cxx20_compat_constexpr_var : Warning<
+  "definition of a %select{static variable|thread_local variable|variable "
+  "of non-literal type}1 in a constexpr %select{function|constructor}0 "
   "is incompatible with C++ standards before C++2b">,
   InGroup<CXXPre2bCompat>, DefaultIgnore;
 def err_constexpr_local_var_non_literal_type : Error<
   "variable of non-literal type %1 cannot be defined in a constexpr "
   "%select{function|constructor}0 before C++2b">;
 def ext_constexpr_local_var_no_init : ExtWarn<
   "uninitialized variable in a constexpr %select{function|constructor}0 "
   "is a C++20 extension">, InGroup<CXX20>;

clang/lib/Sema/SemaDeclCXX.cpp

case Decl::Decomposition: {
       //   thread storage duration or [before C++2a] for which no
       //   initialization is performed.
       const auto *VD = cast<VarDecl>(DclIt);
       if (VD->isThisDeclarationADefinition()) {
         if (VD->isStaticLocal()) {
           if (Kind == Sema::CheckConstexprKind::Diagnose) {
             SemaRef.Diag(VD->getLocation(),
                          SemaRef.getLangOpts().CPlusPlus2b
-                             ? diag::warn_cxx20_compat_constexpr_static_var
+                             ? diag::warn_cxx20_compat_constexpr_var
                              : diag::ext_constexpr_static_var)
                 << isa<CXXConstructorDecl>(Dcl)
                 << (VD->getTLSKind() == VarDecl::TLS_Dynamic);
           } else if (!SemaRef.getLangOpts().CPlusPlus2b) {
             return false;
           }
         }
-        if (!SemaRef.LangOpts.CPlusPlus2b &&
-            CheckLiteralType(SemaRef, Kind, VD->getLocation(), VD->getType(),
-                             diag::err_constexpr_local_var_non_literal_type,
-                             isa<CXXConstructorDecl>(Dcl)))
+        if (SemaRef.LangOpts.CPlusPlus2b) {
+          if (!VD->getType()->isLiteralType(SemaRef.Context))
+            SemaRef.Diag(VD->getLocation(),
+                         diag::warn_cxx20_compat_constexpr_var)
+                << isa<CXXConstructorDecl>(Dcl) << 2;
+        } else if (CheckLiteralType(
+                       SemaRef, Kind, VD->getLocation(), VD->getType(),
           return false;
         if (!VD->getType()->isDependentType() &&
             !VD->hasInit() && !VD->isCXXForRangeDecl()) {
           if (Kind == Sema::CheckConstexprKind::Diagnose) {
             SemaRef.Diag(
                 VD->getLocation(),
                 SemaRef.getLangOpts().CPlusPlus20
                     ? diag::warn_cxx17_compat_constexpr_local_var_no_init
                     : diag::ext_constexpr_local_var_no_init)

clang/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3-2b.cpp

 label:; // expected-warning {{use of this statement in a constexpr function is incompatible with C++ standards before C++2b}}
 }
 
 struct NonLiteral {
   NonLiteral() {}
 };
 
 constexpr void non_literal() { // expected-error {{constexpr function never produces a constant expression}}
-  NonLiteral n;                // expected-note {{non-literal type 'NonLiteral' cannot be used in a constant expression}}
+  NonLiteral n;                // expected-note {{non-literal type 'NonLiteral' cannot be used in a constant expression}} \
+                               // expected-warning {{definition of a variable of non-literal type in a constexpr function is incompatible with C++ standards before C++2b}}
 }
 
 constexpr void non_literal2(bool b) {
   if (!b)
-    NonLiteral n;
+    NonLiteral n; // expected-warning {{definition of a variable of non-literal type in a constexpr function is incompatible with C++ standards before C++2b}}
 }
 
 constexpr int c_thread_local(int n) {
   if (!n)
     return 0;
   static _Thread_local int a; // expected-warning {{definition of a static variable in a constexpr function is incompatible with C++ standards before C++2b}}
   _Thread_local int b;        // // expected-error {{'_Thread_local' variables must have global storage}}
   return 0;

clang/test/SemaCXX/constant-expression-cxx2b.cpp

constexpr int thread_local_constexpr() { // expected-error {{constexpr function never produces a constant expression}}
   thread_local constexpr int m = 42;     // expected-note {{control flows through the definition of a thread_local variable}} \
                                          // cxx2b-warning {{definition of a thread_local variable in a constexpr function is incompatible with C++ standards before C++2b}}
   return m;
 }
 
 constexpr int non_literal(bool b) {
   if (!b)
     return 0;
-  NonLiteral n;
+  NonLiteral n; // cxx2b-warning {{definition of a variable of non-literal type in a constexpr function is incompatible with C++ standards before C++2b}}
 }
 
 constexpr int non_literal_1 = non_literal(false);
 
 namespace eval_goto {
 
 constexpr int f(int x) {
   if (x) {
int test_in_lambdas() {
   };
   c(0);
   constexpr auto c_error = c(1); // expected-error {{constexpr variable 'c_error' must be initialized by a constant expression}} \
                                  // expected-note {{in call to}}
 
   auto non_literal = [](bool b) constexpr {
     if (!b)
       NonLiteral n; // cxx2b-note {{non-literal type 'NonLiteral' cannot be used in a constant expression}} \
-                    // cxx2a-error {{variable of non-literal type 'NonLiteral' cannot be defined in a constexpr function before C++2b}}
+                    // cxx2a-error {{variable of non-literal type 'NonLiteral' cannot be defined in a constexpr function before C++2b}} \
+                    // cxx2b-warning {{definition of a variable of non-literal type in a constexpr function is incompatible with C++ standards before C++2b}}
     return 0;
   };
 
 #if __cplusplus > 202002L
   constexpr auto non_literal_ko = non_literal(false); // cxx2b-error {{constexpr variable 'non_literal_ko' must be initialized by a constant expression}} \
                                                       // cxx2b-note {{in call}}
 
   constexpr auto non_literal_ok = non_literal(true);
constexpr auto c_ok = c(0); // cxx2a-error {{must be initialized by a constant expression}} \
                               // cxx2a-note {{non-constexpr function}}
 
   constexpr auto c_error = c(1); // expected-error {{constexpr variable 'c_error' must be initialized by a constant expression}} \
                                  // cxx2a-note {{non-constexpr function}} \
                                  // cxx2b-note {{in call to}}
 
   auto non_literal = [](bool b) { // cxx2a-note 2{{declared here}}
     if (b)
-      NonLiteral n; // cxx2b-note {{non-literal type 'NonLiteral' cannot be used in a constant expression}}
+      NonLiteral n; // cxx2b-note {{non-literal type 'NonLiteral' cannot be used in a constant expression}} \
+                    // cxx2b-warning {{definition of a variable of non-literal type in a constexpr function is incompatible with C++ standards before C++2b}}
     return 0;
   };
 
   constexpr auto non_literal_ko = non_literal(true); // expected-error {{constexpr variable 'non_literal_ko' must be initialized by a constant expression}} \
                                                      // cxx2a-note {{non-constexpr function}} \
                                                      // cxx2b-note {{in call}}
 
   constexpr auto non_literal_ok = non_literal(false); // cxx2a-error {{must be initialized by a constant expression}} \
                                                       // cxx2a-note {{non-constexpr function}}
 }
 
 template <typename T>
 constexpr auto dependent_var_def_lambda(void) {
   return [](bool b) { // cxx2a-note {{declared here}}
     if (!b)
-      T t;
+      T t; // cxx2b-warning {{definition of a variable of non-literal type in a constexpr function is incompatible with C++ standards before C++2b}}
     return 0;
   };
 }
 
 constexpr auto non_literal_valid_in_cxx2b = dependent_var_def_lambda<NonLiteral>()(true); // \
     // cxx2a-error {{constexpr variable 'non_literal_valid_in_cxx2b' must be initialized by a constant expression}} \
-    // cxx2a-note  {{non-constexpr function}}
+    // cxx2a-note {{non-constexpr function}} \
+    // cxx2b-note {{in instantiation}}