Warn if using `elifdef` & `elifndef` in not C2x & C++2b mode

[ken-matsui]

Summary

This patch resolves https://github.com/llvm/llvm-project/issues/55306.

[aaron.ballman]

Thank you for working on this!

clang/include/clang/Basic/DiagnosticLexKinds.td

696–698

I think we need two diagnostics, one for C2x and one for C++2b (https://wg21.link/p2334 was adopted for C++23). Each of these diagnostics should come in a pair:

def warn_cxx20_compat_pp_directive : Warning<
  "use of a '#%select{elifdef|elifndef}0' directive is incompatible with C++ standards before C++2b",
  InGroup<CXXPre2bCompat>, DefaultIgnore;
def ext_cxx20_pp_directive : ExtWarn<
  "use of a '#%select{elifdef|elifndef}0' directive is a C++2b extension",
  InGroup<CXX2b>;

and similar for C, except with wording about C standards and in the C warning groups.

clang/lib/Lex/PPDirectives.cpp

3262–3263

And then here, you would do something like:

unsigned DiagID;
if (getLangOpts().CPlusPlus)
  DiagID = getLangOpts().CPlusPlus2b ? diag::warn_cxx20_compat_pp_directive : diag::ext_cxx20_pp_directive;
else
  DiagID = getLangOpts().C2x ? diag::warn_c17_compat_pp_directive : diag::ext_c2b_pp_directive;

Diag(ElifToken, DiagID) << ...;

clang/test/Preprocessor/ext-c2x-pp-directive.c

14

Might as well add a declaration so that we don't have to expect the diagnostic.

Also, this will need tests for both forms of the diagnostics, C++ tests, and it'd probably be good to verify the behavior when the previous conditional is skipped. e.g.,

#if 0
#elifdef WHATEVER // We should still warn on this; we wouldn't if we didn't recognize it as a conditional
#endif

[ken-matsui]

Update the code as reviewed

[ken-matsui]

Thank you so much for your review!

clang/include/clang/Basic/DiagnosticLexKinds.td

696–698

I thought I had to use Extension here, but what is the difference between Warning, ExtWarn, and Extension?

[aaron.ballman]

It looks like precommit CI caught some related failures:

Failed Tests (7):
  Clang :: Lexer/deprecate-macro.c
  Clang :: Lexer/unsafe-macro.c
  Clang :: Preprocessor/elifdef.c
  Clang :: Preprocessor/if_warning.c
  Clang :: Preprocessor/ifdef-recover.c
  Clang :: Preprocessor/macro_misc.c
  Clang :: Preprocessor/macro_vaopt_check.cpp

clang/include/clang/Basic/DiagnosticLexKinds.td

696–698

Great question!

Extension diagnostics are warnings that are enabled via -pedantic but otherwise off by default.
ExtWarn diagnostics are warnings that are enabled via -pedantic but are also on by default.
Warning diagnostics are warnings. You can use things like DefaultIgnore or DefaultError to control the default behavior of the warning.
Error diagnostics are errors.

You'll typically use Extension or ExtWarn for things that are extensions of some sort, but which one you use depends on how important the warning is. We've traditionally handled "use of whatever is a C++NN extension" warnings as ExtWarn whereas the "use of whatever is incompatible with C++ standards before C++NN" warnings are usually Extension.

[ken-matsui]

In D125178#3503617, @aaron.ballman wrote:

It looks like precommit CI caught some related failures:

Failed Tests (7):
  Clang :: Lexer/deprecate-macro.c
  Clang :: Lexer/unsafe-macro.c
  Clang :: Preprocessor/elifdef.c
  Clang :: Preprocessor/if_warning.c
  Clang :: Preprocessor/ifdef-recover.c
  Clang :: Preprocessor/macro_misc.c
  Clang :: Preprocessor/macro_vaopt_check.cpp

Ahhh, my bad. Thank you for your heads up!

clang/include/clang/Basic/DiagnosticLexKinds.td

696–698

In this case, compat_pp_directive for C++2b & C2x uses Warning, but why did you choose it rather than Extension?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticLexKinds.td

696–698

Oh shoot, I misspoke and said ExtWarn when I meant Warning that's default ignored. Sorry about that, and thank you for asking for clarification!

I picked that because it's the pattern used by many other diagnostic pairs:

def ext_inline_variable : ExtWarn<
  "inline variables are a C++17 extension">, InGroup<CXX17>;
def warn_cxx14_compat_inline_variable : Warning<
  "inline variables are incompatible with C++ standards before C++17">,
  DefaultIgnore, InGroup<CXXPre17Compat>;

def ext_for_range_begin_end_types_differ : ExtWarn<
  "'begin' and 'end' returning different types (%0 and %1) is a C++17 extension">,
  InGroup<CXX17>;
def warn_for_range_begin_end_types_differ : Warning<
  "'begin' and 'end' returning different types (%0 and %1) is incompatible "
  "with C++ standards before C++17">, InGroup<CXXPre17Compat>, DefaultIgnore;

def ext_constexpr_body_invalid_stmt : ExtWarn<
  "use of this statement in a constexpr %select{function|constructor}0 "
  "is a C++14 extension">, InGroup<CXX14>;
def warn_cxx11_compat_constexpr_body_invalid_stmt : Warning<
  "use of this statement in a constexpr %select{function|constructor}0 "
  "is incompatible with C++ standards before C++14">,
  InGroup<CXXPre14Compat>, DefaultIgnore;

def ext_constexpr_function_try_block_cxx20 : ExtWarn<
  "function try block in constexpr %select{function|constructor}0 is "
  "a C++20 extension">, InGroup<CXX20>;
def warn_cxx17_compat_constexpr_function_try_block : Warning<
  "function try block in constexpr %select{function|constructor}0 is "
  "incompatible with C++ standards before C++20">,
  InGroup<CXXPre20Compat>, DefaultIgnore;

[ken-matsui]

clang/include/clang/Basic/DiagnosticLexKinds.td

696–698

Oh, that makes sense. Thank you for your explanation!

[ken-matsui]

Fix failed tests

[aaron.ballman]

clang/include/clang/Basic/DiagnosticLexKinds.td

701

(You may also need to rewrap to 80 col limits.)

And do the same for the other three, so that you can use PPElifDiag for it.

clang/lib/Lex/PPDirectives.cpp

663

I like the way you're thinking about this, but I don't like how clever the code is. I'd rather update the comment on PPElifDiag to mention warn_*_compat_pp_directive and ext_*_pp_directive, and then update those diagnostics to have a bogus select slot for the PED_Elif that will never be used.

[ken-matsui]

Update the code as reviewed

[ken-matsui]

Thank you for your review! I updated the code.

[aaron.ballman]

LGTM aside from a super minor nit in a test file. Can you please add a release note for the new diagnostics as well?

I'm happy to land this for you when you're ready, but let me know which email address you'd like me to use this time around.

clang/test/Preprocessor/ifdef-recover.c

23–29 ↗

(On Diff #428636)

Just to keep comment styles the same.

[ken-matsui]

Updated the code as reviewed, added a release note, and merged ext-cpp2b-pp-directive.cpp & ext-c2x-pp-directive.c into ext-pp-directive.c.

[ken-matsui]

Thank you so much for your review!

My public email address is: [e-mail].

[aaron.ballman]

Oops, it looks like precommit CI found a relevant issue, can you fix it up?

Failed Tests (1):
  Clang :: Preprocessor/elifdef.c
 
Testing Time: 790.67s
  Skipped          :    56
  Unsupported      :  1997
  Passed           : 87720
  Expectedly Failed:   234
  Failed           :     1

[ken-matsui]

Fix the failed test

[ken-matsui]

I fixed it!

clang/include/clang/Basic/DiagnosticLexKinds.td

def err_pp_linemarker_invalid_flag : Error<
   "invalid flag line marker directive">;
 def err_pp_linemarker_invalid_pop : Error<
   "invalid line marker flag '2': cannot pop empty include stack">;
 def ext_pp_line_too_big : Extension<
   "C requires #line number to be less than %0, allowed as extension">;
 def warn_cxx98_compat_pp_line_too_big : Warning<
   "#line number greater than 32767 is incompatible with C++98">,
   InGroup<CXX98CompatPedantic>, DefaultIgnore;
+def ext_c2x_pp_directive : Extension<
+  "%select{#elifdef|#elifndef}0 is a C2x extension">,
+  InGroup<CPre2xCompatPedantic>;
 
 def err_pp_visibility_non_macro : Error<"no macro named %0">;
 
 def err_pp_arc_cf_code_audited_syntax : Error<"expected 'begin' or 'end'">;
 def err_pp_double_begin_of_arc_cf_code_audited : Error<
   "already inside '#pragma clang arc_cf_code_audited'">;
 def err_pp_unmatched_end_of_arc_cf_code_audited : Error<
   "not currently inside '#pragma clang arc_cf_code_audited'">;
 def err_pp_include_in_arc_cf_code_audited : Error<
   "cannot %select{#include files|import headers}0 "
   "inside '#pragma clang arc_cf_code_audited'">;

clang/lib/Lex/PPDirectives.cpp

if (Directive.startswith("if")) {
         // If this is a #elif with a #else before it, report the error.
         if (CondInfo.FoundElse)
           Diag(Tok, diag::pp_err_elif_after_else)
               << (IsElifDef ? PED_Elifdef : PED_Elifndef);
 
         // If this is in a skipping block or if we're already handled this #if
         // block, don't bother parsing the condition.
         if (CondInfo.WasSkipping || CondInfo.FoundNonSkip) {
           // FIXME: We should probably do at least some minimal parsing of the
           // condition to verify that it is well-formed. The current state
           // allows #elif* directives with completely malformed (or missing)
           // conditions.
           DiscardUntilEndOfDirective();
         } else {
           // Restore the value of LexingRawMode so that identifiers are
           // looked up, etc, inside the #elif[n]def expression.
           assert(CurPPLexer->LexingRawMode && "We have to be skipping here!");

 void Preprocessor::HandleElifFamilyDirective(Token &ElifToken,
                                              const Token &HashToken,
                                              tok::PPKeywordKind Kind) {
   PPElifDiag DirKind = Kind == tok::pp_elif      ? PED_Elif
                        : Kind == tok::pp_elifdef ? PED_Elifdef
                                                  : PED_Elifndef;
   ++NumElse;
 
+  // Warn if using `#elifdef` & `#elifndef` in not C2x mode.
+  switch (DirKind) {
+    case PED_Elifdef:
+    case PED_Elifndef:
+      if (!getLangOpts().C2x)
+        Diag(ElifToken, diag::ext_c2x_pp_directive) << DirKind - 1;
+      break;
+    default:
+      break;
+  }
+
   // #elif directive in a non-skipping conditional... start skipping.
   // We don't care what the condition is, because we will always skip it (since
   // the block immediately before it was included).
   SourceRange ConditionRange = DiscardUntilEndOfDirective();
 
   PPConditionalInfo CI;
   if (CurPPLexer->popConditionalLevel(CI)) {
     Diag(ElifToken, diag::pp_err_elif_without_if) << DirKind;

clang/test/Preprocessor/ext-c2x-pp-directive.c

+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify -pedantic %s
+// RUN: not %clang_cc1 -std=c99 -fsyntax-only -verify %s
+// RUN: not %clang_cc1 -std=c2x -fsyntax-only -verify -pedantic %s
+// RUN: not %clang_cc1 -std=c2x -fsyntax-only -verify %s
+
+#if 1
+#elifndef FOO // expected-warning {{#elifndef is a C2x extension}}
+#endif
+
+#if 1
+#elifdef BAR // expected-warning {{#elifdef is a C2x extension}}
+#endif
+
+// expected-warning {{ISO C requires a translation unit to contain at least one declaration}}