[Clang] Add warnings for CWG2521

[rZhBoYao]

Summary

1. Teach -Wuser-defined-literals to warn on using double underscores in literal suffix identifiers.
2. Add -Wdeprecated-literal-operator to warn about the use of the first grammar production of literal-operator-id, which defaults to off for now.

[rZhBoYao]

Few questions:
I think this applies to all the language modes?
Somehow, https://wg21.link/CWG2521 is not redirecting to https://cplusplus.github.io/CWG/issues/2521.html. Is this normal?

[Endill]

Thank you for working on this!

In D152632#4411587, @rZhBoYao wrote:

Few questions:
I think this applies to all the language modes?

Yes, we rarely limit the scope of DRs, and I'm looking forward to get rid of exceptions from this approach (grep "since" in cxx_dr_status.html).

Somehow, https://wg21.link/CWG2521 is not redirecting to https://cplusplus.github.io/CWG/issues/2521.html. Is this normal?

It happens, especially with recently updated DRs. I see it in CWG index, so you can ignore broken wg21.link.

clang/www/cxx_dr_status.html

14936

This file shouldn't be edited manually. You should write a test in clang/test/CXX/drs/dr25xx.cpp, and then run clang/www/make_cxx_dr_status script that updates cxx_dr_status.html.

[rZhBoYao]

Addressed comments.
Thanks for letting me know how to structure tests for DRs.

[Endill]

clang/test/CXX/drs/dr25xx.cpp

2 ↗

(On Diff #530292)

We avoid passing compiler options here, because it affects all the tests in the file. If you need to enable or disable a specific diagnostic, better wrap your test with

#pragma clang diagnostic push
#pragma clang diagnostic warning "-Wdeprecated-literal-operation" // or "ignored" instead of "warning"
// ...
#pragma clang diagnostic pop

That said, files containing newer DRs are not too well maintained at the moment (but I'll get to them at some point). This file in particular lacks RUN lines for all missing language modes, and -pedantic-errors. I guess the latter enables diagnostics that are needed for your test. You can use dr5xx.cpp as an example.

[rZhBoYao]

Overhaul for dr25xx.cpp.

For each test case, tried to support as many language modes as possible.
Not sure what those -triple x86_64-unknown-unknown are for? I leave them there nonetheless.

-Wdeprecated-literal-operator is under -Wdeprecated which is not under -Wpedantic so is not triggered by -pedantic-errors. Does this need change? I imagine it would be pretty disruptive. On the other hand, pedantic users might care about deprecation 🤔️.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

411

Oye, I'm of two minds about DefaultIgnore.

On the one hand, this is going to fire *a lot*: https://sourcegraph.com/search?q=context:global+operator%5B%5B:space:%5D%5D*%5C%22%5C%22%5B%5B:space:%5D%5D%5BA-Za-z0-9_%5D%2B&patternType=regexp&case=yes&sm=1&groupBy=repo

On the other hand, if we don't warn about it being deprecated, users won't know about it, which makes it harder to do anything about it the longer we silently allow it. (We have plenty of experience with this particular flavor of pain.)

I think we should warn about this by default. It's under its own warning group, so users who want to ignore the warning and live dangerously can do so. And it will be silenced in system headers automatically, so issuing the diagnostic should generally be actionable for users.

9268–9271

Hmmm, you've moved things in the right direction, but I think this should also live under the -Wreserved-identifier group given that it's about reserved identifiers.

clang/lib/Sema/SemaDeclCXX.cpp

16448

I think we should use IdentifierInfo::isReserved() to determine why it's reserved.

clang/lib/Sema/SemaExprCXX.cpp

512–517

Hmmm this means we're issuing two diagnostics -- missing an else here?

clang/test/CXX/drs/dr17xx.cpp

129

So we give an error if there's not a space, but then give a deprecation warning when there is a space?

[Endill]

Thank you for taking care of other tests under dr25xx.cpp!

In D152632#4412075, @rZhBoYao wrote:

Overhaul for dr25xx.cpp.

For each test case, tried to support as many language modes as possible.
Not sure what those -triple x86_64-unknown-unknown are for? I leave them there nonetheless.

They are usually used for codegen tests, so no need to specify the triple if you don't need to.

-Wdeprecated-literal-operator is under -Wdeprecated which is not under -Wpedantic so is not triggered by -pedantic-errors. Does this need change? I imagine it would be pretty disruptive. On the other hand, pedantic users might care about deprecation 🤔️.

I don't think we need to change status quo. In general I don't expect diagnostics grouping to care about DR tests, and I don't think it should.

clang/test/CXX/drs/dr25xx.cpp

71 ↗

(On Diff #530304)

Is it possible to put expected directive after the code, like we do in majority of existing tests? This means using only negative line offsets after @

[rZhBoYao]

clang/include/clang/Basic/DiagnosticSemaKinds.td

411

I'm thinking the same after seeing P2863R0 § 6.9

However, a lot of tests are written in the deprecated way. I think a patch removing all the whitespaces only preceding this one is needed, to not clutter up this one.

clang/lib/Sema/SemaDeclCXX.cpp

16448

Totally. But I think we need another function, maybe isReservedLiteralSuffixId, since the rule for literal suffix identifiers is very different from that for "identifiers appearing as a token or preprocessing-token".

clang/lib/Sema/SemaExprCXX.cpp

512–517

Does it have to be one or the other?

For example:

long double operator""      _KM(long double); // only warn reserved identifier

After replacing the identifier with km and recompile,
now there would be a new warning that was not previously shown,
which may be quite frustrating.

---

Oh wait, issuing two fixit hint seems to be very bad!

clang/test/CXX/drs/dr17xx.cpp

129

I found it very confusing when editing the next line. After some research, I think that came from C++11 over.literal. But the rule should have long been superseded by CWG1473 right? Since DRs are applied retroactively.

Additionally, GCC doesn't warn on this. Should we retire this error?

clang/test/CXX/drs/dr25xx.cpp

71 ↗

(On Diff #530304)

Will do

[rZhBoYao]

Depends on D153156

PS: I'm going on a vacation for 2 weeks. See you guys in July :)

[shafik]

I am wondering why we don't fold this into -Wreserved-identifier

clang/include/clang/Basic/IdentifierTable.h

56 ↗

(On Diff #532223)

I would think starting with a double underscore would also not be allowed, at least that is my reading.

[rZhBoYao]

Defaults to on for C++23 only. Enable for all the language modes in another patch.

[rZhBoYao]

In D152632#4443284, @shafik wrote:

I am wondering why we don't fold this into -Wreserved-identifier

To quota https://eel.is/c++draft/over.literal#1.sentence-2

The "ud-suffix" of the user-defined-string-literal or the identifier in a literal-operator-id is called a literal suffix identifier.

However "ud-suffix" and "identifiers appearing as a token or preprocessing-token" are treated differently in the standard, reserved-identifier-wise.
Thus, I don't think folding -Wuser-defined-literals into -Wreserved-identifier is a good idea.

clang/include/clang/Basic/DiagnosticSemaKinds.td

411

Default on and diagnose for C++23 only. Enable for all the lang modes in https://reviews.llvm.org/D153156

clang/include/clang/Basic/IdentifierTable.h

56 ↗

(On Diff #532223)

That is indeed the case.

[Endill]

DR testing side looks good, but you should wait for more approvals.
Thank you for addressing all the issues there!

[ldionne]

This breaks libc++: https://buildkite.com/llvm-project/clang-ci/builds/26#01894b7d-8d6f-4893-ab14-1147229390dc

Doesn't mean don't land it, but please try to address the issues in this patch to keep everyone green.

[aaron.ballman]

Mostly just minor nits from me, otherwise this is looking close to good! If you're able to handle the libc++ failures as part of this patch as well, that'd be great (but isn't required for landing the changes).

clang/docs/ReleaseNotes.rst

139–142

151–152

clang/include/clang/Basic/DiagnosticSemaKinds.td

410–411

clang/lib/Sema/SemaDeclCXX.cpp

16448–16449

Please spell out the types instead of using auto.

clang/lib/Sema/SemaExprCXX.cpp

525

Can you pass in II here instead of II->getName() or does that give bad behavior?

[rZhBoYao]

-Wdeprecated-literal-operator will be on by default in https://reviews.llvm.org/D153156
This one is now blocked by the libc++ fix: https://reviews.llvm.org/D155200

[aaron.ballman]

LGTM assuming precommit CI doesn't find any surprises.

[rZhBoYao]

Hmm… I don’t see why check-format keeps failing. I git clang-format thrice before uploading.
https://buildkite.com/llvm-project/premerge-checks/builds/164452

clang/docs/ReleaseNotes.rst

 C++2c Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 - Compiler flags ``-std=c++2c`` and ``-std=gnu++2c`` have been added for experimental C++2c implementation work.
 
 Resolutions to C++ Defect Reports
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Implemented `DR2397 <https://wg21.link/CWG2397>`_ which allows ``auto`` specifier for pointers
   and reference to arrays.
+- Implemented `CWG2521 <https://wg21.link/CWG2521>`_ which reserves using ``__`` in user defined literal suffixes
+  for C++ implementations, and deprecates literal operator function declarations using an identifier.
+  Taught ``-Wuser-defined-literals`` for the former, on by default, and added ``-Wdeprecated-literal-operator``
+  for the latter, off by default.
+
+  .. code-block:: c++
+
+    // What follows is warned by -Wuser-defined-literals
+    // albeit "ill-formed, no diagnostic required".
+    // Its behavior is undefined, [reserved.names.general]p2.
+    string operator ""__i18n(const char*, std::size_t);
+
+    // Let's assume this is not in the global namespace.
+    // -Wdeprecated-literal-operator whines about extra spaces.
+    string operator "" _i18n(const char*, std::size_t);
+    //                ^ an extra space
 
 C Language Changes
 ------------------
 - Support for outputs from asm goto statements along indirect edges has been
   added. (`#53562 <https://github.com/llvm/llvm-project/issues/53562>`_)
 - Fixed a bug that prevented initialization of an ``_Atomic``-qualified pointer
   from a null pointer constant.
 - Fixed a bug that prevented casting to an ``_Atomic``-qualified type.

clang/include/clang/Basic/DiagnosticGroups.td

 
 def DeprecatedArrayCompare : DiagGroup<"deprecated-array-compare">;
 def DeprecatedAttributes : DiagGroup<"deprecated-attributes">;
 def DeprecatedCommaSubscript : DiagGroup<"deprecated-comma-subscript">;
 def DeprecatedCopyWithUserProvidedCopy : DiagGroup<"deprecated-copy-with-user-provided-copy">;
 def DeprecatedCopyWithUserProvidedDtor : DiagGroup<"deprecated-copy-with-user-provided-dtor">;
 def DeprecatedCopy : DiagGroup<"deprecated-copy", [DeprecatedCopyWithUserProvidedCopy]>;
 def DeprecatedCopyWithDtor : DiagGroup<"deprecated-copy-with-dtor", [DeprecatedCopyWithUserProvidedDtor]>;
+def DeprecatedLiteralOperator : DiagGroup<"deprecated-literal-operator">;
 // For compatibility with GCC.
 def : DiagGroup<"deprecated-copy-dtor", [DeprecatedCopyWithDtor]>;
 def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
 def UnavailableDeclarations : DiagGroup<"unavailable-declarations">;
 def UnguardedAvailabilityNew : DiagGroup<"unguarded-availability-new">;
 def UnguardedAvailability : DiagGroup<"unguarded-availability",
                                       [UnguardedAvailabilityNew]>;
 // partial-availability is an alias of unguarded-availability.
def Deprecated : DiagGroup<"deprecated", [DeprecatedAnonEnumEnumConversion,
                                           DeprecatedDeclarations,
                                           DeprecatedDynamicExceptionSpec,
                                           DeprecatedEnumCompare,
                                           DeprecatedEnumCompareConditional,
                                           DeprecatedEnumEnumConversion,
                                           DeprecatedEnumFloatConversion,
                                           DeprecatedBuiltins,
                                           DeprecatedIncrementBool,
+                                          DeprecatedLiteralOperator,
                                           DeprecatedPragma,
                                           DeprecatedRegister,
                                           DeprecatedThisCapture,
                                           DeprecatedType,
                                           DeprecatedVolatile,
                                           DeprecatedWritableStr]>,
                  DiagCategory<"Deprecations">;
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_reserved_extern_symbol: Warning<
   "identifier %0 is reserved because %select{"
   "<ERROR>|" // ReservedIdentifierStatus::NotReserved
   "it starts with '_' at global scope|"
   "it starts with '_' and has C language linkage|"
   "it starts with '__'|"
   "it starts with '_' followed by a capital letter|"
   "it contains '__'}1">,
   InGroup<ReservedIdentifier>, DefaultIgnore;
+def warn_deprecated_literal_operator_id: Warning<
+  "identifier '%0' preceded by space(s) in the literal operator declaration "
+  "is deprecated">, InGroup<DeprecatedLiteralOperator>, DefaultIgnore;
 def warn_reserved_module_name : Warning<
   "%0 is a reserved name for a module">, InGroup<ReservedModuleIdentifier>;
 
 def warn_parameter_size: Warning<
   "%0 is a large (%1 bytes) pass-by-value argument; "
   "pass it by reference instead ?">, InGroup<LargeByValueCopy>;
 def warn_return_value_size: Warning<
   "return value of %0 is a large (%1 bytes) pass-by-value object; "
def err_literal_operator_template_with_params : Error<
   "literal operator template cannot have any parameters">;
 def err_literal_operator_template : Error<
   "template parameter list for literal operator must be either 'char...' or 'typename T, T...'">;
 def err_literal_operator_extern_c : Error<
   "literal operator must have C++ linkage">;
 def ext_string_literal_operator_template : ExtWarn<
   "string literal operator templates are a GNU extension">,
   InGroup<GNUStringLiteralOperatorTemplate>;
 def warn_user_literal_reserved : Warning<
-  "user-defined literal suffixes not starting with '_' are reserved"
+  "user-defined literal suffixes containing '__' or not starting with '_' are reserved"
   "%select{; no literal will invoke this operator|}0">,
   InGroup<UserDefinedLiterals>;
 
 // C++ conversion functions
 def err_conv_function_not_member : Error<
   "conversion function must be a non-static member function">;
 def err_conv_function_return_type : Error<
   "conversion function cannot have a return type">;
 def err_conv_function_with_params : Error<
   "conversion function cannot have any parameters">;

clang/lib/Sema/SemaDeclCXX.cpp

if (Param->hasDefaultArg()) {
            diag::err_literal_operator_default_argument)
         << Param->getDefaultArgRange();
       break;
     }
   }
 
   StringRef LiteralName
     = FnDecl->getDeclName().getCXXLiteralIdentifier()->getName();
-  if (LiteralName[0] != '_' &&
+  if ((LiteralName[0] != '_' || LiteralName.contains("__")) &&
       !getSourceManager().isInSystemHeader(FnDecl->getLocation())) {
-    // C++11 [usrlit.suffix]p1:
-    //   Literal suffix identifiers that do not start with an underscore
-    //   are reserved for future standardization.
+    // C++23 [usrlit.suffix]p1:
+    //   Literal suffix identifiers that do not start with an underscore are
+    //   reserved for future standardization. Literal suffix identifiers that
+    //   contain a double underscore __ are reserved for use by C++
+    //   implementations.
     Diag(FnDecl->getLocation(), diag::warn_user_literal_reserved)
       << StringLiteralParser::isValidUDSuffix(getLangOpts(), LiteralName);
   }
 
   return false;
 }
 
 /// ActOnStartLinkageSpecification - Parsed the beginning of a C++

clang/lib/Sema/SemaExprCXX.cpp

bool Sema::checkLiteralOperatorId(const CXXScopeSpec &SS,
   if (!IsUDSuffix) {
     // [over.literal] p8
     //
     // double operator""_Bq(long double);  // OK: not a reserved identifier
     // double operator"" _Bq(long double); // ill-formed, no diagnostic required
     IdentifierInfo *II = Name.Identifier;
     ReservedIdentifierStatus Status = II->isReserved(PP.getLangOpts());
     SourceLocation Loc = Name.getEndLoc();
-    if (isReservedInAllContexts(Status) &&
-        !PP.getSourceManager().isInSystemHeader(Loc)) {
+    if (!PP.getSourceManager().isInSystemHeader(Loc)) {
+      if (isReservedInAllContexts(Status)) {
         Diag(Loc, diag::warn_reserved_extern_symbol)
             << II << static_cast<int>(Status)
             << FixItHint::CreateReplacement(
                    Name.getSourceRange(),
                    (StringRef("operator\"\"") + II->getName()).str());
       }
+      Diag(Loc, diag::warn_deprecated_literal_operator_id)
+          << II->getName()
+          << FixItHint::CreateReplacement(
+                 Name.getSourceRange(),
+                 (StringRef("operator\"\"") + II->getName()).str());
+    }
   }
 
   if (!SS.isValid())
     return false;
 
   switch (SS.getScopeRep()->getKind()) {
   case NestedNameSpecifier::Identifier:
   case NestedNameSpecifier::TypeSpec:
   case NestedNameSpecifier::TypeSpecWithTemplate:
     // Per C++11 [over.literal]p2, literal operators can only be declared at
     // namespace scope. Therefore, this unqualified-id cannot name anything.
     // Reject it early, because we have no AST representation for this in the
     // case where the scope is dependent.
     Diag(Name.getBeginLoc(), diag::err_literal_operator_id_outside_namespace)
         << SS.getScopeRep();
     return true;

clang/test/CXX/drs/dr17xx.cpp

#if __cplusplus >= 201103L
   } b;
   A a{b};
 #endif
 }
 
 namespace dr1762 { // dr1762: 14
 #if __cplusplus >= 201103L
   float operator ""_E(const char *);
   // expected-error@+2 {{invalid suffix on literal; C++11 requires a space between literal and identifier}}
-  // expected-warning@+1 {{user-defined literal suffixes not starting with '_' are reserved; no literal will invoke this operator}}
+  // expected-warning@+1 {{user-defined literal suffixes containing '__' or not starting with '_' are reserved; no literal will invoke this operator}}
   float operator ""E(const char *);
 #endif
 }
 
 namespace dr1778 { // dr1778: 9
   // Superseded by P1286R2.
 #if __cplusplus >= 201103L
   struct A { A() noexcept(true) = default; };

clang/test/CXX/lex/lex.literal/lex.ext/p1.cpp

 // RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s
 
-void operator "" p31(long double); // expected-warning{{user-defined literal suffixes not starting with '_' are reserved}}
+void operator "" p31(long double); // expected-warning{{user-defined literal suffixes containing '__' or not starting with '_' are reserved}}
 void operator "" _p31(long double);
-long double operator "" pi(long double); // expected-warning{{user-defined literal suffixes not starting with '_' are reserved}}
+long double operator "" pi(long double); // expected-warning{{user-defined literal suffixes containing '__' or not starting with '_' are reserved}}
 
 float hexfloat = 0x1p31; // allow hexfloats

clang/test/CXX/lex/lex.literal/lex.ext/p10.cpp

 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 using size_t = decltype(sizeof(int));
-void operator "" wibble(const char *); // expected-warning {{user-defined literal suffixes not starting with '_' are reserved; no literal will invoke this operator}}
-void operator "" wibble(const char *, size_t); // expected-warning {{user-defined literal suffixes not starting with '_' are reserved; no literal will invoke this operator}}
+void operator "" wibble(const char *); // expected-warning {{user-defined literal suffixes containing '__' or not starting with '_' are reserved; no literal will invoke this operator}}
+void operator "" wibble(const char *, size_t); // expected-warning {{user-defined literal suffixes containing '__' or not starting with '_' are reserved; no literal will invoke this operator}}
 
 template<typename T>
 void f() {
   // A program containing a reserved ud-suffix is ill-formed.
   123wibble; // expected-error {{invalid suffix 'wibble'}}
   123.0wibble; // expected-error {{invalid suffix 'wibble'}}
   const char *p = ""wibble; // expected-error {{invalid suffix on literal; C++11 requires a space between literal and identifier}} expected-error {{expected ';'}}
   const char *q = R"x("hello")x"wibble; // expected-error {{invalid suffix on literal; C++11 requires a space between literal and identifier}} expected-error {{expected ';'}}
 }

clang/test/Parser/cxx0x-literal-operators.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
 
 void operator "" (const char *); // expected-error {{expected identifier}}
 void operator "k" foo(const char *); // \
   expected-error {{string literal after 'operator' must be '""'}} \
-  expected-warning{{user-defined literal suffixes not starting with '_' are reserved}}
+  expected-warning{{user-defined literal suffixes containing '__' or not starting with '_' are reserved}}
 void operator "" tester (const char *); // \
-  expected-warning{{user-defined literal suffixes not starting with '_' are reserved}}
+  expected-warning{{user-defined literal suffixes containing '__' or not starting with '_' are reserved}}

clang/test/SemaCXX/deprecated-literal-operator.cpp

+// RUN: %clang_cc1 -std=c++23 %s -verify -Wdeprecated-literal-operator
+// RUN: %clang_cc1 -std=c++23 %s -verify -Wdeprecated
+
+namespace depr {
+
+decltype(sizeof 0) operator ""  _\u{3C0}___(long double); // \
+expected-warning{{identifier '_π___' preceded by space(s) in the literal operator declaration is deprecated}} \
+expected-warning{{user-defined literal suffixes containing '__' or not starting with '_' are reserved}}
+
+template <char... Chars> consteval auto
+operator""       _div() { return (... / (Chars - '0')); } // OK, deprecated \
+expected-warning{{identifier '_div' preceded by space(s) in the literal operator declaration is deprecated}}
+
+} // namespace depr
+
+using depr::operator"" _\u{3C0}___, depr::operator""_div; // \
+expected-warning{{identifier '_π___' preceded by space(s) in the literal operator declaration is deprecated}}
+
+[[maybe_unused]] constexpr auto div = 841_div;

clang/test/SemaCXX/no-warn-user-defined-literals-in-system-headers.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wsystem-headers -isystem %S %s
 
 #include <no-warn-user-defined-literals-in-system-headers.h>
 
-void operator "" bar(long double); // expected-warning{{user-defined literal suffixes not starting with '_' are reserved}}
+void operator "" bar(long double); // expected-warning{{user-defined literal suffixes containing '__' or not starting with '_' are reserved}}

clang/www/cxx_dr_status.html

<tr id="2520">
     <td>DR</td>
     <td>Template signature and default template arguments</td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="2521">
     <td><a href="https://cplusplus.github.io/CWG/issues/2521.html">2521</a></td>
     <td>DR</td>
     <td>User-defined literals and reserved identifiers</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="unreleased" align="center">Clang 17</td>
   </tr>
   <tr class="open" id="2522">
     <td><a href="https://cplusplus.github.io/CWG/issues/2522.html">2522</a></td>
     <td>open</td>
     <td>Removing placemarker tokens and retention of whitespace</td>
     <td align="center">Not resolved</td>
   </tr>
   <tr id="2523">